[DPE-2320] fix monitor secret id parsing issue (#181)

* Fix secret id comparison (current usage: secret-changed event)
* Unittests for secret-changed event

---------

Co-authored-by: Judit Novak <judit.novak@canonical.com>

src/charm.py

@@ -4,6 +4,7 @@
 # See LICENSE file for licensing details.
 
 import logging
+import re
 import time
 from typing import Dict, List, Optional, Set
 
@@ -282,6 +283,26 @@ def _scope_opj(self, scope: Scopes):
     def _peer_data(self, scope: Scopes):
         return self.relation.data[self._scope_opj(scope)]
 
+    @staticmethod
+    def _compare_secret_ids(secret_id1: str, secret_id2: str) -> bool:
+        """Reliable comparison on secret equality.
+
+        NOTE: Secret IDs may be of any of these forms:
+         - secret://9663a790-7828-4186-8b21-2624c58b6cfe/citb87nubg2s766pab40
+         - secret:citb87nubg2s766pab40
+        """
+        if not secret_id1 or not secret_id2:
+            return False
+
+        regex = re.compile(".*[^/][/:]")
+
+        pure_id1 = regex.sub("", secret_id1)
+        pure_id2 = regex.sub("", secret_id2)
+
+        if pure_id1 and pure_id2:
+            return pure_id1 == pure_id2
+        return False
+
     # END: generic helper methods
 
     # BEGIN: charm events
@@ -533,23 +554,32 @@ def _on_set_password(self, event: ActionEvent) -> None:
         )
 
     def _on_secret_remove(self, event):
-        logging.debug(f"Secret {event._id} seems to have no observers, could be removed")
+        logging.debug(f"Secret {event.secret.id} seems to have no observers, could be removed")
 
     def _on_secret_changed(self, event):
-        secret = event.secret
+        """Handles secrets changes event.
 
-        if secret.id == self.app_peer_data.get(Config.Secrets.SECRET_INTERNAL_LABEL, None):
+        When user run set-password action, juju leader changes the password inside the database
+        and inside the secret object. This action runs the restart for monitoring tool and
+        for backup tool on non-leader units to keep them working with MongoDB. The same workflow
+        occurs on TLS certs change.
+        """
+        if self._compare_secret_ids(
+            event.secret.id, self.app_peer_data.get(Config.Secrets.SECRET_INTERNAL_LABEL)
+        ):
             scope = APP_SCOPE
-        elif secret.id == self.unit_peer_data.get(Config.Secrets.SECRET_INTERNAL_LABEL, None):
+        elif self._compare_secret_ids(
+            event.secret.id, self.unit_peer_data.get(Config.Secrets.SECRET_INTERNAL_LABEL)
+        ):
             scope = UNIT_SCOPE
         else:
-            logging.debug(
-                "Secret {event._id}:{event.secret.id} changed, but it's irrelevant for us"
-            )
+            logging.debug("Secret %s changed, but it's unknown", event.secret.id)
             return
-        logging.debug(f"Secret {event._id} for scope {scope} changed, refreshing")
+        logging.debug("Secret %s for scope %s changed, refreshing", event.secret.id, scope)
+
         self._juju_secrets_get(scope)
         self._connect_mongodb_exporter()
+        self._connect_pbm_agent()
 
     # END: actions
 

tests/integration/metrics_tests/test_metrics.py

@@ -15,6 +15,7 @@
 DATABASE_APP_NAME = "mongodb-k8s"
 MONGODB_EXPORTER_PORT = 9216
 MEDIAN_REELECTION_TIME = 12
+RESTART_TIMEOUT = 10
 
 
 @pytest.fixture(scope="module")
@@ -75,8 +76,8 @@ async def test_endpoints_new_password(ops_test: OpsTest):
     leader_unit = await ha_helpers.find_unit(ops_test, leader=True)
     action = await leader_unit.run_action("set-password", **{"username": "monitor"})
     action = await action.wait()
-    # wait for non-leader units to receive relation changed event.
-    time.sleep(3)
+    # wait for non-leader units to receive relation changed event and restart services.
+    time.sleep(RESTART_TIMEOUT)
     await ops_test.model.wait_for_idle()
 
     await verify_endpoints(ops_test, mongodb_application_name)

tests/unit/test_charm.py

@@ -700,6 +700,37 @@ def test_delete_password(self):
             self.harness.charm.remove_secret("unit", "non-existing-secret")
             assert "No secret unit:non-existing-secret" in self._caplog.text
 
+    @parameterized.expand([("app"), ("unit")])
+    @patch("charm.MongoDBCharm._connect_mongodb_exporter")
+    def test_on_secret_changed(self, scope, connect_exporter):
+        """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!"""
+        secret_id = self.harness.charm.set_secret(scope, "new-secret", "bla")
+
+        secret = self.harness.charm.model.get_secret(id=secret_id)
+
+        event = mock.Mock()
+        event.secret = secret
+        secret_id = self.harness.charm._on_secret_changed(event)
+        connect_exporter.assert_called()
+
+    @parameterized.expand([("app"), ("unit")])
+    @pytest.mark.usefixtures("use_caplog")
+    @patch("charm.MongoDBCharm._connect_mongodb_exporter")
+    def test_on_other_secret_changed(self, scope, connect_exporter):
+        """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!"""
+        # "Hack": creating a secret outside of the normal MongoDBCharm.set_secret workflow
+        scope_obj = self.harness.charm._scope_opj(scope)
+        secret = scope_obj.add_secret({"key": "value"})
+
+        event = mock.Mock()
+        event.secret = secret
+
+        with self._caplog.at_level(logging.DEBUG):
+            self.harness.charm._on_secret_changed(event)
+            assert f"Secret {secret.id} changed, but it's unknown" in self._caplog.text
+
+        connect_exporter.assert_not_called()
+
     @patch("charm.MongoDBConnection")
     @patch("charm.MongoDBCharm._connect_mongodb_exporter")
     def test_connect_to_mongo_exporter_on_set_password(self, connect_exporter, connection):