From 870e3fecf0dab964d9bb3a0cb27492fa7bc24984 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Wed, 20 Dec 2023 13:23:04 -0800 Subject: [PATCH 01/11] fix: ensure unique generated locations --- .../mode/static/nginx/config/http/config.go | 5 +++ internal/mode/static/nginx/config/servers.go | 12 +++--- .../mode/static/nginx/config/servers_test.go | 38 +++++++++---------- .../nginx/config/validation/http_njs_match.go | 17 ++++++--- .../config/validation/http_njs_match_test.go | 5 ++- .../overview/gateway-api-compatibility.md | 2 +- site/content/overview/resource-validation.md | 1 + 7 files changed, 47 insertions(+), 33 deletions(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index f99b3a6429..164c664813 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -86,3 +86,8 @@ type MapParameter struct { Value string Result string } + +const ( + // InternalLocationPrefix indicates that the path is internal type. + InternalLocationPrefix string = "/ngf-internal" +) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 58e2524929..9441239cca 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -106,7 +106,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 +118,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 +216,11 @@ func initializeExternalLocations( } func initializeInternalLocation( - rule dataplane.PathRule, + pathruleIdx int, 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) } @@ -544,8 +544,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("%s-rule%d-route%d", http.InternalLocationPrefix, ruleIdx, routeIdx) } func createDefaultRootLocation() http.Location { diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 91d3f9aae7..fc47787abf 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: "/ngf-internal-rule0-route0"}, + {Method: "PATCH", RedirectPath: "/ngf-internal-rule0-route1"}, + {Any: true, RedirectPath: "/ngf-internal-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: "/ngf-internal-rule1-route0", }, } exactMatches := []httpMatch{ { Method: "GET", - RedirectPath: "/test_exact_route0", + RedirectPath: "/ngf-internal-rule11-route0", }, } redirectHeaderMatches := []httpMatch{ { Headers: []string{"redirect:this"}, - RedirectPath: "/redirect-with-headers_prefix_route0", + RedirectPath: "/ngf-internal-rule5-route0", }, } rewriteHeaderMatches := []httpMatch{ { Headers: []string{"rewrite:this"}, - RedirectPath: "/rewrite-with-headers_prefix_route0", + RedirectPath: "/ngf-internal-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: "/ngf-internal-rule9-route0", }, } @@ -553,19 +553,19 @@ func TestCreateServers(t *testing.T) { return []http.Location{ { - Path: "/_prefix_route0", + Path: "/ngf-internal-rule0-route0", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "/_prefix_route1", + Path: "/ngf-internal-rule0-route1", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "/_prefix_route2", + Path: "/ngf-internal-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: "/ngf-internal-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: "/ngf-internal-rule5-route0", Return: &http.Return{ Body: "$scheme://foo.example.com:8080$request_uri", Code: 302, @@ -651,7 +651,7 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: rewriteProxySetHeaders, }, { - Path: "/rewrite-with-headers_prefix_route0", + Path: "/ngf-internal-rule7-route0", Rewrites: []string{"^ $request_uri", "^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, Internal: true, ProxyPass: "http://test_foo_80", @@ -678,7 +678,7 @@ func TestCreateServers(t *testing.T) { }, }, { - Path: "/invalid-filter-with-headers_prefix_route0", + Path: "/ngf-internal-rule9-route0", Return: &http.Return{ Code: http.StatusInternalServerError, }, @@ -698,7 +698,7 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: baseHeaders, }, { - Path: "/test_exact_route0", + Path: "/ngf-internal-rule11-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, Internal: true, @@ -1711,17 +1711,17 @@ func TestCreatePathForMatch(t *testing.T) { panic bool }{ { - expected: "/path_prefix_route1", + expected: "/ngf-internal-rule0-route1", pathType: dataplane.PathTypePrefix, }, { - expected: "/path_exact_route1", + expected: "/ngf-internal-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)) } } 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..7a8a2ad237 100644 --- a/internal/mode/static/nginx/config/validation/http_njs_match.go +++ b/internal/mode/static/nginx/config/validation/http_njs_match.go @@ -8,6 +8,7 @@ import ( k8svalidation "k8s.io/apimachinery/pkg/util/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" ) // HTTPNJSMatchValidator validates values used for matching a request. @@ -15,6 +16,10 @@ import ( // so changes to the implementation change the validation rules here. type HTTPNJSMatchValidator struct{} +const ( + internalPathErrMsgFmt = "paths starting with %q are reserved for internal use" +) + // ValidatePathInMatch a path used in the location directive. func (HTTPNJSMatchValidator) ValidatePathInMatch(path string) error { if path == "" { @@ -26,12 +31,12 @@ 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) + // should not match the prefix for internal location + if strings.HasPrefix(path, http.InternalLocationPrefix) { + return fmt.Errorf(internalPathErrMsgFmt, http.InternalLocationPrefix) + } + + 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..19abccb5f6 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,8 @@ func TestValidatePathInMatch(t *testing.T) { "/", "/path", "/path/subpath-123", + "/ngf-ngf-internal", + "/test-ngf-internal", ) testInvalidValuesForSimpleValidator( t, @@ -23,7 +25,8 @@ func TestValidatePathInMatch(t *testing.T) { "/path;", "path", "", - "/path$", + "/ngf-internal", + "/ngf-internal-test", ) } diff --git a/site/content/overview/gateway-api-compatibility.md b/site/content/overview/gateway-api-compatibility.md index b10133fe01..cf789e02c2 100644 --- a/site/content/overview/gateway-api-compatibility.md +++ b/site/content/overview/gateway-api-compatibility.md @@ -147,7 +147,7 @@ See the [static-mode]({{< relref "/reference/cli-help.md#static-mode">}}) comman - `hostnames`: Supported. - `rules` - `matches` - - `path`: Partially supported. Only `PathPrefix` and `Exact` types. + - `path`: Partially supported. Only `PathPrefix` and `Exact` types. The path cannot start with "/ngf-internal" as such paths are reserved for internal use. - `headers`: Partially supported. Only `Exact` type. - `queryParams`: Partially supported. Only `Exact` type. - `method`: Supported. diff --git a/site/content/overview/resource-validation.md b/site/content/overview/resource-validation.md index 2519aeef20..446711624c 100644 --- a/site/content/overview/resource-validation.md +++ b/site/content/overview/resource-validation.md @@ -92,6 +92,7 @@ This step catches the following cases of invalid values: - Valid values from the Gateway API perspective, but invalid for NGINX, because NGINX has stricter validation requirements for certain fields. These values will cause NGINX to fail to reload or operate erroneously. - Invalid values (both from the Gateway API and NGINX perspectives) that were not rejected because Step 1 was bypassed. Similar to the previous case, these values will cause NGINX to fail to reload or operate erroneously. - Malicious values that inject unrestricted NGINX config into the NGINX configuration (similar to an SQL injection attack). +- Valid values from the Gateway API perspective, but invalid for NGINX Gateway Fabric, because such values can prevent generating NGINX configuration. Below is an example of how NGINX Gateway Fabric rejects an invalid resource. The validation error is reported via the status: From 5c92c613ebe874786a51da38abae6d9fcfd1e2f8 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 10 Jan 2024 13:49:20 -0700 Subject: [PATCH 02/11] Update site/content/overview/resource-validation.md Co-authored-by: Alan Dooley --- site/content/overview/resource-validation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/content/overview/resource-validation.md b/site/content/overview/resource-validation.md index 446711624c..22afe1c2cb 100644 --- a/site/content/overview/resource-validation.md +++ b/site/content/overview/resource-validation.md @@ -90,9 +90,9 @@ This step catches the following cases of invalid values: - Valid values from the Gateway API perspective but not supported by NGINX Gateway Fabric yet. For example, a feature in an HTTPRoute routing rule. For the list of supported features see [Gateway API Compatibility](gateway-api-compatibility.md) doc. - Valid values from the Gateway API perspective, but invalid for NGINX, because NGINX has stricter validation requirements for certain fields. These values will cause NGINX to fail to reload or operate erroneously. +- Valid values from the Gateway API perspective, but invalid for NGINX Gateway Fabric, because such values can prevent generating NGINX configuration. - Invalid values (both from the Gateway API and NGINX perspectives) that were not rejected because Step 1 was bypassed. Similar to the previous case, these values will cause NGINX to fail to reload or operate erroneously. - Malicious values that inject unrestricted NGINX config into the NGINX configuration (similar to an SQL injection attack). -- Valid values from the Gateway API perspective, but invalid for NGINX Gateway Fabric, because such values can prevent generating NGINX configuration. Below is an example of how NGINX Gateway Fabric rejects an invalid resource. The validation error is reported via the status: From 7656a7a99422781a9dc0a24afbeae962dcb3701b Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 10 Jan 2024 13:51:13 -0700 Subject: [PATCH 03/11] Update internal/mode/static/nginx/config/http/config.go Co-authored-by: Michael Pleshakov --- internal/mode/static/nginx/config/http/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 164c664813..a0af23888d 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -88,6 +88,6 @@ type MapParameter struct { } const ( - // InternalLocationPrefix indicates that the path is internal type. + // InternalLocationPrefix is used as a prefix for internal locations to avoid path conflicts with HTTPRoute routing rules. InternalLocationPrefix string = "/ngf-internal" ) From 8701e6544548ecf033186708edd1872ea2aba8f9 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Wed, 10 Jan 2024 14:30:59 -0700 Subject: [PATCH 04/11] update location block name --- .../mode/static/nginx/config/http/config.go | 3 +- internal/mode/static/nginx/config/servers.go | 2 +- .../mode/static/nginx/config/servers_test.go | 36 +++++++++---------- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index a0af23888d..eff2c5986d 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -88,6 +88,7 @@ type MapParameter struct { } const ( - // InternalLocationPrefix is used as a prefix for internal locations to avoid path conflicts with HTTPRoute routing rules. + // InternalLocationPrefix is used as a prefix for internal locations + // to avoid path conflicts with HTTPRoute routing rules. InternalLocationPrefix string = "/ngf-internal" ) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 9441239cca..7d670e36ab 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -545,7 +545,7 @@ func createPath(rule dataplane.PathRule) string { } func createPathForMatch(ruleIdx, routeIdx int) string { - return fmt.Sprintf("%s-rule%d-route%d", http.InternalLocationPrefix, ruleIdx, routeIdx) + return fmt.Sprintf("rule%d-route%d", ruleIdx, routeIdx) } func createDefaultRootLocation() http.Location { diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index fc47787abf..01b4f2b64c 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: "/ngf-internal-rule0-route0"}, - {Method: "PATCH", RedirectPath: "/ngf-internal-rule0-route1"}, - {Any: true, RedirectPath: "/ngf-internal-rule0-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: "/ngf-internal-rule1-route0", + RedirectPath: "rule1-route0", }, } exactMatches := []httpMatch{ { Method: "GET", - RedirectPath: "/ngf-internal-rule11-route0", + RedirectPath: "rule11-route0", }, } redirectHeaderMatches := []httpMatch{ { Headers: []string{"redirect:this"}, - RedirectPath: "/ngf-internal-rule5-route0", + RedirectPath: "rule5-route0", }, } rewriteHeaderMatches := []httpMatch{ { Headers: []string{"rewrite:this"}, - RedirectPath: "/ngf-internal-rule7-route0", + RedirectPath: "rule7-route0", }, } rewriteProxySetHeaders := []http.Header{ @@ -541,7 +541,7 @@ func TestCreateServers(t *testing.T) { invalidFilterHeaderMatches := []httpMatch{ { Headers: []string{"filter:this"}, - RedirectPath: "/ngf-internal-rule9-route0", + RedirectPath: "rule9-route0", }, } @@ -553,19 +553,19 @@ func TestCreateServers(t *testing.T) { return []http.Location{ { - Path: "/ngf-internal-rule0-route0", + Path: "rule0-route0", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "/ngf-internal-rule0-route1", + Path: "rule0-route1", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "/ngf-internal-rule0-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: "/ngf-internal-rule1-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: "/ngf-internal-rule5-route0", + Path: "rule5-route0", Return: &http.Return{ Body: "$scheme://foo.example.com:8080$request_uri", Code: 302, @@ -651,7 +651,7 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: rewriteProxySetHeaders, }, { - Path: "/ngf-internal-rule7-route0", + Path: "rule7-route0", Rewrites: []string{"^ $request_uri", "^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, Internal: true, ProxyPass: "http://test_foo_80", @@ -678,7 +678,7 @@ func TestCreateServers(t *testing.T) { }, }, { - Path: "/ngf-internal-rule9-route0", + Path: "rule9-route0", Return: &http.Return{ Code: http.StatusInternalServerError, }, @@ -698,7 +698,7 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: baseHeaders, }, { - Path: "/ngf-internal-rule11-route0", + Path: "rule11-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, Internal: true, @@ -1711,11 +1711,11 @@ func TestCreatePathForMatch(t *testing.T) { panic bool }{ { - expected: "/ngf-internal-rule0-route1", + expected: "rule0-route1", pathType: dataplane.PathTypePrefix, }, { - expected: "/ngf-internal-rule0-route1", + expected: "rule0-route1", pathType: dataplane.PathTypeExact, }, } From b4459ad229e2682b426224f05b6353bab69e6237 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Wed, 10 Jan 2024 15:26:53 -0700 Subject: [PATCH 05/11] remove prefix check --- internal/mode/static/nginx/config/http/config.go | 6 ------ .../static/nginx/config/validation/http_njs_match.go | 10 ---------- site/content/overview/gateway-api-compatibility.md | 2 +- 3 files changed, 1 insertion(+), 17 deletions(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index eff2c5986d..f99b3a6429 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -86,9 +86,3 @@ type MapParameter struct { Value string Result string } - -const ( - // InternalLocationPrefix is used as a prefix for internal locations - // to avoid path conflicts with HTTPRoute routing rules. - InternalLocationPrefix string = "/ngf-internal" -) 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 7a8a2ad237..bad9c548ff 100644 --- a/internal/mode/static/nginx/config/validation/http_njs_match.go +++ b/internal/mode/static/nginx/config/validation/http_njs_match.go @@ -8,7 +8,6 @@ import ( k8svalidation "k8s.io/apimachinery/pkg/util/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" ) // HTTPNJSMatchValidator validates values used for matching a request. @@ -16,10 +15,6 @@ import ( // so changes to the implementation change the validation rules here. type HTTPNJSMatchValidator struct{} -const ( - internalPathErrMsgFmt = "paths starting with %q are reserved for internal use" -) - // ValidatePathInMatch a path used in the location directive. func (HTTPNJSMatchValidator) ValidatePathInMatch(path string) error { if path == "" { @@ -31,11 +26,6 @@ func (HTTPNJSMatchValidator) ValidatePathInMatch(path string) error { return errors.New(msg) } - // should not match the prefix for internal location - if strings.HasPrefix(path, http.InternalLocationPrefix) { - return fmt.Errorf(internalPathErrMsgFmt, http.InternalLocationPrefix) - } - return nil } diff --git a/site/content/overview/gateway-api-compatibility.md b/site/content/overview/gateway-api-compatibility.md index cf789e02c2..b10133fe01 100644 --- a/site/content/overview/gateway-api-compatibility.md +++ b/site/content/overview/gateway-api-compatibility.md @@ -147,7 +147,7 @@ See the [static-mode]({{< relref "/reference/cli-help.md#static-mode">}}) comman - `hostnames`: Supported. - `rules` - `matches` - - `path`: Partially supported. Only `PathPrefix` and `Exact` types. The path cannot start with "/ngf-internal" as such paths are reserved for internal use. + - `path`: Partially supported. Only `PathPrefix` and `Exact` types. - `headers`: Partially supported. Only `Exact` type. - `queryParams`: Partially supported. Only `Exact` type. - `method`: Supported. From 48d266e873d61847cb861fc5ad3d522f12a8d8d9 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Wed, 10 Jan 2024 15:42:51 -0700 Subject: [PATCH 06/11] update location prefix documentation --- .../static/nginx/config/validation/http_njs_match_test.go | 5 +---- site/content/overview/resource-validation.md | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) 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 19abccb5f6..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,8 +13,7 @@ func TestValidatePathInMatch(t *testing.T) { "/", "/path", "/path/subpath-123", - "/ngf-ngf-internal", - "/test-ngf-internal", + "/route0-rule0", ) testInvalidValuesForSimpleValidator( t, @@ -25,8 +24,6 @@ func TestValidatePathInMatch(t *testing.T) { "/path;", "path", "", - "/ngf-internal", - "/ngf-internal-test", ) } diff --git a/site/content/overview/resource-validation.md b/site/content/overview/resource-validation.md index 22afe1c2cb..2519aeef20 100644 --- a/site/content/overview/resource-validation.md +++ b/site/content/overview/resource-validation.md @@ -90,7 +90,6 @@ This step catches the following cases of invalid values: - Valid values from the Gateway API perspective but not supported by NGINX Gateway Fabric yet. For example, a feature in an HTTPRoute routing rule. For the list of supported features see [Gateway API Compatibility](gateway-api-compatibility.md) doc. - Valid values from the Gateway API perspective, but invalid for NGINX, because NGINX has stricter validation requirements for certain fields. These values will cause NGINX to fail to reload or operate erroneously. -- Valid values from the Gateway API perspective, but invalid for NGINX Gateway Fabric, because such values can prevent generating NGINX configuration. - Invalid values (both from the Gateway API and NGINX perspectives) that were not rejected because Step 1 was bypassed. Similar to the previous case, these values will cause NGINX to fail to reload or operate erroneously. - Malicious values that inject unrestricted NGINX config into the NGINX configuration (similar to an SQL injection attack). From d5343ac1b2f1c2f6935d294c8c1cca47cfe1eecb Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Thu, 11 Jan 2024 15:40:25 -0700 Subject: [PATCH 07/11] update location block rules --- internal/mode/static/nginx/config/servers.go | 20 +++---- .../mode/static/nginx/config/servers_test.go | 59 ++++++++----------- 2 files changed, 33 insertions(+), 46 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 7d670e36ab..8b6ebf7f5e 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 { @@ -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) } } @@ -545,7 +539,7 @@ func createPath(rule dataplane.PathRule) string { } func createPathForMatch(ruleIdx, routeIdx int) string { - return fmt.Sprintf("rule%d-route%d", ruleIdx, routeIdx) + return fmt.Sprintf("@rule%d-route%d", ruleIdx, routeIdx) } func createDefaultRootLocation() http.Location { diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 01b4f2b64c..28985ce203 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: "rule0-route0"}, - {Method: "PATCH", RedirectPath: "rule0-route1"}, - {Any: true, RedirectPath: "rule0-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: "rule1-route0", + RedirectPath: "@rule1-route0", }, } exactMatches := []httpMatch{ { Method: "GET", - RedirectPath: "rule11-route0", + RedirectPath: "@rule11-route0", }, } redirectHeaderMatches := []httpMatch{ { Headers: []string{"redirect:this"}, - RedirectPath: "rule5-route0", + RedirectPath: "@rule5-route0", }, } rewriteHeaderMatches := []httpMatch{ { Headers: []string{"rewrite:this"}, - RedirectPath: "rule7-route0", + RedirectPath: "@rule7-route0", }, } rewriteProxySetHeaders := []http.Header{ @@ -541,7 +541,7 @@ func TestCreateServers(t *testing.T) { invalidFilterHeaderMatches := []httpMatch{ { Headers: []string{"filter:this"}, - RedirectPath: "rule9-route0", + RedirectPath: "@rule9-route0", }, } @@ -553,19 +553,19 @@ func TestCreateServers(t *testing.T) { return []http.Location{ { - Path: "rule0-route0", + Path: "@rule0-route0", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "rule0-route1", + Path: "@rule0-route1", Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { - Path: "rule0-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: "rule1-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: "rule5-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: "rule7-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: "rule9-route0", + Path: "@rule9-route0", Return: &http.Return{ Code: http.StatusInternalServerError, }, @@ -698,7 +698,7 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: baseHeaders, }, { - Path: "rule11-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,11 +1704,11 @@ func TestCreatePathForMatch(t *testing.T) { panic bool }{ { - expected: "rule0-route1", + expected: "@rule0-route1", pathType: dataplane.PathTypePrefix, }, { - expected: "rule0-route1", + expected: "@rule0-route1", pathType: dataplane.PathTypeExact, }, } From f3fae104bc1b21de261a65ce4fbf65e3b7d91b37 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Fri, 12 Jan 2024 10:48:32 -0700 Subject: [PATCH 08/11] remove internal rewrite check --- internal/mode/static/nginx/modules/src/httpmatches.js | 9 +-------- .../mode/static/nginx/modules/test/httpmatches.test.js | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) 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..65685ea33a 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/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 = [ From e6ff4eb272499e7f445e273a6ffcd252b0cb3274 Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Tue, 16 Jan 2024 10:32:31 -0700 Subject: [PATCH 09/11] remove internal directive for location block --- internal/mode/static/nginx/config/servers.go | 2 +- internal/mode/static/nginx/config/servers_template.go | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 8b6ebf7f5e..a150068202 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -214,7 +214,7 @@ func initializeExternalLocations( } func initializeInternalLocation( - pathruleIdx int, + pathruleIdx, matchRuleIdx int, match dataplane.Match, ) (http.Location, httpMatch) { 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 }} From 84f47f0a8dd833feb321ba6654b47860e9614f9e Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Tue, 16 Jan 2024 12:12:45 -0700 Subject: [PATCH 10/11] simplify internalizeInternalLocations --- .../mode/static/nginx/config/http/config.go | 1 - internal/mode/static/nginx/config/servers.go | 9 ++--- .../mode/static/nginx/config/servers_test.go | 35 +------------------ 3 files changed, 3 insertions(+), 42 deletions(-) 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 a150068202..b4cdeb4779 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -218,7 +218,7 @@ func initializeInternalLocation( matchRuleIdx int, match dataplane.Match, ) (http.Location, httpMatch) { - path := createPathForMatch(pathruleIdx, matchRuleIdx) + path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx) return createMatchLocation(path), createHTTPMatch(match, path) } @@ -443,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, } } @@ -538,10 +537,6 @@ func createPath(rule dataplane.PathRule) string { } } -func createPathForMatch(ruleIdx, routeIdx int) string { - return fmt.Sprintf("@rule%d-route%d", ruleIdx, routeIdx) -} - func createDefaultRootLocation() http.Location { return http.Location{ Path: "/", diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 28985ce203..a78259353e 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -554,19 +554,16 @@ func TestCreateServers(t *testing.T) { return []http.Location{ { Path: "@rule0-route0", - Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { Path: "@rule0-route1", - Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, { Path: "@rule0-route2", - Internal: true, ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, }, @@ -576,7 +573,6 @@ func TestCreateServers(t *testing.T) { }, { Path: "@rule1-route0", - Internal: true, ProxyPass: "http://$test__route1_rule1$request_uri", ProxySetHeaders: baseHeaders, }, @@ -628,7 +624,6 @@ func TestCreateServers(t *testing.T) { Body: "$scheme://foo.example.com:8080$request_uri", Code: 302, }, - Internal: true, }, { Path: "/redirect-with-headers/", @@ -653,7 +648,6 @@ func TestCreateServers(t *testing.T) { { Path: "@rule7-route0", Rewrites: []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, - Internal: true, ProxyPass: "http://test_foo_80", ProxySetHeaders: rewriteProxySetHeaders, }, @@ -682,7 +676,6 @@ func TestCreateServers(t *testing.T) { Return: &http.Return{ Code: http.StatusInternalServerError, }, - Internal: true, }, { Path: "/invalid-filter-with-headers/", @@ -701,7 +694,6 @@ func TestCreateServers(t *testing.T) { Path: "@rule11-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: baseHeaders, - Internal: true, }, { Path: "= /test", @@ -1687,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: "@rule0-route1", - pathType: dataplane.PathTypePrefix, - }, - { - expected: "@rule0-route1", - pathType: dataplane.PathTypeExact, - }, - } - - for _, tc := range tests { - result := createPathForMatch(0, 1) - g.Expect(result).To(Equal(tc.expected)) - } -} - func TestGenerateProxySetHeaders(t *testing.T) { tests := []struct { filters *dataplane.HTTPFilters From bba2edfe04b64348508c8ec6319444f94986b54b Mon Sep 17 00:00:00 2001 From: "sa.choudhary" Date: Tue, 16 Jan 2024 14:14:55 -0700 Subject: [PATCH 11/11] fix unit tests --- internal/mode/static/nginx/modules/test/httpmatches.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index 65685ea33a..669638b819 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/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?Arg1=value1&arg2=value2%3DSOME%3Dother%3Dvalue', + redirectPath: '/a-match', }; const tests = [ @@ -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', }, ];