Merge pull request #3698 from Shopify/fix-disable-catch-all

Fix --disable-catch-all
kubernetes · Jan 25, 2019 · 6618b39 · 6618b39
2 parents bd74dce + ca74960
commit 6618b39
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 4 deletions.
diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go
@@ -323,7 +323,7 @@ func New(checkOCSP bool,
 			klog.Infof("ignoring delete for ingress %v based on annotation %v", ing.Name, class.IngressKey)
 			return
 		}
-		if ing.Spec.Backend != nil && disableCatchAll {
+		if isCatchAllIngress(ing.Spec) && disableCatchAll {
 			klog.Infof("ignoring delete for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
 			return
 		}
@@ -348,7 +348,7 @@ func New(checkOCSP bool,
 				klog.Infof("ignoring add for ingress %v based on annotation %v with value %v", ing.Name, class.IngressKey, a)
 				return
 			}
-			if ing.Spec.Backend != nil && disableCatchAll {
+			if isCatchAllIngress(ing.Spec) && disableCatchAll {
 				klog.Infof("ignoring add for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
 				return
 			}
@@ -370,7 +370,7 @@ func New(checkOCSP bool,
 			validOld := class.IsValid(oldIng)
 			validCur := class.IsValid(curIng)
 			if !validOld && validCur {
-				if curIng.Spec.Backend != nil && disableCatchAll {
+				if isCatchAllIngress(curIng.Spec) && disableCatchAll {
 					klog.Infof("ignoring update for catch-all ingress %v/%v because of --disable-catch-all", curIng.Namespace, curIng.Name)
 					return
 				}
@@ -382,7 +382,7 @@ func New(checkOCSP bool,
 				ingDeleteHandler(old)
 				return
 			} else if validCur && !reflect.DeepEqual(old, cur) {
-				if curIng.Spec.Backend != nil && disableCatchAll {
+				if isCatchAllIngress(curIng.Spec) && disableCatchAll {
 					klog.Infof("ignoring update for catch-all ingress %v/%v and delete old one because of --disable-catch-all", curIng.Namespace, curIng.Name)
 					ingDeleteHandler(old)
 					return
@@ -617,6 +617,12 @@ func New(checkOCSP bool,
 	return store
 }
 
+// isCatchAllIngress 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 extensions.IngressSpec) bool {
+	return spec.Backend != nil && len(spec.Rules) == 0
+}
+
 // syncIngress parses ingress annotations converting the value of the
 // annotation to a go struct
 func (s *k8sStore) syncIngress(ing *extensions.Ingress) {

diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go
@@ -432,6 +432,36 @@ func newSingleIngressWithRules(name, path, host, ns, service string, port int, a
 	return newSingleIngress(name, ns, annotations, spec)
 }
 
+// NewSingleIngressWithBackendAndRules creates an ingress with both a default backend and a rule
+func NewSingleIngressWithBackendAndRules(name, path, host, ns, defaultService string, defaultPort int, service string, port int, annotations *map[string]string) *extensions.Ingress {
+	spec := extensions.IngressSpec{
+		Backend: &extensions.IngressBackend{
+			ServiceName: defaultService,
+			ServicePort: intstr.FromInt(defaultPort),
+		},
+		Rules: []extensions.IngressRule{
+			{
+				Host: host,
+				IngressRuleValue: extensions.IngressRuleValue{
+					HTTP: &extensions.HTTPIngressRuleValue{
+						Paths: []extensions.HTTPIngressPath{
+							{
+								Path: path,
+								Backend: extensions.IngressBackend{
+									ServiceName: service,
+									ServicePort: intstr.FromInt(port),
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	return newSingleIngress(name, ns, annotations, spec)
+}
+
 // NewSingleCatchAllIngress creates a simple ingress with a catch-all backend
 func NewSingleCatchAllIngress(name, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
 	spec := extensions.IngressSpec{

diff --git a/test/e2e/settings/disable_catch_all.go b/test/e2e/settings/disable_catch_all.go
@@ -109,4 +109,24 @@ var _ = framework.IngressNginxDescribe("Disabled catch-all", func() {
 		Expect(errs).To(BeNil())
 		Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
 	})
+
+	It("should allow Ingress with both a default backend and rules", func() {
+		host := "foo"
+
+		ing := framework.NewSingleIngressWithBackendAndRules("not-catch-all", "/rulepath", host, f.IngressController.Namespace, "http-svc", 80, "http-svc", 80, nil)
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host, func(cfg string) bool {
+			return strings.Contains(cfg, "server_name foo")
+		})
+
+		resp, _, errs := gorequest.New().
+			Get(f.IngressController.HTTPURL).
+			Set("Host", host).
+			End()
+
+		Expect(errs).To(BeNil())
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+
+	})
 })