From 0dea321dd0784465c6bad38306dd894a8b6016c6 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 29 Nov 2023 13:53:45 +0100 Subject: [PATCH 1/6] extra bindings and network model rework --- README.md | 6 ++ scenario/consistency_checker.py | 35 +++++++ scenario/mocking.py | 32 ++++-- scenario/state.py | 162 ++++++++++++++++-------------- tests/test_consistency_checker.py | 38 +++++++ tests/test_e2e/test_network.py | 38 +++---- 6 files changed, 211 insertions(+), 100 deletions(-) diff --git a/README.md b/README.md index 583dbaad..677427e5 100644 --- a/README.md +++ b/README.md @@ -527,6 +527,12 @@ remote_unit_2_is_joining_event = relation.joined_event(remote_unit_id=2) remote_unit_2_is_joining_event = Event('foo-relation-changed', relation=relation, relation_remote_unit_id=2) ``` +### Networks + +Each relation a charm has will +A charm can define some `extra-bindings` + + # Containers When testing a kubernetes charm, you can mock container interactions. When using the null state (`State()`), there will diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index 80636db8..f939eeab 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -24,6 +24,15 @@ logger = scenario_logger.getChild("consistency_checker") +def get_all_relations(charm_spec: "_CharmSpec"): + nonpeer_relations_meta = chain( + charm_spec.meta.get("requires", {}).items(), + charm_spec.meta.get("provides", {}).items(), + ) + peer_relations_meta = charm_spec.meta.get("peers", {}).items() + return list(chain(nonpeer_relations_meta, peer_relations_meta)) + + class Results(NamedTuple): """Consistency checkers return type.""" @@ -69,6 +78,7 @@ def check_consistency( check_secrets_consistency, check_storages_consistency, check_relation_consistency, + check_network_consistency, ): results = check( state=state, @@ -386,6 +396,31 @@ def check_secrets_consistency( return Results(errors, []) +def check_network_consistency( + *, + state: "State", + event: "Event", # noqa: U100 + charm_spec: "_CharmSpec", + **_kwargs, # noqa: U101 +) -> Results: + errors = [] + + meta_bindings = set(charm_spec.meta.get("extra-bindings", ())) + state_bindings = set(state.extra_bindings) + if diff := state_bindings.difference(meta_bindings): + errors.append( + f"Some extra-bindings defined in State are not in metadata.yaml: {diff}.", + ) + + endpoints = {i[0] for i in get_all_relations(charm_spec)} + if collisions := endpoints.intersection(meta_bindings): + errors.append( + f"Extra bindings and integration endpoints cannot share the same name: {collisions}.", + ) + + return Results(errors, []) + + def check_relation_consistency( *, state: "State", diff --git a/scenario/mocking.py b/scenario/mocking.py index e5bd4298..a6a21f42 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -22,7 +22,7 @@ from ops.testing import _TestingPebbleClient from scenario.logger import logger as scenario_logger -from scenario.state import JujuLogLine, Mount, PeerRelation, Port, Storage +from scenario.state import JujuLogLine, Mount, Network, PeerRelation, Port, Storage if TYPE_CHECKING: from scenario.context import Context @@ -238,15 +238,31 @@ def config_get(self): return state_config # full config def network_get(self, binding_name: str, relation_id: Optional[int] = None): - if relation_id: - logger.warning("network-get -r not implemented") + # is this an extra-binding-provided network? + if binding_name in self._charm_spec.meta.get("extra-bindings", ()): + network = self._state.extra_bindings.get(binding_name, Network.default()) + return network.hook_tool_output_fmt() + # Is this a network attached to a relation? relations = self._state.get_relations(binding_name) - if not relations: - raise RelationNotFoundError() - - network = next(filter(lambda r: r.name == binding_name, self._state.networks)) - return network.hook_tool_output_fmt() + if relation_id: + try: + relation = next( + filter( + lambda r: r.relation_id == relation_id, + relations, + ), + ) + except StopIteration as e: + logger.error( + f"network-get error: " + f"No relation found with endpoint={binding_name} and id={relation_id}.", + ) + raise RelationNotFoundError() from e + else: + # TODO: is this accurate? + relation = relations[0] + return relation.network.hook_tool_output_fmt() # setter methods: these can mutate the state. def application_version_set(self, version: str): diff --git a/scenario/state.py b/scenario/state.py index e5fb1d1c..398c587a 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -224,6 +224,73 @@ def normalize_name(s: str): return s.replace("-", "_") +@dataclasses.dataclass(frozen=True) +class Address(_DCBase): + hostname: str + value: str + cidr: str + address: str = "" # legacy + + +@dataclasses.dataclass(frozen=True) +class BindAddress(_DCBase): + interface_name: str + addresses: List[Address] + mac_address: Optional[str] = None + + def hook_tool_output_fmt(self): + # dumps itself to dict in the same format the hook tool would + # todo support for legacy (deprecated `interfacename` and `macaddress` fields? + dct = { + "interface-name": self.interface_name, + "addresses": [dataclasses.asdict(addr) for addr in self.addresses], + } + if self.mac_address: + dct["mac-address"] = self.mac_address + return dct + + +@dataclasses.dataclass(frozen=True) +class Network(_DCBase): + bind_addresses: List[BindAddress] + ingress_addresses: List[str] + egress_subnets: List[str] + + def hook_tool_output_fmt(self): + # dumps itself to dict in the same format the hook tool would + return { + "bind-addresses": [ba.hook_tool_output_fmt() for ba in self.bind_addresses], + "egress-subnets": self.egress_subnets, + "ingress-addresses": self.ingress_addresses, + } + + @classmethod + def default( + cls, + private_address: str = "1.1.1.1", + hostname: str = "", + cidr: str = "", + interface_name: str = "", + mac_address: Optional[str] = None, + egress_subnets=("1.1.1.2/32",), + ingress_addresses=("1.1.1.2",), + ) -> "Network": + """Helper to create a minimal, heavily defaulted Network.""" + return cls( + bind_addresses=[ + BindAddress( + interface_name=interface_name, + mac_address=mac_address, + addresses=[ + Address(hostname=hostname, value=private_address, cidr=cidr), + ], + ), + ], + egress_subnets=list(egress_subnets), + ingress_addresses=list(ingress_addresses), + ) + + _next_relation_id_counter = 1 @@ -238,15 +305,27 @@ def next_relation_id(update=True): @dataclasses.dataclass(frozen=True) class RelationBase(_DCBase): endpoint: str + """Relation endpoint name. Must match some endpoint name defined in metadata.yaml.""" - # we can derive this from the charm's metadata interface: str = None + """Interface name. Must match the interface name attached to this endpoint in metadata.yaml. + If left empty, it will be automatically derived from metadata.yaml.""" - # Every new Relation instance gets a new one, if there's trouble, override. relation_id: int = dataclasses.field(default_factory=next_relation_id) + """Juju relation ID. Every new Relation instance gets a unique one, + if there's trouble, override.""" local_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) + """This application's databag for this relation.""" + local_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) + """This unit's databag for this relation.""" + + # TODO should we parametrize/randomize this default value to make each + # relation have a different network? + network: Network = dataclasses.field(default=None) + """Network associated with this relation. + If left empty, a default network will be assigned automatically.""" @property def _databags(self): @@ -276,6 +355,9 @@ def __post_init__(self): for databag in self._databags: self._validate_databag(databag) + if self.network is None: + object.__setattr__(self, "network", Network.default()) + def _validate_databag(self, databag: dict): if not isinstance(databag, dict): raise StateValidationError( @@ -587,77 +669,6 @@ def pebble_ready_event(self): return Event(path=normalize_name(self.name + "-pebble-ready"), container=self) -@dataclasses.dataclass(frozen=True) -class Address(_DCBase): - hostname: str - value: str - cidr: str - address: str = "" # legacy - - -@dataclasses.dataclass(frozen=True) -class BindAddress(_DCBase): - interface_name: str - addresses: List[Address] - mac_address: Optional[str] = None - - def hook_tool_output_fmt(self): - # dumps itself to dict in the same format the hook tool would - # todo support for legacy (deprecated `interfacename` and `macaddress` fields? - dct = { - "interface-name": self.interface_name, - "addresses": [dataclasses.asdict(addr) for addr in self.addresses], - } - if self.mac_address: - dct["mac-address"] = self.mac_address - return dct - - -@dataclasses.dataclass(frozen=True) -class Network(_DCBase): - name: str - - bind_addresses: List[BindAddress] - ingress_addresses: List[str] - egress_subnets: List[str] - - def hook_tool_output_fmt(self): - # dumps itself to dict in the same format the hook tool would - return { - "bind-addresses": [ba.hook_tool_output_fmt() for ba in self.bind_addresses], - "egress-subnets": self.egress_subnets, - "ingress-addresses": self.ingress_addresses, - } - - @classmethod - def default( - cls, - name, - private_address: str = "1.1.1.1", - hostname: str = "", - cidr: str = "", - interface_name: str = "", - mac_address: Optional[str] = None, - egress_subnets=("1.1.1.2/32",), - ingress_addresses=("1.1.1.2",), - ) -> "Network": - """Helper to create a minimal, heavily defaulted Network.""" - return cls( - name=name, - bind_addresses=[ - BindAddress( - interface_name=interface_name, - mac_address=mac_address, - addresses=[ - Address(hostname=hostname, value=private_address, cidr=cidr), - ], - ), - ], - egress_subnets=list(egress_subnets), - ingress_addresses=list(ingress_addresses), - ) - - @dataclasses.dataclass(frozen=True) class _EntityStatus(_DCBase): """This class represents StatusBase and should not be interacted with directly.""" @@ -806,8 +817,9 @@ class State(_DCBase): """The present configuration of this charm.""" relations: List["AnyRelation"] = dataclasses.field(default_factory=list) """All relations that currently exist for this charm.""" - networks: List[Network] = dataclasses.field(default_factory=list) - """All networks currently provisioned for this charm.""" + extra_bindings: Dict[str, Network] = dataclasses.field(default_factory=dict) + """All extra bindings currently provisioned for this charm. + If a metadata-defined extra-binding is left empty, it will be defaulted.""" containers: List[Container] = dataclasses.field(default_factory=list) """All containers (whether they can connect or not) that this charm is aware of.""" storage: List[Storage] = dataclasses.field(default_factory=list) diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index bcdc7df8..66415cf9 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -15,6 +15,7 @@ Storage, SubordinateRelation, _CharmSpec, + Network, ) @@ -467,3 +468,40 @@ def test_resource_states(): meta={"name": "yamlman"}, ), ) + + +def test_networks_consistency(): + assert_inconsistent( + State(extra_bindings={"foo": Network.default()}), + Event("start"), + _CharmSpec( + MyCharm, + meta={"name": "wonky"}, + ), + ) + + assert_inconsistent( + State(extra_bindings={"foo": Network.default()}), + Event("start"), + _CharmSpec( + MyCharm, + meta={ + "name": "pinky", + "extra-bindings": {"foo": {}}, + "requires": {"foo": {"interface": "bar"}}, + }, + ), + ) + + assert_consistent( + State(extra_bindings={"foo": Network.default()}), + Event("start"), + _CharmSpec( + MyCharm, + meta={ + "name": "pinky", + "extra-bindings": {"foo": {}}, + "requires": {"bar": {"interface": "bar"}}, + }, + ), + ) diff --git a/tests/test_e2e/test_network.py b/tests/test_e2e/test_network.py index 8c10c6cc..ba707810 100644 --- a/tests/test_e2e/test_network.py +++ b/tests/test_e2e/test_network.py @@ -3,7 +3,8 @@ from ops.charm import CharmBase from ops.framework import Framework -from scenario.state import Event, Network, Relation, State, _CharmSpec +from scenario import Context +from scenario.state import Network, Relation, State from tests.helpers import trigger @@ -28,13 +29,17 @@ def _on_event(self, event): def test_ip_get(mycharm): - mycharm._call = lambda *_: True - - def fetch_unit_address(charm: CharmBase): - rel = charm.model.get_relation("metrics-endpoint") - assert str(charm.model.get_binding(rel).network.bind_address) == "1.1.1.1" + ctx = Context( + mycharm, + meta={ + "name": "foo", + "requires": {"metrics-endpoint": {"interface": "foo"}}, + "extra-bindings": {"foo": {}}, + }, + ) - trigger( + with ctx.manager( + "update_status", State( relations=[ Relation( @@ -44,16 +49,15 @@ def fetch_unit_address(charm: CharmBase): relation_id=1, ), ], - networks=[Network.default("metrics-endpoint")], + extra_bindings={"foo": Network.default(private_address="4.4.4.4")}, ), - "update_status", - mycharm, - meta={ - "name": "foo", - "requires": {"metrics-endpoint": {"interface": "foo"}}, - }, - post_event=fetch_unit_address, - ) + ) as mgr: + # we have a network for the relation + rel = mgr.charm.model.get_relation("metrics-endpoint") + assert str(mgr.charm.model.get_binding(rel).network.bind_address) == "1.1.1.1" + + # and an extra binding + assert str(mgr.charm.model.get_binding("foo").network.bind_address) == "4.4.4.4" def test_no_relation_error(mycharm): @@ -74,7 +78,7 @@ def fetch_unit_address(charm: CharmBase): relation_id=1, ), ], - networks=[Network.default("metrics-endpoint")], + extra_bindings={"foo": Network.default()}, ), "update_status", mycharm, From f76cc742c16e9b6c530c59c30949968f95c78477 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 29 Nov 2023 14:25:54 +0100 Subject: [PATCH 2/6] tolerate dead relations --- README.md | 2 +- pyproject.toml | 2 +- scenario/mocking.py | 39 ++++++++++++++++++- scenario/state.py | 5 ++- tests/test_consistency_checker.py | 2 +- tests/test_e2e/test_network.py | 63 +++++++++++++++++++++++-------- 6 files changed, 91 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 677427e5..5d71117c 100644 --- a/README.md +++ b/README.md @@ -529,7 +529,7 @@ remote_unit_2_is_joining_event = Event('foo-relation-changed', relation=relation ### Networks -Each relation a charm has will +Each relation a charm has is associated with A charm can define some `extra-bindings` diff --git a/pyproject.toml b/pyproject.toml index 84b93d88..611b793c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,7 @@ build-backend = "setuptools.build_meta" [project] name = "ops-scenario" -version = "5.7.1" +version = "6.0" authors = [ { name = "Pietro Pasotti", email = "pietro.pasotti@canonical.com" } diff --git a/scenario/mocking.py b/scenario/mocking.py index a6a21f42..af7dda9d 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -5,6 +5,7 @@ import random import shutil from io import StringIO +from itertools import chain from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional, Set, Tuple, Union @@ -240,9 +241,30 @@ def config_get(self): def network_get(self, binding_name: str, relation_id: Optional[int] = None): # is this an extra-binding-provided network? if binding_name in self._charm_spec.meta.get("extra-bindings", ()): + if relation_id is not None: + # this should not happen + raise RuntimeError( + "cannot pass relation_id to network_get if the binding name is " + "that of an extra-binding. Extra-bindings are not mapped to relation IDs.", + ) network = self._state.extra_bindings.get(binding_name, Network.default()) return network.hook_tool_output_fmt() + # is binding_name a valid relation binding name? + meta = self._charm_spec.meta + if binding_name not in set( + chain( + meta.get("peers", ()), + meta.get("requires", ()), + meta.get("provides", ()), + ), + ): + logger.error( + f"cannot get network binding for {binding_name}: is not a valid relation " + f"endpoint name nor an extra-binding.", + ) + raise RelationNotFoundError() + # Is this a network attached to a relation? relations = self._state.get_relations(binding_name) if relation_id: @@ -260,8 +282,23 @@ def network_get(self, binding_name: str, relation_id: Optional[int] = None): ) raise RelationNotFoundError() from e else: - # TODO: is this accurate? + if not relations: + logger.warning( + "Requesting the network for an endpoint with no active relations " + "will return a defaulted network.", + ) + return Network.default().hook_tool_output_fmt() + + # TODO: is this accurate? Any relation in particular? relation = relations[0] + + from scenario.state import SubordinateRelation # avoid cyclic imports + + if isinstance(relation, SubordinateRelation): + raise RuntimeError( + "Subordinate relation has no associated network binding.", + ) + return relation.network.hook_tool_output_fmt() # setter methods: these can mutate the state. diff --git a/scenario/state.py b/scenario/state.py index 398c587a..18d18d47 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -325,7 +325,8 @@ class RelationBase(_DCBase): # relation have a different network? network: Network = dataclasses.field(default=None) """Network associated with this relation. - If left empty, a default network will be assigned automatically.""" + If left empty, a default network will be assigned automatically + (except for subordinate relations).""" @property def _databags(self): @@ -355,7 +356,7 @@ def __post_init__(self): for databag in self._databags: self._validate_databag(databag) - if self.network is None: + if type(self) is not SubordinateRelation and self.network is None: object.__setattr__(self, "network", Network.default()) def _validate_databag(self, databag: dict): diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index 66415cf9..bb8d75da 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -8,6 +8,7 @@ Action, Container, Event, + Network, PeerRelation, Relation, Secret, @@ -15,7 +16,6 @@ Storage, SubordinateRelation, _CharmSpec, - Network, ) diff --git a/tests/test_e2e/test_network.py b/tests/test_e2e/test_network.py index ba707810..cdfcf530 100644 --- a/tests/test_e2e/test_network.py +++ b/tests/test_e2e/test_network.py @@ -4,7 +4,7 @@ from ops.framework import Framework from scenario import Context -from scenario.state import Network, Relation, State +from scenario.state import Network, Relation, State, SubordinateRelation from tests.helpers import trigger @@ -33,7 +33,10 @@ def test_ip_get(mycharm): mycharm, meta={ "name": "foo", - "requires": {"metrics-endpoint": {"interface": "foo"}}, + "requires": { + "metrics-endpoint": {"interface": "foo"}, + "deadnodead": {"interface": "bar"}, + }, "extra-bindings": {"foo": {}}, }, ) @@ -56,19 +59,52 @@ def test_ip_get(mycharm): rel = mgr.charm.model.get_relation("metrics-endpoint") assert str(mgr.charm.model.get_binding(rel).network.bind_address) == "1.1.1.1" + # we have a network for a binding without relations on it + assert ( + str(mgr.charm.model.get_binding("deadnodead").network.bind_address) + == "1.1.1.1" + ) + # and an extra binding assert str(mgr.charm.model.get_binding("foo").network.bind_address) == "4.4.4.4" +def test_no_sub_binding(mycharm): + ctx = Context( + mycharm, + meta={ + "name": "foo", + "requires": {"bar": {"interface": "foo", "scope": "container"}}, + }, + ) + + with ctx.manager( + "update_status", + State( + relations=[ + SubordinateRelation("bar"), + ] + ), + ) as mgr: + with pytest.raises(RuntimeError): + # sub relations have no network + mgr.charm.model.get_binding("bar").network + + def test_no_relation_error(mycharm): """Attempting to call get_binding on a non-existing relation -> RelationNotFoundError""" - mycharm._call = lambda *_: True - def fetch_unit_address(charm: CharmBase): - with pytest.raises(RelationNotFoundError): - _ = charm.model.get_binding("foo").network + ctx = Context( + mycharm, + meta={ + "name": "foo", + "requires": {"metrics-endpoint": {"interface": "foo"}}, + "extra-bindings": {"bar": {}}, + }, + ) - trigger( + with ctx.manager( + "update_status", State( relations=[ Relation( @@ -78,13 +114,8 @@ def fetch_unit_address(charm: CharmBase): relation_id=1, ), ], - extra_bindings={"foo": Network.default()}, + extra_bindings={"bar": Network.default()}, ), - "update_status", - mycharm, - meta={ - "name": "foo", - "requires": {"metrics-endpoint": {"interface": "foo"}}, - }, - post_event=fetch_unit_address, - ) + ) as mgr: + with pytest.raises(RelationNotFoundError): + net = mgr.charm.model.get_binding("foo").network From d02b8bb0051df722af9e936e4ede657ceaa2ae93 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 30 Nov 2023 10:25:04 +0100 Subject: [PATCH 3/6] re-rework --- README.md | 2 +- scenario/consistency_checker.py | 31 ++++------ scenario/mocking.py | 96 +++++++++++-------------------- scenario/state.py | 44 ++++++++------ tests/test_consistency_checker.py | 6 +- tests/test_e2e/test_network.py | 6 +- 6 files changed, 77 insertions(+), 108 deletions(-) diff --git a/README.md b/README.md index 5d71117c..919ed795 100644 --- a/README.md +++ b/README.md @@ -529,7 +529,7 @@ remote_unit_2_is_joining_event = Event('foo-relation-changed', relation=relation ### Networks -Each relation a charm has is associated with +Simplifying a bit the Juju "spaces" model, each integration endpoint a charm defines in its metadata is associated with a network. Regardless of whether there is a living relation over that endpoint, that is. A charm can define some `extra-bindings` diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index f939eeab..c494e746 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -4,7 +4,6 @@ import os from collections import Counter from collections.abc import Sequence -from itertools import chain from numbers import Number from typing import TYPE_CHECKING, Iterable, List, NamedTuple, Tuple @@ -24,15 +23,6 @@ logger = scenario_logger.getChild("consistency_checker") -def get_all_relations(charm_spec: "_CharmSpec"): - nonpeer_relations_meta = chain( - charm_spec.meta.get("requires", {}).items(), - charm_spec.meta.get("provides", {}).items(), - ) - peer_relations_meta = charm_spec.meta.get("peers", {}).items() - return list(chain(nonpeer_relations_meta, peer_relations_meta)) - - class Results(NamedTuple): """Consistency checkers return type.""" @@ -406,13 +396,20 @@ def check_network_consistency( errors = [] meta_bindings = set(charm_spec.meta.get("extra-bindings", ())) - state_bindings = set(state.extra_bindings) - if diff := state_bindings.difference(meta_bindings): + all_relations = charm_spec.get_all_relations() + non_sub_relations = { + endpoint + for endpoint, metadata in all_relations + if metadata.get("scope") != "container" # mark of a sub + } + + state_bindings = set(state.networks) + if diff := state_bindings.difference(meta_bindings.union(non_sub_relations)): errors.append( - f"Some extra-bindings defined in State are not in metadata.yaml: {diff}.", + f"Some network bindings defined in State are not in metadata.yaml: {diff}.", ) - endpoints = {i[0] for i in get_all_relations(charm_spec)} + endpoints = {endpoint for endpoint, metadata in all_relations} if collisions := endpoints.intersection(meta_bindings): errors.append( f"Extra bindings and integration endpoints cannot share the same name: {collisions}.", @@ -429,12 +426,8 @@ def check_relation_consistency( **_kwargs, # noqa: U101 ) -> Results: errors = [] - nonpeer_relations_meta = chain( - charm_spec.meta.get("requires", {}).items(), - charm_spec.meta.get("provides", {}).items(), - ) peer_relations_meta = charm_spec.meta.get("peers", {}).items() - all_relations_meta = list(chain(nonpeer_relations_meta, peer_relations_meta)) + all_relations_meta = charm_spec.get_all_relations() def _get_relations(r): try: diff --git a/scenario/mocking.py b/scenario/mocking.py index af7dda9d..fa43fe26 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -5,7 +5,6 @@ import random import shutil from io import StringIO -from itertools import chain from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional, Set, Tuple, Union @@ -145,20 +144,21 @@ def _get_secret(self, id=None, label=None): # in scenario, you can create Secret(id="foo"), # but ops.Secret will prepend a "secret:" prefix to that ID. # we allow getting secret by either version. - try: - return next( - filter( - lambda s: canonicalize_id(s.id) == canonicalize_id(id), - self._state.secrets, - ), - ) - except StopIteration: - raise SecretNotFoundError() + secrets = [ + s + for s in self._state.secrets + if canonicalize_id(s.id) == canonicalize_id(id) + ] + if not secrets: + raise SecretNotFoundError(id) + return secrets[0] + elif label: - try: - return next(filter(lambda s: s.label == label, self._state.secrets)) - except StopIteration: - raise SecretNotFoundError() + secrets = [s for s in self._state.secrets if s.label == label] + if not secrets: + raise SecretNotFoundError(label) + return secrets[0] + else: # if all goes well, this should never be reached. ops.model.Secret will check upon # instantiation that either an id or a label are set, and raise a TypeError if not. @@ -239,67 +239,35 @@ def config_get(self): return state_config # full config def network_get(self, binding_name: str, relation_id: Optional[int] = None): - # is this an extra-binding-provided network? - if binding_name in self._charm_spec.meta.get("extra-bindings", ()): + # validation: + extra_bindings = self._charm_spec.meta.get("extra-bindings", ()) + all_endpoints = self._charm_spec.get_all_relations() + non_sub_relations = { + name for name, meta in all_endpoints if meta.get("scope") != "container" + } + + # - is binding_name a valid binding name? + if binding_name in extra_bindings: + logger.warning("extra-bindings is a deprecated feature") # fyi + + # - verify that if the binding is an extra binding, we're not ignoring a relation_id if relation_id is not None: # this should not happen - raise RuntimeError( + logger.error( "cannot pass relation_id to network_get if the binding name is " "that of an extra-binding. Extra-bindings are not mapped to relation IDs.", ) - network = self._state.extra_bindings.get(binding_name, Network.default()) - return network.hook_tool_output_fmt() - - # is binding_name a valid relation binding name? - meta = self._charm_spec.meta - if binding_name not in set( - chain( - meta.get("peers", ()), - meta.get("requires", ()), - meta.get("provides", ()), - ), - ): + # - verify that the binding is a relation endpoint name, but not a subordinate one + elif binding_name not in non_sub_relations: logger.error( f"cannot get network binding for {binding_name}: is not a valid relation " f"endpoint name nor an extra-binding.", ) raise RelationNotFoundError() - # Is this a network attached to a relation? - relations = self._state.get_relations(binding_name) - if relation_id: - try: - relation = next( - filter( - lambda r: r.relation_id == relation_id, - relations, - ), - ) - except StopIteration as e: - logger.error( - f"network-get error: " - f"No relation found with endpoint={binding_name} and id={relation_id}.", - ) - raise RelationNotFoundError() from e - else: - if not relations: - logger.warning( - "Requesting the network for an endpoint with no active relations " - "will return a defaulted network.", - ) - return Network.default().hook_tool_output_fmt() - - # TODO: is this accurate? Any relation in particular? - relation = relations[0] - - from scenario.state import SubordinateRelation # avoid cyclic imports - - if isinstance(relation, SubordinateRelation): - raise RuntimeError( - "Subordinate relation has no associated network binding.", - ) - - return relation.network.hook_tool_output_fmt() + # We look in State.networks for an override. If not given, we return a default network. + network = self._state.networks.get(binding_name, Network.default()) + return network.hook_tool_output_fmt() # setter methods: these can mutate the state. def application_version_set(self, version: str): diff --git a/scenario/state.py b/scenario/state.py index 18d18d47..7ec2f951 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -9,6 +9,7 @@ import typing from collections import namedtuple from enum import Enum +from itertools import chain from pathlib import Path, PurePosixPath from typing import Any, Callable, Dict, List, Literal, Optional, Set, Tuple, Type, Union from uuid import uuid4 @@ -321,13 +322,6 @@ class RelationBase(_DCBase): local_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) """This unit's databag for this relation.""" - # TODO should we parametrize/randomize this default value to make each - # relation have a different network? - network: Network = dataclasses.field(default=None) - """Network associated with this relation. - If left empty, a default network will be assigned automatically - (except for subordinate relations).""" - @property def _databags(self): """Yield all databags in this relation.""" @@ -356,9 +350,6 @@ def __post_init__(self): for databag in self._databags: self._validate_databag(databag) - if type(self) is not SubordinateRelation and self.network is None: - object.__setattr__(self, "network", Network.default()) - def _validate_databag(self, databag: dict): if not isinstance(databag, dict): raise StateValidationError( @@ -818,9 +809,14 @@ class State(_DCBase): """The present configuration of this charm.""" relations: List["AnyRelation"] = dataclasses.field(default_factory=list) """All relations that currently exist for this charm.""" - extra_bindings: Dict[str, Network] = dataclasses.field(default_factory=dict) - """All extra bindings currently provisioned for this charm. - If a metadata-defined extra-binding is left empty, it will be defaulted.""" + networks: Dict[str, Network] = dataclasses.field(default_factory=dict) + """Manual overrides for any relation and extra bindings currently provisioned for this charm. + If a metadata-defined relation endpoint is not explicitly mapped to a Network in this field, + it will be defaulted. + [CAVEAT: `extra-bindings` is a deprecated, regretful feature in juju/ops. For completeness we + support it, but use at your own risk.] If a metadata-defined extra-binding is left empty, + it will be defaulted. + """ containers: List[Container] = dataclasses.field(default_factory=list) """All containers (whether they can connect or not) that this charm is aware of.""" storage: List[Storage] = dataclasses.field(default_factory=list) @@ -912,11 +908,13 @@ def with_unit_status(self, status: StatusBase) -> "State": def get_container(self, container: Union[str, Container]) -> Container: """Get container from this State, based on an input container or its name.""" - name = container.name if isinstance(container, Container) else container - try: - return next(filter(lambda c: c.name == name, self.containers)) - except StopIteration as e: - raise ValueError(f"container: {name}") from e + container_name = ( + container.name if isinstance(container, Container) else container + ) + containers = [c for c in self.containers if c.name == container_name] + if not containers: + raise ValueError(f"container: {container_name} not found in the State") + return containers[0] def get_relations(self, endpoint: str) -> Tuple["AnyRelation", ...]: """Get all relations on this endpoint from the current state.""" @@ -999,6 +997,16 @@ def autoload(charm_type: Type["CharmType"]): is_autoloaded=True, ) + def get_all_relations(self) -> List[Tuple[str, Dict[str, str]]]: + """A list of all relation endpoints defined in the metadata.""" + return list( + chain( + self.meta.get("requires", {}).items(), + self.meta.get("provides", {}).items(), + self.meta.get("peers", {}).items(), + ), + ) + def sort_patch(patch: List[Dict], key=lambda obj: obj["path"] + obj["op"]): return sorted(patch, key=key) diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index bb8d75da..ef92e6d9 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -472,7 +472,7 @@ def test_resource_states(): def test_networks_consistency(): assert_inconsistent( - State(extra_bindings={"foo": Network.default()}), + State(networks={"foo": Network.default()}), Event("start"), _CharmSpec( MyCharm, @@ -481,7 +481,7 @@ def test_networks_consistency(): ) assert_inconsistent( - State(extra_bindings={"foo": Network.default()}), + State(networks={"foo": Network.default()}), Event("start"), _CharmSpec( MyCharm, @@ -494,7 +494,7 @@ def test_networks_consistency(): ) assert_consistent( - State(extra_bindings={"foo": Network.default()}), + State(networks={"foo": Network.default()}), Event("start"), _CharmSpec( MyCharm, diff --git a/tests/test_e2e/test_network.py b/tests/test_e2e/test_network.py index cdfcf530..997ad331 100644 --- a/tests/test_e2e/test_network.py +++ b/tests/test_e2e/test_network.py @@ -52,7 +52,7 @@ def test_ip_get(mycharm): relation_id=1, ), ], - extra_bindings={"foo": Network.default(private_address="4.4.4.4")}, + networks={"foo": Network.default(private_address="4.4.4.4")}, ), ) as mgr: # we have a network for the relation @@ -86,7 +86,7 @@ def test_no_sub_binding(mycharm): ] ), ) as mgr: - with pytest.raises(RuntimeError): + with pytest.raises(RelationNotFoundError): # sub relations have no network mgr.charm.model.get_binding("bar").network @@ -114,7 +114,7 @@ def test_no_relation_error(mycharm): relation_id=1, ), ], - extra_bindings={"bar": Network.default()}, + networks={"bar": Network.default()}, ), ) as mgr: with pytest.raises(RelationNotFoundError): From 9928aa9674bd38a9b93078bf36ab19489276d4ba Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 30 Nov 2023 10:32:48 +0100 Subject: [PATCH 4/6] readme --- README.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 919ed795..6a523b78 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,7 @@ A scenario test consists of three broad steps: - optionally, you can use a context manager to get a hold of the charm instance and run assertions on internal APIs and the internal state of the charm and operator framework. The most basic scenario is one in which all is defaulted and barely any data is -available. The charm has no config, no relations, no networks, no leadership, and its status is `unknown`. +available. The charm has no config, no relations, no leadership, and its status is `unknown`. With that, we can write the simplest possible scenario test: @@ -530,8 +530,22 @@ remote_unit_2_is_joining_event = Event('foo-relation-changed', relation=relation ### Networks Simplifying a bit the Juju "spaces" model, each integration endpoint a charm defines in its metadata is associated with a network. Regardless of whether there is a living relation over that endpoint, that is. -A charm can define some `extra-bindings` +If your charm has a relation `"foo"` (defined in metadata.yaml), then the charm will be able at runtime to do `self.model.get_binding("foo").network`. +The network you'll get by doing so is heavily defaulted (see `state.Network.default`) and good for most use-cases because the charm should typically not be concerned about what IP it gets. + +On top of the relation-provided network bindings, a charm can also define some `extra-bindings` in its metadata.yaml and access them at runtime. Note that this is a deprecated feature that should not be relied upon. For completeness, we support it in Scenario. + +If you want to, you can override any of these relation or extra-binding associated networks with a custom one by passing it to `State.networks`. + +```python +from scenario import State, Network +state = State(networks={ + 'foo': Network.default(private_address='4.4.4.4') +}) +``` + +Where `foo` can either be the name of an `extra-bindings`-defined binding, or a relation endpoint. # Containers From bd28d7933a8e090d69104bc461f512678de4e6b2 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 5 Dec 2023 10:10:01 +0100 Subject: [PATCH 5/6] pr comments --- scenario/state.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scenario/state.py b/scenario/state.py index 7ec2f951..6c38076b 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -241,7 +241,7 @@ class BindAddress(_DCBase): def hook_tool_output_fmt(self): # dumps itself to dict in the same format the hook tool would - # todo support for legacy (deprecated `interfacename` and `macaddress` fields? + # todo support for legacy (deprecated) `interfacename` and `macaddress` fields? dct = { "interface-name": self.interface_name, "addresses": [dataclasses.asdict(addr) for addr in self.addresses], @@ -268,13 +268,13 @@ def hook_tool_output_fmt(self): @classmethod def default( cls, - private_address: str = "1.1.1.1", + private_address: str = "192.0.2.0", hostname: str = "", cidr: str = "", interface_name: str = "", mac_address: Optional[str] = None, - egress_subnets=("1.1.1.2/32",), - ingress_addresses=("1.1.1.2",), + egress_subnets=("192.0.2.0/24",), + ingress_addresses=("192.0.2.0",), ) -> "Network": """Helper to create a minimal, heavily defaulted Network.""" return cls( From 3af35d0e44a77b96999c872da9cb3e0fc906e201 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 5 Dec 2023 10:25:03 +0100 Subject: [PATCH 6/6] fixed network tests --- tests/test_e2e/test_network.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_e2e/test_network.py b/tests/test_e2e/test_network.py index 997ad331..b0f7b8a4 100644 --- a/tests/test_e2e/test_network.py +++ b/tests/test_e2e/test_network.py @@ -57,12 +57,12 @@ def test_ip_get(mycharm): ) as mgr: # we have a network for the relation rel = mgr.charm.model.get_relation("metrics-endpoint") - assert str(mgr.charm.model.get_binding(rel).network.bind_address) == "1.1.1.1" + assert str(mgr.charm.model.get_binding(rel).network.bind_address) == "192.0.2.0" # we have a network for a binding without relations on it assert ( str(mgr.charm.model.get_binding("deadnodead").network.bind_address) - == "1.1.1.1" + == "192.0.2.0" ) # and an extra binding