Merge ~mthaddon/charm-k8s-ingress/+git/charm-k8s-ingress:config-changed-unittest into charm-k8s-ingress:master

[Tom Haddon]

Remove fake config item, update unit tests, refactor how we generate k8s api calls for easier testing

Remove fake config item, update unit tests, refactor how we generate k8s api calls for easier testing

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

FAILED: Continuous integration, rev:f6938cd2024c1d2412fdce4268c1941715ca8af8
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/1/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/is/job/lp-charm-test/31/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/49054/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/1//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

PASSED: Continuous integration, rev:7fea2aecaf46a1927e822172ef888f6128b0ab1e
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/2/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/is/job/lp-charm-test/32/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/49069/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/2//rebuild

review: Approve (continuous-integration)

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-is (prod-jenkaas-is)]

PASSED: Continuous integration, rev:4a62f3ec695281044454c9fd22686b53728ec76b
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/3/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/is/job/lp-charm-test/33/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/49105/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/3//rebuild

review: Approve (continuous-integration)

[Jon Seager (jnsgruk)]

Looks good to me - nice progress! :)

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision f5ac67a015852f341d978192f4b3efc78bcaa6e9

diff --git a/Makefile b/Makefile
index a3166a1..2d7e3ee 100644
--- a/Makefile
+++ b/Makefile
@@ -17,7 +17,7 @@ clean:
 	@echo "Cleaning files"
 	@git clean -fXd
 
-mattermost.charm: src/*.py requirements.txt
+k8s-ingress.charm: src/*.py requirements.txt
 	charmcraft build
 
 .PHONY: lint test unittest clean
diff --git a/config.yaml b/config.yaml
index b2bd3c8..fe2691d 100644
--- a/config.yaml
+++ b/config.yaml
@@ -21,7 +21,3 @@ options:
     default: 0
     description: The port of the service to create an ingress for.
     type: int
-  thing:
-    default: 🎁
-    description: A thing used by the charm.
-    type: string
diff --git a/metadata.yaml b/metadata.yaml
index 6b4c014..f1a62fc 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -5,11 +5,8 @@ description: |
   An operator to configure a kubernetes ingress.
 summary: |
   An operator to configure a kubernetes ingress.
-min-juju-version: 2.9.0
-platforms:
-  - kubernetes
-systems:
-  - os: ubuntu
+bases:
+  - name: ubuntu
     channel: 20.04/stable
 
 containers:
diff --git a/src/charm.py b/src/charm.py
index 4b85b26..3bd55ff 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -21,7 +21,6 @@ import kubernetes
 from ops.charm import CharmBase
 from ops.main import main
 from ops.model import ActiveStatus
-from ops.framework import StoredState
 
 logger = logging.getLogger(__name__)
 
@@ -49,12 +48,22 @@ class CharmK8SIngressCharm(CharmBase):
     """Charm the service."""
 
     _authed = False
-    _stored = StoredState()
 
     def __init__(self, *args):
         super().__init__(*args)
         self.framework.observe(self.on.config_changed, self._on_config_changed)
-        self._stored.set_default(things=[])
+
+    @property
+    def _ingress_name(self):
+        """Return an ingress name for use creating a k8s ingress."""
+        # Follow the same naming convention as Juju.
+        return "{}-ingress".format(self.config["service-name"])
+
+    @property
+    def _service_name(self):
+        """Return a service name for the use creating a k8s service."""
+        # Avoid collision with service name created by Juju.
+        return "{}-service".format(self.config["service-name"])
 
     def k8s_auth(self):
         """Authenticate to kubernetes."""
@@ -73,19 +82,12 @@ class CharmK8SIngressCharm(CharmBase):
 
         self._authed = True
 
-    def _define_service(self):
-        """Create or update a service in kubernetes."""
-        self.k8s_auth()
-        api = _core_v1_api()
-
-        # Juju will create a service named the same as the application. We
-        # need to use a different name to avoid collision.
-        service_name = "{}-service".format(self.config["service-name"])
-
-        body = kubernetes.client.V1Service(
+    def _get_k8s_service(self):
+        """Get a K8s service definition."""
+        return kubernetes.client.V1Service(
             api_version="v1",
             kind="Service",
-            metadata=kubernetes.client.V1ObjectMeta(name=service_name),
+            metadata=kubernetes.client.V1ObjectMeta(name=self._service_name),
             spec=kubernetes.client.V1ServiceSpec(
                 selector={"app.kubernetes.io/name": self.config["service-name"]},
                 ports=[
@@ -97,8 +99,53 @@ class CharmK8SIngressCharm(CharmBase):
                 ],
             ),
         )
+
+    def _get_k8s_ingress(self):
+        """Get a K8s ingress definition."""
+        return kubernetes.client.NetworkingV1beta1Ingress(
+            api_version="networking.k8s.io/v1beta1",
+            kind="Ingress",
+            metadata=kubernetes.client.V1ObjectMeta(
+                name=self._ingress_name,
+                annotations={
+                    "nginx.ingress.kubernetes.io/rewrite-target": "/",
+                    "nginx.ingress.kubernetes.io/ssl-redirect": "false",
+                },
+            ),
+            spec=kubernetes.client.NetworkingV1beta1IngressSpec(
+                rules=[
+                    kubernetes.client.NetworkingV1beta1IngressRule(
+                        host=self.config["service-hostname"],
+                        http=kubernetes.client.NetworkingV1beta1HTTPIngressRuleValue(
+                            paths=[
+                                kubernetes.client.NetworkingV1beta1HTTPIngressPath(
+                                    path="/",
+                                    backend=kubernetes.client.NetworkingV1beta1IngressBackend(
+                                        service_port=self.config["service-port"],
+                                        service_name=self._service_name,
+                                    ),
+                                )
+                            ]
+                        ),
+                    )
+                ]
+            ),
+        )
+
+    def _report_service_ips(self):
+        """Report on service IP(s)."""
+        self.k8s_auth()
+        api = _core_v1_api()
+        services = api.list_namespaced_service(namespace=self.config["service-namespace"])
+        return [x.spec.cluster_ip for x in services.items if x.metadata.name == self._service_name]
+
+    def _define_service(self):
+        """Create or update a service in kubernetes."""
+        self.k8s_auth()
+        api = _core_v1_api()
+        body = self._get_k8s_service()
         services = api.list_namespaced_service(namespace=self.config["service-namespace"])
-        if service_name in [x.metadata.name for x in services.items]:
+        if self._service_name in [x.metadata.name for x in services.items]:
             # Currently failing with port[1].name required but we're only
             # defining one port above...
             # api.patch_namespaced_service(
@@ -107,7 +154,7 @@ class CharmK8SIngressCharm(CharmBase):
             #    body=body,
             # )
             api.delete_namespaced_service(
-                name=service_name,
+                name=self._service_name,
                 namespace=self.config["service-namespace"],
             )
             api.create_namespaced_service(
@@ -134,43 +181,11 @@ class CharmK8SIngressCharm(CharmBase):
         """Create or update an ingress in kubernetes."""
         self.k8s_auth()
         api = _networking_v1_beta1_api()
-
-        # Follow the same naming convention as Juju.
-        ingress_name = "{}-ingress".format(self.config["service-name"])
-
-        body = kubernetes.client.NetworkingV1beta1Ingress(
-            api_version="networking.k8s.io/v1beta1",
-            kind="Ingress",
-            metadata=kubernetes.client.V1ObjectMeta(
-                name=ingress_name,
-                annotations={
-                    "nginx.ingress.kubernetes.io/rewrite-target": "/",
-                    "nginx.ingress.kubernetes.io/ssl-redirect": "false",
-                },
-            ),
-            spec=kubernetes.client.NetworkingV1beta1IngressSpec(
-                rules=[
-                    kubernetes.client.NetworkingV1beta1IngressRule(
-                        host=self.config["service-hostname"],
-                        http=kubernetes.client.NetworkingV1beta1HTTPIngressRuleValue(
-                            paths=[
-                                kubernetes.client.NetworkingV1beta1HTTPIngressPath(
-                                    path="/",
-                                    backend=kubernetes.client.NetworkingV1beta1IngressBackend(
-                                        service_port=self.config["service-port"],
-                                        service_name="{}-service".format(self.config["service-name"]),
-                                    ),
-                                )
-                            ]
-                        ),
-                    )
-                ]
-            ),
-        )
+        body = self._get_k8s_ingress()
         ingresses = api.list_namespaced_ingress(namespace=self.config["service-namespace"])
-        if ingress_name in [x.metadata.name for x in ingresses.items]:
+        if self._ingress_name in [x.metadata.name for x in ingresses.items]:
             api.patch_namespaced_ingress(
-                name=ingress_name,
+                name=self._ingress_name,
                 namespace=self.config["service-namespace"],
                 body=body,
             )
@@ -192,14 +207,14 @@ class CharmK8SIngressCharm(CharmBase):
 
     def _on_config_changed(self, _):
         """Handle the config changed event."""
-        current = self.config["thing"]
-        if current not in self._stored.things:
-            logger.debug("found a new thing: %r", current)
-            self._stored.things.append(current)
+        msg = ""
         if self.config["service-name"]:
             self._define_service()
             self._define_ingress()
-        self.unit.status = ActiveStatus()
+            # It's not recommended to do this via ActiveStatus, but we don't
+            # have another way of reporting status yet.
+            msg = "Ingress with service IP(s): {}".format(", ".join(self._report_service_ips()))
+        self.unit.status = ActiveStatus(msg)
 
 
 if __name__ == "__main__":
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 861051d..2bca57e 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -4,17 +4,93 @@
 import mock
 import unittest
 
+import kubernetes
+
+from ops.model import ActiveStatus
 from ops.testing import Harness
 from charm import CharmK8SIngressCharm
 
 
 class TestCharm(unittest.TestCase):
+    def setUp(self):
+        """Setup the harness object."""
+        self.harness = Harness(CharmK8SIngressCharm)
+        self.addCleanup(self.harness.cleanup)
+        self.harness.begin()
+
+    @mock.patch('charm.CharmK8SIngressCharm._report_service_ips')
     @mock.patch('charm.CharmK8SIngressCharm._define_ingress')
     @mock.patch('charm.CharmK8SIngressCharm._define_service')
-    def test_config_changed(self, _define_service, _define_ingress):
-        harness = Harness(CharmK8SIngressCharm)
-        self.addCleanup(harness.cleanup)
-        harness.begin()
-        self.assertEqual(list(harness.charm._stored.things), [])
-        harness.update_config({"thing": "foo"})
-        self.assertEqual(list(harness.charm._stored.things), ["foo"])
+    def test_config_changed(self, _define_service, _define_ingress, _report_service_ips):
+        """Test our config changed handler."""
+        _report_service_ips.return_value = ["10.0.1.12"]
+        # Confirm our _define_ingress and _define_service methods haven't been called.
+        self.assertEqual(_define_ingress.call_count, 0)
+        self.assertEqual(_define_service.call_count, 0)
+        # Test if config-changed is called with service-name empty, our methods still
+        # aren't called.
+        self.harness.update_config({"service-name": ""})
+        self.assertEqual(_define_ingress.call_count, 0)
+        self.assertEqual(_define_service.call_count, 0)
+        # And now test if we set a service-name config, our methods are called.
+        self.harness.update_config({"service-name": "gunicorn"})
+        self.assertEqual(_define_ingress.call_count, 1)
+        self.assertEqual(_define_service.call_count, 1)
+        # Confirm status is as expected.
+        self.assertEqual(self.harness.charm.unit.status, ActiveStatus('Ingress with service IP(s): 10.0.1.12'))
+
+    def test_get_k8s_ingress(self):
+        """Test getting our definition of a k8s ingress."""
+        self.harness.disable_hooks()
+        self.harness.update_config({"service-name": "gunicorn", "service-port": 80, "service-hostname": "foo.internal"})
+        expected = kubernetes.client.NetworkingV1beta1Ingress(
+            api_version="networking.k8s.io/v1beta1",
+            kind="Ingress",
+            metadata=kubernetes.client.V1ObjectMeta(
+                name="gunicorn-ingress",
+                annotations={
+                    "nginx.ingress.kubernetes.io/rewrite-target": "/",
+                    "nginx.ingress.kubernetes.io/ssl-redirect": "false",
+                },
+            ),
+            spec=kubernetes.client.NetworkingV1beta1IngressSpec(
+                rules=[
+                    kubernetes.client.NetworkingV1beta1IngressRule(
+                        host="foo.internal",
+                        http=kubernetes.client.NetworkingV1beta1HTTPIngressRuleValue(
+                            paths=[
+                                kubernetes.client.NetworkingV1beta1HTTPIngressPath(
+                                    path="/",
+                                    backend=kubernetes.client.NetworkingV1beta1IngressBackend(
+                                        service_port=80,
+                                        service_name="gunicorn-service",
+                                    ),
+                                )
+                            ]
+                        ),
+                    )
+                ]
+            ),
+        )
+        self.assertEqual(self.harness.charm._get_k8s_ingress(), expected)
+
+    def test_get_k8s_service(self):
+        """Test getting our definition of a k8s service."""
+        self.harness.disable_hooks()
+        self.harness.update_config({"service-name": "gunicorn", "service-port": 80})
+        expected = kubernetes.client.V1Service(
+            api_version="v1",
+            kind="Service",
+            metadata=kubernetes.client.V1ObjectMeta(name="gunicorn-service"),
+            spec=kubernetes.client.V1ServiceSpec(
+                selector={"app.kubernetes.io/name": "gunicorn"},
+                ports=[
+                    kubernetes.client.V1ServicePort(
+                        name="tcp-80",
+                        port=80,
+                        target_port=80,
+                    )
+                ],
+            ),
+        )
+        self.assertEqual(self.harness.charm._get_k8s_service(), expected)