From 911eff7cd464cae6d0244630b479b3c92eace291 Mon Sep 17 00:00:00 2001 From: Mia Altieri <32723809+MiaAltieri@users.noreply.github.com> Date: Tue, 12 Dec 2023 12:47:28 -0800 Subject: [PATCH] Update libs (#314) ### sumary 1. update data interfaces lib to most recent version 2. update mongodb provider lib to only set pertinent information for mongos applications 3. update config server inteface lib to use best practices --- .github/workflows/ci.yaml | 2 +- .../data_platform_libs/v0/data_interfaces.py | 410 +++++++++++++----- .../mongodb/v0/config_server_interface.py | 82 +--- lib/charms/mongodb/v1/mongodb_provider.py | 6 +- tests/unit/test_config_server_lib.py | 12 +- tox.ini | 10 + 6 files changed, 334 insertions(+), 188 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 96203a607..be9592ac7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -67,7 +67,7 @@ jobs: build: name: Build charms - uses: canonical/data-platform-workflows/.github/workflows/build_charms_with_cache.yaml@v5.1.2 + uses: canonical/data-platform-workflows/.github/workflows/build_charms_with_cache.yaml@v7 integration-test: strategy: diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index 9071655a8..714eace46 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -298,7 +298,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): from collections import namedtuple from datetime import datetime from enum import Enum -from typing import Dict, List, Optional, Set, Union +from typing import Callable, Dict, List, Optional, Set, Tuple, Union from ops import JujuVersion, Secret, SecretInfo, SecretNotFoundError from ops.charm import ( @@ -320,7 +320,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 20 +LIBPATCH = 24 PYDEPS = ["ops>=2.0.0"] @@ -377,11 +377,24 @@ class SecretsIllegalUpdateError(SecretError): """Secrets aren't yet available for Juju version used.""" -def get_encoded_field( +def get_encoded_dict( relation: Relation, member: Union[Unit, Application], field: str -) -> Union[str, List[str], Dict[str, str]]: +) -> Optional[Dict[str, str]]: """Retrieve and decode an encoded field from relation data.""" - return json.loads(relation.data[member].get(field, "{}")) + data = json.loads(relation.data[member].get(field, "{}")) + if isinstance(data, dict): + return data + logger.error("Unexpected datatype for %s instead of dict.", str(data)) + + +def get_encoded_list( + relation: Relation, member: Union[Unit, Application], field: str +) -> Optional[List[str]]: + """Retrieve and decode an encoded field from relation data.""" + data = json.loads(relation.data[member].get(field, "[]")) + if isinstance(data, list): + return data + logger.error("Unexpected datatype for %s instead of list.", str(data)) def set_encoded_field( @@ -406,16 +419,11 @@ def diff(event: RelationChangedEvent, bucket: Union[Unit, Application]) -> Diff: keys from the event relation databag. """ # Retrieve the old data from the data key in the application relation databag. - old_data = get_encoded_field(event.relation, bucket, "data") + old_data = get_encoded_dict(event.relation, bucket, "data") if not old_data: old_data = {} - if not isinstance(old_data, dict): - # We should never get here, added to re-assure pyright - logger.error("Previous databag diff is of a wrong type.") - old_data = {} - # Retrieve the new data from the event relation databag. new_data = ( {key: value for key, value in event.relation.data[event.app].items() if key != "data"} @@ -518,14 +526,28 @@ def get_content(self) -> Dict[str, str]: """Getting cached secret content.""" if not self._secret_content: if self.meta: - self._secret_content = self.meta.get_content() + try: + self._secret_content = self.meta.get_content(refresh=True) + except (ValueError, ModelError) as err: + # https://bugs.launchpad.net/juju/+bug/2042596 + # Only triggered when 'refresh' is set + msg = "ERROR either URI or label should be used for getting an owned secret but not both" + if isinstance(err, ModelError) and msg not in str(err): + raise + # Due to: ValueError: Secret owner cannot use refresh=True + self._secret_content = self.meta.get_content() return self._secret_content def set_content(self, content: Dict[str, str]) -> None: """Setting cached secret content.""" - if self.meta: + if not self.meta: + return + + if content: self.meta.set_content(content) self._secret_content = content + else: + self.meta.remove_all_revisions() def get_info(self) -> Optional[SecretInfo]: """Wrapper function to apply the corresponding call on the Secret object within CachedSecret if any.""" @@ -622,6 +644,16 @@ def _fetch_my_specific_relation_data( """Fetch data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" raise NotImplementedError + @abstractmethod + def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: + """Update data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" + raise NotImplementedError + + @abstractmethod + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Delete data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" + raise NotImplementedError + # Internal helper methods @staticmethod @@ -688,9 +720,9 @@ def _group_secret_fields(secret_fields: List[str]) -> Dict[SecretGroup, List[str secret_fieldnames_grouped.setdefault(SecretGroup.EXTRA, []).append(key) return secret_fieldnames_grouped - def _retrieve_group_secret_contents( + def _get_group_secret_contents( self, - relation_id: int, + relation: Relation, group: SecretGroup, secret_fields: Optional[Union[Set[str], List[str]]] = None, ) -> Dict[str, str]: @@ -698,12 +730,30 @@ def _retrieve_group_secret_contents( if not secret_fields: secret_fields = [] - if (secret := self._get_relation_secret(relation_id, group)) and ( + if (secret := self._get_relation_secret(relation.id, group)) and ( secret_data := secret.get_content() ): return {k: v for k, v in secret_data.items() if k in secret_fields} return {} + @staticmethod + def _content_for_secret_group( + content: Dict[str, str], secret_fields: Set[str], group_mapping: SecretGroup + ) -> Dict[str, str]: + """Select : pairs from input, that belong to this particular Secret group.""" + if group_mapping == SecretGroup.EXTRA: + return { + k: v + for k, v in content.items() + if k in secret_fields and k not in SECRET_LABEL_MAP.keys() + } + + return { + k: v + for k, v in content.items() + if k in secret_fields and SECRET_LABEL_MAP.get(k) == group_mapping + } + @juju_secrets_only def _get_relation_secret_data( self, relation_id: int, group_mapping: SecretGroup, relation_name: Optional[str] = None @@ -713,6 +763,49 @@ def _get_relation_secret_data( if secret: return secret.get_content() + # Core operations on Relation Fields manipulations (regardless whether the field is in the databag or in a secret) + # Internal functions to be called directly from transparent public interface functions (+closely related helpers) + + def _process_secret_fields( + self, + relation: Relation, + req_secret_fields: Optional[List[str]], + impacted_rel_fields: List[str], + operation: Callable, + *args, + **kwargs, + ) -> Tuple[Dict[str, str], Set[str]]: + """Isolate target secret fields of manipulation, and execute requested operation by Secret Group.""" + result = {} + + # If the relation started on a databag, we just stay on the databag + # (Rolling upgrades may result in a relation starting on databag, getting secrets enabled on-the-fly) + # self.local_app is sufficient to check (ignored if Requires, never has secrets -- works if Provides) + fallback_to_databag = ( + req_secret_fields + and self.local_unit.is_leader() + and set(req_secret_fields) & set(relation.data[self.local_app]) + ) + + normal_fields = set(impacted_rel_fields) + if req_secret_fields and self.secrets_enabled and not fallback_to_databag: + normal_fields = normal_fields - set(req_secret_fields) + secret_fields = set(impacted_rel_fields) - set(normal_fields) + + secret_fieldnames_grouped = self._group_secret_fields(list(secret_fields)) + + for group in secret_fieldnames_grouped: + # operation() should return nothing when all goes well + if group_result := operation(relation, group, secret_fields, *args, **kwargs): + # If "meaningful" data was returned, we take it. (Some 'operation'-s only return success/failure.) + if isinstance(group_result, dict): + result.update(group_result) + else: + # If it wasn't found as a secret, let's give it a 2nd chance as "normal" field + # Needed when Juju3 Requires meets Juju2 Provider + normal_fields |= set(secret_fieldnames_grouped[group]) + return (result, normal_fields) + def _fetch_relation_data_without_secrets( self, app: Application, relation: Relation, fields: Optional[List[str]] ) -> Dict[str, str]: @@ -723,6 +816,9 @@ def _fetch_relation_data_without_secrets( This is used typically when the Provides side wants to read the Requires side's data, or when the Requires side may want to read its own data. """ + if app not in relation.data or not relation.data[app]: + return {} + if fields: return {k: relation.data[app][k] for k in fields if k in relation.data[app]} else: @@ -743,43 +839,66 @@ def _fetch_relation_data_with_secrets( Provides side itself). """ result = {} + normal_fields = [] - normal_fields = fields - if not normal_fields: - normal_fields = list(relation.data[app].keys()) - - if req_secret_fields and self.secrets_enabled: - if fields: - # Processing from what was requested - normal_fields = set(fields) - set(req_secret_fields) - secret_fields = set(fields) - set(normal_fields) - - secret_fieldnames_grouped = self._group_secret_fields(list(secret_fields)) - - for group in secret_fieldnames_grouped: - if contents := self._retrieve_group_secret_contents( - relation.id, group, secret_fields - ): - result.update(contents) - else: - # If it wasn't found as a secret, let's give it a 2nd chance as "normal" field - normal_fields |= set(secret_fieldnames_grouped[group]) - else: - # Processing from what is given, i.e. retrieving all - normal_fields = [ - f for f in relation.data[app].keys() if not self._is_secret_field(f) - ] - secret_fields = [f for f in relation.data[app].keys() if self._is_secret_field(f)] - for group in SecretGroup: - result.update( - self._retrieve_group_secret_contents(relation.id, group, req_secret_fields) - ) + if not fields: + if app not in relation.data or not relation.data[app]: + return {} + + all_fields = list(relation.data[app].keys()) + normal_fields = [field for field in all_fields if not self._is_secret_field(field)] + + # There must have been secrets there + if all_fields != normal_fields and req_secret_fields: + # So we assemble the full fields list (without 'secret-' fields) + fields = normal_fields + req_secret_fields + + if fields: + result, normal_fields = self._process_secret_fields( + relation, req_secret_fields, fields, self._get_group_secret_contents + ) # Processing "normal" fields. May include leftover from what we couldn't retrieve as a secret. - result.update({k: relation.data[app][k] for k in normal_fields if k in relation.data[app]}) + # (Typically when Juju3 Requires meets Juju2 Provides) + if normal_fields: + result.update( + self._fetch_relation_data_without_secrets(app, relation, list(normal_fields)) + ) return result - # Public methods + def _update_relation_data_without_secrets( + self, app: Application, relation: Relation, data: Dict[str, str] + ) -> None: + """Updating databag contents when no secrets are involved.""" + if app not in relation.data or relation.data[app] is None: + return + + if any(self._is_secret_field(key) for key in data.keys()): + raise SecretsIllegalUpdateError("Can't update secret {key}.") + + if relation: + relation.data[app].update(data) + + def _delete_relation_data_without_secrets( + self, app: Application, relation: Relation, fields: List[str] + ) -> None: + """Remove databag fields 'fields' from Relation.""" + if app not in relation.data or not relation.data[app]: + return + + for field in fields: + try: + relation.data[app].pop(field) + except KeyError: + logger.debug( + "Non-existing field was attempted to be removed from the databag %s, %s", + str(relation.id), + str(field), + ) + pass + + # Public interface methods + # Handling Relation Fields seamlessly, regardless if in databag or a Juju Secret def get_relation(self, relation_name, relation_id) -> Relation: """Safe way of retrieving a relation.""" @@ -790,9 +909,6 @@ def get_relation(self, relation_name, relation_id) -> Relation: "Relation %s %s couldn't be retrieved", relation_name, relation_id ) - if not relation.app: - raise DataInterfacesError("Relation's application missing") - return relation def fetch_relation_data( @@ -879,12 +995,19 @@ def fetch_my_relation_field( if relation_data := self.fetch_my_relation_data([relation_id], [field], relation_name): return relation_data.get(relation_id, {}).get(field) - # Public methods - mandatory override - - @abstractmethod + @leader_only def update_relation_data(self, relation_id: int, data: dict) -> None: """Update the data within the relation.""" - raise NotImplementedError + relation_name = self.relation_name + relation = self.get_relation(relation_name, relation_id) + return self._update_relation_data(relation, data) + + @leader_only + def delete_relation_data(self, relation_id: int, fields: List[str]) -> None: + """Remove field from the relation.""" + relation_name = self.relation_name + relation = self.get_relation(relation_name, relation_id) + return self._delete_relation_data(relation, fields) # Base DataProvides and DataRequires @@ -910,59 +1033,95 @@ def _diff(self, event: RelationChangedEvent) -> Diff: # Private methods handling secrets - @leader_only @juju_secrets_only def _add_relation_secret( - self, relation_id: int, content: Dict[str, str], group_mapping: SecretGroup - ) -> Optional[Secret]: + self, relation: Relation, content: Dict[str, str], group_mapping: SecretGroup + ) -> bool: """Add a new Juju Secret that will be registered in the relation databag.""" - relation = self.get_relation(self.relation_name, relation_id) - secret_field = self._generate_secret_field_name(group_mapping) if relation.data[self.local_app].get(secret_field): - logging.error("Secret for relation %s already exists, not adding again", relation_id) - return + logging.error("Secret for relation %s already exists, not adding again", relation.id) + return False - label = self._generate_secret_label(self.relation_name, relation_id, group_mapping) + label = self._generate_secret_label(self.relation_name, relation.id, group_mapping) secret = self.secrets.add(label, content, relation) # According to lint we may not have a Secret ID if secret.meta and secret.meta.id: relation.data[self.local_app][secret_field] = secret.meta.id - @leader_only + # Return the content that was added + return True + @juju_secrets_only def _update_relation_secret( - self, relation_id: int, content: Dict[str, str], group_mapping: SecretGroup - ): + self, relation: Relation, content: Dict[str, str], group_mapping: SecretGroup + ) -> bool: """Update the contents of an existing Juju Secret, referred in the relation databag.""" - secret = self._get_relation_secret(relation_id, group_mapping) + secret = self._get_relation_secret(relation.id, group_mapping) if not secret: - logging.error("Can't update secret for relation %s", relation_id) - return + logging.error("Can't update secret for relation %s", relation.id) + return False old_content = secret.get_content() full_content = copy.deepcopy(old_content) full_content.update(content) secret.set_content(full_content) - @staticmethod - def _secret_content_grouped( - content: Dict[str, str], secret_fields: Set[str], group_mapping: SecretGroup - ) -> Dict[str, str]: - if group_mapping == SecretGroup.EXTRA: - return { - k: v - for k, v in content.items() - if k in secret_fields and k not in SECRET_LABEL_MAP.keys() - } + # Return True on success + return True - return { - k: v - for k, v in content.items() - if k in secret_fields and SECRET_LABEL_MAP.get(k) == group_mapping - } + def _add_or_update_relation_secrets( + self, + relation: Relation, + group: SecretGroup, + secret_fields: Set[str], + data: Dict[str, str], + ) -> bool: + """Update contents for Secret group. If the Secret doesn't exist, create it.""" + secret_content = self._content_for_secret_group(data, secret_fields, group) + if self._get_relation_secret(relation.id, group): + return self._update_relation_secret(relation, secret_content, group) + else: + return self._add_relation_secret(relation, secret_content, group) + + @juju_secrets_only + def _delete_relation_secret( + self, relation: Relation, group: SecretGroup, secret_fields: List[str], fields: List[str] + ) -> bool: + """Update the contents of an existing Juju Secret, referred in the relation databag.""" + secret = self._get_relation_secret(relation.id, group) + + if not secret: + logging.error("Can't delete secret for relation %s", str(relation.id)) + return False + + old_content = secret.get_content() + new_content = copy.deepcopy(old_content) + for field in fields: + try: + new_content.pop(field) + except KeyError: + logging.error( + "Non-existing secret was attempted to be removed %s, %s", + str(relation.id), + str(field), + ) + return False + + secret.set_content(new_content) + + # Remove secret from the relation if it's fully gone + if not new_content: + field = self._generate_secret_field_name(group) + try: + relation.data[self.local_app].pop(field) + except KeyError: + pass + + # Return the content that was removed + return True # Mandatory internal overrides @@ -1004,45 +1163,42 @@ def _fetch_my_specific_relation_data( """Fetching our own relation data.""" secret_fields = None if relation.app: - secret_fields = get_encoded_field(relation, relation.app, REQ_SECRET_FIELDS) + secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) return self._fetch_relation_data_with_secrets( self.local_app, - secret_fields if isinstance(secret_fields, list) else None, + secret_fields, relation, fields, ) - # Public methods -- mandatory overrides - - @leader_only - def update_relation_data(self, relation_id: int, fields: Dict[str, str]) -> None: + def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: """Set values for fields not caring whether it's a secret or not.""" - relation = self.get_relation(self.relation_name, relation_id) - + req_secret_fields = [] if relation.app: - relation_secret_fields = get_encoded_field(relation, relation.app, REQ_SECRET_FIELDS) - else: - relation_secret_fields = [] + req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) - normal_fields = list(fields) - if relation_secret_fields and self.secrets_enabled: - normal_fields = set(fields.keys()) - set(relation_secret_fields) - secret_fields = set(fields.keys()) - set(normal_fields) + _, normal_fields = self._process_secret_fields( + relation, + req_secret_fields, + list(data), + self._add_or_update_relation_secrets, + data=data, + ) - secret_fieldnames_grouped = self._group_secret_fields(list(secret_fields)) + normal_content = {k: v for k, v in data.items() if k in normal_fields} + self._update_relation_data_without_secrets(self.local_app, relation, normal_content) - for group in secret_fieldnames_grouped: - secret_content = self._secret_content_grouped(fields, secret_fields, group) - if self._get_relation_secret(relation_id, group): - self._update_relation_secret(relation_id, secret_content, group) - else: - self._add_relation_secret(relation_id, secret_content, group) + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Delete fields from the Relation not caring whether it's a secret or not.""" + req_secret_fields = [] + if relation.app: + req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) - normal_content = {k: v for k, v in fields.items() if k in normal_fields} - relation.data[self.local_app].update( # pyright: ignore [reportGeneralTypeIssues] - normal_content + _, normal_fields = self._process_secret_fields( + relation, req_secret_fields, fields, self._delete_relation_secret, fields=fields ) + self._delete_relation_data_without_secrets(self.local_app, relation, list(normal_fields)) # Public methods - "native" @@ -1245,26 +1401,30 @@ def _fetch_my_specific_relation_data(self, relation, fields: Optional[List[str]] """Fetching our own relation data.""" return self._fetch_relation_data_without_secrets(self.local_app, relation, fields) - # Public methods -- mandatory overrides - - @leader_only - def update_relation_data(self, relation_id: int, data: dict) -> None: + def _update_relation_data(self, relation: Relation, data: dict) -> None: """Updates a set of key-value pairs in the relation. This function writes in the application data bag, therefore, only the leader unit can call it. Args: - relation_id: the identifier for a particular relation. + relation: the particular relation. data: dict containing the key-value pairs that should be updated in the relation. """ - if any(self._is_secret_field(key) for key in data.keys()): - raise SecretsIllegalUpdateError("Requires side can't update secrets.") + return self._update_relation_data_without_secrets(self.local_app, relation, data) - relation = self.charm.model.get_relation(self.relation_name, relation_id) - if relation: - relation.data[self.local_app].update(data) + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Deletes a set of fields from the relation. + + This function writes in the application data bag, therefore, + only the leader unit can call it. + + Args: + relation: the particular relation. + fields: list containing the field names that should be removed from the relation. + """ + return self._delete_relation_data_without_secrets(self.local_app, relation, fields) # General events @@ -1676,7 +1836,8 @@ def _assign_relation_alias(self, relation_id: int) -> None: # We need to set relation alias also on the application level so, # it will be accessible in show-unit juju command, executed for a consumer application unit - self.update_relation_data(relation_id, {"alias": available_aliases[0]}) + if self.local_unit.is_leader(): + self.update_relation_data(relation_id, {"alias": available_aliases[0]}) def _emit_aliased_event(self, event: RelationChangedEvent, event_name: str) -> None: """Emit an aliased event to a particular relation if it has an alias. @@ -1763,6 +1924,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: # Sets both database and extra user roles in the relation # if the roles are provided. Otherwise, sets only the database. + if not self.local_unit.is_leader(): + return + if self.extra_user_roles: self.update_relation_data( event.relation.id, @@ -2022,6 +2186,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the Kafka relation is created.""" super()._on_relation_created_event(event) + if not self.local_unit.is_leader(): + return + # Sets topic, extra user roles, and "consumer-group-prefix" in the relation relation_data = { f: getattr(self, f.replace("-", "_"), "") @@ -2194,6 +2361,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the OpenSearch relation is created.""" super()._on_relation_created_event(event) + if not self.local_unit.is_leader(): + return + # Sets both index and extra user roles in the relation if the roles are provided. # Otherwise, sets only the index. data = {"index": self.index} diff --git a/lib/charms/mongodb/v0/config_server_interface.py b/lib/charms/mongodb/v0/config_server_interface.py index ca4c210be..bf03fb0e2 100644 --- a/lib/charms/mongodb/v0/config_server_interface.py +++ b/lib/charms/mongodb/v0/config_server_interface.py @@ -35,7 +35,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 4 +LIBPATCH = 5 class ClusterProvider(Object): @@ -87,29 +87,16 @@ def _on_relation_changed(self, event) -> None: self.charm.client_relations.oversee_users(None, None) # TODO Future PR, use secrets - self.update_relation_data( - event.relation.id, - { - KEYFILE_KEY: self.charm.get_secret( - Config.Relations.APP_SCOPE, Config.Secrets.SECRET_KEYFILE_NAME - ), - CONFIG_SERVER_DB_KEY: config_server_db, - }, - ) - - def update_relation_data(self, relation_id: int, data: dict) -> None: - """Updates a set of key-value pairs in the relation. - - This function writes in the application data bag, therefore, only the leader unit can call - it. - - Args: - relation_id: the identifier for a particular relation. - data: dict containing the key-value pairs - that should be updated in the relation. - """ if self.charm.unit.is_leader(): - self.database_provides.update_relation_data(relation_id, data) + self.database_provides.update_relation_data( + event.relation.id, + { + KEYFILE_KEY: self.charm.get_secret( + Config.Relations.APP_SCOPE, Config.Secrets.SECRET_KEYFILE_NAME + ), + CONFIG_SERVER_DB_KEY: config_server_db, + }, + ) def generate_config_server_db(self) -> str: """Generates the config server database for mongos to connect to.""" @@ -136,45 +123,34 @@ def __init__( relation_name=self.relation_name, database_name=self.charm.database, extra_user_roles=self.charm.extra_user_roles, + additional_secret_fields=[KEYFILE_KEY], ) super().__init__(charm, self.relation_name) self.framework.observe( - charm.on[self.relation_name].relation_created, self._on_relation_created_event + charm.on[self.relation_name].relation_created, + self.database_requires._on_relation_created_event, ) self.framework.observe( charm.on[self.relation_name].relation_changed, self._on_relation_changed ) # TODO Future PRs handle scale down - def _on_relation_created_event(self, event): - """Sets database and extra user roles in the relation.""" - if not self.charm.unit.is_leader(): - return - - if not self.charm.database: - logger.info("Waiting for database from application") - event.defer() - return - - rel_data = {"database": self.charm.database} - if self.charm.extra_user_roles: - rel_data["extra-user-roles"] = str(self.charm.extra_user_roles) - - self.update_relation_data(event.relation.id, rel_data) - def _on_relation_changed(self, event) -> None: """Starts/restarts monogs with config server information.""" - relation_data = event.relation.data[event.app] - if not relation_data.get(KEYFILE_KEY) or not relation_data.get(CONFIG_SERVER_DB_KEY): + key_file_contents = self.database_requires.fetch_relation_field( + event.relation.id, KEYFILE_KEY + ) + config_server_db = self.database_requires.fetch_relation_field( + event.relation.id, CONFIG_SERVER_DB_KEY + ) + if not key_file_contents or not config_server_db: event.defer() self.charm.unit.status = WaitingStatus("Waiting for secrets from config-server") return - updated_keyfile = self.update_keyfile(key_file_contents=relation_data.get(KEYFILE_KEY)) - updated_config = self.update_config_server_db( - config_server_db=relation_data.get(CONFIG_SERVER_DB_KEY) - ) + updated_keyfile = self.update_keyfile(key_file_contents=key_file_contents) + updated_config = self.update_config_server_db(config_server_db=config_server_db) # avoid restarting mongos when possible if not updated_keyfile and not updated_config and self.is_mongos_running(): @@ -234,18 +210,4 @@ def update_keyfile(self, key_file_contents: str) -> bool: return True - def update_relation_data(self, relation_id: int, data: dict) -> None: - """Updates a set of key-value pairs in the relation. - - This function writes in the application data bag, therefore, only the leader unit can call - it. - - Args: - relation_id: the identifier for a particular relation. - data: dict containing the key-value pairs - that should be updated in the relation. - """ - if self.charm.unit.is_leader(): - self.database_requires.update_relation_data(relation_id, data) - # END: helper functions diff --git a/lib/charms/mongodb/v1/mongodb_provider.py b/lib/charms/mongodb/v1/mongodb_provider.py index aa634c650..691997a8b 100644 --- a/lib/charms/mongodb/v1/mongodb_provider.py +++ b/lib/charms/mongodb/v1/mongodb_provider.py @@ -31,7 +31,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 2 +LIBPATCH = 3 logger = logging.getLogger(__name__) REL_NAME = "database" @@ -267,6 +267,10 @@ def update_app_relation_data(self) -> None: username = self._get_username_from_relation_id(relation.id) password = self._get_or_set_password(relation) config = self._get_config(username, password) + # relations with the mongos server should not connect though the config-server directly + if self.charm.is_role(Config.Role.CONFIG_SERVER): + continue + if username in database_users: self.database_provides.set_endpoints( relation.id, diff --git a/tests/unit/test_config_server_lib.py b/tests/unit/test_config_server_lib.py index 2e2e83291..211be5461 100644 --- a/tests/unit/test_config_server_lib.py +++ b/tests/unit/test_config_server_lib.py @@ -1,7 +1,7 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. import unittest -from unittest.mock import patch +from unittest import mock from ops.testing import Harness @@ -22,8 +22,7 @@ def setUp(self): self.charm = self.harness.charm self.addCleanup(self.harness.cleanup) - @patch("charm.ClusterProvider.update_relation_data") - def test_on_relation_joined_failed_hook_checks(self, update_relation_data): + def test_on_relation_joined_failed_hook_checks(self): """Tests that no relation data is set when cluster joining conditions are not met.""" def is_not_config_mock_call(*args): @@ -36,7 +35,8 @@ def is_not_config_mock_call(*args): self.harness.charm.is_role = is_not_config_mock_call relation_id = self.harness.add_relation("cluster", "mongos") self.harness.add_relation_unit(relation_id, "mongos/0") - update_relation_data.assert_not_called() + self.harness.charm.cluster.database_provides.update_relation_data = mock.Mock() + self.harness.charm.cluster.database_provides.update_relation_data.assert_not_called() # fails because db has not been initialized del self.harness.charm.app_peer_data["db_initialised"] @@ -47,9 +47,9 @@ def is_config_mock_call(*args): self.harness.charm.is_role = is_config_mock_call self.harness.add_relation_unit(relation_id, "mongos/1") - update_relation_data.assert_not_called() + self.harness.charm.cluster.database_provides.update_relation_data.assert_not_called() # fails because not leader self.harness.set_leader(False) self.harness.add_relation_unit(relation_id, "mongos/2") - update_relation_data.assert_not_called() + self.harness.charm.cluster.database_provides.update_relation_data.assert_not_called() diff --git a/tox.ini b/tox.ini index edaae3a4a..3cf190a65 100644 --- a/tox.ini +++ b/tox.ini @@ -76,6 +76,7 @@ deps = pytest juju==3.2.0.1 pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released pytest-mock git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt @@ -92,6 +93,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -107,6 +109,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -122,6 +125,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -137,6 +141,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -157,6 +162,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -172,6 +178,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -187,6 +194,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -202,6 +210,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands = @@ -218,6 +227,7 @@ deps = juju==3.2.0.1 pytest-mock pytest-operator + protobuf==3.20 # temporary fix until new libjuju is released git+https://github.com/canonical/data-platform-workflows@v5.1.2\#subdirectory=python/pytest_plugins/pytest_operator_cache -r {tox_root}/requirements.txt commands =