From 0fe9f7a488e1e34c9359aac4447b86d6c9da5d58 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 18 Apr 2024 21:07:22 +1200 Subject: [PATCH 01/16] Remove deprecated functionality. --- tests/test_context.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_context.py b/tests/test_context.py index 2ca8b93a..20dc702a 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -49,6 +49,18 @@ def test_run_action(): assert e.action.id == expected_id +def test_cleanup(): + ctx = Context(MyCharm, meta={"name": "foo"}) + state = State() + + ctx.run("start", state) + assert ctx.emitted_events + + ctx.cleanup() + assert not ctx.emitted_events # and others... +>>>>>>> b1b76c0 (Remove deprecated functionality.) + + @pytest.mark.parametrize("app_name", ("foo", "bar", "george")) @pytest.mark.parametrize("unit_id", (1, 2, 42)) def test_app_name(app_name, unit_id): From a3048665284501cd0afc696eb11c01d4ee465bbc Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 2 Apr 2024 11:44:31 +1300 Subject: [PATCH 02/16] Add basic StoredState consistency checks. --- scenario/consistency_checker.py | 31 +++++++++++++++++++++++++++++++ tests/test_consistency_checker.py | 13 +++++++++++++ 2 files changed, 44 insertions(+) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 1334bbbd..61d34b16 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -658,3 +658,34 @@ def check_storedstate_consistency( f"The StoredState object {ss.owner_path}.{ss.name} should contain only simple types.", ) return Results(errors, []) + + +def check_storedstate_consistency( + *, + state: "State", + **_kwargs, # noqa: U101 +) -> Results: + """Check the internal consistency of `state.storedstate`.""" + errors = [] + + # Attribute names must be unique on each object. + names = defaultdict(list) + for ss in state.stored_state: + names[ss.owner_path].append(ss.name) + for owner, owner_names in names.items(): + if len(owner_names) != len(set(owner_names)): + errors.append( + f"{owner} has multiple StoredState objects with the same name.", + ) + + # The content must be marshallable. + for ss in state.stored_state: + # We don't need the marshalled state, just to know that it can be. + # This is the same "only simple types" check that ops does. + try: + marshal.dumps(ss.content) + except ValueError: + errors.append( + f"{ss!r} must contain only simple types.", + ) + return Results(errors, []) diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index 1b97b94b..9f89c0be 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -714,6 +714,19 @@ def test_storedstate_consistency(): } ), _Event("start"), + + +def test_storedstate_consistency(): + assert_consistent( + State( + stored_state=[ + StoredState(None, content={"foo": "bar"}), + StoredState(None, "my_stored_state", content={"foo": 1}), + StoredState("MyCharmLib", content={"foo": None}), + StoredState("OtherCharmLib", content={"foo": (1, 2, 3)}), + ] + ), + Event("start"), _CharmSpec( MyCharm, meta={ From b21679b029461beef7c790b6bc38d66ab00e5788 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 24 May 2024 12:41:56 +1200 Subject: [PATCH 03/16] Update scenario/consistency_checker.py Co-authored-by: PietroPasotti --- scenario/consistency_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 61d34b16..e16fb300 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -686,6 +686,6 @@ def check_storedstate_consistency( marshal.dumps(ss.content) except ValueError: errors.append( - f"{ss!r} must contain only simple types.", + f"The StoredState object {ss.owner_path}.{ss.name} should contain only simple types.", ) return Results(errors, []) From 2aea5f0ab8f4fa730167275c9705c81bc2d775d7 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 30 May 2024 19:40:56 +1200 Subject: [PATCH 04/16] Fix broken files. --- scenario/mocking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 6a65ef41..fee031ff 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -555,7 +555,7 @@ def action_get(self): def storage_add(self, name: str, count: int = 1): if not isinstance(count, int) or isinstance(count, bool): raise TypeError( - f"storage count must be integer, got: {count} ({type(count)})", + f"storage count must be integer, got: {count} ({type(count)}", ) if "/" in name: From 2c7db99ce8fec0f85527d132cddc46f70083d3fb Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 30 May 2024 19:43:14 +1200 Subject: [PATCH 05/16] Update scenario/mocking.py --- scenario/mocking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index fee031ff..6a65ef41 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -555,7 +555,7 @@ def action_get(self): def storage_add(self, name: str, count: int = 1): if not isinstance(count, int) or isinstance(count, bool): raise TypeError( - f"storage count must be integer, got: {count} ({type(count)}", + f"storage count must be integer, got: {count} ({type(count)})", ) if "/" in name: From 875a54e69ccc6659c4a45e395ea8817f5e8510fb Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 22 May 2024 17:09:36 +1200 Subject: [PATCH 06/16] WiP --- README.md | 39 ++++++++++++++++++++------------------- scenario/mocking.py | 44 ++++++++++++++++++++++++++++++++------------ scenario/state.py | 8 +++++--- 3 files changed, 57 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index ca0b0dc6..0f040e76 100644 --- a/README.md +++ b/README.md @@ -804,22 +804,26 @@ Scenario has secrets. Here's how you use them. state = scenario.State( secrets={ scenario.Secret( - {0: {'key': 'public'}}, - id='foo', - ), - }, + current={'key': 'public'}, + latest={'key': 'public', 'cert': 'private'}, + ) + ] ) ``` -The only mandatory arguments to Secret are its secret ID (which should be unique) and its 'contents': that is, a mapping -from revision numbers (integers) to a `str:str` dict representing the payload of the revision. +The only mandatory arguments to Secret is the `latest` contents: that is, a `str:str` mapping +representing the payload of the revision. If the unit that's handling the event +is tracking a different revision of the content, then `current` should also be +provided - if it's not, then Scenario assumes that `current` is the `latest`. +If there are other revisions of the content, simply don't include them: the +unit has no way of knowing about these. There are three cases: - the secret is owned by this app but not this unit, in which case this charm can only manage it if we are the leader - the secret is owned by this unit, in which case this charm can always manage it (leader or not) -- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it +- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it (this includes user secrets) -Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm', and that other charm has granted us view rights. +Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm' (or a user), and that other has granted us view rights. The presence of the secret in `State.secrets` entails that we have access to it, either as owners or as grantees. Therefore, if we're not owners, we must be grantees. Absence of a Secret from the known secrets list means we are not entitled to obtaining it in any way. The charm, indeed, shouldn't even know it exists. @@ -833,28 +837,25 @@ To specify a secret owned by this unit (or app): state = scenario.State( secrets={ scenario.Secret( - {0: {'key': 'private'}}, - id='foo', + latest={'key': 'private'}, owner='unit', # or 'app' + # The secret owner has granted access to the "remote" app over some relation with ID 0: remote_grants={0: {"remote"}} - # the secret owner has granted access to the "remote" app over some relation with ID 0 - ), - }, + ) + } ) ``` -To specify a secret owned by some other application and give this unit (or app) access to it: +To specify a secret owned by some other application, or a user secret, and give this unit (or app) access to it: ```python state = scenario.State( secrets={ scenario.Secret( - {0: {'key': 'public'}}, - id='foo', + latest={'key': 'public'}, # owner=None, which is the default - revision=0, # the revision that this unit (or app) is currently tracking - ), - }, + ) + } ) ``` diff --git a/scenario/mocking.py b/scenario/mocking.py index 6a65ef41..3425911f 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -2,7 +2,6 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. import datetime -import random import shutil from io import StringIO from pathlib import Path @@ -45,6 +44,7 @@ Storage, _EntityStatus, _port_cls_by_protocol, + _generate_secret_id, _RawPortProtocolLiteral, _RawStatusLiteral, ) @@ -212,11 +212,6 @@ def _get_secret(self, id=None, label=None): # instantiation that either an id or a label are set, and raise a TypeError if not. raise RuntimeError("need id or label.") - @staticmethod - def _generate_secret_id(): - id = "".join(map(str, [random.choice(list(range(10))) for _ in range(20)])) - return f"secret:{id}" - def _check_app_data_access(self, is_app: bool): if not isinstance(is_app, bool): raise TypeError("is_app parameter to relation_get must be a boolean") @@ -368,10 +363,10 @@ def secret_add( ) -> str: from scenario.state import Secret - secret_id = self._generate_secret_id() + secret_id = _generate_secret_id() secret = Secret( id=secret_id, - contents={0: content}, + latest=content, label=label, description=description, expire=expire, @@ -411,7 +406,7 @@ def secret_get( secret = self._get_secret(id, label) juju_version = self._context.juju_version if not (juju_version == "3.1.7" or juju_version >= "3.3.1"): - # in this medieval juju chapter, + # In this medieval Juju chapter, # secret owners always used to track the latest revision. # ref: https://bugs.launchpad.net/juju/+bug/2037120 if secret.owner is not None: @@ -422,8 +417,9 @@ def secret_get( revision = max(secret.contents.keys()) if refresh: secret._set_revision(revision) + return secret.latest - return secret.contents[revision] + return secret.current def secret_info_get( self, @@ -493,13 +489,37 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None del secret.remote_grants[relation_id] def secret_remove(self, id: str, *, revision: Optional[int] = None): + # If revision that is not the tracked or latest one is removed, then it + # doesn't make any difference to the charm, and this is not reflected + # in the output state. If a test needs to verify that this remove was + # called then it needs to mock the remove call, because this doesn't + # change the state *as it is visible to the unit executing the charm*. + # If the *tracked* revision, which is not also the latest, is removed, + # then the secret is still accessible in the current hook (from the + # input state), but afterwards the unit will always get SecretNotFound + # when doing secret-get, even when using refresh=True. + # TODO: Juju will hopefully change things to make it possible to fix this: + # https://bugs.launchpad.net/juju/+bug/2063519 + # If the *latest* revision is removed, then when using secret-get with + # peek SecretNotFound is returned, even if there are older revisions. + # secret-info will still show the same revision number. If using refresh + # or set, a "state is changing too quickly" error is returned. + # TODO: Is it possible to get out of this state? secret = self._get_secret(id) self._check_can_manage_secret(secret) if revision: - del secret.contents[revision] + # This is the latest revision. + # if revision == TODO: + # secret.latest.clear() + # This is the currently tracked revision. + if revision == secret.revision: + secret.current.clear() + # else this is some other revision: we don't need to do anything. else: - secret.contents.clear() + if secret.current: + secret.current.clear() + secret.latest.clear() def relation_remote_app_name( self, diff --git a/scenario/state.py b/scenario/state.py index 9362b2c2..fd6c5a45 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -5,7 +5,10 @@ import dataclasses import datetime import inspect +import random import re +import string +import warnings from collections import namedtuple from enum import Enum from itertools import chain @@ -284,7 +287,7 @@ class Secret(_max_posargs(1)): owner: Literal["unit", "app", None] = None # what revision is currently tracked by this charm. Only meaningful if owner=False - revision: int = 0 + revision: int = 1 # mapping from relation IDs to remote unit/apps to which this secret has been granted. # Only applicable if owner @@ -312,9 +315,8 @@ def _update_metadata( rotate: Optional[SecretRotate] = None, ): """Update the metadata.""" - revision = max(self.contents.keys()) if content: - self.contents[revision + 1] = content + self.latest = content # bypass frozen dataclass if label: From 66858a75fea026fdf2ff7faff309e71847952ceb Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 31 May 2024 17:28:42 +1200 Subject: [PATCH 07/16] More WiP. --- scenario/mocking.py | 33 +++-- scenario/state.py | 42 ++++-- tests/test_e2e/test_secrets.py | 233 +++++++++++++-------------------- 3 files changed, 140 insertions(+), 168 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 3425911f..8dec84ad 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -44,7 +44,6 @@ Storage, _EntityStatus, _port_cls_by_protocol, - _generate_secret_id, _RawPortProtocolLiteral, _RawStatusLiteral, ) @@ -363,9 +362,7 @@ def secret_add( ) -> str: from scenario.state import Secret - secret_id = _generate_secret_id() secret = Secret( - id=secret_id, latest=content, label=label, description=description, @@ -374,7 +371,7 @@ def secret_add( owner=owner, ) secrets = set(self._state.secrets) - secrets.add(secret) + secret_id = secrets.add(secret) self._state._update_secrets(frozenset(secrets)) return secret_id @@ -412,11 +409,9 @@ def secret_get( if secret.owner is not None: refresh = True - revision = secret.revision if peek or refresh: - revision = max(secret.contents.keys()) if refresh: - secret._set_revision(revision) + secret._track_latest_revision() return secret.latest return secret.current @@ -435,7 +430,7 @@ def secret_info_get( return SecretInfo( id=secret.id, label=secret.label, - revision=max(secret.contents), + revision=secret.latest_revision, expires=secret.expire, rotation=secret.rotate, rotates=None, # not implemented yet. @@ -454,6 +449,11 @@ def secret_set( secret = self._get_secret(id, label) self._check_can_manage_secret(secret) + if content == secret.latest: + logger.warning( + f"secret {id} contents set to the existing value: new revision created but not needed", + ) + secret._update_metadata( content=content, label=label, @@ -489,11 +489,18 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None del secret.remote_grants[relation_id] def secret_remove(self, id: str, *, revision: Optional[int] = None): + secret = self._get_secret(id) + self._check_can_manage_secret(secret) + # Removing all revisions is modelled as no content. + # If revision that is not the tracked or latest one is removed, then it # doesn't make any difference to the charm, and this is not reflected # in the output state. If a test needs to verify that this remove was # called then it needs to mock the remove call, because this doesn't # change the state *as it is visible to the unit executing the charm*. + if revision not in (secret.tracked_revision, secret.latest_revision): + return + # If the *tracked* revision, which is not also the latest, is removed, # then the secret is still accessible in the current hook (from the # input state), but afterwards the unit will always get SecretNotFound @@ -505,16 +512,18 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None): # secret-info will still show the same revision number. If using refresh # or set, a "state is changing too quickly" error is returned. # TODO: Is it possible to get out of this state? - secret = self._get_secret(id) - self._check_can_manage_secret(secret) if revision: + if revision == secret.tracked_revision: + # TODO We really need to remove the secret from the state somehow. + pass # This is the latest revision. # if revision == TODO: # secret.latest.clear() # This is the currently tracked revision. - if revision == secret.revision: - secret.current.clear() + if revision == secret.tracked_revision: + # TODO: figure out what to do here. + pass # else this is some other revision: we don't need to do anything. else: if secret.current: diff --git a/scenario/state.py b/scenario/state.py index fd6c5a45..1ba420eb 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -8,7 +8,6 @@ import random import re import string -import warnings from collections import namedtuple from enum import Enum from itertools import chain @@ -271,12 +270,21 @@ def _to_ops(self) -> CloudSpec_Ops: ) +def _generate_secret_id(): + # This doesn't account for collisions, but the odds are so low that it + # should not be possible in any realistic test run. + secret_id = "".join( + random.choice(string.ascii_lowercase + string.digits) for _ in range(20) + ) + return f"secret:{secret_id}" + + @dataclasses.dataclass(frozen=True) class Secret(_max_posargs(1)): - # mapping from revision IDs to each revision's contents - contents: Dict[int, "RawSecretRevisionContents"] + latest: "RawSecretRevisionContents" + current: Optional["RawSecretRevisionContents"] = None - id: str + id: str = dataclasses.field(default_factory=_generate_secret_id) # CAUTION: ops-created Secrets (via .add_secret()) will have a canonicalized # secret id (`secret:` prefix) # but user-created ones will not. Using post-init to patch it in feels bad, but requiring the user to @@ -287,7 +295,11 @@ class Secret(_max_posargs(1)): owner: Literal["unit", "app", None] = None # what revision is currently tracked by this charm. Only meaningful if owner=False - revision: int = 1 + tracked_revision: int = 1 + + # TODO: enforce that latest_revision >= tracked_revision + # what revision is the latest for this secret. + latest_revision: int = 1 # mapping from relation IDs to remote unit/apps to which this secret has been granted. # Only applicable if owner @@ -301,10 +313,15 @@ class Secret(_max_posargs(1)): def __hash__(self) -> int: return hash(self.id) - def _set_revision(self, revision: int): - """Set a new tracked revision.""" + def __post_init__(self): + if self.current is None: + # bypass frozen dataclass + object.__setattr__(self, "current", self.latest) + + def _track_latest_revision(self): + """Set the current revision to the tracked revision.""" # bypass frozen dataclass - object.__setattr__(self, "revision", revision) + object.__setattr__(self, "tracked_revision", self.latest_revision) def _update_metadata( self, @@ -315,10 +332,10 @@ def _update_metadata( rotate: Optional[SecretRotate] = None, ): """Update the metadata.""" - if content: - self.latest = content - # bypass frozen dataclass + object.__setattr__(self, "latest_revision", self.latest_revision + 1) + if content: + object.__setattr__(self, "latest", content) if label: object.__setattr__(self, "label", label) if description: @@ -1348,6 +1365,9 @@ def _update_secrets(self, new_secrets: FrozenSet[Secret]): # bypass frozen dataclass object.__setattr__(self, "secrets", new_secrets) + def _update_secrets(): + pass + def with_can_connect(self, container_name: str, can_connect: bool) -> "State": def replacer(container: Container): if container.name == container_name: diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 85fda55c..d313b184 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -38,84 +38,77 @@ def test_get_secret_no_secret(mycharm): def test_get_secret(mycharm): ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret({"a": "b"}) with ctx.manager( - state=State(secrets={Secret(id="foo", contents={0: {"a": "b"}})}), + state=State(secrets={secret}), event=ctx.on.update_status(), ) as mgr: - assert mgr.charm.model.get_secret(id="foo").get_content()["a"] == "b" + assert mgr.charm.model.get_secret(id=secret.id).get_content()["a"] == "b" @pytest.mark.parametrize("owner", ("app", "unit")) def test_get_secret_get_refresh(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + current={"a": "b"}, + latest={"a": "c"}, + owner=owner, + ) with ctx.manager( ctx.on.update_status(), - State( - secrets={ - Secret( - id="foo", - contents={ - 0: {"a": "b"}, - 1: {"a": "c"}, - }, - owner=owner, - ) - } - ), + State(secrets={secret}), ) as mgr: charm = mgr.charm - assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" + assert ( + charm.model.get_secret(id=secret.id).get_content(refresh=True)["a"] == "c" + ) @pytest.mark.parametrize("app", (True, False)) def test_get_secret_nonowner_peek_update(mycharm, app): ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + current={"a": "b"}, + latest={"a": "c"}, + ) with ctx.manager( ctx.on.update_status(), State( leader=app, - secrets={ - Secret( - id="foo", - contents={ - 0: {"a": "b"}, - 1: {"a": "c"}, - }, - ), - }, + secrets={secret}, ), ) as mgr: charm = mgr.charm - assert charm.model.get_secret(id="foo").get_content()["a"] == "b" - assert charm.model.get_secret(id="foo").peek_content()["a"] == "c" - assert charm.model.get_secret(id="foo").get_content()["a"] == "b" + assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" + assert charm.model.get_secret(id=secret.id).peek_content()["a"] == "c" + assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" - assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" - assert charm.model.get_secret(id="foo").get_content()["a"] == "c" + assert ( + charm.model.get_secret(id=secret.id).get_content(refresh=True)["a"] == "c" + ) + assert charm.model.get_secret(id=secret.id).get_content()["a"] == "c" @pytest.mark.parametrize("owner", ("app", "unit")) def test_get_secret_owner_peek_update(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + current={"a": "b"}, + latest={"a": "c"}, + owner=owner, + ) with ctx.manager( ctx.on.update_status(), State( - secrets={ - Secret( - id="foo", - contents={ - 0: {"a": "b"}, - 1: {"a": "c"}, - }, - owner=owner, - ) - } + secrets={secret}, ), ) as mgr: charm = mgr.charm - assert charm.model.get_secret(id="foo").get_content()["a"] == "b" - assert charm.model.get_secret(id="foo").peek_content()["a"] == "c" - assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" + assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" + assert charm.model.get_secret(id=secret.id).peek_content()["a"] == "c" + assert ( + charm.model.get_secret(id=secret.id).get_content(refresh=True)["a"] == "c" + ) @pytest.mark.parametrize("owner", ("app", "unit")) @@ -144,11 +137,8 @@ def test_secret_changed_owner_evt_fails(mycharm, owner): def test_consumer_events_failures(mycharm, evt_suffix, revision): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - id="foo", - contents={ - 0: {"a": "b"}, - 1: {"a": "c"}, - }, + current={"a": "b"}, + latest={"a": "c"}, ) kwargs = {"secret": secret} if revision is not None: @@ -179,8 +169,8 @@ def test_add(mycharm, app): def test_set_legacy_behaviour(mycharm): # in juju < 3.1.7, secret owners always used to track the latest revision. # ref: https://bugs.launchpad.net/juju/+bug/2037120 - rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} ctx = Context(mycharm, meta={"name": "local"}, juju_version="3.1.6") + rev1, rev2 = {"foo": "bar"}, {"foo": "baz", "qux": "roz"} with ctx.manager( ctx.on.update_status(), State(), @@ -205,26 +195,14 @@ def test_set_legacy_behaviour(mycharm): == rev2 ) - secret.set_content(rev3) state_out = mgr.run() - secret = charm.model.get_secret(label="mylabel") - assert ( - secret.get_content() - == secret.peek_content() - == secret.get_content(refresh=True) - == rev3 - ) - assert state_out.get_secret(label="mylabel").contents == { - 0: rev1, - 1: rev2, - 2: rev3, - } + assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 def test_set(mycharm): - rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} ctx = Context(mycharm, meta={"name": "local"}) + rev1, rev2 = {"foo": "bar"}, {"foo": "baz", "qux": "roz"} with ctx.manager( ctx.on.update_status(), State(), @@ -242,21 +220,14 @@ def test_set(mycharm): assert secret.get_content() == rev1 assert secret.peek_content() == secret.get_content(refresh=True) == rev2 - secret.set_content(rev3) state_out = mgr.run() - assert secret.get_content() == rev2 - assert secret.peek_content() == secret.get_content(refresh=True) == rev3 - assert state_out.get_secret(label="mylabel").contents == { - 0: rev1, - 1: rev2, - 2: rev3, - } + assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 def test_set_juju33(mycharm): - rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} ctx = Context(mycharm, meta={"name": "local"}, juju_version="3.3.1") + rev1, rev2 = {"foo": "bar"}, {"foo": "baz", "qux": "roz"} with ctx.manager( ctx.on.update_status(), State(), @@ -270,44 +241,32 @@ def test_set_juju33(mycharm): assert secret.peek_content() == rev2 assert secret.get_content(refresh=True) == rev2 - secret.set_content(rev3) state_out = mgr.run() - assert secret.get_content() == rev2 - assert secret.peek_content() == rev3 - assert secret.get_content(refresh=True) == rev3 - assert state_out.get_secret(label="mylabel").contents == { - 0: rev1, - 1: rev2, - 2: rev3, - } + assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 @pytest.mark.parametrize("app", (True, False)) def test_meta(mycharm, app): ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + owner="app" if app else "unit", + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + latest={"a": "b"}, + ) with ctx.manager( ctx.on.update_status(), State( leader=True, - secrets={ - Secret( - owner="app" if app else "unit", - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - contents={ - 0: {"a": "b"}, - }, - ) - }, + secrets=[secret], ), ) as mgr: charm = mgr.charm assert charm.model.get_secret(label="mylabel") - secret = charm.model.get_secret(id="foo") + secret = charm.model.get_secret(id=secret.id) info = secret.get_info() assert secret.label is None @@ -326,31 +285,27 @@ def test_secret_permission_model(mycharm, leader, owner): ) ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + latest={"a": "b"}, + ) + secret_id = secret.id with ctx.manager( ctx.on.update_status(), State( leader=leader, - secrets={ - Secret( - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - owner=owner, - contents={ - 0: {"a": "b"}, - }, - ) - }, + secrets=[secret], ), ) as mgr: - secret = mgr.charm.model.get_secret(id="foo") + secret = mgr.charm.model.get_secret(id=secret_id) assert secret.get_content()["a"] == "b" assert secret.peek_content() assert secret.get_content(refresh=True) # can always view - secret: ops_Secret = mgr.charm.model.get_secret(id="foo") + secret: ops_Secret = mgr.charm.model.get_secret(id=secret_id) if expect_manage: assert secret.get_content() @@ -379,22 +334,18 @@ def test_grant(mycharm, app): ctx = Context( mycharm, meta={"name": "local", "requires": {"foo": {"interface": "bar"}}} ) + secret = Secret( + owner="unit", + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + latest={"a": "b"}, + ) with ctx.manager( ctx.on.update_status(), State( relations=[Relation("foo", "remote")], - secrets={ - Secret( - owner="unit", - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - contents={ - 0: {"a": "b"}, - }, - ) - }, + secrets={secret}, ), ) as mgr: charm = mgr.charm @@ -412,19 +363,15 @@ def test_update_metadata(mycharm): exp = datetime.datetime(2050, 12, 12) ctx = Context(mycharm, meta={"name": "local"}) + secret = Secret( + owner="unit", + label="mylabel", + latest={"a": "b"}, + ) with ctx.manager( ctx.on.update_status(), State( - secrets={ - Secret( - owner="unit", - id="foo", - label="mylabel", - contents={ - 0: {"a": "b"}, - }, - ) - }, + secrets={secret}, ), ) as mgr: secret = mgr.charm.model.get_secret(label="mylabel") @@ -464,29 +411,26 @@ def _on_start(self, _): def test_grant_nonowner(mycharm): - def post_event(charm: CharmBase): - secret = charm.model.get_secret(id="foo") + secret = Secret( + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + latest={"a": "b"}, + ) + secret_id = secret.id + def post_event(charm: CharmBase): + secret = charm.model.get_secret(id=secret_id) secret = charm.model.get_secret(label="mylabel") foo = charm.model.get_relation("foo") with pytest.raises(ModelError): secret.grant(relation=foo) - out = trigger( + trigger( State( relations={Relation("foo", "remote")}, - secrets={ - Secret( - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - contents={ - 0: {"a": "b"}, - }, - ) - }, + secrets={secret}, ), "update_status", mycharm, @@ -497,8 +441,7 @@ def post_event(charm: CharmBase): def test_add_grant_revoke_remove(): class GrantingCharm(CharmBase): - def __init__(self, *args): - super().__init__(*args) + pass ctx = Context( GrantingCharm, meta={"name": "foo", "provides": {"bar": {"interface": "bar"}}} From f1ce8589eccf5ae6711612996582213487aa1c85 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 31 May 2024 17:29:43 +1200 Subject: [PATCH 08/16] Copy _update_secrets from the other branch. --- scenario/state.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scenario/state.py b/scenario/state.py index 1ba420eb..62d7c746 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -1365,8 +1365,10 @@ def _update_secrets(self, new_secrets: FrozenSet[Secret]): # bypass frozen dataclass object.__setattr__(self, "secrets", new_secrets) - def _update_secrets(): - pass + def _update_secrets(self, new_secrets: List[Secret]): + """Update the current secrets.""" + # bypass frozen dataclass + object.__setattr__(self, "secrets", new_secrets) def with_can_connect(self, container_name: str, can_connect: bool) -> "State": def replacer(container: Container): From 825bbd9413a2fa406d391c3173490c856823022d Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 2 Aug 2024 13:03:38 +1200 Subject: [PATCH 09/16] Simplify secret management. --- README.md | 22 +++++ scenario/consistency_checker.py | 31 ------- scenario/context.py | 2 + scenario/mocking.py | 84 +++++++++---------- scenario/state.py | 29 +++---- tests/test_consistency_checker.py | 29 ++----- tests/test_context.py | 12 --- tests/test_context_on.py | 18 ++-- tests/test_e2e/test_secrets.py | 135 +++++++++++++++++++++++------- 9 files changed, 195 insertions(+), 167 deletions(-) diff --git a/README.md b/README.md index 0f040e76..a25842c3 100644 --- a/README.md +++ b/README.md @@ -859,6 +859,28 @@ state = scenario.State( ) ``` +When handling the `secret-expired` and `secret-remove` events, the charm must remove the specified revision of the secret. For `secret-remove`, the revision will no longer be in the `State`, because it's no longer in use (which is why the `secret-remove` event was triggered). To ensure that the charm is removing the secret, check the context for the history of secret removal: + +```python +class SecretCharm(ops.CharmBase): + def __init__(self, framework): + super().__init__(framework) + self.framework.observe(self.on.secret_remove, self._on_secret_remove) + + def _on_secret_remove(self, event): + event.secret.remove_revision(event.revision) + + +ctx = Context(SecretCharm, meta={"name": "foo"}) +secret = Secret({"password": "xxxxxxxx"}, owner="app") +old_revision = 42 +state = ctx.run( + ctx.on.secret_remove(secret, revision=old_revision), + State(leader=True, secrets={secret}) +) +assert ctx.secret_removal_history == [old_revision] +``` + ## StoredState Scenario can simulate StoredState. You can define it on the input side as: diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index e16fb300..1334bbbd 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -658,34 +658,3 @@ def check_storedstate_consistency( f"The StoredState object {ss.owner_path}.{ss.name} should contain only simple types.", ) return Results(errors, []) - - -def check_storedstate_consistency( - *, - state: "State", - **_kwargs, # noqa: U101 -) -> Results: - """Check the internal consistency of `state.storedstate`.""" - errors = [] - - # Attribute names must be unique on each object. - names = defaultdict(list) - for ss in state.stored_state: - names[ss.owner_path].append(ss.name) - for owner, owner_names in names.items(): - if len(owner_names) != len(set(owner_names)): - errors.append( - f"{owner} has multiple StoredState objects with the same name.", - ) - - # The content must be marshallable. - for ss in state.stored_state: - # We don't need the marshalled state, just to know that it can be. - # This is the same "only simple types" check that ops does. - try: - marshal.dumps(ss.content) - except ValueError: - errors.append( - f"The StoredState object {ss.owner_path}.{ss.name} should contain only simple types.", - ) - return Results(errors, []) diff --git a/scenario/context.py b/scenario/context.py index 6dcf910a..1136148c 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -376,6 +376,7 @@ class Context: - :attr:`app_status_history`: record of the app statuses the charm has set - :attr:`unit_status_history`: record of the unit statuses the charm has set - :attr:`workload_version_history`: record of the workload versions the charm has set + - :attr:`secret_removal_history`: record of the secret revisions the charm has removed - :attr:`emitted_events`: record of the events (including custom) that the charm has processed This allows you to write assertions not only on the output state, but also, to some @@ -551,6 +552,7 @@ def __init__( self.app_status_history: List["_EntityStatus"] = [] self.unit_status_history: List["_EntityStatus"] = [] self.workload_version_history: List[str] = [] + self.secret_removal_history: List[int] = [] self.emitted_events: List[EventBase] = [] self.requested_storages: Dict[str, int] = {} diff --git a/scenario/mocking.py b/scenario/mocking.py index 8dec84ad..942d4d56 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -197,14 +197,14 @@ def _get_secret(self, id=None, label=None): if canonicalize_id(s.id) == canonicalize_id(id) ] if not secrets: - raise SecretNotFoundError(id) + raise SecretNotFoundError(id) from None return secrets[0] elif label: - secrets = [s for s in self._state.secrets if s.label == label] - if not secrets: - raise SecretNotFoundError(label) - return secrets[0] + try: + return self._state.get_secret(label=label) + except KeyError: + raise SecretNotFoundError(label) from None else: # if all goes well, this should never be reached. ops.model.Secret will check upon @@ -371,9 +371,9 @@ def secret_add( owner=owner, ) secrets = set(self._state.secrets) - secret_id = secrets.add(secret) + secrets.add(secret) self._state._update_secrets(frozenset(secrets)) - return secret_id + return secret.id def _check_can_manage_secret( self, @@ -414,6 +414,7 @@ def secret_get( secret._track_latest_revision() return secret.latest + assert secret.current is not None return secret.current def secret_info_get( @@ -430,7 +431,7 @@ def secret_info_get( return SecretInfo( id=secret.id, label=secret.label, - revision=secret.latest_revision, + revision=secret._latest_revision, expires=secret.expire, rotation=secret.rotate, rotates=None, # not implemented yet. @@ -450,8 +451,12 @@ def secret_set( self._check_can_manage_secret(secret) if content == secret.latest: + # In Juju 3.6 and higher, this is a no-op, but it's good to warn + # charmers if they are doing this, because it's not generally good + # practice. + # https://bugs.launchpad.net/juju/+bug/2069238 logger.warning( - f"secret {id} contents set to the existing value: new revision created but not needed", + f"secret {id} contents set to the existing value: new revision not needed", ) secret._update_metadata( @@ -491,44 +496,33 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None def secret_remove(self, id: str, *, revision: Optional[int] = None): secret = self._get_secret(id) self._check_can_manage_secret(secret) - # Removing all revisions is modelled as no content. - - # If revision that is not the tracked or latest one is removed, then it - # doesn't make any difference to the charm, and this is not reflected - # in the output state. If a test needs to verify that this remove was - # called then it needs to mock the remove call, because this doesn't - # change the state *as it is visible to the unit executing the charm*. - if revision not in (secret.tracked_revision, secret.latest_revision): + + # Removing all revisions means that the secret is removed, so is no + # longer in the state. + if revision is None: + secrets = set(self._state.secrets) + secrets.remove(secret) + self._state._update_secrets(frozenset(secrets)) return - # If the *tracked* revision, which is not also the latest, is removed, - # then the secret is still accessible in the current hook (from the - # input state), but afterwards the unit will always get SecretNotFound - # when doing secret-get, even when using refresh=True. - # TODO: Juju will hopefully change things to make it possible to fix this: - # https://bugs.launchpad.net/juju/+bug/2063519 - # If the *latest* revision is removed, then when using secret-get with - # peek SecretNotFound is returned, even if there are older revisions. - # secret-info will still show the same revision number. If using refresh - # or set, a "state is changing too quickly" error is returned. - # TODO: Is it possible to get out of this state? - - if revision: - if revision == secret.tracked_revision: - # TODO We really need to remove the secret from the state somehow. - pass - # This is the latest revision. - # if revision == TODO: - # secret.latest.clear() - # This is the currently tracked revision. - if revision == secret.tracked_revision: - # TODO: figure out what to do here. - pass - # else this is some other revision: we don't need to do anything. - else: - if secret.current: - secret.current.clear() - secret.latest.clear() + # Juju does not prevent removing the tracked or latest revision, but it + # is always a mistake to do this. Rather than having the state model a + # secret where the tracked/latest revision cannot be retrieved but the + # secret still exists, we raise instead, so that charms know that there + # is a problem with their code. + if revision in (secret._tracked_revision, secret._latest_revision): + raise ValueError( + "Charms should not remove the latest revision of a secret. " + "Add a new revision with `set_content()` instead, and the previous " + "revision will be garbage collected when no longer used.", + ) + + # For all other revisions, the content is not visible to the charm + # (this is as designed: the secret is being removed, so it should no + # longer be in use). That means that the state does not need to be + # modified - however, unit tests should verify that the remove call was + # executed, so we track that in a history list in the context. + self._context.secret_removal_history.append(revision) def relation_remote_app_name( self, diff --git a/scenario/state.py b/scenario/state.py index 62d7c746..f474fa46 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -285,22 +285,11 @@ class Secret(_max_posargs(1)): current: Optional["RawSecretRevisionContents"] = None id: str = dataclasses.field(default_factory=_generate_secret_id) - # CAUTION: ops-created Secrets (via .add_secret()) will have a canonicalized - # secret id (`secret:` prefix) - # but user-created ones will not. Using post-init to patch it in feels bad, but requiring the user to - # add the prefix manually every time seems painful as well. # indicates if the secret is owned by THIS unit, THIS app or some other app/unit. # if None, the implication is that the secret has been granted to this unit. owner: Literal["unit", "app", None] = None - # what revision is currently tracked by this charm. Only meaningful if owner=False - tracked_revision: int = 1 - - # TODO: enforce that latest_revision >= tracked_revision - # what revision is the latest for this secret. - latest_revision: int = 1 - # mapping from relation IDs to remote unit/apps to which this secret has been granted. # Only applicable if owner remote_grants: Dict[int, Set[str]] = dataclasses.field(default_factory=dict) @@ -310,6 +299,12 @@ class Secret(_max_posargs(1)): expire: Optional[datetime.datetime] = None rotate: Optional[SecretRotate] = None + # what revision is currently tracked by this charm. Only meaningful if owner=False + _tracked_revision: int = 1 + + # what revision is the latest for this secret. + _latest_revision: int = 1 + def __hash__(self) -> int: return hash(self.id) @@ -321,7 +316,8 @@ def __post_init__(self): def _track_latest_revision(self): """Set the current revision to the tracked revision.""" # bypass frozen dataclass - object.__setattr__(self, "tracked_revision", self.latest_revision) + object.__setattr__(self, "_tracked_revision", self._latest_revision) + object.__setattr__(self, "current", self.latest) def _update_metadata( self, @@ -333,7 +329,9 @@ def _update_metadata( ): """Update the metadata.""" # bypass frozen dataclass - object.__setattr__(self, "latest_revision", self.latest_revision + 1) + object.__setattr__(self, "_latest_revision", self._latest_revision + 1) + # TODO: if this is done twice in the same hook, then Juju ignores the + # first call, it doesn't continue to update like this does. if content: object.__setattr__(self, "latest", content) if label: @@ -1365,11 +1363,6 @@ def _update_secrets(self, new_secrets: FrozenSet[Secret]): # bypass frozen dataclass object.__setattr__(self, "secrets", new_secrets) - def _update_secrets(self, new_secrets: List[Secret]): - """Update the current secrets.""" - # bypass frozen dataclass - object.__setattr__(self, "secrets", new_secrets) - def with_can_connect(self, container_name: str, can_connect: bool) -> "State": def replacer(container: Container): if container.name == container_name: diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index 9f89c0be..4587224a 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -303,7 +303,7 @@ def test_config_secret_old_juju(juju_version): @pytest.mark.parametrize("bad_v", ("1.0", "0", "1.2", "2.35.42", "2.99.99", "2.99")) def test_secrets_jujuv_bad(bad_v): - secret = Secret("secret:foo", {0: {"a": "b"}}) + secret = Secret(latest={"a": "b"}) assert_inconsistent( State(secrets={secret}), _Event("bar"), @@ -312,14 +312,14 @@ def test_secrets_jujuv_bad(bad_v): ) assert_inconsistent( State(secrets={secret}), - secret.changed_event, + _Event("secret_changed", secret=secret), _CharmSpec(MyCharm, {}), bad_v, ) assert_inconsistent( State(), - secret.changed_event, + _Event("secret_changed", secret=secret), _CharmSpec(MyCharm, {}), bad_v, ) @@ -328,7 +328,7 @@ def test_secrets_jujuv_bad(bad_v): @pytest.mark.parametrize("good_v", ("3.0", "3.1", "3", "3.33", "4", "100")) def test_secrets_jujuv_bad(good_v): assert_consistent( - State(secrets={Secret(id="secret:foo", contents={0: {"a": "b"}})}), + State(secrets={Secret(latest={"a": "b"})}), _Event("bar"), _CharmSpec(MyCharm, {}), good_v, @@ -336,14 +336,14 @@ def test_secrets_jujuv_bad(good_v): def test_secret_not_in_state(): - secret = Secret(id="secret:foo", contents={"a": "b"}) + secret = Secret(latest={"a": "b"}) assert_inconsistent( State(), _Event("secret_changed", secret=secret), _CharmSpec(MyCharm, {}), ) assert_consistent( - State(secrets=[secret]), + State(secrets={secret}), _Event("secret_changed", secret=secret), _CharmSpec(MyCharm, {}), ) @@ -714,19 +714,6 @@ def test_storedstate_consistency(): } ), _Event("start"), - - -def test_storedstate_consistency(): - assert_consistent( - State( - stored_state=[ - StoredState(None, content={"foo": "bar"}), - StoredState(None, "my_stored_state", content={"foo": 1}), - StoredState("MyCharmLib", content={"foo": None}), - StoredState("OtherCharmLib", content={"foo": (1, 2, 3)}), - ] - ), - Event("start"), _CharmSpec( MyCharm, meta={ @@ -737,9 +724,7 @@ def test_storedstate_consistency(): assert_inconsistent( State( stored_states={ - StoredState( - owner_path=None, content={"secret": Secret(id="foo", contents={})} - ) + StoredState(owner_path=None, content={"secret": Secret(latest={})}) } ), _Event("start"), diff --git a/tests/test_context.py b/tests/test_context.py index 20dc702a..2ca8b93a 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -49,18 +49,6 @@ def test_run_action(): assert e.action.id == expected_id -def test_cleanup(): - ctx = Context(MyCharm, meta={"name": "foo"}) - state = State() - - ctx.run("start", state) - assert ctx.emitted_events - - ctx.cleanup() - assert not ctx.emitted_events # and others... ->>>>>>> b1b76c0 (Remove deprecated functionality.) - - @pytest.mark.parametrize("app_name", ("foo", "bar", "george")) @pytest.mark.parametrize("unit_id", (1, 2, 42)) def test_app_name(app_name, unit_id): diff --git a/tests/test_context_on.py b/tests/test_context_on.py index 151bc303..6f749d4b 100644 --- a/tests/test_context_on.py +++ b/tests/test_context_on.py @@ -81,10 +81,8 @@ def test_simple_events(event_name, event_kind): ) def test_simple_secret_events(as_kwarg, event_name, event_kind, owner): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) - secret = scenario.Secret( - id="secret:123", contents={0: {"password": "xxxx"}}, owner=owner - ) - state_in = scenario.State(secrets=[secret]) + secret = scenario.Secret(latest={"password": "xxxx"}, owner=owner) + state_in = scenario.State(secrets={secret}) # These look like: # ctx.run(ctx.on.secret_changed(secret=secret), state) # The secret must always be passed because the same event name is used for @@ -114,11 +112,11 @@ def test_simple_secret_events(as_kwarg, event_name, event_kind, owner): def test_revision_secret_events(event_name, event_kind): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - id="secret:123", - contents={42: {"password": "yyyy"}, 43: {"password": "xxxx"}}, + current={"password": "yyyy"}, + latest={"password": "xxxx"}, owner="app", ) - state_in = scenario.State(secrets=[secret]) + state_in = scenario.State(secrets={secret}) # These look like: # ctx.run(ctx.on.secret_expired(secret=secret, revision=revision), state) # The secret and revision must always be passed because the same event name @@ -137,11 +135,11 @@ def test_revision_secret_events(event_name, event_kind): def test_revision_secret_events_as_positional_arg(event_name): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - id="secret:123", - contents={42: {"password": "yyyy"}, 43: {"password": "xxxx"}}, + current={"password": "yyyy"}, + latest={"password": "xxxx"}, owner=None, ) - state_in = scenario.State(secrets=[secret]) + state_in = scenario.State(secrets={secret}) with pytest.raises(TypeError): ctx.run(getattr(ctx.on, event_name)(secret, 42), state_in) diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index d313b184..a7f79694 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -36,9 +36,10 @@ def test_get_secret_no_secret(mycharm): assert mgr.charm.model.get_secret(label="foo") -def test_get_secret(mycharm): +@pytest.mark.parametrize("owner", ("app", "unit")) +def test_get_secret(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) - secret = Secret({"a": "b"}) + secret = Secret({"a": "b"}, owner=owner) with ctx.manager( state=State(secrets={secret}), event=ctx.on.update_status(), @@ -81,6 +82,7 @@ def test_get_secret_nonowner_peek_update(mycharm, app): charm = mgr.charm assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" assert charm.model.get_secret(id=secret.id).peek_content()["a"] == "c" + # Verify that the peek has not refreshed: assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" assert ( @@ -106,6 +108,8 @@ def test_get_secret_owner_peek_update(mycharm, owner): charm = mgr.charm assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" assert charm.model.get_secret(id=secret.id).peek_content()["a"] == "c" + # Verify that the peek has not refreshed: + assert charm.model.get_secret(id=secret.id).get_content()["a"] == "b" assert ( charm.model.get_secret(id=secret.id).get_content(refresh=True)["a"] == "c" ) @@ -115,11 +119,8 @@ def test_get_secret_owner_peek_update(mycharm, owner): def test_secret_changed_owner_evt_fails(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - id="foo", - contents={ - 0: {"a": "b"}, - 1: {"a": "c"}, - }, + current={"a": "b"}, + latest={"a": "c"}, owner=owner, ) with pytest.raises(ValueError): @@ -162,7 +163,7 @@ def test_add(mycharm, app): assert mgr.output.secrets secret = mgr.output.get_secret(label="mylabel") - assert secret.contents[0] == {"foo": "bar"} + assert secret.latest == secret.current == {"foo": "bar"} assert secret.label == "mylabel" @@ -197,7 +198,11 @@ def test_set_legacy_behaviour(mycharm): state_out = mgr.run() - assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 + assert ( + state_out.get_secret(label="mylabel").current + == state_out.get_secret(label="mylabel").latest + == rev2 + ) def test_set(mycharm): @@ -216,13 +221,20 @@ def test_set(mycharm): == rev1 ) + # TODO: if this is done in the same event hook, it's more complicated + # than this. Figure out what we should do here. + # Also the next test, for Juju 3.3 secret.set_content(rev2) assert secret.get_content() == rev1 assert secret.peek_content() == secret.get_content(refresh=True) == rev2 state_out = mgr.run() - assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 + assert ( + state_out.get_secret(label="mylabel").current + == state_out.get_secret(label="mylabel").latest + == rev2 + ) def test_set_juju33(mycharm): @@ -243,24 +255,28 @@ def test_set_juju33(mycharm): state_out = mgr.run() - assert state_out.get_secret(label="mylabel").current == state_out.get_secret(label="mylabel").latest == rev2 + assert ( + state_out.get_secret(label="mylabel").current + == state_out.get_secret(label="mylabel").latest + == rev2 + ) @pytest.mark.parametrize("app", (True, False)) def test_meta(mycharm, app): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( + {"a": "b"}, owner="app" if app else "unit", label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, - latest={"a": "b"}, ) with ctx.manager( ctx.on.update_status(), State( leader=True, - secrets=[secret], + secrets={secret}, ), ) as mgr: charm = mgr.charm @@ -286,27 +302,26 @@ def test_secret_permission_model(mycharm, leader, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( + {"a": "b"}, label="mylabel", + owner=owner, description="foobarbaz", rotate=SecretRotate.HOURLY, - latest={"a": "b"}, ) secret_id = secret.id with ctx.manager( ctx.on.update_status(), State( leader=leader, - secrets=[secret], + secrets={secret}, ), ) as mgr: - secret = mgr.charm.model.get_secret(id=secret_id) + # can always view + secret: ops_Secret = mgr.charm.model.get_secret(id=secret_id) assert secret.get_content()["a"] == "b" assert secret.peek_content() assert secret.get_content(refresh=True) - # can always view - secret: ops_Secret = mgr.charm.model.get_secret(id=secret_id) - if expect_manage: assert secret.get_content() assert secret.peek_content() @@ -335,11 +350,11 @@ def test_grant(mycharm, app): mycharm, meta={"name": "local", "requires": {"foo": {"interface": "bar"}}} ) secret = Secret( + {"a": "b"}, owner="unit", label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, - latest={"a": "b"}, ) with ctx.manager( ctx.on.update_status(), @@ -364,9 +379,9 @@ def test_update_metadata(mycharm): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( + {"a": "b"}, owner="unit", label="mylabel", - latest={"a": "b"}, ) with ctx.manager( ctx.on.update_status(), @@ -403,7 +418,7 @@ def _on_start(self, _): secret = self.unit.add_secret({"foo": "bar"}) secret.grant(self.model.relations["bar"][0]) - state = State(leader=leader, relations=[Relation("bar")]) + state = State(leader=leader, relations={Relation("bar")}) ctx = Context( GrantingCharm, meta={"name": "foo", "provides": {"bar": {"interface": "bar"}}} ) @@ -412,10 +427,10 @@ def _on_start(self, _): def test_grant_nonowner(mycharm): secret = Secret( + {"a": "b"}, label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, - latest={"a": "b"}, ) secret_id = secret.id @@ -480,7 +495,71 @@ class GrantingCharm(CharmBase): secret = charm.model.get_secret(label="mylabel") secret.remove_all_revisions() - assert not mgr.output.get_secret(label="mylabel").contents # secret wiped + with pytest.raises(KeyError): + mgr.output.get_secret(label="mylabel") + + +def test_secret_removed_event(): + class SecretCharm(CharmBase): + def __init__(self, framework): + super().__init__(framework) + self.framework.observe(self.on.secret_remove, self._on_secret_remove) + + def _on_secret_remove(self, event): + event.secret.remove_revision(event.revision) + + ctx = Context(SecretCharm, meta={"name": "foo"}) + secret = Secret({"a": "b"}, owner="app") + old_revision = 42 + state = ctx.run( + ctx.on.secret_remove(secret, revision=old_revision), + State(leader=True, secrets={secret}), + ) + assert secret in state.secrets + assert ctx.secret_removal_history == [old_revision] + + +def test_secret_expired_event(): + class SecretCharm(CharmBase): + def __init__(self, framework): + super().__init__(framework) + self.framework.observe(self.on.secret_expired, self._on_secret_expired) + + def _on_secret_expired(self, event): + event.secret.set_content({"password": "newpass"}) + event.secret.remove_revision(event.revision) + + ctx = Context(SecretCharm, meta={"name": "foo"}) + secret = Secret({"password": "oldpass"}, owner="app") + old_revision = 42 + state = ctx.run( + ctx.on.secret_expired(secret, revision=old_revision), + State(leader=True, secrets={secret}), + ) + assert state.get_secret(id=secret.id).latest == {"password": "newpass"} + assert ctx.secret_removal_history == [old_revision] + + +def test_remove_bad_revision(): + class SecretCharm(CharmBase): + def __init__(self, framework): + super().__init__(framework) + self.framework.observe(self.on.secret_remove, self._on_secret_remove) + + def _on_secret_remove(self, event): + with pytest.raises(ValueError): + event.secret.remove_revision(event.revision) + + ctx = Context(SecretCharm, meta={"name": "foo"}) + secret = Secret({"a": "b"}, owner="app") + ctx.run( + ctx.on.secret_remove(secret, revision=secret._latest_revision), + State(leader=True, secrets={secret}), + ) + ctx.run( + ctx.on.secret_remove(secret, revision=secret._tracked_revision), + State(leader=True, secrets={secret}), + ) def test_no_additional_positional_arguments(): @@ -490,12 +569,10 @@ def test_no_additional_positional_arguments(): def test_default_values(): contents = {"foo": "bar"} - id = "secret:1" - secret = Secret(contents, id=id) - assert secret.contents == contents - assert secret.id == id + secret = Secret(contents) + assert secret.latest == secret.current == contents + assert secret.id.startswith("secret:") assert secret.label is None - assert secret.revision == 0 assert secret.description is None assert secret.owner is None assert secret.rotate is None From 39ea1c5bb0bdfd5512a5777e5e0ecaf5f51954b8 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 5 Aug 2024 16:40:44 +1200 Subject: [PATCH 10/16] 'payload' should be 'content'. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a25842c3..363367d8 100644 --- a/README.md +++ b/README.md @@ -812,7 +812,7 @@ state = scenario.State( ``` The only mandatory arguments to Secret is the `latest` contents: that is, a `str:str` mapping -representing the payload of the revision. If the unit that's handling the event +representing the content of the revision. If the unit that's handling the event is tracking a different revision of the content, then `current` should also be provided - if it's not, then Scenario assumes that `current` is the `latest`. If there are other revisions of the content, simply don't include them: the From 0d2cf9fc85d8955b6998544f8b2581ee6e834902 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 5 Aug 2024 16:45:15 +1200 Subject: [PATCH 11/16] Small adjustments, per review. --- README.md | 2 +- scenario/context.py | 56 ++-------------------------------- scenario/mocking.py | 4 +-- tests/test_e2e/test_secrets.py | 4 +-- 4 files changed, 7 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index 363367d8..ecf31dc6 100644 --- a/README.md +++ b/README.md @@ -878,7 +878,7 @@ state = ctx.run( ctx.on.secret_remove(secret, revision=old_revision), State(leader=True, secrets={secret}) ) -assert ctx.secret_removal_history == [old_revision] +assert ctx.removed_secret_revisions == [old_revision] ``` ## StoredState diff --git a/scenario/context.py b/scenario/context.py index 1136148c..d3efedc5 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -376,7 +376,7 @@ class Context: - :attr:`app_status_history`: record of the app statuses the charm has set - :attr:`unit_status_history`: record of the unit statuses the charm has set - :attr:`workload_version_history`: record of the workload versions the charm has set - - :attr:`secret_removal_history`: record of the secret revisions the charm has removed + - :attr:`removed_secret_revisions`: record of the secret revisions the charm has removed - :attr:`emitted_events`: record of the events (including custom) that the charm has processed This allows you to write assertions not only on the output state, but also, to some @@ -442,48 +442,6 @@ def __init__( ): """Represents a simulated charm's execution context. - It is the main entry point to running a scenario test. - - It contains: the charm source code being executed, the metadata files associated with it, - a charm project repository root, and the juju version to be simulated. - - After you have instantiated Context, typically you will call one of `run()` or - `run_action()` to execute the charm once, write any assertions you like on the output - state returned by the call, write any assertions you like on the Context attributes, - then discard the Context. - Each Context instance is in principle designed to be single-use: - Context is not cleaned up automatically between charm runs. - You can call `.clear()` to do some clean up, but we don't guarantee all state will be gone. - - Any side effects generated by executing the charm, that are not rightful part of the State, - are in fact stored in the Context: - - ``juju_log``: record of what the charm has sent to juju-log - - ``app_status_history``: record of the app statuses the charm has set - - ``unit_status_history``: record of the unit statuses the charm has set - - ``workload_version_history``: record of the workload versions the charm has set - - ``emitted_events``: record of the events (including custom ones) that the charm has - processed - - This allows you to write assertions not only on the output state, but also, to some - extent, on the path the charm took to get there. - - A typical scenario test will look like: - - >>> from scenario import Context, State - >>> from ops import ActiveStatus - >>> from charm import MyCharm, MyCustomEvent # noqa - >>> - >>> def test_foo(): - >>> # Arrange: set the context up - >>> c = Context(MyCharm) - >>> # Act: prepare the state and emit an event - >>> state_out = c.run(c.update_status(), State()) - >>> # Assert: verify the output state is what you think it should be - >>> assert state_out.unit_status == ActiveStatus('foobar') - >>> # Assert: verify the Context contains what you think it should - >>> assert len(c.emitted_events) == 4 - >>> assert isinstance(c.emitted_events[3], MyCustomEvent) - :arg charm_type: the CharmBase subclass to call ``ops.main()`` on. :arg meta: charm metadata to use. Needs to be a valid metadata.yaml format (as a dict). If none is provided, we will search for a ``metadata.yaml`` file in the charm root. @@ -498,16 +456,6 @@ def __init__( :arg app_trusted: whether the charm has Juju trust (deployed with ``--trust`` or added with ``juju trust``). Defaults to False. :arg charm_root: virtual charm root the charm will be executed with. - If the charm, say, expects a `./src/foo/bar.yaml` file present relative to the - execution cwd, you need to use this. E.g.: - - >>> import scenario - >>> import tempfile - >>> virtual_root = tempfile.TemporaryDirectory() - >>> local_path = Path(local_path.name) - >>> (local_path / 'foo').mkdir() - >>> (local_path / 'foo' / 'bar.yaml').write_text('foo: bar') - >>> scenario.Context(... charm_root=virtual_root).run(...) """ if not any((meta, actions, config)): @@ -552,7 +500,7 @@ def __init__( self.app_status_history: List["_EntityStatus"] = [] self.unit_status_history: List["_EntityStatus"] = [] self.workload_version_history: List[str] = [] - self.secret_removal_history: List[int] = [] + self.removed_secret_revisions: List[int] = [] self.emitted_events: List[EventBase] = [] self.requested_storages: Dict[str, int] = {} diff --git a/scenario/mocking.py b/scenario/mocking.py index 942d4d56..296581de 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -197,7 +197,7 @@ def _get_secret(self, id=None, label=None): if canonicalize_id(s.id) == canonicalize_id(id) ] if not secrets: - raise SecretNotFoundError(id) from None + raise SecretNotFoundError(id) return secrets[0] elif label: @@ -522,7 +522,7 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None): # longer be in use). That means that the state does not need to be # modified - however, unit tests should verify that the remove call was # executed, so we track that in a history list in the context. - self._context.secret_removal_history.append(revision) + self._context.removed_secret_revisions.append(revision) def relation_remote_app_name( self, diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index a7f79694..f6b1f9b4 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -516,7 +516,7 @@ def _on_secret_remove(self, event): State(leader=True, secrets={secret}), ) assert secret in state.secrets - assert ctx.secret_removal_history == [old_revision] + assert ctx.removed_secret_revisions == [old_revision] def test_secret_expired_event(): @@ -537,7 +537,7 @@ def _on_secret_expired(self, event): State(leader=True, secrets={secret}), ) assert state.get_secret(id=secret.id).latest == {"password": "newpass"} - assert ctx.secret_removal_history == [old_revision] + assert ctx.removed_secret_revisions == [old_revision] def test_remove_bad_revision(): From 2ec2256928a8b941997971ca9771df5a7615fe42 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 5 Aug 2024 16:45:44 +1200 Subject: [PATCH 12/16] Apply suggestions from code review Co-authored-by: PietroPasotti --- scenario/mocking.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 296581de..d64086cd 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -514,13 +514,13 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None): raise ValueError( "Charms should not remove the latest revision of a secret. " "Add a new revision with `set_content()` instead, and the previous " - "revision will be garbage collected when no longer used.", + "revision will be cleaned up by the secret owner when no longer in use.", ) # For all other revisions, the content is not visible to the charm # (this is as designed: the secret is being removed, so it should no # longer be in use). That means that the state does not need to be - # modified - however, unit tests should verify that the remove call was + # modified - however, unit tests should be able to verify that the remove call was # executed, so we track that in a history list in the context. self._context.removed_secret_revisions.append(revision) From 15cf19077a65a3ac8e6796508bb07ee9bb2fbc66 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 6 Aug 2024 18:56:00 +1200 Subject: [PATCH 13/16] Adjustments to the naming, per review. --- README.md | 21 ++++++++-------- scenario/mocking.py | 10 ++++---- scenario/state.py | 14 +++++------ tests/test_consistency_checker.py | 10 ++++---- tests/test_context_on.py | 10 ++++---- tests/test_e2e/test_secrets.py | 40 +++++++++++++++---------------- 6 files changed, 52 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index ecf31dc6..f8f81835 100644 --- a/README.md +++ b/README.md @@ -804,19 +804,20 @@ Scenario has secrets. Here's how you use them. state = scenario.State( secrets={ scenario.Secret( - current={'key': 'public'}, - latest={'key': 'public', 'cert': 'private'}, + tracked_content={'key': 'public'}, + latest_content={'key': 'public', 'cert': 'private'}, ) ] ) ``` -The only mandatory arguments to Secret is the `latest` contents: that is, a `str:str` mapping -representing the content of the revision. If the unit that's handling the event -is tracking a different revision of the content, then `current` should also be -provided - if it's not, then Scenario assumes that `current` is the `latest`. -If there are other revisions of the content, simply don't include them: the -unit has no way of knowing about these. +The only mandatory arguments to Secret is the `tracked_content` dict: a `str:str` +mapping representing the content of the revision. If there is a newer revision +of the content than the one the unit that's handling the event is tracking, then +`latest_content` should also be provided - if it's not, then Scenario assumes +that `latest_content` is the `tracked_content`. If there are other revisions of +the content, simply don't include them: the unit has no way of knowing about +these. There are three cases: - the secret is owned by this app but not this unit, in which case this charm can only manage it if we are the leader @@ -837,7 +838,7 @@ To specify a secret owned by this unit (or app): state = scenario.State( secrets={ scenario.Secret( - latest={'key': 'private'}, + {'key': 'private'}, owner='unit', # or 'app' # The secret owner has granted access to the "remote" app over some relation with ID 0: remote_grants={0: {"remote"}} @@ -852,7 +853,7 @@ To specify a secret owned by some other application, or a user secret, and give state = scenario.State( secrets={ scenario.Secret( - latest={'key': 'public'}, + {'key': 'public'}, # owner=None, which is the default ) } diff --git a/scenario/mocking.py b/scenario/mocking.py index d64086cd..5284dbd3 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -363,7 +363,7 @@ def secret_add( from scenario.state import Secret secret = Secret( - latest=content, + content, label=label, description=description, expire=expire, @@ -412,10 +412,10 @@ def secret_get( if peek or refresh: if refresh: secret._track_latest_revision() - return secret.latest + assert secret.latest_content is not None + return secret.latest_content - assert secret.current is not None - return secret.current + return secret.tracked_content def secret_info_get( self, @@ -450,7 +450,7 @@ def secret_set( secret = self._get_secret(id, label) self._check_can_manage_secret(secret) - if content == secret.latest: + if content == secret.latest_content: # In Juju 3.6 and higher, this is a no-op, but it's good to warn # charmers if they are doing this, because it's not generally good # practice. diff --git a/scenario/state.py b/scenario/state.py index f474fa46..76819ee5 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -280,9 +280,9 @@ def _generate_secret_id(): @dataclasses.dataclass(frozen=True) -class Secret(_max_posargs(1)): - latest: "RawSecretRevisionContents" - current: Optional["RawSecretRevisionContents"] = None +class Secret(_max_posargs(2)): + tracked_content: "RawSecretRevisionContents" + latest_content: Optional["RawSecretRevisionContents"] = None id: str = dataclasses.field(default_factory=_generate_secret_id) @@ -309,15 +309,15 @@ def __hash__(self) -> int: return hash(self.id) def __post_init__(self): - if self.current is None: + if self.latest_content is None: # bypass frozen dataclass - object.__setattr__(self, "current", self.latest) + object.__setattr__(self, "latest_content", self.tracked_content) def _track_latest_revision(self): """Set the current revision to the tracked revision.""" # bypass frozen dataclass object.__setattr__(self, "_tracked_revision", self._latest_revision) - object.__setattr__(self, "current", self.latest) + object.__setattr__(self, "tracked_content", self.latest_content) def _update_metadata( self, @@ -333,7 +333,7 @@ def _update_metadata( # TODO: if this is done twice in the same hook, then Juju ignores the # first call, it doesn't continue to update like this does. if content: - object.__setattr__(self, "latest", content) + object.__setattr__(self, "latest_content", content) if label: object.__setattr__(self, "label", label) if description: diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index 4587224a..38dcfbc2 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -303,7 +303,7 @@ def test_config_secret_old_juju(juju_version): @pytest.mark.parametrize("bad_v", ("1.0", "0", "1.2", "2.35.42", "2.99.99", "2.99")) def test_secrets_jujuv_bad(bad_v): - secret = Secret(latest={"a": "b"}) + secret = Secret({"a": "b"}) assert_inconsistent( State(secrets={secret}), _Event("bar"), @@ -328,7 +328,7 @@ def test_secrets_jujuv_bad(bad_v): @pytest.mark.parametrize("good_v", ("3.0", "3.1", "3", "3.33", "4", "100")) def test_secrets_jujuv_bad(good_v): assert_consistent( - State(secrets={Secret(latest={"a": "b"})}), + State(secrets={Secret({"a": "b"})}), _Event("bar"), _CharmSpec(MyCharm, {}), good_v, @@ -336,7 +336,7 @@ def test_secrets_jujuv_bad(good_v): def test_secret_not_in_state(): - secret = Secret(latest={"a": "b"}) + secret = Secret({"a": "b"}) assert_inconsistent( State(), _Event("secret_changed", secret=secret), @@ -723,9 +723,7 @@ def test_storedstate_consistency(): ) assert_inconsistent( State( - stored_states={ - StoredState(owner_path=None, content={"secret": Secret(latest={})}) - } + stored_states={StoredState(owner_path=None, content={"secret": Secret({})})} ), _Event("start"), _CharmSpec( diff --git a/tests/test_context_on.py b/tests/test_context_on.py index 6f749d4b..c74b0185 100644 --- a/tests/test_context_on.py +++ b/tests/test_context_on.py @@ -81,7 +81,7 @@ def test_simple_events(event_name, event_kind): ) def test_simple_secret_events(as_kwarg, event_name, event_kind, owner): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) - secret = scenario.Secret(latest={"password": "xxxx"}, owner=owner) + secret = scenario.Secret({"password": "xxxx"}, owner=owner) state_in = scenario.State(secrets={secret}) # These look like: # ctx.run(ctx.on.secret_changed(secret=secret), state) @@ -112,8 +112,8 @@ def test_simple_secret_events(as_kwarg, event_name, event_kind, owner): def test_revision_secret_events(event_name, event_kind): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - current={"password": "yyyy"}, - latest={"password": "xxxx"}, + {"password": "yyyy"}, + {"password": "xxxx"}, owner="app", ) state_in = scenario.State(secrets={secret}) @@ -135,8 +135,8 @@ def test_revision_secret_events(event_name, event_kind): def test_revision_secret_events_as_positional_arg(event_name): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - current={"password": "yyyy"}, - latest={"password": "xxxx"}, + {"password": "yyyy"}, + {"password": "xxxx"}, owner=None, ) state_in = scenario.State(secrets={secret}) diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index f6b1f9b4..b8f88dab 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -51,8 +51,8 @@ def test_get_secret(mycharm, owner): def test_get_secret_get_refresh(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - current={"a": "b"}, - latest={"a": "c"}, + tracked_content={"a": "b"}, + latest_content={"a": "c"}, owner=owner, ) with ctx.manager( @@ -69,8 +69,8 @@ def test_get_secret_get_refresh(mycharm, owner): def test_get_secret_nonowner_peek_update(mycharm, app): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - current={"a": "b"}, - latest={"a": "c"}, + {"a": "b"}, + {"a": "c"}, ) with ctx.manager( ctx.on.update_status(), @@ -95,8 +95,8 @@ def test_get_secret_nonowner_peek_update(mycharm, app): def test_get_secret_owner_peek_update(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - current={"a": "b"}, - latest={"a": "c"}, + {"a": "b"}, + {"a": "c"}, owner=owner, ) with ctx.manager( @@ -119,8 +119,8 @@ def test_get_secret_owner_peek_update(mycharm, owner): def test_secret_changed_owner_evt_fails(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - current={"a": "b"}, - latest={"a": "c"}, + {"a": "b"}, + {"a": "c"}, owner=owner, ) with pytest.raises(ValueError): @@ -138,8 +138,8 @@ def test_secret_changed_owner_evt_fails(mycharm, owner): def test_consumer_events_failures(mycharm, evt_suffix, revision): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - current={"a": "b"}, - latest={"a": "c"}, + {"a": "b"}, + {"a": "c"}, ) kwargs = {"secret": secret} if revision is not None: @@ -163,7 +163,7 @@ def test_add(mycharm, app): assert mgr.output.secrets secret = mgr.output.get_secret(label="mylabel") - assert secret.latest == secret.current == {"foo": "bar"} + assert secret.latest_content == secret.tracked_content == {"foo": "bar"} assert secret.label == "mylabel" @@ -199,8 +199,8 @@ def test_set_legacy_behaviour(mycharm): state_out = mgr.run() assert ( - state_out.get_secret(label="mylabel").current - == state_out.get_secret(label="mylabel").latest + state_out.get_secret(label="mylabel").tracked_content + == state_out.get_secret(label="mylabel").latest_content == rev2 ) @@ -231,8 +231,8 @@ def test_set(mycharm): state_out = mgr.run() assert ( - state_out.get_secret(label="mylabel").current - == state_out.get_secret(label="mylabel").latest + state_out.get_secret(label="mylabel").tracked_content + == state_out.get_secret(label="mylabel").latest_content == rev2 ) @@ -256,8 +256,8 @@ def test_set_juju33(mycharm): state_out = mgr.run() assert ( - state_out.get_secret(label="mylabel").current - == state_out.get_secret(label="mylabel").latest + state_out.get_secret(label="mylabel").tracked_content + == state_out.get_secret(label="mylabel").latest_content == rev2 ) @@ -536,7 +536,7 @@ def _on_secret_expired(self, event): ctx.on.secret_expired(secret, revision=old_revision), State(leader=True, secrets={secret}), ) - assert state.get_secret(id=secret.id).latest == {"password": "newpass"} + assert state.get_secret(id=secret.id).latest_content == {"password": "newpass"} assert ctx.removed_secret_revisions == [old_revision] @@ -564,13 +564,13 @@ def _on_secret_remove(self, event): def test_no_additional_positional_arguments(): with pytest.raises(TypeError): - Secret({}, None) + Secret({}, {}, None) def test_default_values(): contents = {"foo": "bar"} secret = Secret(contents) - assert secret.latest == secret.current == contents + assert secret.latest_content == secret.tracked_content == contents assert secret.id.startswith("secret:") assert secret.label is None assert secret.description is None From bc8ae250c03675fe40b2e55117e408c75d76fe75 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 7 Aug 2024 15:10:45 +1200 Subject: [PATCH 14/16] Fix README. --- README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f8f81835..66ae8099 100644 --- a/README.md +++ b/README.md @@ -705,9 +705,9 @@ check doesn't have to match the event being generated: by the time that Juju sends a pebble-check-failed event the check might have started passing again. ```python -ctx = scenario.Context(MyCharm, meta={"name": "foo", "containers": {"my-container": {}}}) +ctx = scenario.Context(MyCharm, meta={"name": "foo", "containers": {"my_container": {}}}) check_info = scenario.CheckInfo("http-check", failures=7, status=ops.pebble.CheckStatus.DOWN) -container = scenario.Container("my-container", check_infos={check_info}) +container = scenario.Container("my_container", check_infos={check_info}) state = scenario.State(containers={container}) ctx.run(ctx.on.pebble_check_failed(info=check_info, container=container), state=state) ``` @@ -807,7 +807,7 @@ state = scenario.State( tracked_content={'key': 'public'}, latest_content={'key': 'public', 'cert': 'private'}, ) - ] + } ) ``` @@ -872,12 +872,12 @@ class SecretCharm(ops.CharmBase): event.secret.remove_revision(event.revision) -ctx = Context(SecretCharm, meta={"name": "foo"}) -secret = Secret({"password": "xxxxxxxx"}, owner="app") +ctx = scenario.Context(SecretCharm, meta={"name": "foo"}) +secret = scenario.Secret({"password": "xxxxxxxx"}, owner="app") old_revision = 42 state = ctx.run( ctx.on.secret_remove(secret, revision=old_revision), - State(leader=True, secrets={secret}) + scenario.State(leader=True, secrets={secret}) ) assert ctx.removed_secret_revisions == [old_revision] ``` From 09e759967ff88310dd3c70f8eb7bb27d53165f34 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 7 Aug 2024 15:12:10 +1200 Subject: [PATCH 15/16] Make the grant relation id clearer. --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 66ae8099..a0699cde 100644 --- a/README.md +++ b/README.md @@ -835,13 +835,14 @@ If this charm does not own the secret, but also it was not granted view rights b To specify a secret owned by this unit (or app): ```python +rel = scenario.Relation("web") state = scenario.State( secrets={ scenario.Secret( {'key': 'private'}, owner='unit', # or 'app' - # The secret owner has granted access to the "remote" app over some relation with ID 0: - remote_grants={0: {"remote"}} + # The secret owner has granted access to the "remote" app over some relation: + remote_grants={rel.id: {"remote"}} ) } ) From c26e0591dffe091c80392247158198b08c35efd0 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 7 Aug 2024 15:20:42 +1200 Subject: [PATCH 16/16] Go back to making only one content positional. --- scenario/state.py | 2 +- tests/test_context_on.py | 8 ++++---- tests/test_e2e/test_secrets.py | 17 ++++++++--------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/scenario/state.py b/scenario/state.py index 76819ee5..c873b1d4 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -280,7 +280,7 @@ def _generate_secret_id(): @dataclasses.dataclass(frozen=True) -class Secret(_max_posargs(2)): +class Secret(_max_posargs(1)): tracked_content: "RawSecretRevisionContents" latest_content: Optional["RawSecretRevisionContents"] = None diff --git a/tests/test_context_on.py b/tests/test_context_on.py index c74b0185..8ddbf4d4 100644 --- a/tests/test_context_on.py +++ b/tests/test_context_on.py @@ -112,8 +112,8 @@ def test_simple_secret_events(as_kwarg, event_name, event_kind, owner): def test_revision_secret_events(event_name, event_kind): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - {"password": "yyyy"}, - {"password": "xxxx"}, + tracked_content={"password": "yyyy"}, + latest_content={"password": "xxxx"}, owner="app", ) state_in = scenario.State(secrets={secret}) @@ -135,8 +135,8 @@ def test_revision_secret_events(event_name, event_kind): def test_revision_secret_events_as_positional_arg(event_name): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) secret = scenario.Secret( - {"password": "yyyy"}, - {"password": "xxxx"}, + tracked_content={"password": "yyyy"}, + latest_content={"password": "xxxx"}, owner=None, ) state_in = scenario.State(secrets={secret}) diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index b8f88dab..5983c7df 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -1,5 +1,4 @@ import datetime -import warnings import pytest from ops.charm import CharmBase @@ -69,8 +68,8 @@ def test_get_secret_get_refresh(mycharm, owner): def test_get_secret_nonowner_peek_update(mycharm, app): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - {"a": "b"}, - {"a": "c"}, + tracked_content={"a": "b"}, + latest_content={"a": "c"}, ) with ctx.manager( ctx.on.update_status(), @@ -95,8 +94,8 @@ def test_get_secret_nonowner_peek_update(mycharm, app): def test_get_secret_owner_peek_update(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - {"a": "b"}, - {"a": "c"}, + tracked_content={"a": "b"}, + latest_content={"a": "c"}, owner=owner, ) with ctx.manager( @@ -119,8 +118,8 @@ def test_get_secret_owner_peek_update(mycharm, owner): def test_secret_changed_owner_evt_fails(mycharm, owner): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - {"a": "b"}, - {"a": "c"}, + tracked_content={"a": "b"}, + latest_content={"a": "c"}, owner=owner, ) with pytest.raises(ValueError): @@ -138,8 +137,8 @@ def test_secret_changed_owner_evt_fails(mycharm, owner): def test_consumer_events_failures(mycharm, evt_suffix, revision): ctx = Context(mycharm, meta={"name": "local"}) secret = Secret( - {"a": "b"}, - {"a": "c"}, + tracked_content={"a": "b"}, + latest_content={"a": "c"}, ) kwargs = {"secret": secret} if revision is not None: