Fix/add unit tests; Styling changes

kubernetes · Sep 14, 2018 · 0de19c8 · 0de19c8
1 parent 0e6f0bb
commit 0de19c8
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 24 deletions.
diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
@@ -334,7 +334,7 @@ func buildLocation(input interface{}, rewrite bool) string {
 		return fmt.Sprintf(`~* ^%s%s`, path, baseuri)
 	}
 
-	if rewrite == true {
+	if rewrite {
 		if path == slash {
 			return path
 		}

diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go
@@ -50,6 +50,7 @@ var (
 		XForwardedPrefix            bool
 		DynamicConfigurationEnabled bool
 		SecureBackend               bool
+		atLeastOneNeedsRewrite      bool
 	}{
 		"when secure backend enabled": {
 			"/",
@@ -61,7 +62,8 @@ var (
 			false,
 			false,
 			false,
-			true},
+			true,
+			false},
 		"when secure backend and stickeness enabled": {
 			"/",
 			"/",
@@ -72,7 +74,8 @@ var (
 			true,
 			false,
 			false,
-			true},
+			true,
+			false},
 		"when secure backend and dynamic config enabled": {
 			"/",
 			"/",
@@ -83,7 +86,8 @@ var (
 			false,
 			false,
 			true,
-			true},
+			true,
+			false},
 		"when secure backend, stickeness and dynamic config enabled": {
 			"/",
 			"/",
@@ -94,7 +98,8 @@ var (
 			true,
 			false,
 			true,
-			true},
+			true,
+			false},
 		"invalid redirect / to / with dynamic config enabled": {
 			"/",
 			"/",
@@ -105,6 +110,7 @@ var (
 			false,
 			false,
 			true,
+			false,
 			false},
 		"invalid redirect / to /": {
 			"/",
@@ -116,6 +122,7 @@ var (
 			false,
 			false,
 			false,
+			false,
 			false},
 		"redirect / to /jenkins": {
 			"/",
@@ -131,7 +138,8 @@ proxy_pass http://upstream-name;
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /something to /": {
 			"/something",
 			"/",
@@ -146,7 +154,8 @@ proxy_pass http://upstream-name;
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /end-with-slash/ to /not-root": {
 			"/end-with-slash/",
 			"/not-root",
@@ -161,7 +170,8 @@ proxy_pass http://upstream-name;
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /something-complex to /not-root": {
 			"/something-complex",
 			"/not-root",
@@ -176,7 +186,8 @@ proxy_pass http://upstream-name;
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect / to /jenkins and rewrite": {
 			"/",
 			"/jenkins",
@@ -194,7 +205,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /something to / and rewrite": {
 			"/something",
 			"/",
@@ -212,7 +224,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /end-with-slash/ to /not-root and rewrite": {
 			"/end-with-slash/",
 			"/not-root",
@@ -230,7 +243,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /something-complex to /not-root and rewrite": {
 			"/something-complex",
 			"/not-root",
@@ -248,7 +262,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect /something to / and rewrite with specific scheme": {
 			"/something",
 			"/",
@@ -266,7 +281,8 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1<base href="
 			false,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect / to /something with sticky enabled": {
 			"/",
 			"/something",
@@ -281,7 +297,8 @@ proxy_pass http://sticky-upstream-name;
 			true,
 			false,
 			false,
-			false},
+			false,
+			true},
 		"redirect / to /something with sticky and dynamic config enabled": {
 			"/",
 			"/something",
@@ -296,7 +313,8 @@ proxy_pass http://upstream_balancer;
 			true,
 			false,
 			true,
-			false},
+			false,
+			true},
 		"add the X-Forwarded-Prefix header": {
 			"/there",
 			"/something",
@@ -312,7 +330,32 @@ proxy_pass http://sticky-upstream-name;
 			true,
 			true,
 			false,
-			false},
+			false,
+			true},
+		"do not use ^~ location modifier on index when ingress does not use rewrite target but at least one other ingress does": {
+			"/",
+			"/",
+			"/",
+			"proxy_pass http://upstream-name;",
+			false,
+			"",
+			false,
+			false,
+			false,
+			false,
+			true},
+		"use ^~ location modifier when ingress does not use rewrite target but at least one other ingress does": {
+			"/something",
+			"/something",
+			"^~ /something",
+			"proxy_pass http://upstream-name;",
+			false,
+			"",
+			false,
+			false,
+			false,
+			false,
+			true},
 	}
 )
 
@@ -382,7 +425,7 @@ func TestBuildLocation(t *testing.T) {
 			Rewrite: rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL},
 		}
 
-		newLoc := buildLocation(loc, tc.Path != tc.Target)
+		newLoc := buildLocation(loc, tc.atLeastOneNeedsRewrite)
 		if tc.Location != newLoc {
 			t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
 		}

diff --git a/test/e2e/annotations/rewrite.go b/test/e2e/annotations/rewrite.go
@@ -121,13 +121,9 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
 	It("should use correct longest path match", func() {
 		host := "rewrite.bar.com"
 		expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/.well-known/acme/challenge", host)
-		annotations := map[string]string{}
-		rewriteAnnotations := map[string]string{
-			"nginx.ingress.kubernetes.io/rewrite-target": "/new/backend",
-		}
 
 		By("creating a regular ingress definition")
-		ing := framework.NewSingleIngress("kube-lego", "/.well-known/acme/challenge", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		ing := framework.NewSingleIngress("kube-lego", "/.well-known/acme/challenge", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{})
 		_, err := f.EnsureIngress(ing)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ing).NotTo(BeNil())
@@ -149,7 +145,10 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
 		Expect(body).Should(ContainSubstring(expectBodyRequestURI))
 
 		By(`creating an ingress definition with the rewrite-target annotation set on the "/" location`)
-		rewriteIng := framework.NewSingleIngress("rewrite-index", "/", host, f.IngressController.Namespace, "http-svc", 80, &rewriteAnnotations)
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/rewrite-target": "/new/backend",
+		}
+		rewriteIng := framework.NewSingleIngress("rewrite-index", "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
 		_, err = f.EnsureIngress(rewriteIng)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(rewriteIng).NotTo(BeNil())