Skip to content

Commit

Permalink
fix: adjust Harness secret behaviour to align with Juju (canonical#1248)
Browse files Browse the repository at this point in the history
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 canonical#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 canonical#1229.
  • Loading branch information
tonyandrewmeyer authored and amandahla committed Jun 26, 2024
1 parent bfd1963 commit 9d45cc4
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 51 deletions.
11 changes: 8 additions & 3 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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() <ops.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):
Expand Down
3 changes: 2 additions & 1 deletion ops/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
57 changes: 49 additions & 8 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -1365,13 +1371,22 @@ 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.
Args:
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)
Expand All @@ -1381,14 +1396,24 @@ 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)

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)

Expand All @@ -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 <ops.charm.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"}`.
Expand All @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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 <ops.charm.SecretRemoveEvent>` or
:class:`ops.SecretExpiredEvent <ops.charm.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
Expand All @@ -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 <ops.charm.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
Expand Down
29 changes: 18 additions & 11 deletions ops/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand All @@ -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]
Expand Down
18 changes: 0 additions & 18 deletions test/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"}'""")

Expand Down
27 changes: 17 additions & 10 deletions test/test_testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -6002,9 +6002,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):
Expand All @@ -6025,7 +6025,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):
Expand All @@ -6037,7 +6037,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):
Expand All @@ -6052,15 +6052,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):
Expand Down

0 comments on commit 9d45cc4

Please sign in to comment.