From 74f192c2bd25b0ac730ef3fd234368774f0f6363 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:54:29 -0400 Subject: [PATCH] [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 --- .../observability_libs/v0/cert_handler.py | 19 ++--- .../observability_libs/v1/cert_handler.py | 27 +++--- pyproject.toml | 4 + tests/unit/test_cert_handler_v0.py | 80 ++++++++++++++++++ tests/unit/test_cert_handler_v1.py | 82 +++++++++++++++++++ tox.ini | 16 +++- 6 files changed, 201 insertions(+), 27 deletions(-) create mode 100644 tests/unit/test_cert_handler_v0.py create mode 100644 tests/unit/test_cert_handler_v1.py diff --git a/lib/charms/observability_libs/v0/cert_handler.py b/lib/charms/observability_libs/v0/cert_handler.py index e2e3998..275cf7d 100644 --- 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,13 +421,11 @@ 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 @@ -439,4 +433,5 @@ def _on_certificates_relation_broken(self, event: RelationBrokenEvent) -> None: 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 index 362240a..3b87ad4 100644 --- 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 index 9b06d37..edb2675 100644 --- 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 new file mode 100644 index 0000000..0618921 --- /dev/null +++ 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 new file mode 100644 index 0000000..40b57c8 --- /dev/null +++ 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 index 7d6511f..8db3b20 100644 --- 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 \ No newline at end of file + rm -rf ./lib/charms/tls_certificates_interface