refactor: use secret config instead of action for configuring TLS ing…

…ress gateway (#401)

* refactor: use secret config instead of action

The recommended way of handling user secrets is by saving the user secret ID
in a charm configuration, that way the charm can use it to get the contents of the secret.
When the charm is granted with the user secret via the CLI, it allows the charm to observe
secret events, which help handle changes in their contents.
The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because
of the above, this commit does a small refactor to remove the set-tls and unset-tls actions
and instead directly use user secrets.

This commit also renames ssl to tls to be more accurate when referring
to the actual configuration that gets done in the Gateway.

Part of #398

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
@@ -679,17 +676,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 +874,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 +913,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 +922,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 +998,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"

charms/istio-pilot/src/manifests/gateway.yaml.j2

@@ -33,6 +33,6 @@ metadata:
   namespace: {{ namespace }}
 type: kubernetes.io/tls
 data:
-  tls.crt: {{ ssl_crt }}
-  tls.key: {{ ssl_key }}
+  tls.crt: {{ tls_crt }}
+  tls.key: {{ tls_key }}
 {% endif %}