diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index f99b3a6429..59ce01d3f6 100644 --- 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 index 58e2524929..b4cdeb4779 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -95,10 +95,8 @@ 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 { @@ -106,7 +104,7 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http. 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 index 7574efb1b8..9afa4cdf04 100644 --- 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 index 91d3f9aae7..a78259353e 100644 --- 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 index 0db9ce6d9e..bad9c548ff 100644 --- 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 index 0fd99ae5b9..942d944af8 100644 --- 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$", ) } diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 1f36ae57ec..0fcf5ef210 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/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) { diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index ac020abba9..669638b819 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -439,7 +439,7 @@ describe('redirect', () => { params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, }), matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. - expectedRedirect: '/a-match?Arg1=value1&arg2=value2%3DSOME%3Dother%3Dvalue', + expectedRedirect: '/a-match', }, ];