[cert handler] do not observe rel broken directly (#99)

* [cert handler] do not observe rel broken directly * codespell ignore assertIn * [cert handler] don't fail on vault.clear() failures (#100) --------- Co-authored-by: PietroPasotti <[email protected]>
canonical · Jun 19, 2024 · 74f192c · 74f192c
1 parent e6b44fd
commit 74f192c
Show file tree

Hide file tree

Showing 6 changed files with 201 additions and 27 deletions.
diff --git a/lib/charms/observability_libs/v0/cert_handler.py b/lib/charms/observability_libs/v0/cert_handler.py
@@ -58,7 +58,7 @@
 
 import logging
 
-from ops.charm import CharmBase, RelationBrokenEvent
+from ops.charm import CharmBase
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.model import Relation
 
@@ -67,7 +67,7 @@
 
 LIBID = "b5cd5cd580f3428fa5f59a8876dcbe6a"
 LIBAPI = 0
-LIBPATCH = 13
+LIBPATCH = 14
 
 
 def is_ip_address(value: str) -> bool:
@@ -158,10 +158,6 @@ def __init__(
             self.certificates.on.all_certificates_invalidated,  # pyright: ignore
             self._on_all_certificates_invalidated,
         )
-        self.framework.observe(
-            self.charm.on[self.certificates_relation_name].relation_broken,  # pyright: ignore
-            self._on_certificates_relation_broken,
-        )
 
         # Peer relation events
         self.framework.observe(
@@ -425,18 +421,17 @@ def _on_certificate_invalidated(self, event: CertificateInvalidatedEvent) -> Non
         self.on.cert_changed.emit()  # pyright: ignore
 
     def _on_all_certificates_invalidated(self, event: AllCertificatesInvalidatedEvent) -> None:
-        # Do what you want with this information, probably remove all certificates
-        # Note: assuming "limit: 1" in metadata
-        self._generate_csr(overwrite=True, clear_cert=True)
-        self.on.cert_changed.emit()  # pyright: ignore
-
-    def _on_certificates_relation_broken(self, event: RelationBrokenEvent) -> None:
         """Clear the certificates data when removing the relation."""
+        # Note: assuming "limit: 1" in metadata
+        # The "certificates_relation_broken" event is converted to "all invalidated" custom
+        # event by the tls-certificates library. Per convention, we let the lib manage the
+        # relation and we do not observe "certificates_relation_broken" directly.
         if self._peer_relation:
             private_key = self._private_key
             # This is a workaround for https://bugs.launchpad.net/juju/+bug/2024583
             self._peer_relation.data[self.charm.unit].clear()
             if private_key:
                 self._peer_relation.data[self.charm.unit].update({"private_key": private_key})
 
+        # We do not generate a CSR here because the relation is gone.
         self.on.cert_changed.emit()  # pyright: ignore
diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
@@ -58,7 +58,7 @@
 
 import logging
 
-from ops.charm import CharmBase, RelationBrokenEvent
+from ops.charm import CharmBase
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.jujuversion import JujuVersion
 from ops.model import Relation, Secret, SecretNotFoundError
@@ -67,7 +67,7 @@
 
 LIBID = "b5cd5cd580f3428fa5f59a8876dcbe6a"
 LIBAPI = 1
-LIBPATCH = 10
+LIBPATCH = 11
 
 VAULT_SECRET_LABEL = "cert-handler-private-vault"
 
@@ -260,7 +260,13 @@ def retrieve(self) -> Dict[str, str]:
 
     def clear(self):
         """Clear the vault."""
-        self._backend.clear()
+        try:
+            self._backend.clear()
+        except SecretNotFoundError:
+            # guard against: https://github.com/canonical/observability-libs/issues/95
+            # this is fine, it might mean an earlier hook had already called .clear()
+            # not sure what exactly the root cause is, might be a juju bug
+            logger.debug("Could not clear vault: secret is gone already.")
 
 
 class CertHandler(Object):
@@ -344,10 +350,6 @@ def __init__(
             self.certificates.on.all_certificates_invalidated,  # pyright: ignore
             self._on_all_certificates_invalidated,
         )
-        self.framework.observe(
-            self.charm.on[self.certificates_relation_name].relation_broken,  # pyright: ignore
-            self._on_certificates_relation_broken,
-        )
         self.framework.observe(
             self.charm.on.upgrade_charm,  # pyright: ignore
             self._on_upgrade_charm,
@@ -574,14 +576,13 @@ def _on_certificate_invalidated(self, event: CertificateInvalidatedEvent) -> Non
             self.on.cert_changed.emit()  # pyright: ignore
 
     def _on_all_certificates_invalidated(self, _: AllCertificatesInvalidatedEvent) -> None:
-        # Do what you want with this information, probably remove all certificates
-        # Note: assuming "limit: 1" in metadata
-        self._generate_csr(overwrite=True, clear_cert=True)
-        self.on.cert_changed.emit()  # pyright: ignore
-
-    def _on_certificates_relation_broken(self, _: RelationBrokenEvent) -> None:
         """Clear all secrets data when removing the relation."""
+        # Note: assuming "limit: 1" in metadata
+        # The "certificates_relation_broken" event is converted to "all invalidated" custom
+        # event by the tls-certificates library. Per convention, we let the lib manage the
+        # relation and we do not observe "certificates_relation_broken" directly.
         self.vault.clear()
+        # We do not generate a CSR here because the relation is gone.
         self.on.cert_changed.emit()  # pyright: ignore
 
     def _check_juju_supports_secrets(self) -> bool:

diff --git a/pyproject.toml b/pyproject.toml
@@ -34,3 +34,7 @@ pythonPlatform = "All"
 minversion = "6.0"
 log_cli_level = "INFO"
 asyncio_mode = "auto"
+
+[tool.codespell]
+skip = ".git,.tox,build,venv*"
+ignore-words-list = "assertIn"
diff --git a/tests/unit/test_cert_handler_v0.py b/tests/unit/test_cert_handler_v0.py
@@ -0,0 +1,80 @@
+# Copyright 2022 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import unittest
+from textwrap import dedent
+from unittest.mock import patch
+
+from charms.observability_libs.v0.cert_handler import CertHandler
+from ops.charm import CharmBase
+from ops.model import ActiveStatus
+from ops.testing import Harness
+
+
+class StandInCharm(CharmBase):
+    meta = dedent(
+        """
+            name: test-charm
+            peers:
+              peers:
+                interface: peers
+            requires:
+              certificates:
+                interface: tls-certificates
+                limit: 1
+        """
+    )
+
+    def __init__(self, *args):
+        super().__init__(*args)
+
+        self.cert_handler = CertHandler(
+            charm=self,
+            key="stand-in-server-cert",
+            peer_relation_name="peers",
+        )
+
+        self.framework.observe(self.cert_handler.on.cert_changed, self._on_server_cert_changed)
+
+    def _on_server_cert_changed(self, event):
+        self.unit.status = ActiveStatus("metrics endpoints changed")
+
+
+class TestCertHandlerV0(unittest.TestCase):
+
+    def setUp(self) -> None:
+        self.harness = Harness(StandInCharm, meta=StandInCharm.meta)
+        self.harness.begin_with_initial_hooks()
+
+    def test_tls_is_inactive(self):
+        # GIVEN an isolated charm
+        charm = self.harness.charm
+
+        # THEN private key is ready, but the CSR isn't
+        self.assertIn("-----BEGIN RSA PRIVATE KEY-----", charm.cert_handler._private_key)
+        self.assertEqual(charm.cert_handler._csr, None)
+
+        # AND the "enabled" property is False
+        self.assertEqual(charm.cert_handler.enabled, False)
+
+        # WHEN a certificates relations just joins
+        self.relation_id = self.harness.add_relation("certificates", "ca")
+        self.harness.add_relation_unit(self.relation_id, "ca/0")
+
+        # THEN the CSR is ready, and tls is "enabled"
+        self.assertIn("-----BEGIN CERTIFICATE REQUEST-----", charm.cert_handler._csr)
+        self.assertEqual(charm.cert_handler.enabled, True)
+
+    def test_custom_event_emitted_when_certificates_relation_removed(self):
+        # GIVEN a tls relation
+        charm = self.harness.charm
+        self.relation_id = self.harness.add_relation("certificates", "ca")
+        self.harness.add_relation_unit(self.relation_id, "ca/0")
+
+        # WHEN the relation is removed
+        observer_patcher = patch.object(charm, "_on_server_cert_changed")
+        mock = observer_patcher.start()
+        self.harness.remove_relation(self.relation_id)
+
+        # THEN the "cert changed" observer is called
+        mock.assert_called_once()
diff --git a/tests/unit/test_cert_handler_v1.py b/tests/unit/test_cert_handler_v1.py
@@ -0,0 +1,82 @@
+# Copyright 2022 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import unittest
+from textwrap import dedent
+from unittest.mock import patch
+
+from charms.observability_libs.v1.cert_handler import CertHandler
+from ops.charm import CharmBase
+from ops.model import ActiveStatus
+from ops.testing import Harness
+
+
+class StandInCharm(CharmBase):
+    meta = dedent(
+        """
+            name: test-charm
+            peers:
+              peers:
+                interface: peers
+            requires:
+              certificates:
+                interface: tls-certificates
+                limit: 1
+        """
+    )
+
+    def __init__(self, *args):
+        super().__init__(*args)
+
+        self.cert_handler = CertHandler(
+            charm=self,
+            key="stand-in-server-cert",
+            sans=["demo.example.internal"],
+        )
+
+        self.framework.observe(self.cert_handler.on.cert_changed, self._on_server_cert_changed)
+
+    def _on_server_cert_changed(self, event):
+        self.unit.status = ActiveStatus("metrics endpoints changed")
+
+
+class TestCertHandlerV1(unittest.TestCase):
+
+    def setUp(self) -> None:
+        self.harness = Harness(StandInCharm, meta=StandInCharm.meta)
+        self.harness.begin_with_initial_hooks()
+
+    def test_tls_is_inactive(self):
+        # GIVEN an isolated charm
+        charm = self.harness.charm
+
+        # THEN private key is ready, but the CSR isn't
+        self.assertIn("-----BEGIN RSA PRIVATE KEY-----", charm.cert_handler.private_key)
+        self.assertEqual(charm.cert_handler._csr, None)
+
+        # AND the "enabled" and "available" properties are both False
+        self.assertEqual(charm.cert_handler.enabled, False)
+        self.assertEqual(charm.cert_handler.available, False)
+
+        # WHEN a certificates relations just joins
+        self.relation_id = self.harness.add_relation("certificates", "ca")
+        self.harness.add_relation_unit(self.relation_id, "ca/0")
+
+        # THEN the CSR is ready, tls is "enabled" but not "available"
+        self.assertIn("-----BEGIN CERTIFICATE REQUEST-----", charm.cert_handler._csr)
+        self.assertEqual(charm.cert_handler.enabled, True)
+        self.assertEqual(charm.cert_handler.available, False)
+
+    def test_custom_event_emitted_when_certificates_relation_removed(self):
+        # GIVEN a tls relation
+        charm = self.harness.charm
+        self.relation_id = self.harness.add_relation("certificates", "ca")
+        self.harness.add_relation_unit(self.relation_id, "ca/0")
+
+        # WHEN the relation is removed
+        observer_patcher = patch.object(charm, "_on_server_cert_changed")
+        mock = observer_patcher.start()
+        self.harness.remove_relation(self.relation_id)
+
+        # THEN the "cert changed" observer is called
+        mock.assert_called_once()
diff --git a/tox.ini b/tox.ini
@@ -44,7 +44,7 @@ deps =
     codespell
 commands =
     codespell {[vars]lib_path}
-    codespell . --skip .git --skip .tox --skip build --skip venv --skip .mypy_cache
+    codespell .
     ruff {[vars]all_path}
     black --check --diff {[vars]all_path}
 
@@ -64,8 +64,20 @@ description = Run unit tests
 deps =
     pytest
     coverage[toml]
+    cryptography  # tls_certificates
+    jsonschema  # tls_certificates
     -r{toxinidir}/requirements.txt
+allowlist_externals =
+    mkdir
+    sh
 commands =
+    # Download 3rd party libs our libs depend on
+    mkdir -p {toxinidir}/lib/charms/tls_certificates_interface/v2
+    sh -c 'stat {toxinidir}/lib/charms/tls_certificates_interface/v2/tls_certificates.py > /dev/null 2>&1 || wget "https://raw.githubusercontent.com/canonical/tls-certificates-interface/main/lib/charms/tls_certificates_interface/v2/tls_certificates.py" -P {toxinidir}/lib/charms/tls_certificates_interface/v2'
+
+    mkdir -p {toxinidir}/lib/charms/tls_certificates_interface/v3
+    sh -c 'stat {toxinidir}/lib/charms/tls_certificates_interface/v3/tls_certificates.py > /dev/null 2>&1 || wget "https://raw.githubusercontent.com/canonical/tls-certificates-interface/main/lib/charms/tls_certificates_interface/v3/tls_certificates.py" -P {toxinidir}/lib/charms/tls_certificates_interface/v3'
+
     python -m doctest {[vars]lib_path}/v0/kubernetes_compute_resources_patch.py
     coverage run \
       --source={[vars]lib_path} \
@@ -100,4 +112,4 @@ allowlist_externals =
 commands =
     charmcraft fetch-lib charms.tls_certificates_interface.v2.tls_certificates
     pytest -v --tb native {[vars]tst_path}/scenario --log-cli-level=INFO -s {posargs}
-    rm -rf ./lib/charms/tls_certificates_interface
+    rm -rf ./lib/charms/tls_certificates_interface