refactor: use the istioctl wrapper for installing the control plane (#…

…357)

* refactor: use the istioctl wrapper for installing the control plane

This refactor uses the install method form the istioctl wrapper to
install the control plane without calling subprocess directly in the
charm code.

This commit also changes the unit tests and lints the code and tests
to adapt to the recent refactor.

Part of #351

charms/istio-pilot/src/charm.py

@@ -2,7 +2,6 @@
 
 import base64
 import logging
-import subprocess
 from typing import List, Optional
 
 import tenacity
@@ -66,6 +65,11 @@
 VIRTUAL_SERVICE_TEMPLATE_FILES = ["src/manifests/virtual_service.yaml.j2"]
 ISTIOCTL_PATH = "./istioctl"
 ISTIOCTL_DEPOYMENT_PROFILE = "minimal"
+INSTALL_FAILED_MSG = (
+    "Failed to install Istio Control Plane. "
+    "{message} Make sure the cluster has no Istio installations already present and "
+    "that you have provided the right configuration values."
+)
 UPGRADE_FAILED_MSG = (
     "Failed to upgrade Istio.  {message}  To recover Istio, see [the upgrade docs]"
     "(https://github.com/canonical/istio-operators/blob/main/charms/istio-pilot/README.md) for "
@@ -178,6 +182,7 @@ def _get_image_config(self):
 
     def install(self, _):
         """Install charm."""
+
         self._log_and_set_status(MaintenanceStatus("Deploying Istio control plane"))
 
         image_config = self._get_image_config()
@@ -187,29 +192,36 @@ def install(self, _):
         global_proxy_image = image_config["global-proxy-image"]
         global_proxy_init_image = image_config["global-proxy-init-image"]
 
-        # Call istioctl install and set parameters based on image configuration
-        subprocess.check_call(
-            [
-                "./istioctl",
-                "install",
-                "-y",
-                "--set",
-                "profile=minimal",
-                "--set",
-                f"values.global.istioNamespace={self.model.name}",
-                "--set",
-                f"values.pilot.image={pilot_image}",
-                "--set",
-                f"values.global.tag={global_tag}",
-                "--set",
-                f"values.global.hub={global_hub}",
-                "--set",
-                f"values.global.proxy.image={global_proxy_image}",
-                "--set",
-                f"values.global.proxy_init.image={global_proxy_init_image}",
-            ]
+        # Generate extra flags to pass to the istioctl install command
+        istioctl_extra_flags = [
+            "--set",
+            f"values.pilot.image={pilot_image}",
+            "--set",
+            f"values.global.tag={global_tag}",
+            "--set",
+            f"values.global.hub={global_hub}",
+            "--set",
+            f"values.global.proxy.image={global_proxy_image}",
+            "--set",
+            f"values.global.proxy_init.image={global_proxy_init_image}",
+        ]
+
+        # Call the istioctl wrapper to install the Istio Control Plane
+        istioctl = Istioctl(
+            ISTIOCTL_PATH,
+            self.model.name,
+            ISTIOCTL_DEPOYMENT_PROFILE,
+            istioctl_extra_flags=istioctl_extra_flags,
         )
 
+        try:
+            istioctl.install()
+        except IstioctlError as e:
+            self.log.error(INSTALL_FAILED_MSG.format(message=str(e)))
+            raise GenericCharmRuntimeError(
+                "Failed to install control plane. See juju debug-log for details."
+            ) from e
+
         self.unit.status = ActiveStatus()
 
     def reconcile(self, event):

charms/istio-pilot/src/istioctl.py

@@ -1,5 +1,6 @@
 import logging
 import subprocess
+from typing import List, Optional
 
 import lightkube.resources.policy_v1  # noqa: F401
 import yaml
@@ -11,7 +12,11 @@ class IstioctlError(Exception):
 
 class Istioctl:
     def __init__(
-        self, istioctl_path: str, namespace: str = "istio-system", profile: str = "minimal"
+        self,
+        istioctl_path: str,
+        namespace: str = "istio-system",
+        profile: str = "minimal",
+        istioctl_extra_flags: Optional[List[str]] = None,
     ):
         """Wrapper for the istioctl binary.
 
@@ -22,23 +27,37 @@ def __init__(
             istioctl_path (str): Path to the istioctl binary to be wrapped
             namespace (str): The namespace to install Istio into
             profile (str): The Istio profile to use for installation or upgrades
+            istioctl_extra_flags (optional, list): A list containing extra flags to pass to istioctl
         """
         self._istioctl_path = istioctl_path
         self._namespace = namespace
         self._profile = profile
+        self._istioctl_extra_flags = (
+            istioctl_extra_flags if istioctl_extra_flags is not None else []
+        )
 
     @property
     def _istioctl_flags(self):
-        return [
-            "-s",
+        istioctl_flags = [
+            "--set",
             f"profile={self._profile}",
-            "-s",
+            "--set",
             f"values.global.istioNamespace={self._namespace}",
         ]
+        istioctl_flags.extend(self._istioctl_extra_flags)
+        return istioctl_flags
 
     def install(self):
         """Wrapper for the `istioctl install` command."""
+        install_msg = (
+            "Installing the Istio Control Plane with the following settings:\n"
+            "Profile: {self._profile}\n"
+            "Namespace: {self._namespace}\n"
+            "Istioctl extra flags: {self._istioctl_extra_flags}"
+        )
+
         try:
+            logging.info(install_msg)
             subprocess.check_call([self._istioctl_path, "install", "-y", *self._istioctl_flags])
         except subprocess.CalledProcessError as cpe:
             error_msg = f"Failed to install istio using istioctl.  Exit code: {cpe.returncode}."

charms/istio-pilot/tests/unit/test_charm.py

@@ -1255,7 +1255,7 @@ def test_upgrade_failed_getting_version(
         harness,
         mocked_cert_subject,
     ):
-        """Tests that charm.upgrade_charm fails when precheck fails."""
+        """Tests that charm.upgrade_charm fails when getting version fails."""
         harness.begin()
 
         with pytest.raises(GenericCharmRuntimeError):
@@ -1270,7 +1270,7 @@ def test_upgrade_failed_version_check(
         harness,
         mocked_cert_subject,
     ):
-        """Tests that charm.upgrade_charm fails when precheck fails."""
+        """Tests that charm.upgrade_charm fails when version check fails."""
         model_name = "test-model"
         harness.set_model_name(model_name)
 
@@ -1279,14 +1279,20 @@ def test_upgrade_failed_version_check(
         with pytest.raises(GenericCharmRuntimeError):
             harness.charm.upgrade_charm("mock_event")
 
+    @patch("charm._validate_upgrade_version")  # Do not validate versions
+    @patch("charm.Istioctl.version")  # Pass istioctl version check
+    @patch("charm.Istioctl.precheck")  # Fail istioctl precheck
     @patch("charm.Istioctl.upgrade", side_effect=IstioctlError())  # Fail istioctl upgrade
     def test_upgrade_failed_during_upgrade(
         self,
         _mocked_istioctl_upgrade,
+        _mocked_istioctl_precheck,
+        _mocked_istioctl_version,
+        _mocked_validate_upgrade_version,
         harness,
         mocked_cert_subject,
     ):
-        """Tests that charm.upgrade_charm fails when upgrade process fails."""
+        """Tests that charm.upgrade_charm fails during upgrade."""
         harness.begin()
 
         with pytest.raises(GenericCharmRuntimeError):
@@ -1436,26 +1442,6 @@ def mock_loadbalancer_hostname_service_not_ready():
     return mock_nodeport_service
 
 
-# autouse to ensure we don't accidentally call out, but
-# can also be used explicitly to get access to the mock.
-@pytest.fixture(autouse=True)
-def mocked_check_call(mocker):
-    mocked_check_call = mocker.patch("charm.subprocess.check_call")
-    mocked_check_call.return_value = 0
-
-    yield mocked_check_call
-
-
-# autouse to ensure we don't accidentally call out, but
-# can also be used explicitly to get access to the mock.
-@pytest.fixture(autouse=True)
-def mocked_check_output(mocker):
-    mocked_check_output = mocker.patch("charm.subprocess.check_output")
-    mocked_check_output.return_value = "stdout"
-
-    yield mocked_check_output
-
-
 @pytest.fixture()
 def mocked_lightkube_client(mocked_lightkube_client_class):
     mocked_instance = MagicMock()

charms/istio-pilot/tests/unit/test_istioctl.py

@@ -13,13 +13,19 @@
 PROFILE = "my-profile"
 TEST_DATA_PATH = Path("./tests/unit/istioctl_data/")
 EXAMPLE_MANIFEST = TEST_DATA_PATH / "example_manifest.yaml"
+EXPECTED_ISTIOCTL_FLAGS = [
+    "--set",
+    f"profile={PROFILE}",
+    "--set",
+    f"values.global.istioNamespace={NAMESPACE}",
+]
 
 
 # autouse to ensure we don't accidentally call out, but
 # can also be used explicitly to get access to the mock.
 @pytest.fixture(autouse=True)
 def mocked_check_call(mocker):
-    mocked_check_call = mocker.patch("charm.subprocess.check_call")
+    mocked_check_call = mocker.patch("subprocess.check_call")
     mocked_check_call.return_value = 0
 
     yield mocked_check_call
@@ -29,7 +35,7 @@ def mocked_check_call(mocker):
 # can also be used explicitly to get access to the mock.
 @pytest.fixture(autouse=True)
 def mocked_check_output(mocker):
-    mocked_check_output = mocker.patch("charm.subprocess.check_output")
+    mocked_check_output = mocker.patch("subprocess.check_output")
     mocked_check_output.return_value = "stdout"
 
     yield mocked_check_output
@@ -53,7 +59,7 @@ def mocked_check_output_failing(mocked_check_output):
     yield mocked_check_output
 
 
-def test_istioctl_install(mocked_check_call):
+def test_istioctl_install_no_extra_flags(mocked_check_call):
     """Tests that istioctl.install() calls the binary successfully with the expected arguments."""
     ictl = Istioctl(istioctl_path=ISTIOCTL_BINARY, namespace=NAMESPACE, profile=PROFILE)
 
@@ -64,11 +70,32 @@ def test_istioctl_install(mocked_check_call):
         ISTIOCTL_BINARY,
         "install",
         "-y",
-        "-s",
-        f"profile={PROFILE}",
-        "-s",
-        f"values.global.istioNamespace={NAMESPACE}",
     ]
+    expected_call_args.extend(EXPECTED_ISTIOCTL_FLAGS)
+
+    mocked_check_call.assert_called_once_with(expected_call_args)
+
+
+def test_istioctl_install_extra_flags(mocked_check_call):
+    """Tests that istioctl.install() calls the binary successfully with the expected arguments."""
+    extra_flags = ["--set", "some-flag"]
+    ictl = Istioctl(
+        istioctl_path=ISTIOCTL_BINARY,
+        namespace=NAMESPACE,
+        profile=PROFILE,
+        istioctl_extra_flags=extra_flags,
+    )
+
+    ictl.install()
+
+    # Assert that we call istioctl with the expected arguments
+    expected_call_args = [
+        ISTIOCTL_BINARY,
+        "install",
+        "-y",
+    ]
+    expected_call_args.extend(EXPECTED_ISTIOCTL_FLAGS)
+    expected_call_args.extend(extra_flags)
 
     mocked_check_call.assert_called_once_with(expected_call_args)
 
@@ -92,11 +119,8 @@ def test_istioctl_manifest(mocked_check_output):
         ISTIOCTL_BINARY,
         "manifest",
         "generate",
-        "-s",
-        f"profile={PROFILE}",
-        "-s",
-        f"values.global.istioNamespace={NAMESPACE}",
     ]
+    expected_call_args.extend(EXPECTED_ISTIOCTL_FLAGS)
 
     mocked_check_output.assert_called_once_with(expected_call_args)
 
@@ -158,17 +182,14 @@ def test_istioctl_upgrade(mocked_check_output):
 
     ictl.upgrade()
 
-    mocked_check_output.assert_called_once_with(
-        [
-            ISTIOCTL_BINARY,
-            "upgrade",
-            "-y",
-            "-s",
-            f"profile={PROFILE}",
-            "-s",
-            f"values.global.istioNamespace={NAMESPACE}",
-        ]
-    )
+    expected_call_args = [
+        ISTIOCTL_BINARY,
+        "upgrade",
+        "-y",
+    ]
+    expected_call_args.extend(EXPECTED_ISTIOCTL_FLAGS)
+
+    mocked_check_output.assert_called_once_with(expected_call_args)
 
 
 def test_istioctl_upgrade_error(mocked_check_output_failing):