From 9f27fa20d412aec28527ba5204d701db8a2c5702 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Thu, 24 Aug 2023 15:45:20 +0200 Subject: [PATCH] fix: use reconcilers and istioctl in remove event handler (#315) * fix: use reconcilers and istioctl in remove event handler of istio-pilot Using the many reconcilers we have in the charm code as well as using istioctl uninstall to remove all resources handled by this charm, ensures istio-pilot can be removed properly. This commit refactors the remove method in src/istioctl.py to use istioctl uninstall instead of generating the manifest files and deleting them with lightkube. This guarantees that the tool (which contains all istio knowledge) removes istio resources form the cluster w/o conflicts or leaving traces behind. Fixes #292 * tests: add removal test case on bundle integration tests --- charms/istio-pilot/requirements-unit.txt | 6 ++- charms/istio-pilot/requirements.in | 4 ++ charms/istio-pilot/requirements.txt | 6 ++- charms/istio-pilot/src/charm.py | 41 +++++++------------ charms/istio-pilot/src/istioctl.py | 31 ++++++++------ .../istio-pilot/tests/unit/test_istioctl.py | 17 ++------ tests/test_bundle.py | 8 ++++ 7 files changed, 58 insertions(+), 55 deletions(-) diff --git a/charms/istio-pilot/requirements-unit.txt b/charms/istio-pilot/requirements-unit.txt index 3de81905..586d3810 100644 --- a/charms/istio-pilot/requirements-unit.txt +++ b/charms/istio-pilot/requirements-unit.txt @@ -54,8 +54,10 @@ lightkube==0.14.0 # -r requirements-unit.in # -r requirements.in # charmed-kubeflow-chisme -lightkube-models==1.27.1.4 - # via lightkube +lightkube-models==1.26.0.4 + # via + # -r requirements.in + # lightkube markupsafe==2.1.3 # via jinja2 ops==2.4.1 diff --git a/charms/istio-pilot/requirements.in b/charms/istio-pilot/requirements.in index ecf4db2f..db8e9046 100644 --- a/charms/istio-pilot/requirements.in +++ b/charms/istio-pilot/requirements.in @@ -1,6 +1,10 @@ charmed-kubeflow-chisme jinja2 lightkube +# Pinning lightkube-models to keep compatibility with istio 1.17 +# which supports versions 1.23, 1.24, 1.25, 1.26 of k8s +# Update the pin when this charm is upgraded to <=1.18 +lightkube-models<1.27 ops packaging requests diff --git a/charms/istio-pilot/requirements.txt b/charms/istio-pilot/requirements.txt index 58cd6993..4308df00 100644 --- a/charms/istio-pilot/requirements.txt +++ b/charms/istio-pilot/requirements.txt @@ -44,8 +44,10 @@ lightkube==0.14.0 # via # -r requirements.in # charmed-kubeflow-chisme -lightkube-models==1.27.1.4 - # via lightkube +lightkube-models==1.26.0.4 + # via + # -r requirements.in + # lightkube markupsafe==2.1.3 # via jinja2 ops==2.4.1 diff --git a/charms/istio-pilot/src/charm.py b/charms/istio-pilot/src/charm.py index bbbfec5a..cdae3b48 100755 --- a/charms/istio-pilot/src/charm.py +++ b/charms/istio-pilot/src/charm.py @@ -246,33 +246,22 @@ def reconcile(self, event): # Reports (status and logs) any handled errors, or sets to ActiveStatus self._report_handled_errors(errors=handled_errors) - def remove(self, _): - """Remove charm.""" - - manifests = subprocess.check_output( - [ - "./istioctl", - "manifest", - "generate", - "-s", - "profile=minimal", - "-s", - f"values.global.istioNamespace={self.model.name}", - ] - ) + def remove(self, event): + """Remove charm, the Kubernetes resources it uses, and the istio specific resources.""" + istioctl = Istioctl(ISTIOCTL_PATH, self.model.name, ISTIOCTL_DEPOYMENT_PROFILE) + handled_errors = [] + try: + self._reconcile_gateway() + ingress_data = self._get_ingress_data(event) + self._reconcile_ingress(ingress_data) + ingress_auth_data = self._get_ingress_auth_data(event) + self._reconcile_ingress_auth(ingress_auth_data) + istioctl.remove() + except (ApiError, ErrorWithStatus, IstioctlError) as error: + handled_errors.append(error) - # TODO: Update resource_handler to use the newer handler - custom_resource_classes = [ - self._resource_handler.get_custom_resource_class_from_filename(resource_file) - for resource_file in self._resource_files - ] - for resource in custom_resource_classes: - self._resource_handler.delete_existing_resources( - resource, namespace=self.model.name, ignore_unauthorized=True - ) - self._resource_handler.delete_manifest( - manifests, namespace=self.model.name, ignore_not_found=True, ignore_unauthorized=True - ) + if handled_errors: + self._report_handled_errors(errors=handled_errors) def upgrade_charm(self, _): """Upgrade charm. diff --git a/charms/istio-pilot/src/istioctl.py b/charms/istio-pilot/src/istioctl.py index 610332d2..5f012976 100644 --- a/charms/istio-pilot/src/istioctl.py +++ b/charms/istio-pilot/src/istioctl.py @@ -1,9 +1,8 @@ import logging import subprocess +import lightkube.resources.policy_v1 # noqa: F401 import yaml -from charmed_kubeflow_chisme.lightkube.batch import delete_many -from lightkube import Client, codecs class IstioctlError(Exception): @@ -95,18 +94,26 @@ def precheck(self): ) from cpe def remove(self): - """Removes the Istio installation using istioctl and Lightkube. + """Removes the Istio installation using istioctl. - TODO: Should we use `istioctl x uninstall` here instead of lightkube? It is an - experimental feature but included in all istioctl versions we support. + Raises: + IstioctlError: if the istioctl uninstall subprocess fails """ - manifest = self.manifest() - - # Render YAML into Lightkube Objects - k8s_objects = codecs.load_all_yaml(manifest, create_resources_for_crds=True) - - client = Client() - delete_many(client=client, objs=k8s_objects) + try: + subprocess.check_call( + [ + self._istioctl_path, + "x", + "uninstall", + "--purge", + "-y", + ] + ) + except subprocess.CalledProcessError as cpe: + raise IstioctlError( + "Remove failed during `istioctl x uninstall --purge` with error code" + f" {cpe.returncode}" + ) from cpe def upgrade(self): """Upgrades the Istio installation using istioctl. diff --git a/charms/istio-pilot/tests/unit/test_istioctl.py b/charms/istio-pilot/tests/unit/test_istioctl.py index e519fc5d..a38e1230 100644 --- a/charms/istio-pilot/tests/unit/test_istioctl.py +++ b/charms/istio-pilot/tests/unit/test_istioctl.py @@ -5,7 +5,6 @@ import pytest import yaml from jinja2 import Environment, FileSystemLoader -from lightkube import ApiError from istioctl import Istioctl, IstioctlError, get_client_version, get_control_plane_version @@ -124,26 +123,18 @@ def mocked_lightkube_client(mocker): yield mocked_lightkube_client -def test_istioctl_remove(mocked_check_output, mocked_lightkube_client): - mocked_check_output.return_value = Path(EXAMPLE_MANIFEST).read_text() - +def test_istioctl_remove(mocked_check_call): ictl = Istioctl(istioctl_path=ISTIOCTL_BINARY, namespace=NAMESPACE, profile=PROFILE) ictl.remove() - assert mocked_lightkube_client.delete.call_count == 3 - + mocked_check_call.assert_called_once_with([ISTIOCTL_BINARY, "x", "uninstall", "--purge", "-y"]) -def test_istioctl_remove_error(mocked_check_output, mocked_lightkube_client, mocker): - mocked_check_output.return_value = Path(EXAMPLE_MANIFEST).read_text() - - api_error = ApiError(response=mocker.MagicMock()) - mocked_lightkube_client.delete.return_value = None - mocked_lightkube_client.delete.side_effect = api_error +def test_istioctl_remove_error(mocked_check_call_failing): ictl = Istioctl(istioctl_path=ISTIOCTL_BINARY, namespace=NAMESPACE, profile=PROFILE) - with pytest.raises(ApiError): + with pytest.raises(IstioctlError): ictl.remove() diff --git a/tests/test_bundle.py b/tests/test_bundle.py index a2d90f24..052a5047 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -315,6 +315,14 @@ async def test_disable_ingress_auth(ops_test: OpsTest): ) +async def test_charms_removal(ops_test: OpsTest): + """Test the istio-operators can be removed without errors.""" + # NOTE: the istio-gateway charm has to be removed before istio-pilot since + # the latter contains all the CRDs that istio-gateway depends on. + await ops_test.model.remove_application(ISTIO_GATEWAY_APP_NAME, block_until_done=True) + await ops_test.model.remove_application(ISTIO_PILOT, block_until_done=True) + + def deploy_workload_with_gateway(workload_name: str, gateway_port: int, namespace: str): """Deploys an http server and opens a path through the existing istio proxy.""" client = lightkube.Client()