Merge pull request #5896 from kubernetes/revert-5691-use-regex

Revert "use-regex annotation should be applied to only one Location"

internal/ingress/controller/template/template.go

@@ -171,6 +171,7 @@ var (
 		"buildOpentracing":                   buildOpentracing,
 		"proxySetHeader":                     proxySetHeader,
 		"buildInfluxDB":                      buildInfluxDB,
+		"enforceRegexModifier":               enforceRegexModifier,
 		"buildCustomErrorDeps":               buildCustomErrorDeps,
 		"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
 		"shouldLoadModSecurityModule":        shouldLoadModSecurityModule,
@@ -372,9 +373,26 @@ 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{}) string {
+func buildLocation(input interface{}, enforceRegex bool) string {
 	location, ok := input.(*ingress.Location)
 	if !ok {
 		klog.Errorf("expected an '*ingress.Location' type but %T was returned", input)
@@ -382,7 +400,7 @@ func buildLocation(input interface{}) string {
 	}
 
 	path := location.Path
-	if location.Rewrite.UseRegex {
+	if enforceRegex {
 		return fmt.Sprintf(`~* "^%s"`, path)
 	}
 

internal/ingress/controller/template/template_test.go

@@ -64,7 +64,7 @@ var (
 		Sticky           bool
 		XForwardedPrefix string
 		SecureBackend    bool
-		UseRegex         bool
+		enforceRegex     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)
+	actual := buildLocation(invalidType, true)
 
 	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, UseRegex: tc.UseRegex},
+			Rewrite: rewrite.Config{Target: tc.Target},
 		}
 
-		newLoc := buildLocation(loc)
+		newLoc := buildLocation(loc, tc.enforceRegex)
 		if tc.Location != newLoc {
 			t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
 		}
@@ -1195,6 +1195,32 @@ 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.

rootfs/etc/nginx/template/nginx.tmpl

@@ -903,8 +903,9 @@ stream {
 
         {{ buildMirrorLocations $server.Locations }}
 
+        {{ $enforceRegex := enforceRegexModifier $server.Locations }}
         {{ range $location := $server.Locations }}
-        {{ $path := buildLocation $location }}
+        {{ $path := buildLocation $location $enforceRegex }}
         {{ $proxySetHeader := proxySetHeader $location }}
         {{ $authPath := buildAuthLocation $location $all.Cfg.GlobalExternalAuth.URL }}
         {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL }}

test/e2e/annotations/rewrite.go

@@ -95,8 +95,7 @@ var _ = framework.DescribeAnnotation("rewrite-target use-regex enable-rewrite-lo
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, `location / {`) &&
+				return strings.Contains(server, `location ~* "^/" {`) && strings.Contains(server, `location ~* "^/.well-known/acme/challenge" {`)
-					strings.Contains(server, `location /.well-known/acme/challenge {`)
 			})
 
 		ginkgo.By("making a second request to the non-rewritten location")
@@ -130,7 +129,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'")
@@ -171,7 +170,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}" {`)
 			})