Merge ~pjdc/charm-k8s-ingress/+git/charm-k8s-ingress:automatic-ingress-class into charm-k8s-ingress:master

[Paul Collins]

when ingress-class is empty, look up the cluster's default class

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

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

[Paul Collins (pjdc)]

Since microk8s changed its default ingress name to "public", and the nginx controller hard-codes its default to "nginx", something like this makes for a more pleasant experience when getting started with this charm.

 * https://github.com/ubuntu/microk8s/issues/2035
 * https://kubernetes.github.io/ingress-nginx/user-guide/multiple-ingress/#multiple-ingress-nginx-controllers

[Jon Seager (jnsgruk)]

Looks good, a couple of minor nits that are in no way blocking. Thanks! :)

review: Approve

[Tom Haddon (mthaddon)]

This is a really nice change, but a few comments inline about some changes that would be good to include.

[Paul Collins (pjdc)]

Addressed all, I believe! Please take a look.

review: Needs Resubmitting

[Tom Haddon (mthaddon)]

Looks great, thanks very much!

review: Approve

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

Change successfully merged at revision 0590bdb95c2c91f838c820c1c4063204cbfebef3

diff --git a/config.yaml b/config.yaml
index edaed4c..e15b8ea 100644
--- a/config.yaml
+++ b/config.yaml
@@ -4,12 +4,25 @@ options:
   ingress-class:
     default: ""
     description: >
-      The ingress class annotation to target for this ingress resource. If your
-      kubernetes cluster has multiple ingress controllers, this allows you to
-      target the correct one. See https://kubernetes.github.io/ingress-nginx/user-guide/multiple-ingress/
-      for more details. Leaving empty will mean the "kubernetes.io/ingress.class"
-      annotation is unset. This value isn't available to be set via the relation
-      as it's a deploy-time configuration item.
+      The ingress class to target for this ingress resource.
+
+      If your cluster has multiple ingress controllers, this allows
+      you to select the correct one, by setting the ingressClassName
+      field on the ingress resource created by the charm.
+
+      This value isn't available to be set via the relation as it's a
+      property of the cluster's configuration.
+
+      If this value is empty, the charm will use whichever ingress class has the
+      "ingressclass.kubernetes.io/is-default-class" annotation set to "true".
+
+      If multiple ingress classes are so configured, no selection will be made.
+
+      For more details, see:
+
+       * https://kubernetes.github.io/ingress-nginx/user-guide/multiple-ingress/
+
+       * https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class
     type: string
   limit-rps:
     default: 0
diff --git a/requirements.txt b/requirements.txt
index cd914f9..05a91aa 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,4 @@
-kubernetes
+# IngressClass resources are not supported by the current
+# release, so select the latest known pre-release version.
+kubernetes == 18.17.0a1
 ops
diff --git a/src/charm.py b/src/charm.py
index c1a9481..72cb7d2 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -247,10 +247,6 @@ class NginxIngressCharm(CharmBase):
         else:
             annotations["nginx.ingress.kubernetes.io/ssl-redirect"] = "false"
 
-        # Run-time configuration options.
-        if self.config["ingress-class"]:
-            annotations["kubernetes.io/ingress.class"] = self.config["ingress-class"]
-
         return kubernetes.client.NetworkingV1beta1Ingress(
             api_version="networking.k8s.io/v1beta1",
             kind="Ingress",
@@ -298,11 +294,41 @@ class NginxIngressCharm(CharmBase):
                 self._service_name,
             )
 
+    def _look_up_and_set_ingress_class(self, api, body):
+        """Set the configured ingress class, otherwise the cluster's default ingress class."""
+        ingress_class = self.config['ingress-class']
+        if not ingress_class:
+            defaults = [
+                item.metadata.name
+                for item in api.list_ingress_class().items
+                if item.metadata.annotations.get('ingressclass.kubernetes.io/is-default-class')
+                == 'true'
+            ]
+
+            if not defaults:
+                logger.warning("Cluster has no default ingress class defined")
+                return
+
+            if len(defaults) > 1:
+                logger.warning(
+                    "Multiple default ingress classes defined, declining to choose between them. "
+                    "They are: {}".format(' '.join(sorted(defaults)))
+                )
+                return
+
+            ingress_class = defaults[0]
+            logger.info(
+                "Using ingress class {} as it is the cluster's default".format(ingress_class)
+            )
+
+        body.spec.ingress_class_name = ingress_class
+
     def _define_ingress(self):
         """Create or update an ingress in kubernetes."""
         self.k8s_auth()
         api = _networking_v1_beta1_api()
         body = self._get_k8s_ingress()
+        self._look_up_and_set_ingress_class(api, body)
         ingresses = api.list_namespaced_ingress(namespace=self._namespace)
         if self._ingress_name in [x.metadata.name for x in ingresses.items]:
             api.replace_namespaced_ingress(
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index d5da7b4..2d9b816 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -3,7 +3,7 @@
 
 import unittest
 
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 import kubernetes
 
@@ -359,7 +359,6 @@ class TestCharm(unittest.TestCase):
             metadata=kubernetes.client.V1ObjectMeta(
                 name="gunicorn-ingress",
                 annotations={
-                    "kubernetes.io/ingress.class": "nginx",
                     "nginx.ingress.kubernetes.io/affinity": "cookie",
                     "nginx.ingress.kubernetes.io/affinity-mode": "balanced",
                     "nginx.ingress.kubernetes.io/proxy-body-size": "20m",
@@ -460,3 +459,105 @@ class TestCharm(unittest.TestCase):
             ),
         )
         self.assertEqual(self.harness.charm._get_k8s_service(), expected)
+
+
+INGRESS_CLASS_PUBLIC_DEFAULT = kubernetes.client.V1beta1IngressClass(
+    metadata=kubernetes.client.V1ObjectMeta(
+        annotations={
+            'ingressclass.kubernetes.io/is-default-class': 'true',
+        },
+        name='public',
+    ),
+    spec=kubernetes.client.V1beta1IngressClassSpec(
+        controller='k8s.io/ingress-nginx',
+    ),
+)
+
+INGRESS_CLASS_PRIVATE = kubernetes.client.V1beta1IngressClass(
+    metadata=kubernetes.client.V1ObjectMeta(
+        annotations={},
+        name='private',
+    ),
+    spec=kubernetes.client.V1beta1IngressClassSpec(
+        controller='k8s.io/ingress-nginx',
+    ),
+)
+
+INGRESS_CLASS_PRIVATE_DEFAULT = kubernetes.client.V1beta1IngressClass(
+    metadata=kubernetes.client.V1ObjectMeta(
+        annotations={
+            'ingressclass.kubernetes.io/is-default-class': 'true',
+        },
+        name='private',
+    ),
+    spec=kubernetes.client.V1beta1IngressClassSpec(
+        controller='k8s.io/ingress-nginx',
+    ),
+)
+
+ZERO_INGRESS_CLASS_LIST = kubernetes.client.V1beta1IngressClassList(items=[])
+
+ONE_INGRESS_CLASS_LIST = kubernetes.client.V1beta1IngressClassList(
+    items=[
+        INGRESS_CLASS_PUBLIC_DEFAULT,
+    ],
+)
+
+TWO_INGRESS_CLASSES_LIST = kubernetes.client.V1beta1IngressClassList(
+    items=[
+        INGRESS_CLASS_PUBLIC_DEFAULT,
+        INGRESS_CLASS_PRIVATE,
+    ]
+)
+
+TWO_INGRESS_CLASSES_LIST_TWO_DEFAULT = kubernetes.client.V1beta1IngressClassList(
+    items=[
+        INGRESS_CLASS_PUBLIC_DEFAULT,
+        INGRESS_CLASS_PRIVATE_DEFAULT,
+    ]
+)
+
+
+def _make_mock_api_list_ingress_class(return_value):
+    mock_api = MagicMock()
+    mock_api.list_ingress_class.return_value = return_value
+    return mock_api
+
+
+class TestCharmLookUpAndSetIngressClass(unittest.TestCase):
+    def setUp(self):
+        self.harness = Harness(NginxIngressCharm)
+        self.addCleanup(self.harness.cleanup)
+        self.harness.begin()
+        self.harness.disable_hooks()
+        self.harness.update_config(
+            {"service-hostname": "foo.internal", "service-name": "gunicorn", "service-port": 80}
+        )
+
+    def test_zero_ingress_class(self):
+        """If there are no ingress classes, there's nothing to choose from."""
+        api = _make_mock_api_list_ingress_class(ZERO_INGRESS_CLASS_LIST)
+        body = self.harness.charm._get_k8s_ingress()
+        self.harness.charm._look_up_and_set_ingress_class(api, body)
+        self.assertIsNone(body.spec.ingress_class_name)
+
+    def test_one_ingress_class(self):
+        """If there's one default ingress class, choose that."""
+        api = _make_mock_api_list_ingress_class(ONE_INGRESS_CLASS_LIST)
+        body = self.harness.charm._get_k8s_ingress()
+        self.harness.charm._look_up_and_set_ingress_class(api, body)
+        self.assertEqual(body.spec.ingress_class_name, 'public')
+
+    def test_two_ingress_classes(self):
+        """If there are two ingress classes, one default, choose that."""
+        api = _make_mock_api_list_ingress_class(TWO_INGRESS_CLASSES_LIST)
+        body = self.harness.charm._get_k8s_ingress()
+        self.harness.charm._look_up_and_set_ingress_class(api, body)
+        self.assertEqual(body.spec.ingress_class_name, 'public')
+
+    def test_two_ingress_classes_two_default(self):
+        """If there are two ingress classes, both default, choose neither."""
+        api = _make_mock_api_list_ingress_class(TWO_INGRESS_CLASSES_LIST_TWO_DEFAULT)
+        body = self.harness.charm._get_k8s_ingress()
+        self.harness.charm._look_up_and_set_ingress_class(api, body)
+        self.assertIsNone(body.spec.ingress_class_name)