From cc4d5f22833ab332941cc545ed63e5849b80087c Mon Sep 17 00:00:00 2001 From: Mahnoor Mehboob Date: Wed, 21 Apr 2021 17:49:58 -0400 Subject: [PATCH 1/4] update catch-all ingress requirement logic --- internal/ingress/controller/store/store.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index ad3aef5ad..98a36842f 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -328,7 +328,7 @@ func New( return } - if isCatchAllIngress(ing.Spec) && disableCatchAll { + if hasCatchAllIngress(ing.Spec) && disableCatchAll { klog.InfoS("Ignoring delete for catch-all because of --disable-catch-all", "ingress", klog.KObj(ing)) return } @@ -353,7 +353,7 @@ func New( return } - if isCatchAllIngress(ing.Spec) && disableCatchAll { + if hasCatchAllIngress(ing.Spec) && disableCatchAll { klog.InfoS("Ignoring add for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(ing)) return } @@ -377,7 +377,7 @@ func New( validOld := class.IsValid(oldIng) validCur := class.IsValid(curIng) if !validOld && validCur { - if isCatchAllIngress(curIng.Spec) && disableCatchAll { + if hasCatchAllIngress(curIng.Spec) && disableCatchAll { klog.InfoS("ignoring update for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(curIng)) return } @@ -389,7 +389,7 @@ func New( ingDeleteHandler(old) return } else if validCur && !reflect.DeepEqual(old, cur) { - if isCatchAllIngress(curIng.Spec) && disableCatchAll { + if hasCatchAllIngress(curIng.Spec) && disableCatchAll { klog.InfoS("ignoring update for catch-all ingress and delete old one because of --disable-catch-all", "ingress", klog.KObj(curIng)) ingDeleteHandler(old) return @@ -624,10 +624,10 @@ func New( return store } -// isCatchAllIngress returns whether or not an ingress produces a +// hasCatchAllIngress returns whether or not an ingress produces a // catch-all server, and so should be ignored when --disable-catch-all is set -func isCatchAllIngress(spec networkingv1beta1.IngressSpec) bool { - return spec.Backend != nil && len(spec.Rules) == 0 +func hasCatchAllIngress(spec networkingv1beta1.IngressSpec) bool { + return spec.Backend != nil } // syncIngress parses ingress annotations converting the value of the From 2503b23b0933fa5294cb0d7037f140530dbfc893 Mon Sep 17 00:00:00 2001 From: Mahnoor Mehboob Date: Thu, 22 Apr 2021 12:01:41 -0400 Subject: [PATCH 2/4] Alter e2e test for disable_catch_all.go --- internal/ingress/controller/store/store.go | 12 ++++++------ test/e2e/settings/disable_catch_all.go | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 98a36842f..0088c3b01 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -328,7 +328,7 @@ func New( return } - if hasCatchAllIngress(ing.Spec) && disableCatchAll { + if hasCatchAllIngressRule(ing.Spec) && disableCatchAll { klog.InfoS("Ignoring delete for catch-all because of --disable-catch-all", "ingress", klog.KObj(ing)) return } @@ -353,7 +353,7 @@ func New( return } - if hasCatchAllIngress(ing.Spec) && disableCatchAll { + if hasCatchAllIngressRule(ing.Spec) && disableCatchAll { klog.InfoS("Ignoring add for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(ing)) return } @@ -377,7 +377,7 @@ func New( validOld := class.IsValid(oldIng) validCur := class.IsValid(curIng) if !validOld && validCur { - if hasCatchAllIngress(curIng.Spec) && disableCatchAll { + if hasCatchAllIngressRule(curIng.Spec) && disableCatchAll { klog.InfoS("ignoring update for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(curIng)) return } @@ -389,7 +389,7 @@ func New( ingDeleteHandler(old) return } else if validCur && !reflect.DeepEqual(old, cur) { - if hasCatchAllIngress(curIng.Spec) && disableCatchAll { + if hasCatchAllIngressRule(curIng.Spec) && disableCatchAll { klog.InfoS("ignoring update for catch-all ingress and delete old one because of --disable-catch-all", "ingress", klog.KObj(curIng)) ingDeleteHandler(old) return @@ -624,9 +624,9 @@ func New( return store } -// hasCatchAllIngress returns whether or not an ingress produces a +// hasCatchAllIngressRule returns whether or not an ingress produces a // catch-all server, and so should be ignored when --disable-catch-all is set -func hasCatchAllIngress(spec networkingv1beta1.IngressSpec) bool { +func hasCatchAllIngressRule(spec networkingv1beta1.IngressSpec) bool { return spec.Backend != nil } diff --git a/test/e2e/settings/disable_catch_all.go b/test/e2e/settings/disable_catch_all.go index 71374ee39..1a8791d80 100644 --- a/test/e2e/settings/disable_catch_all.go +++ b/test/e2e/settings/disable_catch_all.go @@ -48,7 +48,7 @@ var _ = framework.IngressNginxDescribe("[Flag] disable-catch-all", func() { assert.Nil(ginkgo.GinkgoT(), err, "updating ingress controller deployment flags") }) - ginkgo.It("should ignore catch all Ingress", func() { + ginkgo.It("should ignore catch all Ingress with backend", func() { host := "foo" ing := framework.NewSingleCatchAllIngress("catch-all", f.Namespace, framework.EchoService, 80, nil) @@ -67,6 +67,18 @@ var _ = framework.IngressNginxDescribe("[Flag] disable-catch-all", func() { }) }) + ginkgo.It("should ignore catch all Ingress with backend and rules", func() { + host := "foo" + + ing := framework.NewSingleIngressWithBackendAndRules(host, "/", host, f.Namespace, framework.EchoService, 80, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxServer("_", func(cfg string) bool { + return strings.Contains(cfg, `set $ingress_name ""`) && + strings.Contains(cfg, `set $proxy_upstream_name "upstream-default-backend"`) + }) + }) + ginkgo.It("should delete Ingress updated to catch-all", func() { host := "foo" @@ -105,10 +117,10 @@ var _ = framework.IngressNginxDescribe("[Flag] disable-catch-all", func() { Status(http.StatusNotFound) }) - ginkgo.It("should allow Ingress with both a default backend and rules", func() { + ginkgo.It("should allow Ingress with rules", func() { host := "foo" - ing := framework.NewSingleIngressWithBackendAndRules("not-catch-all", "/rulepath", host, f.Namespace, framework.EchoService, 80, framework.EchoService, 80, nil) + ing := framework.NewSingleIngress("not-catch-all", "/", host, f.Namespace, framework.EchoService, 80, nil) f.EnsureIngress(ing) f.WaitForNginxServer(host, func(cfg string) bool { From 8f7fecab176970266f76b86ec9d328bbadfe56cb Mon Sep 17 00:00:00 2001 From: Mahnoor Mehboob Date: Sat, 24 Apr 2021 11:49:45 -0400 Subject: [PATCH 3/4] Deny catch-all ingress when DisableCatchAll is set --- internal/ingress/controller/controller.go | 4 ++++ .../ingress/controller/controller_test.go | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index a29b0b14b..3f9aab8ee 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -224,6 +224,10 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { return nil } + if n.cfg.DisableCatchAll && ing.Spec.Backend != nil { + return fmt.Errorf("This deployment is trying to create a catch-all ingress while DisableCatchAll flag is set to true. Remove '.spec.backend' or set DisableCatchAll flag to false.") + } + if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix { for key := range ing.ObjectMeta.GetAnnotations() { if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) { diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index 1542c7abb..c5b29ba8c 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -256,6 +256,28 @@ func TestCheckIngress(t *testing.T) { } }) + t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) { + nginx.command = testNginxTestCommand{ + t: t, + err: nil, + } + nginx.cfg.DisableCatchAll = true + + ing.Spec.Backend = &networking.IngressBackend{ + ServiceName: "http-svc", + ServicePort: intstr.IntOrString{ + IntVal: 80, + }, + } + + if nginx.CheckIngress(ing) == nil { + t.Errorf("with a new catch-all ingress and catch-alls disable, should return error") + } + + // set back to nil for next test + ing.Spec.Backend = nil + }) + t.Run("When the ingress is in a different namespace than the watched one", func(t *testing.T) { nginx.command = testNginxTestCommand{ t: t, From bc8a731e28ccf33ece598cf2e793692176538cf5 Mon Sep 17 00:00:00 2001 From: Mahnoor Mehboob Date: Mon, 26 Apr 2021 17:32:50 -0400 Subject: [PATCH 4/4] reset backend and disableCatchAll to og value --- internal/ingress/controller/controller_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index c5b29ba8c..a7560c3bf 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -257,6 +257,9 @@ func TestCheckIngress(t *testing.T) { }) t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) { + backendBefore := ing.Spec.Backend + disableCatchAllBefore := nginx.cfg.DisableCatchAll + nginx.command = testNginxTestCommand{ t: t, err: nil, @@ -274,8 +277,9 @@ func TestCheckIngress(t *testing.T) { t.Errorf("with a new catch-all ingress and catch-alls disable, should return error") } - // set back to nil for next test - ing.Spec.Backend = nil + // reset backend and catch-all flag + ing.Spec.Backend = backendBefore + nginx.cfg.DisableCatchAll = disableCatchAllBefore }) t.Run("When the ingress is in a different namespace than the watched one", func(t *testing.T) {