feat: enable Istio CNI plugin by default (#365)

* feat: enable Istio CNI plugin by default

This commit enables the Istio CNI plugin by default in all new
deployments of istio-pilot provided the required charm configuration is
present; otherwise the control plane remains intact.
It is also ensured that the required configurations are present when
upgrading to future versions so the plugin is correctly installed on
existing control planes (versions <1.17).

Fixes #356
Part of #351

charms/istio-pilot/config.yaml

@@ -4,6 +4,20 @@ options:
     type: string
     description: |
       The domain name to be used by the charm to send a Certificate Signing Request (CSR) to a TLS certificate provider. In the absence of this configuration option, the charm will try to use the ingress gateway service hostname (if configured by a LB) or its IP address.
+  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

charms/istio-pilot/src/charm.py

@@ -108,10 +108,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,
         )
 
@@ -201,20 +202,19 @@ 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"]
         global_hub = image_config["global-hub"]
         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",
@@ -227,12 +227,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:
@@ -281,8 +310,9 @@ def set_tls(self, event) -> None:
     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
@@ -305,6 +335,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()
@@ -373,8 +412,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:
@@ -968,6 +1013,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.

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

@@ -203,6 +203,7 @@ def test_ingress_auth_and_gateway(
         self,
         mocked_remove_gateway,
         harness,
+        mocker,
         mocked_lightkube_client,
         kubernetes_resource_handler_with_client,
     ):
@@ -223,6 +224,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()
 
@@ -277,6 +279,7 @@ def test_ingress_relation(
         self,
         mocked_handle_istio_pilot_relation,
         harness,
+        mocker,
         mocked_lightkube_client,
         kubernetes_resource_handler_with_client,
     ):
@@ -298,6 +301,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()
@@ -347,7 +351,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
@@ -359,6 +363,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()
@@ -379,6 +384,7 @@ def test_gateway_info_relation(
         self,
         mocked_is_gateway_service_up,
         harness,
+        mocker,
         mocked_lightkube_client,
         kubernetes_resource_handler_with_client,
     ):
@@ -393,6 +399,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
@@ -407,13 +414,66 @@ 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."""
 
     def test_reconcile_handling_nonfatal_errors(
         self,
         harness,
+        mocker,
         all_operator_reconcile_handlers_mocked,
         mocked_cert_subject,
     ):
@@ -434,6 +494,7 @@ def test_reconcile_handling_nonfatal_errors(
         )
 
         harness.begin()
+        mocker.patch("charm.Operator.upgrade_charm")
 
         # Act
         harness.charm.reconcile("event")
@@ -1024,6 +1085,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,
@@ -1277,7 +1359,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()