From 9ede3124da80bc2406efc8a6b9a9ab5740ecc66d Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Tue, 30 Aug 2022 18:58:48 +0000 Subject: [PATCH] Consider ingress revoked in utests (#94) * Consider traefik removal in utest * Fix ingress (traefik lib changed locally) * Update lib --- lib/charms/traefik_k8s/v1/ingress.py | 39 +++++++++++++++++++++++----- pyproject.toml | 7 ++++- tests/unit/test_external_url.py | 11 ++++++++ 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/lib/charms/traefik_k8s/v1/ingress.py b/lib/charms/traefik_k8s/v1/ingress.py index e1620965..fbf611ee 100644 --- a/lib/charms/traefik_k8s/v1/ingress.py +++ b/lib/charms/traefik_k8s/v1/ingress.py @@ -57,7 +57,7 @@ def _on_ingress_revoked(self, event: IngressPerAppRevokedEvent): from typing import Any, Dict, Optional, Tuple import yaml -from ops.charm import CharmBase, RelationEvent +from ops.charm import CharmBase, RelationBrokenEvent, RelationEvent from ops.framework import EventSource, Object, ObjectEvents, StoredState from ops.model import ModelError, Relation @@ -69,7 +69,7 @@ def _on_ingress_revoked(self, event: IngressPerAppRevokedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 2 +LIBPATCH = 3 DEFAULT_RELATION_NAME = "ingress" RELATION_INTERFACE = "ingress" @@ -273,6 +273,16 @@ def _handle_relation_broken(self, event): def wipe_ingress_data(self, relation: Relation): """Clear ingress data from relation.""" + assert self.unit.is_leader(), "only leaders can do this" + try: + relation.data + except ModelError as e: + log.warning( + "error {} accessing relation data for {!r}. " + "Probably a ghost of a dead relation is still " + "lingering around.".format(e, relation.name) + ) + return del relation.data[self.app]["ingress"] def _get_requirer_data(self, relation: Relation) -> RequirerData: @@ -435,12 +445,17 @@ def _handle_relation(self, event): if self.is_ready(): # Avoid spurious events, emit only when there is a NEW URL available - new_url = self.url + new_url = ( + None + if isinstance(event, RelationBrokenEvent) + else self._get_url_from_relation_data() + ) if self._stored.current_url != new_url: self._stored.current_url = new_url self.on.ready.emit(event.relation, new_url) def _handle_relation_broken(self, event): + self._stored.current_url = None self.on.revoked.emit(event.relation) def _handle_upgrade_or_leader(self, event): @@ -451,7 +466,7 @@ def _handle_upgrade_or_leader(self, event): def is_ready(self): """The Requirer is ready if the Provider has sent valid data.""" try: - return bool(self.url) + return bool(self._get_url_from_relation_data()) except DataValidationError as e: log.warning("Requirer not ready; validation error encountered: %s" % str(e)) return False @@ -475,6 +490,7 @@ def provide_ingress_requirements(self, *, host: str = None, port: int): # require one unit to publish it -- it will not differ between units, # unlike in ingress-per-unit. assert self.unit.is_leader(), "only leaders should do this." + assert self.relation, "no relation" if not host: host = socket.getfqdn() @@ -493,11 +509,10 @@ def relation(self): """The established Relation instance, or None.""" return self.relations[0] if self.relations else None - @property - def url(self) -> Optional[str]: + def _get_url_from_relation_data(self) -> Optional[str]: """The full ingress URL to reach the current unit. - May return None if the URL isn't available yet. + Returns None if the URL isn't available yet. """ relation = self.relation if not relation: @@ -519,3 +534,13 @@ def url(self) -> Optional[str]: ingress: ProviderIngressData = yaml.safe_load(raw) _validate_data({"ingress": ingress}, INGRESS_PROVIDES_APP_SCHEMA) return ingress["url"] + + @property + def url(self) -> Optional[str]: + """The full ingress URL to reach the current unit. + + Returns None if the URL isn't available yet. + """ + data = self._stored.current_url or None # type: ignore + assert isinstance(data, (str, type(None))) # for static checker + return data diff --git a/pyproject.toml b/pyproject.toml index a7212de2..7c9b1fc7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,7 +52,7 @@ allow_redefinition = true # Ignore libraries that do not have type hint nor stubs [[tool.mypy.overrides]] -module = ["ops.*", "lightkube.*", "git.*", "pytest_operator.*", "validators.*"] +module = ["ops.*", "lightkube.*", "git.*", "validators.*"] ignore_missing_imports = true [[tool.mypy.overrides]] @@ -60,6 +60,11 @@ module = ["charms.grafana_k8s.*", "charms.traefik_k8s.*", "charms.observability_ follow_imports = "silent" warn_unused_ignores = false +[[tool.mypy.overrides]] +module = ["pytest_operator.*"] +follow_imports = "skip" +ignore_missing_imports = true + [tool.pytest.ini_options] minversion = "6.0" log_cli_level = "INFO" diff --git a/tests/unit/test_external_url.py b/tests/unit/test_external_url.py index 29e81c20..7a27e84d 100644 --- a/tests/unit/test_external_url.py +++ b/tests/unit/test_external_url.py @@ -127,6 +127,17 @@ def test_config_option_overrides_traefik(self): self.assertEqual(self.get_url_cli_arg(), external_url_ingress) self.assertTrue(self.is_service_running()) + # AND WHEN the traefik relation is removed + self.harness.remove_relation_unit(rel_id, "traefik-app/0") + self.harness.remove_relation(rel_id) + + # NOTE intentionally not emptying out relation data manually + # app_data = {"ingress": ""} + # self.harness.update_relation_data(rel_id, "traefik-app", app_data) + + # THEN the fqdn is used as external url + self.assertEqual(self.get_url_cli_arg(), self.fqdn_url) + @patch.object(AlertmanagerCharm, "_check_config", lambda *a, **kw: ("ok", "")) @patch("socket.getfqdn", new=lambda *args: "fqdn") @k8s_resource_multipatch