feat cherry-pick setup TLS with secret changes from 1.17 to main

This PR cherry-picks all the work done for enabling TLS ingress gateway using juju secrets.

* docs: add instructions for TLS with secrets (#421)
* refactor: use secret config instead of action for configuring TLS ingress gateway (#401)
* fix: check routes is not empty before popping values (#424)

Fixes #398

charms/istio-pilot/README.md

@@ -11,8 +11,11 @@ juju deploy istio-pilot --trust
 juju deploy istio-gateway --trust --config kind=ingress istio-ingressgateway
 juju relate istio-pilot istio-ingressgateway
 ```
+
 ## Enable TLS ingress gateway for a single host
 
+### TLS certificate providers
+
 This charm provides means to integrate with TLS certificates providers that help with this configuration. The following can be used as TLS certificates providers depending on the use case or security requirements of an organisation:
 
 * For development or non-production environments, the istio-pilot charm can be related to the [self-signed-certificates-operator](https://github.com/canonical/self-signed-certificates-operator/tree/main).
@@ -27,6 +30,37 @@ juju relate istio-pilot:certificates <TLS certificates providers>:certificates
 
 > Please refer to the official documentation for more details about the [TLS ingress gateway for a single host](https://istio.io/latest/docs/tasks/traffic-management/ingress/secure-ingress/#configure-a-tls-ingress-gateway-for-a-single-host).
 
+### TLS cert and key
+
+⚠️ WARNING: This feature has been added due to #380, but will be supported only in 1.17 and 1.22 (and the versions released in between), after that, it will be removed in favour of integrating with a TLS certificates provider.
+
+> NOTE: this feature works with juju 3.4 and above.
+
+This charm allows TLS certificate and key files to configure the TLS ingress gateway.
+
+1. [Create a user secret](https://juju.is/docs/juju/manage-secrets#heading--add-a-secret) to hold the TLS certificate and key values (as strings)
+
+```
+juju add-secret istio-tls-secret tls-crt="$(cat CERT_FILE)" tls-key=$"$(cat KEY_FILE)"
+```
+
+where
+
+* `tls-crt` holds the value of the TLS certificate file as a string
+* `tls-key` holds the value of the TLS key file as a string
+
+2. [Grant `istio-pilot` access](https://juju.is/docs/juju/manage-secrets#heading--grant-access-to-a-secret) to the secret
+
+```
+juju grant-secret istio-tls-secret istio-pilot
+```
+
+3. Pass the secret ID as a configuration
+
+```
+juju config istio-pilot tls-secret-id=secret:<secret ID resulting from step 1>
+```
+
 ## Enable the Istio CNI plugin
 
 This charm provides means to enable the [Istio CNI plugin](https://istio.io/latest/docs/setup/additional-setup/cni/) in the Istio control plane by setting up the following configuration options:

charms/istio-pilot/config.yaml

@@ -38,3 +38,9 @@ options:
     type: string
     default: istio-ingressgateway-workload
     description: Name of the service created by istio-gateway to use as a Gateway
+  tls-secret-id:
+    type: secret
+    description: |
+      A configuration option to store the user secret ID that stores the TLS certificate and key values.
+      The secret ID is the result of adding a secret with the following format
+      juju add-secret istio-tls-secret tls-crt="$(cat CERT_FILE)" tls-key=$"$(cat KEY_FILE)"

charms/istio-pilot/metadata.yaml

@@ -102,4 +102,4 @@ peers:
   peers:
     interface: istio_pilot_peers
 assumes:
-- juju >= 2.9.0
+- juju >= 3.5

charms/istio-pilot/requirements-unit.txt

@@ -68,7 +68,7 @@ lightkube-models==1.26.0.4
     #   lightkube
 markupsafe==2.1.3
     # via jinja2
-ops==2.6.0
+ops==2.13.0
     # via
     #   -r requirements-unit.in
     #   -r requirements.in

charms/istio-pilot/requirements.txt

@@ -52,7 +52,7 @@ lightkube-models==1.26.0.4
     #   lightkube
 markupsafe==2.1.3
     # via jinja2
-ops==2.6.0
+ops==2.13.0
     # via
     #   -r requirements.in
     #   charmed-kubeflow-chisme

charms/istio-pilot/src/charm.py

@@ -30,6 +30,7 @@
     Application,
     BlockedStatus,
     MaintenanceStatus,
+    ModelError,
     SecretNotFoundError,
     WaitingStatus,
 )
@@ -77,7 +78,6 @@
     "{message} Make sure the cluster has no Istio installations already present and "
     "that you have provided the right configuration values."
 )
-TLS_SECRET_LABEL = "istio-tls-secret"
 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 "
@@ -121,16 +121,13 @@ def __init__(self, *args):
         self.framework.observe(self._cert_handler.on.cert_changed, self.reconcile)
 
         # ---- Start of the block
-        # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21.
+        # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.22.
         # ---- For details please refer to canonical/istio-operators#380.
-        # ---- FIXME: Remove this block after releasing 1.21.
-        # Save SSL information and reconcile
-        self.framework.observe(self.on.set_tls_action, self.set_tls)
+        # ---- FIXME: Remove this block after releasing 1.22.
+        # Save TLS information and reconcile
+        self._tls_secret_id = self.config.get("tls-secret-id")
         self.framework.observe(self.on.secret_changed, self.reconcile)
 
-        # Remove SSL information and reconcile
-        self.framework.observe(self.on.unset_tls_action, self.unset_tls)
-        self.framework.observe(self.on.secret_remove, self.reconcile)
         # ---- End of the block
 
         # Event handling for managing the Istio control plane
@@ -202,39 +199,6 @@ def _get_image_config(self):
         image_config = yaml.safe_load(self.model.config[IMAGE_CONFIGURATION])
         return image_config
 
-    # ---- Start of the block
-    # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21.
-    # ---- For details please refer to canonical/istio-operators#380.
-    # ---- FIXME: Remove this block after releasing 1.21.
-    def unset_tls(self, event) -> None:
-        """Remove the secret that saves TLS information and reconcile in case of changes."""
-        try:
-            secret = self.model.get_secret(label=TLS_SECRET_LABEL)
-            secret.remove_all_revisions()
-        except SecretNotFoundError:
-            self.log.info("No secret was removed.")
-        self.reconcile(event)
-
-    def set_tls(self, event) -> None:
-        """Save TLS information in a juju secret and reconcile in case of changes."""
-
-        # Because the action itself has some validation, we are guaranteed that
-        # BOTH of these values are passed as a string with minimum length 1
-        ssl_key = event.params.get("ssl-key", None)
-        ssl_crt = event.params.get("ssl-crt", None)
-
-        content = {"ssl-key": ssl_key, "ssl-crt": ssl_crt}
-
-        # Save the secret with the content that was passed through the action
-        try:
-            secret = self.model.get_secret(label=TLS_SECRET_LABEL)
-            secret.set_content(content)
-        except SecretNotFoundError:
-            self.app.add_secret({"ssl-key": ssl_key, "ssl-crt": ssl_crt}, label=TLS_SECRET_LABEL)
-        self.reconcile(event)
-
-    # ---- End of the block
-
     @property
     def _istioctl_extra_flags(self):
         """Return extra flags to pass to istioctl commands."""
@@ -609,8 +573,8 @@ def _get_ingress_data(self, event) -> dict:
                     f" departing application's name.  We assume that the departing application's"
                     f" is not in routes.keys='{list(routes.keys())}'."
                 )
-            else:
-                routes.pop((event.relation, event.app))
+            elif routes:
+                routes.pop((event.relation, event.app), None)
 
         return routes
 
@@ -679,17 +643,17 @@ def _reconcile_gateway(self):
             "gateway_name": self._gateway_name,
             "namespace": self._gateway_namespace,
             "port": self._gateway_port,
-            "ssl_crt": None,
-            "ssl_key": None,
+            "tls_crt": None,
+            "tls_key": None,
             "secure": False,
         }
 
         # Secure the gateway, if certificates relation is enabled and
         # both the CA cert and key are provided
         if self._use_https():
             self._log_and_set_status(MaintenanceStatus("Setting TLS Ingress"))
-            context["ssl_crt"] = self._ssl_info["ssl-crt"]
-            context["ssl_key"] = self._ssl_info["ssl-key"]
+            context["tls_crt"] = self._tls_info["tls-crt"]
+            context["tls_key"] = self._tls_info["tls-key"]
             context["secure"] = True
 
         krh = KubernetesResourceHandler(
@@ -877,40 +841,35 @@ def _istiod_svc(self):
             return exporter_ip
 
     @property
-    def _ssl_info(self) -> Dict[str, str]:
-        """Return a dictionary with SSL cert and key values.
+    def _tls_info(self) -> Dict[str, str]:
+        """Return a dictionary with TLS cert and key values.
 
         The dictionary is built based on available information, if
         the istio-tls-secret is found, it is prioritised and returned;
         otherwise, the information shared by a TLS certificate provider.
         """
 
-        # FIXME: remove the try/catch block and just return the dictionary that contains
-        # data from the CertHandler after 1.21
-        try:
-            ssl_secret = self.model.get_secret(label=TLS_SECRET_LABEL)
+        # FIXME: remove the if statement and just return the dictionary that contains
+        # data from the CertHandler after 1.22
+        if self._use_https_with_tls_secret():
+            tls_secret = self.model.get_secret(id=self._tls_secret_id)
             return {
-                "ssl-crt": base64.b64encode(
-                    ssl_secret.get_content(refresh=True)["ssl-crt"].encode("ascii")
+                "tls-crt": base64.b64encode(
+                    tls_secret.get_content(refresh=True)["tls-crt"].encode("ascii")
                 ).decode("utf-8"),
-                "ssl-key": base64.b64encode(
-                    ssl_secret.get_content(refresh=True)["ssl-key"].encode("ascii")
+                "tls-key": base64.b64encode(
+                    tls_secret.get_content(refresh=True)["tls-key"].encode("ascii")
                 ).decode("utf-8"),
             }
-        except SecretNotFoundError:
-            return {
-                "ssl-crt": base64.b64encode(self._cert_handler.cert.encode("ascii")).decode(
-                    "utf-8"
-                ),
-                "ssl-key": base64.b64encode(self._cert_handler.key.encode("ascii")).decode(
-                    "utf-8"
-                ),
-            }
+        return {
+            "tls-crt": base64.b64encode(self._cert_handler.cert.encode("ascii")).decode("utf-8"),
+            "tls-key": base64.b64encode(self._cert_handler.key.encode("ascii")).decode("utf-8"),
+        }
 
     # ---- Start of the block
-    # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.21.
+    # ---- WARNING: this feature is not recommended, but is supported in 1.17-1.22.
     # ---- For details please refer to canonical/istio-operators#380.
-    # ---- FIXME: Remove this block after releasing 1.21.
+    # ---- FIXME: Remove this block after releasing 1.22.
     def _use_https(self) -> bool:
         """Return True if only one of the TLS configurations are enabled, False if none are.
 
@@ -921,7 +880,7 @@ def _use_https(self) -> bool:
             self.log.error(
                 "Only one TLS configuration is supported at a time."
                 "Either remove the TLS certificate provider relation,"
-                "or the TLS manual configuration with the remove-tls-secret action."
+                "or the tls-secret-id configuration option."
             )
             raise ErrorWithStatus(
                 "Only one TLS configuration is supported at a time. See logs for details.",
@@ -930,47 +889,70 @@ def _use_https(self) -> bool:
         return self._use_https_with_tls_provider() or self._use_https_with_tls_secret()
 
     def _use_https_with_tls_secret(self) -> bool:
-        """Return True if both SSL key and crt are set with save-tls-secret, False otherwise.
+        """Return True if tls-secret-id has been configured, False otherwise.
 
         Raises:
-            ErrorWithStatus: if one of the values is missing.
+            ErrorWithStatus("tls-secret-id was provided, but the secret could not be found - ...):
+                if a secret ID is passed through the tls-secret-id configuration option, but the
+                secret cannot be found.
+            ErrorWithStatus("Access to the istio-tls-secret must be granted."):
+                if the secret ID is passed, but the access to the secret is not granted.
+            ErrorWithStatus(Missing TLS value(s), please add them to the secret")
+                if any of the TLS values are missing from the secret.
         """
 
-        # Ensure the secret that holds the values exist otherwise fail as this
-        # is an error in our code
+        # Check if the tls-secret-id holds any value and get the secret
+        if not self._tls_secret_id:
+            return False
+
         try:
-            secret = self.model.get_secret(label=TLS_SECRET_LABEL)
+            secret = self.model.get_secret(id=self._tls_secret_id)
         except SecretNotFoundError:
-            return False
+            raise ErrorWithStatus(
+                "tls-secret-id was provided, but the secret could not be found - "
+                "please provide a secret ID of a secret that exists.",
+                BlockedStatus,
+            )
+        # FIXME: right now, juju will raise a ModelError when the application hasn't been
+        # granted access to the secret, but because of https://bugs.launchpad.net/juju/+bug/2067336
+        # this behaviour will change. Once that is done, we must ensure we reflect the change by
+        # removing this exception and add a new one that covers the actual behaviour.
+        # See canonical/istio-operators#420 for more details
+        except ModelError as model_error:
+            if "ERROR permission denied" in model_error.args[0]:
+                # Block the unit when there is an ERROR permission denied
+                raise ErrorWithStatus(
+                    (
+                        f"Permission denied trying to access TLS secret.\n"
+                        f"Access to the secret with id: {self._tls_secret_id} must be granted."
+                        " See juju grant-secret --help for details on granting permission."
+                    ),
+                    BlockedStatus,
+                )
 
-        ssl_key = secret.get_content(refresh=True)["ssl-key"]
-        ssl_crt = secret.get_content(refresh=True)["ssl-crt"]
-
-        # This block of code is more a validation than a behaviour of the charm
-        # Ideally this will never be executed, but we need a mechanism to know that
-        # these values were correctly saved in the secret
-        if _xor(ssl_key, ssl_crt):
-            missing = "ssl-key"
-            if not secret.get_content(refresh=True)["ssl-crt"]:
-                missing = "ssl-crt"
-            self.log.error(f"Missing {missing}, this is most likely an error with the charm.")
-            raise GenericCharmRuntimeError(f"Missing {missing}, cannot configure TLS.")
-        elif not ssl_key and not ssl_crt:
+        try:
+            tls_key = secret.get_content(refresh=True)["tls-key"]
+            tls_crt = secret.get_content(refresh=True)["tls-crt"]
+        except KeyError as err:
             self.log.error(
-                "Missing both SSL key and cert values, this is most likely an error with the charm."
+                f"Cannot configure TLS - Missing TLS {err.args} value(s), "
+                "make sure they are passed as contents of the TLS secret."
+            )
+            raise ErrorWithStatus(
+                f"Missing TLS {err.args} value(s), please add them to the TLS secret",
+                BlockedStatus,
             )
-            raise GenericCharmRuntimeError("Missing SSL values, cannot configure TLS.")
 
-        # If both the SSL key and cert are provided, we configure TLS
-        if ssl_key and ssl_crt:
+        # If both the TLS key and cert are provided, we configure TLS
+        if tls_key and tls_crt:
             return True
 
     # ---- End of the block
 
-    # FIXME: Replace the below line with the one commented out after releasing 1.21
+    # FIXME: Replace the below line with the one commented out after releasing 1.22
     # def _use_https(self) -> bool:
     def _use_https_with_tls_provider(self) -> bool:
-        """Return True if SSL key and cert are provided by a TLS cert provider, False otherwise.
+        """Return True if TLS key and cert are provided by a TLS cert provider, False otherwise.
 
         Raises:
             ErrorWithStatus: if one of the values is missing.
@@ -983,7 +965,7 @@ def _use_https_with_tls_provider(self) -> bool:
         # If the certificates relation is established, we can assume
         # that we want to configure TLS
         if _xor(self._cert_handler.cert, self._cert_handler.key):
-            # Fail if ssl is only partly configured as this is probably a mistake
+            # Fail if tls is only partly configured as this is probably a mistake
             missing = "pkey"
             if not self._cert_handler.cert:
                 missing = "CA cert"