fix: Ensure unique generated locations (#1445)

Problem: As a user, I want to ensure internal location blocks are not named the same as other defined location blocks. Solution: Removed prefix name for internal location blocks, and gave an indexed name to avoid collision with other location blocks. Updated unit tests and cleaned up unused fields tracking `internal` location blocks.
nginxinc · Jan 16, 2024 · b2d078b · b2d078b
1 parent 9dc786a
commit b2d078b
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 108 deletions.
diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go
@@ -18,7 +18,6 @@ type Location struct {
 	ProxyPass       string
 	HTTPMatchVar    string
 	ProxySetHeaders []Header
-	Internal        bool
 }
 
 // Header defines a HTTP header to be passed to the proxied server.

diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go
@@ -95,18 +95,16 @@ func createServer(virtualServer dataplane.VirtualServer) http.Server {
 // rewriteConfig contains the configuration for a location to rewrite paths,
 // as specified in a URLRewrite filter
 type rewriteConfig struct {
-	// InternalRewrite rewrites an internal URI to the original URI (ex: /coffee_prefix_route0 -> /coffee)
-	InternalRewrite string
-	// MainRewrite rewrites the original URI to the new URI (ex: /coffee -> /beans)
-	MainRewrite string
+	// Rewrite rewrites the original URI to the new URI (ex: /coffee -> /beans)
+	Rewrite string
 }
 
 func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.Location {
 	maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(pathRules)
 	locs := make([]http.Location, 0, maxLocs)
 	var rootPathExists bool
 
-	for _, rule := range pathRules {
+	for pathRuleIdx, rule := range pathRules {
 		matches := make([]httpMatch, 0, len(rule.MatchRules))
 
 		if rule.Path == rootPath {
@@ -118,7 +116,7 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.
 		for matchRuleIdx, r := range rule.MatchRules {
 			buildLocations := extLocations
 			if len(rule.MatchRules) != 1 || !isPathOnlyMatch(r.Match) {
-				intLocation, match := initializeInternalLocation(rule, matchRuleIdx, r.Match)
+				intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match)
 				buildLocations = []http.Location{intLocation}
 				matches = append(matches, match)
 			}
@@ -216,11 +214,11 @@ func initializeExternalLocations(
 }
 
 func initializeInternalLocation(
-	rule dataplane.PathRule,
+	pathruleIdx,
 	matchRuleIdx int,
 	match dataplane.Match,
 ) (http.Location, httpMatch) {
-	path := createPathForMatch(rule.Path, rule.PathType, matchRuleIdx)
+	path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx)
 	return createMatchLocation(path), createHTTPMatch(match, path)
 }
 
@@ -252,11 +250,8 @@ func updateLocationsForFilters(
 	proxyPass := createProxyPass(matchRule.BackendGroup, matchRule.Filters.RequestURLRewrite)
 	for i := range buildLocations {
 		if rewrites != nil {
-			if buildLocations[i].Internal && rewrites.InternalRewrite != "" {
-				buildLocations[i].Rewrites = append(buildLocations[i].Rewrites, rewrites.InternalRewrite)
-			}
-			if rewrites.MainRewrite != "" {
-				buildLocations[i].Rewrites = append(buildLocations[i].Rewrites, rewrites.MainRewrite)
+			if rewrites.Rewrite != "" {
+				buildLocations[i].Rewrites = append(buildLocations[i].Rewrites, rewrites.Rewrite)
 			}
 		}
 		buildLocations[i].ProxySetHeaders = proxySetHeaders
@@ -319,10 +314,9 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p
 	rewrites := &rewriteConfig{}
 
 	if filter.Path != nil {
-		rewrites.InternalRewrite = "^ $request_uri"
 		switch filter.Path.Type {
 		case dataplane.ReplaceFullPath:
-			rewrites.MainRewrite = fmt.Sprintf("^ %s break", filter.Path.Replacement)
+			rewrites.Rewrite = fmt.Sprintf("^ %s break", filter.Path.Replacement)
 		case dataplane.ReplacePrefixMatch:
 			filterPrefix := filter.Path.Replacement
 			if filterPrefix == "" {
@@ -347,7 +341,7 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p
 				replacement = fmt.Sprintf("%s/$1", filterPrefix)
 			}
 
-			rewrites.MainRewrite = fmt.Sprintf("%s %s break", regex, replacement)
+			rewrites.Rewrite = fmt.Sprintf("%s %s break", regex, replacement)
 		}
 	}
 
@@ -449,8 +443,7 @@ func createProxyPass(backendGroup dataplane.BackendGroup, filter *dataplane.HTTP
 
 func createMatchLocation(path string) http.Location {
 	return http.Location{
-		Path:     path,
-		Internal: true,
+		Path: path,
 	}
 }
 
@@ -544,10 +537,6 @@ func createPath(rule dataplane.PathRule) string {
 	}
 }
 
-func createPathForMatch(path string, pathType dataplane.PathType, routeIdx int) string {
-	return fmt.Sprintf("%s_%s_route%d", path, pathType, routeIdx)
-}
-
 func createDefaultRootLocation() http.Location {
 	return http.Location{
 		Path:   "/",

diff --git a/internal/mode/static/nginx/config/servers_template.go b/internal/mode/static/nginx/config/servers_template.go
@@ -33,10 +33,6 @@ server {
 
         {{ range $l := $s.Locations }}
     location {{ $l.Path }} {
-        {{ if $l.Internal -}}
-        internal;
-        {{ end }}
-
         {{- range $r := $l.Rewrites }}
         rewrite {{ $r }};
         {{- end }}

diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go
@@ -490,34 +490,34 @@ func TestCreateServers(t *testing.T) {
 	}
 
 	slashMatches := []httpMatch{
-		{Method: "POST", RedirectPath: "/_prefix_route0"},
-		{Method: "PATCH", RedirectPath: "/_prefix_route1"},
-		{Any: true, RedirectPath: "/_prefix_route2"},
+		{Method: "POST", RedirectPath: "@rule0-route0"},
+		{Method: "PATCH", RedirectPath: "@rule0-route1"},
+		{Any: true, RedirectPath: "@rule0-route2"},
 	}
 	testMatches := []httpMatch{
 		{
 			Method:       "GET",
 			Headers:      []string{"Version:V1", "test:foo", "my-header:my-value"},
 			QueryParams:  []string{"GrEat=EXAMPLE", "test=foo=bar"},
-			RedirectPath: "/test_prefix_route0",
+			RedirectPath: "@rule1-route0",
 		},
 	}
 	exactMatches := []httpMatch{
 		{
 			Method:       "GET",
-			RedirectPath: "/test_exact_route0",
+			RedirectPath: "@rule11-route0",
 		},
 	}
 	redirectHeaderMatches := []httpMatch{
 		{
 			Headers:      []string{"redirect:this"},
-			RedirectPath: "/redirect-with-headers_prefix_route0",
+			RedirectPath: "@rule5-route0",
 		},
 	}
 	rewriteHeaderMatches := []httpMatch{
 		{
 			Headers:      []string{"rewrite:this"},
-			RedirectPath: "/rewrite-with-headers_prefix_route0",
+			RedirectPath: "@rule7-route0",
 		},
 	}
 	rewriteProxySetHeaders := []http.Header{
@@ -541,7 +541,7 @@ func TestCreateServers(t *testing.T) {
 	invalidFilterHeaderMatches := []httpMatch{
 		{
 			Headers:      []string{"filter:this"},
-			RedirectPath: "/invalid-filter-with-headers_prefix_route0",
+			RedirectPath: "@rule9-route0",
 		},
 	}
 
@@ -553,20 +553,17 @@ func TestCreateServers(t *testing.T) {
 
 		return []http.Location{
 			{
-				Path:            "/_prefix_route0",
-				Internal:        true,
+				Path:            "@rule0-route0",
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
 			{
-				Path:            "/_prefix_route1",
-				Internal:        true,
+				Path:            "@rule0-route1",
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
 			{
-				Path:            "/_prefix_route2",
-				Internal:        true,
+				Path:            "@rule0-route2",
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
@@ -575,8 +572,7 @@ func TestCreateServers(t *testing.T) {
 				HTTPMatchVar: expectedMatchString(slashMatches),
 			},
 			{
-				Path:            "/test_prefix_route0",
-				Internal:        true,
+				Path:            "@rule1-route0",
 				ProxyPass:       "http://$test__route1_rule1$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
@@ -623,12 +619,11 @@ func TestCreateServers(t *testing.T) {
 				},
 			},
 			{
-				Path: "/redirect-with-headers_prefix_route0",
+				Path: "@rule5-route0",
 				Return: &http.Return{
 					Body: "$scheme://foo.example.com:8080$request_uri",
 					Code: 302,
 				},
-				Internal: true,
 			},
 			{
 				Path:         "/redirect-with-headers/",
@@ -651,9 +646,8 @@ func TestCreateServers(t *testing.T) {
 				ProxySetHeaders: rewriteProxySetHeaders,
 			},
 			{
-				Path:            "/rewrite-with-headers_prefix_route0",
-				Rewrites:        []string{"^ $request_uri", "^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"},
-				Internal:        true,
+				Path:            "@rule7-route0",
+				Rewrites:        []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"},
 				ProxyPass:       "http://test_foo_80",
 				ProxySetHeaders: rewriteProxySetHeaders,
 			},
@@ -678,11 +672,10 @@ func TestCreateServers(t *testing.T) {
 				},
 			},
 			{
-				Path: "/invalid-filter-with-headers_prefix_route0",
+				Path: "@rule9-route0",
 				Return: &http.Return{
 					Code: http.StatusInternalServerError,
 				},
-				Internal: true,
 			},
 			{
 				Path:         "/invalid-filter-with-headers/",
@@ -698,10 +691,9 @@ func TestCreateServers(t *testing.T) {
 				ProxySetHeaders: baseHeaders,
 			},
 			{
-				Path:            "/test_exact_route0",
+				Path:            "@rule11-route0",
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
-				Internal:        true,
 			},
 			{
 				Path:         "= /test",
@@ -1274,8 +1266,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^ /full-path break",
+				Rewrite: "^ /full-path break",
 			},
 			msg: "full path",
 		},
@@ -1288,8 +1279,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original(.*)$ /prefix-path$1 break",
+				Rewrite: "^/original(.*)$ /prefix-path$1 break",
 			},
 			msg: "prefix path no trailing slashes",
 		},
@@ -1302,8 +1292,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original(?:/(.*))?$ /$1 break",
+				Rewrite: "^/original(?:/(.*))?$ /$1 break",
 			},
 			msg: "prefix path empty string",
 		},
@@ -1316,8 +1305,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original(?:/(.*))?$ /$1 break",
+				Rewrite: "^/original(?:/(.*))?$ /$1 break",
 			},
 			msg: "prefix path /",
 		},
@@ -1330,8 +1318,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original(?:/(.*))?$ /trailing/$1 break",
+				Rewrite: "^/original(?:/(.*))?$ /trailing/$1 break",
 			},
 			msg: "prefix path replacement with trailing /",
 		},
@@ -1344,8 +1331,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original/(.*)$ /prefix-path/$1 break",
+				Rewrite: "^/original/(.*)$ /prefix-path/$1 break",
 			},
 			msg: "prefix path original with trailing /",
 		},
@@ -1358,8 +1344,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original/(.*)$ /trailing/$1 break",
+				Rewrite: "^/original/(.*)$ /trailing/$1 break",
 			},
 			msg: "prefix path both with trailing slashes",
 		},
@@ -1694,38 +1679,13 @@ func TestCreateMatchLocation(t *testing.T) {
 	g := NewWithT(t)
 
 	expected := http.Location{
-		Path:     "/path",
-		Internal: true,
+		Path: "/path",
 	}
 
 	result := createMatchLocation("/path")
 	g.Expect(result).To(Equal(expected))
 }
 
-func TestCreatePathForMatch(t *testing.T) {
-	g := NewWithT(t)
-
-	tests := []struct {
-		expected string
-		pathType dataplane.PathType
-		panic    bool
-	}{
-		{
-			expected: "/path_prefix_route1",
-			pathType: dataplane.PathTypePrefix,
-		},
-		{
-			expected: "/path_exact_route1",
-			pathType: dataplane.PathTypeExact,
-		},
-	}
-
-	for _, tc := range tests {
-		result := createPathForMatch("/path", tc.pathType, 1)
-		g.Expect(result).To(Equal(tc.expected))
-	}
-}
-
 func TestGenerateProxySetHeaders(t *testing.T) {
 	tests := []struct {
 		filters         *dataplane.HTTPFilters

diff --git a/internal/mode/static/nginx/config/validation/http_njs_match.go b/internal/mode/static/nginx/config/validation/http_njs_match.go
@@ -26,12 +26,7 @@ func (HTTPNJSMatchValidator) ValidatePathInMatch(path string) error {
 		return errors.New(msg)
 	}
 
-	// FIXME(pleshakov): This function will no longer be
-	// needed once https://github.com/nginxinc/nginx-gateway-fabric/issues/428 is fixed.
-	// That's because the location path gets into the set directive in the location block.
-	// Example: set $http_matches "[{\"redirectPath\":\"/coffee_route0\" ...
-	// Where /coffee is tha path.
-	return validateCommonNJSMatchPart(path)
+	return nil
 }
 
 func (HTTPNJSMatchValidator) ValidateHeaderNameInMatch(name string) error {

diff --git a/internal/mode/static/nginx/config/validation/http_njs_match_test.go b/internal/mode/static/nginx/config/validation/http_njs_match_test.go
@@ -13,6 +13,7 @@ func TestValidatePathInMatch(t *testing.T) {
 		"/",
 		"/path",
 		"/path/subpath-123",
+		"/route0-rule0",
 	)
 	testInvalidValuesForSimpleValidator(
 		t,
@@ -23,7 +24,6 @@ func TestValidatePathInMatch(t *testing.T) {
 		"/path;",
 		"path",
 		"",
-		"/path$",
 	)
 }