From 154e3c68cfad5475769d3ba3ce7c80d812a91579 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Mon, 4 Dec 2023 13:08:25 +0100 Subject: [PATCH] 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 | 58 ++++++++++------- charms/istio-pilot/src/istioctl.py | 25 +++++-- charms/istio-pilot/tests/unit/test_charm.py | 32 +++------ .../istio-pilot/tests/unit/test_istioctl.py | 65 ++++++++++++------- 4 files changed, 108 insertions(+), 72 deletions(-) diff --git a/charms/istio-pilot/src/charm.py b/charms/istio-pilot/src/charm.py index 95923746..898eb525 100755 --- a/charms/istio-pilot/src/charm.py +++ b/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 " @@ -176,8 +180,9 @@ def _get_image_config(self): image_config = yaml.safe_load(self.model.config[IMAGE_CONFIGURATION]) return image_config - def install(self, _): + def install(self, event): """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="istioctl install failed")) + raise GenericCharmRuntimeError( + "Failed to install control plane. See juju debug-log for details." + ) from e + self.unit.status = ActiveStatus() def reconcile(self, event): diff --git a/charms/istio-pilot/src/istioctl.py b/charms/istio-pilot/src/istioctl.py index 5f012976..4372fab6 100644 --- a/charms/istio-pilot/src/istioctl.py +++ b/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]] = [], ): """Wrapper for the istioctl binary. @@ -19,6 +24,7 @@ def __init__( and other istioctl commands. Args: + istioctl_extra_flags (optional, list): A list containing extra flags to pass to istioctl 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 @@ -26,19 +32,30 @@ def __init__( self._istioctl_path = istioctl_path self._namespace = namespace self._profile = profile + self._istioctl_extra_flags = istioctl_extra_flags @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}." diff --git a/charms/istio-pilot/tests/unit/test_charm.py b/charms/istio-pilot/tests/unit/test_charm.py index 70109784..37a09269 100644 --- a/charms/istio-pilot/tests/unit/test_charm.py +++ b/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() diff --git a/charms/istio-pilot/tests/unit/test_istioctl.py b/charms/istio-pilot/tests/unit/test_istioctl.py index a38e1230..edc4b486 100644 --- a/charms/istio-pilot/tests/unit/test_istioctl.py +++ b/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):