From cda59ccc9c8095119346ff5c948d8a17c100ead9 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 10 Sep 2021 14:14:01 -0300 Subject: [PATCH] Add new flag to watch ingressclass by name instead of spec (#7609) --- .../templates/controller-daemonset.yaml | 3 + .../templates/controller-deployment.yaml | 3 + charts/ingress-nginx/values.yaml | 3 + cmd/nginx/flags.go | 10 +- .../controller/ingressclass/ingressclass.go | 4 + internal/ingress/controller/store/store.go | 7 +- .../ingress/controller/store/store_test.go | 108 +++++++++++++++++- test/e2e/settings/ingress_class.go | 68 +++++++++++ 8 files changed, 201 insertions(+), 5 deletions(-) diff --git a/charts/ingress-nginx/templates/controller-daemonset.yaml b/charts/ingress-nginx/templates/controller-daemonset.yaml index 2e6b32170..68291edd7 100644 --- a/charts/ingress-nginx/templates/controller-daemonset.yaml +++ b/charts/ingress-nginx/templates/controller-daemonset.yaml @@ -114,6 +114,9 @@ spec: {{- if .Values.controller.healthCheckHost }} - --healthz-host={{ .Values.controller.healthCheckHost }} {{- end }} + {{- if .Values.controller.ingressClassByName }} + - --ingress-class-by-name=true + {{- end }} {{- if .Values.controller.watchIngressWithoutClass }} - --watch-ingress-without-class=true {{- end }} diff --git a/charts/ingress-nginx/templates/controller-deployment.yaml b/charts/ingress-nginx/templates/controller-deployment.yaml index 681955f6f..24714a523 100644 --- a/charts/ingress-nginx/templates/controller-deployment.yaml +++ b/charts/ingress-nginx/templates/controller-deployment.yaml @@ -115,6 +115,9 @@ spec: {{- if not (eq .Values.controller.healthCheckPath "/healthz") }} - --health-check-path={{ .Values.controller.healthCheckPath }} {{- end }} + {{- if .Values.controller.ingressClassByName }} + - --ingress-class-by-name=true + {{- end }} {{- if .Values.controller.watchIngressWithoutClass }} - --watch-ingress-without-class=true {{- end }} diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index 8ef8ea8e7..dff440313 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -66,6 +66,9 @@ controller: # Defaults to false watchIngressWithoutClass: false + # Process IngressClass per name (additionally as per spec.controller) + ingressClassByName: false + # Required for use with CNI based kubernetes installations (such as ones set up by kubeadm), # since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920 # is merged diff --git a/cmd/nginx/flags.go b/cmd/nginx/flags.go index c12dc7399..42c14dd51 100644 --- a/cmd/nginx/flags.go +++ b/cmd/nginx/flags.go @@ -68,6 +68,9 @@ referenced in an Ingress Object should be the same value specified here to make watchWithoutClass = flags.Bool("watch-ingress-without-class", false, `Define if Ingress Controller should also watch for Ingresses without an IngressClass or the annotation specified`) + ingressClassByName = flags.Bool("ingress-class-by-name", false, + `Define if Ingress Controller should watch for Ingress Class by Name together with Controller Class`) + configMap = flags.String("configmap", "", `Name of the ConfigMap containing custom global configurations for the controller.`) @@ -299,9 +302,10 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g SSLProxy: *sslProxyPort, }, IngressClassConfiguration: &ingressclass.IngressClassConfiguration{ - Controller: *ingressClassController, - AnnotationValue: *ingressClassAnnotation, - WatchWithoutClass: *watchWithoutClass, + Controller: *ingressClassController, + AnnotationValue: *ingressClassAnnotation, + WatchWithoutClass: *watchWithoutClass, + IngressClassByName: *ingressClassByName, }, DisableCatchAll: *disableCatchAll, ValidationWebhook: *validationWebhook, diff --git a/internal/ingress/controller/ingressclass/ingressclass.go b/internal/ingress/controller/ingressclass/ingressclass.go index 025a4e2a5..f13a2a05c 100644 --- a/internal/ingress/controller/ingressclass/ingressclass.go +++ b/internal/ingress/controller/ingressclass/ingressclass.go @@ -42,4 +42,8 @@ type IngressClassConfiguration struct { // WatchWithoutClass defines if Controller should watch to Ingress Objects that does // not contain an IngressClass configuration WatchWithoutClass bool + + //IngressClassByName defines if the Controller should watch for Ingress Classes by + // .metadata.name together with .spec.Controller + IngressClassByName bool } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index dd18594e9..d5f7ce63c 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -426,7 +426,12 @@ func New( ingressClassEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { ingressclass := obj.(*networkingv1.IngressClass) - if ingressclass.Spec.Controller != icConfig.Controller { + foundClassByName := false + if icConfig.IngressClassByName && ingressclass.Name == icConfig.AnnotationValue { + klog.InfoS("adding ingressclass as ingress-class-by-name is configured", "ingressclass", klog.KObj(ingressclass)) + foundClassByName = true + } + if !foundClassByName && ingressclass.Spec.Controller != icConfig.Controller { klog.InfoS("ignoring ingressclass as the spec.controller is not the same of this ingress", "ingressclass", klog.KObj(ingressclass)) return } diff --git a/internal/ingress/controller/store/store_test.go b/internal/ingress/controller/store/store_test.go index 35e5955b1..9004094a3 100644 --- a/internal/ingress/controller/store/store_test.go +++ b/internal/ingress/controller/store/store_test.go @@ -316,7 +316,7 @@ func TestStore(t *testing.T) { err := framework.WaitForIngressInNamespace(clientSet, ns, ing.Name) if err != nil { - t.Errorf("error waiting for secret: %v", err) + t.Errorf("error waiting for ingress: %v", err) } time.Sleep(1 * time.Second) @@ -486,6 +486,112 @@ func TestStore(t *testing.T) { } }) + t.Run("should return two events for add and delete and one for update of ingress and watch-ingress-by-name", func(t *testing.T) { + ns := createNamespace(clientSet, t) + defer deleteNamespace(ns, clientSet, t) + ic := createIngressClass(clientSet, t, "not-k8s.io/by-name") + defer deleteIngressClass(ic, clientSet, t) + + createConfigMap(clientSet, ns, t) + + stopCh := make(chan struct{}) + updateCh := channels.NewRingChannel(1024) + + var add uint64 + var upd uint64 + var del uint64 + + go func(ch *channels.RingChannel) { + for { + evt, ok := <-ch.Out() + if !ok { + return + } + + e := evt.(Event) + if e.Obj == nil { + continue + } + if _, ok := e.Obj.(*networking.Ingress); !ok { + continue + } + + switch e.Type { + case CreateEvent: + atomic.AddUint64(&add, 1) + case UpdateEvent: + atomic.AddUint64(&upd, 1) + case DeleteEvent: + atomic.AddUint64(&del, 1) + } + } + }(updateCh) + + ingressClassconfig := &ingressclass.IngressClassConfiguration{ + Controller: ingressclass.DefaultControllerName, + AnnotationValue: ic, + IngressClassByName: true, + } + + storer := New( + ns, + fmt.Sprintf("%v/config", ns), + fmt.Sprintf("%v/tcp", ns), + fmt.Sprintf("%v/udp", ns), + "", + 10*time.Minute, + clientSet, + updateCh, + false, + ingressClassconfig) + + storer.Run(stopCh) + validSpec := commonIngressSpec + validSpec.IngressClassName = &ic + ing := ensureIngress(&networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingclass-by-name", + Namespace: ns, + }, + Spec: validSpec, + }, clientSet, t) + + err := framework.WaitForIngressInNamespace(clientSet, ns, ing.Name) + if err != nil { + t.Errorf("error waiting for ingress: %v", err) + } + time.Sleep(1 * time.Second) + + ingressUpdated := ing.DeepCopy() + ingressUpdated.Spec.Rules[0].Host = "update-dummy" + _ = ensureIngress(ingressUpdated, clientSet, t) + if err != nil { + t.Errorf("error updating ingress: %v", err) + } + // Secret takes a bit to update + time.Sleep(3 * time.Second) + + err = clientSet.NetworkingV1().Ingresses(ingressUpdated.Namespace).Delete(context.TODO(), ingressUpdated.Name, metav1.DeleteOptions{}) + if err != nil { + t.Errorf("error deleting ingress: %v", err) + } + + err = framework.WaitForNoIngressInNamespace(clientSet, ingressUpdated.Namespace, ingressUpdated.Name) + if err != nil { + t.Errorf("error waiting for ingress deletion: %v", err) + } + + if atomic.LoadUint64(&add) != 1 { + t.Errorf("expected 1 event of type Create but %v occurred", add) + } + if atomic.LoadUint64(&upd) != 1 { + t.Errorf("expected 1 event of type Update but %v occurred", upd) + } + if atomic.LoadUint64(&del) != 1 { + t.Errorf("expected 1 event of type Delete but %v occurred", del) + } + }) + t.Run("should not receive updates for ingress with invalid class annotation", func(t *testing.T) { ns := createNamespace(clientSet, t) defer deleteNamespace(ns, clientSet, t) diff --git a/test/e2e/settings/ingress_class.go b/test/e2e/settings/ingress_class.go index 3ba42f311..9740eef38 100644 --- a/test/e2e/settings/ingress_class.go +++ b/test/e2e/settings/ingress_class.go @@ -510,4 +510,72 @@ var _ = framework.IngressNginxDescribe("[Flag] ingress-class", func() { }) }) + + ginkgo.Context("With ingress-class-by-name flag", func() { + ginkgo.BeforeEach(func() { + err := f.UpdateIngressControllerDeployment(func(deployment *appsv1.Deployment) error { + args := []string{} + for _, v := range deployment.Spec.Template.Spec.Containers[0].Args { + if strings.Contains(v, "--ingress-class-by-name") && + strings.Contains(v, "--ingress-class=test-new-ingress-class") { + continue + } + + args = append(args, v) + } + args = append(args, "--ingress-class=test-new-ingress-class") + args = append(args, "--ingress-class-by-name") + deployment.Spec.Template.Spec.Containers[0].Args = args + _, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{}) + + return err + }) + assert.Nil(ginkgo.GinkgoT(), err, "updating ingress controller deployment flags") + }) + + ginkgo.It("should watch Ingress that uses the class name even if spec is different", func() { + validHostClassName := "validhostclassname" + + ing := framework.NewSingleIngress(validHostClassName, "/", validHostClassName, f.Namespace, framework.EchoService, 80, nil) + ing.Spec.IngressClassName = &otherIngressClassName + f.EnsureIngress(ing) + + validHostClass := "validhostclassspec" + ing = framework.NewSingleIngress(validHostClass, "/", validHostClass, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + invalidHost := "invalidannotation" + annotations := map[string]string{ + ingressclass.IngressKey: "testclass123", + } + ing = framework.NewSingleIngress(invalidHost, "/", invalidHost, f.Namespace, framework.EchoService, 80, annotations) + ing.Spec.IngressClassName = nil + f.EnsureIngress(ing) + + f.WaitForNginxConfiguration(func(cfg string) bool { + return strings.Contains(cfg, "server_name validhostclassname") && + strings.Contains(cfg, "server_name validhostclassspec") && + !strings.Contains(cfg, "server_name invalidannotation") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", validHostClass). + Expect(). + Status(http.StatusOK) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", validHostClassName). + Expect(). + Status(http.StatusOK) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", invalidHost). + Expect(). + Status(http.StatusNotFound) + }) + + }) })