From 85cdcda9c4eb8845b88f1cb979529e0341b3e50f Mon Sep 17 00:00:00 2001 From: natalia Date: Fri, 9 Feb 2024 17:38:37 +0100 Subject: [PATCH 1/7] feat: add oathkeeper-info interface --- lib/charms/oathkeeper/v0/oathkeeper_info.py | 194 ++++++++++++++++++++ metadata.yaml | 4 + pyproject.toml | 2 +- src/charm.py | 38 +++- tests/unit/capture_events.py | 85 +++++++++ tests/unit/conftest.py | 10 + tests/unit/test_charm.py | 59 +++++- 7 files changed, 388 insertions(+), 4 deletions(-) create mode 100644 lib/charms/oathkeeper/v0/oathkeeper_info.py create mode 100644 tests/unit/capture_events.py diff --git a/lib/charms/oathkeeper/v0/oathkeeper_info.py b/lib/charms/oathkeeper/v0/oathkeeper_info.py new file mode 100644 index 00000000..e210d562 --- /dev/null +++ b/lib/charms/oathkeeper/v0/oathkeeper_info.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python3 +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Interface library for sharing oathkeeper info. + +This library provides a Python API for both requesting and providing oathkeeper deployment info, +such as endpoints, namespace and ConfigMap details. +## Getting Started +To get started using the library, you need to fetch the library using `charmcraft`. +```shell +cd some-charm +charmcraft fetch-lib charms.oathkeeper.v0.oathkeeper_info +``` +To use the library from the requirer side: +In the `metadata.yaml` of the charm, add the following: +```yaml +requires: + oathkeeper-info: + interface: oathkeeper_info + limit: 1 +``` +Then, to initialise the library: +```python +from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRequirer +Class SomeCharm(CharmBase): + def __init__(self, *args): + self.oathkeeper_info_relation = OathkeeperInfoRequirer(self) + self.framework.observe(self.on.some_event_emitted, self.some_event_function) + def some_event_function(): + # fetch the relation info + if self.oathkeeper_info_relation.is_ready(): + oathkeeper_data = self.oathkeeper_info_relation.get_oathkeeper_info() +``` +""" + +import logging +from typing import Dict, Optional + +from ops.charm import CharmBase, RelationCreatedEvent +from ops.framework import EventBase, EventSource, Object, ObjectEvents + +# The unique Charmhub library identifier, never change it +LIBID = "c801a227f45b46099d7f87cff2dc6e39" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 1 + +RELATION_NAME = "oathkeeper-info" +INTERFACE_NAME = "oathkeeper_info" +logger = logging.getLogger(__name__) + + +class OathkeeperInfoRelationReadyEvent(EventBase): + """Event to notify the charm that the relation is ready.""" + + +class OathkeeperInfoProviderEvents(ObjectEvents): + """Event descriptor for events raised by `OathkeeperInfoProvider`.""" + + ready = EventSource(OathkeeperInfoRelationReadyEvent) + + +class OathkeeperInfoProvider(Object): + """Provider side of the oathkeeper-info relation.""" + + on = OathkeeperInfoProviderEvents() + + def __init__(self, charm: CharmBase, relation_name: str = RELATION_NAME): + super().__init__(charm, relation_name) + + self._charm = charm + self._relation_name = relation_name + + events = self._charm.on[relation_name] + self.framework.observe(events.relation_created, self._on_info_provider_relation_created) + + def _on_info_provider_relation_created(self, event: RelationCreatedEvent) -> None: + self.on.ready.emit() + + def send_info_relation_data( + self, + public_endpoint: str, + rules_configmap_name: str, + configmaps_namespace: str, + ) -> None: + """Updates relation with endpoints and configmaps info.""" + if not self._charm.unit.is_leader(): + return + + relations = self.model.relations[self._relation_name] + info_databag = { + "public_endpoint": public_endpoint, + "rules_configmap_name": rules_configmap_name, + "configmaps_namespace": configmaps_namespace, + } + + for relation in relations: + relation.data[self._charm.app].update(info_databag) + + def get_requirer_info(self) -> Optional[str]: + """Get the requirer info.""" + info = self.model.relations[self._relation_name] + if len(info) == 0: + return None + + if not (app := info[0].app): + return None + + data = info[0].data[app] + + if not data: + logger.info("No relation data available.") + return None + + return data["access_rules_file"] + + +class OathkeeperInfoRelationError(Exception): + """Base class for the relation exceptions.""" + + pass + + +class OathkeeperInfoRelationMissingError(OathkeeperInfoRelationError): + """Raised when the relation is missing.""" + + def __init__(self) -> None: + self.message = "Missing oathkeeper-info relation with oathkeeper" + super().__init__(self.message) + + +class OathkeeperInfoRelationDataMissingError(OathkeeperInfoRelationError): + """Raised when information is missing from the relation.""" + + def __init__(self, message: str) -> None: + self.message = message + super().__init__(self.message) + + +class OathkeeperInfoRequirer(Object): + """Requirer side of the oathkeeper-info relation.""" + + def __init__(self, charm: CharmBase, relation_name: str = RELATION_NAME): + super().__init__(charm, relation_name) + self._charm = charm + self._relation_name = relation_name + + def update_requirer_info_relation_data( + self, + access_rules_file: str, + ) -> None: + """Updates relation with access rules info.""" + if not self._charm.unit.is_leader(): + return + + relations = self.model.relations[self._relation_name] + info_databag = { + "access_rules_file": access_rules_file, + } + + for relation in relations: + relation.data[self._charm.app].update(info_databag) + + def is_ready(self) -> bool: + """Checks whether the relation data is ready. + + Returns True when Oathkeeper shared the config; False otherwise. + """ + relation = self.model.get_relation(self._relation_name) + if not relation or not relation.app or not relation.data[relation.app]: + return False + return True + + def get_oathkeeper_info(self) -> Optional[Dict]: + """Get the oathkeeper info.""" + info = self.model.relations[self._relation_name] + if len(info) == 0: + raise OathkeeperInfoRelationMissingError() + + if not (app := info[0].app): + raise OathkeeperInfoRelationMissingError() + + data = info[0].data[app] + + if not data: + logger.info("No relation data available.") + raise OathkeeperInfoRelationDataMissingError("Missing relation data") + + return data diff --git a/metadata.yaml b/metadata.yaml index 969d9863..b2f3196f 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -22,6 +22,10 @@ provides: interface: auth_proxy forward-auth: interface: forward_auth + oathkeeper-info: + interface: oathkeeper_info + description: | + Provides oathkeeper deployment info to a related application requires: kratos-endpoint-info: interface: kratos_endpoints diff --git a/pyproject.toml b/pyproject.toml index fcf30553..dcbc8d47 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,7 +26,7 @@ profile = "black" max-line-length = 99 max-doc-length = 99 max-complexity = 10 -exclude = [".git", "__pycache__", ".tox", "build", "dist", "*.egg_info", "venv"] +exclude = [".git", "__pycache__", ".tox", "build", "dist", "*.egg_info", "venv", "tests/unit/capture_events.py"] select = ["E", "W", "F", "C", "N", "R", "D", "H"] # Ignore W503, E501 because using black creates errors with this # Ignore D107 Missing docstring in __init__ diff --git a/src/charm.py b/src/charm.py index 6738f3d8..66bd8d76 100755 --- a/src/charm.py +++ b/src/charm.py @@ -29,6 +29,7 @@ ForwardAuthRelationRemovedEvent, InvalidForwardAuthConfigEvent, ) +from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoProvider from charms.observability_libs.v0.cert_handler import CertChanged, CertHandler from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch from charms.traefik_k8s.v2.ingress import ( @@ -111,6 +112,7 @@ def __init__(self, *args): relation_name=self._forward_auth_relation_name, forward_auth_config=self._forward_auth_config, ) + self.info_provider = OathkeeperInfoProvider(self) self.service_patcher = KubernetesServicePatch( self, [("oathkeeper-api", OATHKEEPER_API_PORT)] @@ -160,6 +162,10 @@ def __init__(self, *args): self._on_forward_auth_relation_removed, ) + self.framework.observe( + self.info_provider.on.ready, self._update_oathkeeper_info_relation_data + ) + self.framework.observe(self.cert_handler.on.cert_changed, self._on_cert_changed) self.framework.observe(self.on.list_rules_action, self._on_list_rules_action) @@ -231,9 +237,21 @@ def _oathkeeper_service_is_running(self) -> bool: return service.is_running() @property - def _forward_auth_config(self) -> ForwardAuthConfig: + def _scheme(self) -> str: scheme = "https" if self._is_tls_ready() and not self.config["dev"] else "http" - decisions_url = f"{scheme}://{self.app.name}.{self.model.name}.svc.cluster.local:{OATHKEEPER_API_PORT}/decisions" + return scheme + + @property + def _public_endpoint(self) -> str: + public_endpoint = ( + self.ingress.url + or f"{self._scheme}://{self.app.name}.{self.model.name}.svc.cluster.local:{OATHKEEPER_API_PORT}" + ) + return public_endpoint + + @property + def _forward_auth_config(self) -> ForwardAuthConfig: + decisions_url = f"{self._scheme}://{self.app.name}.{self.model.name}.svc.cluster.local:{OATHKEEPER_API_PORT}/decisions" return ForwardAuthConfig( decisions_address=decisions_url, app_names=self.auth_proxy.get_app_names(), @@ -257,6 +275,8 @@ def _get_all_access_rules_repositories(self) -> Optional[List]: if cm_access_rules := self.access_rules_configmap.get(): for key in cm_access_rules.keys(): repositories.append(f"{self._access_rules_dir_path}/{key}") + if admin_ui_access_rules_file := self.info_provider.get_requirer_info(): + repositories.append(admin_ui_access_rules_file) return repositories def _render_conf_file(self) -> str: @@ -292,6 +312,15 @@ def _update_config(self) -> None: self.cert_handler.cert, self.cert_handler.key, self.cert_handler.ca ) + def _update_oathkeeper_info_relation_data(self, event: HookEvent) -> None: + logger.info("Sending oathkeeper info") + + self.info_provider.send_info_relation_data( + public_endpoint=self._public_endpoint, + rules_configmap_name=self.access_rules_configmap.name, + configmaps_namespace=self.model.name, + ) + def _get_kratos_endpoint_info(self, key: str) -> Optional[str]: if not self.model.relations[self._kratos_relation_name]: logger.info("Kratos relation not found") @@ -388,6 +417,7 @@ def _on_oathkeeper_pebble_ready(self, event: PebbleReadyEvent) -> None: """Event Handler for pebble ready event.""" self._patch_statefulset() self._handle_status_update_config(event) + self._update_oathkeeper_info_relation_data(event) def _on_config_changed(self, event: ConfigChangedEvent): self.forward_auth.update_forward_auth_config(self._forward_auth_config) @@ -405,10 +435,14 @@ def _on_ingress_ready(self, event: IngressPerAppReadyEvent) -> None: if self.unit.is_leader(): logger.info(f"This app's ingress URL: {event.url}") + self._update_oathkeeper_info_relation_data(event) + def _on_ingress_revoked(self, event: IngressPerAppRevokedEvent) -> None: if self.unit.is_leader(): logger.info("This app no longer has ingress") + self._update_oathkeeper_info_relation_data(event) + def _on_cert_changed(self, event: CertChanged) -> None: if not self._container.can_connect(): logger.info(f"Cannot connect to Oathkeeper container. Deferring the {event} event.") diff --git a/tests/unit/capture_events.py b/tests/unit/capture_events.py new file mode 100644 index 00000000..dbc4903e --- /dev/null +++ b/tests/unit/capture_events.py @@ -0,0 +1,85 @@ +"""This is a library providing a utility for unittesting events fired on a +Harness-ed Charm. + +Example usage: + +>>> from charms.harness_extensions.v0.capture_events import capture +>>> with capture(RelationEvent) as captured: +>>> harness.add_relation('foo', 'remote') +>>> assert captured.event.unit.name == 'remote' +""" + +# The unique Charmhub library identifier, never change it +LIBID = "9fcdab70e26d4eee9797c0e542ab397a" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 3 + +# Copyright 2022 Canonical Ltd. +# See LICENSE file for licensing details. + +from contextlib import contextmanager +from typing import Generic, Iterator, Optional, Type, TypeVar + +from ops.charm import CharmBase +from ops.framework import EventBase + +_T = TypeVar("_T", bound=EventBase) + + +@contextmanager +def capture_events(charm: CharmBase, *types: Type[EventBase]): + """Capture all events of type `*types` (using instance checks).""" + allowed_types = types or (EventBase,) + + captured = [] + _real_emit = charm.framework._emit + + def _wrapped_emit(evt): + if isinstance(evt, allowed_types): + captured.append(evt) + return _real_emit(evt) + + charm.framework._emit = _wrapped_emit # type: ignore # noqa # ugly + + yield captured + + charm.framework._emit = _real_emit # type: ignore # noqa # ugly + + +class Captured(Generic[_T]): + """Object to type and expose return value of capture().""" + + _event = None + + @property + def event(self) -> Optional[_T]: + """Return the captured event.""" + return self._event + + @event.setter + def event(self, val: _T): + self._event = val + + +@contextmanager +def capture(charm: CharmBase, typ_: Type[_T] = EventBase) -> Iterator[Captured[_T]]: + """Capture exactly 1 event of type `typ_`. + + Will raise if more/less events have been fired, or if the returned event + does not pass an instance check. + """ + result = Captured() + with capture_events(charm, typ_) as captured: + if not captured: + yield result + + assert len(captured) <= 1, f"too many events captured: {captured}" + assert len(captured) >= 1, f"no event of type {typ_} emitted." + event = captured[0] + assert isinstance(event, typ_), f"expected {typ_}, not {type(event)}" + result.event = event diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 8014ed09..ee5e3ae4 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -48,6 +48,7 @@ def mocked_oathkeeper_configmap(mocker: MockerFixture) -> MagicMock: @pytest.fixture(autouse=True) def mocked_access_rules_configmap(mocker: MockerFixture) -> MagicMock: mock = mocker.patch("charm.AccessRulesConfigMap", autospec=True) + mock.return_value.name = "access-rules" return mock.return_value @@ -115,3 +116,12 @@ def mocked_update_forward_auth(mocker: MockerFixture) -> MagicMock: "charms.oathkeeper.v0.forward_auth.ForwardAuthProvider.update_forward_auth_config" ) return mocked_update_forward_auth + + +@pytest.fixture() +def mocked_oathkeeper_requirer_info(mocker: MockerFixture) -> MagicMock: + mocked_oathkeeper_requirer_info = mocker.patch( + "charms.oathkeeper.v0.oathkeeper_info.OathkeeperInfoProvider.get_requirer_info" + ) + mocked_oathkeeper_requirer_info.return_value = "requirer-access-rules.json" + return mocked_oathkeeper_requirer_info diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 13bce7b7..3d6b9e12 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -8,6 +8,8 @@ import pytest import yaml +from capture_events import capture_events +from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRelationReadyEvent from jinja2 import Template from ops.model import ActiveStatus, WaitingStatus from ops.pebble import ExecError @@ -56,6 +58,13 @@ def setup_kratos_relation(harness: Harness) -> int: return relation_id +def setup_oathkeeper_info_relation(harness: Harness) -> int: + relation_id = harness.add_relation("oathkeeper-info", "requirer") + harness.add_relation_unit(relation_id, "requirer/0") + + return relation_id + + def setup_peer_relation(harness: Harness) -> Tuple[int, str]: app_name = "oathkeeper" relation_id = harness.add_relation("oathkeeper", app_name) @@ -218,7 +227,13 @@ def test_on_pebble_ready_correct_plan(harness: Harness) -> None: "startup": "enabled", "command": f"oathkeeper serve -c {CONFIG_FILE_PATH}", } - } + }, + "checks": { + "alive": { + "override": "replace", + "http": {"url": "http://localhost:4456/health/alive"}, + }, + }, } updated_plan = harness.get_container_pebble_plan(CONTAINER_NAME).to_dict() assert expected_plan == updated_plan @@ -750,3 +765,45 @@ def test_forward_auth_config_updated_on_tls_set_up( harness.charm.cert_handler.on.cert_changed.emit() mocked_update_forward_auth.assert_called() + + +def test_oathkeeper_info_ready_event_emitted_when_relation_created(harness: Harness) -> None: + with capture_events(harness.charm) as captured: + _ = setup_oathkeeper_info_relation(harness) + + assert any(isinstance(e, OathkeeperInfoRelationReadyEvent) for e in captured) + + +def test_oathkeeper_info_updated_on_relation_ready(harness: Harness) -> None: + harness.charm.info_provider.send_info_relation_data = mocked_handle = Mock(return_value=None) + _ = setup_oathkeeper_info_relation(harness) + + mocked_handle.assert_called_with( + public_endpoint="http://oathkeeper.testing.svc.cluster.local:4456", + rules_configmap_name="access-rules", + configmaps_namespace="testing", + ) + + +def test_config_file_includes_oathkeeper_info_requirer_rules( + harness: Harness, + mocked_oathkeeper_configmap: MagicMock, + mocked_oathkeeper_requirer_info: MagicMock, +) -> None: + harness.set_can_connect(CONTAINER_NAME, True) + harness.charm.on.oathkeeper_pebble_ready.emit(CONTAINER_NAME) + _ = setup_oathkeeper_info_relation(harness) + + with open("templates/oathkeeper.yaml.j2", "r") as file: + template = Template(file.read()) + + expected_config = template.render( + access_rules=[ + "requirer-access-rules.json", + ], + ) + + configmap = mocked_oathkeeper_configmap.update.call_args_list[-1][0][0] + container_config = configmap["oathkeeper.yaml"] + + assert yaml.safe_load(container_config) == yaml.safe_load(expected_config) From 6ea65cd17f4e34690c5700316b49ba534d8fb194 Mon Sep 17 00:00:00 2001 From: natalia Date: Fri, 9 Feb 2024 18:02:36 +0100 Subject: [PATCH 2/7] refactor: update oathkeeper config when relation changes --- lib/charms/oathkeeper/v0/oathkeeper_info.py | 14 +++++++------- src/charm.py | 13 +++++++++++-- tests/unit/test_charm.py | 11 +++++++++-- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/lib/charms/oathkeeper/v0/oathkeeper_info.py b/lib/charms/oathkeeper/v0/oathkeeper_info.py index e210d562..6960969c 100644 --- a/lib/charms/oathkeeper/v0/oathkeeper_info.py +++ b/lib/charms/oathkeeper/v0/oathkeeper_info.py @@ -37,7 +37,7 @@ def some_event_function(): import logging from typing import Dict, Optional -from ops.charm import CharmBase, RelationCreatedEvent +from ops.charm import CharmBase, RelationChangedEvent from ops.framework import EventBase, EventSource, Object, ObjectEvents # The unique Charmhub library identifier, never change it @@ -55,14 +55,14 @@ def some_event_function(): logger = logging.getLogger(__name__) -class OathkeeperInfoRelationReadyEvent(EventBase): - """Event to notify the charm that the relation is ready.""" +class OathkeeperInfoRelationChangedEvent(EventBase): + """Event to notify the charm that the relation data has changed.""" class OathkeeperInfoProviderEvents(ObjectEvents): """Event descriptor for events raised by `OathkeeperInfoProvider`.""" - ready = EventSource(OathkeeperInfoRelationReadyEvent) + data_changed = EventSource(OathkeeperInfoRelationChangedEvent) class OathkeeperInfoProvider(Object): @@ -77,10 +77,10 @@ def __init__(self, charm: CharmBase, relation_name: str = RELATION_NAME): self._relation_name = relation_name events = self._charm.on[relation_name] - self.framework.observe(events.relation_created, self._on_info_provider_relation_created) + self.framework.observe(events.relation_changed, self._on_info_provider_relation_changed) - def _on_info_provider_relation_created(self, event: RelationCreatedEvent) -> None: - self.on.ready.emit() + def _on_info_provider_relation_changed(self, event: RelationChangedEvent) -> None: + self.on.data_changed.emit() def send_info_relation_data( self, diff --git a/src/charm.py b/src/charm.py index 66bd8d76..3cc74bbf 100755 --- a/src/charm.py +++ b/src/charm.py @@ -29,7 +29,10 @@ ForwardAuthRelationRemovedEvent, InvalidForwardAuthConfigEvent, ) -from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoProvider +from charms.oathkeeper.v0.oathkeeper_info import ( + OathkeeperInfoProvider, + OathkeeperInfoRelationChangedEvent, +) from charms.observability_libs.v0.cert_handler import CertChanged, CertHandler from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch from charms.traefik_k8s.v2.ingress import ( @@ -163,7 +166,7 @@ def __init__(self, *args): ) self.framework.observe( - self.info_provider.on.ready, self._update_oathkeeper_info_relation_data + self.info_provider.on.data_changed, self._on_oathkeeper_info_relation_changed ) self.framework.observe(self.cert_handler.on.cert_changed, self._on_cert_changed) @@ -431,6 +434,12 @@ def _on_remove(self, event: RemoveEvent) -> None: def _on_kratos_relation_changed(self, event: RelationChangedEvent) -> None: self._handle_status_update_config(event) + def _on_oathkeeper_info_relation_changed( + self, event: OathkeeperInfoRelationChangedEvent + ) -> None: + self._update_oathkeeper_info_relation_data(event) + self._handle_status_update_config(event) + def _on_ingress_ready(self, event: IngressPerAppReadyEvent) -> None: if self.unit.is_leader(): logger.info(f"This app's ingress URL: {event.url}") diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 3d6b9e12..a1fa2a4c 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -9,7 +9,7 @@ import pytest import yaml from capture_events import capture_events -from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRelationReadyEvent +from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRelationChangedEvent from jinja2 import Template from ops.model import ActiveStatus, WaitingStatus from ops.pebble import ExecError @@ -61,6 +61,13 @@ def setup_kratos_relation(harness: Harness) -> int: def setup_oathkeeper_info_relation(harness: Harness) -> int: relation_id = harness.add_relation("oathkeeper-info", "requirer") harness.add_relation_unit(relation_id, "requirer/0") + harness.update_relation_data( + relation_id, + "requirer", + { + "access_rules_file": "requirer-access_rules.json", + }, + ) return relation_id @@ -771,7 +778,7 @@ def test_oathkeeper_info_ready_event_emitted_when_relation_created(harness: Harn with capture_events(harness.charm) as captured: _ = setup_oathkeeper_info_relation(harness) - assert any(isinstance(e, OathkeeperInfoRelationReadyEvent) for e in captured) + assert any(isinstance(e, OathkeeperInfoRelationChangedEvent) for e in captured) def test_oathkeeper_info_updated_on_relation_ready(harness: Harness) -> None: From 80df282a5e62728b228ab2f0b24ea3246732dfbb Mon Sep 17 00:00:00 2001 From: natalia Date: Mon, 12 Feb 2024 11:22:26 +0100 Subject: [PATCH 3/7] refactor: get_requirer_info --- lib/charms/oathkeeper/v0/oathkeeper_info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/charms/oathkeeper/v0/oathkeeper_info.py b/lib/charms/oathkeeper/v0/oathkeeper_info.py index 6960969c..170cffc9 100644 --- a/lib/charms/oathkeeper/v0/oathkeeper_info.py +++ b/lib/charms/oathkeeper/v0/oathkeeper_info.py @@ -117,7 +117,7 @@ def get_requirer_info(self) -> Optional[str]: logger.info("No relation data available.") return None - return data["access_rules_file"] + return data.get("access_rules_file") class OathkeeperInfoRelationError(Exception): From 156e16f387aa6884181904cdae3f934842921241 Mon Sep 17 00:00:00 2001 From: natalia Date: Mon, 12 Feb 2024 13:19:34 +0100 Subject: [PATCH 4/7] refactor: get filenames from configmap --- src/charm.py | 2 -- tests/unit/conftest.py | 10 +++++----- tests/unit/test_charm.py | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/charm.py b/src/charm.py index 3cc74bbf..82ad60d5 100755 --- a/src/charm.py +++ b/src/charm.py @@ -278,8 +278,6 @@ def _get_all_access_rules_repositories(self) -> Optional[List]: if cm_access_rules := self.access_rules_configmap.get(): for key in cm_access_rules.keys(): repositories.append(f"{self._access_rules_dir_path}/{key}") - if admin_ui_access_rules_file := self.info_provider.get_requirer_info(): - repositories.append(admin_ui_access_rules_file) return repositories def _render_conf_file(self) -> str: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index ee5e3ae4..bccc005b 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -119,9 +119,9 @@ def mocked_update_forward_auth(mocker: MockerFixture) -> MagicMock: @pytest.fixture() -def mocked_oathkeeper_requirer_info(mocker: MockerFixture) -> MagicMock: - mocked_oathkeeper_requirer_info = mocker.patch( - "charms.oathkeeper.v0.oathkeeper_info.OathkeeperInfoProvider.get_requirer_info" +def mocked_oathkeeper_access_rules_list(mocker: MockerFixture) -> MagicMock: + mocked_oathkeeper_access_rules_list = mocker.patch( + "charm.OathkeeperCharm._get_all_access_rules_repositories" ) - mocked_oathkeeper_requirer_info.return_value = "requirer-access-rules.json" - return mocked_oathkeeper_requirer_info + mocked_oathkeeper_access_rules_list.return_value = ["requirer-access-rules.json"] + return mocked_oathkeeper_access_rules_list diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index a1fa2a4c..0caebef1 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -795,7 +795,7 @@ def test_oathkeeper_info_updated_on_relation_ready(harness: Harness) -> None: def test_config_file_includes_oathkeeper_info_requirer_rules( harness: Harness, mocked_oathkeeper_configmap: MagicMock, - mocked_oathkeeper_requirer_info: MagicMock, + mocked_oathkeeper_access_rules_list: MagicMock, ) -> None: harness.set_can_connect(CONTAINER_NAME, True) harness.charm.on.oathkeeper_pebble_ready.emit(CONTAINER_NAME) From 4eafa0cac3dcec44407374dfe01f6a4dde63f70f Mon Sep 17 00:00:00 2001 From: natalia Date: Tue, 13 Feb 2024 11:50:38 +0100 Subject: [PATCH 5/7] refactor: create admin ui key in access rules cm --- lib/charms/oathkeeper/v0/oathkeeper_info.py | 47 +++------------------ src/charm.py | 14 ++++-- tests/unit/test_charm.py | 6 ++- 3 files changed, 21 insertions(+), 46 deletions(-) diff --git a/lib/charms/oathkeeper/v0/oathkeeper_info.py b/lib/charms/oathkeeper/v0/oathkeeper_info.py index 170cffc9..0ed3603e 100644 --- a/lib/charms/oathkeeper/v0/oathkeeper_info.py +++ b/lib/charms/oathkeeper/v0/oathkeeper_info.py @@ -37,7 +37,7 @@ def some_event_function(): import logging from typing import Dict, Optional -from ops.charm import CharmBase, RelationChangedEvent +from ops.charm import CharmBase, RelationCreatedEvent from ops.framework import EventBase, EventSource, Object, ObjectEvents # The unique Charmhub library identifier, never change it @@ -55,14 +55,14 @@ def some_event_function(): logger = logging.getLogger(__name__) -class OathkeeperInfoRelationChangedEvent(EventBase): - """Event to notify the charm that the relation data has changed.""" +class OathkeeperInfoRelationCreatedEvent(EventBase): + """Event to notify the charm that the relation is ready.""" class OathkeeperInfoProviderEvents(ObjectEvents): """Event descriptor for events raised by `OathkeeperInfoProvider`.""" - data_changed = EventSource(OathkeeperInfoRelationChangedEvent) + ready = EventSource(OathkeeperInfoRelationCreatedEvent) class OathkeeperInfoProvider(Object): @@ -77,10 +77,10 @@ def __init__(self, charm: CharmBase, relation_name: str = RELATION_NAME): self._relation_name = relation_name events = self._charm.on[relation_name] - self.framework.observe(events.relation_changed, self._on_info_provider_relation_changed) + self.framework.observe(events.relation_created, self._on_info_provider_relation_created) - def _on_info_provider_relation_changed(self, event: RelationChangedEvent) -> None: - self.on.data_changed.emit() + def _on_info_provider_relation_created(self, event: RelationCreatedEvent) -> None: + self.on.ready.emit() def send_info_relation_data( self, @@ -102,23 +102,6 @@ def send_info_relation_data( for relation in relations: relation.data[self._charm.app].update(info_databag) - def get_requirer_info(self) -> Optional[str]: - """Get the requirer info.""" - info = self.model.relations[self._relation_name] - if len(info) == 0: - return None - - if not (app := info[0].app): - return None - - data = info[0].data[app] - - if not data: - logger.info("No relation data available.") - return None - - return data.get("access_rules_file") - class OathkeeperInfoRelationError(Exception): """Base class for the relation exceptions.""" @@ -150,22 +133,6 @@ def __init__(self, charm: CharmBase, relation_name: str = RELATION_NAME): self._charm = charm self._relation_name = relation_name - def update_requirer_info_relation_data( - self, - access_rules_file: str, - ) -> None: - """Updates relation with access rules info.""" - if not self._charm.unit.is_leader(): - return - - relations = self.model.relations[self._relation_name] - info_databag = { - "access_rules_file": access_rules_file, - } - - for relation in relations: - relation.data[self._charm.app].update(info_databag) - def is_ready(self) -> bool: """Checks whether the relation data is ready. diff --git a/src/charm.py b/src/charm.py index 82ad60d5..a17ccd25 100755 --- a/src/charm.py +++ b/src/charm.py @@ -31,7 +31,7 @@ ) from charms.oathkeeper.v0.oathkeeper_info import ( OathkeeperInfoProvider, - OathkeeperInfoRelationChangedEvent, + OathkeeperInfoRelationCreatedEvent, ) from charms.observability_libs.v0.cert_handler import CertChanged, CertHandler from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch @@ -166,7 +166,7 @@ def __init__(self, *args): ) self.framework.observe( - self.info_provider.on.data_changed, self._on_oathkeeper_info_relation_changed + self.info_provider.on.ready, self._on_oathkeeper_info_relation_ready ) self.framework.observe(self.cert_handler.on.cert_changed, self._on_cert_changed) @@ -432,9 +432,15 @@ def _on_remove(self, event: RemoveEvent) -> None: def _on_kratos_relation_changed(self, event: RelationChangedEvent) -> None: self._handle_status_update_config(event) - def _on_oathkeeper_info_relation_changed( - self, event: OathkeeperInfoRelationChangedEvent + def _on_oathkeeper_info_relation_ready( + self, event: OathkeeperInfoRelationCreatedEvent ) -> None: + if not self._container.exists("/etc/config/access-rules/admin_ui_rules.json"): + # Create an empty configMap key for admin ui + # to make sure it will be enlisted in oathkeeper config + patch = {"data": {"admin_ui_rules.json": ""}} + self.access_rules_configmap.patch(patch=patch, cm_name="access-rules") + self._update_oathkeeper_info_relation_data(event) self._handle_status_update_config(event) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 0caebef1..b518dbf9 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -9,7 +9,7 @@ import pytest import yaml from capture_events import capture_events -from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRelationChangedEvent +from charms.oathkeeper.v0.oathkeeper_info import OathkeeperInfoRelationCreatedEvent from jinja2 import Template from ops.model import ActiveStatus, WaitingStatus from ops.pebble import ExecError @@ -775,13 +775,15 @@ def test_forward_auth_config_updated_on_tls_set_up( def test_oathkeeper_info_ready_event_emitted_when_relation_created(harness: Harness) -> None: + harness.set_can_connect(CONTAINER_NAME, True) with capture_events(harness.charm) as captured: _ = setup_oathkeeper_info_relation(harness) - assert any(isinstance(e, OathkeeperInfoRelationChangedEvent) for e in captured) + assert any(isinstance(e, OathkeeperInfoRelationCreatedEvent) for e in captured) def test_oathkeeper_info_updated_on_relation_ready(harness: Harness) -> None: + harness.set_can_connect(CONTAINER_NAME, True) harness.charm.info_provider.send_info_relation_data = mocked_handle = Mock(return_value=None) _ = setup_oathkeeper_info_relation(harness) From 215d2e19a236223ead29f8b3851c812824ed928a Mon Sep 17 00:00:00 2001 From: natalia Date: Tue, 13 Feb 2024 11:53:35 +0100 Subject: [PATCH 6/7] refactor: verify container connection before checking if file exists --- src/charm.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/charm.py b/src/charm.py index a17ccd25..6ff2b8c6 100755 --- a/src/charm.py +++ b/src/charm.py @@ -435,6 +435,11 @@ def _on_kratos_relation_changed(self, event: RelationChangedEvent) -> None: def _on_oathkeeper_info_relation_ready( self, event: OathkeeperInfoRelationCreatedEvent ) -> None: + if not self._container.can_connect(): + logger.info(f"Cannot connect to Oathkeeper container. Deferring the {event} event.") + event.defer() + return + if not self._container.exists("/etc/config/access-rules/admin_ui_rules.json"): # Create an empty configMap key for admin ui # to make sure it will be enlisted in oathkeeper config From 587e18c14b06834aa4823e4bd1ae9286dee1cb4e Mon Sep 17 00:00:00 2001 From: natalia Date: Tue, 13 Feb 2024 12:21:48 +0100 Subject: [PATCH 7/7] refactor: update info on update-status event --- src/charm.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/charm.py b/src/charm.py index 6ff2b8c6..9cfadac8 100755 --- a/src/charm.py +++ b/src/charm.py @@ -52,6 +52,7 @@ PebbleReadyEvent, RelationChangedEvent, RemoveEvent, + UpdateStatusEvent, ) from ops.main import main from ops.model import ( @@ -145,6 +146,7 @@ def __init__(self, *args): self.framework.observe(self.on.oathkeeper_pebble_ready, self._on_oathkeeper_pebble_ready) self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.config_changed, self._on_config_changed) + self.framework.observe(self.on.update_status, self._on_update_status) self.framework.observe(self.on.remove, self._on_remove) self.framework.observe( @@ -418,11 +420,13 @@ def _on_oathkeeper_pebble_ready(self, event: PebbleReadyEvent) -> None: """Event Handler for pebble ready event.""" self._patch_statefulset() self._handle_status_update_config(event) - self._update_oathkeeper_info_relation_data(event) def _on_config_changed(self, event: ConfigChangedEvent): self.forward_auth.update_forward_auth_config(self._forward_auth_config) + def _on_update_status(self, event: UpdateStatusEvent) -> None: + self._update_oathkeeper_info_relation_data(event) + def _on_remove(self, event: RemoveEvent) -> None: if not self.unit.is_leader(): return @@ -435,6 +439,8 @@ def _on_kratos_relation_changed(self, event: RelationChangedEvent) -> None: def _on_oathkeeper_info_relation_ready( self, event: OathkeeperInfoRelationCreatedEvent ) -> None: + self._update_oathkeeper_info_relation_data(event) + if not self._container.can_connect(): logger.info(f"Cannot connect to Oathkeeper container. Deferring the {event} event.") event.defer() @@ -446,7 +452,6 @@ def _on_oathkeeper_info_relation_ready( patch = {"data": {"admin_ui_rules.json": ""}} self.access_rules_configmap.patch(patch=patch, cm_name="access-rules") - self._update_oathkeeper_info_relation_data(event) self._handle_status_update_config(event) def _on_ingress_ready(self, event: IngressPerAppReadyEvent) -> None: