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

@@ -24,3 +24,6 @@ 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.

charms/istio-pilot/src/charm.py

@@ -31,6 +31,7 @@
     Application,
     BlockedStatus,
     MaintenanceStatus,
+    ModelError,
     SecretNotFoundError,
     WaitingStatus,
 )
@@ -124,13 +125,19 @@ def __init__(self, *args):
         # ---- 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.
-        # Save SSL information and reconcile
-        self.framework.observe(self.on.set_tls_action, self.set_tls)
+        # 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)
+        # Remove TLS information and reconcile
+        # NOTE: contrary to what users would think, removing a user secret
+        # via the CLI "juju remove-secret <secret:secret-ID> will not trigger
+        # a secret_remove event.
+        # The way I have found a workaround is to remove the config option
+        # that stores the secret ID, that way the logic for handling a config-changed
+        # runs and updates the bits where the secret contents are used, in our case
+        # the TLS configuration of the Gateway.
+        # self.framework.observe(self.on.secret_remove, self.reconcile)
         # ---- End of the block
 
         # Event handling for managing the Istio control plane
@@ -238,39 +245,6 @@ def install(self, _):
 
         self.unit.status = ActiveStatus()
 
-    # ---- 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
-
     def reconcile(self, event):
         """Reconcile the state of the charm.
 
@@ -627,17 +601,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(
@@ -825,8 +799,8 @@ 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;
@@ -836,21 +810,21 @@ def _ssl_info(self) -> Dict[str, str]:
         # 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)
+            tls_secret = self.model.get_secret(id=self._tls_secret_id, label=TLS_SECRET_LABEL)
             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(
+                "tls-crt": base64.b64encode(self._cert_handler.cert.encode("ascii")).decode(
                     "utf-8"
                 ),
-                "ssl-key": base64.b64encode(self._cert_handler.key.encode("ascii")).decode(
+                "tls-key": base64.b64encode(self._cert_handler.key.encode("ascii")).decode(
                     "utf-8"
                 ),
             }
@@ -869,7 +843,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.",
@@ -878,47 +852,63 @@ 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, label=TLS_SECRET_LABEL)
         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.\n"
+                f"Also make sure the name of the secret is {TLS_SECRET_LABEL}",
+                BlockedStatus,
+            )
+        except ModelError as e:
+            if "ERROR permission denied" in e.args:
+                # Block the unit when there is an ERROR permission denied
+                raise ErrorWithStatus(
+                    "Access to the istio-tls-secret must be granted.",
+                    BlockedStatus,
+                )
+            raise e
 
-        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 ke:
             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 {ke.args} value(s), "
+                "make sure they are passed as contents of the {TLS_SECRET_LABEL} secret."
+            )
+            raise ErrorWithStatus(
+                f"Missing TLS {ke.args} value(s), please add them to the {TLS_SECRET_LABEL} 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
     # 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.
@@ -931,7 +921,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 %}