Fix password_id not propagated when secrets enabled (#29)

canonical · Jan 2, 2024 · ea49ecf · ea49ecf
1 parent 3079ff8
commit ea49ecf
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 23 deletions.
diff --git a/metadata.yaml b/metadata.yaml
@@ -31,3 +31,7 @@ provides:
   smtp-legacy:
     interface: smtp
 
+peers:
+  smtp-peers:
+    interface: smtp-instance
+
diff --git a/src-docs/charm.py.md b/src-docs/charm.py.md
@@ -15,7 +15,7 @@ SMTP Integrator Charm service.
 ## <kbd>class</kbd> `SmtpIntegratorOperatorCharm`
 Charm the service. 
 
-<a href="../src/charm.py#L23"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L24"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>function</kbd> `__init__`
 

diff --git a/src-docs/charm_state.py.md b/src-docs/charm_state.py.md
@@ -51,12 +51,11 @@ Represents the state of the SMTP Integrator charm.
  - <b>`port`</b>:  The port of the outgoing SMTP relay. 
  - <b>`user`</b>:  The SMTP AUTH user to use for the outgoing SMTP relay. 
  - <b>`password`</b>:  The SMTP AUTH password to use for the outgoing SMTP relay. 
- - <b>`password_id`</b>:  The secret ID where the SMTP AUTH password for the SMTP relay is stored. 
  - <b>`auth_type`</b>:  The type used to authenticate with the SMTP relay. 
  - <b>`transport_security`</b>:  The security protocol to use for the outgoing SMTP relay. 
  - <b>`domain`</b>:  The domain used by the sent emails from SMTP relay. 
 
-<a href="../src/charm_state.py#L79"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm_state.py#L77"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>function</kbd> `__init__`
 
@@ -77,7 +76,7 @@ Initialize a new instance of the CharmState class.
 
 ---
 
-<a href="../src/charm_state.py#L94"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm_state.py#L91"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>classmethod</kbd> `from_charm`
 

diff --git a/src/charm.py b/src/charm.py
@@ -6,6 +6,7 @@
 """SMTP Integrator Charm service."""
 
 import logging
+from typing import Optional
 
 import ops
 from charms.smtp_integrator.v0 import smtp
@@ -50,9 +51,10 @@ def _on_relation_created(self, event: ops.RelationCreatedEvent) -> None:
         """
         if not self.model.unit.is_leader():
             return
-        if secret_id := self._charm_state.password_id:
-            secret = self.model.get_secret(id=secret_id)
-            secret.grant(event.relation)
+        if self._has_secrets():
+            secret = self._store_password_as_secret()
+            if secret:
+                secret.grant(event.relation)
         self._update_smtp_relation(event.relation)
 
     def _on_legacy_relation_created(self, event: ops.RelationCreatedEvent) -> None:
@@ -68,16 +70,37 @@ def _on_legacy_relation_created(self, event: ops.RelationCreatedEvent) -> None:
     def _on_config_changed(self, _) -> None:
         """Handle changes in configuration."""
         self.unit.status = ops.MaintenanceStatus("Configuring charm")
-        self._store_password_as_secret()
+        if self._has_secrets():
+            self._store_password_as_secret()
         self._update_relations()
         self.unit.status = ops.ActiveStatus()
 
-    def _store_password_as_secret(self) -> None:
-        """Store the SMTP password as a secret."""
-        if self._charm_state.password and self._has_secrets():
-            # Note https://github.com/juju/python-libjuju/issues/970
+    def _store_password_as_secret(self) -> Optional[ops.Secret]:
+        """Store the SMTP password as a secret.
+
+        Returns:
+            the secret id.
+        """
+        peer_relation = self.model.get_relation("smtp-peers")
+        assert peer_relation  # nosec
+        secret = None
+        if secret_id := peer_relation.data[self.app].get("secret-id"):
+            try:
+                secret = self.model.get_secret(id=secret_id)
+            except ops.SecretNotFoundError as exc:
+                logger.exception("Failed to get secret id %s: %s", secret_id, str(exc))
+                del peer_relation.data[self.app][secret_id]
+        if self._charm_state.password and not secret:
             secret = self.app.add_secret({"password": self._charm_state.password})
-            self._charm_state.password_id = secret.id
+            peer_relation.data[self.app].update({"secret-id": secret.id})
+            return secret
+        if self._charm_state.password and secret:
+            secret.set_content({"password": self._charm_state.password})
+            return secret
+        if secret:
+            secret.remove_all_revisions()
+            del peer_relation.data[self.app][secret_id]
+        return None
 
     def _update_relations(self) -> None:
         """Update all SMTP data for the existing relations."""
@@ -127,11 +150,14 @@ def _get_smtp_data(self) -> smtp.SmtpRelationData:
         Returns:
             SmtpRelationData containing the SMTP details.
         """
+        peer_relation = self.model.get_relation("smtp-peers")
+        assert peer_relation  # nosec
+        password_id = peer_relation.data[self.app].get("secret-id")
         return smtp.SmtpRelationData(
             host=self._charm_state.host,
             port=self._charm_state.port,
             user=self._charm_state.user,
-            password_id=self._charm_state.password_id,
+            password_id=password_id,
             auth_type=self._charm_state.auth_type,
             transport_security=self._charm_state.transport_security,
             domain=self._charm_state.domain,

diff --git a/src/charm_state.py b/src/charm_state.py
@@ -61,7 +61,6 @@ class CharmState:  # pylint: disable=too-many-instance-attributes
         port: The port of the outgoing SMTP relay.
         user: The SMTP AUTH user to use for the outgoing SMTP relay.
         password: The SMTP AUTH password to use for the outgoing SMTP relay.
-        password_id: The secret ID where the SMTP AUTH password for the SMTP relay is stored.
         auth_type: The type used to authenticate with the SMTP relay.
         transport_security: The security protocol to use for the outgoing SMTP relay.
         domain: The domain used by the sent emails from SMTP relay.
@@ -71,7 +70,6 @@ class CharmState:  # pylint: disable=too-many-instance-attributes
     port: int
     user: Optional[str]
     password: Optional[str]
-    password_id: Optional[str]
     auth_type: Optional[smtp.AuthType]
     transport_security: Optional[smtp.TransportSecurity]
     domain: Optional[str]
@@ -86,7 +84,6 @@ def __init__(self, *, smtp_integrator_config: SmtpIntegratorConfig):
         self.port = smtp_integrator_config.port
         self.user = smtp_integrator_config.user
         self.password = smtp_integrator_config.password
-        self.password_id = None
         self.auth_type = smtp_integrator_config.auth_type
         self.transport_security = smtp_integrator_config.transport_security
         self.domain = smtp_integrator_config.domain

diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
@@ -106,14 +106,31 @@ def test_legacy_relation_joined_populates_data():
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
     harness.begin()
-    harness.charm.on.config_changed.emit()
     harness.add_relation("smtp-legacy", "example")
     data = harness.model.get_relation("smtp-legacy").data[harness.model.app]
     assert data["host"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["host"]
     assert data["port"] == str(MINIMAL_CHARM_CONFIG_WITH_PASSWORD["port"])
     assert data["password"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["password"]
 
 
+def test_config_changed_joined_populates_data():
+    """
+    arrange: set up a charm with valid configuration and leadership for the unit.
+    act: add an smtp-legacy relation and trigger a configuration change.
+    assert: the smtp-legacy relation gets populated with the SMTP data.
+    """
+    harness = Harness(SmtpIntegratorOperatorCharm)
+    harness.set_leader(True)
+    harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
+    harness.begin()
+    harness.add_relation("smtp-legacy", "example")
+    harness.charm.on.config_changed.emit()
+    data = harness.model.get_relation("smtp-legacy").data[harness.model.app]
+    assert data["host"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["host"]
+    assert data["port"] == str(MINIMAL_CHARM_CONFIG_WITH_PASSWORD["port"])
+    assert data["password"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["password"]
+
+
 def test_legacy_relation_joined_doesnt_populate_password_id():
     """
     arrange: set up a charm with valid configuration and leadership for the unit.
@@ -124,7 +141,6 @@ def test_legacy_relation_joined_doesnt_populate_password_id():
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
     harness.begin()
-    harness.charm.on.config_changed.emit()
     harness.add_relation("smtp-legacy", "example")
     data = harness.model.get_relation("smtp-legacy").data[harness.model.app]
     assert "password_id" not in data
@@ -142,8 +158,29 @@ def test_relation_joined_when_secrets_enabled_populates_data(mock_juju_env):
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
     harness.begin()
-    harness.charm.on.config_changed.emit()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
+    harness.add_relation("smtp", "example")
+    data = harness.model.get_relation("smtp").data[harness.model.app]
+    assert data["host"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["host"]
+    assert data["port"] == str(MINIMAL_CHARM_CONFIG_WITH_PASSWORD["port"])
+    assert data["password_id"] is not None
+
+
+@patch.object(ops.JujuVersion, "from_environ")
+def test_config_changed_when_secrets_enabled_populates_data(mock_juju_env):
+    """
+    arrange: set up a charm with valid configuration mimicking Juju 3 and leadership for the unit.
+    act: add an smtp relation and trigger a configuration change.
+    assert: the smtp relation gets populated with the SMTP data.
+    """
+    mock_juju_env.return_value = MagicMock(has_secrets=True)
+    harness = Harness(SmtpIntegratorOperatorCharm)
+    harness.set_leader(True)
+    harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
+    harness.begin()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
     harness.add_relation("smtp", "example")
+    harness.charm.on.config_changed.emit()
     data = harness.model.get_relation("smtp").data[harness.model.app]
     assert data["host"] == MINIMAL_CHARM_CONFIG_WITH_PASSWORD["host"]
     assert data["port"] == str(MINIMAL_CHARM_CONFIG_WITH_PASSWORD["port"])
@@ -162,7 +199,7 @@ def test_relation_joined_when_secrets_enabled_doesnt_populate_password(mock_juju
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
     harness.begin()
-    harness.charm.on.config_changed.emit()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
     harness.add_relation("smtp", "example")
     data = harness.model.get_relation("smtp").data[harness.model.app]
     assert "password" not in data
@@ -180,7 +217,6 @@ def test_relation_joined_when_no_secrets_enabled(mock_juju_env):
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG_WITH_PASSWORD)
     harness.begin()
-    harness.charm.on.config_changed.emit()
     harness.add_relation("smtp", "example")
     data = harness.model.get_relation("smtp").data[harness.model.app]
     assert data == {}
@@ -198,7 +234,7 @@ def test_relation_joined_when_no_password_configured(mock_juju_env):
     harness.set_leader(True)
     harness.update_config(MINIMAL_CHARM_CONFIG)
     harness.begin()
-    harness.charm.on.config_changed.emit()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
     harness.add_relation("smtp", "example")
     data = harness.model.get_relation("smtp").data[harness.model.app]
     assert data["host"] == MINIMAL_CHARM_CONFIG["host"]
@@ -217,6 +253,7 @@ def test_legacy_relation_joined_when_not_leader():
     harness.set_leader(False)
     harness.update_config(MINIMAL_CHARM_CONFIG)
     harness.begin()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
     harness.charm.on.config_changed.emit()
     harness.add_relation("smtp-legacy", "example")
     data = harness.model.get_relation("smtp-legacy").data[harness.model.app]
@@ -235,6 +272,7 @@ def test_relation_joined_when_not_leader(mock_juju_env):
     harness.set_leader(False)
     harness.update_config(MINIMAL_CHARM_CONFIG)
     harness.begin()
+    harness.add_relation("smtp-peers", harness.charm.app.name)
     harness.charm.on.config_changed.emit()
     harness.add_relation("smtp", "example")
     data = harness.model.get_relation("smtp").data[harness.model.app]