Merge pull request #3765 from ElvinEfendi/custom-http-error-bug-fix

simplify customhttperrors e2e test and add regression test and fix a bug

internal/ingress/types_equals.go

@@ -449,6 +449,22 @@ func (l1 *Location) Equal(l2 *Location) bool {
 		return false
 	}
 
+	if len(l1.CustomHTTPErrors) != len(l2.CustomHTTPErrors) {
+		return false
+	}
+	for _, code1 := range l1.CustomHTTPErrors {
+		found := false
+		for _, code2 := range l2.CustomHTTPErrors {
+			if code1 == code2 {
+				found = true
+				break
+			}
+		}
+		if !found {
+			return false
+		}
+	}
+
 	if !(&l1.ModSecurity).Equal(&l2.ModSecurity) {
 		return false
 	}

test/e2e/annotations/customhttperrors.go

@@ -22,6 +22,9 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+
+	extensions "k8s.io/api/extensions/v1beta1"
+
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
@@ -29,13 +32,13 @@ var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func(
 	f := framework.NewDefaultFramework("custom-http-errors")
 
 	BeforeEach(func() {
-		f.NewEchoDeploymentWithReplicas(2)
+		f.NewEchoDeploymentWithReplicas(1)
 	})
 
 	AfterEach(func() {
 	})
 
-	It("should set proxy_intercept_errors", func() {
+	It("configures Nginx correctly", func() {
 		host := "customerrors.foo.com"
 
 		errorCodes := []string{"404", "500"}
@@ -47,73 +50,51 @@ var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func(
 		ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
 		f.EnsureIngress(ing)
 
-		f.WaitForNginxServer(host,
+		var serverConfig string
-			func(server string) bool {
+		f.WaitForNginxServer(host, func(sc string) bool {
-				return Expect(server).Should(ContainSubstring("proxy_intercept_errors on;"))
+			serverConfig = sc
-			})
+			return strings.Contains(serverConfig, fmt.Sprintf("server_name %s", host))
-	})
+		})
 
-	It("should create error routes", func() {
+		By("turning on proxy_intercept_errors directive")
-		host := "customerrors.foo.com"
+		Expect(serverConfig).Should(ContainSubstring("proxy_intercept_errors on;"))
-		errorCodes := []string{"404", "500"}
-
-		annotations := map[string]string{
-			"nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(errorCodes, ","),
-		}
-
-		ing := framework.NewSingleIngress(host, "/test", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
-		f.EnsureIngress(ing)
 
+		By("configuring error_page directive")
 		for _, code := range errorCodes {
-			f.WaitForNginxServer(host,
+			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("error_page %s = @custom_%s", code, code)))
-				func(server string) bool {
-					return Expect(server).Should(ContainSubstring(fmt.Sprintf("@custom_%s", code)))
-				})
-		}
-	})
-
-	It("should set up error_page routing", func() {
-		host := "customerrors.foo.com"
-		errorCodes := []string{"404", "500"}
-
-		annotations := map[string]string{
-			"nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(errorCodes, ","),
 		}
 
-		ing := framework.NewSingleIngress(host, "/test", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		By("creating error locations")
-		f.EnsureIngress(ing)
-
 		for _, code := range errorCodes {
-			f.WaitForNginxServer(host,
+			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("location @custom_%s", code)))
-				func(server string) bool {
-					return Expect(server).Should(ContainSubstring(fmt.Sprintf("error_page %s = @custom_%s", code, code)))
-				})
 		}
-	})
 
-	It("should create only one of each error route", func() {
+		By("updating configuration when only custom-http-error value changes")
-		host := "customerrors.foo.com"
+		err := framework.UpdateIngress(f.KubeClientSet, f.IngressController.Namespace, host, func(ingress *extensions.Ingress) error {
-		errorCodes := [][]string{{"404", "500"}, {"400", "404"}}
+			ingress.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-http-errors"] = "503"
-
+			return nil
-		for i, codeSet := range errorCodes {
+		})
-			annotations := map[string]string{
+		Expect(err).ToNot(HaveOccurred())
-				"nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(codeSet, ","),
+		f.WaitForNginxServer(host, func(sc string) bool {
+			if serverConfig != sc {
+				serverConfig = sc
+				return true
 			}
+			return false
+		})
+		Expect(serverConfig).Should(ContainSubstring("location @custom_503"))
+		Expect(serverConfig).ShouldNot(ContainSubstring("location @custom_400"))
+		Expect(serverConfig).ShouldNot(ContainSubstring("location @custom_500"))
 
-			ing := framework.NewSingleIngress(
+		By("ignoring duplicate values (503 in this case) per server")
-				fmt.Sprintf("%s-%d", host, i), fmt.Sprintf("/test-%d", i),
+		annotations["nginx.ingress.kubernetes.io/custom-http-errors"] = "404, 503"
-				host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		ing = framework.NewSingleIngress(fmt.Sprintf("%s-else", host), "/else", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
-			f.EnsureIngress(ing)
+		f.EnsureIngress(ing)
-		}
+		f.WaitForNginxServer(host, func(sc string) bool {
-
+			serverConfig = sc
-		for _, codeSet := range errorCodes {
+			return strings.Contains(serverConfig, "location /else")
-			for _, code := range codeSet {
+		})
-				f.WaitForNginxServer(host,
+		count := strings.Count(serverConfig, fmt.Sprintf("location @custom_503"))
-					func(server string) bool {
+		Expect(count).Should(Equal(1))
-						count := strings.Count(server, fmt.Sprintf("location @custom_%s", code))
-						return Expect(count).Should(Equal(1))
-					})
-			}
-		}
 	})
 })