From 2d12b44155ab32e2ee73a470cd4a51817638e64d Mon Sep 17 00:00:00 2001 From: Neha Oudin <17551419+Gu1nness@users.noreply.github.com> Date: Tue, 3 Sep 2024 13:38:16 +0200 Subject: [PATCH] feat(libs): Correct use of data_interfaces (#472) Co-authored-by: Mia Altieri <32723809+MiaAltieri@users.noreply.github.com> --- .../mongodb/v0/config_server_interface.py | 34 ++++++++++++++----- tests/unit/test_config_server_lib.py | 33 ++++++++++++++++++ 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/lib/charms/mongodb/v0/config_server_interface.py b/lib/charms/mongodb/v0/config_server_interface.py index cdb733d98..84c33ba0c 100644 --- a/lib/charms/mongodb/v0/config_server_interface.py +++ b/lib/charms/mongodb/v0/config_server_interface.py @@ -11,10 +11,11 @@ from charms.data_platform_libs.v0.data_interfaces import ( DatabaseProvides, + DatabaseRequestedEvent, DatabaseRequires, ) from charms.mongodb.v1.mongos import MongosConnection -from ops.charm import CharmBase, EventBase, RelationBrokenEvent +from ops.charm import CharmBase, EventBase, RelationBrokenEvent, RelationChangedEvent from ops.framework import Object from ops.model import ( ActiveStatus, @@ -42,7 +43,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 11 +LIBPATCH = 12 class ClusterProvider(Object): @@ -57,6 +58,9 @@ def __init__( self.database_provides = DatabaseProvides(self.charm, relation_name=self.relation_name) super().__init__(charm, self.relation_name) + self.framework.observe( + self.database_provides.on.database_requested, self._on_database_requested + ) self.framework.observe( charm.on[self.relation_name].relation_changed, self._on_relation_changed ) @@ -105,8 +109,14 @@ def is_valid_mongos_integration(self) -> bool: return True - def _on_relation_changed(self, event) -> None: - """Handles providing mongos with KeyFile and hosts.""" + def _on_database_requested(self, event: DatabaseRequestedEvent | RelationChangedEvent) -> None: + """Handles the database requested event. + + The first time secrets are written to relations should be on this event. + + Note: If secrets are written for the first time on other events we risk + the chance of writing secrets in plain sight. + """ if not self.pass_hook_checks(event): if not self.is_valid_mongos_integration(): self.charm.status.set_and_share_status( @@ -116,12 +126,9 @@ def _on_relation_changed(self, event) -> None: ) logger.info("Skipping relation joined event: hook checks did not pass") return - config_server_db = self.generate_config_server_db() - # create user and set secrets for mongos relation self.charm.client_relations.oversee_users(None, None) - relation_data = { KEYFILE_KEY: self.charm.get_secret( Config.Relations.APP_SCOPE, Config.Secrets.SECRET_KEYFILE_NAME @@ -135,9 +142,20 @@ def _on_relation_changed(self, event) -> None: ) if int_tls_ca: relation_data[INT_TLS_CA_KEY] = int_tls_ca - self.database_provides.update_relation_data(event.relation.id, relation_data) + def _on_relation_changed(self, event: RelationChangedEvent) -> None: + """Handles providing mongos with KeyFile and hosts.""" + # First we need to ensure that the database requested event has run + # otherwise we risk the chance of writing secrets in plain sight. + if not self.database_provides.fetch_relation_field(event.relation.id, "database"): + logger.info("Database Requested has not run yet, skipping.") + event.defer() + return + + # TODO : This workflow is a fix until we have time for a better and complete fix (DPE-5513) + self._on_database_requested(event) + def _on_relation_broken(self, event) -> None: if self.charm.upgrade_in_progress: logger.warning( diff --git a/tests/unit/test_config_server_lib.py b/tests/unit/test_config_server_lib.py index e360e0387..98f35a4ba 100644 --- a/tests/unit/test_config_server_lib.py +++ b/tests/unit/test_config_server_lib.py @@ -56,6 +56,39 @@ def is_config_mock_call(*args): self.harness.add_relation_unit(relation_id, "mongos/2") self.harness.charm.cluster.database_provides.update_relation_data.assert_not_called() + @mock.patch("charms.mongodb.v1.mongodb_provider.MongoDBProvider.oversee_users") + @mock.patch("data_platform_helpers.version_check.CrossAppVersionChecker.is_local_charm") + @mock.patch( + "data_platform_helpers.version_check.CrossAppVersionChecker.is_integrated_to_locally_built_charm" + ) + @mock.patch("charm.get_charm_revision") + @mock.patch("ops.framework.EventBase.defer") + def test_relation_changed_fail_if_no_database_field( + self, defer, get_charm_rev, is_integrated, is_local, oversee + ): + + def is_config_mock_call(*args): + assert args == ("config-server",) + return True + + self.harness.charm.app_peer_data["db_initialised"] = "True" + self.harness.set_leader(True) + self.harness.charm.is_role = is_config_mock_call + self.harness.charm.cluster.database_provides.update_relation_data = mock.Mock() + + relation_id = self.harness.add_relation("cluster", "mongos") + self.harness.add_relation_unit(relation_id, "mongos/0") + + # Fails because there is no `database` field in the relation data + relation = self.harness.charm.model.get_relation("cluster") + self.harness.charm.on["cluster"].relation_changed.emit(relation=relation) + defer.assert_called() + + # Success, we have the for the database field. + # Note: updating the relation data here triggers a relation_changed event. + self.harness.update_relation_data(relation_id, "mongos", {"database": "test-database"}) + self.harness.charm.cluster.database_provides.update_relation_data.assert_called() + def test_update_rel_data_failed_hook_checks(self): """Tests that no relation data is set when the cluster is not ready."""