Merge ~mthaddon/charm-k8s-ingress/+git/charm-k8s-ingress:ingress-library into charm-k8s-ingress:master

[Tom Haddon]

Create a library for handling the ingress relation

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[🤖 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:8837b9753918e265d6a36a8d5ca83242aa19ccc2
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/27/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/is/job/lp-charm-test/59/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/65332/

Click here to trigger a rebuild:
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/27//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:2127ca8c1738d2bd88ad00e97951f33833dd92b6
https://jenkins.canonical.com/is/job/lp-charm-k8s-ingress-ci/28/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/is/job/lp-charm-test/60/
    None: https://jenkins.canonical.com/is/job/lp-update-mp/73324/

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

review: Approve (continuous-integration)

[Stuart Bishop (stub)]

It would be possible to avoid needing to pass in any parameters to the IngressRequires.update method. The charm would pass in the correct parameters to the IngressRequires constructor, and update() would simply apply them to the relation. As it stands, the charm could pass in different settings and we end up with the object state not matching what is on the relation (but the charm shouldn't be doing that).

Now I think of it, you might be able to avoid the update method and needing a config-changed handler entirely. The IngressRequires.__init__ method could detect that its settings have changed (using state), and do the update there. However, I'm not entirely sure you can get away with it, since at this point the charm object is still being constructed and the operator framework might not be initialized enough.

The code is all looking great, and this can be landed. If you agree with the above, they can be addressed in a followup branch since this is not yet a published stable API.

review: Approve

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

Change successfully merged at revision 39fcba8217e5fb34c862e31a8ef12e772a83594f

diff --git a/README.md b/README.md
index f98aad4..55b9e33 100644
--- a/README.md
+++ b/README.md
@@ -40,18 +40,15 @@ juju relate ingress gunicorn
 This will create an K8s ingress called `gunicorn-ingress` and a K8s service
 called `gunicorn-service`. The gunicorn charm in question, which can be found
 https://code.launchpad.net/~mthaddon/charm-k8s-gunicorn/+git/charm-k8s-gunicorn/+ref/pebble
-implements the relation as follows, as a trivial example:
+implements the relation using the ingress library, as a trivial example:
 ```
+from charms.ingress.v0.ingress import IngressRequires
+
 # In __init__:
-self.framework.observe(self.on['ingress'].relation_changed, self._on_ingress_changed)
+self.ingress = IngressRequires(self, self.config["external_hostname"], self.app.name, 80)
 
-# And the _on_ingress_changed method.
-def _on_ingress_changed(self, event: ops.framework.EventBase) -> None:
-    """Handle the ingress relation changed event."""
-    if self.unit.is_leader():
-        event.relation.data[self.app]["service-hostname"] = self.config["external_hostname"]
-        event.relation.data[self.app]["service-name"] = self.app.name
-        event.relation.data[self.app]["service-port"] = "80"
+# In config-changed handler
+self.ingress.update_config({"service_hostname": self.config["external_hostname"]})
 ```
 All of the config items in `config.yaml` with the exception of `kube-config` can
 be set via the relation, e.g. `tls-secret-name` or `max-body-size`.
diff --git a/lib/charms/ingress/v0/ingress.py b/lib/charms/ingress/v0/ingress.py
new file mode 100644
index 0000000..f4f2f27
--- /dev/null
+++ b/lib/charms/ingress/v0/ingress.py
@@ -0,0 +1,163 @@
+"""Library for the ingress relation.
+
+This library contains the Requires and Provides classes for handling
+the ingress interface.
+
+Import `IngressRequires` in your charm, with required options:
+    - "self" (the charm itself)
+    - service_hostname
+    - service_name
+    - service_port
+Optionally you can also pass:
+    - max_body_size
+    - service_namespace
+    - session_cookie_max_age
+    - tls_secret_name
+
+As an example:
+```
+from charms.ingress.v0.ingress import IngressRequires
+
+# In your charm's `__init__` method.
+self.ingress = IngressRequires(self, self.config["external_hostname"], self.app.name, 80)
+
+# In your charm's `config-changed` handler.
+self.ingress.update_config({"service_hostname": self.config["external_hostname"]})
+```
+"""
+
+import logging
+
+from ops.framework import EventBase, Object
+from ops.model import BlockedStatus
+
+# The unique Charmhub library identifier, never change it
+LIBID = "2d35a009b0d64fe186c99a8c9e53c6ab"
+
+# Increment this major API version when introducing breaking changes
+LIBAPI = 0
+
+# Increment this PATCH version before using `charmcraft push-lib` or reset
+# to 0 if you are raising the major API version
+LIBPATCH = 4
+
+logger = logging.getLogger(__name__)
+
+REQUIRED_INGRESS_RELATION_FIELDS = {
+    "service-hostname",
+    "service-name",
+    "service-port",
+}
+
+OPTIONAL_INGRESS_RELATION_FIELDS = {
+    "max-body-size",
+    "service-namespace",
+    "session-cookie-max-age",
+    "tls-secret-name",
+}
+
+
+class IngressAvailableEvent(EventBase):
+    pass
+
+
+class IngressRequires(Object):
+    """This class defines the functionality for the 'requires' side of the 'ingress' relation.
+
+    Hook events observed:
+        - relation-changed
+    """
+
+    def __init__(
+        self,
+        charm,
+        service_hostname,
+        service_name,
+        service_port,
+        *,
+        max_body_size=0,
+        service_namespace="",
+        session_cookie_max_age=0,
+        tls_secret_name=""
+    ):
+        super().__init__(charm, "ingress")
+
+        self.framework.observe(charm.on["ingress"].relation_changed, self._on_relation_changed)
+        self.charm = charm
+
+        # Ingress properties - Required.
+        self.service_hostname = service_hostname
+        self.service_name = service_name
+        self.service_port = service_port
+
+        # Ingress properties - Optional.
+        self.max_body_size = max_body_size
+        self.service_namespace = service_namespace
+        self.session_cookie_max_age = session_cookie_max_age
+        self.tls_secret_name = tls_secret_name
+
+    def _on_relation_changed(self, event):
+        """Handle the relation-changed event."""
+        # `self.unit` isn't available here, so use `self.model.unit`.
+        if self.model.unit.is_leader():
+            # Required.
+            event.relation.data[self.model.app]["service-hostname"] = self.service_hostname
+            event.relation.data[self.model.app]["service-name"] = self.service_name
+            event.relation.data[self.model.app]["service-port"] = str(self.service_port)
+            # Optional.
+            if self.max_body_size:
+                event.relation.data[self.model.app]["max-body-size"] = str(self.max_body_size)
+            if self.service_namespace:
+                event.relation.data[self.model.app]["service-namespace"] = self.service_namespace
+            if self.session_cookie_max_age:
+                event.relation.data[self.model.app]["session-cookie-max-age"] = self.session_cookie_max_age
+            if self.tls_secret_name:
+                event.relation.data[self.model.app]["tls-secret-name"] = self.tls_secret_name
+
+    def update_config(self, config_dict):
+        """Allow for updates to relation."""
+        if self.model.unit.is_leader():
+            relation = self.model.get_relation("ingress")
+            if relation:
+                for key in config_dict:
+                    relation.data[self.model.app][key.replace("_", "-")] = config_dict[key]
+
+
+class IngressProvides(Object):
+    """This class defines the functionality for the 'provides' side of the 'ingress' relation.
+
+    Hook events observed:
+        - relation-changed
+    """
+
+    def __init__(self, charm):
+        super().__init__(charm, "ingress")
+        # Observe the relation-changed hook event and bind
+        # self.on_relation_changed() to handle the event.
+        self.framework.observe(charm.on["ingress"].relation_changed, self._on_relation_changed)
+        self.charm = charm
+
+    def _on_relation_changed(self, event):
+        """Handle a change to the ingress relation.
+
+        Confirm we have the fields we expect to receive."""
+        # `self.unit` isn't available here, so use `self.model.unit`.
+        if not self.model.unit.is_leader():
+            return
+
+        ingress_data = {
+            field: event.relation.data[event.app].get(field)
+            for field in REQUIRED_INGRESS_RELATION_FIELDS | OPTIONAL_INGRESS_RELATION_FIELDS
+        }
+
+        missing_fields = sorted(
+            [field for field in REQUIRED_INGRESS_RELATION_FIELDS if ingress_data.get(field) is None]
+        )
+
+        if missing_fields:
+            logger.error("Missing required data fields for ingress relation: {}".format(", ".join(missing_fields)))
+            self.model.unit.status = BlockedStatus("Missing fields for ingress: {}".format(", ".join(missing_fields)))
+
+        # Create an event that our charm can use to decide it's okay to
+        # configure the ingress.
+        self.charm.on.ingress_available.emit()
diff --git a/metadata.yaml b/metadata.yaml
index b6d24d1..34313d9 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -18,7 +18,7 @@ resources:
     type: oci-image
     description: A placeholder image, operator code won't do anything with it.
 
-requires:
+provides:
   ingress:
     interface: ingress
     limit: 1
diff --git a/src/charm.py b/src/charm.py
index f2aaaea..c7cf6be 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -8,29 +8,18 @@ from pathlib import Path
 
 import kubernetes
 
-from ops.charm import CharmBase
-from ops.main import main
-from ops.model import (
-    ActiveStatus,
-    BlockedStatus,
+from charms.ingress.v0.ingress import (
+    IngressAvailableEvent,
+    IngressProvides,
 )
+from ops.charm import CharmBase, CharmEvents
+from ops.framework import EventSource
+from ops.main import main
+from ops.model import ActiveStatus
 
 
 logger = logging.getLogger(__name__)
 
-REQUIRED_INGRESS_RELATION_FIELDS = {
-    "service-hostname",
-    "service-name",
-    "service-port",
-}
-
-OPTIONAL_INGRESS_RELATION_FIELDS = {
-    "max-body-size",
-    "service-namespace",
-    "session-cookie-max-age",
-    "tls-secret-name",
-}
-
 
 def _core_v1_api():
     """Use the v1 k8s API."""
@@ -51,37 +40,26 @@ def _fix_lp_1892255():
     )
 
 
+class IngressCharmEvents(CharmEvents):
+    """Custom charm events."""
+
+    ingress_available = EventSource(IngressAvailableEvent)
+
+
 class IngressCharm(CharmBase):
     """Charm the service."""
 
     _authed = False
+    on = IngressCharmEvents()
 
     def __init__(self, *args):
         super().__init__(*args)
         self.framework.observe(self.on.config_changed, self._on_config_changed)
 
-        # ingress relation handling.
-        self.framework.observe(self.on["ingress"].relation_changed, self._on_ingress_relation_changed)
-
-    def _on_ingress_relation_changed(self, event):
-        """Handle a change to the ingress relation.
-
-        Confirm we have the fields we expect to receive."""
-        if not self.unit.is_leader():
-            return
-
-        ingress_data = {
-            field: event.relation.data[event.app].get(field)
-            for field in REQUIRED_INGRESS_RELATION_FIELDS | OPTIONAL_INGRESS_RELATION_FIELDS
-        }
-
-        missing_fields = sorted(
-            [field for field in REQUIRED_INGRESS_RELATION_FIELDS if ingress_data.get(field) is None]
-        )
-
-        if missing_fields:
-            logger.error("Missing required data fields for ingress relation: {}".format(", ".join(missing_fields)))
-            self.unit.status = BlockedStatus("Missing fields for ingress: {}".format(", ".join(missing_fields)))
+        # 'ingress' relation handling.
+        self.ingress = IngressProvides(self)
+        # When the 'ingress' is ready to configure, do so.
+        self.framework.observe(self.on.ingress_available, self._on_config_changed)
 
     def _get_config_or_relation_data(self, field, fallback):
         """Helper method to get data from config or the ingress relation."""
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 45a9b09..4075546 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -231,7 +231,10 @@ class TestCharm(unittest.TestCase):
         }
         with self.assertLogs(level="ERROR") as logger:
             self.harness.update_relation_data(relation_id, 'gunicorn', relations_data)
-            msg = "ERROR:charm:Missing required data fields for ingress relation: service-hostname, service-port"
+            msg = (
+                "ERROR:charms.ingress.v0.ingress:Missing required data fields for "
+                "ingress relation: service-hostname, service-port"
+            )
             self.assertEqual(sorted(logger.output), [msg])
             # Confirm blocked status.
             self.assertEqual(
diff --git a/tox.ini b/tox.ini
index b4c85dc..7a39457 100644
--- a/tox.ini
+++ b/tox.ini
@@ -29,11 +29,11 @@ deps = -r{toxinidir}/tests/functional/requirements.txt
        -r{toxinidir}/requirements.txt
 
 [testenv:black]
-commands = black --skip-string-normalization --line-length=120 src/ tests/
+commands = black --skip-string-normalization --line-length=120 src/ tests/ lib/
 deps = black
 
 [testenv:lint]
-commands = flake8 src/ tests/
+commands = flake8 src/ tests/ lib/
 # Pin flake8 to 3.7.9 to match focal
 deps =
     flake8==3.7.9