Merge ~jose-phillips/charm-k8s-ingress:master into charm-k8s-ingress:master

[Jose Phillips]

Allow custom annotations on charms

with this patch you can apply custom annotations for the k8s nginx controller

Closes #1955596
fix bug #1955596

With this patch will allow custom annotations for the nginx controller

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

294711c... by Jose Phillips on 2022-01-02

Removed a hardcoded custom annotation

[Tom Haddon (mthaddon)]

So far we've avoided taking the path of allowing freeform annotations to be added. While allowing freeform changes like this offers more flexibility, it is a little more error prone and means the specific options aren't as well documented, so the charm author needs to know more about the specific options. As an example, you have the following in config.yaml:

'{"nginx.ingress.kubernetes.io/ssl-passthrough": "True", "nginx.ingress.kubernetes.io/proxy-http-version': "1.1"}'

But if each was a separate option it would be exposed as such in `lib/charms/nginx_ingress_integrator/v0/ingress.py`.

Can you provide a bit more background on which options you'd need to use and why?

review: Needs Information

[Jose Phillips (jose-phillips)]

i understand that every option can be more easily for users and developers but Annotations can came from another plugin or requirement.

we start needing some options like Client certificate etc but we also required cert-manager that is a complete annotation out of the scope of the kubernetes ingress documentantion example.
 "cert-manager.io/cluster-issuer" also another internal plugins annotations required for the application that we are developing.

closing this as a normal configuration will lock some developers that they can't use this charm for addons required on the ingress controller.

as the annotations are just key value i don't think can be a issue.

[Tom Haddon (mthaddon)]

Hi Jose,

Sorry for the delay in updates here. I need to discuss this with some other people and should have a chance to do that next week. I'll update here once I have more information.

[Tom Haddon (mthaddon)]

Adding this to the relation is a bit problematic for the reasons mentioned above:
- It's error prone (there's even a typo in the example you include in config.yaml :) ).
- The list of things the relation supports is no longer documented in a straightforward way because you can also do anything you want via this freeform option.
- You need to be careful of collisions between existing options and the custom annotations.
- If you add something to the relation as an explicit option which someone was previously specifying via custom annotations how do you deal with that?

Anything that we add to the relation ties us to supporting that for this charm as well as any other charms that implement this relation.

However, we could add it as config, which would mean your deployment scenario is supported, just not directly via the relation. If you're okay with that, please update the merge proposal to only implement this as config (if so we'll want to check someone isn't specifying something via custom annotations that they can specify via explicit config/relations, and we'll also want to update unit tests to cover this), and I'll re-review.

Unmerged commits

294711c... by Jose Phillips on 2022-01-02

Removed a hardcoded custom annotation

0fff935... by Jose Phillips on 2021-12-28

Allow custom relations on charms

with this patch you can apply custom annotations for the k8s nginx controller

Closes #1955596

diff --git a/.gitignore b/.gitignore
index ca38d73..6d3c0ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,3 +4,4 @@
 .tox
 .coverage
 __pycache__
+build
\ No newline at end of file
diff --git a/config.yaml b/config.yaml
index 9e95591..8110a1d 100644
--- a/config.yaml
+++ b/config.yaml
@@ -94,3 +94,12 @@ options:
     default: ""
     description: The name of the TLS secret to use. Leaving this empty will configure an ingress with TLS disabled.
     type: string
+  custom-annotations:
+    default: ""
+    description: > 
+      JSON dictionary containing the keys and values to set as annotation in the ingress resource.
+      Please see: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/
+      Example:
+      '{"nginx.ingress.kubernetes.io/ssl-passthrough": "True", "nginx.ingress.kubernetes.io/proxy-http-version': "1.1"}'
+    type: string
+    
\ No newline at end of file
diff --git a/lib/charms/nginx_ingress_integrator/v0/ingress.py b/lib/charms/nginx_ingress_integrator/v0/ingress.py
index 1871e20..1eb9ce7 100644
--- a/lib/charms/nginx_ingress_integrator/v0/ingress.py
+++ b/lib/charms/nginx_ingress_integrator/v0/ingress.py
@@ -22,6 +22,7 @@ Import `IngressRequires` in your charm, with two required options:
     - service-namespace
     - session-cookie-max-age
     - tls-secret-name
+    - custom-annotations
 
 See [the config section](https://charmhub.io/nginx-ingress-integrator/configure) for descriptions
 of each, along with the required type.
@@ -87,6 +88,7 @@ OPTIONAL_INGRESS_RELATION_FIELDS = {
     "session-cookie-max-age",
     "tls-secret-name",
     "path-routes",
+    "custom-annotations"
 }
 
 
diff --git a/src/charm.py b/src/charm.py
index 38a5255..2661bc6 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -3,6 +3,7 @@
 # See LICENSE file for licensing details.
 
 import logging
+import json
 
 import kubernetes.client
 
@@ -176,6 +177,13 @@ class NginxIngressCharm(CharmBase):
         """Return the tls-secret-name to use for k8s ingress (if any)."""
         return self._get_config_or_relation_data("tls-secret-name", "")
 
+    @property
+    def _custom_annotations(self):
+        """Return the custom annotations to use for k8s ingress (if any)."""
+
+        return self._get_config_or_relation_data("custom-annotations", "")
+
+
     def k8s_auth(self):
         """Authenticate to kubernetes."""
         if self._authed:
@@ -259,6 +267,10 @@ class NginxIngressCharm(CharmBase):
                 self._service_name.upper()
             )
             annotations["nginx.ingress.kubernetes.io/session-cookie-samesite"] = "Lax"
+        if self._custom_annotations != "":
+            values=json.loads(self._custom_annotations)
+            for key , value in values.items():
+                annotations[key] = value
         if self._tls_secret_name:
             spec.tls = [
                 kubernetes.client.V1IngressTLS(
@@ -417,6 +429,11 @@ class NginxIngressCharm(CharmBase):
                 msg = "Ingress with service IP(s): {}".format(
                     ", ".join(self._report_service_ips())
                 )
+
+            except json.JSONDecodeError as e:
+                logger.error("Error in custom-annotations: " + str(e))
+                self.unit.status = BlockedStatus("Invalid JSON in custom-annotations")
+                return     
             except kubernetes.client.exceptions.ApiException as e:
                 if e.status == 403:
                     logger.error(
@@ -439,6 +456,11 @@ class NginxIngressCharm(CharmBase):
             try:
                 self._remove_ingress()
                 self._remove_service()
+            
+            except json.JSONDecodeError as e:
+                logger.error("Error in custom-annotations: " + str(e))
+                self.unit.status = BlockedStatus("Invalid JSON in custom-annotations")
+                return 
             except kubernetes.client.exceptions.ApiException as e:
                 if e.status == 403:
                     logger.error(
@@ -449,7 +471,7 @@ class NginxIngressCharm(CharmBase):
                         "Insufficient permissions, try: `juju trust {} --scope=cluster`".format(
                             self.app.name
                         )
-                    )
+                    )    
                     return
                 else:
                     raise