From bb403df507c26a1ea2d988c4d861842d579429de Mon Sep 17 00:00:00 2001 From: Daniel Arndt Date: Fri, 19 Jul 2024 11:10:49 -0300 Subject: [PATCH] refactor: Remove event-specific PKI hooks, call `_configure` (#433) --- src/charm.py | 64 +++-------- tests/unit/test_charm.py | 242 ++++++++++++++++----------------------- 2 files changed, 116 insertions(+), 190 deletions(-) diff --git a/src/charm.py b/src/charm.py index 58f55deb..60cc4134 100755 --- a/src/charm.py +++ b/src/charm.py @@ -21,8 +21,6 @@ from charms.loki_k8s.v1.loki_push_api import LogForwarder from charms.prometheus_k8s.v0.prometheus_scrape import MetricsEndpointProvider from charms.tls_certificates_interface.v3.tls_certificates import ( - CertificateAvailableEvent, - CertificateCreationRequestEvent, TLSCertificatesProvidesV3, TLSCertificatesRequiresV3, ) @@ -53,7 +51,6 @@ CollectStatusEvent, ConfigChangedEvent, InstallEvent, - RelationJoinedEvent, RemoveEvent, ) from ops.main import main @@ -165,13 +162,24 @@ def __init__(self, *args): scheme=lambda: "https", ) self.s3_requirer = S3Requirer(self, S3_RELATION_NAME) + + configure_events = [ + self.on.update_status, + self.on.vault_pebble_ready, + self.on.config_changed, + self.on[PEER_RELATION_NAME].relation_created, + self.on[PEER_RELATION_NAME].relation_changed, + self.on.tls_certificates_pki_relation_joined, + self.tls_certificates_pki.on.certificate_available, + self.vault_pki.on.certificate_creation_request, + self.vault_autounseal_requires.on.vault_autounseal_details_ready, + self.vault_autounseal_provides.on.vault_autounseal_requirer_relation_created, + self.vault_autounseal_requires.on.vault_autounseal_provider_relation_broken, + ] + for event in configure_events: + self.framework.observe(event, self._configure) self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.collect_unit_status, self._on_collect_status) - self.framework.observe(self.on.update_status, self._configure) - self.framework.observe(self.on.vault_pebble_ready, self._configure) - self.framework.observe(self.on.config_changed, self._configure) - self.framework.observe(self.on[PEER_RELATION_NAME].relation_created, self._configure) - self.framework.observe(self.on[PEER_RELATION_NAME].relation_changed, self._configure) self.framework.observe(self.on.remove, self._on_remove) self.framework.observe(self.on.authorize_charm_action, self._on_authorize_charm_action) self.framework.observe(self.on.create_backup_action, self._on_create_backup_action) @@ -180,34 +188,10 @@ def __init__(self, *args): self.framework.observe( self.vault_kv.on.new_vault_kv_client_attached, self._on_new_vault_kv_client_attached ) - self.framework.observe( - self.on.tls_certificates_pki_relation_joined, - self._on_tls_certificates_pki_relation_joined, - ) - self.framework.observe( - self.tls_certificates_pki.on.certificate_available, - self._on_tls_certificate_pki_certificate_available, - ) - self.framework.observe( - self.vault_pki.on.certificate_creation_request, - self._on_vault_pki_certificate_creation_request, - ) - self.framework.observe( - self.vault_autounseal_requires.on.vault_autounseal_details_ready, - self._configure, - ) - self.framework.observe( - self.vault_autounseal_provides.on.vault_autounseal_requirer_relation_created, - self._configure, - ) self.framework.observe( self.vault_autounseal_provides.on.vault_autounseal_requirer_relation_broken, self._on_vault_autounseal_requirer_relation_broken, ) - self.framework.observe( - self.vault_autounseal_requires.on.vault_autounseal_provider_relation_broken, - self._configure, - ) def _on_vault_autounseal_requirer_relation_broken( self, event: VaultAutounsealRequirerRelationBroken @@ -485,10 +469,6 @@ def _on_new_vault_kv_client_attached(self, event: NewVaultKvClientAttachedEvent) nonce=event.nonce, ) - def _on_tls_certificates_pki_relation_joined(self, _: RelationJoinedEvent) -> None: - """Handle the tls-certificates-pki relation joined event.""" - self._configure_pki_secrets_engine() - def _configure_pki_secrets_engine(self) -> None: """Configure the PKI secrets engine.""" if not self.unit.is_leader(): @@ -646,18 +626,6 @@ def _get_pki_ca_certificate(self) -> Optional[str]: logger.info("No certificate matches the PKI CSR in secrets") return None - def _on_tls_certificate_pki_certificate_available(self, event: CertificateAvailableEvent): - """Handle the tls-certificates-pki certificate available event.""" - self._add_ca_certificate_to_pki_secrets_engine() - - def _on_vault_pki_certificate_creation_request( - self, event: CertificateCreationRequestEvent - ) -> None: - """Handle the vault-pki certificate creation request event.""" - self._generate_pki_certificate_for_requirer( - event.certificate_signing_request, event.relation_id - ) - def _generate_pki_certificate_for_requirer(self, csr: str, relation_id: int): """Generate a PKI certificate for a TLS requirer.""" if not self.unit.is_leader(): diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 7588fecf..cf10bdc0 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -16,14 +16,14 @@ CHARM_POLICY_NAME, CHARM_POLICY_PATH, PKI_CSR_SECRET_LABEL, + PKI_RELATION_NAME, S3_RELATION_NAME, + TLS_CERTIFICATES_PKI_RELATION_NAME, VAULT_CHARM_APPROLE_SECRET_LABEL, VaultCharm, config_file_content_matches, ) from charms.tls_certificates_interface.v3.tls_certificates import ( - CertificateAvailableEvent, - CertificateCreationRequestEvent, ProviderCertificate, ) from charms.vault_k8s.v0.vault_autounseal import ( @@ -52,7 +52,6 @@ TLS_CERTIFICATES_LIB_PATH = "charms.tls_certificates_interface.v3.tls_certificates" VAULT_AUTOUNSEAL_LIB_PATH = "charms.vault_k8s.v0.vault_autounseal" VAULT_KV_RELATION_NAME = "vault-kv" -TLS_CERTIFICATES_PKI_RELATION_NAME = "tls-certificates-pki" VAULT_KV_REQUIRER_APPLICATION_NAME = "vault-kv-requirer" PKI_MOUNT = "charm-pki" PKI_ROLE_NAME = "charm" @@ -1456,7 +1455,6 @@ def test_given_prerequisites_are_met_when_new_vault_kv_client_attached_then_kv_r event.egress_subnet = "2.2.2.0/24" event.nonce = "123123" self.harness.charm._on_new_vault_kv_client_attached(event) - self.harness.get_relation_data(rel_id, self.app_name) set_vault_url.assert_called() set_mount.assert_called() set_ca_certificate.assert_called() @@ -1535,12 +1533,14 @@ def test_given_vault_is_available_when_tls_certificates_pki_relation_joined_then self, patch_request_certificate_creation, ): + self.harness.add_storage(storage_name="config", attach=True) csr = "some csr content" self.harness.update_config({"common_name": "vault"}) self.mock_vault.configure_mock( **{ "is_initialized.return_value": True, "is_api_available.return_value": True, + "is_sealed.return_value": False, "get_intermediate_ca.return_value": "vault", "generate_pki_intermediate_ca_csr.return_value": csr, }, @@ -1614,21 +1614,29 @@ def test_given_vault_pki_configured_when_common_name_is_changed_then_new_certifi ) @patch("charm.get_common_name_from_certificate", new=Mock) + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.request_certificate_creation") + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_requirer_csrs") + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_provider_certificates") @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_assigned_certificates") - def test_given_vault_is_available_when_pki_certificate_is_available_then_certificate_added_to_vault_pki( + def test_given_vault_is_available_when_pki_certificate_is_available_then_certificate_added_to_vault_pki_and_latest_issuer_set_to_default( self, - patch_get_assigned_certificates, + patch_get_assigned_certificates: MagicMock, + patch_get_provider_certificates: MagicMock, + patch_get_requirer_csrs: MagicMock, + patch_request_certificate_creation: MagicMock, ): + csr = "some csr content" self.mock_vault.configure_mock( **{ "is_initialized.return_value": True, + "is_sealed.return_value": False, "is_api_available.return_value": True, "is_pki_role_created.return_value": False, "get_intermediate_ca.return_value": "vault", "is_common_name_allowed_in_pki_role.return_value": False, + "generate_pki_intermediate_ca_csr.return_value": csr, }, ) - csr = "some csr content" certificate = "some certificate" ca = "some ca" chain = [ca] @@ -1640,33 +1648,47 @@ def test_given_vault_is_available_when_pki_certificate_is_available_then_certifi role_id="root token content", secret_id="whatever secret id", ) - self._set_csr_secret_in_peer_relation(relation_id=peer_relation_id, csr="some csr content") - event = CertificateAvailableEvent( - handle=Mock(), + self._set_csr_secret_in_peer_relation(relation_id=peer_relation_id, csr=csr) + + relation_id = self.harness.add_relation( + relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, remote_app="tls-provider" + ) + provider_certificate = ProviderCertificate( + relation_id=relation_id, + application_name="tls-provider", + csr=csr, certificate=certificate, - certificate_signing_request=csr, ca=ca, chain=chain, + revoked=False, + expiry_time=datetime.now(timezone.utc), ) - relation_id = self.harness.add_relation( - relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, remote_app="tls-provider" - ) - - patch_get_assigned_certificates.return_value = [ - ProviderCertificate( - relation_id=relation_id, - application_name="tls-provider", - csr=csr, - certificate=certificate, - ca=ca, - chain=chain, - revoked=False, - expiry_time=datetime.now(timezone.utc), - ) - ] + patch_get_assigned_certificates.return_value = [provider_certificate] + patch_get_provider_certificates.return_value = [provider_certificate] + patch_get_requirer_csrs.return_value = [Mock(csr=csr)] + self.harness.add_storage("config", attach=True) - self.harness.charm._on_tls_certificate_pki_certificate_available(event=event) + # Reset mock counts, in case they were called during setup + self.mock_vault.reset_mock() + # When + self.harness.update_relation_data( + relation_id, + "tls-provider", + { + "certificates": json.dumps( + [ + { + "certificate_signing_request": "csr", + "certificate": certificate, + "ca": "ca", + "chain": ["chain"], + } + ] + ) + }, + ) + # Then self.mock_vault.set_pki_intermediate_ca_certificate.assert_called_with( certificate=certificate, mount=PKI_MOUNT, @@ -1674,28 +1696,38 @@ def test_given_vault_is_available_when_pki_certificate_is_available_then_certifi self.mock_vault.create_or_update_pki_charm_role.assert_called_with( allowed_domains="vault", mount=PKI_MOUNT, role=PKI_ROLE_NAME ) + self.mock_vault.make_latest_pki_issuer_default.assert_called_with(mount=PKI_MOUNT) @patch("ops.model.Container.restart", new=Mock) @patch("charm.get_common_name_from_certificate", new=Mock) + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.request_certificate_creation") + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_requirer_csrs") + @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_provider_certificates") @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_assigned_certificates") def test_given_vault_pki_configured_when_common_name_is_changed_then_new_certificate_added_to_vault_pki( self, patch_get_assigned_certificates, + patch_get_provider_certificates, + patch_get_requirer_csrs, + patch_request_certificate_creation, ): csr = "some csr content" - self.harness.update_config({"common_name": "vault"}) self.mock_vault.configure_mock( **{ "is_initialized.return_value": True, + "is_sealed.return_value": False, "is_api_available.return_value": True, + "is_pki_role_created.return_value": False, "get_intermediate_ca.return_value": "vault", - "generate_pki_intermediate_ca_csr.return_value": csr, - "is_sealed.return_value": False, "is_common_name_allowed_in_pki_role.return_value": False, + "generate_pki_intermediate_ca_csr.return_value": csr, }, ) + certificate = "some certificate" + ca = "some ca" + chain = [ca] + self.harness.update_config({"common_name": "vault"}) self.harness.set_leader(is_leader=True) - self.harness.add_storage(storage_name="config", attach=True) self.harness.set_can_connect(container=self.container_name, val=True) peer_relation_id = self._set_peer_relation() self._set_approle_secret( @@ -1703,46 +1735,29 @@ def test_given_vault_pki_configured_when_common_name_is_changed_then_new_certifi secret_id="whatever secret id", ) self._set_csr_secret_in_peer_relation(relation_id=peer_relation_id, csr=csr) - certificate = "some certificate" - ca = "some ca" - chain = [ca] - event = CertificateAvailableEvent( - handle=Mock(), - certificate=certificate, - certificate_signing_request=csr, - ca=ca, - chain=chain, - ) + relation_id = self.harness.add_relation( relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, remote_app="tls-provider" ) - - patch_get_assigned_certificates.return_value = [ - ProviderCertificate( - relation_id=relation_id, - application_name="tls-provider", - csr=csr, - certificate=certificate, - ca=ca, - chain=chain, - revoked=False, - expiry_time=datetime.now(timezone.utc), - ) - ] - - self.harness.charm._on_tls_certificate_pki_certificate_available(event=event) - - self.mock_vault.set_pki_intermediate_ca_certificate.assert_called_with( + provider_certificate = ProviderCertificate( + relation_id=relation_id, + application_name="tls-provider", + csr=csr, certificate=certificate, - mount=PKI_MOUNT, - ) - self.mock_vault.create_or_update_pki_charm_role.assert_called_with( - allowed_domains="vault", mount=PKI_MOUNT, role=PKI_ROLE_NAME + ca=ca, + chain=chain, + revoked=False, + expiry_time=datetime.now(timezone.utc), ) + patch_get_assigned_certificates.return_value = [provider_certificate] + patch_get_provider_certificates.return_value = [provider_certificate] + patch_get_requirer_csrs.return_value = [Mock(csr=csr)] + self.harness.add_storage("config", attach=True) - self.harness.update_config({"common_name": "new_common_name"}) + # Reset mock counts, in case they were called during setup + self.mock_vault.reset_mock() - self.harness.charm._on_tls_certificate_pki_certificate_available(event=event) + self.harness.update_config({"common_name": "new_common_name"}) self.mock_vault.set_pki_intermediate_ca_certificate.assert_called_with( certificate=certificate, @@ -1753,12 +1768,15 @@ def test_given_vault_pki_configured_when_common_name_is_changed_then_new_certifi ) @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesProvidesV3.set_relation_certificate") + @patch("charm.get_common_name_from_certificate") @patch("charm.get_common_name_from_csr") def test_given_vault_available_when_vault_pki_certificate_creation_request_then_certificate_is_provided( self, patch_get_common_name_from_csr, + patch_get_common_name_from_certificate, patch_set_relation_certificate, ): + # Given csr = "some csr content" certificate = "some certificate" ca = "some ca" @@ -1766,6 +1784,7 @@ def test_given_vault_available_when_vault_pki_certificate_creation_request_then_ self.mock_vault.configure_mock( **{ "is_initialized.return_value": True, + "is_sealed.return_value": False, "is_api_available.return_value": True, "is_pki_role_created.return_value": True, "sign_pki_certificate_signing_request.return_value": Certificate( @@ -1775,30 +1794,32 @@ def test_given_vault_available_when_vault_pki_certificate_creation_request_then_ ), }, ) - relation_id = self.harness.add_relation( - relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, remote_app="tls-provider" - ) common_name = "vault" - relation_id = 99 + # TODO: Use real certificates so we don't need to mock this out patch_get_common_name_from_csr.return_value = common_name + patch_get_common_name_from_certificate.return_value = common_name self.harness.update_config({"common_name": common_name}) self.harness.set_leader(is_leader=True) + self.harness.add_storage(storage_name="config", attach=True) self.harness.set_can_connect(container=self.container_name, val=True) self._set_peer_relation() - self._set_approle_secret( - role_id="root token content", - secret_id="whatever secret id", + self._set_approle_secret(role_id="root token content", secret_id="whatever secret id") + self.harness.add_relation( + relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, + remote_app="tls-provider", ) - event = CertificateCreationRequestEvent( - handle=Mock(), - certificate_signing_request=csr, - relation_id=relation_id, - is_ca=False, + # When + # TODO: Mock the lib so we don't need to deal with the databag + relation_id = self.harness.add_relation( + relation_name=PKI_RELATION_NAME, + remote_app="vault-cert-requirer", + unit_data={ + "certificate_signing_requests": json.dumps([{"certificate_signing_request": csr}]) + }, ) - self.harness.charm._on_vault_pki_certificate_creation_request(event=event) - + # Then self.mock_vault.sign_pki_certificate_signing_request.assert_called_with( mount=PKI_MOUNT, csr=csr, @@ -1814,69 +1835,6 @@ def test_given_vault_available_when_vault_pki_certificate_creation_request_then_ chain=chain, ) - @patch("charm.get_common_name_from_certificate", new=Mock) - @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.get_assigned_certificates") - def test_given_vault_is_available_when_pki_certificate_is_available_then_default_issuer_is_set_to_latest_issuer( - self, - patch_get_assigned_certificates, - ): - self.mock_vault.configure_mock( - **{ - "is_initialized.return_value": True, - "is_api_available.return_value": True, - "is_pki_role_created.return_value": False, - "get_intermediate_ca.return_value": "vault", - "is_common_name_allowed_in_pki_role.return_value": False, - }, - ) - csr = "some csr content" - certificate = "some certificate" - ca = "some ca" - chain = [ca] - self.harness.update_config({"common_name": "vault"}) - self.harness.set_leader(is_leader=True) - self.harness.set_can_connect(container=self.container_name, val=True) - peer_relation_id = self._set_peer_relation() - self._set_approle_secret( - role_id="root token content", - secret_id="whatever secret id", - ) - self._set_csr_secret_in_peer_relation(relation_id=peer_relation_id, csr="some csr content") - event = CertificateAvailableEvent( - handle=Mock(), - certificate=certificate, - certificate_signing_request=csr, - ca=ca, - chain=chain, - ) - relation_id = self.harness.add_relation( - relation_name=TLS_CERTIFICATES_PKI_RELATION_NAME, remote_app="tls-provider" - ) - - patch_get_assigned_certificates.return_value = [ - ProviderCertificate( - relation_id=relation_id, - application_name="tls-provider", - csr=csr, - certificate=certificate, - ca=ca, - chain=chain, - revoked=False, - expiry_time=datetime.now(timezone.utc), - ) - ] - - self.harness.charm._on_tls_certificate_pki_certificate_available(event=event) - - self.mock_vault.set_pki_intermediate_ca_certificate.assert_called_with( - certificate=certificate, - mount=PKI_MOUNT, - ) - self.mock_vault.create_or_update_pki_charm_role.assert_called_with( - allowed_domains="vault", mount=PKI_MOUNT, role=PKI_ROLE_NAME - ) - self.mock_vault.make_latest_pki_issuer_default.assert_called_with(mount=PKI_MOUNT) - @patch("ops.testing._TestingPebbleClient.restart_services", new=MagicMock) @patch(f"{VAULT_AUTOUNSEAL_LIB_PATH}.VaultAutounsealRequires.get_details") def test_given_autounseal_details_available_when_autounseal_details_ready_then_transit_stanza_generated(