diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index 45d57fefd..fbe989726 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -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 = 25 +LIBPATCH = 26 PYDEPS = ["ops>=2.0.0"] @@ -477,12 +477,19 @@ class CachedSecret: The data structure is precisely re-using/simulating as in the actual Secret Storage """ - def __init__(self, charm: CharmBase, label: str, secret_uri: Optional[str] = None): + def __init__( + self, + charm: CharmBase, + component: Union[Application, Unit], + label: str, + secret_uri: Optional[str] = None, + ): self._secret_meta = None self._secret_content = {} self._secret_uri = secret_uri self.label = label self.charm = charm + self.component = component def add_secret(self, content: Dict[str, str], relation: Relation) -> Secret: """Create a new secret.""" @@ -491,7 +498,7 @@ def add_secret(self, content: Dict[str, str], relation: Relation) -> Secret: "Secret is already defined with uri %s", self._secret_uri ) - secret = self.charm.app.add_secret(content, label=self.label) + secret = self.component.add_secret(content, label=self.label) if relation.app != self.charm.app: # If it's not a peer relation, grant is to be applied secret.grant(relation) @@ -523,8 +530,13 @@ def get_content(self) -> Dict[str, str]: 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): + known_model_errors = [ + "ERROR either URI or label should be used for getting an owned secret but not both", + "ERROR secret owner cannot use --refresh", + ] + if isinstance(err, ModelError) and not any( + msg in str(err) for msg in known_model_errors + ): raise # Due to: ValueError: Secret owner cannot use refresh=True self._secret_content = self.meta.get_content() @@ -550,14 +562,15 @@ def get_info(self) -> Optional[SecretInfo]: class SecretCache: """A data structure storing CachedSecret objects.""" - def __init__(self, charm): + def __init__(self, charm: CharmBase, component: Union[Application, Unit]): self.charm = charm + self.component = component self._secrets: Dict[str, CachedSecret] = {} def get(self, label: str, uri: Optional[str] = None) -> Optional[CachedSecret]: """Getting a secret from Juju Secret store or cache.""" if not self._secrets.get(label): - secret = CachedSecret(self.charm, label, uri) + secret = CachedSecret(self.charm, self.component, label, uri) if secret.meta: self._secrets[label] = secret return self._secrets.get(label) @@ -567,7 +580,7 @@ def add(self, label: str, content: Dict[str, str], relation: Relation) -> Cached if self._secrets.get(label): raise SecretAlreadyExistsError(f"Secret {label} already exists") - secret = CachedSecret(self.charm, label) + secret = CachedSecret(self.charm, self.component, label) secret.add_secret(content, relation) self._secrets[label] = secret return self._secrets[label] @@ -579,6 +592,8 @@ def add(self, label: str, content: Dict[str, str], relation: Relation) -> Cached class DataRelation(Object, ABC): """Base relation data mainpulation (abstract) class.""" + SCOPE = Scope.APP + # Local map to associate mappings with secrets potentially as a group SECRET_LABEL_MAP = { "username": SecretGroup.USER, @@ -599,8 +614,8 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: self._on_relation_changed_event, ) self._jujuversion = None - self.secrets = SecretCache(self.charm) - self.component = self.local_app + self.component = self.local_app if self.SCOPE == Scope.APP else self.local_unit + self.secrets = SecretCache(self.charm, self.component) @property def relations(self) -> List[Relation]: @@ -808,7 +823,7 @@ def _process_secret_fields( return (result, normal_fields) def _fetch_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, fields: Optional[List[str]] + self, component: Union[Application, Unit], relation: Relation, fields: Optional[List[str]] ) -> Dict[str, str]: """Fetching databag contents when no secrets are involved. @@ -817,17 +832,19 @@ 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]: + if component not in relation.data or not relation.data[component]: return {} if fields: - return {k: relation.data[app][k] for k in fields if k in relation.data[app]} + return { + k: relation.data[component][k] for k in fields if k in relation.data[component] + } else: - return dict(relation.data[app]) + return dict(relation.data[component]) def _fetch_relation_data_with_secrets( self, - app: Union[Application, Unit], + component: Union[Application, Unit], req_secret_fields: Optional[List[str]], relation: Relation, fields: Optional[List[str]] = None, @@ -843,10 +860,10 @@ def _fetch_relation_data_with_secrets( normal_fields = [] if not fields: - if app not in relation.data or not relation.data[app]: + if component not in relation.data or not relation.data[component]: return {} - all_fields = list(relation.data[app].keys()) + all_fields = list(relation.data[component].keys()) normal_fields = [field for field in all_fields if not self._is_secret_field(field)] # There must have been secrets there @@ -863,30 +880,30 @@ def _fetch_relation_data_with_secrets( # (Typically when Juju3 Requires meets Juju2 Provides) if normal_fields: result.update( - self._fetch_relation_data_without_secrets(app, relation, list(normal_fields)) + self._fetch_relation_data_without_secrets(component, relation, list(normal_fields)) ) return result def _update_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, data: Dict[str, str] + self, component: Union[Application, Unit], 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: + if component not in relation.data or relation.data[component] is None: return if relation: - relation.data[app].update(data) + relation.data[component].update(data) def _delete_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, fields: List[str] + self, component: Union[Application, Unit], relation: Relation, fields: List[str] ) -> None: """Remove databag fields 'fields' from Relation.""" - if app not in relation.data or relation.data[app] is None: + if component not in relation.data or relation.data[component] is None: return for field in fields: try: - relation.data[app].pop(field) + relation.data[component].pop(field) except KeyError: logger.error( "Non-existing field '%s' was attempted to be removed from the databag (relation ID: %s)", @@ -1311,7 +1328,7 @@ def _register_secret_to_relation( label = self._generate_secret_label(relation_name, relation_id, group) # Fetchin the Secret's meta information ensuring that it's locally getting registered with - CachedSecret(self.charm, label, secret_id).meta + CachedSecret(self.charm, self.component, label, secret_id).meta def _register_secrets_to_relation(self, relation: Relation, params_name_list: List[str]): """Make sure that secrets of the provided list are locally 'registered' from the databag. @@ -1648,9 +1665,10 @@ def fetch_relation_field( class DataPeerUnit(DataPeer): """Unit databag representation.""" + SCOPE = Scope.UNIT + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.component = self.local_unit # General events diff --git a/lib/charms/mongodb/v0/mongodb_secrets.py b/lib/charms/mongodb/v0/mongodb_secrets.py deleted file mode 100644 index 6a1589c66..000000000 --- a/lib/charms/mongodb/v0/mongodb_secrets.py +++ /dev/null @@ -1,137 +0,0 @@ -"""Secrets related helper classes/functions.""" -# Copyright 2023 Canonical Ltd. -# See LICENSE file for licensing details. - -from typing import Dict, Optional - -from ops import Secret, SecretInfo -from ops.charm import CharmBase -from ops.model import SecretNotFoundError - -from config import Config -from exceptions import SecretAlreadyExistsError - -# The unique Charmhub library identifier, never change it - -# The unique Charmhub library identifier, never change it -LIBID = "89cefc863fd747d7ace12cb508e7bec2" - -# Increment this major API version when introducing breaking changes -LIBAPI = 0 - -# Increment this PATCH version before using `charmcraft publish-lib` or reset -# to 0 if you are raising the major API version -LIBPATCH = 1 - -APP_SCOPE = Config.Relations.APP_SCOPE -UNIT_SCOPE = Config.Relations.UNIT_SCOPE -Scopes = Config.Relations.Scopes - - -def generate_secret_label(charm: CharmBase, scope: Scopes) -> str: - """Generate unique group_mappings for secrets within a relation context. - - Defined as a standalone function, as the choice on secret labels definition belongs to the - Application Logic. To be kept separate from classes below, which are simply to provide a - (smart) abstraction layer above Juju Secrets. - """ - members = [charm.app.name, scope] - return f"{'.'.join(members)}" - - -# Secret cache - - -class CachedSecret: - """Abstraction layer above direct Juju access with caching. - - The data structure is precisely re-using/simulating Juju Secrets behavior, while - also making sure not to fetch a secret multiple times within the same event scope. - """ - - def __init__(self, charm: CharmBase, label: str, secret_uri: Optional[str] = None): - self._secret_meta = None - self._secret_content = {} - self._secret_uri = secret_uri - self.label = label - self.charm = charm - - def add_secret(self, content: Dict[str, str], scope: Scopes) -> Secret: - """Create a new secret.""" - if self._secret_uri: - raise SecretAlreadyExistsError( - "Secret is already defined with uri %s", self._secret_uri - ) - - if scope == Config.Relations.APP_SCOPE: - secret = self.charm.app.add_secret(content, label=self.label) - else: - secret = self.charm.unit.add_secret(content, label=self.label) - self._secret_uri = secret.id - self._secret_meta = secret - return self._secret_meta - - @property - def meta(self) -> Optional[Secret]: - """Getting cached secret meta-information.""" - if self._secret_meta: - return self._secret_meta - - if not (self._secret_uri or self.label): - return - - try: - self._secret_meta = self.charm.model.get_secret(label=self.label) - except SecretNotFoundError: - if self._secret_uri: - self._secret_meta = self.charm.model.get_secret( - id=self._secret_uri, label=self.label - ) - return self._secret_meta - - 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() - return self._secret_content - - def set_content(self, content: Dict[str, str]) -> None: - """Setting cached secret content.""" - if self.meta: - self.meta.set_content(content) - self._secret_content = content - - def get_info(self) -> Optional[SecretInfo]: - """Wrapper function for get the corresponding call on the Secret object if any.""" - if self.meta: - return self.meta.get_info() - - -class SecretCache: - """A data structure storing CachedSecret objects.""" - - def __init__(self, charm): - self.charm = charm - self._secrets: Dict[str, CachedSecret] = {} - - def get(self, label: str, uri: Optional[str] = None) -> Optional[CachedSecret]: - """Getting a secret from Juju Secret store or cache.""" - if not self._secrets.get(label): - secret = CachedSecret(self.charm, label, uri) - if secret.meta: - self._secrets[label] = secret - return self._secrets.get(label) - - def add(self, label: str, content: Dict[str, str], scope: Scopes) -> CachedSecret: - """Adding a secret to Juju Secret.""" - if self._secrets.get(label): - raise SecretAlreadyExistsError(f"Secret {label} already exists") - - secret = CachedSecret(self.charm, label) - secret.add_secret(content, scope) - self._secrets[label] = secret - return self._secrets[label] - - -# END: Secret cache diff --git a/src/charm.py b/src/charm.py index 288f5b953..d0e49930e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -11,6 +11,7 @@ from pathlib import Path from typing import Dict, List, Optional, Set +from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit from charms.grafana_agent.v0.cos_agent import COSAgentProvider from charms.mongodb.v0.config_server_interface import ClusterProvider from charms.mongodb.v0.mongodb import ( @@ -19,7 +20,6 @@ NotReadyError, PyMongoError, ) -from charms.mongodb.v0.mongodb_secrets import SecretCache, generate_secret_label from charms.mongodb.v0.mongodb_tls import MongoDBTLS from charms.mongodb.v1.helpers import ( KEY_FILE, @@ -137,7 +137,30 @@ def __init__(self, *args): log_slots=Config.Monitoring.LOG_SLOTS, ) - self.secrets = SecretCache(self) + self.peer_relation_app = DataPeer( + self, + relation_name=Config.Relations.PEERS, + additional_secret_fields=[ + "backup-password", + "operator-password", + "monitor-password", + ], + secret_field_name=Config.Secrets.SECRET_INTERNAL_LABEL, + deleted_label=Config.Secrets.SECRET_DELETED_LABEL, + ) + self.peer_relation_unit = DataPeerUnit( + self, + relation_name=Config.Relations.PEERS, + additional_secret_fields=[ + "ca-secret", + "key-secret", + "cert-secret", + "csr-secret", + "chain-secret", + ], + secret_field_name=Config.Secrets.SECRET_INTERNAL_LABEL, + deleted_label=Config.Secrets.SECRET_DELETED_LABEL, + ) # BEGIN: properties @@ -647,20 +670,31 @@ def _on_secret_changed(self, event: SecretChangedEvent): for backup tool on non-leader units to keep them working with MongoDB. The same workflow occurs on TLS certs change. """ - label = None - if generate_secret_label(self, Config.Relations.APP_SCOPE) == event.secret.label: - label = generate_secret_label(self, Config.Relations.APP_SCOPE) + # The above two checks will have to be changed when + # https://warthogs.atlassian.net/browse/DPE-3313 is processed + if ( + self.peer_relation_app._generate_secret_label(self, Config.Relations.APP_SCOPE, None) + == event.secret.label + ): scope = APP_SCOPE - elif generate_secret_label(self, Config.Relations.UNIT_SCOPE) == event.secret.label: - label = generate_secret_label(self, Config.Relations.UNIT_SCOPE) + elif ( + self.peer_relation_unit._generate_secret_label(self, Config.Relations.UNIT_SCOPE, None) + == event.secret.label + ): scope = UNIT_SCOPE else: - logging.debug("Secret %s changed, but it's unknown", event.secret.id) + logging.debug( + "Secret ID: %s label: %s changed, but it's unknown", + event.secret.id, + event.secret.label, + ) return - logging.debug("Secret %s for scope %s changed, refreshing", event.secret.id, scope) - - # Refreshing cache - self.secrets.get(label) + logging.debug( + "Secret ID: %s label: %s scope: %s changed, refreshing", + event.secret.id, + event.secret.label, + scope, + ) # changed secrets means that the URIs used for PBM and mongodb_exporter are now out of date self._connect_mongodb_exporter() @@ -1135,14 +1169,16 @@ def _unit_ip(self, unit: Unit) -> str: def get_secret(self, scope: str, key: str) -> Optional[str]: """Get secret from the secret storage.""" - label = generate_secret_label(self, scope) - secret = self.secrets.get(label) - if not secret: + peers = self.model.get_relation(Config.Relations.PEERS) + + if not peers: return - value = secret.get_content().get(key) - if value != Config.Secrets.SECRET_DELETED_LABEL: - return value + if scope == APP_SCOPE: + value = self.peer_relation_app.fetch_my_relation_field(peers.id, key) + else: + value = self.peer_relation_unit.fetch_my_relation_field(peers.id, key) + return value def set_secret(self, scope: str, key: str, value: Optional[str]) -> Optional[str]: """Set secret in the secret storage. @@ -1153,32 +1189,31 @@ def set_secret(self, scope: str, key: str, value: Optional[str]) -> Optional[str if not value: return self.remove_secret(scope, key) - label = generate_secret_label(self, scope) - secret = self.secrets.get(label) - if not secret: - self.secrets.add(label, {key: value}, scope) + peers = self.model.get_relation(Config.Relations.PEERS) + + if not peers: + return + + label = None + if scope == APP_SCOPE: + self.peer_relation_app.update_relation_data(peers.id, {key: value}) + label = self.peer_relation_app._generate_secret_label(self, APP_SCOPE, None) else: - content = secret.get_content() - content.update({key: value}) - secret.set_content(content) + self.peer_relation_unit.update_relation_data(peers.id, {key: value}) + label = self.peer_relation_unit._generate_secret_label(self, UNIT_SCOPE, None) return label def remove_secret(self, scope, key) -> None: """Removing a secret.""" - label = generate_secret_label(self, scope) - secret = self.secrets.get(label) - - if not secret: - return + peers = self.model.get_relation(Config.Relations.PEERS) - content = secret.get_content() - - if not content.get(key) or content[key] == Config.Secrets.SECRET_DELETED_LABEL: - logger.error(f"Non-existing secret {scope}:{key} was attempted to be removed.") + if not peers: return - content[key] = Config.Secrets.SECRET_DELETED_LABEL - secret.set_content(content) + if scope == APP_SCOPE: + self.peer_relation_app.delete_relation_data(peers.id, [key]) + else: + self.peer_relation_unit.delete_relation_data(peers.id, [key]) def start_mongod_service(self): """Starts the mongod service and if necessary starts mongos. diff --git a/src/config.py b/src/config.py index 95263189a..dfe24573d 100644 --- a/src/config.py +++ b/src/config.py @@ -83,8 +83,6 @@ class Relations: class Secrets: """Secrets related constants.""" - SECRET_LABEL = "secret" - SECRET_CACHE_LABEL = "cache" SECRET_KEYFILE_NAME = "keyfile" SECRET_INTERNAL_LABEL = "internal-secret" SECRET_DELETED_LABEL = "None" diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py new file mode 100644 index 000000000..a837da309 --- /dev/null +++ b/tests/unit/conftest.py @@ -0,0 +1,15 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +from unittest.mock import PropertyMock + +import pytest + + +@pytest.fixture(autouse=True) +def ensure_secrets(mocker): + mocker.patch( + "charms.data_platform_libs.v0.data_interfaces.JujuVersion.has_secrets", + new_callable=PropertyMock, + return_value=True, + ) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index db867f039..d5fac7ff9 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -9,7 +9,13 @@ import pytest from charms.operator_libs_linux.v1 import snap -from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus +from ops.model import ( + ActiveStatus, + BlockedStatus, + MaintenanceStatus, + RelationDataTypeError, + WaitingStatus, +) from ops.testing import Harness from parameterized import parameterized from pymongo.errors import ConfigurationError, ConnectionFailure, OperationFailure @@ -724,6 +730,7 @@ def test_set_reset_existing_password_app(self): self._setup_secrets() # Getting current password + self.harness.set_leader(True) self.harness.charm.set_secret("app", "monitor-password", "bla") assert self.harness.charm.get_secret("app", "monitor-password") == "bla" @@ -739,6 +746,7 @@ def test_set_secret_returning_secret_id(self, scope): def test_set_reset_new_secret(self, scope): """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!""" # Getting current password + self.harness.set_leader(True) self.harness.charm.set_secret(scope, "new-secret", "bla") assert self.harness.charm.get_secret(scope, "new-secret") == "bla" @@ -752,7 +760,7 @@ def test_set_reset_new_secret(self, scope): @parameterized.expand([("app"), ("unit")]) def test_invalid_secret(self, scope): - with self.assertRaises(TypeError): + with self.assertRaises(RelationDataTypeError): self.harness.charm.set_secret("unit", "somekey", 1) self.harness.charm.set_secret("unit", "somekey", "") @@ -762,45 +770,57 @@ def test_invalid_secret(self, scope): def test_delete_password(self): """NOTE: currently ops.testing seems to allow for non-leader to remove secrets too!""" self._setup_secrets() + self.harness.set_leader(True) assert self.harness.charm.get_secret("app", "monitor-password") self.harness.charm.remove_secret("app", "monitor-password") assert self.harness.charm.get_secret("app", "monitor-password") is None - assert self.harness.charm.set_secret("unit", "somekey", "somesecret") - self.harness.charm.remove_secret("unit", "somekey") - assert self.harness.charm.get_secret("unit", "somekey") is None + assert self.harness.charm.set_secret("unit", "ca-secret", "somesecret") + self.harness.charm.remove_secret("unit", "ca-secret") + assert self.harness.charm.get_secret("unit", "ca-secret") is None with self._caplog.at_level(logging.ERROR): self.harness.charm.remove_secret("app", "monitor-password") assert ( - "Non-existing secret app:monitor-password was attempted to be removed." + "Non-existing secret {'monitor-password'} was attempted to be removed." in self._caplog.text ) - self.harness.charm.remove_secret("unit", "somekey") + self.harness.charm.remove_secret("unit", "ca-secret") assert ( - "Non-existing secret unit:somekey was attempted to be removed." + "Non-existing secret {'ca-secret'} was attempted to be removed." in self._caplog.text ) self.harness.charm.remove_secret("app", "non-existing-secret") assert ( - "Non-existing secret app:non-existing-secret was attempted to be removed." + "Non-existing field 'non-existing-secret' was attempted to be removed" in self._caplog.text ) - self.harness.charm.remove_secret("unit", "non-existing-secret") + self.harness.charm.remove_secret("unit", "non-existing-secret-unit") assert ( - "Non-existing secret unit:non-existing-secret was attempted to be removed." + "Non-existing field 'non-existing-secret-unit' was attempted to be removed" in self._caplog.text ) - @parameterized.expand([("app"), ("unit")]) @patch("charm.MongodbOperatorCharm._connect_mongodb_exporter") - def test_on_secret_changed(self, scope, connect_exporter): + def test_on_secret_changed(self, connect_exporter): + """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!""" + self.harness.set_leader(True) + secret_label = self.harness.charm.set_secret("app", "operator-password", "bla") + secret = self.harness.charm.model.get_secret(label=secret_label) + + event = mock.Mock() + event.secret = secret + secret_label = self.harness.charm._on_secret_changed(event) + connect_exporter.assert_called() + + @patch("charm.MongodbOperatorCharm._connect_mongodb_exporter") + def test_on_secret_changed_unit(self, connect_exporter): """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!""" - secret_label = self.harness.charm.set_secret(scope, "new-secret", "bla") + secret_label = self.harness.charm.set_secret("unit", "ca-secret", "bla") secret = self.harness.charm.model.get_secret(label=secret_label) event = mock.Mock() @@ -815,14 +835,17 @@ 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 MongodbOperatorCharm.set_secret workflow scope_obj = self.harness.charm._scope_obj(scope) - secret = scope_obj.add_secret({"key": "value"}) + secret = scope_obj.add_secret({"key": "value"}, label="some-label") 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 + assert ( + f"Secret ID: {secret.id} label: some-label changed, but it's unknown" + in self._caplog.text + ) connect_exporter.assert_not_called()