diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index 45d57fef..fbe98972 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/tests/integration/database-charm/actions.yaml b/tests/integration/database-charm/actions.yaml index 226f0ff7..e569a00a 100644 --- a/tests/integration/database-charm/actions.yaml +++ b/tests/integration/database-charm/actions.yaml @@ -88,3 +88,10 @@ delete-peer-relation-field: field: type: string description: Relation field + +delete-peer-secret: + description: Delete Peer secret + params: + component: + type: string + description: app/unit diff --git a/tests/integration/database-charm/src/charm.py b/tests/integration/database-charm/src/charm.py index a256b3c6..5f4667f1 100755 --- a/tests/integration/database-charm/src/charm.py +++ b/tests/integration/database-charm/src/charm.py @@ -65,10 +65,7 @@ def __init__(self, *args): self.peer_relation_unit = DataPeerUnit( self, relation_name=PEER, - additional_secret_fields=[ - "monitor-password", - "secret-field", - ], + additional_secret_fields=["monitor-password", "secret-field", "my-unit-secret"], secret_field_name=SECRET_INTERNAL_LABEL, deleted_label=SECRET_DELETED_LABEL, ) @@ -108,6 +105,7 @@ def __init__(self, *args): self.framework.observe( self.on.delete_peer_relation_field_action, self._on_delete_peer_relation_field ) + self.framework.observe(self.on.delete_peer_secret_action, self._on_delete_peer_secret) def _on_change_admin_password(self, event: ActionEvent): """Change the admin password.""" @@ -332,6 +330,23 @@ def _on_delete_peer_relation_field(self, event: ActionEvent): relation = self.peer_relation_unit.relations[0] self.peer_relation_unit.delete_relation_data(relation.id, [event.params["field"]]) + def _on_delete_peer_secret(self, event: ActionEvent): + """Delete requested relation field.""" + component = event.params["component"] + + # Charms should be compatible with old vesrions, to simulate rolling upgrade + if DATA_INTERFACES_VERSION <= 17: + return + + secret = None + if component == "app": + secret = self.model.get_secret(label="database.app") + else: + secret = self.model.get_secret(label="database.unit") + + if secret: + secret.remove_all_revisions() + if __name__ == "__main__": main(DatabaseCharm) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index e1fa64f0..9a6dee0b 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -288,7 +288,7 @@ async def check_logs(ops_test: OpsTest, strings: str, limit: int = 10) -> bool: return False -async def get_secret_by_label(ops_test, label: str) -> Dict[str, str]: +async def get_secret_by_label(ops_test, label: str, owner: str = "") -> Dict[str, str]: secrets_raw = await ops_test.juju("list-secrets") secret_ids = [ secret_line.split()[0] for secret_line in secrets_raw[1].split("\n")[1:] if secret_line @@ -301,4 +301,5 @@ async def get_secret_by_label(ops_test, label: str) -> Dict[str, str]: secret_data = json.loads(secret_data_raw[1]) if label == secret_data[secret_id].get("label"): - return secret_data[secret_id]["content"]["Data"] + if not owner or owner == secret_data[secret_id].get("owner"): + return secret_data[secret_id]["content"]["Data"] diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index a60df982..025dcb87 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -162,6 +162,11 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest): # Setting and verifying two secret fields leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME) unit_name = f"{DATABASE_APP_NAME}/{leader_id}" + + # Generally we shouldn't have test decision based on pytest.mark.parametrize + # but I think this is a valid exception + owner = "database" if component == "app" else unit_name + action = await ops_test.model.units.get(unit_name).run_action( "set-peer-relation-field", **{"component": component, "field": "monitor-password", "value": "blablabla"}, @@ -174,7 +179,7 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest): ) await action.wait() - secret = await get_secret_by_label(ops_test, f"database.{component}") + secret = await get_secret_by_label(ops_test, f"database.{component}", owner) assert secret.get("monitor-password") == "blablabla" assert secret.get("secret-field") == "blablabla2" @@ -197,7 +202,7 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest): ) await action.wait() - secret = await get_secret_by_label(ops_test, f"database.{component}") + secret = await get_secret_by_label(ops_test, f"database.{component}", owner) assert not secret.get("not-a-secret") action = await ops_test.model.units.get(unit_name).run_action( @@ -212,7 +217,7 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest): ) await action.wait() - secret = await get_secret_by_label(ops_test, f"database.{component}") + secret = await get_secret_by_label(ops_test, f"database.{component}", owner) assert secret.get("secret-field") == "blablabla2" assert secret.get("monitor-password") == "#DELETED#" @@ -246,6 +251,110 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest): ) ) + # Cleanup + action = await ops_test.model.units.get(unit_name).run_action( + "delete-peer-secret", **{"component": component} + ) + await action.wait() + + +@pytest.mark.abort_on_fail +@pytest.mark.usefixtures("only_with_juju_secrets") +async def test_peer_relation_non_leader_unit_secrets(ops_test: OpsTest): + """Testing peer relation using the DataPeer class.""" + # Setting and verifying two secret fields + non_leader_unit_id = await get_non_leader_id(ops_test, DATABASE_APP_NAME) + unit_name = f"{DATABASE_APP_NAME}/{non_leader_unit_id}" + action = await ops_test.model.units.get(unit_name).run_action( + "set-peer-relation-field", + **{"component": "unit", "field": "monitor-password", "value": "blablabla"}, + ) + await action.wait() + + action = await ops_test.model.units.get(unit_name).run_action( + "set-peer-relation-field", + **{"component": "unit", "field": "secret-field", "value": "blablabla2"}, + ) + await action.wait() + + secret = await get_secret_by_label(ops_test, "database.unit", unit_name) + assert secret.get("monitor-password") == "blablabla" + assert secret.get("secret-field") == "blablabla2" + + action = await ops_test.model.units.get(unit_name).run_action( + "get-peer-relation-field", **{"component": "unit", "field": "monitor-password"} + ) + await action.wait() + assert action.results.get("value") == "blablabla" + + action = await ops_test.model.units.get(unit_name).run_action( + "get-peer-relation-field", **{"component": "unit", "field": "secret-field"} + ) + await action.wait() + assert action.results.get("value") == "blablabla2" + + # Setting and verifying a non-secret field + action = await ops_test.model.units.get(unit_name).run_action( + "set-peer-relation-field", + **{"component": "unit", "field": "not-a-secret", "value": "plain text"}, + ) + await action.wait() + + secret = await get_secret_by_label(ops_test, "database.unit", unit_name) + assert not secret.get("not-a-secret") + + action = await ops_test.model.units.get(unit_name).run_action( + "get-peer-relation-field", **{"component": "unit", "field": "not-a-secret"} + ) + await action.wait() + assert action.results.get("value") == "plain text" + + # Deleting all fields + action = await ops_test.model.units.get(unit_name).run_action( + "delete-peer-relation-field", **{"component": "unit", "field": "monitor-password"} + ) + await action.wait() + + secret = await get_secret_by_label(ops_test, "database.unit", unit_name) + assert secret.get("secret-field") == "blablabla2" + assert secret.get("monitor-password") == "#DELETED#" + + action = await ops_test.model.units.get(unit_name).run_action( + "get-peer-relation-field", **{"component": "unit", "field": "monitor-password"} + ) + await action.wait() + assert not action.results.get("value") + + action = await ops_test.model.units.get(unit_name).run_action( + "delete-peer-relation-field", **{"component": "unit", "field": "not-a-secret"} + ) + await action.wait() + + action = await ops_test.model.units.get(unit_name).run_action( + "get-peer-relation-field", **{"component": "unit", "field": "not-a-secret"} + ) + await action.wait() + assert not action.results.get("value") + + assert not ( + await get_application_relation_data( + ops_test, DATABASE_APP_NAME, "database-peers", "not-a-secret", app_or_unit="unit" + ) + ) + + # Internal secret URI is not saved on the databag + assert not ( + await get_application_relation_data( + ops_test, DATABASE_APP_NAME, "database-peers", "internal-secret", app_or_unit="unit" + ) + ) + + # Cleanup + action = await ops_test.model.units.get(unit_name).run_action( + "delete-peer-secret", **{"component": "unit"} + ) + await action.wait() + @pytest.mark.abort_on_fail async def test_peer_relation_non_leader_can_read_app_data(ops_test: OpsTest):