Fix a bug in Ingress update handler

internal/ingress/controller/store/store.go

@@ -304,6 +304,42 @@ func New(checkOCSP bool,
 	)
 	store.listers.Pod.Store = store.informers.Pod.GetStore()
 
+	ingDeleteHandler := func(obj interface{}) {
+		ing, ok := obj.(*extensions.Ingress)
+		if !ok {
+			// If we reached here it means the ingress was deleted but its final state is unrecorded.
+			tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+			if !ok {
+				klog.Errorf("couldn't get object from tombstone %#v", obj)
+				return
+			}
+			ing, ok = tombstone.Obj.(*extensions.Ingress)
+			if !ok {
+				klog.Errorf("Tombstone contained object that is not an Ingress: %#v", obj)
+				return
+			}
+		}
+		if !class.IsValid(ing) {
+			klog.Infof("ignoring delete for ingress %v based on annotation %v", ing.Name, class.IngressKey)
+			return
+		}
+		if ing.Spec.Backend != nil && disableCatchAll {
+			klog.Infof("ignoring delete for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
+			return
+		}
+		recorder.Eventf(ing, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name))
+
+		store.listers.IngressWithAnnotation.Delete(ing)
+
+		key := k8s.MetaNamespaceKey(ing)
+		store.secretIngressMap.Delete(key)
+
+		updateCh.In() <- Event{
+			Type: DeleteEvent,
+			Obj:  obj,
+		}
+	}
+
 	ingEventHandler := cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			ing := obj.(*extensions.Ingress)
@@ -327,41 +363,7 @@ func New(checkOCSP bool,
 				Obj:  obj,
 			}
 		},
-		DeleteFunc: func(obj interface{}) {
+		DeleteFunc: ingDeleteHandler,
-			ing, ok := obj.(*extensions.Ingress)
-			if !ok {
-				// If we reached here it means the ingress was deleted but its final state is unrecorded.
-				tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
-				if !ok {
-					klog.Errorf("couldn't get object from tombstone %#v", obj)
-					return
-				}
-				ing, ok = tombstone.Obj.(*extensions.Ingress)
-				if !ok {
-					klog.Errorf("Tombstone contained object that is not an Ingress: %#v", obj)
-					return
-				}
-			}
-			if !class.IsValid(ing) {
-				klog.Infof("ignoring delete for ingress %v based on annotation %v", ing.Name, class.IngressKey)
-				return
-			}
-			if ing.Spec.Backend != nil && disableCatchAll {
-				klog.Infof("ignoring delete for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
-				return
-			}
-			recorder.Eventf(ing, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name))
-
-			store.listers.IngressWithAnnotation.Delete(ing)
-
-			key := k8s.MetaNamespaceKey(ing)
-			store.secretIngressMap.Delete(key)
-
-			updateCh.In() <- Event{
-				Type: DeleteEvent,
-				Obj:  obj,
-			}
-		},
 		UpdateFunc: func(old, cur interface{}) {
 			oldIng := old.(*extensions.Ingress)
 			curIng := cur.(*extensions.Ingress)
@@ -377,18 +379,16 @@ func New(checkOCSP bool,
 				recorder.Eventf(curIng, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
 			} else if validOld && !validCur {
 				klog.Infof("removing ingress %v based on annotation %v", curIng.Name, class.IngressKey)
-				// FIXME: this does not actually delete the Ingress resource.
+				ingDeleteHandler(old)
-				// The existing one will be updated with latest changes and invalid ingress.class will be missed.
+				return
-				recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
 			} else if validCur && !reflect.DeepEqual(old, cur) {
 				if curIng.Spec.Backend != nil && disableCatchAll {
-					klog.Infof("ignoring update for catch-all ingress %v/%v because of --disable-catch-all", curIng.Namespace, curIng.Name)
+					klog.Infof("ignoring update for catch-all ingress %v/%v and delete old one because of --disable-catch-all", curIng.Namespace, curIng.Name)
-					// FIXME: this does not actually delete the Ingress resource.
+					ingDeleteHandler(old)
-					// The existing one will be updated with latest changes.
+					return
-					recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
-				} else {
-					recorder.Eventf(curIng, corev1.EventTypeNormal, "UPDATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
 				}
+
+				recorder.Eventf(curIng, corev1.EventTypeNormal, "UPDATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
 			} else {
 				klog.Infof("ignoring ingress %v based on annotation %v", curIng.Name, class.IngressKey)
 				return

test/e2e/settings/disable_catch_all.go

@@ -17,10 +17,16 @@ limitations under the License.
 package settings
 
 import (
+	"net/http"
 	"strings"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/parnurzeal/gorequest"
 	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	extensions "k8s.io/api/extensions/v1beta1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
@@ -64,46 +70,43 @@ var _ = framework.IngressNginxDescribe("Disabled catch-all", func() {
 		})
 	})
 
-	// FIXME: This test doesn't work because of a bug in Ingress update handle in store package.
+	It("should delete Ingress updated to catch-all", func() {
-	// It("should delete Ingress updated to catch-all", func() {
+		host := "foo"
-	// 	host := "foo"
 
-	// 	ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)
+		ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)
-	// 	f.EnsureIngress(ing)
+		f.EnsureIngress(ing)
 
-	// 	f.WaitForNginxServer(host,
+		f.WaitForNginxServer(host,
-	// 		func(server string) bool {
+			func(server string) bool {
-	// 			return strings.Contains(server, "server_name foo")
+				return strings.Contains(server, "server_name foo")
-	// 		})
+			})
 
-	// 	resp, _, errs := gorequest.New().
+		resp, _, errs := gorequest.New().
-	// 		Get(f.IngressController.HTTPURL).
+			Get(f.IngressController.HTTPURL).
-	// 		Set("Host", host).
+			Set("Host", host).
-	// 		End()
+			End()
-	// 	Expect(errs).To(BeNil())
+		Expect(errs).To(BeNil())
-	// 	Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
 
-	// 	err := framework.UpdateIngress(f.KubeClientSet, f.IngressController.Namespace, host, func(ingress *extensions.Ingress) error {
+		err := framework.UpdateIngress(f.KubeClientSet, f.IngressController.Namespace, host, func(ingress *extensions.Ingress) error {
-	// 		ingress.Spec.Rules = nil
+			ingress.Spec.Rules = nil
-	// 		ingress.Spec.Backend = &extensions.IngressBackend{
+			ingress.Spec.Backend = &extensions.IngressBackend{
-	// 			ServiceName: "http-svc",
+				ServiceName: "http-svc",
-	// 			ServicePort: intstr.FromInt(80),
+				ServicePort: intstr.FromInt(80),
-	// 		}
+			}
-	// 		return nil
+			return nil
-	// 	})
+		})
-	// 	Expect(err).ToNot(HaveOccurred())
+		Expect(err).ToNot(HaveOccurred())
 
-	// 	f.WaitForNginxConfiguration(func(cfg string) bool {
+		f.WaitForNginxConfiguration(func(cfg string) bool {
-	// 		return !strings.Contains(cfg, "server_name foo") &&
+			return !strings.Contains(cfg, "server_name foo")
-	// 			!strings.Contains(cfg, `set $ingress_name "foo"`) &&
+		})
-	// 			!strings.Contains(cfg, `set $service_name "http-svc"`)
-	// 	})
 
-	// 	resp, _, errs = gorequest.New().
+		resp, _, errs = gorequest.New().
-	// 		Get(f.IngressController.HTTPURL).
+			Get(f.IngressController.HTTPURL).
-	// 		Set("Host", host).
+			Set("Host", host).
-	// 		End()
+			End()
-	// 	Expect(errs).To(BeNil())
+		Expect(errs).To(BeNil())
-	// 	Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
+		Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
-	// })
+	})
 })

test/e2e/settings/ingress_class.go

@@ -0,0 +1,160 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package settings
+
+import (
+	"net/http"
+	"strings"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/parnurzeal/gorequest"
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
+
+var _ = framework.IngressNginxDescribe("Ingress class", func() {
+	f := framework.NewDefaultFramework("ingress-class")
+
+	BeforeEach(func() {
+		f.NewEchoDeploymentWithReplicas(1)
+	})
+
+	AfterEach(func() {
+	})
+
+	Context("Without a specific ingress-class", func() {
+
+		It("should ignore Ingress with class", func() {
+			invalidHost := "foo"
+			annotations := map[string]string{
+				"kubernetes.io/ingress.class": "testclass",
+			}
+			ing := framework.NewSingleIngress(invalidHost, "/", invalidHost, f.IngressController.Namespace, "http-svc", 80, &annotations)
+			f.EnsureIngress(ing)
+
+			validHost := "bar"
+			ing = framework.NewSingleIngress(validHost, "/", validHost, f.IngressController.Namespace, "http-svc", 80, nil)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxConfiguration(func(cfg string) bool {
+				return !strings.Contains(cfg, "server_name foo") &&
+					strings.Contains(cfg, "server_name bar")
+			})
+
+			resp, _, errs := gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Set("Host", invalidHost).
+				End()
+			Expect(errs).To(BeNil())
+			Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
+
+			resp, _, errs = gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Set("Host", validHost).
+				End()
+			Expect(errs).To(BeNil())
+			Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+		})
+	})
+
+	Context("With a specific ingress-class", func() {
+		BeforeEach(func() {
+			framework.UpdateDeployment(f.KubeClientSet, f.IngressController.Namespace, "nginx-ingress-controller", 1,
+				func(deployment *appsv1beta1.Deployment) error {
+					args := deployment.Spec.Template.Spec.Containers[0].Args
+					args = append(args, "--ingress-class=testclass")
+					deployment.Spec.Template.Spec.Containers[0].Args = args
+					_, err := f.KubeClientSet.AppsV1beta1().Deployments(f.IngressController.Namespace).Update(deployment)
+
+					return err
+				})
+		})
+
+		It("should ignore Ingress with no class", func() {
+			invalidHost := "bar"
+
+			ing := framework.NewSingleIngress(invalidHost, "/", invalidHost, f.IngressController.Namespace, "http-svc", 80, nil)
+			f.EnsureIngress(ing)
+
+			validHost := "foo"
+			annotations := map[string]string{
+				"kubernetes.io/ingress.class": "testclass",
+			}
+			ing = framework.NewSingleIngress(validHost, "/", validHost, f.IngressController.Namespace, "http-svc", 80, &annotations)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxServer(validHost, func(cfg string) bool {
+				return strings.Contains(cfg, "server_name foo")
+			})
+
+			f.WaitForNginxConfiguration(func(cfg string) bool {
+				return !strings.Contains(cfg, "server_name bar")
+			})
+
+			resp, _, errs := gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Set("Host", validHost).
+				End()
+			Expect(errs).To(BeNil())
+			Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+
+			resp, _, errs = gorequest.New().
+				Get(f.IngressController.HTTPURL).
+				Set("Host", invalidHost).
+				End()
+			Expect(errs).To(BeNil())
+			Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
+		})
+
+		It("should delete Ingress when class is removed", func() {
+			host := "foo"
+			annotations := map[string]string{
+				"kubernetes.io/ingress.class": "testclass",
+			}
+			ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+			ing = 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))
+
+			delete(ing.Annotations, "kubernetes.io/ingress.class")
+			ing = f.EnsureIngress(ing)
+
+			f.WaitForNginxConfiguration(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.StatusNotFound))
+		})
+	})
+
+})