Prevent paths in HTTPRoute matches from conflicting with internal locations in NGINX

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 := createPathForMatch(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)
 		}
 	}
 
@@ -544,8 +538,8 @@ 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 createPathForMatch(ruleIdx, routeIdx int) string {
+	return fmt.Sprintf("@rule%d-route%d", ruleIdx, routeIdx)
 }
 
 func createDefaultRootLocation() http.Location {

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 }}

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,19 +553,19 @@ func TestCreateServers(t *testing.T) {
 
 		return []http.Location{
 			{
-				Path:            "/_prefix_route0",
+				Path:            "@rule0-route0",
 				Internal:        true,
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
 			{
-				Path:            "/_prefix_route1",
+				Path:            "@rule0-route1",
 				Internal:        true,
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 			},
 			{
-				Path:            "/_prefix_route2",
+				Path:            "@rule0-route2",
 				Internal:        true,
 				ProxyPass:       "http://test_foo_80$request_uri",
 				ProxySetHeaders: baseHeaders,
@@ -575,7 +575,7 @@ func TestCreateServers(t *testing.T) {
 				HTTPMatchVar: expectedMatchString(slashMatches),
 			},
 			{
-				Path:            "/test_prefix_route0",
+				Path:            "@rule1-route0",
 				Internal:        true,
 				ProxyPass:       "http://$test__route1_rule1$request_uri",
 				ProxySetHeaders: baseHeaders,
@@ -623,7 +623,7 @@ 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,
@@ -651,8 +651,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"},
+				Path:            "@rule7-route0",
+				Rewrites:        []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"},
 				Internal:        true,
 				ProxyPass:       "http://test_foo_80",
 				ProxySetHeaders: rewriteProxySetHeaders,
@@ -678,7 +678,7 @@ func TestCreateServers(t *testing.T) {
 				},
 			},
 			{
-				Path: "/invalid-filter-with-headers_prefix_route0",
+				Path: "@rule9-route0",
 				Return: &http.Return{
 					Code: http.StatusInternalServerError,
 				},
@@ -698,7 +698,7 @@ 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,
@@ -1274,8 +1274,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^ /full-path break",
+				Rewrite: "^ /full-path break",
 			},
 			msg: "full path",
 		},
@@ -1288,8 +1287,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 +1300,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 +1313,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) {
 				},
 			},
 			expected: &rewriteConfig{
-				InternalRewrite: "^ $request_uri",
-				MainRewrite:     "^/original(?:/(.*))?$ /$1 break",
+				Rewrite: "^/original(?:/(.*))?$ /$1 break",
 			},
 			msg: "prefix path /",
 		},
@@ -1330,8 +1326,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 +1339,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 +1352,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",
 		},
@@ -1711,17 +1704,17 @@ func TestCreatePathForMatch(t *testing.T) {
 		panic    bool
 	}{
 		{
-			expected: "/path_prefix_route1",
+			expected: "@rule0-route1",
 			pathType: dataplane.PathTypePrefix,
 		},
 		{
-			expected: "/path_exact_route1",
+			expected: "@rule0-route1",
 			pathType: dataplane.PathTypeExact,
 		},
 	}
 
 	for _, tc := range tests {
-		result := createPathForMatch("/path", tc.pathType, 1)
+		result := createPathForMatch(0, 1)
 		g.Expect(result).To(Equal(tc.expected))
 	}
 }

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 {

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$",
 	)
 }
 

internal/mode/static/nginx/modules/src/httpmatches.js

@@ -46,14 +46,7 @@ function redirect(r) {
     return;
   }
 
-  // If performing a rewrite, $request_uri won't be used,
-  // so we have to preserve args in the internal redirect.
-  let args = qs.stringify(r.args);
-  if (args) {
-    args = '?' + args;
-  }
-
-  r.internalRedirect(match.redirectPath + args);
+  r.internalRedirect(match.redirectPath);
 }
 
 function extractMatchesFromRequest(r) {

internal/mode/static/nginx/modules/test/httpmatches.test.js

@@ -403,7 +403,7 @@ describe('redirect', () => {
     method: 'GET',
     headers: ['header1:value1', 'header2:value2'],
     params: ['Arg1=value1', 'arg2=value2=SOME=other=value'],
-    redirectPath: '/a-match',
+    redirectPath: '/a-match?Arg1=value1&arg2=value2%3DSOME%3Dother%3Dvalue',
   };
 
   const tests = [