Merge pull request #7069 from mahnoorm/update-catch-all-requirement

Update catch-all ingress requirement logic

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)) {

internal/ingress/controller/controller_test.go

@@ -256,6 +256,32 @@ 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,
+			}
+			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")
+			}
+
+			// 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) {
 			nginx.command = testNginxTestCommand{
 				t:   t,

internal/ingress/controller/store/store.go

@@ -328,7 +328,7 @@ func New(
 			return
 		}
 
-		if isCatchAllIngress(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 isCatchAllIngress(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 isCatchAllIngress(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 isCatchAllIngress(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,10 +624,10 @@ func New(
 	return store
 }
 
-// isCatchAllIngress 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 isCatchAllIngress(spec networkingv1beta1.IngressSpec) bool {
+func hasCatchAllIngressRule(spec networkingv1beta1.IngressSpec) bool {
-	return spec.Backend != nil && len(spec.Rules) == 0
+	return spec.Backend != nil
 }
 
 // syncIngress parses ingress annotations converting the value of the

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 {