fix: Removes the vault-kv secret when the unit is removed (#455)

lib/charms/vault_k8s/v0/vault_kv.py

@@ -135,7 +135,7 @@ def get_nonce(self):
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 10
+LIBPATCH = 11
 
 PYDEPS = ["pydantic", "pytest-interface-tester"]
 
@@ -259,6 +259,25 @@ class VaultKvGoneAwayEvent(ops.EventBase):
     pass
 
 
+class VaultKvClientDetachedEvent(ops.EventBase):
+    """VaultKvClientDetachedEvent Event."""
+
+    def __init__(self, handle: ops.Handle, unit_name: str):
+        super().__init__(handle)
+        self.unit_name = unit_name
+
+    def snapshot(self) -> Dict[str, Any]:
+        """Return snapshot data that should be persisted."""
+        return {
+            "unit_name": self.unit_name,
+        }
+
+    def restore(self, snapshot: Dict[str, Any]) -> None:
+        """Restore the event from a snapshot."""
+        super().restore(snapshot)
+        self.unit_name = snapshot["unit_name"]
+
+
 class NewVaultKvClientAttachedEvent(ops.EventBase):
     """New vault kv client attached event."""
 
@@ -306,7 +325,7 @@ class VaultKvProviderEvents(ops.ObjectEvents):
     """List of events that the Vault Kv provider charm can leverage."""
 
     new_vault_kv_client_attached = ops.EventSource(NewVaultKvClientAttachedEvent)
-    gone_away = ops.EventSource(VaultKvGoneAwayEvent)
+    vault_kv_client_detached = ops.EventSource(VaultKvClientDetachedEvent)
 
 
 class VaultKvProvides(ops.Object):
@@ -327,8 +346,8 @@ def __init__(
             self._on_relation_changed,
         )
         self.framework.observe(
-            self.charm.on[relation_name].relation_broken,
-            self._on_vault_kv_relation_broken,
+            self.charm.on[relation_name].relation_departed,
+            self._on_vault_kv_relation_departed,
         )
 
     def _on_relation_changed(self, event: ops.RelationChangedEvent):
@@ -356,9 +375,10 @@ def _on_relation_changed(self, event: ops.RelationChangedEvent):
                 nonce=event.relation.data[unit]["nonce"],
             )
 
-    def _on_vault_kv_relation_broken(self, event: ops.RelationBrokenEvent):
-        """Handle relation broken."""
-        self.on.gone_away.emit()
+    def _on_vault_kv_relation_departed(self, event: ops.RelationDepartedEvent):
+        """Handle relation departed."""
+        if event.departing_unit:
+            self.on.vault_kv_client_detached.emit(unit_name=event.departing_unit.name)
 
     def set_vault_url(self, relation: ops.Relation, vault_url: str):
         """Set the vault_url on the relation."""

src/charm.py

@@ -39,7 +39,11 @@
     Vault,
     VaultClientError,
 )
-from charms.vault_k8s.v0.vault_kv import NewVaultKvClientAttachedEvent, VaultKvProvides
+from charms.vault_k8s.v0.vault_kv import (
+    NewVaultKvClientAttachedEvent,
+    VaultKvClientDetachedEvent,
+    VaultKvProvides,
+)
 from charms.vault_k8s.v0.vault_s3 import S3, S3Error
 from charms.vault_k8s.v0.vault_tls import File, VaultCertsError, VaultTLSManager
 from container import Container
@@ -187,6 +191,9 @@ 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.vault_kv.on.vault_kv_client_detached, self._on_vault_kv_client_detached
+        )
         self.framework.observe(
             self.vault_autounseal_provides.on.vault_autounseal_requirer_relation_broken,
             self._on_vault_autounseal_requirer_relation_broken,
@@ -466,6 +473,10 @@ def _on_new_vault_kv_client_attached(self, event: NewVaultKvClientAttachedEvent)
             nonce=event.nonce,
         )
 
+    def _on_vault_kv_client_detached(self, event: VaultKvClientDetachedEvent):
+        label = self._get_vault_kv_secret_label(unit_name=event.unit_name)
+        self._remove_juju_secret_by_label(label=label)
+
     def _configure_pki_secrets_engine(self) -> None:
         """Configure the PKI secrets engine."""
         if not self.unit.is_leader():
@@ -889,7 +900,7 @@ def _on_restore_backup_action(self, event: ActionEvent) -> None:
                     ca_cert_path=self.tls.get_tls_file_path_in_charm(File.CA),
                 )
                 if role_id and secret_id and not vault.authenticate(AppRole(role_id, secret_id)):
-                    self._remove_approle_auth_secret()
+                    self._remove_juju_secret_by_label(VAULT_CHARM_APPROLE_SECRET_LABEL)
         except Exception as e:
             logger.error("Failed to remove old approle secret: %s", e)
             event.fail(message="Failed to remove old approle secret.")
@@ -970,30 +981,38 @@ def _ensure_unit_credentials(
             token_ttl="1h",
             token_max_ttl="1h",
         )
+        juju_secret_label = self._get_vault_kv_secret_label(unit_name=unit_name)
         secret = self._create_or_update_kv_secret(
             vault,
+            nonce,
             relation,
             role_id,
             role_name,
             egress_subnets,
+            juju_secret_label,
         )
         self.vault_kv.set_unit_credentials(relation, nonce, secret)
 
     def _create_or_update_kv_secret(
         self,
         vault: Vault,
+        nonce: str,
         relation: Relation,
         role_id: str,
         role_name: str,
         egress_subnets: List[str],
+        label: str,
     ) -> Secret:
         """Create or update a KV secret for a unit.
 
         Fetch secret id from peer relation, if it exists, update the secret,
         otherwise create it.
         """
-        label = KV_SECRET_PREFIX + role_name
-        secret_id = self._get_vault_kv_secret_in_peer_relation(label)
+        # TODO bug: https://bugs.launchpad.net/juju/+bug/2075153
+        # Until the referenced bug is fixed we must pass the secret ID here
+        # not to lose the secret://modeluuid:secretID format
+        current_credentials = self.vault_kv.get_credentials(relation)
+        secret_id = current_credentials.get(nonce, None)
         if secret_id is None:
             return self._create_kv_secret(
                 vault, relation, role_id, role_name, egress_subnets, label
@@ -1020,7 +1039,6 @@ def _create_kv_secret(
         )
         if secret.id is None:
             raise RuntimeError(f"Unexpected error, just created secret {label!r} has no id")
-        self._set_vault_kv_secret_in_peer_relation(label, secret.id)
         secret.grant(relation)
         return secret
 
@@ -1058,27 +1076,6 @@ def _remove_stale_nonce(self, relation: Relation, nonce: str) -> None:
         if nonce not in set(credential_nonces):
             self.vault_kv.remove_unit_credentials(relation, nonce=nonce)
 
-    def _get_vault_kv_secrets_in_peer_relation(self) -> Dict[str, str]:
-        """Return the vault kv secrets from the peer relation."""
-        if not self._is_peer_relation_created():
-            raise RuntimeError("Peer relation not created")
-        relation = self.model.get_relation(PEER_RELATION_NAME)
-        secrets = json.loads(relation.data[self.app].get("vault-kv-secrets", "{}"))  # type: ignore[union-attr]  # noqa: E501
-        return secrets
-
-    def _get_vault_kv_secret_in_peer_relation(self, label: str) -> Optional[str]:
-        """Return the vault kv secret id associated to input label from peer relation."""
-        return self._get_vault_kv_secrets_in_peer_relation().get(label)
-
-    def _set_vault_kv_secret_in_peer_relation(self, label: str, secret_id: str):
-        """Set the vault kv secret in the peer relation."""
-        if not self._is_peer_relation_created():
-            raise RuntimeError("Peer relation not created")
-        secrets = self._get_vault_kv_secrets_in_peer_relation()
-        secrets[label] = secret_id
-        relation = self.model.get_relation(PEER_RELATION_NAME)
-        relation.data[self.app].update({"vault-kv-secrets": json.dumps(secrets, sort_keys=True)})  # type: ignore[union-attr]  # noqa: E501
-
     def _delete_vault_data(self) -> None:
         """Delete Vault's data."""
         try:
@@ -1299,14 +1296,18 @@ def _get_approle_auth_secret(self) -> Tuple[Optional[str], Optional[str]]:
         )
         return role_id, secret_id
 
-    def _remove_approle_auth_secret(self) -> None:
-        """Remove the approle secret if it exists."""
+    def _remove_juju_secret_by_label(self, label: str):
+        """Remove the specified secret if it exists."""
         try:
-            juju_secret = self.model.get_secret(label=VAULT_CHARM_APPROLE_SECRET_LABEL)
+            juju_secret = self.model.get_secret(label=label)
             juju_secret.remove_all_revisions()
         except SecretNotFoundError:
             return
 
+    def _get_vault_kv_secret_label(self, unit_name: str):
+        unit_name_dash = unit_name.replace("/", "-")
+        return f"{KV_SECRET_PREFIX}{unit_name_dash}"
+
     def _get_missing_s3_parameters(self) -> List[str]:
         """Return the list of missing S3 parameters.
 

tests/unit/lib/charms/vault_k8s/v0/test_vault_kv.py

@@ -10,6 +10,7 @@
 from charms.vault_k8s.v0.vault_kv import (
     KVRequest,
     NewVaultKvClientAttachedEvent,
+    VaultKvClientDetachedEvent,
     VaultKvConnectedEvent,
     VaultKvGoneAwayEvent,
     VaultKvProvides,
@@ -20,16 +21,18 @@
 from ops import testing
 from ops.charm import CharmBase
 
+VAULT_KV_RELATION_NAME = "vault-kv"
+
 
 class VaultKvProviderCharm(CharmBase):
     metadata_yaml = textwrap.dedent(
-        """
+        f"""
         name: vault-kv-provider
         containers:
           vault:
             resource: vault-image
         provides:
-          vault-kv:
+          {VAULT_KV_RELATION_NAME}:
             interface: vault-kv
         """
     )
@@ -40,20 +43,26 @@ def __init__(self, *args):
         self.framework.observe(
             self.interface.on.new_vault_kv_client_attached, self._on_new_vault_kv_client_attached
         )
+        self.framework.observe(
+            self.interface.on.vault_kv_client_detached, self._on_vault_kv_client_detached
+        )
 
     def _on_new_vault_kv_client_attached(self, event: NewVaultKvClientAttachedEvent):
         pass
 
+    def _on_vault_kv_client_detached(self, event: VaultKvClientDetachedEvent):
+        pass
+
 
 class VaultKvRequirerCharm(CharmBase):
     metadata_yaml = textwrap.dedent(
-        """
+        f"""
         name: vault-kv-requirer
         containers:
           my-app:
             resource: my-app-image
         requires:
-          vault-kv:
+          {VAULT_KV_RELATION_NAME}:
             interface: vault-kv
         """
     )
@@ -62,7 +71,7 @@ def __init__(self, *args):
         super().__init__(*args)
         self.interface = VaultKvRequires(
             self,
-            relation_name="vault-kv",
+            relation_name=VAULT_KV_RELATION_NAME,
             mount_suffix="dummy",
         )
         self.framework.observe(self.interface.on.connected, self._on_connected)
@@ -90,7 +99,7 @@ def setUp(self):
     def setup_relation(self, remote_app: str = "vault-kv-requires", leader: bool = True) -> tuple:
         """Set up a relation between the charm and a remote app with 1 unit."""
         remote_unit = remote_app + "/0"
-        rel_name = "vault-kv"
+        rel_name = VAULT_KV_RELATION_NAME
         self.harness.set_leader(leader)
         rel_id = self.harness.add_relation(rel_name, remote_app)
         relation = self.harness.model.get_relation(rel_name, rel_id)
@@ -414,6 +423,17 @@ def test_given_2_requests_when_get_kv_requests_then_requests_are_returned(self):
         assert expected_kv_request_1 in kv_requests
         assert expected_kv_request_2 in kv_requests
 
+    @patch("test_vault_kv.VaultKvProviderCharm._on_vault_kv_client_detached")
+    def test_given_vault_kv_relation_when_relation_departed_then_vault_kv_client_detached_event_fired(
+        self, _on_vault_kv_client_detached
+    ):
+        _, remote_unit, _, rel_id = self.setup_relation()
+        self.harness.remove_relation(rel_id)
+        args, _ = _on_vault_kv_client_detached.call_args
+        event = args[0]
+        assert isinstance(event, VaultKvClientDetachedEvent)
+        assert event.unit_name == remote_unit
+
 
 class TestVaultKvRequires(unittest.TestCase):
     def setUp(self):
@@ -426,7 +446,7 @@ def setUp(self):
     def setup_relation(self, leader: bool = True) -> tuple:
         remote_app = "vault-kv-provides"
         remote_unit = remote_app + "/0"
-        rel_name = "vault-kv"
+        rel_name = VAULT_KV_RELATION_NAME
         self.harness.set_leader(leader)
         rel_id = self.harness.add_relation(rel_name, remote_app)
         relation = self.harness.model.get_relation(rel_name, rel_id)

tests/unit/test_charm.py

@@ -1506,6 +1506,43 @@ def test_given_prerequisites_are_met_when_new_vault_kv_client_attached_then_kv_m
             SecretsBackend.KV_V2, "charm-vault-kv-requirer-suffix"
         )
 
+    @patch("ops.model.Secret.remove_all_revisions")
+    def test_given_vault_kv_client_when_client_detached_then_kv_secret_is_removed(
+        self,
+        patch_remove_secret,
+    ):
+        self.mock_vault.configure_mock(
+            **{
+                "configure_approle.return_value": "12345678",
+                "generate_role_secret_id.return_value": "11111111",
+            },
+        )
+        self.mock_vault_tls_manager.pull_tls_file_from_workload.return_value = "test cert"
+
+        self.harness.add_relation(relation_name="vault-peers", remote_app="vault")
+        self.harness.set_leader(is_leader=True)
+        self.harness.set_can_connect(container=self.container_name, val=True)
+        event = Mock()
+        event.params = {"relation_name": "relation", "relation_id": "99"}
+        self._set_approle_secret(
+            role_id="root token content",
+            secret_id="whatever secret id",
+        )
+        rel_id, _ = self.setup_vault_kv_relation()
+        event = Mock()
+        event.relation_name = VAULT_KV_RELATION_NAME
+        event.relation_id = rel_id
+        event.app_name = VAULT_KV_REQUIRER_APPLICATION_NAME
+        event.unit_name = f"{VAULT_KV_REQUIRER_APPLICATION_NAME}/0"
+        event.mount_suffix = "suffix"
+        event.egress_subnets = ["2.2.2.0/24"]
+        event.nonce = "123123"
+        self.harness.charm._on_new_vault_kv_client_attached(event)
+        kv_client_detached_event = Mock()
+        kv_client_detached_event.unit_name = f"{VAULT_KV_REQUIRER_APPLICATION_NAME}/0"
+        self.harness.charm._on_vault_kv_client_detached(kv_client_detached_event)
+        patch_remove_secret.assert_called()
+
     # Test PKI
     @patch("charm.get_common_name_from_certificate", new=Mock)
     @patch(f"{TLS_CERTIFICATES_LIB_PATH}.TLSCertificatesRequiresV3.request_certificate_creation")