From 55882029fc9dd99f74ccae77af5d923f4ea665d8 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Wed, 7 Aug 2024 15:41:08 +0300 Subject: [PATCH 01/13] draft waypoint --- charmcraft.yaml | 18 +++++ requirements.txt | 6 ++ src/charm.py | 173 +++++++++++++++++++++++++++++++++++++++++++---- src/models.py | 48 +++++++++++++ 4 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 src/models.py diff --git a/charmcraft.yaml b/charmcraft.yaml index 84bdad3..abe4e5a 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -43,3 +43,21 @@ parts: # Pydantic is not actually used by the lib but charmcraft install pydeps from libs even if # they are not used. - pydantic>2.0 + plugin: charm + build-packages: [git] + +config: + options: + ready-timeout: + type: int + default: 100 + description: > + The maximum time (in seconds) to wait for the waypoint deployment to be + ready. This applies specifically to the deployment created for the Istio + waypoint controller. If the deployment does not become ready within this time, + charm will go into blocked state. + model-on-mesh: + type: bool + default: False + description: > + Add the whole model on the mesh and connect it to the waypoint \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 618ba75..e56ea1f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1,7 @@ ops ~= 2.5 +pydantic>=2 + +# A collection of helpers and shared code for using Lightkube +# Code: https://github.com/canonical/lightkube-extensions +# Deps: charm +lightkube-extensions @ git+https://github.com/canonical/lightkube-extensions.git@main \ No newline at end of file diff --git a/src/charm.py b/src/charm.py index 77a7132..95be792 100755 --- a/src/charm.py +++ b/src/charm.py @@ -6,39 +6,182 @@ """Istio Beacon Charm.""" import logging +import time import ops +from lightkube_extensions.batch import KubernetesResourceManager, create_charm_default_labels +from lightkube.generic_resource import create_namespaced_resource +from lightkube.core.client import Client +from models import Metadata, Listener, IstioWaypointSpec, IstioWaypointResource, AllowedRoutes +from lightkube.models.meta_v1 import ObjectMeta +from ops.model import ActiveStatus, MaintenanceStatus, BlockedStatus +from ops import Relation +from lightkube.resources.apps_v1 import Deployment +from lightkube.core.exceptions import ApiError +from lightkube.resources.core_v1 import Namespace logger = logging.getLogger(__name__) +RESOURCE_TYPES = { + "Gateway": create_namespaced_resource( + "gateway.networking.k8s.io", "v1", "Gateway", "gateways" + ), +} + +WAYPOINT_RESOURCE_TYPES = {RESOURCE_TYPES["Gateway"]} +WAYPOINT_LABEL = "istio-waypoint" + class IstioBeaconCharm(ops.CharmBase): """Charm the service.""" def __init__(self, *args): super().__init__(*args) - self.framework.observe(self.on.start, self.on_start) + + self._lightkube_field_manager: str = self.app.name + self._lightkube_client = None + + self.framework.observe(self.on.config_changed, self._on_config_changed) + self.framework.observe(self.on.remove, self._on_remove) self.framework.observe(self.on["service-mesh"].relation_changed, self.on_mesh_changed) + self.framework.observe(self.on["service-mesh"].relation_broken, self.on_mesh_broken) - def on_start(self, _event): - """Event handler for start.""" - self.unit.status = ops.ActiveStatus() + def _on_config_changed(self, _): + """Event handler for config changed.""" + self._sync_all_resources() - def on_mesh_changed(self, _event): + def on_mesh_changed(self, _): """Event handler for service-mesh relation_changed.""" - self.update_mesh() + self._sync_all_resources() + + def on_mesh_broken(self, _): + """Event handler for service-mesh relation_broken.""" + self._sync_all_resources() + + def _on_remove(self, _): + """Event handler for remove.""" + self._remove_labels() + krm = self._get_waypoint_resource_manager() + krm.delete() + + @property + def lightkube_client(self): + """Returns a lightkube client configured for this charm.""" + if self._lightkube_client is None: + self._lightkube_client = Client( + namespace=self.model.name, field_manager=self._lightkube_field_manager + ) + return self._lightkube_client + + def _get_waypoint_resource_manager(self): + return KubernetesResourceManager( + labels=create_charm_default_labels( + self.app.name, self.model.name, scope=WAYPOINT_LABEL + ), + resource_types=WAYPOINT_RESOURCE_TYPES, # pyright: ignore + lightkube_client=self.lightkube_client, + logger=logger, + ) + + def _is_deployment_ready(self) -> bool: + """Check if the deployment is ready after 10 attempts.""" + timeout = int(self.config["ready-timeout"]) + check_interval = 10 + attempts = timeout // check_interval + + for _ in range(attempts): + try: + deployment = self.lightkube_client.get( + Deployment, + name=f"{self.app.name}-{self.model.name}-waypoint", + namespace=self.model.name, + ) + if ( + deployment.status + and deployment.status.readyReplicas == deployment.status.replicas + ): + return True + except ApiError as e: + logger.error(f"Error checking waypoint deployment status: {e}") + + time.sleep(check_interval) + + return False + + def _is_ready(self) -> bool: + + if not self._is_deployment_ready(): + return False + return True + + def _sync_all_resources(self): + self.unit.status = MaintenanceStatus("Validating waypoint readiness") + self._sync_waypoint_resources() + if self._is_ready(): + self.unit.status = BlockedStatus( + f"Waypoint's k8s deployment not ready, is istio properly installed?" + ) + return + self.unit.status = ActiveStatus() + + def _construct_waypoint(self): + gateway = IstioWaypointResource( + metadata=Metadata( + name=f"{self.app.name}-{self.model.name}-waypoint", + namespace=self.model.name, + labels={"istio.io/waypoint-for": "service"}, + ), + spec=IstioWaypointSpec( + gatewayClassName=f"istio-waypoint", + listeners=[ + Listener( + name="mesh", + port=15008, + protocol="HBONE", + allowedRoutes=AllowedRoutes(namespaces={"from": "All"}), + ) + ], + ), + ) + gateway_resource = RESOURCE_TYPES["Gateway"] + return gateway_resource( + metadata=ObjectMeta.from_dict(gateway.metadata.dict()), + spec=gateway.spec.dict(), + ) + + def _sync_waypoint_resources(self): + resources_list = [] + if not self.unit.is_leader(): + raise RuntimeError("Waypoint can only be provided on the leader unit.") + krm = self._get_waypoint_resource_manager() + resource_to_append = self._construct_waypoint() + resources_list.append(resource_to_append) + krm.reconcile(resources_list) + namespace = self.lightkube_client.get(Namespace, self.model.name) + + if self.config["model-on-mesh"]: + labels = { + "istio.io/use-waypoint": f"{self.app.name}-{self.model.name}-waypoint", + "istio.io/dataplane-mode": "ambient", + } - def update_mesh(self): - """Update the service mesh. + namespace.metadata.labels.update(labels) + self.lightkube_client.patch(Namespace, self.model.name, namespace) + else: + self._remove_labels() - Reads all relations and any config to generate a kubernetes manifest. Then applies the - manifest. - """ - for relation in self.model.relations["service-mesh"]: - logger.error(relation.data[relation.app]) - # Update the mesh - self.unit.status = ops.ActiveStatus() + def _remove_labels(self): + namespace = self.lightkube_client.get(Namespace, self.model.name) + """Remove specific labels from a namespace if they exist.""" + labels_to_remove = {"istio.io/use-waypoint": None, "istio.io/dataplane-mode": None} + # Check if the labels exist and remove them by setting to None + if namespace.metadata.labels and ( + "istio.io/use-waypoint" in namespace.metadata.labels + or "istio.io/dataplane-mode" in namespace.metadata.labels + ): + namespace.metadata.labels.update(labels_to_remove) + self.lightkube_client.patch(Namespace, self.model.name, namespace) if __name__ == "__main__": ops.main(IstioBeaconCharm) # type: ignore diff --git a/src/models.py b/src/models.py new file mode 100644 index 0000000..40cb97d --- /dev/null +++ b/src/models.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 + +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +"""This module defines Pydantic schemas for various resources used in the Kubernetes Gateway API.""" + +from typing import Dict, List, Optional + +from pydantic import BaseModel + + +# Global metadata schema +class Metadata(BaseModel): + """Global metadata schema for Kubernetes resources.""" + + name: str + namespace: str + labels: Optional[Dict[str, str]] = None + annotations: Optional[Dict[str, str]] = None + + +class AllowedRoutes(BaseModel): + """AllowedRoutes defines namespaces from which traffic is allowed.""" + + namespaces: Dict[str, str] + + +class Listener(BaseModel): + """Listener defines a port and protocol configuration.""" + + name: str + port: int + protocol: str + allowedRoutes: AllowedRoutes # noqa: N815 + + +class IstioWaypointSpec(BaseModel): + """IstioWaypointSpec defines the specification of a waypoint.""" + + gatewayClassName: str # noqa: N815 + listeners: List[Listener] + + +class IstioWaypointResource(BaseModel): + """IstioWaypointResource defines the structure of an waypoint Kubernetes resource.""" + + metadata: Metadata + spec: IstioWaypointSpec From e76f56daab837dbe055a9312d77a74a3fb3070f8 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Wed, 7 Aug 2024 22:12:31 +0300 Subject: [PATCH 02/13] adding integration tests --- charmcraft.yaml | 4 +- src/charm.py | 72 ++++++++++++++++++++------------- tests/integration/conftest.py | 66 ++++++++++++++++++++++++++++++ tests/integration/helpers.py | 23 +++++++++++ tests/integration/test_charm.py | 46 ++++++++++++++++----- tests/scenario/test_charm.py | 7 ++-- tox.ini | 2 + 7 files changed, 177 insertions(+), 43 deletions(-) create mode 100644 tests/integration/conftest.py create mode 100644 tests/integration/helpers.py diff --git a/charmcraft.yaml b/charmcraft.yaml index abe4e5a..daee0f5 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -57,7 +57,7 @@ config: waypoint controller. If the deployment does not become ready within this time, charm will go into blocked state. model-on-mesh: - type: bool - default: False + type: boolean + default: false description: > Add the whole model on the mesh and connect it to the waypoint \ No newline at end of file diff --git a/src/charm.py b/src/charm.py index 95be792..13ce564 100755 --- a/src/charm.py +++ b/src/charm.py @@ -9,16 +9,15 @@ import time import ops -from lightkube_extensions.batch import KubernetesResourceManager, create_charm_default_labels -from lightkube.generic_resource import create_namespaced_resource from lightkube.core.client import Client -from models import Metadata, Listener, IstioWaypointSpec, IstioWaypointResource, AllowedRoutes +from lightkube.core.exceptions import ApiError +from lightkube.generic_resource import create_namespaced_resource from lightkube.models.meta_v1 import ObjectMeta -from ops.model import ActiveStatus, MaintenanceStatus, BlockedStatus -from ops import Relation from lightkube.resources.apps_v1 import Deployment -from lightkube.core.exceptions import ApiError from lightkube.resources.core_v1 import Namespace +from lightkube_extensions.batch import KubernetesResourceManager, create_charm_default_labels +from models import AllowedRoutes, IstioWaypointResource, IstioWaypointSpec, Listener, Metadata +from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus logger = logging.getLogger(__name__) @@ -60,7 +59,7 @@ def on_mesh_broken(self, _): def _on_remove(self, _): """Event handler for remove.""" - self._remove_labels() + self._update_labels(False) krm = self._get_waypoint_resource_manager() krm.delete() @@ -117,9 +116,9 @@ def _is_ready(self) -> bool: def _sync_all_resources(self): self.unit.status = MaintenanceStatus("Validating waypoint readiness") self._sync_waypoint_resources() - if self._is_ready(): + if not self._is_ready(): self.unit.status = BlockedStatus( - f"Waypoint's k8s deployment not ready, is istio properly installed?" + "Waypoint's k8s deployment not ready, is istio properly installed?" ) return self.unit.status = ActiveStatus() @@ -132,7 +131,7 @@ def _construct_waypoint(self): labels={"istio.io/waypoint-for": "service"}, ), spec=IstioWaypointSpec( - gatewayClassName=f"istio-waypoint", + gatewayClassName="istio-waypoint", listeners=[ Listener( name="mesh", @@ -157,31 +156,48 @@ def _sync_waypoint_resources(self): resource_to_append = self._construct_waypoint() resources_list.append(resource_to_append) krm.reconcile(resources_list) - namespace = self.lightkube_client.get(Namespace, self.model.name) if self.config["model-on-mesh"]: - labels = { + self._update_labels() + else: + self._update_labels(False) + + def _update_labels(self, add_labels=True): + """Add or remove specific labels from a namespace based on the add_labels flag. + + Args: + add_labels (bool): If True, add labels; if False, remove labels. + """ + try: + namespace = self.lightkube_client.get(Namespace, self.model.name) + except ApiError as e: + logger.error(f"Error checking namespace labels {e}") + return + + if add_labels: + labels_to_add = { "istio.io/use-waypoint": f"{self.app.name}-{self.model.name}-waypoint", "istio.io/dataplane-mode": "ambient", } - - namespace.metadata.labels.update(labels) - self.lightkube_client.patch(Namespace, self.model.name, namespace) + if namespace.metadata and namespace.metadata.labels: + namespace.metadata.labels.update(labels_to_add) else: - self._remove_labels() - - def _remove_labels(self): - namespace = self.lightkube_client.get(Namespace, self.model.name) - """Remove specific labels from a namespace if they exist.""" - labels_to_remove = {"istio.io/use-waypoint": None, "istio.io/dataplane-mode": None} - - # Check if the labels exist and remove them by setting to None - if namespace.metadata.labels and ( - "istio.io/use-waypoint" in namespace.metadata.labels - or "istio.io/dataplane-mode" in namespace.metadata.labels - ): - namespace.metadata.labels.update(labels_to_remove) + labels_to_remove = {"istio.io/use-waypoint": None, "istio.io/dataplane-mode": None} + + if ( + namespace.metadata + and namespace.metadata.labels + and ( + "istio.io/use-waypoint" in namespace.metadata.labels + or "istio.io/dataplane-mode" in namespace.metadata.labels + ) + ): + namespace.metadata.labels.update(labels_to_remove) + try: self.lightkube_client.patch(Namespace, self.model.name, namespace) + except ApiError as e: + logger.error(f"Error patching istio labels {e}") + if __name__ == "__main__": ops.main(IstioBeaconCharm) # type: ignore diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py new file mode 100644 index 0000000..b5079ae --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,66 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + + +import functools +import logging +from collections import defaultdict +from datetime import datetime + +import pytest +from pytest_operator.plugin import OpsTest + +logger = logging.getLogger(__name__) + + +class Store(defaultdict): + def __init__(self): + super(Store, self).__init__(Store) + + def __getattr__(self, key): + """Override __getattr__ so dot syntax works on keys.""" + try: + return self[key] + except KeyError: + raise AttributeError(key) + + def __setattr__(self, key, value): + """Override __setattr__ so dot syntax works on keys.""" + self[key] = value + + +store = Store() + + +def timed_memoizer(func): + @functools.wraps(func) + async def wrapper(*args, **kwargs): + fname = func.__qualname__ + logger.info("Started: %s" % fname) + start_time = datetime.now() + if fname in store.keys(): + ret = store[fname] + else: + logger.info("Return for {} not cached".format(fname)) + ret = await func(*args, **kwargs) + store[fname] = ret + logger.info("Finished: {} in: {} seconds".format(fname, datetime.now() - start_time)) + return ret + + return wrapper + + +@pytest.fixture(scope="module") +@timed_memoizer +async def istio_beacon_charm(ops_test: OpsTest): + count = 0 + while True: + try: + charm = await ops_test.build_charm(".", verbosity="debug") + return charm + except RuntimeError: + logger.warning("Failed to build istio-ingress. Trying again!") + count += 1 + + if count == 3: + raise diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py new file mode 100644 index 0000000..9516420 --- /dev/null +++ b/tests/integration/helpers.py @@ -0,0 +1,23 @@ +from lightkube.core.client import Client +from lightkube.resources.core_v1 import Namespace +from pytest_operator.plugin import OpsTest + + +async def validate_labels(ops_test: OpsTest, app_name: str, should_be_present: bool): + """Validate the presence or absence of specific labels in the namespace.""" + client = Client() + + namespace_name = ops_test.model_name + namespace = client.get(Namespace, namespace_name) + + expected_labels = { + "istio.io/use-waypoint": f"{app_name}-{namespace_name}-waypoint", + "istio.io/dataplane-mode": "ambient", + } + + for label, expected_value in expected_labels.items(): + actual_value = namespace.metadata.labels.get(label) + if should_be_present: + assert actual_value == expected_value, f"Label {label} is missing or incorrect." + else: + assert actual_value is None, f"Label {label} should have been removed." diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 0171a6d..ac85426 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -2,10 +2,13 @@ # See LICENSE file for licensing details. import logging +from dataclasses import asdict, dataclass from pathlib import Path +from typing import Optional import pytest import yaml +from helpers import validate_labels from pytest_operator.plugin import OpsTest logger = logging.getLogger(__name__) @@ -14,17 +17,42 @@ APP_NAME = METADATA["name"] +@dataclass +class CharmDeploymentConfiguration: + entity_url: str # aka charm name or local path to charm + application_name: str + channel: str + trust: bool + config: Optional[dict] = None + + +ISTIO_K8S = CharmDeploymentConfiguration( + entity_url="istio-k8s", application_name="istio-k8s", channel="latest/edge", trust=True +) + + @pytest.mark.abort_on_fail -async def test_build_and_deploy(ops_test: OpsTest): - """Build the charm-under-test and deploy it together with related charms. +async def test_deploy_dependencies(ops_test: OpsTest): + await ops_test.model.deploy(**asdict(ISTIO_K8S)) + await ops_test.model.wait_for_idle([ISTIO_K8S.application_name], status="active", timeout=1000) + - Assert on the unit status before any relations/configurations take place. - """ - # Build and deploy charm from local source folder - charm = await ops_test.build_charm(".") +@pytest.mark.abort_on_fail +async def test_deployment(ops_test: OpsTest, istio_beacon_charm): + await ops_test.model.deploy(istio_beacon_charm, application_name=APP_NAME, trust=True), + await ops_test.model.wait_for_idle([APP_NAME], status="active", timeout=1000) + + +@pytest.mark.abort_on_fail +async def test_mesh_config(ops_test: OpsTest): + await ops_test.model.applications[APP_NAME].set_config({"model-on-mesh": "true"}) + await ops_test.model.wait_for_idle( + [APP_NAME], status="active", timeout=1000, raise_on_error=False + ) + await validate_labels(ops_test, APP_NAME, should_be_present=True) - # Deploy the charm and wait for active/idle status - await ops_test.model.deploy(charm, application_name=APP_NAME), + await ops_test.model.applications[APP_NAME].set_config({"model-on-mesh": "false"}) await ops_test.model.wait_for_idle( - apps=[APP_NAME], status="active", raise_on_blocked=True, timeout=1000 + [APP_NAME], status="active", timeout=1000, raise_on_error=False ) + await validate_labels(ops_test, APP_NAME, should_be_present=False) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 2696d58..0a0119a 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -4,9 +4,8 @@ import scenario from charm import IstioBeaconCharm - +# TODO: introduce mocks needed for testing model-on-mesh def test_relation_changed_status(): ctx = scenario.Context(IstioBeaconCharm) - rel = scenario.Relation(endpoint="service-mesh", interface="service_mesh") - out = ctx.run(rel.changed_event, scenario.State()) - assert out.unit_status.name == "active" + out = ctx.run('start', scenario.State()) + assert out.unit_status.name == "unknown" diff --git a/tox.ini b/tox.ini index 1595490..2456d64 100644 --- a/tox.ini +++ b/tox.ini @@ -27,8 +27,10 @@ pass_env = description = Apply coding style standards to code deps = black + ruff commands = black {[vars]all_path} + ruff check --fix {[vars]all_path} [testenv:lint] description = Check code against coding style standards From bdb708801b19d04577f2d8d6c737ea82f52c4211 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Wed, 7 Aug 2024 22:20:53 +0300 Subject: [PATCH 03/13] linting --- src/charm.py | 6 +++--- tests/scenario/test_charm.py | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/charm.py b/src/charm.py index 13ce564..0af551c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -171,8 +171,8 @@ def _update_labels(self, add_labels=True): try: namespace = self.lightkube_client.get(Namespace, self.model.name) except ApiError as e: - logger.error(f"Error checking namespace labels {e}") - return + logger.error(f"Error checking namespace labels {e}") + return if add_labels: labels_to_add = { @@ -196,7 +196,7 @@ def _update_labels(self, add_labels=True): try: self.lightkube_client.patch(Namespace, self.model.name, namespace) except ApiError as e: - logger.error(f"Error patching istio labels {e}") + logger.error(f"Error patching istio labels {e}") if __name__ == "__main__": diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 0a0119a..a7cf7ea 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -4,8 +4,9 @@ import scenario from charm import IstioBeaconCharm + # TODO: introduce mocks needed for testing model-on-mesh def test_relation_changed_status(): ctx = scenario.Context(IstioBeaconCharm) - out = ctx.run('start', scenario.State()) + out = ctx.run("start", scenario.State()) assert out.unit_status.name == "unknown" From a598cc21357d4dd1ce0962bf0070349175c1366c Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 19 Aug 2024 08:11:26 +0000 Subject: [PATCH 04/13] address comments and fix Itests (iter I) --- charmcraft.yaml | 3 ++- src/charm.py | 12 +++++++----- tests/integration/test_charm.py | 23 +++++++++++++++-------- tests/scenario/test_charm.py | 1 + 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index daee0f5..a897e4b 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -60,4 +60,5 @@ config: type: boolean default: false description: > - Add the whole model on the mesh and connect it to the waypoint \ No newline at end of file + Add this charm's model to the service mesh. + All charms in this model will automatically be added to the mesh. \ No newline at end of file diff --git a/src/charm.py b/src/charm.py index 0af551c..268f5f9 100755 --- a/src/charm.py +++ b/src/charm.py @@ -16,9 +16,10 @@ from lightkube.resources.apps_v1 import Deployment from lightkube.resources.core_v1 import Namespace from lightkube_extensions.batch import KubernetesResourceManager, create_charm_default_labels -from models import AllowedRoutes, IstioWaypointResource, IstioWaypointSpec, Listener, Metadata from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus +from models import AllowedRoutes, IstioWaypointResource, IstioWaypointSpec, Listener, Metadata + logger = logging.getLogger(__name__) RESOURCE_TYPES = { @@ -114,6 +115,9 @@ def _is_ready(self) -> bool: return True def _sync_all_resources(self): + if not self.unit.is_leader(): + self.unit.status = BlockedStatus("Waypoint can only be provided on the leader unit.") + return self.unit.status = MaintenanceStatus("Validating waypoint readiness") self._sync_waypoint_resources() if not self._is_ready(): @@ -144,14 +148,12 @@ def _construct_waypoint(self): ) gateway_resource = RESOURCE_TYPES["Gateway"] return gateway_resource( - metadata=ObjectMeta.from_dict(gateway.metadata.dict()), - spec=gateway.spec.dict(), + metadata=ObjectMeta.from_dict(gateway.metadata.model_dump()), + spec=gateway.spec.model_dump(), ) def _sync_waypoint_resources(self): resources_list = [] - if not self.unit.is_leader(): - raise RuntimeError("Waypoint can only be provided on the leader unit.") krm = self._get_waypoint_resource_manager() resource_to_append = self._construct_waypoint() resources_list.append(resource_to_append) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index ac85426..2f5e613 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -39,20 +39,27 @@ async def test_deploy_dependencies(ops_test: OpsTest): @pytest.mark.abort_on_fail async def test_deployment(ops_test: OpsTest, istio_beacon_charm): - await ops_test.model.deploy(istio_beacon_charm, application_name=APP_NAME, trust=True), - await ops_test.model.wait_for_idle([APP_NAME], status="active", timeout=1000) + + # Not the model name just an alias + await ops_test.track_model("beacon") + istio_beacon = ops_test.models.get("beacon") + await istio_beacon.model.deploy(istio_beacon_charm, application_name=APP_NAME, trust=True), + await istio_beacon.model.wait_for_idle([APP_NAME], status="active", timeout=1000) @pytest.mark.abort_on_fail async def test_mesh_config(ops_test: OpsTest): - await ops_test.model.applications[APP_NAME].set_config({"model-on-mesh": "true"}) - await ops_test.model.wait_for_idle( + + # Not the model name just an alias + istio_beacon = ops_test.models.get("beacon") + await istio_beacon.model.applications[APP_NAME].set_config({"model-on-mesh": "true"}) + await istio_beacon.model.wait_for_idle( [APP_NAME], status="active", timeout=1000, raise_on_error=False ) - await validate_labels(ops_test, APP_NAME, should_be_present=True) + await validate_labels(istio_beacon, APP_NAME, should_be_present=True) - await ops_test.model.applications[APP_NAME].set_config({"model-on-mesh": "false"}) - await ops_test.model.wait_for_idle( + await istio_beacon.model.applications[APP_NAME].set_config({"model-on-mesh": "false"}) + await istio_beacon.model.wait_for_idle( [APP_NAME], status="active", timeout=1000, raise_on_error=False ) - await validate_labels(ops_test, APP_NAME, should_be_present=False) + await validate_labels(istio_beacon, APP_NAME, should_be_present=False) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index a7cf7ea..ad788b8 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. import scenario + from charm import IstioBeaconCharm From d01f0c205c3efdd86688f9eadedc92340bb7ac5c Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 19 Aug 2024 08:49:20 +0000 Subject: [PATCH 05/13] itests (iter II) --- .github/workflows/pull-request.yaml | 2 +- tests/integration/test_charm.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 1080b5f..8fbf1cc 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -8,5 +8,5 @@ on: jobs: pull-request: name: PR - uses: canonical/observability/.github/workflows/charm-pull-request.yaml@main + uses: canonical/observability/.github/workflows/charm-pull-request.yaml@istio-test secrets: inherit diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 2f5e613..24f9c31 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -52,6 +52,7 @@ async def test_mesh_config(ops_test: OpsTest): # Not the model name just an alias istio_beacon = ops_test.models.get("beacon") + # await istio_beacon.model.set_config({"update-status-hook-interval": "30s"}) await istio_beacon.model.applications[APP_NAME].set_config({"model-on-mesh": "true"}) await istio_beacon.model.wait_for_idle( [APP_NAME], status="active", timeout=1000, raise_on_error=False From e0bbe4557f3b30c56109753e370ecb0e0523add9 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 19 Aug 2024 10:42:07 +0000 Subject: [PATCH 06/13] address comments and fix Itests (iter III) --- src/charm.py | 84 +++++++++++++++++++++++---------- tests/integration/helpers.py | 1 + tests/integration/test_charm.py | 1 - 3 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/charm.py b/src/charm.py index 268f5f9..806eca7 100755 --- a/src/charm.py +++ b/src/charm.py @@ -60,7 +60,7 @@ def on_mesh_broken(self, _): def _on_remove(self, _): """Event handler for remove.""" - self._update_labels(False) + self._remove_labels() krm = self._get_waypoint_resource_manager() krm.delete() @@ -160,45 +160,77 @@ def _sync_waypoint_resources(self): krm.reconcile(resources_list) if self.config["model-on-mesh"]: - self._update_labels() + self._add_labels() else: - self._update_labels(False) + self._remove_labels() - def _update_labels(self, add_labels=True): - """Add or remove specific labels from a namespace based on the add_labels flag. + def _get_namespace(self): + """Retrieve the namespace object.""" + try: + return self.lightkube_client.get(Namespace, self.model.name) + except ApiError as e: + logger.error(f"Error fetching namespace: {e}") + return None - Args: - add_labels (bool): If True, add labels; if False, remove labels. - """ + def _patch_namespace(self, namespace): + """Patch the namespace with updated labels.""" try: - namespace = self.lightkube_client.get(Namespace, self.model.name) + self.lightkube_client.patch(Namespace, self.model.name, namespace) except ApiError as e: - logger.error(f"Error checking namespace labels {e}") + logger.error(f"Error patching namespace: {e}") + + def _add_labels(self): + """Add specific labels to the namespace.""" + namespace = self._get_namespace() + if not namespace: return - if add_labels: + if namespace.metadata and namespace.metadata.labels: + existing_labels = namespace.metadata.labels + if ( + existing_labels.get("istio.io/use-waypoint") + or existing_labels.get("istio.io/dataplane-mode") + ) and existing_labels.get( + "istio.io/use-waypoint/managed-by" + ) != f"{self.app.name}-{self.model.name}": + logger.error( + f"Cannot add labels: Namespace '{self.model.name}' is already configured with Istio labels managed by another entity." + ) + return + labels_to_add = { "istio.io/use-waypoint": f"{self.app.name}-{self.model.name}-waypoint", "istio.io/dataplane-mode": "ambient", + "istio.io/use-waypoint/managed-by": f"{self.app.name}-{self.model.name}", } - if namespace.metadata and namespace.metadata.labels: - namespace.metadata.labels.update(labels_to_add) - else: - labels_to_remove = {"istio.io/use-waypoint": None, "istio.io/dataplane-mode": None} + namespace.metadata.labels.update(labels_to_add) + self._patch_namespace(namespace) + + def _remove_labels(self): + """Remove specific labels from the namespace.""" + namespace = self._get_namespace() + if not namespace: + return + + if namespace.metadata and namespace.metadata.labels: if ( - namespace.metadata - and namespace.metadata.labels - and ( - "istio.io/use-waypoint" in namespace.metadata.labels - or "istio.io/dataplane-mode" in namespace.metadata.labels - ) + namespace.metadata.labels.get("istio.io/use-waypoint/managed-by") + != f"{self.app.name}-{self.model.name}" ): - namespace.metadata.labels.update(labels_to_remove) - try: - self.lightkube_client.patch(Namespace, self.model.name, namespace) - except ApiError as e: - logger.error(f"Error patching istio labels {e}") + logger.warning( + f"Cannot remove labels: Namespace '{self.model.name}' has Istio labels managed by another entity." + ) + return + + labels_to_remove = { + "istio.io/use-waypoint": None, + "istio.io/dataplane-mode": None, + "istio.io/use-waypoint/managed-by": None, + } + + namespace.metadata.labels.update(labels_to_remove) + self._patch_namespace(namespace) if __name__ == "__main__": diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 9516420..c7a146d 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -13,6 +13,7 @@ async def validate_labels(ops_test: OpsTest, app_name: str, should_be_present: b expected_labels = { "istio.io/use-waypoint": f"{app_name}-{namespace_name}-waypoint", "istio.io/dataplane-mode": "ambient", + "istio.io/use-waypoint/managed-by": f"{app_name}-{namespace_name}", } for label, expected_value in expected_labels.items(): diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 24f9c31..2f5e613 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -52,7 +52,6 @@ async def test_mesh_config(ops_test: OpsTest): # Not the model name just an alias istio_beacon = ops_test.models.get("beacon") - # await istio_beacon.model.set_config({"update-status-hook-interval": "30s"}) await istio_beacon.model.applications[APP_NAME].set_config({"model-on-mesh": "true"}) await istio_beacon.model.wait_for_idle( [APP_NAME], status="active", timeout=1000, raise_on_error=False From 96a5fe3067ea7d4ade4188a2153a3afef27c5b23 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 19 Aug 2024 11:30:10 +0000 Subject: [PATCH 07/13] amend label name --- src/charm.py | 8 ++++---- tests/integration/helpers.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/charm.py b/src/charm.py index 806eca7..353c202 100755 --- a/src/charm.py +++ b/src/charm.py @@ -191,7 +191,7 @@ def _add_labels(self): existing_labels.get("istio.io/use-waypoint") or existing_labels.get("istio.io/dataplane-mode") ) and existing_labels.get( - "istio.io/use-waypoint/managed-by" + "istio.io.waypoint-managed-by" ) != f"{self.app.name}-{self.model.name}": logger.error( f"Cannot add labels: Namespace '{self.model.name}' is already configured with Istio labels managed by another entity." @@ -201,7 +201,7 @@ def _add_labels(self): labels_to_add = { "istio.io/use-waypoint": f"{self.app.name}-{self.model.name}-waypoint", "istio.io/dataplane-mode": "ambient", - "istio.io/use-waypoint/managed-by": f"{self.app.name}-{self.model.name}", + "istio.io.waypoint-managed-by": f"{self.app.name}-{self.model.name}", } namespace.metadata.labels.update(labels_to_add) @@ -215,7 +215,7 @@ def _remove_labels(self): if namespace.metadata and namespace.metadata.labels: if ( - namespace.metadata.labels.get("istio.io/use-waypoint/managed-by") + namespace.metadata.labels.get("istio.io.waypoint-managed-by") != f"{self.app.name}-{self.model.name}" ): logger.warning( @@ -226,7 +226,7 @@ def _remove_labels(self): labels_to_remove = { "istio.io/use-waypoint": None, "istio.io/dataplane-mode": None, - "istio.io/use-waypoint/managed-by": None, + "istio.io.waypoint-managed-by": None, } namespace.metadata.labels.update(labels_to_remove) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index c7a146d..d883dc8 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -13,7 +13,7 @@ async def validate_labels(ops_test: OpsTest, app_name: str, should_be_present: b expected_labels = { "istio.io/use-waypoint": f"{app_name}-{namespace_name}-waypoint", "istio.io/dataplane-mode": "ambient", - "istio.io/use-waypoint/managed-by": f"{app_name}-{namespace_name}", + "istio.io.waypoint-managed-by": f"{app_name}-{namespace_name}", } for label, expected_value in expected_labels.items(): From 50b3c6672904d7830789930f677308c2660430cb Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Thu, 22 Aug 2024 11:18:19 +0000 Subject: [PATCH 08/13] address comments and add tests (iter IV) --- .github/workflows/pull-request.yaml | 1 + src/charm.py | 69 ++++---- tests/integration/conftest.py | 3 +- tests/integration/helpers.py | 7 +- tests/integration/test_charm.py | 2 + tests/unit/test_charm.py | 243 ++++++++++++++++++++++++++++ tox.ini | 10 +- 7 files changed, 301 insertions(+), 34 deletions(-) create mode 100644 tests/unit/test_charm.py diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 8fbf1cc..776be8d 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -5,6 +5,7 @@ on: branches: - main +# TODO: Remove when #8 closes jobs: pull-request: name: PR diff --git a/src/charm.py b/src/charm.py index 353c202..3dfd7b1 100755 --- a/src/charm.py +++ b/src/charm.py @@ -40,7 +40,7 @@ def __init__(self, *args): self._lightkube_field_manager: str = self.app.name self._lightkube_client = None - + self._managed_labels = f"{self.app.name}-{self.model.name}" self.framework.observe(self.on.config_changed, self._on_config_changed) self.framework.observe(self.on.remove, self._on_remove) self.framework.observe(self.on["service-mesh"].relation_changed, self.on_mesh_changed) @@ -83,7 +83,7 @@ def _get_waypoint_resource_manager(self): logger=logger, ) - def _is_deployment_ready(self) -> bool: + def _is_waypoint_deployment_ready(self) -> bool: """Check if the deployment is ready after 10 attempts.""" timeout = int(self.config["ready-timeout"]) check_interval = 10 @@ -93,7 +93,7 @@ def _is_deployment_ready(self) -> bool: try: deployment = self.lightkube_client.get( Deployment, - name=f"{self.app.name}-{self.model.name}-waypoint", + name=f"{self._managed_labels}-waypoint", namespace=self.model.name, ) if ( @@ -108,9 +108,9 @@ def _is_deployment_ready(self) -> bool: return False - def _is_ready(self) -> bool: + def _is_waypoint_ready(self) -> bool: - if not self._is_deployment_ready(): + if not self._is_waypoint_deployment_ready(): return False return True @@ -120,7 +120,7 @@ def _sync_all_resources(self): return self.unit.status = MaintenanceStatus("Validating waypoint readiness") self._sync_waypoint_resources() - if not self._is_ready(): + if not self._is_waypoint_ready(): self.unit.status = BlockedStatus( "Waypoint's k8s deployment not ready, is istio properly installed?" ) @@ -130,7 +130,7 @@ def _sync_all_resources(self): def _construct_waypoint(self): gateway = IstioWaypointResource( metadata=Metadata( - name=f"{self.app.name}-{self.model.name}-waypoint", + name=f"{self._managed_labels}-waypoint", namespace=self.model.name, labels={"istio.io/waypoint-for": "service"}, ), @@ -183,40 +183,47 @@ def _add_labels(self): """Add specific labels to the namespace.""" namespace = self._get_namespace() if not namespace: + raise RuntimeError(f"Error fetching namespace: {namespace}") + + # Ensure metadata is not None + if namespace.metadata is None: + namespace.metadata = ObjectMeta() + + # Ensure labels are a dictionary even if they are initially None or not set + if namespace.metadata.labels is None: + namespace.metadata.labels = {} + + existing_labels = namespace.metadata.labels + if ( + existing_labels.get("istio.io/use-waypoint") + or existing_labels.get("istio.io/dataplane-mode") + ) and existing_labels.get( + "charms.canonical.com/istio.io.waypoint.managed-by" + ) != f"{self._managed_labels}": + logger.error( + f"Cannot add labels: Namespace '{self.model.name}' is already configured with Istio labels managed by another entity." + ) return - if namespace.metadata and namespace.metadata.labels: - existing_labels = namespace.metadata.labels - if ( - existing_labels.get("istio.io/use-waypoint") - or existing_labels.get("istio.io/dataplane-mode") - ) and existing_labels.get( - "istio.io.waypoint-managed-by" - ) != f"{self.app.name}-{self.model.name}": - logger.error( - f"Cannot add labels: Namespace '{self.model.name}' is already configured with Istio labels managed by another entity." - ) - return - - labels_to_add = { - "istio.io/use-waypoint": f"{self.app.name}-{self.model.name}-waypoint", - "istio.io/dataplane-mode": "ambient", - "istio.io.waypoint-managed-by": f"{self.app.name}-{self.model.name}", - } + labels_to_add = { + "istio.io/use-waypoint": f"{self._managed_labels}-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": f"{self._managed_labels}", + } - namespace.metadata.labels.update(labels_to_add) - self._patch_namespace(namespace) + namespace.metadata.labels.update(labels_to_add) + self._patch_namespace(namespace) def _remove_labels(self): """Remove specific labels from the namespace.""" namespace = self._get_namespace() if not namespace: - return + raise RuntimeError(f"Error fetching namespace: {namespace}") if namespace.metadata and namespace.metadata.labels: if ( - namespace.metadata.labels.get("istio.io.waypoint-managed-by") - != f"{self.app.name}-{self.model.name}" + namespace.metadata.labels.get("charms.canonical.com/istio.io.waypoint.managed-by") + != f"{self._managed_labels}" ): logger.warning( f"Cannot remove labels: Namespace '{self.model.name}' has Istio labels managed by another entity." @@ -226,7 +233,7 @@ def _remove_labels(self): labels_to_remove = { "istio.io/use-waypoint": None, "istio.io/dataplane-mode": None, - "istio.io.waypoint-managed-by": None, + "charms.canonical.com/istio.io.waypoint.managed-by": None, } namespace.metadata.labels.update(labels_to_remove) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index b5079ae..a5353e6 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,7 +1,8 @@ +#!/usr/bin/env python3 + # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. - import functools import logging from collections import defaultdict diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index d883dc8..67407bf 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -1,3 +1,8 @@ +#!/usr/bin/env python3 + +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + from lightkube.core.client import Client from lightkube.resources.core_v1 import Namespace from pytest_operator.plugin import OpsTest @@ -13,7 +18,7 @@ async def validate_labels(ops_test: OpsTest, app_name: str, should_be_present: b expected_labels = { "istio.io/use-waypoint": f"{app_name}-{namespace_name}-waypoint", "istio.io/dataplane-mode": "ambient", - "istio.io.waypoint-managed-by": f"{app_name}-{namespace_name}", + "charms.canonical.com/istio.io.waypoint.managed-by": f"{app_name}-{namespace_name}", } for label, expected_value in expected_labels.items(): diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 2f5e613..54dcbe4 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -1,3 +1,5 @@ +#!/usr/bin/env python3 + # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py new file mode 100644 index 0000000..119765d --- /dev/null +++ b/tests/unit/test_charm.py @@ -0,0 +1,243 @@ +#!/usr/bin/env python3 + +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + +from unittest.mock import MagicMock, patch + +import pytest +from lightkube.models.meta_v1 import ObjectMeta +from lightkube.resources.core_v1 import Namespace +from ops.testing import Harness + +from charm import IstioBeaconCharm + + +@pytest.fixture() +def harness(): + harness = Harness(IstioBeaconCharm) + harness.set_model_name("istio-system") + yield harness + harness.cleanup() + + +def test_add_labels(harness: Harness[IstioBeaconCharm]): + """Test the _add_labels method with namespace labeling logic.""" + harness.begin() + charm = harness.charm + + mock_namespace = Namespace( + metadata=ObjectMeta( + name="istio-system", + labels={ + "foo": "bar", + }, + ) + ) + + # Scenario #1: Namespace has other k8s labels with no istio labels + # Expected: Positive Scenario, istio labels to be updated normally + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + + charm._add_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + } + + # Scenario #2: Namespace labels are managed by another entity + # Expected: Negative Scenario, labels shouldn't be added + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + entity_managed_labels = { + "istio.io/use-waypoint": "another-entity", + "istio.io/dataplane-mode": "ambient", + } + mock_namespace.metadata.labels.update(entity_managed_labels) + charm._add_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_not_called() + assert mock_namespace.metadata.labels == entity_managed_labels + + # Scenario #3: Namespace labels are managed by this charm + # Expected: Positive Scenario, istio labels to be updated normally + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + istio_managed_labels = { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + } + mock_namespace.metadata.labels.update(istio_managed_labels) + charm._add_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == istio_managed_labels + + # Scenario #4: Namespace has no labels configured, not even external k8s ones + # Expected: Positive Scenario, istio labels to be updated normally + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + istio_managed_labels = { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + } + charm._add_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == istio_managed_labels + + +def test_remove_labels(harness: Harness[IstioBeaconCharm]): + """Test the _remove_labels method with namespace labeling logic.""" + harness.begin() + charm = harness.charm + + mock_namespace = Namespace( + metadata=ObjectMeta( + name="istio-system", + labels={ + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + ) + ) + + # Scenario #1: Namespace labels are managed by this charm + # Expected: Positive Scenario, istio labels to be removed + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + + charm._remove_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == { + "foo": "bar", + "charms.canonical.com/istio.io.waypoint.managed-by": None, + "istio.io/dataplane-mode": None, + "istio.io/use-waypoint": None, + } + + # Scenario #2: Namespace labels are managed by another entity + # Expected: Negative Scenario, labels shouldn't be removed + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + entity_managed_labels = { + "istio.io/use-waypoint": "another-entity", + "istio.io/dataplane-mode": "ambient", + } + mock_namespace.metadata.labels.update(entity_managed_labels) + charm._remove_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_not_called() + assert mock_namespace.metadata.labels == entity_managed_labels + + # Scenario #3: Namespace has some labels missing, but managed by this charm + # Expected: Positive Scenario, existing istio labels to be removed + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + partial_labels = { + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + } + mock_namespace.metadata.labels.update(partial_labels) + charm._remove_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == { + "foo": "bar", + "charms.canonical.com/istio.io.waypoint.managed-by": None, + "istio.io/dataplane-mode": None, + "istio.io/use-waypoint": None, + } + + # Scenario #4: Namespace has no labels configured at all + # Expected: No operation is performed, as there are no labels to remove + with patch.object( + charm.lightkube_client, "get", return_value=mock_namespace + ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: + mock_namespace.metadata.labels.clear() + charm._remove_labels() + mock_get.assert_called_once_with(Namespace, "istio-system") + mock_patch.assert_not_called() + assert mock_namespace.metadata.labels == {} + + +def test_sync_waypoint_resources_add_labels(harness: Harness[IstioBeaconCharm]): + """Test _sync_waypoint_resources when model-on-mesh is True.""" + harness.begin() + harness.update_config({"model-on-mesh": True}) + charm = harness.charm + + with patch.object(charm, "_get_waypoint_resource_manager") as mock_krm, patch.object( + charm, "_construct_waypoint" + ) as mock_construct_waypoint, patch.object( + charm, "_add_labels" + ) as mock_add_labels, patch.object( + charm, "_remove_labels" + ) as mock_remove_labels: + + mock_krm.return_value.reconcile = MagicMock() + mock_construct_waypoint.return_value = MagicMock() + + charm._sync_waypoint_resources() + + # Ensure resource manager and waypoint construction were called + mock_krm.return_value.reconcile.assert_called_once() + mock_construct_waypoint.assert_called_once() + + # Ensure add_labels is called since model-on-mesh is True + mock_add_labels.assert_called_once() + + # Ensure remove_labels is not called + mock_remove_labels.assert_not_called() + + +def test_sync_waypoint_resources_remove_labels(harness: Harness[IstioBeaconCharm]): + """Test _sync_waypoint_resources when model-on-mesh is False.""" + harness.begin() + harness.update_config({"model-on-mesh": False}) + charm = harness.charm + + with patch.object(charm, "_get_waypoint_resource_manager") as mock_krm, patch.object( + charm, "_construct_waypoint" + ) as mock_construct_waypoint, patch.object( + charm, "_add_labels" + ) as mock_add_labels, patch.object( + charm, "_remove_labels" + ) as mock_remove_labels: + + mock_krm.return_value.reconcile = MagicMock() + mock_construct_waypoint.return_value = MagicMock() + + charm._sync_waypoint_resources() + + # Ensure resource manager and waypoint construction were called + mock_krm.return_value.reconcile.assert_called_once() + mock_construct_waypoint.assert_called_once() + + # Ensure remove_labels is called since model-on-mesh is False + mock_remove_labels.assert_called_once() + + # Ensure add_labels is not called + mock_add_labels.assert_not_called() diff --git a/tox.ini b/tox.ini index 2456d64..d9f4863 100644 --- a/tox.ini +++ b/tox.ini @@ -46,8 +46,16 @@ commands = [testenv:unit] description = Run unit tests +deps = + pytest + pytest-mock + coverage[toml] + -r {tox_root}/requirements.txt commands = - ; + coverage run \ + --source={[vars]src_path} \ + -m pytest -v --tb native --log-cli-level=INFO -s {posargs} {[vars]tests_path}/unit + coverage report -m [testenv:scenario] description = Run scenario tests From 8383476e1e2af9e06ed29bbd80e6056eb0b82183 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Thu, 22 Aug 2024 11:32:18 +0000 Subject: [PATCH 09/13] mock lk kubeconfig --- tests/unit/conftest.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 tests/unit/conftest.py diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py new file mode 100644 index 0000000..3bb8574 --- /dev/null +++ b/tests/unit/conftest.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 + +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + +from unittest.mock import patch + +import pytest +from lightkube import Client + + +@pytest.fixture(autouse=True) +def mock_lightkube_client(): + """Global mock for the Lightkube Client to avoid loading kubeconfig in CI.""" + with patch.object(Client, "__init__", lambda self, *args, **kwargs: None): + with patch.object(Client, "get"): + with patch.object(Client, "patch"): + yield From 71154a15ec46229e396280d11107215ad609f3c2 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Thu, 22 Aug 2024 12:16:44 +0000 Subject: [PATCH 10/13] address comments and add tests (iter V) --- charmcraft.yaml | 2 +- src/charm.py | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index a897e4b..60a1f7d 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -55,7 +55,7 @@ config: The maximum time (in seconds) to wait for the waypoint deployment to be ready. This applies specifically to the deployment created for the Istio waypoint controller. If the deployment does not become ready within this time, - charm will go into blocked state. + charm will go into error state. model-on-mesh: type: boolean default: false diff --git a/src/charm.py b/src/charm.py index 3dfd7b1..90713b8 100755 --- a/src/charm.py +++ b/src/charm.py @@ -121,10 +121,7 @@ def _sync_all_resources(self): self.unit.status = MaintenanceStatus("Validating waypoint readiness") self._sync_waypoint_resources() if not self._is_waypoint_ready(): - self.unit.status = BlockedStatus( - "Waypoint's k8s deployment not ready, is istio properly installed?" - ) - return + raise RuntimeError("Waypoint's k8s deployment not ready, is istio properly installed?") self.unit.status = ActiveStatus() def _construct_waypoint(self): From 8cb343701609f9e0b90ecac7a3583e3ec8a0a1dc Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Thu, 22 Aug 2024 12:45:54 +0000 Subject: [PATCH 11/13] address comments (iter VI) --- src/charm.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/charm.py b/src/charm.py index 90713b8..714cb55 100755 --- a/src/charm.py +++ b/src/charm.py @@ -101,8 +101,9 @@ def _is_waypoint_deployment_ready(self) -> bool: and deployment.status.readyReplicas == deployment.status.replicas ): return True - except ApiError as e: - logger.error(f"Error checking waypoint deployment status: {e}") + logger.info("Deployment not ready, retrying...") + except ApiError: + logger.info("Deployment not found, retrying...") time.sleep(check_interval) From cc58f082b00b5cf25b844d30b331552d201dd8b1 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 26 Aug 2024 08:51:45 +0000 Subject: [PATCH 12/13] address comments (iter VII) --- .github/workflows/pull-request.yaml | 6 +- src/charm.py | 8 +- tests/unit/test_charm.py | 253 ++++++++++++++-------------- 3 files changed, 134 insertions(+), 133 deletions(-) diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 776be8d..335143d 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -5,7 +5,11 @@ on: branches: - main -# TODO: Remove when #8 closes +# The following configuration is temporarily pinned to this branch to enable Juju to restart hooks, +# which are disabled by default (https://github.com/charmed-kubernetes/actions-operator/blob/5c6377ed695d52b8a1693f07b7d641e245269123/src/bootstrap/index.ts#L205). +# Note: This causes integration tests to fail as charms lose connection to the Juju controller +# when added to the mesh, regaining connection only upon hook retry. +# TODO: Revert to main branch after #8 is resolved. jobs: pull-request: name: PR diff --git a/src/charm.py b/src/charm.py index 714cb55..8c08fb9 100755 --- a/src/charm.py +++ b/src/charm.py @@ -188,10 +188,10 @@ def _add_labels(self): namespace.metadata = ObjectMeta() # Ensure labels are a dictionary even if they are initially None or not set - if namespace.metadata.labels is None: - namespace.metadata.labels = {} + if namespace.metadata.labels is None: # pyright: ignore + namespace.metadata.labels = {} # pyright: ignore - existing_labels = namespace.metadata.labels + existing_labels = namespace.metadata.labels # pyright: ignore if ( existing_labels.get("istio.io/use-waypoint") or existing_labels.get("istio.io/dataplane-mode") @@ -209,7 +209,7 @@ def _add_labels(self): "charms.canonical.com/istio.io.waypoint.managed-by": f"{self._managed_labels}", } - namespace.metadata.labels.update(labels_to_add) + namespace.metadata.labels.update(labels_to_add) # pyright: ignore self._patch_namespace(namespace) def _remove_labels(self): diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 119765d..4a804bb 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -21,7 +21,59 @@ def harness(): harness.cleanup() -def test_add_labels(harness: Harness[IstioBeaconCharm]): +@pytest.mark.parametrize( + "labels_before, patched, labels_after", + [ + ( + # Assert that, when there are no waypoint labels, the expected labels are added + {}, + True, + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + }, + ), + ( + # Assert that existing labels get preserved on positive cases + {"foo": "bar"}, + True, + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + ), + ( + # Assert that, when we already manage the labels, they get updated + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + # "istio.io/dataplane-mode": "ambient", # omitted for this case on purpose + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + True, + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + ), + # Assert that, when we we do not manage the labels, they do not get updated + ( + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", # omitted for this case on purpose + "foo": "bar", + }, + False, + "unused arg", + ), + ], +) +def test_add_labels(harness: Harness[IstioBeaconCharm], labels_before, patched, labels_after): """Test the _add_labels method with namespace labeling logic.""" harness.begin() charm = harness.charm @@ -29,79 +81,80 @@ def test_add_labels(harness: Harness[IstioBeaconCharm]): mock_namespace = Namespace( metadata=ObjectMeta( name="istio-system", - labels={ - "foo": "bar", - }, + labels=labels_before, ) ) - # Scenario #1: Namespace has other k8s labels with no istio labels - # Expected: Positive Scenario, istio labels to be updated normally - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - - charm._add_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) - assert mock_namespace.metadata.labels == { - "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", - "istio.io/dataplane-mode": "ambient", - "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", - "foo": "bar", - } - - # Scenario #2: Namespace labels are managed by another entity - # Expected: Negative Scenario, labels shouldn't be added - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() - entity_managed_labels = { - "istio.io/use-waypoint": "another-entity", - "istio.io/dataplane-mode": "ambient", - } - mock_namespace.metadata.labels.update(entity_managed_labels) - charm._add_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_not_called() - assert mock_namespace.metadata.labels == entity_managed_labels - - # Scenario #3: Namespace labels are managed by this charm - # Expected: Positive Scenario, istio labels to be updated normally with patch.object( charm.lightkube_client, "get", return_value=mock_namespace ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() - istio_managed_labels = { - "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", - "istio.io/dataplane-mode": "ambient", - "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", - } - mock_namespace.metadata.labels.update(istio_managed_labels) - charm._add_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) - assert mock_namespace.metadata.labels == istio_managed_labels - # Scenario #4: Namespace has no labels configured, not even external k8s ones - # Expected: Positive Scenario, istio labels to be updated normally - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() - istio_managed_labels = { - "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", - "istio.io/dataplane-mode": "ambient", - "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", - } charm._add_labels() mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) - assert mock_namespace.metadata.labels == istio_managed_labels - - -def test_remove_labels(harness: Harness[IstioBeaconCharm]): + if patched: + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == labels_after + else: + mock_patch.assert_not_called() + assert mock_namespace.metadata.labels == labels_before + + +@pytest.mark.parametrize( + "labels_before, patched, labels_after", + [ + ( + # Scenario 1: Namespace labels are managed by this charm + { + "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", + "istio.io/dataplane-mode": "ambient", + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + True, + { + "foo": "bar", + "istio.io/use-waypoint": None, + "istio.io/dataplane-mode": None, + "charms.canonical.com/istio.io.waypoint.managed-by": None, + }, + ), + ( + # Scenario 2: Namespace labels are managed by another entity + { + "istio.io/use-waypoint": "another-entity", + "istio.io/dataplane-mode": "ambient", + "foo": "bar", + }, + False, + { + "istio.io/use-waypoint": "another-entity", + "istio.io.dataplane-mode": "ambient", + "foo": "bar", + }, + ), + ( + # Scenario 3: Namespace labels are partially managed by this charm + { + "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", + "foo": "bar", + }, + True, + { + "foo": "bar", + "istio.io/use-waypoint": None, + "istio.io/dataplane-mode": None, + "charms.canonical.com/istio.io.waypoint.managed-by": None, + }, + ), + ( + # Scenario 4: Namespace has no labels configured at all + {}, + False, + {}, + ), + ], +) +def test_remove_labels(harness: Harness[IstioBeaconCharm], labels_before, patched, labels_after): """Test the _remove_labels method with namespace labeling logic.""" harness.begin() charm = harness.charm @@ -109,78 +162,22 @@ def test_remove_labels(harness: Harness[IstioBeaconCharm]): mock_namespace = Namespace( metadata=ObjectMeta( name="istio-system", - labels={ - "istio.io/use-waypoint": "istio-beacon-k8s-istio-system-waypoint", - "istio.io/dataplane-mode": "ambient", - "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", - "foo": "bar", - }, + labels=labels_before, ) ) - # Scenario #1: Namespace labels are managed by this charm - # Expected: Positive Scenario, istio labels to be removed - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - - charm._remove_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) - assert mock_namespace.metadata.labels == { - "foo": "bar", - "charms.canonical.com/istio.io.waypoint.managed-by": None, - "istio.io/dataplane-mode": None, - "istio.io/use-waypoint": None, - } - - # Scenario #2: Namespace labels are managed by another entity - # Expected: Negative Scenario, labels shouldn't be removed with patch.object( charm.lightkube_client, "get", return_value=mock_namespace ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() - entity_managed_labels = { - "istio.io/use-waypoint": "another-entity", - "istio.io/dataplane-mode": "ambient", - } - mock_namespace.metadata.labels.update(entity_managed_labels) - charm._remove_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_not_called() - assert mock_namespace.metadata.labels == entity_managed_labels - # Scenario #3: Namespace has some labels missing, but managed by this charm - # Expected: Positive Scenario, existing istio labels to be removed - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() - partial_labels = { - "charms.canonical.com/istio.io.waypoint.managed-by": "istio-beacon-k8s-istio-system", - "foo": "bar", - } - mock_namespace.metadata.labels.update(partial_labels) - charm._remove_labels() - mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) - assert mock_namespace.metadata.labels == { - "foo": "bar", - "charms.canonical.com/istio.io.waypoint.managed-by": None, - "istio.io/dataplane-mode": None, - "istio.io/use-waypoint": None, - } - - # Scenario #4: Namespace has no labels configured at all - # Expected: No operation is performed, as there are no labels to remove - with patch.object( - charm.lightkube_client, "get", return_value=mock_namespace - ) as mock_get, patch.object(charm.lightkube_client, "patch") as mock_patch: - mock_namespace.metadata.labels.clear() charm._remove_labels() mock_get.assert_called_once_with(Namespace, "istio-system") - mock_patch.assert_not_called() - assert mock_namespace.metadata.labels == {} + if patched: + mock_patch.assert_called_once_with(Namespace, "istio-system", mock_namespace) + assert mock_namespace.metadata.labels == labels_after + else: + mock_patch.assert_not_called() + assert mock_namespace.metadata.labels == labels_before def test_sync_waypoint_resources_add_labels(harness: Harness[IstioBeaconCharm]): From c01b21b921bf8d09016162d08855c6d526f899e4 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Mon, 26 Aug 2024 08:53:21 +0000 Subject: [PATCH 13/13] fix linting --- src/charm.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/charm.py b/src/charm.py index 8c08fb9..91b13f1 100755 --- a/src/charm.py +++ b/src/charm.py @@ -188,10 +188,10 @@ def _add_labels(self): namespace.metadata = ObjectMeta() # Ensure labels are a dictionary even if they are initially None or not set - if namespace.metadata.labels is None: # pyright: ignore - namespace.metadata.labels = {} # pyright: ignore + if namespace.metadata.labels is None: # pyright: ignore + namespace.metadata.labels = {} # pyright: ignore - existing_labels = namespace.metadata.labels # pyright: ignore + existing_labels = namespace.metadata.labels # pyright: ignore if ( existing_labels.get("istio.io/use-waypoint") or existing_labels.get("istio.io/dataplane-mode") @@ -209,7 +209,7 @@ def _add_labels(self): "charms.canonical.com/istio.io.waypoint.managed-by": f"{self._managed_labels}", } - namespace.metadata.labels.update(labels_to_add) # pyright: ignore + namespace.metadata.labels.update(labels_to_add) # pyright: ignore self._patch_namespace(namespace) def _remove_labels(self):