From 79706f401d93a4e85b729de52a81367f6d3f6451 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 25 Jun 2024 15:45:08 +1200 Subject: [PATCH] fix: adjust Harness secret behaviour to align with Juju (#1248) Adjust Harness so that the errors match what Juju does in practice (I tested each case - the results are in a table in a comment in #1229). * When Juju responds with a message including "not found", we raise `SecretNotFoundError` * When Juju responds with any other message (such as "permission denied"), we raise `ModelError` * When Juju succeeds and only fails at the completion of the hook, we raise `RuntimeError` - in production, the charm cannot catch this, but we do want to surface this error, so `RuntimeError` seems most approprate In addition: * Update the documentation to clarify when secret content is cached (this was a request from the discussion with the data platform team in Madrid) * Correct the documentation regarding when `ModelError` and when `SecretNotFoundError` will be raised with the secret methods * Don't clear the `Secret` object local cache of the secret content on `set_contents`. The contents won't change until `refresh` is used, so there's no point forcing the next call to get the content from Juju. Juju may change the responses so that there is increased consistency (probably via [this bug](https://bugs.launchpad.net/juju/+bug/2067336)) but it seems best if we fix the behaviour to match now, and then if Juju changes in the future, we can update ops then (also deciding at that time how to handle the issue that different Juju versions will have different behaviour). Fixes #1229. --- ops/charm.py | 11 ++++++--- ops/framework.py | 3 ++- ops/model.py | 57 +++++++++++++++++++++++++++++++++++++------- ops/testing.py | 29 +++++++++++++--------- test/test_model.py | 18 -------------- test/test_testing.py | 27 +++++++++++++-------- 6 files changed, 94 insertions(+), 51 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index e083bd43b..9a1d79a12 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -841,7 +841,11 @@ def __init__(self, handle: 'Handle', id: str, label: Optional[str]): @property def secret(self) -> model.Secret: - """The secret instance this event refers to.""" + """The secret instance this event refers to. + + Note that the secret content is not retrieved from the secret storage + until :meth:`Secret.get_content()` is called. + """ backend = self.framework.model._backend return model.Secret(backend=backend, id=self._id, label=self._label) @@ -901,9 +905,10 @@ class SecretRemoveEvent(SecretEvent): observers have updated to that new revision, this event will be fired to inform the secret owner that the old revision can be removed. - Typically, the charm will call + After any required cleanup, the charm should call :meth:`event.secret.remove_revision() ` to - remove the now-unused revision. + remove the now-unused revision. If the charm does not, then the event will + be emitted again, when further revisions are ready for removal. """ def __init__(self, handle: 'Handle', id: str, label: Optional[str], revision: int): diff --git a/ops/framework.py b/ops/framework.py index 661c6959d..e5caf0d8c 100644 --- a/ops/framework.py +++ b/ops/framework.py @@ -953,7 +953,8 @@ def _reemit(self, single_event_path: Optional[str] = None): logger.warning( 'Reference to ops.Object at path %s has been garbage collected ' 'between when the charm was initialised and when the event was emitted. ' - 'Make sure sure you store a reference to the observer.', observer_path + 'Make sure sure you store a reference to the observer.', + observer_path, ) if event.deferred: diff --git a/ops/model.py b/ops/model.py index d87b022dd..040ba09c6 100644 --- a/ops/model.py +++ b/ops/model.py @@ -269,13 +269,19 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) - owners set a label using ``add_secret``, whereas secret observers set a label using ``get_secret`` (see an example at :attr:`Secret.label`). + The content of the secret is retrieved, so calls to + :meth:`Secret.get_content` do not require querying the secret storage + again, unless ``refresh=True`` is used, or :meth:`Secret.set_content` + has been called. + Args: id: Secret ID if fetching by ID. label: Secret label if fetching by label (or updating it). Raises: - SecretNotFoundError: If a secret with this ID or label doesn't exist, - or the caller does not have permission to view it. + SecretNotFoundError: If a secret with this ID or label doesn't exist. + ModelError: if the charm does not have permission to access the + secret. """ if not (id or label): raise TypeError('Must provide an id or label, or both') @@ -1365,6 +1371,10 @@ def _on_secret_changed(self, event): def get_content(self, *, refresh: bool = False) -> Dict[str, str]: """Get the secret's content. + The content of the secret is cached on the :class:`Secret` object, so + subsequent calls do not require querying the secret storage again, + unless ``refresh=True`` is used, or :meth:`set_content` is called. + Returns: A copy of the secret's content dictionary. @@ -1372,6 +1382,11 @@ def get_content(self, *, refresh: bool = False) -> Dict[str, str]: refresh: If true, fetch the latest revision's content and tell Juju to update to tracking that revision. The default is to get the content of the currently-tracked revision. + + Raises: + SecretNotFoundError: if the secret no longer exists. + ModelError: if the charm does not have permission to access the + secret. """ if refresh or self._content is None: self._content = self._backend.secret_get(id=self.id, label=self.label, refresh=refresh) @@ -1381,7 +1396,13 @@ def peek_content(self) -> Dict[str, str]: """Get the content of the latest revision of this secret. This returns the content of the latest revision without updating the - tracking. + tracking. The content is not cached locally, so multiple calls will + result in multiple queries to the secret storage. + + Raises: + SecretNotFoundError: if the secret no longer exists. + ModelError: if the charm does not have permission to access the + secret. """ return self._backend.secret_get(id=self.id, label=self.label, peek=True) @@ -1389,6 +1410,10 @@ def get_info(self) -> SecretInfo: """Get this secret's information (metadata). Only secret owners can fetch this information. + + Raises: + SecretNotFoundError: if the secret no longer exists, or if the charm + does not have permission to access the secret. """ return self._backend.secret_info_get(id=self.id, label=self.label) @@ -1399,6 +1424,10 @@ def set_content(self, content: Dict[str, str]): the secret (the "observers") that a new revision is available with a :class:`ops.SecretChangedEvent `. + If the charm does not have permission to update the secret, or the + secret no longer exists, this method will succeed, but the unit will go + into error state on completion of the current Juju hook. + Args: content: A key-value mapping containing the payload of the secret, for example :code:`{"password": "foo123"}`. @@ -1407,7 +1436,8 @@ def set_content(self, content: Dict[str, str]): if self._id is None: self._id = self.get_info().id self._backend.secret_set(typing.cast(str, self.id), content=content) - self._content = None # invalidate cache so it's refetched next get_content() + # We do not need to invalidate the cache here, as the content is the + # same until `refresh` is used, at which point the cache is invalidated. def set_info( self, @@ -1422,6 +1452,10 @@ def set_info( This will not create a new secret revision (that applies only to :meth:`set_content`). Once attributes are set, they cannot be unset. + If the charm does not have permission to update the secret, or the + secret no longer exists, this method will succeed, but the unit will go + into error state on completion of the current Juju hook. + Args: label: New label to apply. description: New description to apply. @@ -1480,14 +1514,17 @@ def revoke(self, relation: 'Relation', *, unit: Optional[Unit] = None): def remove_revision(self, revision: int): """Remove the given secret revision. - This is normally called when handling + This is normally only called when handling :class:`ops.SecretRemoveEvent ` or :class:`ops.SecretExpiredEvent `. + If the charm does not have permission to remove the secret, or it no + longer exists, this method will succeed, but the unit will go into error + state on completion of the current Juju hook. + Args: - revision: The secret revision to remove. If being called from a - secret event, this should usually be set to - :attr:`SecretRemoveEvent.revision`. + revision: The secret revision to remove. This should usually be set to + :attr:`SecretRemoveEvent.revision` or :attr:`SecretExpiredEvent.revision`. """ if self._id is None: self._id = self.get_info().id @@ -1498,6 +1535,10 @@ def remove_all_revisions(self) -> None: This is called when the secret is no longer needed, for example when handling :class:`ops.RelationBrokenEvent `. + + If the charm does not have permission to remove the secret, or it no + longer exists, this method will succeed, but the unit will go into error + state on completion of the current Juju hook. """ if self._id is None: self._id = self.get_info().id diff --git a/ops/testing.py b/ops/testing.py index 8a508ddce..129e53ade 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -2741,29 +2741,30 @@ def _relation_id_to(self, remote_app: str) -> Optional[int]: return relation_id return None - def _ensure_secret_owner(self, secret: _Secret): + def _has_secret_owner_permission(self, secret: _Secret) -> bool: # For unit secrets, the owner unit has manage permissions. For app # secrets, the leader has manage permissions and other units only have # view permissions. # https://discourse.charmhub.io/t/secret-access-permissions/12627 # For user secrets the secret owner is the model, that is, - # `secret.owner_name == self.model.uuid`, only model admins have + # when `secret.owner_name == self.model.uuid`, only model admins have # manage permissions: https://juju.is/docs/juju/secret. unit_secret = secret.owner_name == self.unit_name app_secret = secret.owner_name == self.app_name if unit_secret or (app_secret and self.is_leader()): - return - raise model.SecretNotFoundError( - f'You must own secret {secret.id!r} to perform this operation' - ) + return True + return False def secret_info_get( self, *, id: Optional[str] = None, label: Optional[str] = None ) -> model.SecretInfo: secret = self._ensure_secret_id_or_label(id, label) - self._ensure_secret_owner(secret) + if not self._has_secret_owner_permission(secret): + raise model.SecretNotFoundError( + f'You must own secret {secret.id!r} to perform this operation' + ) rotates = None rotation = None @@ -2793,7 +2794,8 @@ def secret_set( rotate: Optional[model.SecretRotate] = None, ) -> None: secret = self._ensure_secret(id) - self._ensure_secret_owner(secret) + if not self._has_secret_owner_permission(secret): + raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation') if content is None: content = secret.revisions[-1].content @@ -2866,7 +2868,10 @@ def _secret_add( def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None: secret = self._ensure_secret(id) - self._ensure_secret_owner(secret) + if not self._has_secret_owner_permission(secret): + raise model.SecretNotFoundError( + f'You must own secret {secret.id!r} to perform this operation' + ) if relation_id not in secret.grants: secret.grants[relation_id] = set() @@ -2875,7 +2880,8 @@ def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None: secret = self._ensure_secret(id) - self._ensure_secret_owner(secret) + if not self._has_secret_owner_permission(secret): + raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation') if relation_id not in secret.grants: return @@ -2884,7 +2890,8 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None def secret_remove(self, id: str, *, revision: Optional[int] = None) -> None: secret = self._ensure_secret(id) - self._ensure_secret_owner(secret) + if not self._has_secret_owner_permission(secret): + raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation') if revision is not None: revisions = [r for r in secret.revisions if r.revision != revision] diff --git a/test/test_model.py b/test/test_model.py index 5c9435b12..b0e4f6206 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -3663,24 +3663,6 @@ def test_get_content_copies_dict(self, model: ops.Model, fake_script: FakeScript assert fake_script.calls(clear=True) == [['secret-get', 'secret:z', '--format=json']] - def test_set_content_invalidates_cache(self, model: ops.Model, fake_script: FakeScript): - fake_script.write('secret-get', """echo '{"foo": "bar"}'""") - fake_script.write('secret-set', """exit 0""") - - secret = self.make_secret(model, id='z') - old_content = secret.get_content() - assert old_content == {'foo': 'bar'} - secret.set_content({'new': 'content'}) - fake_script.write('secret-get', """echo '{"new": "content"}'""") - new_content = secret.get_content() - assert new_content == {'new': 'content'} - - assert fake_script.calls(clear=True) == [ - ['secret-get', 'secret:z', '--format=json'], - ['secret-set', 'secret:z', 'new=content'], - ['secret-get', 'secret:z', '--format=json'], - ] - def test_peek_content(self, model: ops.Model, fake_script: FakeScript): fake_script.write('secret-get', """echo '{"foo": "peeked"}'""") diff --git a/test/test_testing.py b/test/test_testing.py index 82ddd6e65..8ee7596dd 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -5782,9 +5782,9 @@ def test_secret_permissions_nonleader(self, request: pytest.FixtureRequest): assert secret.get_content() == {'password': '1234'} with pytest.raises(ops.model.SecretNotFoundError): secret.get_info() - with pytest.raises(ops.model.SecretNotFoundError): + with pytest.raises(RuntimeError): secret.set_content({'password': '5678'}) - with pytest.raises(ops.model.SecretNotFoundError): + with pytest.raises(RuntimeError): secret.remove_all_revisions() def test_add_user_secret(self, request: pytest.FixtureRequest): @@ -5805,7 +5805,7 @@ def test_get_user_secret_without_grant(self, request: pytest.FixtureRequest): request.addfinalizer(harness.cleanup) harness.begin() secret_id = harness.add_user_secret({'password': 'foo'}) - with pytest.raises(ops.SecretNotFoundError): + with pytest.raises(ops.ModelError): harness.model.get_secret(id=secret_id) def test_revoke_user_secret(self, request: pytest.FixtureRequest): @@ -5817,7 +5817,7 @@ def test_revoke_user_secret(self, request: pytest.FixtureRequest): secret_id = harness.add_user_secret(secret_content) harness.grant_secret(secret_id, 'webapp') harness.revoke_secret(secret_id, 'webapp') - with pytest.raises(ops.SecretNotFoundError): + with pytest.raises(ops.ModelError): harness.model.get_secret(id=secret_id) def test_set_user_secret_content(self, request: pytest.FixtureRequest): @@ -5832,15 +5832,22 @@ def test_set_user_secret_content(self, request: pytest.FixtureRequest): secret = harness.model.get_secret(id=secret_id) assert secret.get_content(refresh=True) == {'password': 'bar'} - def test_get_user_secret_info(self, request: pytest.FixtureRequest): - harness = ops.testing.Harness(EventRecorder, meta=yaml.safe_dump({'name': 'webapp'})) + def test_user_secret_permissions(self, request: pytest.FixtureRequest): + harness = ops.testing.Harness(ops.CharmBase, meta='name: database') request.addfinalizer(harness.cleanup) harness.begin() - secret_id = harness.add_user_secret({'password': 'foo'}) - harness.grant_secret(secret_id, 'webapp') - secret = harness.model.get_secret(id=secret_id) - with pytest.raises(ops.SecretNotFoundError): + + # Charms can only view a user secret. + secret_id = harness.add_user_secret({'password': '1234'}) + harness.grant_secret(secret_id, 'database') + secret = harness.charm.model.get_secret(id=secret_id) + assert secret.get_content() == {'password': '1234'} + with pytest.raises(ops.model.SecretNotFoundError): secret.get_info() + with pytest.raises(RuntimeError): + secret.set_content({'password': '5678'}) + with pytest.raises(RuntimeError): + secret.remove_all_revisions() class EventRecorder(ops.CharmBase):