diff --git a/README.md b/README.md index 583dbaad..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: @@ -527,6 +527,26 @@ 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 + +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. + +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 When testing a kubernetes charm, you can mock container interactions. When using the null state (`State()`), there will 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/consistency_checker.py b/scenario/consistency_checker.py index 80636db8..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 @@ -69,6 +68,7 @@ def check_consistency( check_secrets_consistency, check_storages_consistency, check_relation_consistency, + check_network_consistency, ): results = check( state=state, @@ -386,6 +386,38 @@ 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", ())) + 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 network bindings defined in State are not in metadata.yaml: {diff}.", + ) + + 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}.", + ) + + return Results(errors, []) + + def check_relation_consistency( *, state: "State", @@ -394,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 e5bd4298..fa43fe26 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 @@ -144,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. @@ -238,14 +239,34 @@ 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") - - relations = self._state.get_relations(binding_name) - if not relations: + # 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 + 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.", + ) + # - 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() - network = next(filter(lambda r: r.name == binding_name, self._state.networks)) + # 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. diff --git a/scenario/state.py b/scenario/state.py index e5fb1d1c..6c38076b 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 @@ -224,6 +225,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 = "192.0.2.0", + hostname: str = "", + cidr: str = "", + interface_name: str = "", + mac_address: Optional[str] = None, + egress_subnets=("192.0.2.0/24",), + ingress_addresses=("192.0.2.0",), + ) -> "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 +306,21 @@ 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.""" @property def _databags(self): @@ -587,77 +661,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 +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.""" - networks: List[Network] = dataclasses.field(default_factory=list) - """All networks currently provisioned for this charm.""" + 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) @@ -899,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.""" @@ -986,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 bcdc7df8..ef92e6d9 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -8,6 +8,7 @@ Action, Container, Event, + Network, PeerRelation, Relation, Secret, @@ -467,3 +468,40 @@ def test_resource_states(): meta={"name": "yamlman"}, ), ) + + +def test_networks_consistency(): + assert_inconsistent( + State(networks={"foo": Network.default()}), + Event("start"), + _CharmSpec( + MyCharm, + meta={"name": "wonky"}, + ), + ) + + assert_inconsistent( + State(networks={"foo": Network.default()}), + Event("start"), + _CharmSpec( + MyCharm, + meta={ + "name": "pinky", + "extra-bindings": {"foo": {}}, + "requires": {"foo": {"interface": "bar"}}, + }, + ), + ) + + assert_consistent( + State(networks={"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..b0f7b8a4 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, SubordinateRelation from tests.helpers import trigger @@ -28,13 +29,20 @@ 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"}, + "deadnodead": {"interface": "bar"}, + }, + "extra-bindings": {"foo": {}}, + }, + ) - trigger( + with ctx.manager( + "update_status", State( relations=[ Relation( @@ -44,27 +52,59 @@ def fetch_unit_address(charm: CharmBase): relation_id=1, ), ], - networks=[Network.default("metrics-endpoint")], + networks={"foo": Network.default(private_address="4.4.4.4")}, ), - "update_status", + ) 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) == "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) + == "192.0.2.0" + ) + + # 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": {"metrics-endpoint": {"interface": "foo"}}, + "requires": {"bar": {"interface": "foo", "scope": "container"}}, }, - post_event=fetch_unit_address, ) + with ctx.manager( + "update_status", + State( + relations=[ + SubordinateRelation("bar"), + ] + ), + ) as mgr: + with pytest.raises(RelationNotFoundError): + # 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( @@ -74,13 +114,8 @@ def fetch_unit_address(charm: CharmBase): relation_id=1, ), ], - networks=[Network.default("metrics-endpoint")], + networks={"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