use-regex annotation should be applied to only one Location

kubernetes · Jul 6, 2020 · a8a8b5f · a8a8b5f
1 parent ec4fb05
commit a8a8b5f
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 55 deletions.
diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
@@ -171,7 +171,6 @@ var (
 		"buildOpentracing":                   buildOpentracing,
 		"proxySetHeader":                     proxySetHeader,
 		"buildInfluxDB":                      buildInfluxDB,
-		"enforceRegexModifier":               enforceRegexModifier,
 		"buildCustomErrorDeps":               buildCustomErrorDeps,
 		"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
 		"shouldLoadModSecurityModule":        shouldLoadModSecurityModule,
@@ -373,34 +372,17 @@ func needsRewrite(location *ingress.Location) bool {
 	return false
 }
 
-// enforceRegexModifier checks if the "rewrite-target" or "use-regex" annotation
-// is used on any location path within a server
-func enforceRegexModifier(input interface{}) bool {
-	locations, ok := input.([]*ingress.Location)
-	if !ok {
-		klog.Errorf("expected an '[]*ingress.Location' type but %T was returned", input)
-		return false
-	}
-
-	for _, location := range locations {
-		if needsRewrite(location) || location.Rewrite.UseRegex {
-			return true
-		}
-	}
-	return false
-}
-
 // buildLocation produces the location string, if the ingress has redirects
 // (specified through the nginx.ingress.kubernetes.io/rewrite-target annotation)
-func buildLocation(input interface{}, enforceRegex bool) string {
+func buildLocation(input interface{}) string {
 	location, ok := input.(*ingress.Location)
 	if !ok {
 		klog.Errorf("expected an '*ingress.Location' type but %T was returned", input)
 		return slash
 	}
 
 	path := location.Path
-	if enforceRegex {
+	if location.Rewrite.UseRegex {
 		return fmt.Sprintf(`~* "^%s"`, path)
 	}
 

diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go
@@ -64,7 +64,7 @@ var (
 		Sticky           bool
 		XForwardedPrefix string
 		SecureBackend    bool
-		enforceRegex     bool
+		UseRegex         bool
 	}{
 		"when secure backend enabled": {
 			"/",
@@ -275,7 +275,7 @@ func TestQuote(t *testing.T) {
 func TestBuildLocation(t *testing.T) {
 	invalidType := &ingress.Ingress{}
 	expected := "/"
-	actual := buildLocation(invalidType, true)
+	actual := buildLocation(invalidType)
 
 	if !reflect.DeepEqual(expected, actual) {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
@@ -284,10 +284,10 @@ func TestBuildLocation(t *testing.T) {
 	for k, tc := range tmplFuncTestcases {
 		loc := &ingress.Location{
 			Path:    tc.Path,
-			Rewrite: rewrite.Config{Target: tc.Target},
+			Rewrite: rewrite.Config{Target: tc.Target, UseRegex: tc.UseRegex},
 		}
 
-		newLoc := buildLocation(loc, tc.enforceRegex)
+		newLoc := buildLocation(loc)
 		if tc.Location != newLoc {
 			t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
 		}
@@ -1195,32 +1195,6 @@ func TestBuildOpenTracing(t *testing.T) {
 
 }
 
-func TestEnforceRegexModifier(t *testing.T) {
-	invalidType := &ingress.Ingress{}
-	expected := false
-	actual := enforceRegexModifier(invalidType)
-
-	if expected != actual {
-		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
-	}
-
-	locs := []*ingress.Location{
-		{
-			Rewrite: rewrite.Config{
-				Target:   "/alright",
-				UseRegex: true,
-			},
-			Path: "/ok",
-		},
-	}
-	expected = true
-	actual = enforceRegexModifier(locs)
-
-	if expected != actual {
-		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
-	}
-}
-
 func TestShouldLoadModSecurityModule(t *testing.T) {
 	// ### Invalid argument type tests ###
 	// The first tests should return false.

diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
@@ -903,9 +903,8 @@ stream {
 
         {{ buildMirrorLocations $server.Locations }}
 
-        {{ $enforceRegex := enforceRegexModifier $server.Locations }}
         {{ range $location := $server.Locations }}
-        {{ $path := buildLocation $location $enforceRegex }}
+        {{ $path := buildLocation $location }}
         {{ $proxySetHeader := proxySetHeader $location }}
         {{ $authPath := buildAuthLocation $location $all.Cfg.GlobalExternalAuth.URL }}
         {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL }}

diff --git a/test/e2e/annotations/rewrite.go b/test/e2e/annotations/rewrite.go
@@ -95,7 +95,8 @@ var _ = framework.DescribeAnnotation("rewrite-target use-regex enable-rewrite-lo
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, `location ~* "^/" {`) && strings.Contains(server, `location ~* "^/.well-known/acme/challenge" {`)
+				return strings.Contains(server, `location / {`) &&
+					strings.Contains(server, `location /.well-known/acme/challenge {`)
 			})
 
 		ginkgo.By("making a second request to the non-rewritten location")
@@ -129,7 +130,7 @@ var _ = framework.DescribeAnnotation("rewrite-target use-regex enable-rewrite-lo
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, `location ~* "^/foo" {`) && strings.Contains(server, `location ~* "^/foo.+" {`)
+				return strings.Contains(server, `location /foo {`) && strings.Contains(server, `location ~* "^/foo.+" {`)
 			})
 
 		ginkgo.By("ensuring '/foo' matches '~* ^/foo'")
@@ -170,7 +171,7 @@ var _ = framework.DescribeAnnotation("rewrite-target use-regex enable-rewrite-lo
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, `location ~* "^/foo/bar/bar" {`) &&
+				return strings.Contains(server, `location /foo/bar/bar {`) &&
 					strings.Contains(server, `location ~* "^/foo/bar/[a-z]{3}" {`)
 			})