Merge ~twom/charm-k8s-ingress:allow-disabling-rewrite into charm-k8s-ingress:master

[Tom Wardill]

Allow disabling and setting the rewrite target

Add a setting for disabling the rewrite annotation, and another to allow the target to be overridden.

Default to 'on' and '/'.

Some complications around handling the boolean value which there might be a better way of doing, but I'm not sure about the data types we can guarantee.

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

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

[Tom Wardill (twom)]

I'm unsure why black has decided it wants to suddenly change a bunch of the line wrapping in this, I can unpick it if it's preferred.

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

Change successfully merged at revision 1562894c58139f79caa7b83b606496e77058bc94

diff --git a/config.yaml b/config.yaml
index 087d353..f94797d 100644
--- a/config.yaml
+++ b/config.yaml
@@ -57,6 +57,14 @@ options:
       for more details. Unrecognised values will be ignored. The nginx default
       will be used if this config option is set to an empty value.
     type: string
+  rewrite-enabled:
+    default: ""
+    description: Whether requests should be written to the `rewrite-target`
+    type: boolean
+  rewrite-target:
+    default: "/"
+    description: The path to rewrite requests to.
+    type: string
   service-hostname:
     default: ""
     description: The hostname of the service to create an ingress for.
diff --git a/lib/charms/nginx_ingress_integrator/v0/ingress.py b/lib/charms/nginx_ingress_integrator/v0/ingress.py
index 780458e..6eb60ba 100644
--- a/lib/charms/nginx_ingress_integrator/v0/ingress.py
+++ b/lib/charms/nginx_ingress_integrator/v0/ingress.py
@@ -16,6 +16,8 @@ Import `IngressRequires` in your charm, with two required options:
     - max-body-size
     - path-routes
     - retry-errors
+    - rewrite-enabled
+    - rewrite-target
     - service-namespace
     - session-cookie-max-age
     - tls-secret-name
@@ -77,6 +79,8 @@ OPTIONAL_INGRESS_RELATION_FIELDS = {
     "limit-whitelist",
     "max-body-size",
     "retry-errors",
+    "rewrite-target",
+    "rewrite-enabled",
     "service-namespace",
     "session-cookie-max-age",
     "tls-secret-name",
diff --git a/src/charm.py b/src/charm.py
index 4d0d235..5882e04 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -19,6 +19,8 @@ from ops.model import ActiveStatus, BlockedStatus
 
 logger = logging.getLogger(__name__)
 
+BOOLEAN_CONFIG_FIELDS = ["rewrite-enabled"]
+
 
 def _core_v1_api():
     """Use the v1 k8s API."""
@@ -69,6 +71,9 @@ class NginxIngressCharm(CharmBase):
     def _get_config_or_relation_data(self, field, fallback):
         """Helper method to get data from config or the ingress relation."""
         config_data = self.config[field]
+        # A value of False is valid in these fields, so check it's not a null-value instead
+        if field in BOOLEAN_CONFIG_FIELDS and (config_data is not None and config_data != ""):
+            return config_data
         if config_data:
             return config_data
         relation = self.model.get_relation("ingress")
@@ -118,6 +123,19 @@ class NginxIngressCharm(CharmBase):
         return ""
 
     @property
+    def _rewrite_enabled(self):
+        """Return whether rewriting should be enabled from config or relation"""
+        value = self._get_config_or_relation_data("rewrite-enabled", True)
+        # config data is typed, relation data is a string
+        # Convert to string, then compare to a known value.
+        return str(value).lower() == "true"
+
+    @property
+    def _rewrite_target(self):
+        """Return the rewrite target from config or relation."""
+        return self._get_config_or_relation_data("rewrite-target", "/")
+
+    @property
     def _namespace(self):
         """Return the namespace to operate on."""
         return self._get_config_or_relation_data("service-namespace", self.model.name)
@@ -228,9 +246,9 @@ class NginxIngressCharm(CharmBase):
                 )
             ]
         )
-        annotations = {
-            "nginx.ingress.kubernetes.io/rewrite-target": "/",
-        }
+        annotations = {}
+        if self._rewrite_enabled:
+            annotations["nginx.ingress.kubernetes.io/rewrite-target"] = self._rewrite_target
         if self._limit_rps:
             annotations["nginx.ingress.kubernetes.io/limit-rps"] = self._limit_rps
             if self._limit_whitelist:
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index ecc9ea1..193bc7b 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -279,6 +279,55 @@ class TestCharm(unittest.TestCase):
         # Now it's the value from the relation.
         self.assertEqual(self.harness.charm._tls_secret_name, "gunicorn-tls-new")
 
+    def test_rewrite_enabled_property(self):
+        """Test for enabling request rewrites."""
+        # First set via config.
+        self.harness.update_config({"rewrite-enabled": True})
+        self.assertEqual(self.harness.charm._rewrite_enabled, True)
+        relation_id = self.harness.add_relation("ingress", "gunicorn")
+        self.harness.add_relation_unit(relation_id, "gunicorn/0")
+        relations_data = {
+            "rewrite-enabled": "False",
+            "service-name": "gunicorn",
+            "service-hostname": "foo.internal",
+        }
+        self.harness.update_relation_data(relation_id, "gunicorn", relations_data)
+        # Still /test-target because it's set via config.
+        self.assertEqual(self.harness.charm._rewrite_enabled, True)
+        self.harness.update_config({"rewrite-enabled": ""})
+        self.assertEqual(self.harness.charm._rewrite_enabled, False)
+
+    def test_rewrite_annotations(self):
+        self.harness.disable_hooks()
+        self.harness.update_config(
+            {
+                "service-hostname": "foo.internal",
+                "service-name": "gunicorn",
+                "service-port": 80,
+            }
+        )
+        result_dict = self.harness.charm._get_k8s_ingress().to_dict()
+        expected = {
+            "nginx.ingress.kubernetes.io/rewrite-target": "/",
+            "nginx.ingress.kubernetes.io/ssl-redirect": "false",
+        }
+        self.assertEqual(result_dict["metadata"]["annotations"], expected)
+
+        self.harness.update_config({"rewrite-enabled": False})
+        result_dict = self.harness.charm._get_k8s_ingress().to_dict()
+        expected = {"nginx.ingress.kubernetes.io/ssl-redirect": "false"}
+        self.assertEqual(result_dict["metadata"]["annotations"], expected)
+
+        self.harness.update_config({"rewrite-target": "/test-target"})
+        self.harness.update_config({"rewrite-enabled": True})
+
+        expected = {
+            "nginx.ingress.kubernetes.io/rewrite-target": "/test-target",
+            "nginx.ingress.kubernetes.io/ssl-redirect": "false",
+        }
+        result_dict = self.harness.charm._get_k8s_ingress().to_dict()
+        self.assertEqual(result_dict["metadata"]["annotations"], expected)
+
     @patch('charm.NginxIngressCharm._on_config_changed')
     def test_on_ingress_relation_changed(self, _on_config_changed):
         """Test ingress relation changed handler."""