From a8bb04d98178534cc651e91beb06c4e30ae3f438 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Mon, 20 Nov 2023 10:25:49 +0100 Subject: [PATCH 01/13] uniformed owner 'application'-->'app' --- scenario/state.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/scenario/state.py b/scenario/state.py index e5fb1d1c..45120aad 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -135,7 +135,7 @@ class Secret(_DCBase): contents: Dict[int, "RawSecretRevisionContents"] # indicates if the secret is owned by THIS unit, THIS app or some other app/unit. - owner: Literal["unit", "application", None] = None + owner: Literal["unit", "app", None] = None # has this secret been granted to this unit/app or neither? Only applicable if NOT owner granted: Literal["unit", "app", False] = False @@ -152,6 +152,14 @@ class Secret(_DCBase): expire: Optional[datetime.datetime] = None rotate: SecretRotate = SecretRotate.NEVER + def __post_init__(self): + if self.owner == "application": + logger.warning( + "Secret.owner='application' is deprecated in favour of 'app'.", + ) + # bypass frozen dataclass + object.__setattr__(self, "owner", "app") + # consumer-only events @property def changed_event(self): From 43e2b12605e5239fe8bf18b419a457b2e983e9da Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Mon, 20 Nov 2023 11:08:37 +0100 Subject: [PATCH 02/13] lint --- pyproject.toml | 2 +- scenario/mocking.py | 38 +++++++++++++++++++-------- tests/test_e2e/test_secrets.py | 47 ++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3d7b0289..84b93d88 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,7 @@ build-backend = "setuptools.build_meta" [project] name = "ops-scenario" -version = "5.7" +version = "5.7.1" authors = [ { name = "Pietro Pasotti", email = "pietro.pasotti@canonical.com" } diff --git a/scenario/mocking.py b/scenario/mocking.py index e5bd4298..4423cadb 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -31,6 +31,7 @@ Event, ExecOutput, Relation, + Secret, State, SubordinateRelation, _CharmSpec, @@ -302,6 +303,24 @@ def secret_add( self._state.secrets.append(secret) return id + @staticmethod + def _check_secret_data_access( + secret: "Secret", + read: bool = False, + write: bool = False, + ): + # FIXME: match real traceback + # TODO: different behaviours if ownership == 'app'/'unit'? + if read: + if secret.owner is None and secret.granted is False: + raise SecretNotFoundError( + f"You must own secret {secret.id!r} to perform this operation", + ) + + if write: + if secret.owner is None: + raise SecretNotFoundError("this secret is not owned by this unit/app") + def secret_get( self, *, @@ -311,6 +330,8 @@ def secret_get( peek: bool = False, ) -> Dict[str, str]: secret = self._get_secret(id, label) + self._check_secret_data_access(secret, read=True) + revision = secret.revision if peek or refresh: revision = max(secret.contents.keys()) @@ -326,8 +347,9 @@ def secret_info_get( label: Optional[str] = None, ) -> SecretInfo: secret = self._get_secret(id, label) - if not secret.owner: - raise RuntimeError(f"not the owner of {secret}") + + # only "manage"=write access level can read secret info + self._check_secret_data_access(secret, write=True) return SecretInfo( id=secret.id, @@ -349,8 +371,7 @@ def secret_set( rotate: Optional[SecretRotate] = None, ): secret = self._get_secret(id, label) - if not secret.owner: - raise RuntimeError(f"not the owner of {secret}") + self._check_secret_data_access(secret, write=True) secret._update_metadata( content=content, @@ -362,8 +383,7 @@ def secret_set( def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None): secret = self._get_secret(id) - if not secret.owner: - raise RuntimeError(f"not the owner of {secret}") + self._check_secret_data_access(secret, write=True) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name @@ -374,16 +394,14 @@ 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): secret = self._get_secret(id) - if not secret.owner: - raise RuntimeError(f"not the owner of {secret}") + self._check_secret_data_access(secret, write=True) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name secret.remote_grants[relation_id].remove(grantee) def secret_remove(self, id: str, *, revision: Optional[int] = None): secret = self._get_secret(id) - if not secret.owner: - raise RuntimeError(f"not the owner of {secret}") + self._check_secret_data_access(secret, write=True) if revision: del secret.contents[revision] diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 780ca990..17026cea 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -3,9 +3,10 @@ import pytest from ops.charm import CharmBase from ops.framework import Framework -from ops.model import SecretNotFoundError, SecretRotate +from ops.model import ModelError, SecretNotFoundError, SecretRotate from scenario import Context +from scenario.runtime import UncaughtCharmError from scenario.state import Relation, Secret, State from tests.helpers import trigger @@ -41,7 +42,7 @@ def post_event(charm: CharmBase): assert charm.model.get_secret(id="foo").get_content()["a"] == "b" trigger( - State(secrets=[Secret(id="foo", contents={0: {"a": "b"}})]), + State(secrets=[Secret(id="foo", contents={0: {"a": "b"}}, granted="unit")]), "update_status", mycharm, meta={"name": "local"}, @@ -49,7 +50,23 @@ def post_event(charm: CharmBase): ) -def test_get_secret_peek_update(mycharm): +def test_get_secret_not_granted(mycharm): + def post_event(charm: CharmBase): + assert charm.model.get_secret(id="foo").get_content()["a"] == "b" + + with pytest.raises(UncaughtCharmError) as e: + trigger( + State(secrets=[Secret(id="foo", contents={0: {"a": "b"}})]), + "update_status", + mycharm, + meta={"name": "local"}, + post_event=post_event, + ) + + +@pytest.mark.parametrize("owner", ("app", "unit", "application")) +# "application" is deprecated but still supported +def test_get_secret_peek_update(mycharm, owner): def post_event(charm: CharmBase): assert charm.model.get_secret(id="foo").get_content()["a"] == "b" assert charm.model.get_secret(id="foo").peek_content()["a"] == "c" @@ -67,6 +84,7 @@ def post_event(charm: CharmBase): 0: {"a": "b"}, 1: {"a": "c"}, }, + owner=owner, ) ] ), @@ -77,7 +95,9 @@ def post_event(charm: CharmBase): ) -def test_secret_changed_owner_evt_fails(mycharm): +@pytest.mark.parametrize("owner", ("app", "unit", "application")) +# "application" is deprecated but still supported +def test_secret_changed_owner_evt_fails(mycharm, owner): with pytest.raises(ValueError): _ = Secret( id="foo", @@ -85,7 +105,7 @@ def test_secret_changed_owner_evt_fails(mycharm): 0: {"a": "b"}, 1: {"a": "c"}, }, - owner="unit", + owner=owner, ).changed_event @@ -150,11 +170,12 @@ def post_event(charm: CharmBase): ) -def test_meta_nonowner(mycharm): +@pytest.mark.parametrize("granted", ("app", "unit")) +def test_meta_nonowner(mycharm, granted): def post_event(charm: CharmBase): secret = charm.model.get_secret(id="foo") - with pytest.raises(RuntimeError): - info = secret.get_info() + with pytest.raises(SecretNotFoundError): + secret.get_info() trigger( State( @@ -164,6 +185,7 @@ def post_event(charm: CharmBase): label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, + granted=granted, contents={ 0: {"a": "b"}, }, @@ -253,13 +275,10 @@ def post_event(charm: CharmBase): def test_grant_nonowner(mycharm): def post_event(charm: CharmBase): - secret = charm.model.get_secret(id="foo") - with pytest.raises(RuntimeError): - secret = charm.model.get_secret(label="mylabel") - foo = charm.model.get_relation("foo") - secret.grant(relation=foo) + with pytest.raises(SecretNotFoundError): + charm.model.get_secret(id="foo") - out = trigger( + trigger( State( relations=[Relation("foo", "remote")], secrets=[ From 33cf4adbff815080f0a5f4253c6d46e6a0874a41 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Mon, 20 Nov 2023 14:01:44 +0100 Subject: [PATCH 03/13] factored in ownership --- scenario/mocking.py | 26 ++++++++---- tests/test_e2e/test_secrets.py | 74 ++++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 38 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 4423cadb..a6c49199 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -303,23 +303,35 @@ def secret_add( self._state.secrets.append(secret) return id - @staticmethod def _check_secret_data_access( + self, secret: "Secret", read: bool = False, write: bool = False, ): - # FIXME: match real traceback - # TODO: different behaviours if ownership == 'app'/'unit'? + # FIXME: match real tracebacks + self_is_leader = self.is_leader() + if read: - if secret.owner is None and secret.granted is False: - raise SecretNotFoundError( - f"You must own secret {secret.id!r} to perform this operation", - ) + if secret.owner is None: + if secret.granted is False: + raise SecretNotFoundError( + f"You must own secret {secret.id!r} to perform this operation", + ) + if secret.granted == "app" and not self_is_leader: + raise SecretNotFoundError( + f"Only the leader can read secret {secret.id!r} since it was " + f"granted to this app.", + ) if write: if secret.owner is None: raise SecretNotFoundError("this secret is not owned by this unit/app") + if secret.owner == "app" and not self_is_leader: + raise SecretNotFoundError( + f"App-owned secret {secret.id!r} can only be " + f"managed by the leader.", + ) def secret_get( self, diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 17026cea..54f334d8 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -170,33 +170,42 @@ def post_event(charm: CharmBase): ) +@pytest.mark.parametrize("leader", (True, False)) @pytest.mark.parametrize("granted", ("app", "unit")) -def test_meta_nonowner(mycharm, granted): +def test_meta_nonowner(mycharm, granted, leader): def post_event(charm: CharmBase): secret = charm.model.get_secret(id="foo") with pytest.raises(SecretNotFoundError): secret.get_info() - trigger( - State( - secrets=[ - Secret( - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - granted=granted, - contents={ - 0: {"a": "b"}, - }, - ) - ] - ), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) + try: + trigger( + State( + leader=leader, + secrets=[ + Secret( + id="foo", + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + granted=granted, + contents={ + 0: {"a": "b"}, + }, + ) + ], + ), + "update_status", + mycharm, + meta={"name": "local"}, + post_event=post_event, + ) + except UncaughtCharmError as e: + if not leader and granted == "app": + # expected failure + pass + else: + raise @pytest.mark.parametrize("app", (True, False)) @@ -300,19 +309,22 @@ def post_event(charm: CharmBase): ) -class GrantingCharm(CharmBase): - def __init__(self, *args): - super().__init__(*args) - self.framework.observe(self.on.start, self._on_start) - - def _on_start(self, _): - secret = self.app.add_secret({"foo": "bar"}) - secret.grant(self.model.relations["bar"][0]) +@pytest.mark.parametrize("leader", (True, False)) +def test_grant_after_add(leader): + class GrantingCharm(CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.start, self._on_start) + def _on_start(self, _): + if leader: + secret = self.app.add_secret({"foo": "bar"}) + else: + secret = self.unit.add_secret({"foo": "bar"}) + secret.grant(self.model.relations["bar"][0]) -def test_grant_after_add(): context = Context( GrantingCharm, meta={"name": "foo", "provides": {"bar": {"interface": "bar"}}} ) - state = State(relations=[Relation("bar")]) + state = State(leader=leader, relations=[Relation("bar")]) context.run("start", state) From 0aab2af523f248d6a7ac4c6c6ef282f6b125ccb7 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 21 Nov 2023 09:51:18 +0100 Subject: [PATCH 04/13] some more tests and metadata rule fixes --- scenario/mocking.py | 19 ++- tests/test_e2e/test_secrets.py | 284 +++++++++++++++++---------------- 2 files changed, 161 insertions(+), 142 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index a6c49199..42f7b073 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -307,7 +307,7 @@ def _check_secret_data_access( self, secret: "Secret", read: bool = False, - write: bool = False, + manage: bool = False, ): # FIXME: match real tracebacks self_is_leader = self.is_leader() @@ -324,7 +324,7 @@ def _check_secret_data_access( f"granted to this app.", ) - if write: + if manage: if secret.owner is None: raise SecretNotFoundError("this secret is not owned by this unit/app") if secret.owner == "app" and not self_is_leader: @@ -344,6 +344,11 @@ def secret_get( secret = self._get_secret(id, label) self._check_secret_data_access(secret, read=True) + if self._context.juju_version <= "3.2": + # in juju<3.2, secret owners always track the latest revision. + if secret.owner is not None: + refresh = True + revision = secret.revision if peek or refresh: revision = max(secret.contents.keys()) @@ -361,7 +366,7 @@ def secret_info_get( secret = self._get_secret(id, label) # only "manage"=write access level can read secret info - self._check_secret_data_access(secret, write=True) + self._check_secret_data_access(secret, read=True) return SecretInfo( id=secret.id, @@ -383,7 +388,7 @@ def secret_set( rotate: Optional[SecretRotate] = None, ): secret = self._get_secret(id, label) - self._check_secret_data_access(secret, write=True) + self._check_secret_data_access(secret, manage=True) secret._update_metadata( content=content, @@ -395,7 +400,7 @@ def secret_set( def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None): secret = self._get_secret(id) - self._check_secret_data_access(secret, write=True) + self._check_secret_data_access(secret, manage=True) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name @@ -406,14 +411,14 @@ 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): secret = self._get_secret(id) - self._check_secret_data_access(secret, write=True) + self._check_secret_data_access(secret, manage=True) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name secret.remote_grants[relation_id].remove(grantee) def secret_remove(self, id: str, *, revision: Optional[int] = None): secret = self._get_secret(id) - self._check_secret_data_access(secret, write=True) + self._check_secret_data_access(secret, manage=True) if revision: del secret.contents[revision] diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 54f334d8..ec9c035f 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -3,12 +3,10 @@ import pytest from ops.charm import CharmBase from ops.framework import Framework -from ops.model import ModelError, SecretNotFoundError, SecretRotate +from ops.model import SecretNotFoundError, SecretRotate from scenario import Context -from scenario.runtime import UncaughtCharmError from scenario.state import Relation, Secret, State -from tests.helpers import trigger @pytest.fixture(scope="function") @@ -26,48 +24,75 @@ def _on_event(self, event): def test_get_secret_no_secret(mycharm): - def post_event(charm: CharmBase): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", State() + ) as mgr: with pytest.raises(SecretNotFoundError): - assert charm.model.get_secret(id="foo") + assert mgr.charm.model.get_secret(id="foo") with pytest.raises(SecretNotFoundError): - assert charm.model.get_secret(label="foo") - - trigger( - State(), "update_status", mycharm, meta={"name": "local"}, post_event=post_event - ) + assert mgr.charm.model.get_secret(label="foo") def test_get_secret(mycharm): - def post_event(charm: CharmBase): - assert charm.model.get_secret(id="foo").get_content()["a"] == "b" - - trigger( - State(secrets=[Secret(id="foo", contents={0: {"a": "b"}}, granted="unit")]), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) + with Context(mycharm, meta={"name": "local"}).manager( + state=State( + secrets=[Secret(id="foo", contents={0: {"a": "b"}}, granted="unit")] + ), + event="update_status", + ) as mgr: + assert mgr.charm.model.get_secret(id="foo").get_content()["a"] == "b" def test_get_secret_not_granted(mycharm): - def post_event(charm: CharmBase): - assert charm.model.get_secret(id="foo").get_content()["a"] == "b" - - with pytest.raises(UncaughtCharmError) as e: - trigger( - State(secrets=[Secret(id="foo", contents={0: {"a": "b"}})]), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) + with Context(mycharm, meta={"name": "local"}).manager( + state=State(secrets=[Secret(id="foo", contents={0: {"a": "b"}})]), + event="update_status", + ) as mgr: + with pytest.raises(SecretNotFoundError) as e: + assert mgr.charm.model.get_secret(id="foo").get_content()["a"] == "b" @pytest.mark.parametrize("owner", ("app", "unit", "application")) # "application" is deprecated but still supported -def test_get_secret_peek_update(mycharm, owner): - def post_event(charm: CharmBase): +def test_get_secret_get_refresh(mycharm, owner): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State( + secrets=[ + Secret( + id="foo", + contents={ + 0: {"a": "b"}, + 1: {"a": "c"}, + }, + owner=owner, + ) + ] + ), + ) as mgr: + charm = mgr.charm + assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" + + +@pytest.mark.parametrize("app", (True, False)) +def test_get_secret_nonowner_peek_update(mycharm, app): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State( + leader=app, + secrets=[ + Secret( + id="foo", + contents={ + 0: {"a": "b"}, + 1: {"a": "c"}, + }, + granted="app" if app else "unit", + ), + ], + ), + ) 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" @@ -75,7 +100,12 @@ def post_event(charm: CharmBase): assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" assert charm.model.get_secret(id="foo").get_content()["a"] == "c" - trigger( + +@pytest.mark.parametrize("owner", ("app", "unit", "application")) +# "application" is deprecated but still supported +def test_get_secret_owner_peek_update(mycharm, owner): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", State( secrets=[ Secret( @@ -88,11 +118,11 @@ def post_event(charm: CharmBase): ) ] ), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) + ) as mgr: + charm = mgr.charm + assert charm.model.get_secret(id="foo").get_content()["a"] == "c" + assert charm.model.get_secret(id="foo").peek_content()["a"] == "c" + assert charm.model.get_secret(id="foo").get_content(refresh=True)["a"] == "c" @pytest.mark.parametrize("owner", ("app", "unit", "application")) @@ -124,21 +154,44 @@ def test_consumer_events_failures(mycharm, evt_prefix): ) -def test_add(mycharm): - def post_event(charm: CharmBase): - charm.unit.add_secret({"foo": "bar"}, label="mylabel") +@pytest.mark.parametrize("app", (True, False)) +def test_add(mycharm, app): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State(leader=app), + ) as mgr: + charm = mgr.charm + if app: + charm.app.add_secret({"foo": "bar"}, label="mylabel") + else: + charm.unit.add_secret({"foo": "bar"}, label="mylabel") - out = trigger( - State(), "update_status", mycharm, meta={"name": "local"}, post_event=post_event - ) - assert out.secrets - secret = out.secrets[0] + assert mgr.output.secrets + secret = mgr.output.secrets[0] assert secret.contents[0] == {"foo": "bar"} assert secret.label == "mylabel" -def test_meta(mycharm): - def post_event(charm: CharmBase): +@pytest.mark.parametrize("app", (True, False)) +def test_meta(mycharm, app): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State( + secrets=[ + Secret( + owner="app" if app else "unit", + id="foo", + label="mylabel", + description="foobarbaz", + rotate=SecretRotate.HOURLY, + contents={ + 0: {"a": "b"}, + }, + ) + ] + ), + ) as mgr: + charm = mgr.charm assert charm.model.get_secret(label="mylabel") secret = charm.model.get_secret(id="foo") @@ -148,77 +201,44 @@ def post_event(charm: CharmBase): assert info.label == "mylabel" assert info.rotation == SecretRotate.HOURLY - trigger( + +@pytest.mark.parametrize("leader", (True, False)) +@pytest.mark.parametrize("granted", ("app", "unit")) +def test_meta_nonowner(mycharm, granted, leader): + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", State( + leader=leader, secrets=[ Secret( - owner="unit", id="foo", label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, + granted=granted, contents={ 0: {"a": "b"}, }, ) - ] + ], ), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) - - -@pytest.mark.parametrize("leader", (True, False)) -@pytest.mark.parametrize("granted", ("app", "unit")) -def test_meta_nonowner(mycharm, granted, leader): - def post_event(charm: CharmBase): - secret = charm.model.get_secret(id="foo") - with pytest.raises(SecretNotFoundError): - secret.get_info() - - try: - trigger( - State( - leader=leader, - secrets=[ - Secret( - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - granted=granted, - contents={ - 0: {"a": "b"}, - }, - ) - ], - ), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) - except UncaughtCharmError as e: + ) as mgr: if not leader and granted == "app": - # expected failure - pass + with pytest.raises(SecretNotFoundError): + mgr.charm.model.get_secret(id="foo") + return else: - raise + secret = mgr.charm.model.get_secret(id="foo") + + secret.get_info() @pytest.mark.parametrize("app", (True, False)) def test_grant(mycharm, app): - def post_event(charm: CharmBase): - secret = charm.model.get_secret(label="mylabel") - foo = charm.model.get_relation("foo") - if app: - secret.grant(relation=foo) - else: - secret.grant(relation=foo, unit=foo.units.pop()) - - out = trigger( + with Context( + mycharm, meta={"name": "local", "requires": {"foo": {"interface": "bar"}}} + ).manager( + "update_status", State( relations=[Relation("foo", "remote")], secrets=[ @@ -234,29 +254,23 @@ def post_event(charm: CharmBase): ) ], ), - "update_status", - mycharm, - meta={"name": "local", "requires": {"foo": {"interface": "bar"}}}, - post_event=post_event, - ) - - vals = list(out.secrets[0].remote_grants.values()) + ) as mgr: + charm = mgr.charm + secret = charm.model.get_secret(label="mylabel") + foo = charm.model.get_relation("foo") + if app: + secret.grant(relation=foo) + else: + secret.grant(relation=foo, unit=foo.units.pop()) + vals = list(mgr.output.secrets[0].remote_grants.values()) assert vals == [{"remote"}] if app else [{"remote/0"}] def test_update_metadata(mycharm): exp = datetime.datetime(2050, 12, 12) - def post_event(charm: CharmBase): - secret = charm.model.get_secret(label="mylabel") - secret.set_info( - label="babbuccia", - description="blu", - expire=exp, - rotate=SecretRotate.DAILY, - ) - - out = trigger( + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", State( secrets=[ Secret( @@ -269,13 +283,16 @@ def post_event(charm: CharmBase): ) ], ), - "update_status", - mycharm, - meta={"name": "local"}, - post_event=post_event, - ) + ) as mgr: + secret = mgr.charm.model.get_secret(label="mylabel") + secret.set_info( + label="babbuccia", + description="blu", + expire=exp, + rotate=SecretRotate.DAILY, + ) - secret_out = out.secrets[0] + secret_out = mgr.output.secrets[0] assert secret_out.label == "babbuccia" assert secret_out.rotate == SecretRotate.DAILY assert secret_out.description == "blu" @@ -283,11 +300,10 @@ def post_event(charm: CharmBase): def test_grant_nonowner(mycharm): - def post_event(charm: CharmBase): - with pytest.raises(SecretNotFoundError): - charm.model.get_secret(id="foo") - - trigger( + with Context( + mycharm, meta={"name": "local", "requires": {"foo": {"interface": "bar"}}} + ).manager( + "update_status", State( relations=[Relation("foo", "remote")], secrets=[ @@ -302,11 +318,9 @@ def post_event(charm: CharmBase): ) ], ), - "update_status", - mycharm, - meta={"name": "local", "requires": {"foo": {"interface": "bar"}}}, - post_event=post_event, - ) + ) as mgr: + with pytest.raises(SecretNotFoundError): + mgr.charm.model.get_secret(id="foo") @pytest.mark.parametrize("leader", (True, False)) From 9633380e1ca4605a3a75305c60470f709884df74 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 22 Nov 2023 12:16:59 +0100 Subject: [PATCH 05/13] simplified access control logic --- scenario/mocking.py | 10 +-- scenario/state.py | 5 +- tests/test_e2e/test_secrets.py | 127 ++++++++++++++++++++++++++++++--- 3 files changed, 124 insertions(+), 18 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 42f7b073..16248d94 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -314,16 +314,10 @@ def _check_secret_data_access( if read: if secret.owner is None: - if secret.granted is False: + if secret.granted is None: raise SecretNotFoundError( f"You must own secret {secret.id!r} to perform this operation", ) - if secret.granted == "app" and not self_is_leader: - raise SecretNotFoundError( - f"Only the leader can read secret {secret.id!r} since it was " - f"granted to this app.", - ) - if manage: if secret.owner is None: raise SecretNotFoundError("this secret is not owned by this unit/app") @@ -366,7 +360,7 @@ def secret_info_get( secret = self._get_secret(id, label) # only "manage"=write access level can read secret info - self._check_secret_data_access(secret, read=True) + self._check_secret_data_access(secret, manage=True) return SecretInfo( id=secret.id, diff --git a/scenario/state.py b/scenario/state.py index 45120aad..249e010e 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -138,7 +138,7 @@ class Secret(_DCBase): owner: Literal["unit", "app", None] = None # has this secret been granted to this unit/app or neither? Only applicable if NOT owner - granted: Literal["unit", "app", False] = False + granted: Literal["unit", "app", None] = None # what revision is currently tracked by this charm. Only meaningful if owner=False revision: int = 0 @@ -213,8 +213,9 @@ def _update_metadata( ): """Update the metadata.""" revision = max(self.contents.keys()) + self.contents[revision + 1] = content + # bypass frozen dataclass - object.__setattr__(self, "contents"[revision + 1], content) if label: object.__setattr__(self, "label", label) if description: diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index ec9c035f..59e5ebb4 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -3,6 +3,7 @@ import pytest from ops.charm import CharmBase from ops.framework import Framework +from ops.model import Secret as ops_Secret from ops.model import SecretNotFoundError, SecretRotate from scenario import Context @@ -172,11 +173,79 @@ def test_add(mycharm, app): assert secret.label == "mylabel" +def test_set(mycharm): + rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State(), + ) as mgr: + charm = mgr.charm + secret: ops_Secret = charm.unit.add_secret(rev1, label="mylabel") + assert ( + secret.get_content() + == secret.peek_content() + == secret.get_content(refresh=True) + == rev1 + ) + + secret.set_content(rev2) + assert ( + secret.get_content() + == secret.peek_content() + == secret.get_content(refresh=True) + == rev2 + ) + + secret.set_content(rev3) + state_out = mgr.run() + assert ( + secret.get_content() + == secret.peek_content() + == secret.get_content(refresh=True) + == rev3 + ) + + assert state_out.secrets[0].contents == { + 0: rev1, + 1: rev2, + 2: rev3, + } + + +def test_set_juju33(mycharm): + rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} + with Context(mycharm, meta={"name": "local"}, juju_version="3.3").manager( + "update_status", + State(), + ) as mgr: + charm = mgr.charm + secret: ops_Secret = charm.unit.add_secret(rev1, label="mylabel") + assert secret.get_content() == rev1 + + secret.set_content(rev2) + assert secret.get_content() == rev1 + 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.secrets[0].contents == { + 0: rev1, + 1: rev2, + 2: rev3, + } + + @pytest.mark.parametrize("app", (True, False)) def test_meta(mycharm, app): with Context(mycharm, meta={"name": "local"}).manager( "update_status", State( + leader=True, secrets=[ Secret( owner="app" if app else "unit", @@ -188,7 +257,7 @@ def test_meta(mycharm, app): 0: {"a": "b"}, }, ) - ] + ], ), ) as mgr: charm = mgr.charm @@ -203,8 +272,29 @@ def test_meta(mycharm, app): @pytest.mark.parametrize("leader", (True, False)) -@pytest.mark.parametrize("granted", ("app", "unit")) -def test_meta_nonowner(mycharm, granted, leader): +@pytest.mark.parametrize("owner", ("app", "unit", None)) +@pytest.mark.parametrize("granted", ("app", "unit", None)) +def test_secret_permission_model(mycharm, granted, leader, owner): + if granted: + owner = None + + expect_view = bool( + # if you (or your app) owns the secret, you can always view it + (owner is not None) + # can read secrets you don't own if you've been granted them + or granted + ) + + expect_manage = bool( + # if you're the leader and own this app secret + (owner == "app" and leader) + # you own this secret + or (owner == "unit") + ) + + if expect_manage: + assert expect_view + with Context(mycharm, meta={"name": "local"}).manager( "update_status", State( @@ -216,6 +306,7 @@ def test_meta_nonowner(mycharm, granted, leader): description="foobarbaz", rotate=SecretRotate.HOURLY, granted=granted, + owner=owner, contents={ 0: {"a": "b"}, }, @@ -223,14 +314,34 @@ def test_meta_nonowner(mycharm, granted, leader): ], ), ) as mgr: - if not leader and granted == "app": + if expect_view: + secret = mgr.charm.model.get_secret(id="foo") + assert secret.get_content()["a"] == "b" + assert secret.peek_content() + assert secret.get_content(refresh=True) + + else: with pytest.raises(SecretNotFoundError): mgr.charm.model.get_secret(id="foo") - return - else: - secret = mgr.charm.model.get_secret(id="foo") - secret.get_info() + # nothing else to do directly if you can't get a hold of the Secret instance + # but we can try some raw backend calls + with pytest.raises(SecretNotFoundError): + mgr.charm.model._backend.secret_info_get(id="foo") + + with pytest.raises(SecretNotFoundError): + mgr.charm.model._backend.secret_set(id="foo", content={"bo": "fo"}) + + if expect_manage: + secret: ops_Secret = mgr.charm.model.get_secret(id="foo") + assert secret.get_content() + assert secret.peek_content() + assert secret.get_content(refresh=True) + + assert secret.get_info() + secret.set_content({"foo": "boo"}) + assert secret.get_content()["foo"] == "boo" + secret.remove_all_revisions() @pytest.mark.parametrize("app", (True, False)) From 25935dbc1366ca4cce4b49276a9ba1da7a12c6db Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 22 Nov 2023 13:19:18 +0100 Subject: [PATCH 06/13] stripped 'granted' --- scenario/mocking.py | 38 ++++++++---------------- scenario/state.py | 9 +++--- tests/test_e2e/test_secrets.py | 54 +++++++++++----------------------- 3 files changed, 34 insertions(+), 67 deletions(-) diff --git a/scenario/mocking.py b/scenario/mocking.py index 16248d94..11b18a9f 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -303,29 +303,18 @@ def secret_add( self._state.secrets.append(secret) return id - def _check_secret_data_access( + def _check_can_manage_secret( self, secret: "Secret", - read: bool = False, - manage: bool = False, ): # FIXME: match real tracebacks - self_is_leader = self.is_leader() - - if read: - if secret.owner is None: - if secret.granted is None: - raise SecretNotFoundError( - f"You must own secret {secret.id!r} to perform this operation", - ) - if manage: - if secret.owner is None: - raise SecretNotFoundError("this secret is not owned by this unit/app") - if secret.owner == "app" and not self_is_leader: - raise SecretNotFoundError( - f"App-owned secret {secret.id!r} can only be " - f"managed by the leader.", - ) + if secret.owner is None: + raise SecretNotFoundError("this secret is not owned by this unit/app") + if secret.owner == "app" and not self.is_leader(): + raise SecretNotFoundError( + f"App-owned secret {secret.id!r} can only be " + f"managed by the leader.", + ) def secret_get( self, @@ -336,7 +325,6 @@ def secret_get( peek: bool = False, ) -> Dict[str, str]: secret = self._get_secret(id, label) - self._check_secret_data_access(secret, read=True) if self._context.juju_version <= "3.2": # in juju<3.2, secret owners always track the latest revision. @@ -360,7 +348,7 @@ def secret_info_get( secret = self._get_secret(id, label) # only "manage"=write access level can read secret info - self._check_secret_data_access(secret, manage=True) + self._check_can_manage_secret(secret) return SecretInfo( id=secret.id, @@ -382,7 +370,7 @@ def secret_set( rotate: Optional[SecretRotate] = None, ): secret = self._get_secret(id, label) - self._check_secret_data_access(secret, manage=True) + self._check_can_manage_secret(secret) secret._update_metadata( content=content, @@ -394,7 +382,7 @@ def secret_set( def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None): secret = self._get_secret(id) - self._check_secret_data_access(secret, manage=True) + self._check_can_manage_secret(secret) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name @@ -405,14 +393,14 @@ 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): secret = self._get_secret(id) - self._check_secret_data_access(secret, manage=True) + self._check_can_manage_secret(secret) grantee = unit or self._get_relation_by_id(relation_id).remote_app_name secret.remote_grants[relation_id].remove(grantee) def secret_remove(self, id: str, *, revision: Optional[int] = None): secret = self._get_secret(id) - self._check_secret_data_access(secret, manage=True) + self._check_can_manage_secret(secret) if revision: del secret.contents[revision] diff --git a/scenario/state.py b/scenario/state.py index 249e010e..7021a0c6 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -135,11 +135,9 @@ class Secret(_DCBase): contents: Dict[int, "RawSecretRevisionContents"] # 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 - # has this secret been granted to this unit/app or neither? Only applicable if NOT owner - granted: Literal["unit", "app", None] = None - # what revision is currently tracked by this charm. Only meaningful if owner=False revision: int = 0 @@ -831,10 +829,11 @@ class State(_DCBase): model: Model = Model() """The model this charm lives in.""" secrets: List[Secret] = dataclasses.field(default_factory=list) - """The secrets this charm has access to (as an owner, or as a grantee).""" + """The secrets this charm has access to (as an owner, or as a grantee). + The presence of a secret in this list entails that the charm can read it. + Whether it can manage it or not depends on the individual secret's `owner` flag.""" resources: Dict[str, "PathLike"] = dataclasses.field(default_factory=dict) """Mapping from resource name to path at which the resource can be found.""" - planned_units: int = 1 """Number of non-dying planned units that are expected to be running this application. Use with caution.""" diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 59e5ebb4..79bcf725 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -36,9 +36,7 @@ def test_get_secret_no_secret(mycharm): def test_get_secret(mycharm): with Context(mycharm, meta={"name": "local"}).manager( - state=State( - secrets=[Secret(id="foo", contents={0: {"a": "b"}}, granted="unit")] - ), + state=State(secrets=[Secret(id="foo", contents={0: {"a": "b"}}, granted=True)]), event="update_status", ) as mgr: assert mgr.charm.model.get_secret(id="foo").get_content()["a"] == "b" @@ -88,7 +86,6 @@ def test_get_secret_nonowner_peek_update(mycharm, app): 0: {"a": "b"}, 1: {"a": "c"}, }, - granted="app" if app else "unit", ), ], ), @@ -273,18 +270,7 @@ def test_meta(mycharm, app): @pytest.mark.parametrize("leader", (True, False)) @pytest.mark.parametrize("owner", ("app", "unit", None)) -@pytest.mark.parametrize("granted", ("app", "unit", None)) -def test_secret_permission_model(mycharm, granted, leader, owner): - if granted: - owner = None - - expect_view = bool( - # if you (or your app) owns the secret, you can always view it - (owner is not None) - # can read secrets you don't own if you've been granted them - or granted - ) - +def test_secret_permission_model(mycharm, leader, owner): expect_manage = bool( # if you're the leader and own this app secret (owner == "app" and leader) @@ -292,9 +278,6 @@ def test_secret_permission_model(mycharm, granted, leader, owner): or (owner == "unit") ) - if expect_manage: - assert expect_view - with Context(mycharm, meta={"name": "local"}).manager( "update_status", State( @@ -305,7 +288,6 @@ def test_secret_permission_model(mycharm, granted, leader, owner): label="mylabel", description="foobarbaz", rotate=SecretRotate.HOURLY, - granted=granted, owner=owner, contents={ 0: {"a": "b"}, @@ -314,26 +296,15 @@ def test_secret_permission_model(mycharm, granted, leader, owner): ], ), ) as mgr: - if expect_view: - secret = mgr.charm.model.get_secret(id="foo") - assert secret.get_content()["a"] == "b" - assert secret.peek_content() - assert secret.get_content(refresh=True) - - else: - with pytest.raises(SecretNotFoundError): - mgr.charm.model.get_secret(id="foo") - - # nothing else to do directly if you can't get a hold of the Secret instance - # but we can try some raw backend calls - with pytest.raises(SecretNotFoundError): - mgr.charm.model._backend.secret_info_get(id="foo") + secret = mgr.charm.model.get_secret(id="foo") + assert secret.get_content()["a"] == "b" + assert secret.peek_content() + assert secret.get_content(refresh=True) - with pytest.raises(SecretNotFoundError): - mgr.charm.model._backend.secret_set(id="foo", content={"bo": "fo"}) + # can always view + secret: ops_Secret = mgr.charm.model.get_secret(id="foo") if expect_manage: - secret: ops_Secret = mgr.charm.model.get_secret(id="foo") assert secret.get_content() assert secret.peek_content() assert secret.get_content(refresh=True) @@ -343,6 +314,15 @@ def test_secret_permission_model(mycharm, granted, leader, owner): assert secret.get_content()["foo"] == "boo" secret.remove_all_revisions() + else: # cannot manage + # nothing else to do directly if you can't get a hold of the Secret instance + # but we can try some raw backend calls + with pytest.raises(SecretNotFoundError): + secret.get_info() + + with pytest.raises(SecretNotFoundError): + secret.set_content(content={"boo": "foo"}) + @pytest.mark.parametrize("app", (True, False)) def test_grant(mycharm, app): From 0b895434b7069b84263d187dce37ccdcb5504883 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 22 Nov 2023 13:24:04 +0100 Subject: [PATCH 07/13] Doc fixes --- README.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 583dbaad..fc9ee96f 100644 --- a/README.md +++ b/README.md @@ -811,11 +811,16 @@ state = State( ``` 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. +from revision numbers (integers) to a `str:str` dict representing the payload of the revision. -By default, the secret is not owned by **this charm** nor is it granted to it. -Therefore, if charm code attempted to get that secret revision, it would get a permission error: we didn't grant it to -this charm, nor we specified that the secret is owned by it. +There are three cases: +- the secret is owned by this app, in which case only the leader unit can manage it +- 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 + +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. + +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. To specify a secret owned by this unit (or app): @@ -826,7 +831,7 @@ state = State( secrets=[ Secret( id='foo', - contents={0: {'key': 'public'}}, + contents={0: {'key': 'private'}}, owner='unit', # or 'app' remote_grants={0: {"remote"}} # the secret owner has granted access to the "remote" app over some relation with ID 0 @@ -846,7 +851,6 @@ state = State( id='foo', contents={0: {'key': 'public'}}, # owner=None, which is the default - granted="unit", # or "app", revision=0, # the revision that this unit (or app) is currently tracking ) ] From 9100d1ba532e3664199cc4a0bc61fcf24c3f6da8 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 22 Nov 2023 13:33:51 +0100 Subject: [PATCH 08/13] BC --- scenario/state.py | 12 +++++++++++- tests/test_e2e/test_secrets.py | 26 +------------------------- 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/scenario/state.py b/scenario/state.py index 7021a0c6..6f641732 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -138,6 +138,9 @@ class Secret(_DCBase): # if None, the implication is that the secret has been granted to this unit. owner: Literal["unit", "app", None] = None + # deprecated! + granted: Literal["unit", "app", False] = "" + # what revision is currently tracked by this charm. Only meaningful if owner=False revision: int = 0 @@ -151,9 +154,16 @@ class Secret(_DCBase): rotate: SecretRotate = SecretRotate.NEVER def __post_init__(self): + if self.granted != "": + logger.warning( + "``state.Secret.granted`` is deprecated and will be removed in Scenario 6. " + "If a Secret is not owned by the app/unit you are testing, nor has been granted to " + "it by the (remote) owner, then omit it from ``State.secrets`` altogether.", + ) if self.owner == "application": logger.warning( - "Secret.owner='application' is deprecated in favour of 'app'.", + "Secret.owner='application' is deprecated in favour of 'app' " + "and will be removed in Scenario 6.", ) # bypass frozen dataclass object.__setattr__(self, "owner", "app") diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 79bcf725..2d099bfb 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -44,7 +44,7 @@ def test_get_secret(mycharm): def test_get_secret_not_granted(mycharm): with Context(mycharm, meta={"name": "local"}).manager( - state=State(secrets=[Secret(id="foo", contents={0: {"a": "b"}})]), + state=State(secrets=[]), event="update_status", ) as mgr: with pytest.raises(SecretNotFoundError) as e: @@ -390,30 +390,6 @@ def test_update_metadata(mycharm): assert secret_out.expire == exp -def test_grant_nonowner(mycharm): - with Context( - mycharm, meta={"name": "local", "requires": {"foo": {"interface": "bar"}}} - ).manager( - "update_status", - State( - relations=[Relation("foo", "remote")], - secrets=[ - Secret( - id="foo", - label="mylabel", - description="foobarbaz", - rotate=SecretRotate.HOURLY, - contents={ - 0: {"a": "b"}, - }, - ) - ], - ), - ) as mgr: - with pytest.raises(SecretNotFoundError): - mgr.charm.model.get_secret(id="foo") - - @pytest.mark.parametrize("leader", (True, False)) def test_grant_after_add(leader): class GrantingCharm(CharmBase): From 1d4613bbf8e4e481018793305f5eba697ab4e399 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 3 Jan 2024 11:57:54 +0100 Subject: [PATCH 09/13] pr comments --- scenario/context.py | 4 +- scenario/mocking.py | 22 ++++++++-- scenario/state.py | 28 ++++++++----- tests/test_e2e/test_secrets.py | 75 +++++++++++++++++++++++++++++++--- 4 files changed, 108 insertions(+), 21 deletions(-) diff --git a/scenario/context.py b/scenario/context.py index ebdecb93..edf9ea97 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -34,6 +34,8 @@ logger = scenario_logger.getChild("runtime") +DEFAULT_JUJU_VERSION = "3.4" + @dataclasses.dataclass class ActionOutput: @@ -166,7 +168,7 @@ def __init__( actions: Optional[Dict[str, Any]] = None, config: Optional[Dict[str, Any]] = None, charm_root: "PathLike" = None, - juju_version: str = "3.0", + juju_version: str = DEFAULT_JUJU_VERSION, capture_deferred_events: bool = False, capture_framework_events: bool = False, ): diff --git a/scenario/mocking.py b/scenario/mocking.py index 11b18a9f..bdd18be3 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -139,6 +139,14 @@ def _get_relation_by_id( raise RelationNotFoundError() def _get_secret(self, id=None, label=None): + # FIXME: what error would a charm get IRL? + # ops 2.0 supports Secret, but juju only supports it from 3.0.2 + if self._context.juju_version < "3.0.2": + raise RuntimeError( + "secrets are only available in juju >= 3.0.2." + "Set ``Context.juju_version`` to 3.0.2+ to use them.", + ) + canonicalize_id = Secret_Ops._canonicalize_id if id: @@ -309,12 +317,16 @@ def _check_can_manage_secret( ): # FIXME: match real tracebacks if secret.owner is None: - raise SecretNotFoundError("this secret is not owned by this unit/app") - if secret.owner == "app" and not self.is_leader(): raise SecretNotFoundError( + "this secret is not owned by this unit/app or granted to it", + ) + if secret.owner == "app" and not self.is_leader(): + understandable_error = SecretNotFoundError( f"App-owned secret {secret.id!r} can only be " f"managed by the leader.", ) + # charm-facing side: respect ops error + raise ModelError("ERROR permission denied") from understandable_error def secret_get( self, @@ -326,8 +338,10 @@ def secret_get( ) -> Dict[str, str]: secret = self._get_secret(id, label) - if self._context.juju_version <= "3.2": - # in juju<3.2, secret owners always track the latest revision. + if self._context.juju_version < "3.1.7": + # 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: refresh = True diff --git a/scenario/state.py b/scenario/state.py index 6f641732..ecbf4195 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -7,6 +7,7 @@ import inspect import re import typing +import warnings from collections import namedtuple from enum import Enum from pathlib import Path, PurePosixPath @@ -138,7 +139,8 @@ class Secret(_DCBase): # if None, the implication is that the secret has been granted to this unit. owner: Literal["unit", "app", None] = None - # deprecated! + # deprecated! if a secret is not granted to this unit, omit it from State.secrets altogether. + # this attribute will be removed in Scenario 7+ granted: Literal["unit", "app", False] = "" # what revision is currently tracked by this charm. Only meaningful if owner=False @@ -155,16 +157,22 @@ class Secret(_DCBase): def __post_init__(self): if self.granted != "": - logger.warning( - "``state.Secret.granted`` is deprecated and will be removed in Scenario 6. " + msg = ( + "``state.Secret.granted`` is deprecated and will be removed in Scenario 7+. " "If a Secret is not owned by the app/unit you are testing, nor has been granted to " - "it by the (remote) owner, then omit it from ``State.secrets`` altogether.", + "it by the (remote) owner, then omit it from ``State.secrets`` altogether." ) + logger.warning(msg) + warnings.warn(msg, DeprecationWarning, stacklevel=2) + if self.owner == "application": - logger.warning( + msg = ( "Secret.owner='application' is deprecated in favour of 'app' " - "and will be removed in Scenario 6.", + "and will be removed in Scenario 7+." ) + logger.warning(msg) + warnings.warn(msg, DeprecationWarning, stacklevel=2) + # bypass frozen dataclass object.__setattr__(self, "owner", "app") @@ -176,7 +184,7 @@ def changed_event(self): raise ValueError( "This unit will never receive secret-changed for a secret it owns.", ) - return Event(name="secret_changed", secret=self) + return Event("secret_changed", secret=self) # owner-only events @property @@ -186,7 +194,7 @@ def rotate_event(self): raise ValueError( "This unit will never receive secret-rotate for a secret it does not own.", ) - return Event(name="secret_rotate", secret=self) + return Event("secret_rotate", secret=self) @property def expired_event(self): @@ -195,7 +203,7 @@ def expired_event(self): raise ValueError( "This unit will never receive secret-expire for a secret it does not own.", ) - return Event(name="secret_expire", secret=self) + return Event("secret_expire", secret=self) @property def remove_event(self): @@ -204,7 +212,7 @@ def remove_event(self): raise ValueError( "This unit will never receive secret-removed for a secret it does not own.", ) - return Event(name="secret_removed", secret=self) + return Event("secret_removed", secret=self) def _set_revision(self, revision: int): """Set a new tracked revision.""" diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 2d099bfb..cb860e18 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -1,8 +1,10 @@ import datetime +import warnings import pytest from ops.charm import CharmBase from ops.framework import Framework +from ops.model import ModelError from ops.model import Secret as ops_Secret from ops.model import SecretNotFoundError, SecretRotate @@ -118,7 +120,7 @@ def test_get_secret_owner_peek_update(mycharm, owner): ), ) as mgr: charm = mgr.charm - assert charm.model.get_secret(id="foo").get_content()["a"] == "c" + 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" @@ -170,9 +172,11 @@ def test_add(mycharm, app): assert secret.label == "mylabel" -def test_set(mycharm): +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"} - with Context(mycharm, meta={"name": "local"}).manager( + with Context(mycharm, meta={"name": "local"}, juju_version="3.1.6").manager( "update_status", State(), ) as mgr: @@ -209,6 +213,37 @@ def test_set(mycharm): } +def test_set(mycharm): + rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} + with Context(mycharm, meta={"name": "local"}).manager( + "update_status", + State(), + ) as mgr: + charm = mgr.charm + secret: ops_Secret = charm.unit.add_secret(rev1, label="mylabel") + assert ( + secret.get_content() + == secret.peek_content() + == secret.get_content(refresh=True) + == rev1 + ) + + secret.set_content(rev2) + 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.secrets[0].contents == { + 0: rev1, + 1: rev2, + 2: rev3, + } + + def test_set_juju33(mycharm): rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} with Context(mycharm, meta={"name": "local"}, juju_version="3.3").manager( @@ -268,6 +303,32 @@ def test_meta(mycharm, app): assert info.rotation == SecretRotate.HOURLY +def test_secret_deprecation_application(mycharm): + with warnings.catch_warnings(record=True) as captured: + s = Secret("123", {}, owner="application") + assert s.owner == "app" + msg = captured[0].message + assert isinstance(msg, DeprecationWarning) + assert msg.args[0] == ( + "Secret.owner='application' is deprecated in favour of " + "'app' and will be removed in Scenario 7+." + ) + + +@pytest.mark.parametrize("granted", ("app", "unit", False)) +def test_secret_deprecation_granted(mycharm, granted): + with warnings.catch_warnings(record=True) as captured: + s = Secret("123", {}, granted=granted) + assert s.granted == granted + msg = captured[0].message + assert isinstance(msg, DeprecationWarning) + assert msg.args[0] == ( + "``state.Secret.granted`` is deprecated and will be removed in Scenario 7+. " + "If a Secret is not owned by the app/unit you are testing, nor has been granted to " + "it by the (remote) owner, then omit it from ``State.secrets`` altogether." + ) + + @pytest.mark.parametrize("leader", (True, False)) @pytest.mark.parametrize("owner", ("app", "unit", None)) def test_secret_permission_model(mycharm, leader, owner): @@ -311,16 +372,18 @@ def test_secret_permission_model(mycharm, leader, owner): assert secret.get_info() secret.set_content({"foo": "boo"}) - assert secret.get_content()["foo"] == "boo" + assert secret.get_content() == {"a": "b"} # rev1! + assert secret.get_content(refresh=True) == {"foo": "boo"} + secret.remove_all_revisions() else: # cannot manage # nothing else to do directly if you can't get a hold of the Secret instance # but we can try some raw backend calls - with pytest.raises(SecretNotFoundError): + with pytest.raises(ModelError): secret.get_info() - with pytest.raises(SecretNotFoundError): + with pytest.raises(ModelError): secret.set_content(content={"boo": "foo"}) From b88efdc4875b868dd267d5a4d729b6317e872270 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 3 Jan 2024 13:41:46 +0100 Subject: [PATCH 10/13] added test fixes --- tests/helpers.py | 4 ++-- tests/test_e2e/test_secrets.py | 21 ++++++++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index 47fbc9f4..230e1854 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -12,7 +12,7 @@ Union, ) -from scenario.context import Context +from scenario.context import Context, DEFAULT_JUJU_VERSION if TYPE_CHECKING: # pragma: no cover from ops.testing import CharmType @@ -36,7 +36,7 @@ def trigger( actions: Optional[Dict[str, Any]] = None, config: Optional[Dict[str, Any]] = None, charm_root: Optional["PathLike"] = None, - juju_version: str = "3.0", + juju_version: str = DEFAULT_JUJU_VERSION, ) -> "State": ctx = Context( charm_type=charm_type, diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 92b5fab3..013be639 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -10,6 +10,7 @@ from scenario import Context from scenario.state import Relation, Secret, State +from tests.helpers import trigger @pytest.fixture(scope="function") @@ -467,13 +468,21 @@ 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")]) + context = Context( + GrantingCharm, meta={"name": "foo", "provides": {"bar": {"interface": "bar"}}} + ) + context.run("start", state) + def test_grant_nonowner(mycharm): def post_event(charm: CharmBase): secret = charm.model.get_secret(id="foo") - with pytest.raises(RuntimeError): - secret = charm.model.get_secret(label="mylabel") - foo = charm.model.get_relation("foo") + + secret = charm.model.get_secret(label="mylabel") + foo = charm.model.get_relation("foo") + + with pytest.raises(ModelError): secret.grant(relation=foo) out = trigger( @@ -510,11 +519,12 @@ def __init__(self, *args): relation_id = 42 state = State( + leader=True, relations=[ Relation( "bar", remote_app_name=relation_remote_app, relation_id=relation_id ) - ] + ], ) with context.manager("start", state) as mgr: @@ -526,7 +536,6 @@ def __init__(self, *args): assert mgr.output.secrets scenario_secret = mgr.output.secrets[0] - assert scenario_secret.granted is False assert relation_remote_app in scenario_secret.remote_grants[relation_id] with context.manager("start", mgr.output) as mgr: @@ -543,5 +552,3 @@ def __init__(self, *args): secret.remove_all_revisions() assert not mgr.output.secrets[0].contents # secret wiped - state = State(leader=leader, relations=[Relation("bar")]) - context.run("start", state) From 863db34aa694eddd4c63fae5ebd3b8d88c9048f7 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 4 Jan 2024 11:40:42 +0100 Subject: [PATCH 11/13] readme fixes --- README.md | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index f101b44b..9ea76493 100644 --- a/README.md +++ b/README.md @@ -191,10 +191,11 @@ def test_statuses(): WaitingStatus('checking this is right...'), ActiveStatus("I am ruled"), ] - + + # similarly you can check the app status history: assert ctx.app_status_history == [ UnknownStatus(), - ActiveStatus(""), + ... ] ``` @@ -434,9 +435,8 @@ that this unit can see). Because of that, `SubordinateRelation`, compared to `Relation`, always talks in terms of `remote`: - `Relation.remote_units_data` becomes `SubordinateRelation.remote_unit_data` taking a single `Dict[str:str]`. The remote unit ID can be provided as a separate argument. -- `Relation.remote_unit_ids` becomes `SubordinateRelation.primary_id` (a single ID instead of a list of IDs) +- `Relation.remote_unit_ids` becomes `SubordinateRelation.remote_unit_id` (a single ID instead of a list of IDs) - `Relation.remote_units_data` becomes `SubordinateRelation.remote_unit_data` (a single databag instead of a mapping from unit IDs to databags) -- `Relation.remote_app_name` maps to `SubordinateRelation.primary_app_name` ```python from scenario.state import SubordinateRelation @@ -657,12 +657,12 @@ def test_pebble_push(): state_in = State( containers=[container] ) - Context( + ctx = Context( MyCharm, - meta={"name": "foo", "containers": {"foo": {}}}).run( - "start", - state_in, + meta={"name": "foo", "containers": {"foo": {}}} ) + + ctx.run("start", state_in) # this is the root of the simulated container filesystem. Any mounts will be symlinks in it. container_root_fs = container.get_filesystem(ctx) @@ -834,14 +834,19 @@ The only mandatory arguments to Secret are its secret ID (which should be unique from revision numbers (integers) to a `str:str` dict representing the payload of the revision. There are three cases: -- the secret is owned by this app, in which case only the leader unit can manage it +- 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 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. + 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. +[note] +If this charm does not own the secret, but also it was not granted view rights by the (remote) owner, you model this in Scenario by _not adding it to State.secrets_! The presence of a `Secret` in `State.secrets` means, in other words, that the charm has view rights (otherwise, why would we put it there?). If the charm owns the secret, or is leader, it will _also_ have manage rights on top of view ones. +[/note] + To specify a secret owned by this unit (or app): ```python From d72aac2034834698c9db8015b4ba61e89f1f501b Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 4 Jan 2024 16:11:31 +0100 Subject: [PATCH 12/13] removed typer dep --- pyproject.toml | 1 - scenario/strategies.py | 0 tests/test_hypothesis.py | 0 3 files changed, 1 deletion(-) create mode 100644 scenario/strategies.py create mode 100644 tests/test_hypothesis.py diff --git a/pyproject.toml b/pyproject.toml index fdaaab4e..e5a26b8d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,7 +20,6 @@ keywords = ["juju", "test"] dependencies = [ "ops>=2.6", "PyYAML>=6.0.1", - "typer==0.7.0", ] readme = "README.md" requires-python = ">=3.8" diff --git a/scenario/strategies.py b/scenario/strategies.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_hypothesis.py b/tests/test_hypothesis.py new file mode 100644 index 00000000..e69de29b From ba7972eef67fd00b539cac862b86448e661397aa Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 9 Jan 2024 08:58:44 +0100 Subject: [PATCH 13/13] fixed once more jujuv check --- scenario/context.py | 25 +++++++++++++++---------- scenario/mocking.py | 10 +++++----- tests/test_e2e/test_secrets.py | 2 +- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/scenario/context.py b/scenario/context.py index 4d8f51ec..7ca9e903 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -132,10 +132,10 @@ def run(self) -> "State": return cast("State", super().run()) def _runner(self): - return self._ctx._run_event + return self._ctx._run_event # noqa def _get_output(self): - return self._ctx._output_state + return self._ctx._output_state # noqa class _ActionManager(_Manager): @@ -146,10 +146,10 @@ def run(self) -> "ActionOutput": return cast("ActionOutput", super().run()) def _runner(self): - return self._ctx._run_action + return self._ctx._run_action # noqa def _get_output(self): - return self._ctx._finalize_action(self._ctx.output_state) + return self._ctx._finalize_action(self._ctx.output_state) # noqa class Context: @@ -199,7 +199,7 @@ def __init__( >>> from scenario import Context, State >>> from ops import ActiveStatus - >>> from charm import MyCharm, MyCustomEvent + >>> from charm import MyCharm, MyCustomEvent # noqa >>> >>> def test_foo(): >>> # Arrange: set the context up @@ -259,6 +259,11 @@ def __init__( self.charm_spec = spec self.charm_root = charm_root self.juju_version = juju_version + if juju_version.split(".")[0] == "2": + logger.warn( + "Juju 2.x is closed and unsupported. You may encounter inconsistencies.", + ) + self._app_name = app_name self._unit_id = unit_id self._tmp = tempfile.TemporaryDirectory() @@ -366,7 +371,7 @@ def _coalesce_event(event: Union[str, Event]) -> Event: if not isinstance(event, Event): raise InvalidEventError(f"Expected Event | str, got {type(event)}") - if event._is_action_event: + if event._is_action_event: # noqa raise InvalidEventError( "Cannot Context.run() action events. " "Use Context.run_action instead.", @@ -396,9 +401,9 @@ def manager( Usage: >>> with Context().manager("start", State()) as manager: - >>> assert manager.charm._some_private_attribute == "foo" + >>> assert manager.charm._some_private_attribute == "foo" # noqa >>> manager.run() # this will fire the event - >>> assert manager.charm._some_private_attribute == "bar" + >>> assert manager.charm._some_private_attribute == "bar" # noqa :arg event: the Event that the charm will respond to. Can be a string or an Event instance. :arg state: the State instance to use as data source for the hook tool calls that the @@ -415,9 +420,9 @@ def action_manager( Usage: >>> with Context().action_manager("foo-action", State()) as manager: - >>> assert manager.charm._some_private_attribute == "foo" + >>> assert manager.charm._some_private_attribute == "foo" # noqa >>> manager.run() # this will fire the event - >>> assert manager.charm._some_private_attribute == "bar" + >>> assert manager.charm._some_private_attribute == "bar" # noqa :arg action: the Action that the charm will execute. Can be a string or an Action instance. :arg state: the State instance to use as data source for the hook tool calls that the diff --git a/scenario/mocking.py b/scenario/mocking.py index 447b44fe..b15394f0 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -169,7 +169,7 @@ def _get_relation_by_id( def _get_secret(self, id=None, label=None): # FIXME: what error would a charm get IRL? - # ops 2.0 supports Secret, but juju only supports it from 3.0.2 + # ops 2.0 supports secrets, but juju only supports it from 3.0.2 if self._context.juju_version < "3.0.2": raise RuntimeError( "secrets are only available in juju >= 3.0.2." @@ -373,10 +373,10 @@ def _check_can_manage_secret( self, secret: "Secret", ): - # FIXME: match real tracebacks if secret.owner is None: raise SecretNotFoundError( - "this secret is not owned by this unit/app or granted to it", + "this secret is not owned by this unit/app or granted to it. " + "Did you forget passing it to State.secrets?", ) if secret.owner == "app" and not self.is_leader(): understandable_error = SecretNotFoundError( @@ -395,8 +395,8 @@ def secret_get( peek: bool = False, ) -> Dict[str, str]: secret = self._get_secret(id, label) - - if self._context.juju_version < "3.1.7": + juju_version = self._context.juju_version + if not (juju_version == "3.1.7" or juju_version >= "3.3.1"): # in this medieval juju chapter, # secret owners always used to track the latest revision. # ref: https://bugs.launchpad.net/juju/+bug/2037120 diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 013be639..e8e75f7b 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -247,7 +247,7 @@ def test_set(mycharm): def test_set_juju33(mycharm): rev1, rev2, rev3 = {"foo": "bar"}, {"foo": "baz"}, {"foo": "baz", "qux": "roz"} - with Context(mycharm, meta={"name": "local"}, juju_version="3.3").manager( + with Context(mycharm, meta={"name": "local"}, juju_version="3.3.1").manager( "update_status", State(), ) as mgr: