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

@@ -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

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

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

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.

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.

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()
 
 

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()