diff --git a/charms/istio-pilot/config.yaml b/charms/istio-pilot/config.yaml index 8267a914..497c25bd 100644 --- a/charms/istio-pilot/config.yaml +++ b/charms/istio-pilot/config.yaml @@ -1,4 +1,18 @@ options: + cni-bin-dir: + type: string + default: '' + description: > + Path to CNI binaries, e.g. /opt/cni/bin. If not provided, the Istio control plane will be installed/upgraded with the Istio CNI plugin disabled. + This path depends on the Kubernetes installation, please refer to https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/ + for information to find out the correct path. + cni-conf-dir: + type: string + default: '' + description: Path to conflist files describing the CNI configuration, e.g. /etc/cni/net.d. If not provided, the Istio control plane will be installed/upgraded + with the Istio CNI plugin disabled. + This path depends on the Kubernetes installation, please refer to https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/ + for information to find out the correct path. default-gateway: type: string default: istio-gateway diff --git a/charms/istio-pilot/src/charm.py b/charms/istio-pilot/src/charm.py index 7bcebded..b1162c2f 100755 --- a/charms/istio-pilot/src/charm.py +++ b/charms/istio-pilot/src/charm.py @@ -100,10 +100,11 @@ def __init__(self, *args): self._field_manager = "lightkube" # Instantiate a CertHandler + self.peer_relation_name = "peers" self._cert_handler = CertHandler( self, key="istio-cert", - peer_relation_name="peers", + peer_relation_name=self.peer_relation_name, cert_subject=self._cert_subject, ) @@ -180,11 +181,9 @@ def _get_image_config(self): image_config = yaml.safe_load(self.model.config[IMAGE_CONFIGURATION]) return image_config - def install(self, _): - """Install charm.""" - - self._log_and_set_status(MaintenanceStatus("Deploying Istio control plane")) - + @property + def _istioctl_extra_flags(self): + """Return extra flags to pass to istioctl commands.""" image_config = self._get_image_config() pilot_image = image_config["pilot-image"] global_tag = image_config["global-tag"] @@ -192,8 +191,9 @@ def install(self, _): global_proxy_image = image_config["global-proxy-image"] global_proxy_init_image = image_config["global-proxy-init-image"] - # Generate extra flags to pass to the istioctl install command - istioctl_extra_flags = [ + # Extra flags to pass to the istioctl install command + # These flags will configure the container images used by the control plane + extra_flags = [ "--set", f"values.pilot.image={pilot_image}", "--set", @@ -206,12 +206,41 @@ def install(self, _): f"values.global.proxy_init.image={global_proxy_init_image}", ] + # The following are a set of flags that configure the CNI behaviour + # * components.cni.enabled enables the CNI plugin + # * values.cni.cniBinDir and values.cni.cniConfDir tell the plugin where to find + # the CNI binaries and config files + # * values.sidecarInjectorWebhook.injectedAnnotations allows users to inject any + # annotations to the sidecar injected Pods. This particular annotation helps + # provide a solution for canonical/istio-operators#356 + if self._check_cni_configurations(): + extra_flags.extend( + [ + "--set", + "components.cni.enabled=true", + "--set", + f"values.cni.cniBinDir={self.model.config['cni-bin-dir']}", + "--set", + f"values.cni.cniConfDir={self.model.config['cni-conf-dir']}", + "--set", + "values.sidecarInjectorWebhook.injectedAnnotations.traffic\.sidecar\.istio\.io/excludeOutboundIPRanges=0.0.0.0/0", # noqa + ] + ) + return extra_flags + + def install(self, _): + """Install charm.""" + + self._log_and_set_status( + MaintenanceStatus("Deploying Istio control plane with Istio CNI plugin.") + ) + # 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, + istioctl_extra_flags=self._istioctl_extra_flags, ) try: @@ -227,8 +256,9 @@ def install(self, _): def reconcile(self, event): """Reconcile the state of the charm. - This is the main entrypoint for the charm. It: + This is the main entrypoint for the method. It: * Checks if we are the leader, exiting early with WaitingStatus if we are not + * Upgrades the Istio control plane if changes were made to the CNI plugin configurations * Sends data to the istio-pilot relation * Reconciles the ingress-auth relation, establishing whether we need authentication on our ingress gateway @@ -251,6 +281,15 @@ def reconcile(self, event): # so that we can report them at the end. handled_errors = [] + # Call upgrade_charm in case there are new configurations that affect the control plane + # only if the CNI configurations have been provided and have changed from a previous state + # This is useful when there is a missing configuration during the install process + try: + if self._cni_config_changed: + self.upgrade_charm(event) + except GenericCharmRuntimeError as err: + handled_errors.append(err) + # Send istiod information to the istio-pilot relation try: self._handle_istio_pilot_relation() @@ -319,8 +358,14 @@ def upgrade_charm(self, _): Supports upgrade of exactly one minor version at a time. """ - istioctl = Istioctl(ISTIOCTL_PATH, self.model.name, ISTIOCTL_DEPOYMENT_PROFILE) - self._log_and_set_status(MaintenanceStatus("Upgrading Istio")) + self._log_and_set_status(MaintenanceStatus("Upgrading Istio control plane.")) + + istioctl = Istioctl( + ISTIOCTL_PATH, + self.model.name, + ISTIOCTL_DEPOYMENT_PROFILE, + istioctl_extra_flags=self._istioctl_extra_flags, + ) # Check for version compatibility for the upgrade try: @@ -804,6 +849,46 @@ def _log_and_set_status(self, status): log_destination_map[type(status)](status.message) + def _check_cni_configurations(self) -> bool: + """Return True if the necessary CNI configuration options are set, False otherwise.""" + return self.model.config["cni-conf-dir"] and self.model.config["cni-bin-dir"] + + def _cni_config_changed(self): + """ + Returns True if any of the CNI configuration options has changed from a previous state, + False otherwise. + """ + # The peer relation is required to store values, if it does not exist because it was + # removed by accident, the charm should fail + rel = self.model.get_relation(self.peer_relation_name, None) + if not rel: + raise GenericCharmRuntimeError( + "The istio-pilot charm requires a peer relation, make sure it exists." + ) + + # Get current values of the configuration options + current_cni_bin_dir = rel.data[self.unit].get("cni-bin-dir", None) + current_cni_conf_dir = rel.data[self.unit].get("cni-conf-dir", None) + + # Update the values based on the configuration options + rel.data[self.unit].update({"cni-bin-dir": self.model.config["cni-bin-dir"]}) + rel.data[self.unit].update({"cni-conf-dir": self.model.config["cni-conf-dir"]}) + + new_cni_bin_dir = rel.data[self.unit].get("cni-bin-dir", None) + new_cni_conf_dir = rel.data[self.unit].get("cni-conf-dir", None) + + # Compare current vs new values and decide whether they have changed from a previous state + cni_bin_dir_changed = False + cni_conf_dir_changed = False + if current_cni_bin_dir != new_cni_bin_dir: + cni_bin_dir_changed = True + + if current_cni_conf_dir != new_cni_conf_dir: + cni_conf_dir_changed = True + + # If any of the configuration options changed, return True + return cni_bin_dir_changed or cni_conf_dir_changed + def _get_gateway_address_from_svc(svc): """Returns the gateway service address from a kubernetes Service. diff --git a/charms/istio-pilot/tests/unit/test_charm.py b/charms/istio-pilot/tests/unit/test_charm.py index 37a09269..ba65d4a3 100644 --- a/charms/istio-pilot/tests/unit/test_charm.py +++ b/charms/istio-pilot/tests/unit/test_charm.py @@ -195,6 +195,7 @@ def test_ingress_auth_and_gateway( self, mocked_remove_gateway, harness, + mocker, mocked_lightkube_client, kubernetes_resource_handler_with_client, ): @@ -215,6 +216,7 @@ def test_ingress_auth_and_gateway( harness.update_config({"default-gateway": gateway_name}) harness.begin() + mocker.patch("charm.Operator.upgrade_charm") harness.charm.log = MagicMock() @@ -269,6 +271,7 @@ def test_ingress_relation( self, mocked_handle_istio_pilot_relation, harness, + mocker, mocked_lightkube_client, kubernetes_resource_handler_with_client, ): @@ -290,6 +293,7 @@ def test_ingress_relation( harness.update_config({"default-gateway": gateway_name}) harness.begin() + mocker.patch("charm.Operator.upgrade_charm") # Do a reconcile harness.charm.on.config_changed.emit() @@ -339,7 +343,7 @@ def test_ingress_relation( assert "handled 1 error" in harness.charm.model.unit.status.message def test_istio_pilot_relation( - self, harness, mocked_lightkube_client, kubernetes_resource_handler_with_client + self, harness, mocker, mocked_lightkube_client, kubernetes_resource_handler_with_client ): """Charm e2e test that asserts we correctly broadcast data on the istio-pilot relation.""" krh_class, krh_lightkube_client = kubernetes_resource_handler_with_client @@ -351,6 +355,7 @@ def test_istio_pilot_relation( harness.update_config({"default-gateway": gateway_name}) harness.begin() + mocker.patch("charm.Operator.upgrade_charm") # Do a reconcile harness.charm.on.config_changed.emit() @@ -371,6 +376,7 @@ def test_gateway_info_relation( self, mocked_is_gateway_service_up, harness, + mocker, mocked_lightkube_client, kubernetes_resource_handler_with_client, ): @@ -385,6 +391,7 @@ def test_gateway_info_relation( mocked_is_gateway_service_up.return_value = True harness.begin() + mocker.patch("charm.Operator.upgrade_charm") # Act and assert # Add gateway-info relation and check it posts data correctly @@ -399,6 +406,58 @@ def test_gateway_info_relation( assert actual_gateway_up == "true" assert harness.charm.model.unit.status == ActiveStatus() + @pytest.mark.parametrize( + "current_cni_bin_dir, current_cni_conf_dir, new_cni_bin_dir, new_cni_conf_dir, expected_output", # noqa + [ + ("current-bin", "current-conf", "current-bin", "current-conf", False), + ("current-bin", "current-conf", "new-bin", "new-conf", True), + ("current-bin", "current-conf", "current-bin", "new-conf", True), + ("current-bin", "", "new-bin", "", True), + ("", "current-conf", "", "new-conf", True), + ("", "", "", "", False), + ], + ) + def test_cni_config_changed( + self, + current_cni_bin_dir, + current_cni_conf_dir, + new_cni_bin_dir, + new_cni_conf_dir, + expected_output, + harness, + mocked_lightkube_client, + ): + model_name = "my-model" + harness.set_leader(True) + harness.set_model_name(model_name) + + # Set peer relation + rel_id = harness.add_relation("peers", "istio-pilot") + harness.add_relation_unit(rel_id, "istio-pilot/0") + + # Set up relation data with current config values + harness.update_relation_data(rel_id, "istio-pilot/0", {"cni-bin-dir": current_cni_bin_dir}) + harness.update_relation_data( + rel_id, "istio-pilot/0", {"cni-conf-dir": current_cni_conf_dir} + ) + + # Update config values + harness.update_config({"cni-bin-dir": new_cni_bin_dir}) + harness.update_config({"cni-conf-dir": new_cni_conf_dir}) + harness.begin() + actual_output = harness.charm._cni_config_changed() + assert actual_output is expected_output + + def test_cni_config_changed_no_peer_relation(self, harness, mocked_lightkube_client): + model_name = "my-model" + harness.set_leader(True) + harness.set_model_name(model_name) + + harness.begin() + + with pytest.raises(GenericCharmRuntimeError): + harness.charm._cni_config_changed() + class TestCharmHelpers: """Directly test charm helpers and private methods.""" @@ -406,6 +465,7 @@ class TestCharmHelpers: def test_reconcile_handling_nonfatal_errors( self, harness, + mocker, all_operator_reconcile_handlers_mocked, mocked_cert_subject, ): @@ -426,6 +486,7 @@ def test_reconcile_handling_nonfatal_errors( ) harness.begin() + mocker.patch("charm.Operator.upgrade_charm") # Act harness.charm.reconcile("event") @@ -970,6 +1031,27 @@ def test_reconcile_ingress_auth_no_auth( mocked_remove_envoyfilter.assert_called_once() + @patch("charm._remove_envoyfilter") + @patch("charm.KubernetesResourceHandler", return_value=MagicMock()) + @patch("charm.Operator._cni_config_changed", return_value=True) + @patch("charm.Istioctl", return_value=MagicMock()) + def test_reconcile_cni_config_changed( + self, + mocked_istioctl_class, + mocked_cni_config_changed, + mocked_remove_envoyfilter, + mocked_kubernetes_resource_handler_class, + harness, + mocked_lightkube_client, + mocker, + ): + """Tests the upgrade method is called when the CNI config is changed.""" + harness.set_leader(True) + harness.begin() + mocked_upgrade_charm = mocker.patch("charm.Operator.upgrade_charm") + harness.charm.reconcile("event") + mocked_upgrade_charm.assert_called_once() + def test_remove_gateway( self, harness, @@ -1226,7 +1308,23 @@ def test_upgrade_successful( harness.charm.upgrade_charm("mock_event") # Assert that the upgrade was successful - mocked_istioctl_class.assert_called_with("./istioctl", model_name, "minimal") + mocked_istioctl_class.assert_called_with( + "./istioctl", + model_name, + "minimal", + istioctl_extra_flags=[ + "--set", + "values.pilot.image=pilot", + "--set", + "values.global.tag=1.17.3", + "--set", + "values.global.hub=docker.io/istio", + "--set", + "values.global.proxy.image=proxyv2", + "--set", + "values.global.proxy_init.image=proxyv2", + ], + ) mocked_istioctl.upgrade.assert_called_with() mocked_wait_for_update_rollout.assert_called_once()