diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index 948654d33e..69233691bd 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -134,8 +134,8 @@ spec: mountPath: /etc/nginx/conf.d - name: nginx-stream-conf mountPath: /etc/nginx/stream-conf.d - - name: module-includes - mountPath: /etc/nginx/module-includes + - name: nginx-main-includes + mountPath: /etc/nginx/main-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -173,8 +173,8 @@ spec: mountPath: /etc/nginx/conf.d - name: nginx-stream-conf mountPath: /etc/nginx/stream-conf.d - - name: module-includes - mountPath: /etc/nginx/module-includes + - name: nginx-main-includes + mountPath: /etc/nginx/main-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -209,7 +209,7 @@ spec: emptyDir: {} - name: nginx-stream-conf emptyDir: {} - - name: module-includes + - name: nginx-main-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/config/tests/static-deployment.yaml b/config/tests/static-deployment.yaml index bb2fb62765..30f85a9081 100644 --- a/config/tests/static-deployment.yaml +++ b/config/tests/static-deployment.yaml @@ -74,8 +74,8 @@ spec: mountPath: /etc/nginx/conf.d - name: nginx-stream-conf mountPath: /etc/nginx/stream-conf.d - - name: module-includes - mountPath: /etc/nginx/module-includes + - name: nginx-main-includes + mountPath: /etc/nginx/main-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -106,8 +106,8 @@ spec: mountPath: /etc/nginx/conf.d - name: nginx-stream-conf mountPath: /etc/nginx/stream-conf.d - - name: module-includes - mountPath: /etc/nginx/module-includes + - name: nginx-main-includes + mountPath: /etc/nginx/main-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -127,7 +127,7 @@ spec: emptyDir: {} - name: nginx-stream-conf emptyDir: {} - - name: module-includes + - name: nginx-main-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/deploy/aws-nlb/deploy.yaml b/deploy/aws-nlb/deploy.yaml index 49b29bf988..e363b9066a 100644 --- a/deploy/aws-nlb/deploy.yaml +++ b/deploy/aws-nlb/deploy.yaml @@ -248,8 +248,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -280,8 +280,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -302,7 +302,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/azure/deploy.yaml b/deploy/azure/deploy.yaml index 968c1a2926..faacc0332e 100644 --- a/deploy/azure/deploy.yaml +++ b/deploy/azure/deploy.yaml @@ -245,8 +245,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -277,8 +277,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -301,7 +301,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/default/deploy.yaml b/deploy/default/deploy.yaml index 6245a2bbc7..97e03edb4c 100644 --- a/deploy/default/deploy.yaml +++ b/deploy/default/deploy.yaml @@ -245,8 +245,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -277,8 +277,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -299,7 +299,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/experimental-nginx-plus/deploy.yaml b/deploy/experimental-nginx-plus/deploy.yaml index ed9c748e1a..002027468d 100644 --- a/deploy/experimental-nginx-plus/deploy.yaml +++ b/deploy/experimental-nginx-plus/deploy.yaml @@ -260,8 +260,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -292,8 +292,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -314,7 +314,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/experimental/deploy.yaml b/deploy/experimental/deploy.yaml index 28cc7b6d19..fbb09e917a 100644 --- a/deploy/experimental/deploy.yaml +++ b/deploy/experimental/deploy.yaml @@ -251,8 +251,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -283,8 +283,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -305,7 +305,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/nginx-plus/deploy.yaml b/deploy/nginx-plus/deploy.yaml index 76249e80c2..33043d74d5 100644 --- a/deploy/nginx-plus/deploy.yaml +++ b/deploy/nginx-plus/deploy.yaml @@ -256,8 +256,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -288,8 +288,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -310,7 +310,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/nodeport/deploy.yaml b/deploy/nodeport/deploy.yaml index db81fdf259..d45f2c51c2 100644 --- a/deploy/nodeport/deploy.yaml +++ b/deploy/nodeport/deploy.yaml @@ -245,8 +245,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -277,8 +277,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -299,7 +299,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/openshift/deploy.yaml b/deploy/openshift/deploy.yaml index cb78ce0f39..742441ad18 100644 --- a/deploy/openshift/deploy.yaml +++ b/deploy/openshift/deploy.yaml @@ -253,8 +253,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -285,8 +285,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -307,7 +307,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/snippets-filters-nginx-plus/deploy.yaml b/deploy/snippets-filters-nginx-plus/deploy.yaml index b0f42c3f6a..dfb0bcc72e 100644 --- a/deploy/snippets-filters-nginx-plus/deploy.yaml +++ b/deploy/snippets-filters-nginx-plus/deploy.yaml @@ -257,8 +257,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -289,8 +289,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -311,7 +311,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/deploy/snippets-filters/deploy.yaml b/deploy/snippets-filters/deploy.yaml index 7cabc5994a..d48e4b3f3c 100644 --- a/deploy/snippets-filters/deploy.yaml +++ b/deploy/snippets-filters/deploy.yaml @@ -248,8 +248,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -280,8 +280,8 @@ spec: name: nginx-conf - mountPath: /etc/nginx/stream-conf.d name: nginx-stream-conf - - mountPath: /etc/nginx/module-includes - name: module-includes + - mountPath: /etc/nginx/main-includes + name: nginx-main-includes - mountPath: /etc/nginx/secrets name: nginx-secrets - mountPath: /var/run/nginx @@ -302,7 +302,7 @@ spec: - emptyDir: {} name: nginx-stream-conf - emptyDir: {} - name: module-includes + name: nginx-main-includes - emptyDir: {} name: nginx-secrets - emptyDir: {} diff --git a/examples/snippets-filter/example.yaml b/examples/snippets-filter/example.yaml new file mode 100644 index 0000000000..a244ca6b84 --- /dev/null +++ b/examples/snippets-filter/example.yaml @@ -0,0 +1,86 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 1 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway +spec: + gatewayClassName: nginx + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "*.example.com" +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: coffee +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: PathPrefix + value: /coffee + filters: + - type: ExtensionRef + extensionRef: + group: gateway.nginx.org + kind: SnippetsFilter + name: test-all-contexts + backendRefs: + - name: coffee + port: 80 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: SnippetsFilter +metadata: + name: test-all-contexts +spec: + snippets: + - context: main + value: worker_shutdown_timeout 120s; + - context: http + value: aio on; + - context: http.server + value: auth_delay 10s; + - context: http.server.location + value: | + allow 10.0.0.0/8; + deny all; diff --git a/examples/snippets-filter/snippets-filter.yaml b/examples/snippets-filter/snippets-filter.yaml deleted file mode 100644 index cefe9a6ccb..0000000000 --- a/examples/snippets-filter/snippets-filter.yaml +++ /dev/null @@ -1,10 +0,0 @@ -apiVersion: gateway.nginx.org/v1alpha1 -kind: SnippetsFilter -metadata: - name: access-control -spec: - snippets: - - context: http.server.location - value: | - allow 10.0.0.0/8; - deny all; diff --git a/internal/framework/kinds/kinds.go b/internal/framework/kinds/kinds.go index 471c526b98..7700ad39ce 100644 --- a/internal/framework/kinds/kinds.go +++ b/internal/framework/kinds/kinds.go @@ -31,6 +31,8 @@ const ( ObservabilityPolicy = "ObservabilityPolicy" // NginxProxy is the NginxProxy kind. NginxProxy = "NginxProxy" + // SnippetsFilter is the SnippetsFilter kind. + SnippetsFilter = "SnippetsFilter" ) // MustExtractGVK is a function that extracts the GroupVersionKind (GVK) of a client.object. diff --git a/internal/mode/static/nginx/conf/nginx-plus.conf b/internal/mode/static/nginx/conf/nginx-plus.conf index 23d97717c9..08cae40eb7 100644 --- a/internal/mode/static/nginx/conf/nginx-plus.conf +++ b/internal/mode/static/nginx/conf/nginx-plus.conf @@ -1,5 +1,5 @@ load_module /usr/lib/nginx/modules/ngx_http_js_module.so; -include /etc/nginx/module-includes/*.conf; +include /etc/nginx/main-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/conf/nginx.conf b/internal/mode/static/nginx/conf/nginx.conf index 2cbc09fa3f..9f12a7f8e8 100644 --- a/internal/mode/static/nginx/conf/nginx.conf +++ b/internal/mode/static/nginx/conf/nginx.conf @@ -1,5 +1,5 @@ load_module /usr/lib/nginx/modules/ngx_http_js_module.so; -include /etc/nginx/module-includes/*.conf; +include /etc/nginx/main-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/config/base_http_config.go b/internal/mode/static/nginx/config/base_http_config.go index abec446492..4db43b78b0 100644 --- a/internal/mode/static/nginx/config/base_http_config.go +++ b/internal/mode/static/nginx/config/base_http_config.go @@ -4,16 +4,31 @@ import ( gotemplate "text/template" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) var baseHTTPTemplate = gotemplate.Must(gotemplate.New("baseHttp").Parse(baseHTTPTemplateText)) +type httpConfig struct { + Includes []shared.Include + HTTP2 bool +} + func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { - result := executeResult{ - dest: httpConfigFile, - data: helpers.MustExecuteTemplate(baseHTTPTemplate, conf.BaseHTTPConfig), + includes := createIncludesFromSnippets(conf.BaseHTTPConfig.Snippets) + + hc := httpConfig{ + HTTP2: conf.BaseHTTPConfig.HTTP2, + Includes: includes, } - return []executeResult{result} + results := make([]executeResult, 0, len(includes)+1) + results = append(results, executeResult{ + dest: httpConfigFile, + data: helpers.MustExecuteTemplate(baseHTTPTemplate, hc), + }) + results = append(results, createIncludeExecuteResults(includes)...) + + return results } diff --git a/internal/mode/static/nginx/config/base_http_config_template.go b/internal/mode/static/nginx/config/base_http_config_template.go index bbe35a1018..5163904e26 100644 --- a/internal/mode/static/nginx/config/base_http_config_template.go +++ b/internal/mode/static/nginx/config/base_http_config_template.go @@ -23,4 +23,8 @@ map $http_upgrade $connection_upgrade { map $request_uri $request_uri_path { "~^(?P[^?]*)(\?.*)?$" $path; } + +{{ range $i := .Includes -}} +include {{ $i.Name }}; +{{ end -}} ` diff --git a/internal/mode/static/nginx/config/base_http_config_test.go b/internal/mode/static/nginx/config/base_http_config_test.go index f06c4aa725..ef5a2f4ad1 100644 --- a/internal/mode/static/nginx/config/base_http_config_test.go +++ b/internal/mode/static/nginx/config/base_http_config_test.go @@ -1,6 +1,7 @@ package config import ( + "sort" "strings" "testing" @@ -9,7 +10,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) -func TestExecuteBaseHttp(t *testing.T) { +func TestExecuteBaseHttp_HTTP2(t *testing.T) { t.Parallel() confOn := dataplane.Configuration{ BaseHTTPConfig: dataplane.BaseHTTPConfig{ @@ -56,3 +57,53 @@ func TestExecuteBaseHttp(t *testing.T) { }) } } + +func TestExecuteBaseHttp_Snippets(t *testing.T) { + t.Parallel() + + conf := dataplane.Configuration{ + BaseHTTPConfig: dataplane.BaseHTTPConfig{ + Snippets: []dataplane.Snippet{ + { + Name: "snippet1", + Contents: "contents1", + }, + { + Name: "snippet2", + Contents: "contents2", + }, + }, + }, + } + + g := NewWithT(t) + + res := executeBaseHTTPConfig(conf) + g.Expect(res).To(HaveLen(3)) + + sort.Slice( + res, func(i, j int) bool { + return res[i].dest < res[j].dest + }, + ) + + /* + Order of files: + /etc/nginx/conf.d/http.conf + /etc/nginx/includes/snippet1.conf + /etc/nginx/includes/snippet2.conf + */ + + httpRes := string(res[0].data) + g.Expect(httpRes).To(ContainSubstring("map $http_host $gw_api_compliant_host {")) + g.Expect(httpRes).To(ContainSubstring("map $http_upgrade $connection_upgrade {")) + g.Expect(httpRes).To(ContainSubstring("map $request_uri $request_uri_path {")) + g.Expect(httpRes).To(ContainSubstring("include /etc/nginx/includes/snippet1.conf;")) + g.Expect(httpRes).To(ContainSubstring("include /etc/nginx/includes/snippet2.conf;")) + + snippet1IncludeRes := string(res[1].data) + g.Expect(snippet1IncludeRes).To(ContainSubstring("contents1")) + + snippet2IncludeRes := string(res[2].data) + g.Expect(snippet2IncludeRes).To(ContainSubstring("contents2")) +} diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index be2877bf29..fe7fc14ccb 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -23,8 +23,9 @@ const ( // streamFolder is the folder where NGINX Stream configuration files are stored. streamFolder = configFolder + "/stream-conf.d" - // modulesIncludesFolder is the folder where the included "load_module" file is stored. - modulesIncludesFolder = configFolder + "/module-includes" + // mainIncludesFolder is the folder where the main context configuration files are stored. + // For example, these files include load_module directives and snippets that target the main context. + mainIncludesFolder = configFolder + "/main-includes" // secretsFolder is the folder where secrets (like TLS certs/keys) are stored. secretsFolder = configFolder + "/secrets" @@ -44,12 +45,12 @@ const ( // httpMatchVarsFile is the path to the http_match pairs configuration file. httpMatchVarsFile = httpFolder + "/matches.json" - // loadModulesFile is the path to the file containing any load_module directives. - loadModulesFile = modulesIncludesFolder + "/load-modules.conf" + // mainIncludeFile is the path to the file included in the main context. + mainIncludeFile = mainIncludesFolder + "/main.conf" ) // ConfigFolders is a list of folders where NGINX configuration files are stored. -var ConfigFolders = []string{httpFolder, secretsFolder, includesFolder, modulesIncludesFolder, streamFolder} +var ConfigFolders = []string{httpFolder, secretsFolder, includesFolder, mainIncludesFolder, streamFolder} // Generator generates NGINX configuration files. // This interface is used for testing purposes only. @@ -60,9 +61,7 @@ type Generator interface { // GeneratorImpl is an implementation of Generator. // -// It generates files to be written to the following locations, which must exist and available for writing: -// - httpFolder, for HTTP configuration files. -// - secretsFolder, for secrets. +// It generates files to be written to the ConfigFolders locations, which must exist and available for writing. // // It also expects that the main NGINX configuration file nginx.conf is located in configFolder and nginx.conf // includes (https://nginx.org/en/docs/ngx_core_module.html#include) the files from httpFolder. @@ -99,49 +98,16 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { observability.NewGenerator(conf.Telemetry), ) - files = append(files, g.generateHTTPConfig(conf, policyGenerator)...) - - files = append(files, generateConfigVersion(conf.Version)) + files = append(files, g.executeConfigTemplates(conf, policyGenerator)...) for id, bundle := range conf.CertBundles { files = append(files, generateCertBundle(id, bundle)) } - files = append(files, generateLoadModulesConf(conf)) - return files } -func generatePEM(id dataplane.SSLKeyPairID, cert []byte, key []byte) file.File { - c := make([]byte, 0, len(cert)+len(key)+1) - c = append(c, cert...) - c = append(c, '\n') - c = append(c, key...) - - return file.File{ - Content: c, - Path: generatePEMFileName(id), - Type: file.TypeSecret, - } -} - -func generatePEMFileName(id dataplane.SSLKeyPairID) string { - return filepath.Join(secretsFolder, string(id)+".pem") -} - -func generateCertBundle(id dataplane.CertBundleID, cert []byte) file.File { - return file.File{ - Content: cert, - Path: generateCertBundleFileName(id), - Type: file.TypeRegular, - } -} - -func generateCertBundleFileName(id dataplane.CertBundleID) string { - return filepath.Join(secretsFolder, string(id)+".crt") -} - -func (g GeneratorImpl) generateHTTPConfig( +func (g GeneratorImpl) executeConfigTemplates( conf dataplane.Configuration, generator policies.Generator, ) []file.File { @@ -168,6 +134,7 @@ func (g GeneratorImpl) generateHTTPConfig( func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFunc { return []executeFunc{ + executeMainConfig, executeBaseHTTPConfig, g.newExecuteServersFunc(generator), g.executeUpstreams, @@ -177,29 +144,35 @@ func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFu g.executeStreamServers, g.executeStreamUpstreams, executeStreamMaps, + executeVersion, } } -// generateConfigVersion writes the config version file. -func generateConfigVersion(configVersion int) file.File { - c := executeVersion(configVersion) +func generatePEM(id dataplane.SSLKeyPairID, cert []byte, key []byte) file.File { + c := make([]byte, 0, len(cert)+len(key)+1) + c = append(c, cert...) + c = append(c, '\n') + c = append(c, key...) return file.File{ Content: c, - Path: configVersionFile, - Type: file.TypeRegular, + Path: generatePEMFileName(id), + Type: file.TypeSecret, } } -func generateLoadModulesConf(conf dataplane.Configuration) file.File { - var c []byte - if conf.Telemetry.Endpoint != "" { - c = []byte("load_module modules/ngx_otel_module.so;") - } +func generatePEMFileName(id dataplane.SSLKeyPairID) string { + return filepath.Join(secretsFolder, string(id)+".pem") +} +func generateCertBundle(id dataplane.CertBundleID, cert []byte) file.File { return file.File{ - Content: c, - Path: loadModulesFile, + Content: cert, + Path: generateCertBundleFileName(id), Type: file.TypeRegular, } } + +func generateCertBundleFileName(id dataplane.CertBundleID) string { + return filepath.Join(secretsFolder, string(id)+".crt") +} diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 4940d72452..8a56042d6f 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -92,6 +92,26 @@ func TestGenerate(t *testing.T) { }, BaseHTTPConfig: dataplane.BaseHTTPConfig{ HTTP2: true, + Snippets: []dataplane.Snippet{ + { + Name: "http_snippet1", + Contents: "http snippet 1 contents", + }, + { + Name: "http_snippet2", + Contents: "http 2 contents", + }, + }, + }, + MainSnippets: []dataplane.Snippet{ + { + Name: "main_snippet1", + Contents: "main snippet 1 contents", + }, + { + Name: "main_snippet2", + Contents: "main 2 contents", + }, }, } g := NewWithT(t) @@ -101,12 +121,27 @@ func TestGenerate(t *testing.T) { files := generator.Generate(conf) - g.Expect(files).To(HaveLen(7)) + g.Expect(files).To(HaveLen(11)) arrange := func(i, j int) bool { return files[i].Path < files[j].Path } sort.Slice(files, arrange) + /* + Order of files: + /etc/nginx/conf.d/config-version.conf + /etc/nginx/conf.d/http.conf + /etc/nginx/conf.d/matches.json + /etc/nginx/includes/http_snippet1.conf + /etc/nginx/includes/http_snippet2.conf + /etc/nginx/includes/main_snippet1.conf + /etc/nginx/includes/main_snippet2.conf + /etc/nginx/main-includes/main.conf + /etc/nginx/secrets/test-certbundle.crt + /etc/nginx/secrets/test-keypair.pem + /etc/nginx/stream-conf.d/stream.conf + */ + g.Expect(files[0].Type).To(Equal(file.TypeRegular)) g.Expect(files[0].Path).To(Equal("/etc/nginx/conf.d/config-version.conf")) configVersion := string(files[0].Content) @@ -128,6 +163,8 @@ func TestGenerate(t *testing.T) { g.Expect(httpCfg).To(ContainSubstring("batch_count 4;")) g.Expect(httpCfg).To(ContainSubstring("otel_service_name ngf:gw-ns:gw-name:my-name;")) g.Expect(httpCfg).To(ContainSubstring("http2 on;")) + g.Expect(httpCfg).To(ContainSubstring("include /etc/nginx/includes/http_snippet1.conf;")) + g.Expect(httpCfg).To(ContainSubstring("include /etc/nginx/includes/http_snippet2.conf;")) g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/matches.json")) @@ -135,22 +172,32 @@ func TestGenerate(t *testing.T) { expString := "{}" g.Expect(string(files[2].Content)).To(Equal(expString)) - g.Expect(files[3].Path).To(Equal("/etc/nginx/module-includes/load-modules.conf")) - g.Expect(files[3].Content).To(Equal([]byte("load_module modules/ngx_otel_module.so;"))) - - g.Expect(files[4].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt")) - certBundle := string(files[4].Content) + // snippet include files + // content is not checked in this test. + g.Expect(files[3].Path).To(Equal("/etc/nginx/includes/http_snippet1.conf")) + g.Expect(files[4].Path).To(Equal("/etc/nginx/includes/http_snippet2.conf")) + g.Expect(files[5].Path).To(Equal("/etc/nginx/includes/main_snippet1.conf")) + g.Expect(files[6].Path).To(Equal("/etc/nginx/includes/main_snippet2.conf")) + + g.Expect(files[7].Path).To(Equal("/etc/nginx/main-includes/main.conf")) + mainConfStr := string(files[7].Content) + g.Expect(mainConfStr).To(ContainSubstring("load_module modules/ngx_otel_module.so;")) + g.Expect(mainConfStr).To(ContainSubstring("include /etc/nginx/includes/main_snippet1.conf;")) + g.Expect(mainConfStr).To(ContainSubstring("include /etc/nginx/includes/main_snippet2.conf;")) + + g.Expect(files[8].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt")) + certBundle := string(files[8].Content) g.Expect(certBundle).To(Equal("test-cert")) - g.Expect(files[5]).To(Equal(file.File{ + g.Expect(files[9]).To(Equal(file.File{ Type: file.TypeSecret, Path: "/etc/nginx/secrets/test-keypair.pem", Content: []byte("test-cert\ntest-key"), })) - g.Expect(files[6].Path).To(Equal("/etc/nginx/stream-conf.d/stream.conf")) - g.Expect(files[6].Type).To(Equal(file.TypeRegular)) - streamCfg := string(files[6].Content) + g.Expect(files[10].Path).To(Equal("/etc/nginx/stream-conf.d/stream.conf")) + g.Expect(files[10].Type).To(Equal(file.TypeRegular)) + streamCfg := string(files[10].Content) g.Expect(streamCfg).To(ContainSubstring("listen unix:/var/run/nginx/app.example.com-443.sock")) g.Expect(streamCfg).To(ContainSubstring("listen 443")) g.Expect(streamCfg).To(ContainSubstring("app.example.com unix:/var/run/nginx/app.example.com-443.sock")) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index f17d51e5d5..24aecaa3e4 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -13,7 +13,7 @@ type Server struct { ServerName string Listen string Locations []Location - Includes []Include + Includes []shared.Include IsDefaultHTTP bool IsDefaultSSL bool GRPC bool @@ -39,7 +39,7 @@ type Location struct { Return *Return ResponseHeaders ResponseHeaders Rewrites []string - Includes []Include + Includes []shared.Include GRPC bool } @@ -117,9 +117,3 @@ type ServerConfig struct { IPFamily shared.IPFamily Plus bool } - -// Include defines a file that's included via the include directive. -type Include struct { - Name string - Content []byte -} diff --git a/internal/mode/static/nginx/config/includes.go b/internal/mode/static/nginx/config/includes.go new file mode 100644 index 0000000000..81b7ff8b9f --- /dev/null +++ b/internal/mode/static/nginx/config/includes.go @@ -0,0 +1,157 @@ +package config + +import ( + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +// createIncludeExecuteResultsFromServers creates a list of executeResults -- or NGINX config files -- from all +// the includes in the provided servers. Since there may be duplicate includes, such as configuration for policies that +// apply to multiple routes, or snippets filters that are attached to multiple routing rules, this function deduplicates +// all includes, ensuring only a single file per unique include is generated. +func createIncludeExecuteResultsFromServers(servers []http.Server) []executeResult { + uniqueIncludes := make(map[string][]byte) + + // deduplicate include files across servers and location + for _, server := range servers { + for _, include := range server.Includes { + uniqueIncludes[include.Name] = include.Content + } + + for _, loc := range server.Locations { + for _, include := range loc.Includes { + uniqueIncludes[include.Name] = include.Content + } + } + } + + results := make([]executeResult, 0, len(uniqueIncludes)) + + for filename, contents := range uniqueIncludes { + results = append(results, executeResult{ + dest: filename, + data: contents, + }) + } + + return results +} + +// createIncludesFromPolicyGenerateResult converts a list of policies.File into a list of includes. +func createIncludesFromPolicyGenerateResult(resFiles []policies.File) []shared.Include { + if len(resFiles) == 0 { + return nil + } + + includes := make([]shared.Include, 0, len(resFiles)) + for _, file := range resFiles { + includes = append(includes, shared.Include{ + Name: includesFolder + "/" + file.Name, + Content: file.Content, + }) + } + + return includes +} + +// createIncludeFromSnippet converts a dataplane.Snippet into an include. +func createIncludeFromSnippet(snippet dataplane.Snippet) shared.Include { + return shared.Include{ + Name: includesFolder + "/" + snippet.Name + ".conf", + Content: []byte(snippet.Contents), + } +} + +// deduplicateIncludes deduplicates all the includes using the include name as the identifier. +// Duplicate includes are possible when a single policy targets multiple resources, or a snippets filter +// is referenced on multiple routing rules. +func deduplicateIncludes(includes []shared.Include) []shared.Include { + uniqueIncludes := make(map[string]shared.Include) + for _, i := range includes { + if _, ok := uniqueIncludes[i.Name]; !ok { + uniqueIncludes[i.Name] = i + } + } + + results := make([]shared.Include, 0, len(uniqueIncludes)) + for _, i := range uniqueIncludes { + results = append(results, i) + } + + return results +} + +// createIncludesFromLocationSnippetsFilters creates includes for a location from a list of SnippetsFilters. +// A SnippetsFilter can have both a server snippet and a location snippet. This function converts +// all the location snippets in the SnippetsFilters to includes. +func createIncludesFromLocationSnippetsFilters(filters []dataplane.SnippetsFilter) []shared.Include { + if len(filters) == 0 { + return nil + } + + includes := make([]shared.Include, 0) + + for _, f := range filters { + if f.LocationSnippet != nil { + includes = append(includes, createIncludeFromSnippet(*f.LocationSnippet)) + } + } + + return deduplicateIncludes(includes) +} + +// createIncludesFromServerSnippetsFilters creates includes for a server from a dataplane.VirtualServer. +// It finds all the server snippets from the SnippetsFilters on each MatchRule. This function converts all +// the server snippets into includes. +func createIncludesFromServerSnippetsFilters(server dataplane.VirtualServer) []shared.Include { + if len(server.PathRules) == 0 { + return nil + } + + includes := make([]shared.Include, 0) + + for _, pr := range server.PathRules { + for _, mr := range pr.MatchRules { + for _, sf := range mr.Filters.SnippetsFilters { + if sf.ServerSnippet != nil { + includes = append(includes, createIncludeFromSnippet(*sf.ServerSnippet)) + } + } + } + } + + return deduplicateIncludes(includes) +} + +// createIncludesFromSnippets converts a list of Snippets to a list of Includes. +// Used for main and http snippets only. Server and location snippets are handled by other functions above. +func createIncludesFromSnippets(snippets []dataplane.Snippet) []shared.Include { + if len(snippets) == 0 { + return nil + } + + includes := make([]shared.Include, 0) + + for _, s := range snippets { + includes = append(includes, createIncludeFromSnippet(s)) + } + + return deduplicateIncludes(includes) +} + +// createIncludeExecuteResults creates a list of executeResults -- or NGINX config files -- from a list of includes. +// Used for main and http snippets only. Server and location snippets are handled by other functions above. +func createIncludeExecuteResults(includes []shared.Include) []executeResult { + results := make([]executeResult, 0, len(includes)) + + for _, inc := range includes { + results = append(results, executeResult{ + dest: inc.Name, + data: inc.Content, + }) + } + + return results +} diff --git a/internal/mode/static/nginx/config/includes_test.go b/internal/mode/static/nginx/config/includes_test.go new file mode 100644 index 0000000000..b4b4133236 --- /dev/null +++ b/internal/mode/static/nginx/config/includes_test.go @@ -0,0 +1,517 @@ +package config + +import ( + "testing" + + . "github.com/onsi/gomega" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +func TestCreateIncludeExecuteResultsFromServers(t *testing.T) { + t.Parallel() + + servers := []http.Server{ + { + Includes: []shared.Include{ + { + Name: "include-1.conf", + Content: []byte("include-1"), + }, + { + Name: "include-2.conf", + Content: []byte("include-2"), + }, + }, + Locations: []http.Location{ + { + Includes: []shared.Include{ + { + Name: "include-3.conf", + Content: []byte("include-3"), + }, + { + Name: "include-4.conf", + Content: []byte("include-4"), + }, + }, + }, + }, + }, + { + Includes: []shared.Include{ + { + Name: "include-1.conf", // dupe + Content: []byte("include-1"), + }, + { + Name: "include-2.conf", // dupe + Content: []byte("include-2"), + }, + }, + Locations: []http.Location{ + { + Includes: []shared.Include{ + { + Name: "include-3.conf", // dupe + Content: []byte("include-3"), + }, + { + Name: "include-4.conf", // dupe + Content: []byte("include-4"), + }, + { + Name: "include-5.conf", + Content: []byte("include-5"), + }, + }, + }, + }, + }, + } + + results := createIncludeExecuteResultsFromServers(servers) + + expResults := []executeResult{ + { + dest: "include-1.conf", + data: []byte("include-1"), + }, + { + dest: "include-2.conf", + data: []byte("include-2"), + }, + { + dest: "include-3.conf", + data: []byte("include-3"), + }, + { + dest: "include-4.conf", + data: []byte("include-4"), + }, + { + dest: "include-5.conf", + data: []byte("include-5"), + }, + } + + g := NewWithT(t) + + g.Expect(results).To(ConsistOf(expResults)) +} + +func TestCreateIncludesFromPolicyGenerateResult(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + files []policies.File + includes []shared.Include + }{ + { + name: "no files", + files: nil, + includes: nil, + }, + { + name: "additions", + files: []policies.File{ + { + Content: []byte("one"), + Name: "one.conf", + }, + { + Content: []byte("two"), + Name: "two.conf", + }, + { + Content: []byte("three"), + Name: "three.conf", + }, + }, + includes: []shared.Include{ + { + Content: []byte("one"), + Name: includesFolder + "/one.conf", + }, + { + Content: []byte("two"), + Name: includesFolder + "/two.conf", + }, + { + Content: []byte("three"), + Name: includesFolder + "/three.conf", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + includes := createIncludesFromPolicyGenerateResult(test.files) + g.Expect(includes).To(Equal(test.includes)) + }) + } +} + +func TestCreateIncludesFromLocationSnippetsFilter(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + filters []dataplane.SnippetsFilter + expIncludes []shared.Include + }{ + { + name: "no filters", + filters: nil, + expIncludes: nil, + }, + { + name: "filters with no location snippets", + filters: []dataplane.SnippetsFilter{ + { + LocationSnippet: nil, + ServerSnippet: &dataplane.Snippet{Name: "server1", Contents: "directive1"}, + }, + { + LocationSnippet: nil, + ServerSnippet: &dataplane.Snippet{Name: "server2", Contents: "directive2"}, + }, + }, + expIncludes: []shared.Include{}, + }, + { + name: "filters with some location snippets, duplicates should be ignored", + filters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{Name: "location1", Contents: "location directive1"}, + ServerSnippet: &dataplane.Snippet{Name: "server1", Contents: "server directive1"}, + }, + { + LocationSnippet: nil, + ServerSnippet: &dataplane.Snippet{Name: "server2", Contents: "server directive2"}, + }, + { + LocationSnippet: &dataplane.Snippet{Name: "location2", Contents: "location directive2"}, + ServerSnippet: nil, + }, + { + LocationSnippet: &dataplane.Snippet{Name: "location2", Contents: "location directive2"}, // dupe + ServerSnippet: nil, + }, + }, + expIncludes: []shared.Include{ + { + Name: includesFolder + "/location1.conf", + Content: []byte("location directive1"), + }, + { + Name: includesFolder + "/location2.conf", + Content: []byte("location directive2"), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + + includes := createIncludesFromLocationSnippetsFilters(test.filters) + g.Expect(includes).To(ConsistOf(test.expIncludes)) + }) + } +} + +func TestCreateIncludesFromServerSnippetsFilters(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expIncludes []shared.Include + server dataplane.VirtualServer + }{ + { + name: "no path rules (default server) should return nil includes", + server: dataplane.VirtualServer{IsDefault: true, PathRules: nil}, + expIncludes: nil, + }, + { + name: "no snippets filters", + server: dataplane.VirtualServer{ + PathRules: []dataplane.PathRule{ + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + RequestRedirect: &dataplane.HTTPRequestRedirectFilter{}, + SnippetsFilters: nil, + }, + }, + { + Filters: dataplane.HTTPFilters{ + RequestURLRewrite: &dataplane.HTTPURLRewriteFilter{}, + SnippetsFilters: nil, + }, + }, + }, + }, + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + ResponseHeaderModifiers: &dataplane.HTTPHeaderFilter{}, + SnippetsFilters: nil, + }, + }, + { + Filters: dataplane.HTTPFilters{ + ResponseHeaderModifiers: &dataplane.HTTPHeaderFilter{}, + SnippetsFilters: nil, + }, + }, + }, + }, + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + InvalidFilter: &dataplane.InvalidHTTPFilter{}, + }, + }, + }, + }, + }, + }, + expIncludes: []shared.Include{}, + }, + { + name: "some snippets filters, duplicates should be ignored", + server: dataplane.VirtualServer{ + PathRules: []dataplane.PathRule{ + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + ServerSnippet: &dataplane.Snippet{ + Name: "server1", + Contents: "server directive1", + }, + }, + }, + }, + }, + }, + }, + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + ServerSnippet: &dataplane.Snippet{ + Name: "server1", // dupe, should be ignored + Contents: "server directive1", + }, + }, + }, + }, + }, + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + ServerSnippet: &dataplane.Snippet{ + Name: "server2", + Contents: "server directive2", + }, + }, + }, + }, + }, + }, + }, + { + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + ServerSnippet: &dataplane.Snippet{ + Name: "server1", // another dupe, should be ignored + Contents: "server directive1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + expIncludes: []shared.Include{ + { + Name: includesFolder + "/server1.conf", + Content: []byte("server directive1"), + }, + { + Name: includesFolder + "/server2.conf", + Content: []byte("server directive2"), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + includes := createIncludesFromServerSnippetsFilters(test.server) + g.Expect(includes).To(ConsistOf(test.expIncludes)) + }) + } +} + +func TestCreateIncludesFromSnippets(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + snippets []dataplane.Snippet + expIncludes []shared.Include + }{ + { + name: "no snippets", + snippets: nil, + expIncludes: nil, + }, + { + name: "snippets, duplicates are ignored", + snippets: []dataplane.Snippet{ + { + Name: "snippet1", + Contents: "directive1", + }, + { + Name: "snippet2", + Contents: "directive2", + }, + { + Name: "snippet1", // duplicate + Contents: "directive1", + }, + { + Name: "snippet3", + Contents: "directive3", + }, + { + Name: "snippet3", // duplicate + Contents: "directive3", + }, + { + Name: "snippet4", + Contents: "directive4", + }, + }, + expIncludes: []shared.Include{ + { + Name: includesFolder + "/snippet1.conf", + Content: []byte("directive1"), + }, + { + Name: includesFolder + "/snippet2.conf", + Content: []byte("directive2"), + }, + { + Name: includesFolder + "/snippet3.conf", + Content: []byte("directive3"), + }, + { + Name: includesFolder + "/snippet4.conf", + Content: []byte("directive4"), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + + includes := createIncludesFromSnippets(test.snippets) + g.Expect(includes).To(ConsistOf(test.expIncludes)) + }) + } +} + +func TestCreateIncludeExecuteResults(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + includes []shared.Include + expExecuteResults []executeResult + }{ + { + name: "no includes", + includes: nil, + expExecuteResults: []executeResult{}, + }, + { + name: "includes", + includes: []shared.Include{ + { + Name: "include1.conf", + Content: []byte("directive1"), + }, + { + Name: "include2.conf", + Content: []byte("directive2"), + }, + { + Name: "include3.conf", + Content: []byte("directive3"), + }, + }, + expExecuteResults: []executeResult{ + { + dest: "include1.conf", + data: []byte("directive1"), + }, + { + dest: "include2.conf", + data: []byte("directive2"), + }, + { + dest: "include3.conf", + data: []byte("directive3"), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + + results := createIncludeExecuteResults(test.includes) + g.Expect(results).To(ConsistOf(test.expExecuteResults)) + }) + } +} diff --git a/internal/mode/static/nginx/config/main_config.go b/internal/mode/static/nginx/config/main_config.go new file mode 100644 index 0000000000..e0876ae7e0 --- /dev/null +++ b/internal/mode/static/nginx/config/main_config.go @@ -0,0 +1,34 @@ +package config + +import ( + gotemplate "text/template" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +var mainConfigTemplate = gotemplate.Must(gotemplate.New("main").Parse(mainConfigTemplateText)) + +type mainConfig struct { + Includes []shared.Include + TelemetryEnabled bool +} + +func executeMainConfig(conf dataplane.Configuration) []executeResult { + includes := createIncludesFromSnippets(conf.MainSnippets) + + mc := mainConfig{ + TelemetryEnabled: conf.Telemetry.Endpoint != "", + Includes: includes, + } + + results := make([]executeResult, 0, len(includes)+1) + results = append(results, executeResult{ + dest: mainIncludeFile, + data: helpers.MustExecuteTemplate(mainConfigTemplate, mc), + }) + results = append(results, createIncludeExecuteResults(includes)...) + + return results +} diff --git a/internal/mode/static/nginx/config/main_config_template.go b/internal/mode/static/nginx/config/main_config_template.go new file mode 100644 index 0000000000..dbd3dca2d6 --- /dev/null +++ b/internal/mode/static/nginx/config/main_config_template.go @@ -0,0 +1,11 @@ +package config + +const mainConfigTemplateText = ` +{{ if .TelemetryEnabled -}} +load_module modules/ngx_otel_module.so; +{{ end -}} + +{{ range $i := .Includes -}} +include {{ $i.Name }}; +{{ end -}} +` diff --git a/internal/mode/static/nginx/config/main_config_test.go b/internal/mode/static/nginx/config/main_config_test.go new file mode 100644 index 0000000000..251f170c0f --- /dev/null +++ b/internal/mode/static/nginx/config/main_config_test.go @@ -0,0 +1,109 @@ +package config + +import ( + "sort" + "testing" + + . "github.com/onsi/gomega" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +func TestExecuteMainConfig_Telemetry(t *testing.T) { + t.Parallel() + + telemetryOff := dataplane.Configuration{ + Telemetry: dataplane.Telemetry{}, + } + telemetryOn := dataplane.Configuration{ + Telemetry: dataplane.Telemetry{ + Endpoint: "endpoint", + }, + } + loadModuleDirective := "load_module modules/ngx_otel_module.so;" + + tests := []struct { + name string + conf dataplane.Configuration + expLoadModuleDirective bool + }{ + { + name: "telemetry off", + conf: telemetryOff, + expLoadModuleDirective: false, + }, + { + name: "telemetry on", + conf: telemetryOn, + expLoadModuleDirective: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + res := executeMainConfig(test.conf) + g.Expect(res).To(HaveLen(1)) + g.Expect(res[0].dest).To(Equal(mainIncludeFile)) + if test.expLoadModuleDirective { + g.Expect(res[0].data).To(ContainSubstring(loadModuleDirective)) + } else { + g.Expect(res[0].data).ToNot(ContainSubstring(loadModuleDirective)) + } + }) + } +} + +func TestExecuteMainConfig_Snippets(t *testing.T) { + t.Parallel() + + conf := dataplane.Configuration{ + MainSnippets: []dataplane.Snippet{ + { + Name: "snippet1", + Contents: "contents1", + }, + { + Name: "snippet2", + Contents: "contents2", + }, + { + Name: "snippet3", + Contents: "contents3", + }, + }, + } + + g := NewWithT(t) + + res := executeMainConfig(conf) + g.Expect(res).To(HaveLen(4)) + + // sort results by filename + sort.Slice( + res, func(i, j int) bool { + return res[i].dest < res[j].dest + }, + ) + + /* + Order of files: + /etc/nginx/includes/snippet1.conf + /etc/nginx/includes/snippet2.conf + /etc/nginx/includes/snippet3.conf + /etc/nginx/main-includes/main.conf + */ + + g.Expect(res[0].dest).To(Equal("/etc/nginx/includes/snippet1.conf")) + g.Expect(string(res[0].data)).To(ContainSubstring("contents1")) + + g.Expect(res[1].dest).To(Equal("/etc/nginx/includes/snippet2.conf")) + g.Expect(string(res[1].data)).To(ContainSubstring("contents2")) + + g.Expect(res[2].dest).To(Equal("/etc/nginx/includes/snippet3.conf")) + g.Expect(string(res[2].data)).To(ContainSubstring("contents3")) + + g.Expect(res[3].dest).To(Equal(mainIncludeFile)) +} diff --git a/internal/mode/static/nginx/config/policies/generator.go b/internal/mode/static/nginx/config/policies/generator.go index a1428a2f9c..24ed738a69 100644 --- a/internal/mode/static/nginx/config/policies/generator.go +++ b/internal/mode/static/nginx/config/policies/generator.go @@ -10,7 +10,7 @@ import ( type Generator interface { // GenerateForServer generates policy configuration for the server block. GenerateForServer(policies []Policy, server http.Server) GenerateResultFiles - // GenerateForServer generates policy configuration for a normal location block. + // GenerateForLocation generates policy configuration for a normal location block. GenerateForLocation(policies []Policy, location http.Location) GenerateResultFiles // GenerateForInternalLocation generates policy configuration for an internal location block. GenerateForInternalLocation(policies []Policy) GenerateResultFiles diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 0fd1930f5e..33ea858f31 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -124,7 +124,7 @@ func (g GeneratorImpl) executeServers(conf dataplane.Configuration, generator po data: httpMatchConf, } - includeFileResults := createIncludeFileResults(servers) + includeFileResults := createIncludeExecuteResultsFromServers(servers) allResults := make([]executeResult, 0, len(includeFileResults)+2) allResults = append(allResults, includeFileResults...) @@ -145,33 +145,6 @@ func getIPFamily(baseHTTPConfig dataplane.BaseHTTPConfig) shared.IPFamily { return shared.IPFamily{IPv4: true, IPv6: true} } -func createIncludeFileResults(servers []http.Server) []executeResult { - uniqueIncludes := make(map[string][]byte) - - for _, server := range servers { - for _, include := range server.Includes { - uniqueIncludes[include.Name] = include.Content - } - - for _, loc := range server.Locations { - for _, include := range loc.Includes { - uniqueIncludes[include.Name] = include.Content - } - } - } - - results := make([]executeResult, 0, len(uniqueIncludes)) - - for filename, contents := range uniqueIncludes { - results = append(results, executeResult{ - dest: filename, - data: contents, - }) - } - - return results -} - func createServers(conf dataplane.Configuration, generator policies.Generator) ([]http.Server, httpMatchPairs) { servers := make([]http.Server, 0, len(conf.HTTPServers)+len(conf.SSLServers)) finalMatchPairs := make(httpMatchPairs) @@ -229,9 +202,15 @@ func createSSLServer( Listen: listen, } - server.Includes = createIncludesFromPolicyGenerateResult( + policyIncludes := createIncludesFromPolicyGenerateResult( generator.GenerateForServer(virtualServer.Policies, server), ) + snippetIncludes := createIncludesFromServerSnippetsFilters(virtualServer) + + server.Includes = make([]shared.Include, 0, len(policyIncludes)+len(snippetIncludes)) + server.Includes = append(server.Includes, policyIncludes...) + server.Includes = append(server.Includes, snippetIncludes...) + return server, matchPairs } @@ -258,9 +237,14 @@ func createServer( GRPC: grpc, } - server.Includes = createIncludesFromPolicyGenerateResult( + policyIncludes := createIncludesFromPolicyGenerateResult( generator.GenerateForServer(virtualServer.Policies, server), ) + snippetIncludes := createIncludesFromServerSnippetsFilters(virtualServer) + + server.Includes = make([]shared.Include, 0, len(policyIncludes)+len(snippetIncludes)) + server.Includes = append(server.Includes, policyIncludes...) + server.Includes = append(server.Includes, snippetIncludes...) return server, matchPairs } @@ -363,22 +347,6 @@ func needsInternalLocations(rule dataplane.PathRule) bool { return len(rule.MatchRules) == 1 && !isPathOnlyMatch(rule.MatchRules[0].Match) } -func createIncludesFromPolicyGenerateResult(resFiles []policies.File) []http.Include { - if len(resFiles) == 0 { - return nil - } - - includes := make([]http.Include, 0, len(resFiles)) - for _, file := range resFiles { - includes = append(includes, http.Include{ - Name: includesFolder + "/" + file.Name, - Content: file.Content, - }) - } - - return includes -} - // pathAndTypeMap contains a map of paths and any path types defined for that path // for example, {/foo: {exact: {}, prefix: {}}}. type pathAndTypeMap map[string]map[dataplane.PathType]struct{} @@ -488,6 +456,8 @@ func updateLocation( return location } + location.Includes = append(location.Includes, createIncludesFromLocationSnippetsFilters(filters.SnippetsFilters)...) + if filters.RequestRedirect != nil { ret := createReturnValForRedirectFilter(filters.RequestRedirect, listenerPort) location.Return = ret diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 515908c76f..f3e523a4c0 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -19,6 +19,7 @@ import ( func TestExecuteServers(t *testing.T) { t.Parallel() + conf := dataplane.Configuration{ HTTPServers: []dataplane.VirtualServer{ { @@ -35,6 +36,42 @@ func TestExecuteServers(t *testing.T) { Policies: []policies.Policy{ &policiesfakes.FakePolicy{}, }, + PathRules: []dataplane.PathRule{ + { + Path: "/", + PathType: dataplane.PathTypePrefix, + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{ + Name: "location-snippet", + Contents: "location snippet contents", + }, + ServerSnippet: &dataplane.Snippet{ + Name: "server-snippet", + Contents: "server snippet contents", + }, + }, + }, + }, + Match: dataplane.Match{}, + BackendGroup: dataplane.BackendGroup{ + Source: types.NamespacedName{Namespace: "test", Name: "route1"}, + RuleIdx: 0, + Backends: []dataplane.Backend{ + { + UpstreamName: "test_foo_443", + Valid: true, + Weight: 1, + }, + }, + }, + }, + }, + }, + }, }, }, SSLServers: []dataplane.VirtualServer{ @@ -99,6 +136,8 @@ func TestExecuteServers(t *testing.T) { "ssl_certificate_key /etc/nginx/secrets/test-keypair.pem;": 2, "proxy_ssl_server_name on;": 1, "status_zone": 0, + "include /etc/nginx/includes/location-snippet.conf": 1, + "include /etc/nginx/includes/server-snippet.conf": 1, } type assertion func(g *WithT, data string) @@ -118,20 +157,29 @@ func TestExecuteServers(t *testing.T) { includesFolder + "/include-2.conf": func(g *WithT, data string) { g.Expect(data).To(Equal("include-2")) }, + includesFolder + "/location-snippet.conf": func(g *WithT, data string) { + g.Expect(data).To(Equal("location snippet contents")) + }, + includesFolder + "/server-snippet.conf": func(g *WithT, data string) { + g.Expect(data).To(Equal("server snippet contents")) + }, } + g := NewWithT(t) fakeGenerator := &policiesfakes.FakeGenerator{} - fakeGenerator.GenerateForServerReturns(policies.GenerateResultFiles{ - { - Name: "include-1.conf", - Content: []byte("include-1"), - }, - { - Name: "include-2.conf", - Content: []byte("include-2"), + fakeGenerator.GenerateForServerReturns( + policies.GenerateResultFiles{ + { + Name: "include-1.conf", + Content: []byte("include-1"), + }, + { + Name: "include-2.conf", + Content: []byte("include-2"), + }, }, - }) + ) gen := GeneratorImpl{} results := gen.executeServers(conf, fakeGenerator) @@ -1050,10 +1098,11 @@ func TestCreateServers(t *testing.T) { }, } - externalIncludes := []http.Include{ + externalIncludes := []shared.Include{ {Name: "/etc/nginx/includes/include-1.conf", Content: []byte("include-1")}, } - internalIncludes := []http.Include{ + + internalIncludes := []shared.Include{ {Name: "/etc/nginx/includes/internal-include-1.conf", Content: []byte("include-1")}, } @@ -1453,6 +1502,7 @@ func TestCreateServers(t *testing.T) { { ServerName: "cafe.example.com", Locations: getExpectedLocations(false), + Includes: []shared.Include{}, Listen: "8080", GRPC: true, }, @@ -1468,6 +1518,7 @@ func TestCreateServers(t *testing.T) { CertificateKey: expectedPEMPath, }, Locations: getExpectedLocations(true), + Includes: []shared.Include{}, Listen: getSocketNameHTTPS(8443), IsSocket: true, GRPC: true, @@ -1702,17 +1753,338 @@ func TestCreateServersConflicts(t *testing.T) { ServerName: "cafe.example.com", Locations: test.expLocs, Listen: "8080", + Includes: []shared.Include{}, }, } g := NewWithT(t) - result, _ := createServers(dataplane.Configuration{HTTPServers: httpServers}, &policiesfakes.FakeGenerator{}) + result, _ := createServers( + dataplane.Configuration{HTTPServers: httpServers}, + &policiesfakes.FakeGenerator{}, + ) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) } } +func TestCreateServers_Includes(t *testing.T) { + t.Parallel() + + pathRules := []dataplane.PathRule{ + { + Path: "/", + PathType: dataplane.PathTypeExact, + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{ + Name: "location-snippet", + Contents: "location snippet contents", + }, + ServerSnippet: &dataplane.Snippet{ + Name: "server-snippet", + Contents: "server snippet contents", + }, + }, + }, + }, + }, + }, + }, + } + + httpServers := []dataplane.VirtualServer{ + { + IsDefault: true, + Port: 8080, + }, + { + Hostname: "http.example.com", + PathRules: pathRules, + Port: 8080, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + }, + }, + } + + sslServers := []dataplane.VirtualServer{ + { + IsDefault: true, + Port: 8443, + }, + { + Hostname: "ssl.example.com", + SSL: &dataplane.SSL{KeyPairID: "test-keypair"}, + PathRules: pathRules, + Port: 8443, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + }, + }, + } + + fakeGenerator := &policiesfakes.FakeGenerator{} + fakeGenerator.GenerateForLocationReturns(policies.GenerateResultFiles{ + { + Name: "ext-policy.conf", + Content: []byte("external policy conf"), + }, + }) + fakeGenerator.GenerateForServerReturns(policies.GenerateResultFiles{ + { + Name: "server-policy.conf", + Content: []byte("server policy conf"), + }, + }) + + expServers := []http.Server{ + { + IsDefaultHTTP: true, + }, + { + ServerName: "http.example.com", + Locations: []http.Location{ + { + Path: "= /", + Includes: []shared.Include{ + { + Name: includesFolder + "/location-snippet.conf", + Content: []byte("location snippet contents"), + }, + { + Name: includesFolder + "/ext-policy.conf", + Content: []byte("external policy conf"), + }, + }, + }, + }, + Includes: []shared.Include{ + { + Name: includesFolder + "/server-policy.conf", + Content: []byte("server policy conf"), + }, + { + Name: includesFolder + "/server-snippet.conf", + Content: []byte("server snippet contents"), + }, + }, + Listen: "8080", + GRPC: true, + }, + { + IsDefaultSSL: true, + }, + { + ServerName: "ssl.example.com", + Locations: []http.Location{ + { + Path: "= /", + Includes: []shared.Include{ + { + Name: includesFolder + "/location-snippet.conf", + Content: []byte("location snippet contents"), + }, + { + Name: includesFolder + "/ext-policy.conf", + Content: []byte("external policy conf"), + }, + }, + }, + }, + Includes: []shared.Include{ + { + Name: includesFolder + "/server-policy.conf", + Content: []byte("server policy conf"), + }, + { + Name: includesFolder + "/server-snippet.conf", + Content: []byte("server snippet contents"), + }, + }, + }, + } + + g := NewWithT(t) + + conf := dataplane.Configuration{HTTPServers: httpServers, SSLServers: sslServers} + + actualServers, matchPairs := createServers(conf, fakeGenerator) + g.Expect(matchPairs).To(BeEmpty()) + g.Expect(actualServers).To(HaveLen(len(expServers))) + + for i, expServer := range expServers { + g.Expect(actualServers[i].ServerName).To(Equal(expServer.ServerName)) + + if actualServers[i].IsDefaultHTTP || actualServers[i].IsDefaultSSL { + g.Expect(actualServers[i].Includes).To(BeEmpty()) + } else { + g.Expect(actualServers[i].Includes).To(ConsistOf(expServer.Includes)) + g.Expect(actualServers[i].Locations).To(HaveLen(1)) + g.Expect(actualServers[i].Locations[0].Path).To(Equal(expServer.Locations[0].Path)) + g.Expect(actualServers[i].Locations[0].Includes).To(ConsistOf(expServer.Locations[0].Includes)) + } + } +} + +func TestCreateLocations_Includes(t *testing.T) { + t.Parallel() + + httpServer := dataplane.VirtualServer{ + Hostname: "example.com", + PathRules: []dataplane.PathRule{ + { + Path: "/", + PathType: dataplane.PathTypeExact, + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{ + Name: "location-snippet", + Contents: "location snippet contents", + }, + ServerSnippet: &dataplane.Snippet{ + Name: "server-snippet", + Contents: "server snippet 2 contents", + }, + }, + }, + }, + }, + }, + }, + { + Path: "/snippets-prefix-path", + PathType: dataplane.PathTypePrefix, + MatchRules: []dataplane.MatchRule{ + { + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{ + Name: "prefix-path-location-snippet", + Contents: "prefix path location snippet contents", + }, + }, + }, + }, + }, + }, + }, + { + Path: "/snippets-with-method-match", + PathType: dataplane.PathTypeExact, + MatchRules: []dataplane.MatchRule{ + { + Match: dataplane.Match{ + Method: helpers.GetPointer("GET"), + }, + Filters: dataplane.HTTPFilters{ + SnippetsFilters: []dataplane.SnippetsFilter{ + { + LocationSnippet: &dataplane.Snippet{ + Name: "method-match-location-snippet", + Contents: "method match location snippet contents", + }, + }, + }, + }, + }, + }, + }, + }, + Port: 80, + } + + externalPolicyInclude := shared.Include{ + Name: includesFolder + "/ext-policy.conf", + Content: []byte("external policy conf"), + } + + internalPolicyInclude := shared.Include{ + Name: includesFolder + "/int-policy.conf", + Content: []byte("internal policy conf"), + } + + // this test only covers the includes generated for locations, it does not test other location fields. + expLocations := []http.Location{ + { + Path: "= /", + Includes: []shared.Include{ + { + Name: includesFolder + "/location-snippet.conf", + Content: []byte("location snippet contents"), + }, + externalPolicyInclude, + }, + }, + { + Path: "/snippets-prefix-path/", + Includes: []shared.Include{ + { + Name: includesFolder + "/prefix-path-location-snippet.conf", + Content: []byte("prefix path location snippet contents"), + }, + externalPolicyInclude, + }, + }, + { + Path: "= /snippets-prefix-path", + Includes: []shared.Include{ + { + Name: includesFolder + "/prefix-path-location-snippet.conf", + Content: []byte("prefix path location snippet contents"), + }, + externalPolicyInclude, + }, + }, + { + Path: "= /snippets-with-method-match", + Includes: []shared.Include{externalPolicyInclude}, + }, + { + Path: "/_ngf-internal-rule2-route0", + Includes: []shared.Include{ + { + Name: includesFolder + "/method-match-location-snippet.conf", + Content: []byte("method match location snippet contents"), + }, + internalPolicyInclude, + }, + }, + } + + fakeGenerator := &policiesfakes.FakeGenerator{} + fakeGenerator.GenerateForLocationReturns(policies.GenerateResultFiles{ + { + Name: "ext-policy.conf", + Content: []byte("external policy conf"), + }, + }) + fakeGenerator.GenerateForInternalLocationReturns(policies.GenerateResultFiles{ + { + Name: "int-policy.conf", + Content: []byte("internal policy conf"), + }, + }) + + locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator) + + g := NewWithT(t) + g.Expect(grpc).To(BeFalse()) + g.Expect(matches).To(HaveLen(1)) + g.Expect(locations).To(HaveLen(len(expLocations))) + for i, location := range locations { + g.Expect(location.Path).To(Equal(expLocations[i].Path)) + g.Expect(location.Includes).To(ConsistOf(expLocations[i].Includes)) + } +} + func TestCreateLocationsRootPath(t *testing.T) { t.Parallel() hrNsName := types.NamespacedName{Namespace: "test", Name: "route1"} @@ -2885,153 +3257,6 @@ func TestGenerateResponseHeaders(t *testing.T) { } } -func TestCreateIncludesFromPolicyGenerateResult(t *testing.T) { - t.Parallel() - tests := []struct { - name string - files []policies.File - includes []http.Include - }{ - { - name: "no files", - files: nil, - includes: nil, - }, - { - name: "additions", - files: []policies.File{ - { - Content: []byte("one"), - Name: "one.conf", - }, - { - Content: []byte("two"), - Name: "two.conf", - }, - { - Content: []byte("three"), - Name: "three.conf", - }, - }, - includes: []http.Include{ - { - Content: []byte("one"), - Name: includesFolder + "/one.conf", - }, - { - Content: []byte("two"), - Name: includesFolder + "/two.conf", - }, - { - Content: []byte("three"), - Name: includesFolder + "/three.conf", - }, - }, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - includes := createIncludesFromPolicyGenerateResult(test.files) - g.Expect(includes).To(Equal(test.includes)) - }) - } -} - -func TestCreateIncludeFileResults(t *testing.T) { - t.Parallel() - servers := []http.Server{ - { - Includes: []http.Include{ - { - Name: "include-1.conf", - Content: []byte("include-1"), - }, - { - Name: "include-2.conf", - Content: []byte("include-2"), - }, - }, - Locations: []http.Location{ - { - Includes: []http.Include{ - { - Name: "include-3.conf", - Content: []byte("include-3"), - }, - { - Name: "include-4.conf", - Content: []byte("include-4"), - }, - }, - }, - }, - }, - { - Includes: []http.Include{ - { - Name: "include-1.conf", // dupe - Content: []byte("include-1"), - }, - { - Name: "include-2.conf", // dupe - Content: []byte("include-2"), - }, - }, - Locations: []http.Location{ - { - Includes: []http.Include{ - { - Name: "include-3.conf", // dupe - Content: []byte("include-3"), - }, - { - Name: "include-4.conf", // dupe - Content: []byte("include-4"), - }, - { - Name: "include-5.conf", - Content: []byte("include-5"), - }, - }, - }, - }, - }, - } - - results := createIncludeFileResults(servers) - - expResults := []executeResult{ - { - dest: "include-1.conf", - data: []byte("include-1"), - }, - { - dest: "include-2.conf", - data: []byte("include-2"), - }, - { - dest: "include-3.conf", - data: []byte("include-3"), - }, - { - dest: "include-4.conf", - data: []byte("include-4"), - }, - { - dest: "include-5.conf", - data: []byte("include-5"), - }, - } - - g := NewWithT(t) - - g.Expect(results).To(ConsistOf(expResults)) -} - func TestGetIPFamily(t *testing.T) { t.Parallel() test := []struct { diff --git a/internal/mode/static/nginx/config/shared/config.go b/internal/mode/static/nginx/config/shared/config.go index 62ea4bec82..c38acb7e75 100644 --- a/internal/mode/static/nginx/config/shared/config.go +++ b/internal/mode/static/nginx/config/shared/config.go @@ -20,7 +20,7 @@ type IPFamily struct { IPv6 bool } -// RewriteClientIP holds the configuration for the rewrite client IP settings. +// RewriteClientIPSettings holds the configuration for the rewrite client IP settings. type RewriteClientIPSettings struct { RealIPHeader string ProxyProtocol string @@ -31,3 +31,9 @@ type RewriteClientIPSettings struct { const ( ProxyProtocolDirective = " proxy_protocol" ) + +// Include defines a file that's included via the include directive. +type Include struct { + Name string + Content []byte +} diff --git a/internal/mode/static/nginx/config/version.go b/internal/mode/static/nginx/config/version.go index 5baa7f24b8..6e29f36056 100644 --- a/internal/mode/static/nginx/config/version.go +++ b/internal/mode/static/nginx/config/version.go @@ -4,10 +4,16 @@ import ( gotemplate "text/template" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) var versionTemplate = gotemplate.Must(gotemplate.New("version").Parse(versionTemplateText)) -func executeVersion(version int) []byte { - return helpers.MustExecuteTemplate(versionTemplate, version) +func executeVersion(conf dataplane.Configuration) []executeResult { + result := executeResult{ + dest: configVersionFile, + data: helpers.MustExecuteTemplate(versionTemplate, conf.Version), + } + + return []executeResult{result} } diff --git a/internal/mode/static/nginx/config/version_test.go b/internal/mode/static/nginx/config/version_test.go index 176db3dfec..3d30c6c3b2 100644 --- a/internal/mode/static/nginx/config/version_test.go +++ b/internal/mode/static/nginx/config/version_test.go @@ -1,21 +1,20 @@ package config import ( - "strings" "testing" . "github.com/onsi/gomega" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) func TestExecuteVersion(t *testing.T) { t.Parallel() g := NewWithT(t) - expSubStrings := map[string]int{ - "return 200 42;": 1, - } - maps := string(executeVersion(42)) - for expSubStr, expCount := range expSubStrings { - g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr))) - } + conf := dataplane.Configuration{Version: 42} + res := executeVersion(conf) + g.Expect(res).To(HaveLen(1)) + g.Expect(res[0].dest).To(Equal(configVersionFile)) + g.Expect(string(res[0].data)).To(ContainSubstring("return 200 42;")) } diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 8e73e82df4..9da779c7da 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -222,7 +222,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: cfg.MustExtractGVK(&ngfAPI.SnippetsFilter{}), store: newObjectStoreMapAdapter(clusterStore.SnippetsFilters), - predicate: nil, /*TODO(kate-osborn): will add predicate in next PR*/ + predicate: nil, // we always want to write status to SnippetsFilters so we don't filter them out }, }, ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index fe790d6087..d1ec0d3c2c 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -566,8 +566,11 @@ var _ = Describe("ChangeProcessor", func() { Weight: 1, }, }, - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: graph.RouteRuleFilters{ + Filters: []graph.Filter{}, + Valid: true, + }, Matches: hr1.Spec.Rules[0].Matches, RouteBackendRefs: createRouteBackendRefs(hr1.Spec.Rules[0].BackendRefs), }, @@ -610,8 +613,11 @@ var _ = Describe("ChangeProcessor", func() { Hostnames: hr2.Spec.Hostnames, Rules: []graph.RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: graph.RouteRuleFilters{ + Valid: true, + Filters: []graph.Filter{}, + }, Matches: hr2.Spec.Rules[0].Matches, RouteBackendRefs: []graph.RouteBackendRef{}, }, @@ -1938,7 +1944,7 @@ var _ = Describe("ChangeProcessor", func() { paramGC := gc.DeepCopy() paramGC.Spec.ParametersRef = &v1beta1.ParametersReference{ Group: ngfAPI.GroupName, - Kind: v1beta1.Kind(kinds.NginxProxy), + Kind: kinds.NginxProxy, Name: "np", } @@ -2124,6 +2130,76 @@ var _ = Describe("ChangeProcessor", func() { }) }) }) + + Describe("SnippetsFilter resource changed", Ordered, func() { + sfNsName := types.NamespacedName{ + Name: "sf", + Namespace: "test", + } + + sf := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: sfNsName.Name, + Namespace: sfNsName.Namespace, + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextMain, + Value: "main snippet", + }, + }, + }, + } + + sfUpdated := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: sfNsName.Name, + Namespace: sfNsName.Namespace, + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextMain, + Value: "main snippet", + }, + { + Context: ngfAPI.NginxContextHTTP, + Value: "http snippet", + }, + }, + }, + } + It("handles upserts for a SnippetsFilter", func() { + processor.CaptureUpsertChange(sf) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + + processedSf, exists := graph.SnippetsFilters[sfNsName] + Expect(exists).To(BeTrue()) + Expect(processedSf.Source).To(Equal(sf)) + Expect(processedSf.Valid).To(BeTrue()) + }) + It("captures changes for a SnippetsFilter", func() { + processor.CaptureUpsertChange(sfUpdated) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + + processedSf, exists := graph.SnippetsFilters[sfNsName] + Expect(exists).To(BeTrue()) + Expect(processedSf.Source).To(Equal(sfUpdated)) + Expect(processedSf.Valid).To(BeTrue()) + }) + It("handles deletes for a SnippetsFilter", func() { + processor.CaptureDeleteChange(sfUpdated, sfNsName) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.SnippetsFilters).To(BeEmpty()) + }) + }) }) Describe("Ensuring non-changing changes don't override previously changing changes", func() { // Note: in these tests, we deliberately don't fully inspect the returned configuration and statuses diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 0cb959db6a..c97f9efa2d 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -49,6 +49,10 @@ const ( // Used with ResolvedRefs (false). RouteReasonInvalidIPFamily v1.RouteConditionReason = "InvalidServiceIPFamily" + // RouteReasonInvalidFilter is used when an extension ref filter referenced by a Route cannot be resolved, or is + // invalid. Used with ResolvedRefs (false). + RouteReasonInvalidFilter v1.RouteConditionReason = "InvalidFilter" + // GatewayReasonGatewayConflict indicates there are multiple Gateway resources to choose from, // and we ignored the resource in question and picked another Gateway as the winner. // This reason is used with GatewayConditionAccepted (false). @@ -311,6 +315,17 @@ func NewRouteInvalidIPFamily(msg string) conditions.Condition { } } +// NewRouteResolvedRefsInvalidFilter returns a Condition that indicates that the Route has a filter that +// cannot be resolved or is invalid. +func NewRouteResolvedRefsInvalidFilter(msg string) conditions.Condition { + return conditions.Condition{ + Type: string(v1.RouteConditionResolvedRefs), + Status: metav1.ConditionFalse, + Reason: string(RouteReasonInvalidFilter), + Message: msg, + } +} + // NewDefaultListenerConditions returns the default Conditions that must be present in the status of a Listener. func NewDefaultListenerConditions() []conditions.Condition { return []conditions.Condition{ diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index 5c6721f6ed..96d4476a6e 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -15,7 +15,7 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" - policies "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -63,6 +63,7 @@ func BuildConfiguration( CertBundles: certBundles, Telemetry: telemetry, BaseHTTPConfig: baseHTTPConfig, + MainSnippets: buildSnippetsForContext(g.SnippetsFilters, ngfAPI.NginxContextMain), } return config @@ -256,7 +257,7 @@ func buildCertBundles( if err != nil { data = cm.CACert } - bundles[id] = CertBundle(data) + bundles[id] = data } } } @@ -477,8 +478,8 @@ func (hpr *hostPathRules) upsertRoute( } var filters HTTPFilters - if rule.ValidFilters { - filters = createHTTPFilters(rule.Filters) + if rule.Filters.Valid { + filters = createHTTPFilters(rule.Filters.Filters) } else { filters = HTTPFilters{ InvalidFilter: &InvalidHTTPFilter{}, @@ -628,7 +629,7 @@ func buildUpstreams( } for _, rule := range route.Spec.Rules { - if !rule.ValidMatches || !rule.ValidFilters { + if !rule.ValidMatches || !rule.Filters.Valid { // don't generate upstreams for rules that have invalid matches or filters continue } @@ -699,33 +700,41 @@ func getPath(path *v1.HTTPPathMatch) string { return *path.Value } -func createHTTPFilters(filters []v1.HTTPRouteFilter) HTTPFilters { +func createHTTPFilters(filters []graph.Filter) HTTPFilters { var result HTTPFilters for _, f := range filters { - switch f.Type { - case v1.HTTPRouteFilterRequestRedirect: + switch f.FilterType { + case graph.FilterRequestRedirect: if result.RequestRedirect == nil { // using the first filter result.RequestRedirect = convertHTTPRequestRedirectFilter(f.RequestRedirect) } - case v1.HTTPRouteFilterURLRewrite: + case graph.FilterURLRewrite: if result.RequestURLRewrite == nil { // using the first filter result.RequestURLRewrite = convertHTTPURLRewriteFilter(f.URLRewrite) } - case v1.HTTPRouteFilterRequestHeaderModifier: + case graph.FilterRequestHeaderModifier: if result.RequestHeaderModifiers == nil { // using the first filter result.RequestHeaderModifiers = convertHTTPHeaderFilter(f.RequestHeaderModifier) } - case v1.HTTPRouteFilterResponseHeaderModifier: + case graph.FilterResponseHeaderModifier: if result.ResponseHeaderModifiers == nil { // using the first filter result.ResponseHeaderModifiers = convertHTTPHeaderFilter(f.ResponseHeaderModifier) } + case graph.FilterExtensionRef: + if f.ResolvedExtensionRef != nil && f.ResolvedExtensionRef.SnippetsFilter != nil { + result.SnippetsFilters = append( + result.SnippetsFilters, + convertSnippetsFilter(f.ResolvedExtensionRef.SnippetsFilter), + ) + } } } + return result } @@ -834,6 +843,7 @@ func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { // HTTP2 should be enabled by default HTTP2: true, IPFamily: Dual, + Snippets: buildSnippetsForContext(g.SnippetsFilters, ngfAPI.NginxContextHTTP), } if g.NginxProxy == nil || !g.NginxProxy.Valid { return baseConfig @@ -876,6 +886,45 @@ func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { return baseConfig } +func createSnippetName(nc ngfAPI.NginxContext, nsname types.NamespacedName) string { + return fmt.Sprintf( + "SnippetsFilter_%s_%s_%s", + nc, + nsname.Namespace, + nsname.Name, + ) +} + +func buildSnippetsForContext( + snippetFilters map[types.NamespacedName]*graph.SnippetsFilter, + nc ngfAPI.NginxContext, +) []Snippet { + if len(snippetFilters) == 0 { + return nil + } + + snippetsForContext := make([]Snippet, 0) + + for _, filter := range snippetFilters { + if !filter.Valid || !filter.Referenced { + continue + } + + snippetValue, ok := filter.Snippets[nc] + + if !ok { + continue + } + + snippetsForContext = append(snippetsForContext, Snippet{ + Name: createSnippetName(nc, client.ObjectKeyFromObject(filter.Source)), + Contents: snippetValue, + }) + } + + return snippetsForContext +} + func buildPolicies(graphPolicies []*graph.Policy) []policies.Policy { if len(graphPolicies) == 0 { return nil diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 07b6d40946..b0eea67b40 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -21,8 +21,9 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" - policiesfakes "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver/resolverfakes" @@ -160,9 +161,12 @@ func TestBuildConfiguration(t *testing.T) { } } - addFilters := func(hr *graph.L7Route, filters []v1.HTTPRouteFilter) { + addFilters := func(hr *graph.L7Route, filters []graph.Filter) { for i := range hr.Spec.Rules { - hr.Spec.Rules[i].Filters = filters + hr.Spec.Rules[i].Filters = graph.RouteRuleFilters{ + Filters: filters, + Valid: *hr.Spec.Rules[i].Matches[0].Path.Value != invalidFiltersPath, + } } } @@ -217,10 +221,12 @@ func TestBuildConfiguration(t *testing.T) { } rules[i] = graph.RouteRule{ - ValidMatches: validMatches, - ValidFilters: validFilters, + Matches: m, + Filters: graph.RouteRuleFilters{ + Valid: validFilters, + }, BackendRefs: createBackendRefs(validRule), - Matches: m, + ValidMatches: validMatches, } } @@ -259,7 +265,7 @@ func TestBuildConfiguration(t *testing.T) { for idx, r := range route.Spec.Rules { var backends []Backend - if r.ValidFilters && r.ValidMatches { + if r.Filters.Valid && r.ValidMatches { backends = []Backend{expValidBackend} } @@ -326,16 +332,76 @@ func TestBuildConfiguration(t *testing.T) { pathAndType{path: "/", pathType: prefix}, pathAndType{path: invalidFiltersPath, pathType: prefix}, ) - redirect := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestRedirect, + sf1 := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sf", + Namespace: "test", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "location snippet", + ngfAPI.NginxContextHTTPServer: "server snippet", + ngfAPI.NginxContextMain: "main snippet", + ngfAPI.NginxContextHTTP: "http snippet", + }, + } + + sfNotReferenced := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sf-not-referenced", + Namespace: "test", + }, + }, + Valid: true, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextMain: "main snippet no ref", + ngfAPI.NginxContextHTTP: "http snippet no ref", + }, + } + + redirect := graph.Filter{ + FilterType: graph.FilterRequestRedirect, RequestRedirect: &v1.HTTPRequestRedirectFilter{ Hostname: (*v1.PreciseHostname)(helpers.GetPointer("foo.example.com")), }, } - addFilters(routeHR5, []v1.HTTPRouteFilter{redirect}) + extRefFilter := graph.Filter{ + FilterType: graph.FilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + ResolvedExtensionRef: &graph.ExtensionRefFilter{ + Valid: true, + SnippetsFilter: sf1, + }, + } + addFilters(routeHR5, []graph.Filter{redirect, extRefFilter}) expRedirect := HTTPRequestRedirectFilter{ Hostname: helpers.GetPointer("foo.example.com"), } + expExtRefFilter := SnippetsFilter{ + LocationSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServerLocation, + client.ObjectKeyFromObject(extRefFilter.ResolvedExtensionRef.SnippetsFilter.Source), + ), + Contents: "location snippet", + }, + ServerSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServer, + client.ObjectKeyFromObject(extRefFilter.ResolvedExtensionRef.SnippetsFilter.Source), + ), + Contents: "server snippet", + }, + } hr6, expHR6Groups, routeHR6 := createTestResources( "hr-6", @@ -1552,6 +1618,7 @@ func TestBuildConfiguration(t *testing.T) { BackendGroup: expHR5Groups[0], Filters: HTTPFilters{ RequestRedirect: &expRedirect, + SnippetsFilters: []SnippetsFilter{expExtRefFilter}, }, }, }, @@ -2228,6 +2295,42 @@ func TestBuildConfiguration(t *testing.T) { }), msg: "NginxProxy with rewriteClientIP details set", }, + { + graph: getModifiedGraph(func(g *graph.Graph) *graph.Graph { + g.SnippetsFilters = map[types.NamespacedName]*graph.SnippetsFilter{ + client.ObjectKeyFromObject(sf1.Source): sf1, + client.ObjectKeyFromObject(sfNotReferenced.Source): sfNotReferenced, + } + + return g + }), + expConf: getModifiedExpectedConfiguration(func(conf Configuration) Configuration { + conf.MainSnippets = []Snippet{ + { + Name: createSnippetName( + ngfAPI.NginxContextMain, + client.ObjectKeyFromObject(sf1.Source), + ), + Contents: "main snippet", + }, + } + conf.BaseHTTPConfig.Snippets = []Snippet{ + { + Name: createSnippetName( + ngfAPI.NginxContextHTTP, + client.ObjectKeyFromObject(sf1.Source), + ), + Contents: "http snippet", + }, + } + conf.HTTPServers = []VirtualServer{} + conf.SSLServers = []VirtualServer{} + conf.SSLKeyPairs = map[SSLKeyPairID]SSLKeyPair{} + + return conf + }), + msg: "SnippetsFilters with main and http snippet", + }, } for _, test := range tests { @@ -2297,32 +2400,33 @@ func TestGetPath(t *testing.T) { func TestCreateFilters(t *testing.T) { t.Parallel() - redirect1 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestRedirect, + + redirect1 := graph.Filter{ + FilterType: graph.FilterRequestRedirect, RequestRedirect: &v1.HTTPRequestRedirectFilter{ Hostname: helpers.GetPointer[v1.PreciseHostname]("foo.example.com"), }, } - redirect2 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestRedirect, + redirect2 := graph.Filter{ + FilterType: graph.FilterRequestRedirect, RequestRedirect: &v1.HTTPRequestRedirectFilter{ Hostname: helpers.GetPointer[v1.PreciseHostname]("bar.example.com"), }, } - rewrite1 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterURLRewrite, + rewrite1 := graph.Filter{ + FilterType: graph.FilterURLRewrite, URLRewrite: &v1.HTTPURLRewriteFilter{ Hostname: helpers.GetPointer[v1.PreciseHostname]("foo.example.com"), }, } - rewrite2 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterURLRewrite, + rewrite2 := graph.Filter{ + FilterType: graph.FilterURLRewrite, URLRewrite: &v1.HTTPURLRewriteFilter{ Hostname: helpers.GetPointer[v1.PreciseHostname]("bar.example.com"), }, } - requestHeaderModifiers1 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestHeaderModifier, + requestHeaderModifiers1 := graph.Filter{ + FilterType: graph.FilterRequestHeaderModifier, RequestHeaderModifier: &v1.HTTPHeaderFilter{ Set: []v1.HTTPHeader{ { @@ -2332,8 +2436,8 @@ func TestCreateFilters(t *testing.T) { }, }, } - requestHeaderModifiers2 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestHeaderModifier, + requestHeaderModifiers2 := graph.Filter{ + FilterType: graph.FilterRequestHeaderModifier, RequestHeaderModifier: &v1.HTTPHeaderFilter{ Add: []v1.HTTPHeader{ { @@ -2344,8 +2448,8 @@ func TestCreateFilters(t *testing.T) { }, } - responseHeaderModifiers1 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterResponseHeaderModifier, + responseHeaderModifiers1 := graph.Filter{ + FilterType: graph.FilterResponseHeaderModifier, ResponseHeaderModifier: &v1.HTTPHeaderFilter{ Add: []v1.HTTPHeader{ { @@ -2356,8 +2460,8 @@ func TestCreateFilters(t *testing.T) { }, } - responseHeaderModifiers2 := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterResponseHeaderModifier, + responseHeaderModifiers2 := graph.Filter{ + FilterType: graph.FilterResponseHeaderModifier, ResponseHeaderModifier: &v1.HTTPHeaderFilter{ Set: []v1.HTTPHeader{ { @@ -2392,49 +2496,83 @@ func TestCreateFilters(t *testing.T) { }, } + snippetsFilter1 := graph.Filter{ + FilterType: graph.FilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf1", + }, + ResolvedExtensionRef: &graph.ExtensionRefFilter{ + Valid: true, + SnippetsFilter: &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sf1", + Namespace: "default", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "location snippet 1", + ngfAPI.NginxContextMain: "main snippet 1", + ngfAPI.NginxContextHTTPServer: "server snippet 1", + ngfAPI.NginxContextHTTP: "http snippet 1", + }, + }, + }, + } + + snippetsFilter2 := graph.Filter{ + FilterType: graph.FilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf2", + }, + ResolvedExtensionRef: &graph.ExtensionRefFilter{ + Valid: true, + SnippetsFilter: &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sf2", + Namespace: "default", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "location snippet 2", + ngfAPI.NginxContextMain: "main snippet 2", + ngfAPI.NginxContextHTTPServer: "server snippet 2", + ngfAPI.NginxContextHTTP: "http snippet 2", + }, + }, + }, + } + tests := []struct { expected HTTPFilters msg string - filters []v1.HTTPRouteFilter + filters []graph.Filter }{ { - filters: []v1.HTTPRouteFilter{}, + filters: []graph.Filter{}, expected: HTTPFilters{}, msg: "no filters", }, { - filters: []v1.HTTPRouteFilter{ + filters: []graph.Filter{ redirect1, }, expected: HTTPFilters{ RequestRedirect: &expectedRedirect1, }, - msg: "one filter", + msg: "one request redirect filter", }, { - filters: []v1.HTTPRouteFilter{ - redirect1, - redirect2, - }, - expected: HTTPFilters{ - RequestRedirect: &expectedRedirect1, - }, - msg: "two filters, first wins", - }, - { - filters: []v1.HTTPRouteFilter{ - redirect1, - redirect2, - requestHeaderModifiers1, - }, - expected: HTTPFilters{ - RequestRedirect: &expectedRedirect1, - RequestHeaderModifiers: &expectedHeaderModifier1, - }, - msg: "two redirect filters, one request header modifier, first redirect wins", - }, - { - filters: []v1.HTTPRouteFilter{ + filters: []graph.Filter{ redirect1, redirect2, rewrite1, @@ -2443,14 +2581,50 @@ func TestCreateFilters(t *testing.T) { requestHeaderModifiers2, responseHeaderModifiers1, responseHeaderModifiers2, + snippetsFilter1, + snippetsFilter2, }, expected: HTTPFilters{ RequestRedirect: &expectedRedirect1, RequestURLRewrite: &expectedRewrite1, RequestHeaderModifiers: &expectedHeaderModifier1, ResponseHeaderModifiers: &expectedresponseHeaderModifier, + SnippetsFilters: []SnippetsFilter{ + { + LocationSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServerLocation, + types.NamespacedName{Namespace: "default", Name: "sf1"}, + ), + Contents: "location snippet 1", + }, + ServerSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServer, + types.NamespacedName{Namespace: "default", Name: "sf1"}, + ), + Contents: "server snippet 1", + }, + }, + { + LocationSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServerLocation, + types.NamespacedName{Namespace: "default", Name: "sf2"}, + ), + Contents: "location snippet 2", + }, + ServerSnippet: &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServer, + types.NamespacedName{Namespace: "default", Name: "sf2"}, + ), + Contents: "server snippet 2", + }, + }, + }, }, - msg: "two of each filter, first value for each wins", + msg: "two of each filter, first value for each standard filter wins, all ext ref filters added", }, } @@ -2508,7 +2682,7 @@ func refsToValidRules(refs ...[]graph.BackendRef) []graph.RouteRule { for _, ref := range refs { rules = append(rules, graph.RouteRule{ ValidMatches: true, - ValidFilters: true, + Filters: graph.RouteRuleFilters{Valid: true}, BackendRefs: ref, }) } @@ -2991,7 +3165,6 @@ func TestConvertBackendTLS(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - g.Expect(convertBackendTLS(tc.btp)).To(Equal(tc.expected)) }) } @@ -3744,3 +3917,174 @@ func TestBuildRewriteIPSettings(t *testing.T) { }) } } + +func TestCreateSnippetName(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + + name := createSnippetName( + ngfAPI.NginxContextHTTPServerLocation, + types.NamespacedName{Namespace: "some-ns", Name: "some-name"}, + ) + g.Expect(name).To(Equal("SnippetsFilter_http.server.location_some-ns_some-name")) +} + +func TestBuildSnippetForContext(t *testing.T) { + t.Parallel() + + validUnreferenced := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-unreferenced", + Namespace: "default", + }, + }, + Valid: true, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "valid unreferenced", + }, + } + + invalidUnreferenced := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid-unreferenced", + Namespace: "default", + }, + }, + Valid: false, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "invalid unreferenced", + }, + } + + invalidReferenced := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid-referenced", + Namespace: "default", + }, + }, + Valid: false, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "invalid referenced", + }, + } + + validReferenced1 := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-referenced1", + Namespace: "default", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTP: "http valid referenced 1", + ngfAPI.NginxContextMain: "main valid referenced 1", + }, + } + + validReferenced2 := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-referenced2", + Namespace: "other-ns", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextMain: "main valid referenced 2", + ngfAPI.NginxContextHTTP: "http valid referenced 2", + }, + } + + validReferenced3 := &graph.SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-referenced3", + Namespace: "other-ns", + }, + }, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "location valid referenced 2", + }, + } + + expMainSnippets := []Snippet{ + { + Name: createSnippetName(ngfAPI.NginxContextMain, client.ObjectKeyFromObject(validReferenced1.Source)), + Contents: "main valid referenced 1", + }, + { + Name: createSnippetName(ngfAPI.NginxContextMain, client.ObjectKeyFromObject(validReferenced2.Source)), + Contents: "main valid referenced 2", + }, + } + + expHTTPSnippets := []Snippet{ + { + Name: createSnippetName(ngfAPI.NginxContextHTTP, client.ObjectKeyFromObject(validReferenced1.Source)), + Contents: "http valid referenced 1", + }, + { + Name: createSnippetName(ngfAPI.NginxContextHTTP, client.ObjectKeyFromObject(validReferenced2.Source)), + Contents: "http valid referenced 2", + }, + } + + getSnippetsFilters := func() map[types.NamespacedName]*graph.SnippetsFilter { + return map[types.NamespacedName]*graph.SnippetsFilter{ + client.ObjectKeyFromObject(validUnreferenced.Source): validUnreferenced, + client.ObjectKeyFromObject(invalidUnreferenced.Source): invalidUnreferenced, + client.ObjectKeyFromObject(invalidReferenced.Source): invalidReferenced, + client.ObjectKeyFromObject(validReferenced1.Source): validReferenced1, + client.ObjectKeyFromObject(validReferenced2.Source): validReferenced2, + client.ObjectKeyFromObject(validReferenced3.Source): validReferenced3, + } + } + + tests := []struct { + name string + snippetsFilters map[types.NamespacedName]*graph.SnippetsFilter + ctx ngfAPI.NginxContext + expSnippets []Snippet + }{ + { + name: "no snippets filters", + snippetsFilters: nil, + ctx: ngfAPI.NginxContextMain, + expSnippets: nil, + }, + { + name: "main context: mix of invalid, unreferenced, and valid, referenced snippets filters", + snippetsFilters: getSnippetsFilters(), + ctx: ngfAPI.NginxContextMain, + expSnippets: expMainSnippets, + }, + { + name: "http context: mix of invalid, unreferenced, and valid, referenced snippets filters", + snippetsFilters: getSnippetsFilters(), + ctx: ngfAPI.NginxContextHTTP, + expSnippets: expHTTPSnippets, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + snippets := buildSnippetsForContext(test.snippetsFilters, test.ctx) + g.Expect(snippets).To(ConsistOf(test.expSnippets)) + }) + } +} diff --git a/internal/mode/static/state/dataplane/convert.go b/internal/mode/static/state/dataplane/convert.go index 4337a0d787..626e5c9030 100644 --- a/internal/mode/static/state/dataplane/convert.go +++ b/internal/mode/static/state/dataplane/convert.go @@ -3,7 +3,11 @@ package dataplane import ( "fmt" + "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" ) func convertMatch(m v1.HTTPRouteMatch) Match { @@ -104,3 +108,26 @@ func convertPathModifier(path *v1.HTTPPathModifier) *HTTPPathModifier { return nil } + +func convertSnippetsFilter(filter *graph.SnippetsFilter) SnippetsFilter { + result := SnippetsFilter{} + + if snippet, ok := filter.Snippets[ngfAPI.NginxContextHTTPServer]; ok { + result.ServerSnippet = &Snippet{ + Name: createSnippetName(ngfAPI.NginxContextHTTPServer, client.ObjectKeyFromObject(filter.Source)), + Contents: snippet, + } + } + + if snippet, ok := filter.Snippets[ngfAPI.NginxContextHTTPServerLocation]; ok { + result.LocationSnippet = &Snippet{ + Name: createSnippetName( + ngfAPI.NginxContextHTTPServerLocation, + client.ObjectKeyFromObject(filter.Source), + ), + Contents: snippet, + } + } + + return result +} diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index 59110f8cbb..9003b3f38d 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -38,6 +38,8 @@ type Configuration struct { StreamUpstreams []Upstream // BackendGroups holds all unique BackendGroups. BackendGroups []BackendGroup + // MainSnippets holds all the snippets that apply to the main context. + MainSnippets []Snippet // Telemetry holds the Otel configuration. Telemetry Telemetry // BaseHTTPConfig holds the configuration options at the http context. @@ -139,6 +141,18 @@ type HTTPFilters struct { RequestHeaderModifiers *HTTPHeaderFilter // ResponseHeaderModifiers holds the HTTPHeaderFilter. ResponseHeaderModifiers *HTTPHeaderFilter + // SnippetsFilters holds all the SnippetsFilters for the MatchRule. + // Unlike the core and extended filters, there can be more than one SnippetsFilters defined on a routing rule. + SnippetsFilters []SnippetsFilter +} + +// SnippetsFilter holds the location and server snippets in a SnippetsFilter. +// The main and http snippets are stored separately in Configuration.MainSnippets and BaseHTTPConfig.Snippets. +type SnippetsFilter struct { + // LocationSnippet holds the snippet for the location context. + LocationSnippet *Snippet + // ServerSnippet holds the snippet for the server context. + ServerSnippet *Snippet } // HTTPHeader represents an HTTP header. @@ -309,13 +323,23 @@ type SpanAttribute struct { type BaseHTTPConfig struct { // IPFamily specifies the IP family for all servers. IPFamily IPFamilyType + // Snippets contain the snippets that apply to the http context. + Snippets []Snippet // RewriteIPSettings defines configuration for rewriting the client IP to the original client's IP. RewriteClientIPSettings RewriteClientIPSettings // HTTP2 specifies whether http2 should be enabled for all servers. HTTP2 bool } -// RewriteIPSettings defines configuration for rewriting the client IP to the original client's IP. +// Snippet is a snippet of configuration. +type Snippet struct { + // Name is the name of the snippet. + Name string + // Contents is the content of the snippet. + Contents string +} + +// RewriteClientIPSettings defines configuration for rewriting the client IP to the original client's IP. type RewriteClientIPSettings struct { // Mode specifies the mode for rewriting the client IP. Mode RewriteIPModeType diff --git a/internal/mode/static/state/graph/backend_refs.go b/internal/mode/static/state/graph/backend_refs.go index b65fd7cbcc..e8c5120b48 100644 --- a/internal/mode/static/state/graph/backend_refs.go +++ b/internal/mode/static/state/graph/backend_refs.go @@ -71,7 +71,7 @@ func addBackendRefsToRules( if !rule.ValidMatches { continue } - if !rule.ValidFilters { + if !rule.Filters.Valid { continue } @@ -326,7 +326,8 @@ func verifyIPFamily(npCfg *NginxProxy, svcIPFamily []v1.IPFamily) error { if *npIPFamily == ngfAPI.IPv4 { if slices.Contains(svcIPFamily, v1.IPv6Protocol) { // capitalizing error message to match the rest of the error messages associated with a condition - return errors.New( //nolint: stylecheck + //nolint: stylecheck + return errors.New( "Service configured with IPv6 family but NginxProxy is configured with IPv4", ) } @@ -334,7 +335,8 @@ func verifyIPFamily(npCfg *NginxProxy, svcIPFamily []v1.IPFamily) error { if *npIPFamily == ngfAPI.IPv6 { if slices.Contains(svcIPFamily, v1.IPv4Protocol) { // capitalizing error message to match the rest of the error messages associated with a condition - return errors.New( //nolint: stylecheck + //nolint: stylecheck + return errors.New( "Service configured with IPv4 family but NginxProxy is configured with IPv6", ) } diff --git a/internal/mode/static/state/graph/backend_refs_test.go b/internal/mode/static/state/graph/backend_refs_test.go index b4e5b2cf3c..b7c49648da 100644 --- a/internal/mode/static/state/graph/backend_refs_test.go +++ b/internal/mode/static/state/graph/backend_refs_test.go @@ -398,6 +398,8 @@ func TestVerifyIPFamily(t *testing.T) { } func TestAddBackendRefsToRulesTest(t *testing.T) { + t.Parallel() + sectionNameRefs := []ParentRef{ { Idx: 0, @@ -455,28 +457,18 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { hr.Spec.Rules[idx] = RouteRule{ RouteBackendRefs: refs, ValidMatches: true, - ValidFilters: true, + Filters: RouteRuleFilters{ + Filters: []Filter{}, + Valid: true, + }, } } return hr } - hrWithOneBackend := createRoute("hr1", "Service", 1, "svc1") - hrWithTwoBackends := createRoute("hr2", "Service", 2, "svc1") - hrWithTwoDiffBackends := createRoute("hr2", "Service", 2, "svc1") - hrWithInvalidRule := createRoute("hr3", "NotService", 1, "svc1") - hrWithZeroBackendRefs := createRoute("hr4", "Service", 1, "svc1") - hrWithZeroBackendRefs.Spec.Rules[0].RouteBackendRefs = nil - hrWithTwoDiffBackends.Spec.Rules[0].RouteBackendRefs[1].Name = "svc2" - - hrWithOneBackendInvalid := createRoute("hr1", "Service", 1, "svc1") - hrWithOneBackendInvalid.Valid = false - - hrWithOneBackendInvalidMatches := createRoute("hr1", "Service", 1, "svc1") - hrWithOneBackendInvalidMatches.Spec.Rules[0].ValidMatches = false - - hrWithOneBackendInvalidFilters := createRoute("hr1", "Service", 1, "svc1") - hrWithOneBackendInvalidFilters.Spec.Rules[0].ValidFilters = false + modRoute := func(route *L7Route, mod func(*L7Route) *L7Route) *L7Route { + return mod(route) + } getSvc := func(name string) *v1.Service { return &v1.Service{ @@ -614,7 +606,7 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { expectedConditions []conditions.Condition }{ { - route: hrWithOneBackend, + route: createRoute("hr1", "Service", 1, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -628,7 +620,7 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { name: "normal case with one rule with one backend", }, { - route: hrWithTwoBackends, + route: createRoute("hr2", "Service", 2, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -648,7 +640,7 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { name: "normal case with one rule with two backends", }, { - route: hrWithTwoBackends, + route: createRoute("hr2", "Service", 2, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -670,28 +662,37 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { name: "normal case with one rule with two backends and matching policies", }, { - route: hrWithOneBackendInvalid, + route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route.Valid = false + return route + }), expectedBackendRefs: nil, expectedConditions: nil, policies: emptyPolicies, name: "invalid route", }, { - route: hrWithOneBackendInvalidMatches, + route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route.Spec.Rules[0].ValidMatches = false + return route + }), expectedBackendRefs: nil, expectedConditions: nil, policies: emptyPolicies, name: "invalid matches", }, { - route: hrWithOneBackendInvalidFilters, + route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route.Spec.Rules[0].Filters = RouteRuleFilters{Valid: false} + return route + }), expectedBackendRefs: nil, expectedConditions: nil, policies: emptyPolicies, name: "invalid filters", }, { - route: hrWithInvalidRule, + route: createRoute("hr3", "NotService", 1, "svc1"), expectedBackendRefs: []BackendRef{ { Weight: 1, @@ -706,7 +707,10 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { name: "invalid backendRef", }, { - route: hrWithTwoDiffBackends, + route: modRoute(createRoute("hr2", "Service", 2, "svc1"), func(route *L7Route) *L7Route { + route.Spec.Rules[0].RouteBackendRefs[1].Name = "svc2" + return route + }), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -732,7 +736,10 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { name: "invalid backendRef - backend TLS policies do not match for all backends", }, { - route: hrWithZeroBackendRefs, + route: modRoute(createRoute("hr4", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route.Spec.Rules[0].RouteBackendRefs = nil + return route + }), expectedBackendRefs: nil, expectedConditions: nil, name: "zero backendRefs", @@ -741,6 +748,8 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) resolver := newReferenceGrantResolver(nil) addBackendRefsToRules(test.route, resolver, services, test.policies, nil) diff --git a/internal/mode/static/state/graph/common_filter.go b/internal/mode/static/state/graph/common_filter.go new file mode 100644 index 0000000000..5eba3964c0 --- /dev/null +++ b/internal/mode/static/state/graph/common_filter.go @@ -0,0 +1,380 @@ +package graph + +import ( + "fmt" + "slices" + "strings" + + "k8s.io/apimachinery/pkg/util/validation/field" + v1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" +) + +// RouteRuleFilters holds the Filters for a RouteRule. +type RouteRuleFilters struct { + // Filters are the filters in the RouteRule. + Filters []Filter + // Valid indicates if the filters are valid and accepted by the Route. + Valid bool +} + +// Filter is a filter in a Route. The Filter can belong to a GRPCRoute or an HTTPRoute. +type Filter struct { + // RequestHeaderModifier holds an HTTP Request Header Modifier filter. + // Will be non-nil if FilterType is FilterRequestHeaderModifier. + // Can be set on GRPCRoutes and HTTPRoutes. + RequestHeaderModifier *v1.HTTPHeaderFilter + // ResponseHeaderModifier holds an HTTP Response Header Modifier filter. + // Will be non-nil if FilterType is FilterResponseHeaderModifier. + // Can be set on GRPCRoutes and HTTPRoutes. + ResponseHeaderModifier *v1.HTTPHeaderFilter + // RequestRedirect holds an HTTP Request Redirect filter. + // Will be non-nil if FilterType is FilterRequestRedirect. + // Can be set on HTTPRoutes only. + RequestRedirect *v1.HTTPRequestRedirectFilter + // URLRewrite holds an HTTP URL Rewrite filter. + // Will be non-nil if FilterType is FilterURLRewrite. + // Can be set on HTTPRoutes only. + URLRewrite *v1.HTTPURLRewriteFilter + // RequestMirror holds an HTTP Request Mirror filter. + // Will be non-nil if FilterType is FilterRequestMirror. + // Can be set on GRPCRoutes and HTTPRoutes. + RequestMirror *v1.HTTPRequestMirrorFilter + // ExtensionRef holds an Extension Ref filter. + // Will be non-nil if FilterType is FilterExtensionRef. + // Can be set on GRPCRoutes and HTTPRoutes. + ExtensionRef *v1.LocalObjectReference + // ResolvedExtensionRef holds the filter that the Extension Ref points to. + // Will be non-nil if the Extension Ref is non-nil and was resolved successfully. + // Can be set on GRPCRoutes and HTTPRoutes. + ResolvedExtensionRef *ExtensionRefFilter + // RouteType is the type of Route that this filter is on. + RouteType RouteType + // FilterType is the type of filter. + FilterType FilterType +} + +// FilterType is the type of filter. +type FilterType string + +// The following FilterTypes are supported by GRPCRoutes and HTTPRoutes. +const ( + FilterRequestHeaderModifier = FilterType(v1.HTTPRouteFilterRequestHeaderModifier) + FilterResponseHeaderModifier = FilterType(v1.HTTPRouteFilterResponseHeaderModifier) + FilterExtensionRef = FilterType(v1.HTTPRouteFilterExtensionRef) + FilterRequestMirror = FilterType(v1.HTTPRouteFilterRequestMirror) +) + +// The following FilterTypes are supported by HTTPRoutes only. +const ( + FilterRequestRedirect = FilterType(v1.HTTPRouteFilterRequestRedirect) + FilterURLRewrite = FilterType(v1.HTTPRouteFilterURLRewrite) +) + +func convertHTTPRouteFilters(filters []v1.HTTPRouteFilter) []Filter { + routeFilters := make([]Filter, 0, len(filters)) + + for _, filter := range filters { + routeFilters = append(routeFilters, Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterType(filter.Type), + RequestHeaderModifier: filter.RequestHeaderModifier, + ResponseHeaderModifier: filter.ResponseHeaderModifier, + RequestRedirect: filter.RequestRedirect, + URLRewrite: filter.URLRewrite, + RequestMirror: filter.RequestMirror, + ExtensionRef: filter.ExtensionRef, + }) + } + + return routeFilters +} + +func convertGRPCRouteFilters(filters []v1.GRPCRouteFilter) []Filter { + routeFilters := make([]Filter, 0, len(filters)) + + for _, filter := range filters { + routeFilters = append(routeFilters, Filter{ + RouteType: RouteTypeGRPC, + FilterType: FilterType(filter.Type), + RequestHeaderModifier: filter.RequestHeaderModifier, + ResponseHeaderModifier: filter.ResponseHeaderModifier, + RequestMirror: filter.RequestMirror, + ExtensionRef: filter.ExtensionRef, + }) + } + + return routeFilters +} + +func processRouteRuleFilters( + filters []Filter, + path *field.Path, + validator validation.HTTPFieldsValidator, + resolveExtRefFunc resolveExtRefFilter, +) (RouteRuleFilters, routeRuleErrors) { + errors := routeRuleErrors{} + valid := true + + for i, f := range filters { + filterPath := path.Index(i) + + validateErrs := validateFilter(validator, f, filterPath) + if len(validateErrs) > 0 { + errors.invalid = append(errors.invalid, validateErrs...) + valid = false + continue + } + + if f.FilterType == FilterExtensionRef && f.ExtensionRef != nil { + resolved := resolveExtRefFunc(*f.ExtensionRef) + + if resolved == nil { + err := field.NotFound(filterPath.Child("extensionRef"), f.ExtensionRef) + errors.resolve = append(errors.resolve, err) + valid = false + + continue + } + + if !resolved.Valid { + err := field.Invalid( + filterPath.Child("extensionRef"), + f.ExtensionRef, + "referenced filter is invalid. See filter status for more details.", + ) + errors.resolve = append(errors.resolve, err) + valid = false + + continue + } + + filters[i].ResolvedExtensionRef = resolved + } + } + + return RouteRuleFilters{Valid: valid, Filters: filters}, errors +} + +var supportedGRPCFilterTypes = []FilterType{ + FilterResponseHeaderModifier, + FilterRequestHeaderModifier, + FilterExtensionRef, +} + +var supportedHTTPFilterTypes = []FilterType{ + FilterResponseHeaderModifier, + FilterRequestHeaderModifier, + FilterExtensionRef, + FilterRequestRedirect, + FilterURLRewrite, +} + +func validateFilterType(filter Filter, filterPath *field.Path) *field.Error { + if filter.RouteType == RouteTypeGRPC && !slices.Contains(supportedGRPCFilterTypes, filter.FilterType) { + return field.NotSupported(filterPath.Child("type"), filter.FilterType, supportedGRPCFilterTypes) + } + + if !slices.Contains(supportedHTTPFilterTypes, filter.FilterType) { + return field.NotSupported(filterPath.Child("type"), filter.FilterType, supportedHTTPFilterTypes) + } + + return nil +} + +func validateFilter( + validator validation.HTTPFieldsValidator, + filter Filter, + filterPath *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + if err := validateFilterType(filter, filterPath); err != nil { + allErrs = append(allErrs, err) + return allErrs + } + + switch filter.FilterType { + case FilterRequestRedirect: + return validateFilterRedirect(validator, filter.RequestRedirect, filterPath) + case FilterURLRewrite: + return validateFilterRewrite(validator, filter.URLRewrite, filterPath) + case FilterRequestHeaderModifier: + return validateFilterHeaderModifier( + validator, + filter.RequestHeaderModifier, + filterPath.Child(string(filter.FilterType)), + ) + case FilterResponseHeaderModifier: + return validateFilterResponseHeaderModifier( + validator, + filter.ResponseHeaderModifier, + filterPath.Child(string(filter.FilterType)), + ) + case FilterExtensionRef: + return validateExtensionRefFilter(filter.ExtensionRef, filterPath) + default: + panic(fmt.Sprintf("unexpected filter type %v", filter.FilterType)) + } +} + +func validateFilterHeaderModifier( + validator validation.HTTPFieldsValidator, + headerModifier *v1.HTTPHeaderFilter, + filterPath *field.Path, +) field.ErrorList { + if headerModifier == nil { + return field.ErrorList{field.Required(filterPath, "cannot be nil")} + } + + return validateFilterHeaderModifierFields(validator, headerModifier, filterPath) +} + +func validateFilterHeaderModifierFields( + validator validation.HTTPFieldsValidator, + headerModifier *v1.HTTPHeaderFilter, + headerModifierPath *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + // Ensure that the header names are case-insensitive unique + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Add, + headerModifierPath.Child(add), + )...) + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Set, + headerModifierPath.Child(set), + )...) + allErrs = append(allErrs, validateRequestHeaderStringCaseInsensitiveUnique( + headerModifier.Remove, + headerModifierPath.Child(remove), + )...) + + for _, h := range headerModifier.Add { + if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { + valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) + allErrs = append(allErrs, valErr) + } + if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { + valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + for _, h := range headerModifier.Set { + if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { + valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) + allErrs = append(allErrs, valErr) + } + if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { + valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + for _, h := range headerModifier.Remove { + if err := validator.ValidateFilterHeaderName(h); err != nil { + valErr := field.Invalid(headerModifierPath.Child(remove), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + + return allErrs +} + +func validateFilterResponseHeaderModifier( + validator validation.HTTPFieldsValidator, + responseHeaderModifier *v1.HTTPHeaderFilter, + filterPath *field.Path, +) field.ErrorList { + if errList := validateFilterHeaderModifier(validator, responseHeaderModifier, filterPath); errList != nil { + return errList + } + var allErrs field.ErrorList + + allErrs = append(allErrs, validateResponseHeaders( + responseHeaderModifier.Add, + filterPath.Child(add), + )...) + + allErrs = append(allErrs, validateResponseHeaders( + responseHeaderModifier.Set, + filterPath.Child(set), + )...) + + var removeHeaders []v1.HTTPHeader + for _, h := range responseHeaderModifier.Remove { + removeHeaders = append(removeHeaders, v1.HTTPHeader{Name: v1.HTTPHeaderName(h)}) + } + + allErrs = append(allErrs, validateResponseHeaders( + removeHeaders, + filterPath.Child(remove), + )...) + + return allErrs +} + +func validateResponseHeaders( + headers []v1.HTTPHeader, + path *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + disallowedResponseHeaderSet := map[string]struct{}{ + "server": {}, + "date": {}, + "x-pad": {}, + "content-type": {}, + "content-length": {}, + "connection": {}, + } + invalidPrefix := "x-accel" + + for _, h := range headers { + valErr := field.Invalid(path, h, "header name is not allowed") + name := strings.ToLower(string(h.Name)) + if _, exists := disallowedResponseHeaderSet[name]; exists || + strings.HasPrefix(name, strings.ToLower(invalidPrefix)) { + allErrs = append(allErrs, valErr) + } + } + + return allErrs +} + +func validateRequestHeadersCaseInsensitiveUnique( + headers []v1.HTTPHeader, + path *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(string(h.Name)) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} + +func validateRequestHeaderStringCaseInsensitiveUnique(headers []string, path *field.Path) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(h) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} diff --git a/internal/mode/static/state/graph/common_filter_test.go b/internal/mode/static/state/graph/common_filter_test.go new file mode 100644 index 0000000000..1d9a2d1fa6 --- /dev/null +++ b/internal/mode/static/state/graph/common_filter_test.go @@ -0,0 +1,661 @@ +package graph + +import ( + "errors" + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/util/validation/field" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" +) + +func TestValidateFilter(t *testing.T) { + t.Parallel() + + tests := []struct { + filter Filter + name string + expectErrCount int + }{ + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + }, + expectErrCount: 0, + name: "valid HTTP redirect filter", + }, + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterURLRewrite, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, + }, + expectErrCount: 0, + name: "valid HTTP rewrite filter", + }, + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + expectErrCount: 0, + name: "valid HTTP request header modifiers filter", + }, + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + expectErrCount: 0, + name: "valid HTTP response header modifiers filter", + }, + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + }, + expectErrCount: 0, + name: "valid HTTP extension ref filter", + }, + { + filter: Filter{ + RouteType: RouteTypeHTTP, + FilterType: FilterRequestMirror, + }, + expectErrCount: 1, + name: "unsupported HTTP filter type", + }, + { + filter: Filter{ + RouteType: RouteTypeGRPC, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + expectErrCount: 0, + name: "valid GRPC request header modifiers filter", + }, + { + filter: Filter{ + RouteType: RouteTypeGRPC, + FilterType: FilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + expectErrCount: 0, + name: "valid GRPC response header modifiers filter", + }, + { + filter: Filter{ + RouteType: RouteTypeGRPC, + FilterType: FilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + }, + expectErrCount: 0, + name: "valid GRPC extension ref filter", + }, + { + filter: Filter{ + RouteType: RouteTypeGRPC, + FilterType: FilterURLRewrite, + }, + expectErrCount: 1, + name: "unsupported GRPC filter type", + }, + } + + filterPath := field.NewPath("test") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + allErrs := validateFilter(&validationfakes.FakeHTTPFieldsValidator{}, test.filter, filterPath) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + }) + } +} + +func TestValidateFilterResponseHeaderModifier(t *testing.T) { + t.Parallel() + + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + return v + } + + tests := []struct { + filter gatewayv1.HTTPRouteFilter + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectErrCount int + }{ + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + }, + Remove: []string{"Cache-Control"}, + }, + }, + expectErrCount: 0, + name: "valid response header modifier filter", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: nil, + }, + expectErrCount: 1, + name: "nil response header modifier filter", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "$var_name", Value: "gzip"}, + }, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid add", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"$var-name"}, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid remove", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "yhu$"}, + }, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid header value", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Host", Value: "my_host"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "}90yh&$", Value: "gzip$"}, + {Name: "}67yh&$", Value: "compress$"}, + }, + Remove: []string{"Cache-Control$}"}, + }, + }, + expectErrCount: 7, + name: "response header modifier filter all fields invalid", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + {Name: "mYbespokeHEader", Value: "duplicate"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + {Name: "accept-encodING", Value: "gzip"}, + }, + Remove: []string{"Cache-Control", "cache-control"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter not unique names", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Content-Length", Value: "163"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Content-Type", Value: "text/plain"}, + }, + Remove: []string{"X-Pad"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter with disallowed header name", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "X-Accel-Redirect", Value: "/protected/iso.img"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "X-Accel-Limit-Rate", Value: "1024"}, + }, + Remove: []string{"X-Accel-Charset"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter with disallowed header name prefix", + }, + } + + filterPath := field.NewPath("test") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + allErrs := validateFilterResponseHeaderModifier( + test.validator, test.filter.ResponseHeaderModifier, filterPath, + ) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + }) + } +} + +func TestValidateFilterRequestHeaderModifier(t *testing.T) { + t.Parallel() + + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + return v + } + + tests := []struct { + filter gatewayv1.HTTPRouteFilter + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectErrCount int + }{ + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + }, + Remove: []string{"Cache-Control"}, + }, + }, + expectErrCount: 0, + name: "valid request header modifier filter", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: nil, + }, + expectErrCount: 1, + name: "nil request header modifier filter", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "$var_name", Value: "gzip"}, + }, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid add", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"$var-name"}, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid remove", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "yhu$"}, + }, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid header value", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Host", Value: "my_host"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "}90yh&$", Value: "gzip$"}, + {Name: "}67yh&$", Value: "compress$"}, + }, + Remove: []string{"Cache-Control$}"}, + }, + }, + expectErrCount: 7, + name: "request header modifier filter all fields invalid", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + {Name: "mYbespokeHEader", Value: "duplicate"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + {Name: "accept-encodING", Value: "gzip"}, + }, + Remove: []string{"Cache-Control", "cache-control"}, + }, + }, + expectErrCount: 3, + name: "request header modifier filter not unique names", + }, + } + + filterPath := field.NewPath("test") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + allErrs := validateFilterHeaderModifier( + test.validator, test.filter.RequestHeaderModifier, filterPath, + ) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + }) + } +} + +func TestConvertGRPCFilters(t *testing.T) { + t.Parallel() + + requestHeaderFilter1 := &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"request-1"}, + } + requestHeaderFilter2 := &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"request-2"}, + } + + tests := []struct { + name string + grpcFilters []gatewayv1.GRPCRouteFilter + expFilters []Filter + }{ + { + name: "nil filters", + grpcFilters: nil, + expFilters: []Filter{}, + }, + { + name: "empty filters", + grpcFilters: []gatewayv1.GRPCRouteFilter{}, + expFilters: []Filter{}, + }, + { + name: "all filter types", + grpcFilters: []gatewayv1.GRPCRouteFilter{ + { + Type: gatewayv1.GRPCRouteFilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter1, + }, + { + Type: gatewayv1.GRPCRouteFilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter2, // duplicates are added + }, + { + Type: gatewayv1.GRPCRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + { + Type: gatewayv1.GRPCRouteFilterRequestMirror, + RequestMirror: &gatewayv1.HTTPRequestMirrorFilter{}, + }, + { + Type: gatewayv1.GRPCRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{}, + }, + }, + expFilters: []Filter{ + { + RouteType: RouteTypeGRPC, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter1, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter2, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterRequestMirror, + RequestMirror: &gatewayv1.HTTPRequestMirrorFilter{}, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{}, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + convertedFilters := convertGRPCRouteFilters(test.grpcFilters) + g.Expect(convertedFilters).To(Equal(test.expFilters)) + }) + } +} + +func TestConvertHTTPFilters(t *testing.T) { + t.Parallel() + + requestHeaderFilter1 := &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"request-1"}, + } + requestHeaderFilter2 := &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"request-2"}, + } + + tests := []struct { + name string + httpFilters []gatewayv1.HTTPRouteFilter + expFilters []Filter + }{ + { + name: "nil filters", + httpFilters: nil, + expFilters: []Filter{}, + }, + { + name: "empty filters", + httpFilters: []gatewayv1.HTTPRouteFilter{}, + expFilters: []Filter{}, + }, + { + name: "all filter types", + httpFilters: []gatewayv1.HTTPRouteFilter{ + { + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter1, + }, + { + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter2, // duplicates are added + }, + { + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + { + Type: gatewayv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + }, + { + Type: gatewayv1.HTTPRouteFilterURLRewrite, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, + }, + { + Type: gatewayv1.HTTPRouteFilterRequestMirror, + RequestMirror: &gatewayv1.HTTPRequestMirrorFilter{}, + }, + { + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{}, + }, + }, + expFilters: []Filter{ + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter1, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: requestHeaderFilter2, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterURLRewrite, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterRequestMirror, + RequestMirror: &gatewayv1.HTTPRequestMirrorFilter{}, + }, + { + RouteType: RouteTypeHTTP, + FilterType: FilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{}, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + convertedFilters := convertHTTPRouteFilters(test.httpFilters) + g.Expect(convertedFilters).To(Equal(test.expFilters)) + }) + } +} diff --git a/internal/mode/static/state/graph/extension_ref_filter.go b/internal/mode/static/state/graph/extension_ref_filter.go new file mode 100644 index 0000000000..0444e08f98 --- /dev/null +++ b/internal/mode/static/state/graph/extension_ref_filter.go @@ -0,0 +1,49 @@ +package graph + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + v1 "sigs.k8s.io/gateway-api/apis/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" +) + +// ExtensionRefFilter are NGF-specific extensions to the "filter" behavior. +type ExtensionRefFilter struct { + // SnippetsFilter contains the SnippetsFilter. Will be non-nil if the Ref.Kind is SnippetsFilter and the + // SnippetsFilter exists. + // Once we support more filters, we can extend this struct with more filter kinds. + SnippetsFilter *SnippetsFilter + // Valid indicates whether the filter is valid. + Valid bool +} + +// resolveExtRefFilter resolves a LocalObjectReference to an *ExtensionRefFilter. +// If it cannot be resolved, *ExtensionRefFilter will be nil. +type resolveExtRefFilter func(ref v1.LocalObjectReference) *ExtensionRefFilter + +func validateExtensionRefFilter(ref *v1.LocalObjectReference, path *field.Path) field.ErrorList { + var allErrs field.ErrorList + + extRefPath := path.Child("extensionRef") + + if ref == nil { + return field.ErrorList{field.Required(extRefPath, "extensionRef cannot be nil")} + } + + if ref.Name == "" { + allErrs = append(allErrs, field.Required(extRefPath, "name cannot be empty")) + } + + if ref.Group != ngfAPI.GroupName { + allErrs = append(allErrs, field.NotSupported(extRefPath, ref.Group, []string{ngfAPI.GroupName})) + } + + switch ref.Kind { + case kinds.SnippetsFilter: + default: + allErrs = append(allErrs, field.NotSupported(extRefPath, ref.Kind, []string{kinds.SnippetsFilter})) + } + + return allErrs +} diff --git a/internal/mode/static/state/graph/extension_ref_filter_test.go b/internal/mode/static/state/graph/extension_ref_filter_test.go new file mode 100644 index 0000000000..780188be31 --- /dev/null +++ b/internal/mode/static/state/graph/extension_ref_filter_test.go @@ -0,0 +1,105 @@ +package graph + +import ( + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/util/validation/field" + v1 "sigs.k8s.io/gateway-api/apis/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" +) + +func TestValidateExtensionRefFilter(t *testing.T) { + t.Parallel() + testPath := field.NewPath("test") + + tests := []struct { + ref *v1.LocalObjectReference + name string + errSubString []string + expErrCount int + }{ + { + name: "nil ref", + ref: nil, + expErrCount: 1, + errSubString: []string{ + `test.extensionRef: Required value: extensionRef cannot be nil`, + }, + }, + { + name: "empty ref", + ref: &v1.LocalObjectReference{}, + expErrCount: 3, + errSubString: []string{ + `test.extensionRef: Required value: name cannot be empty`, + `test.extensionRef: Unsupported value: "": supported values: "gateway.nginx.org"`, + `test.extensionRef: Unsupported value: "": supported values: "SnippetsFilter"`, + }, + }, + { + name: "ref missing name", + ref: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + }, + expErrCount: 1, + errSubString: []string{ + `test.extensionRef: Required value: name cannot be empty`, + }, + }, + { + name: "ref unsupported group", + ref: &v1.LocalObjectReference{ + Name: v1.ObjectName("filter"), + Group: "unsupported", + Kind: kinds.SnippetsFilter, + }, + expErrCount: 1, + errSubString: []string{ + `test.extensionRef: Unsupported value: "unsupported": supported values: "gateway.nginx.org"`, + }, + }, + { + name: "ref unsupported kind", + ref: &v1.LocalObjectReference{ + Name: v1.ObjectName("filter"), + Group: ngfAPI.GroupName, + Kind: "unsupported", + }, + expErrCount: 1, + errSubString: []string{ + `test.extensionRef: Unsupported value: "unsupported": supported values: "SnippetsFilter"`, + }, + }, + { + name: "valid ref", + ref: &v1.LocalObjectReference{ + Name: v1.ObjectName("filter"), + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + }, + expErrCount: 0, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + g := NewWithT(t) + + errs := validateExtensionRefFilter(test.ref, testPath) + g.Expect(errs).To(HaveLen(test.expErrCount)) + + if len(test.errSubString) > 0 { + aggregateErrStr := errs.ToAggregate().Error() + for _, ss := range test.errSubString { + g.Expect(aggregateErrStr).To(ContainSubstring(ss)) + } + } + }) + } +} diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 20ea60153a..df39c59423 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -226,6 +226,7 @@ func BuildGraph( state.GRPCRoutes, processedGws.GetAllNsNames(), npCfg, + processedSnippetsFilters, ) l4routes := buildL4RoutesForGateways( diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index d59f96dd57..1fbc26ef7c 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -111,6 +111,60 @@ func TestBuildGraph(t *testing.T) { }, } + refSnippetsFilterExtensionRef := &gatewayv1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "ref-snippets-filter", + } + + unreferencedSnippetsFilter := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unref-snippets-filter", + Namespace: testNs, + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextMain, + Value: "main snippet", + }, + }, + }, + } + + referencedSnippetsFilter := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ref-snippets-filter", + Namespace: testNs, + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextHTTPServer, + Value: "server snippet", + }, + }, + }, + } + + processedUnrefSnippetsFilter := &SnippetsFilter{ + Source: unreferencedSnippetsFilter, + Valid: true, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextMain: "main snippet", + }, + } + + processedRefSnippetsFilter := &SnippetsFilter{ + Source: referencedSnippetsFilter, + Valid: true, + Referenced: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServer: "server snippet", + }, + } + createValidRuleWithBackendRefs := func(matches []gatewayv1.HTTPRouteMatch) RouteRule { refs := []BackendRef{ { @@ -127,14 +181,40 @@ func TestBuildGraph(t *testing.T) { }, } return RouteRule{ - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Filters: []Filter{}, + Valid: true, + }, BackendRefs: refs, Matches: matches, RouteBackendRefs: rbrs, } } + createValidRuleWithBackendRefsAndFilters := func( + matches []gatewayv1.HTTPRouteMatch, + routeType RouteType, + ) RouteRule { + rule := createValidRuleWithBackendRefs(matches) + rule.Filters = RouteRuleFilters{ + Filters: []Filter{ + { + RouteType: routeType, + FilterType: FilterExtensionRef, + ExtensionRef: refSnippetsFilterExtensionRef, + ResolvedExtensionRef: &ExtensionRefFilter{ + SnippetsFilter: processedRefSnippetsFilter, + Valid: true, + }, + }, + }, + Valid: true, + } + + return rule + } + routeMatches := []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ @@ -207,6 +287,15 @@ func TestBuildGraph(t *testing.T) { } hr1 := createRoute("hr-1", "gateway-1", "listener-80-1") + addFilterToPath( + hr1, + "/", + gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: refSnippetsFilterExtensionRef, + }, + ) + hr2 := createRoute("hr-2", "wrong-gateway", "listener-80-1") hr3 := createRoute("hr-3", "gateway-1", "listener-443-1") // https listener; should not conflict with hr1 @@ -239,6 +328,12 @@ func TestBuildGraph(t *testing.T) { BackendRef: commonGWBackendRef, }, }, + Filters: []gatewayv1.GRPCRouteFilter{ + { + Type: gatewayv1.GRPCRouteFilterExtensionRef, + ExtensionRef: refSnippetsFilterExtensionRef, + }, + }, }, }, }, @@ -515,26 +610,6 @@ func TestBuildGraph(t *testing.T) { Valid: true, } - snippetsFilter := &ngfAPI.SnippetsFilter{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-snippet-filter", - Namespace: testNs, - }, - Spec: ngfAPI.SnippetsFilterSpec{ - Snippets: []ngfAPI.Snippet{ - { - Context: ngfAPI.NginxContextMain, - Value: "main snippet", - }, - }, - }, - } - - processedSnippetsFilter := &SnippetsFilter{ - Source: snippetsFilter, - Valid: true, - } - createStateWithGatewayClass := func(gc *gatewayv1.GatewayClass) ClusterState { return ClusterState{ GatewayClasses: map[types.NamespacedName]*gatewayv1.GatewayClass{ @@ -585,7 +660,8 @@ func TestBuildGraph(t *testing.T) { gwPolicyKey: gwPolicy, }, SnippetsFilters: map[types.NamespacedName]*ngfAPI.SnippetsFilter{ - client.ObjectKeyFromObject(snippetsFilter): snippetsFilter, + client.ObjectKeyFromObject(unreferencedSnippetsFilter): unreferencedSnippetsFilter, + client.ObjectKeyFromObject(referencedSnippetsFilter): referencedSnippetsFilter, }, } } @@ -609,7 +685,7 @@ func TestBuildGraph(t *testing.T) { }, Spec: L7RouteSpec{ Hostnames: hr1.Spec.Hostnames, - Rules: []RouteRule{createValidRuleWithBackendRefs(routeMatches)}, + Rules: []RouteRule{createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeHTTP)}, }, Policies: []*Policy{processedRoutePolicy}, } @@ -696,7 +772,7 @@ func TestBuildGraph(t *testing.T) { Spec: L7RouteSpec{ Hostnames: gr.Spec.Hostnames, Rules: []RouteRule{ - createValidRuleWithBackendRefs(routeMatches), + createValidRuleWithBackendRefsAndFilters(routeMatches, RouteTypeGRPC), }, }, } @@ -834,7 +910,8 @@ func TestBuildGraph(t *testing.T) { TelemetryEnabled: true, }, SnippetsFilters: map[types.NamespacedName]*SnippetsFilter{ - client.ObjectKeyFromObject(snippetsFilter): processedSnippetsFilter, + client.ObjectKeyFromObject(unreferencedSnippetsFilter): processedUnrefSnippetsFilter, + client.ObjectKeyFromObject(referencedSnippetsFilter): processedRefSnippetsFilter, }, } } diff --git a/internal/mode/static/state/graph/grpcroute.go b/internal/mode/static/state/graph/grpcroute.go index ea3f576972..4d4ed05b75 100644 --- a/internal/mode/static/state/graph/grpcroute.go +++ b/internal/mode/static/state/graph/grpcroute.go @@ -5,6 +5,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" v1 "sigs.k8s.io/gateway-api/apis/v1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -15,6 +16,7 @@ func buildGRPCRoute( ghr *v1.GRPCRoute, gatewayNsNames []types.NamespacedName, http2disabled bool, + snippetsFilters map[types.NamespacedName]*SnippetsFilter, ) *L7Route { r := &L7Route{ Source: ghr, @@ -52,95 +54,124 @@ func buildGRPCRoute( } r.Spec.Hostnames = ghr.Spec.Hostnames - - r.Valid = true r.Attachable = true - rules, atLeastOneValid, allRulesErrs := processGRPCRouteRules(ghr.Spec.Rules, validator) + rules, valid, conds := processGRPCRouteRules( + ghr.Spec.Rules, + validator, + getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), + ) r.Spec.Rules = rules + r.Valid = valid + r.Conditions = append(r.Conditions, conds...) - if len(allRulesErrs) > 0 { - msg := allRulesErrs.ToAggregate().Error() + return r +} - if atLeastOneValid { - r.Conditions = append(r.Conditions, staticConds.NewRoutePartiallyInvalid(msg)) - } else { - msg = "All rules are invalid: " + msg - r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(msg)) +func processGRPCRouteRule( + specRule v1.GRPCRouteRule, + rulePath *field.Path, + validator validation.HTTPFieldsValidator, + resolveExtRefFunc resolveExtRefFilter, +) (RouteRule, routeRuleErrors) { + var errors routeRuleErrors - r.Valid = false + validMatches := true + + for j, match := range specRule.Matches { + matchPath := rulePath.Child("matches").Index(j) + + matchesErrs := validateGRPCMatch(validator, match, matchPath) + if len(matchesErrs) > 0 { + validMatches = false + errors.invalid = append(errors.invalid, matchesErrs...) } } - return r + routeFilters, filterErrors := processRouteRuleFilters( + convertGRPCRouteFilters(specRule.Filters), + rulePath.Child("filters"), + validator, + resolveExtRefFunc, + ) + + errors = errors.append(filterErrors) + + backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) + + // rule.BackendRefs are validated separately because of their special requirements + for _, b := range specRule.BackendRefs { + var interfaceFilters []interface{} + if len(b.Filters) > 0 { + interfaceFilters = make([]interface{}, 0, len(b.Filters)) + for i, v := range b.Filters { + interfaceFilters[i] = v + } + } + rbr := RouteBackendRef{ + BackendRef: b.BackendRef, + Filters: interfaceFilters, + } + backendRefs = append(backendRefs, rbr) + } + + return RouteRule{ + ValidMatches: validMatches, + Matches: convertGRPCMatches(specRule.Matches), + Filters: routeFilters, + RouteBackendRefs: backendRefs, + }, errors } func processGRPCRouteRules( specRules []v1.GRPCRouteRule, validator validation.HTTPFieldsValidator, -) (rules []RouteRule, atLeastOneValid bool, allRulesErrs field.ErrorList) { + resolveExtRefFunc resolveExtRefFilter, +) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) + var ( + allRulesErrors routeRuleErrors + atLeastOneValid bool + ) + for i, rule := range specRules { rulePath := field.NewPath("spec").Child("rules").Index(i) - var allErrs field.ErrorList - var matchesErrs field.ErrorList - var filtersErrs field.ErrorList + rr, errors := processGRPCRouteRule(rule, rulePath, validator, resolveExtRefFunc) - for j, match := range rule.Matches { - matchPath := rulePath.Child("matches").Index(j) - matchesErrs = append(matchesErrs, validateGRPCMatch(validator, match, matchPath)...) - } - - for j, filter := range rule.Filters { - filterPath := rulePath.Child("filters").Index(j) - filtersErrs = append(filtersErrs, validateGRPCFilter(validator, filter, filterPath)...) + if rr.ValidMatches && rr.Filters.Valid { + atLeastOneValid = true } - backendRefs := make([]RouteBackendRef, 0, len(rule.BackendRefs)) - - // rule.BackendRefs are validated separately because of their special requirements - for _, b := range rule.BackendRefs { - var interfaceFilters []interface{} - if len(b.Filters) > 0 { - interfaceFilters = make([]interface{}, 0, len(b.Filters)) - for i, v := range b.Filters { - interfaceFilters[i] = v - } - } - rbr := RouteBackendRef{ - BackendRef: b.BackendRef, - Filters: interfaceFilters, - } - backendRefs = append(backendRefs, rbr) - } + allRulesErrors = allRulesErrors.append(errors) - allErrs = append(allErrs, matchesErrs...) - allErrs = append(allErrs, filtersErrs...) - allRulesErrs = append(allRulesErrs, allErrs...) + rules[i] = rr + } - if len(allErrs) == 0 { - atLeastOneValid = true - } + conds = make([]conditions.Condition, 0, 2) + valid = true - validFilters := len(filtersErrs) == 0 + if len(allRulesErrors.invalid) > 0 { + msg := allRulesErrors.invalid.ToAggregate().Error() - var convertedFilters []v1.HTTPRouteFilter - if validFilters { - convertedFilters = convertGRPCFilters(rule.Filters) + if atLeastOneValid { + conds = append(conds, staticConds.NewRoutePartiallyInvalid(msg)) + } else { + msg = "All rules are invalid: " + msg + conds = append(conds, staticConds.NewRouteUnsupportedValue(msg)) + valid = false } + } - rules[i] = RouteRule{ - ValidMatches: len(matchesErrs) == 0, - ValidFilters: validFilters, - Matches: convertGRPCMatches(rule.Matches), - Filters: convertedFilters, - RouteBackendRefs: backendRefs, - } + // resolve errors do not invalidate routes + if len(allRulesErrors.resolve) > 0 { + msg := allRulesErrors.resolve.ToAggregate().Error() + conds = append(conds, staticConds.NewRouteResolvedRefsInvalidFilter(msg)) } - return rules, atLeastOneValid, allRulesErrs + + return rules, valid, conds } func convertGRPCMatches(grpcMatches []v1.GRPCRouteMatch) []v1.HTTPRouteMatch { @@ -246,57 +277,3 @@ func validateGRPCMethodMatch( } return allErrs } - -func validateGRPCFilter( - validator validation.HTTPFieldsValidator, - filter v1.GRPCRouteFilter, - filterPath *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - switch filter.Type { - case v1.GRPCRouteFilterRequestHeaderModifier: - return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath.Child(string(filter.Type))) - case v1.GRPCRouteFilterResponseHeaderModifier: - return validateFilterHeaderModifier(validator, filter.ResponseHeaderModifier, filterPath.Child(string(filter.Type))) - default: - valErr := field.NotSupported( - filterPath.Child("type"), - filter.Type, - []string{ - string(v1.GRPCRouteFilterRequestHeaderModifier), - string(v1.GRPCRouteFilterResponseHeaderModifier), - }, - ) - allErrs = append(allErrs, valErr) - return allErrs - } -} - -// convertGRPCFilters converts GRPCRouteFilters (a subset of HTTPRouteFilter) to HTTPRouteFilters -// so we can reuse the logic from HTTPRoute filter validation and processing. -func convertGRPCFilters(filters []v1.GRPCRouteFilter) []v1.HTTPRouteFilter { - if len(filters) == 0 { - return nil - } - httpFilters := make([]v1.HTTPRouteFilter, 0, len(filters)) - for _, filter := range filters { - switch filter.Type { - case v1.GRPCRouteFilterRequestHeaderModifier: - httpRequestHeaderFilter := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: filter.RequestHeaderModifier, - } - httpFilters = append(httpFilters, httpRequestHeaderFilter) - case v1.GRPCRouteFilterResponseHeaderModifier: - httpResponseHeaderFilter := v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: filter.ResponseHeaderModifier, - } - httpFilters = append(httpFilters, httpResponseHeaderFilter) - default: - continue - } - } - return httpFilters -} diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 7e8e8b4582..1045939bfd 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -13,6 +13,7 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) @@ -82,7 +83,25 @@ func TestBuildGRPCRoutes(t *testing.T) { t.Parallel() gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} - gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{}) + snippetsFilterRef := v1.GRPCRouteFilter{ + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Name: "sf", + Kind: kinds.SnippetsFilter, + Group: ngfAPI.GroupName, + }, + } + + requestHeaderFilter := v1.GRPCRouteFilter{ + Type: v1.GRPCRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &v1.HTTPHeaderFilter{}, + } + + grRuleWithFilters := v1.GRPCRouteRule{ + Filters: []v1.GRPCRouteFilter{snippetsFilterRef, requestHeaderFilter}, + } + + gr := createGRPCRoute("gr-1", gwNsName.Name, "example.com", []v1.GRPCRouteRule{grRuleWithFilters}) grWrongGateway := createGRPCRoute("gr-2", "some-gateway", "example.com", []v1.GRPCRouteRule{}) @@ -91,6 +110,21 @@ func TestBuildGRPCRoutes(t *testing.T) { client.ObjectKeyFromObject(grWrongGateway): grWrongGateway, } + sf := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "sf", + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextHTTP, + Value: "http snippet", + }, + }, + }, + } + tests := []struct { expected map[RouteKey]*L7Route name string @@ -113,7 +147,39 @@ func TestBuildGRPCRoutes(t *testing.T) { Attachable: true, Spec: L7RouteSpec{ Hostnames: gr.Spec.Hostnames, - Rules: []RouteRule{}, + Rules: []RouteRule{ + { + Matches: convertGRPCMatches(gr.Spec.Rules[0].Matches), + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{ + { + ExtensionRef: snippetsFilterRef.ExtensionRef, + ResolvedExtensionRef: &ExtensionRefFilter{ + SnippetsFilter: &SnippetsFilter{ + Source: sf, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTP: "http snippet", + }, + Valid: true, + Referenced: true, + }, + Valid: true, + }, + RouteType: RouteTypeGRPC, + FilterType: FilterExtensionRef, + }, + { + RequestHeaderModifier: &v1.HTTPHeaderFilter{}, + RouteType: RouteTypeGRPC, + FilterType: FilterRequestHeaderModifier, + }, + }, + }, + ValidMatches: true, + RouteBackendRefs: []RouteBackendRef{}, + }, + }, }, }, }, @@ -140,12 +206,24 @@ func TestBuildGRPCRoutes(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) + + snippetsFilters := map[types.NamespacedName]*SnippetsFilter{ + client.ObjectKeyFromObject(sf): { + Source: sf, + Valid: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTP: "http snippet", + }, + }, + } + routes := buildRoutesForGateways( validator, map[types.NamespacedName]*v1.HTTPRoute{}, grRoutes, test.gwNsNames, npCfg, + snippetsFilters, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -252,6 +330,11 @@ func TestBuildGRPCRoute(t *testing.T) { ) grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + validSnippetsFilterRef := &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf", + } grValidFilterRule.Filters = []v1.GRPCRouteFilter{ { @@ -268,6 +351,10 @@ func TestBuildGRPCRoute(t *testing.T) { }, }, }, + { + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: validSnippetsFilterRef, + }, } grValidFilter := createGRPCRoute( @@ -277,22 +364,70 @@ func TestBuildGRPCRoute(t *testing.T) { []v1.GRPCRouteRule{grValidFilterRule}, ) - convertedFilters := []v1.HTTPRouteFilter{ + // route with invalid snippets filter extension ref + grInvalidSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grInvalidSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ { - Type: v1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &v1.HTTPHeaderFilter{ - Remove: []string{"header"}, + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: "wrong", + Kind: kinds.SnippetsFilter, + Name: "sf", }, }, + } + grInvalidSnippetsFilter := createGRPCRoute( + "gr", + gatewayNsName.Name, + "example.com", + []v1.GRPCRouteRule{grInvalidSnippetsFilterRule}, + ) + + // route with unresolvable snippets filter extension ref + grUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grUnresolvableSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ { - Type: v1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &v1.HTTPHeaderFilter{ - Add: []v1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "does-not-exist", }, }, } + grUnresolvableSnippetsFilter := createGRPCRoute( + "gr", + gatewayNsName.Name, + "example.com", + []v1.GRPCRouteRule{grUnresolvableSnippetsFilterRule}, + ) + + // route with two invalid snippets filter extensions refs: (1) invalid group (2) unresolvable + grInvalidAndUnresolvableSnippetsFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + grInvalidAndUnresolvableSnippetsFilterRule.Filters = []v1.GRPCRouteFilter{ + { + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "does-not-exist", + }, + }, + { + Type: v1.GRPCRouteFilterExtensionRef, + ExtensionRef: &v1.LocalObjectReference{ + Group: "wrong", + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + }, + } + grInvalidAndUnresolvableSnippetsFilter := createGRPCRoute( + "gr", + gatewayNsName.Name, + "example.com", + []v1.GRPCRouteRule{grInvalidAndUnresolvableSnippetsFilterRule}, + ) createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { v := &validationfakes.FakeHTTPFieldsValidator{} @@ -326,14 +461,20 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grBoth.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grBoth.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grBoth.Spec.Rules[1].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -361,8 +502,11 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grEmptyMatch.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grEmptyMatch.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{{BackendRef: backendRef}}, }, @@ -391,10 +535,41 @@ func TestBuildGRPCRoute(t *testing.T) { Rules: []RouteRule{ { ValidMatches: true, - ValidFilters: true, Matches: convertGRPCMatches(grValidFilter.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, - Filters: convertedFilters, + Filters: RouteRuleFilters{ + Filters: []Filter{ + { + RouteType: RouteTypeGRPC, + FilterType: FilterRequestHeaderModifier, + RequestHeaderModifier: &v1.HTTPHeaderFilter{ + Remove: []string{"header"}, + }, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterResponseHeaderModifier, + ResponseHeaderModifier: &v1.HTTPHeaderFilter{ + Add: []v1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + }, + }, + }, + { + RouteType: RouteTypeGRPC, + FilterType: FilterExtensionRef, + ExtensionRef: validSnippetsFilterRef, + ResolvedExtensionRef: &ExtensionRefFilter{ + SnippetsFilter: &SnippetsFilter{ + Valid: true, + Referenced: true, + }, + Valid: true, + }, + }, + }, + Valid: true, + }, }, }, }, @@ -428,8 +603,11 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grInvalidMatchesEmptyMethodFields.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grInvalidMatchesEmptyMethodFields.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -468,8 +646,11 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grInvalidMatchesInvalidMethodFields.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grInvalidMatchesInvalidMethodFields.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -533,14 +714,20 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grOneInvalid.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grOneInvalid.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grOneInvalid.Spec.Rules[1].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -574,8 +761,11 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grInvalidHeadersEmptyType.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grInvalidHeadersEmptyType.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -608,8 +798,11 @@ func TestBuildGRPCRoute(t *testing.T) { Hostnames: grInvalidMatchesNilMethodType.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: convertGRPCMatches(grInvalidMatchesNilMethodType.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -635,16 +828,20 @@ func TestBuildGRPCRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRouteUnsupportedValue( - `All rules are invalid: spec.rules[0].filters[0].type: ` + - `Unsupported value: "RequestMirror": supported values: "RequestHeaderModifier", "ResponseHeaderModifier"`, + `All rules are invalid: spec.rules[0].filters[0].type: Unsupported value: ` + + `"RequestMirror": supported values: "ResponseHeaderModifier", ` + + `"RequestHeaderModifier", "ExtensionRef"`, ), }, Spec: L7RouteSpec{ Hostnames: grInvalidFilter.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: false, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertGRPCRouteFilters(grInvalidFilter.Spec.Rules[0].Filters), + }, Matches: convertGRPCMatches(grInvalidFilter.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, }, @@ -682,6 +879,129 @@ func TestBuildGRPCRoute(t *testing.T) { }, name: "invalid hostname", }, + { + validator: createAllValidValidator(), + gr: grInvalidSnippetsFilter, + expected: &L7Route{ + Source: grInvalidSnippetsFilter, + RouteType: RouteTypeGRPC, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: grInvalidSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + "All rules are invalid: spec.rules[0].filters[0].extensionRef: " + + "Unsupported value: \"wrong\": supported values: \"gateway.nginx.org\"", + ), + }, + Spec: L7RouteSpec{ + Hostnames: grInvalidSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertGRPCRouteFilters(grInvalidSnippetsFilter.Spec.Rules[0].Filters), + }, + Matches: convertGRPCMatches(grInvalidSnippetsFilter.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + + name: "invalid snippet filter extension ref", + }, + { + validator: createAllValidValidator(), + gr: grUnresolvableSnippetsFilter, + expected: &L7Route{ + Source: grUnresolvableSnippetsFilter, + RouteType: RouteTypeGRPC, + Valid: true, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: grUnresolvableSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteResolvedRefsInvalidFilter( + "spec.rules[0].filters[0].extensionRef: Not found: " + + "v1.LocalObjectReference{Group:\"gateway.nginx.org\", Kind:\"SnippetsFilter\", " + + "Name:\"does-not-exist\"}", + ), + }, + Spec: L7RouteSpec{ + Hostnames: grUnresolvableSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertGRPCRouteFilters(grUnresolvableSnippetsFilter.Spec.Rules[0].Filters), + }, + Matches: convertGRPCMatches(grUnresolvableSnippetsFilter.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + + name: "unresolvable snippet filter extension ref", + }, + { + validator: createAllValidValidator(), + gr: grInvalidAndUnresolvableSnippetsFilter, + expected: &L7Route{ + Source: grInvalidAndUnresolvableSnippetsFilter, + RouteType: RouteTypeGRPC, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: grInvalidAndUnresolvableSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + "All rules are invalid: spec.rules[0].filters[1].extensionRef: " + + "Unsupported value: \"wrong\": supported values: \"gateway.nginx.org\"", + ), + staticConds.NewRouteResolvedRefsInvalidFilter( + "spec.rules[0].filters[0].extensionRef: Not found: " + + "v1.LocalObjectReference{Group:\"gateway.nginx.org\", Kind:\"SnippetsFilter\", " + + "Name:\"does-not-exist\"}", + ), + }, + Spec: L7RouteSpec{ + Hostnames: grInvalidAndUnresolvableSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertGRPCRouteFilters(grInvalidAndUnresolvableSnippetsFilter.Spec.Rules[0].Filters), + }, + Matches: convertGRPCMatches(grInvalidAndUnresolvableSnippetsFilter.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + + name: "one invalid and one unresolvable snippet filter extension ref", + }, } gatewayNsNames := []types.NamespacedName{gatewayNsName} @@ -691,7 +1011,11 @@ func TestBuildGRPCRoute(t *testing.T) { t.Parallel() g := NewWithT(t) - route := buildGRPCRoute(test.validator, test.gr, gatewayNsNames, test.http2disabled) + snippetsFilters := map[types.NamespacedName]*SnippetsFilter{ + {Namespace: "test", Name: "sf"}: {Valid: true}, + } + + route := buildGRPCRoute(test.validator, test.gr, gatewayNsNames, test.http2disabled, snippetsFilters) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -769,42 +1093,3 @@ func TestConvertGRPCMatches(t *testing.T) { }) } } - -func TestConvertGRPCFilters(t *testing.T) { - t.Parallel() - grFilters := []v1.GRPCRouteFilter{ - { - Type: "RequestHeaderModifier", - RequestHeaderModifier: &v1.HTTPHeaderFilter{ - Remove: []string{"header"}, - }, - }, - { - Type: "ResponseHeaderModifier", - ResponseHeaderModifier: &v1.HTTPHeaderFilter{ - Add: []v1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, - }, - }, - { - Type: "RequestMirror", - }, - } - - expectedHTTPFilters := []v1.HTTPRouteFilter{ - { - Type: v1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: grFilters[0].RequestHeaderModifier, - }, - { - Type: v1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: grFilters[1].ResponseHeaderModifier, - }, - } - - g := NewWithT(t) - - httpFilters := convertGRPCFilters(grFilters) - g.Expect(helpers.Diff(expectedHTTPFilters, httpFilters)).To(BeEmpty()) -} diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index 1b5bdf3676..8cc3b66ea1 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -8,6 +8,8 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" v1 "sigs.k8s.io/gateway-api/apis/v1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -23,6 +25,7 @@ func buildHTTPRoute( validator validation.HTTPFieldsValidator, ghr *v1.HTTPRoute, gatewayNsNames []types.NamespacedName, + snippetsFilters map[types.NamespacedName]*SnippetsFilter, ) *L7Route { r := &L7Route{ Source: ghr, @@ -52,87 +55,125 @@ func buildHTTPRoute( } r.Spec.Hostnames = ghr.Spec.Hostnames - - r.Valid = true r.Attachable = true - rules, atLeastOneValid, allRulesErrs := processHTTPRouteRules(ghr.Spec.Rules, validator) + rules, valid, conds := processHTTPRouteRules( + ghr.Spec.Rules, + validator, + getSnippetsFilterResolverForNamespace(snippetsFilters, r.Source.GetNamespace()), + ) r.Spec.Rules = rules + r.Conditions = append(r.Conditions, conds...) + r.Valid = valid - if len(allRulesErrs) > 0 { - msg := allRulesErrs.ToAggregate().Error() + return r +} - if atLeastOneValid { - r.Conditions = append(r.Conditions, staticConds.NewRoutePartiallyInvalid(msg)) - } else { - msg = "All rules are invalid: " + msg - r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(msg)) +func processHTTPRouteRule( + specRule v1.HTTPRouteRule, + rulePath *field.Path, + validator validation.HTTPFieldsValidator, + resolveExtRefFunc resolveExtRefFilter, +) (RouteRule, routeRuleErrors) { + var errors routeRuleErrors + + validMatches := true + + for j, match := range specRule.Matches { + matchPath := rulePath.Child("matches").Index(j) - r.Valid = false + matchesErrs := validateMatch(validator, match, matchPath) + if len(matchesErrs) > 0 { + validMatches = false + errors.invalid = append(errors.invalid, matchesErrs...) } } - return r + routeFilters, filterErrors := processRouteRuleFilters( + convertHTTPRouteFilters(specRule.Filters), + rulePath.Child("filters"), + validator, + resolveExtRefFunc, + ) + + errors = errors.append(filterErrors) + + backendRefs := make([]RouteBackendRef, 0, len(specRule.BackendRefs)) + + // rule.BackendRefs are validated separately because of their special requirements + for _, b := range specRule.BackendRefs { + var interfaceFilters []interface{} + if len(b.Filters) > 0 { + interfaceFilters = make([]interface{}, 0, len(b.Filters)) + for i, v := range b.Filters { + interfaceFilters[i] = v + } + } + rbr := RouteBackendRef{ + BackendRef: b.BackendRef, + Filters: interfaceFilters, + } + backendRefs = append(backendRefs, rbr) + } + + return RouteRule{ + ValidMatches: validMatches, + Matches: specRule.Matches, + Filters: routeFilters, + RouteBackendRefs: backendRefs, + }, errors } func processHTTPRouteRules( specRules []v1.HTTPRouteRule, validator validation.HTTPFieldsValidator, -) (rules []RouteRule, atLeastOneValid bool, allRulesErrs field.ErrorList) { + resolveExtRefFunc resolveExtRefFilter, +) (rules []RouteRule, valid bool, conds []conditions.Condition) { rules = make([]RouteRule, len(specRules)) + var ( + allRulesErrors routeRuleErrors + atLeastOneValid bool + ) + for i, rule := range specRules { rulePath := field.NewPath("spec").Child("rules").Index(i) - var matchesErrs field.ErrorList - for j, match := range rule.Matches { - matchPath := rulePath.Child("matches").Index(j) - matchesErrs = append(matchesErrs, validateMatch(validator, match, matchPath)...) - } + rr, errors := processHTTPRouteRule(rule, rulePath, validator, resolveExtRefFunc) - var filtersErrs field.ErrorList - for j, filter := range rule.Filters { - filterPath := rulePath.Child("filters").Index(j) - filtersErrs = append(filtersErrs, validateFilter(validator, filter, filterPath)...) + if rr.ValidMatches && rr.Filters.Valid { + atLeastOneValid = true } - var allErrs field.ErrorList - allErrs = append(allErrs, matchesErrs...) - allErrs = append(allErrs, filtersErrs...) - allRulesErrs = append(allRulesErrs, allErrs...) + allRulesErrors = allRulesErrors.append(errors) - if len(allErrs) == 0 { - atLeastOneValid = true - } + rules[i] = rr + } - backendRefs := make([]RouteBackendRef, 0, len(rule.BackendRefs)) + conds = make([]conditions.Condition, 0, 2) - // rule.BackendRefs are validated separately because of their special requirements - for _, b := range rule.BackendRefs { - var interfaceFilters []interface{} - if len(b.Filters) > 0 { - interfaceFilters = make([]interface{}, 0, len(b.Filters)) - for i, v := range b.Filters { - interfaceFilters[i] = v - } - } - rbr := RouteBackendRef{ - BackendRef: b.BackendRef, - Filters: interfaceFilters, - } - backendRefs = append(backendRefs, rbr) - } + valid = true + + if len(allRulesErrors.invalid) > 0 { + msg := allRulesErrors.invalid.ToAggregate().Error() - rules[i] = RouteRule{ - ValidMatches: len(matchesErrs) == 0, - ValidFilters: len(filtersErrs) == 0, - Matches: rule.Matches, - Filters: rule.Filters, - RouteBackendRefs: backendRefs, + if atLeastOneValid { + conds = append(conds, staticConds.NewRoutePartiallyInvalid(msg)) + } else { + msg = "All rules are invalid: " + msg + conds = append(conds, staticConds.NewRouteUnsupportedValue(msg)) + valid = false } } - return rules, atLeastOneValid, allRulesErrs + + // resolve errors do not invalidate routes + if len(allRulesErrors.resolve) > 0 { + msg := allRulesErrors.resolve.ToAggregate().Error() + conds = append(conds, staticConds.NewRouteResolvedRefsInvalidFilter(msg)) + } + + return rules, valid, conds } func validateMatch( @@ -228,14 +269,19 @@ func validatePathMatch( } if strings.HasPrefix(*path.Value, http.InternalRoutePathPrefix) { - msg := fmt.Sprintf("path cannot start with %s. This prefix is reserved for internal use", - http.InternalRoutePathPrefix) + msg := fmt.Sprintf( + "path cannot start with %s. This prefix is reserved for internal use", + http.InternalRoutePathPrefix, + ) return field.ErrorList{field.Invalid(fieldPath.Child("value"), *path.Value, msg)} } if *path.Type != v1.PathMatchPathPrefix && *path.Type != v1.PathMatchExact { - valErr := field.NotSupported(fieldPath.Child("type"), *path.Type, - []string{string(v1.PathMatchExact), string(v1.PathMatchPathPrefix)}) + valErr := field.NotSupported( + fieldPath.Child("type"), + *path.Type, + []string{string(v1.PathMatchExact), string(v1.PathMatchPathPrefix)}, + ) allErrs = append(allErrs, valErr) } @@ -247,40 +293,6 @@ func validatePathMatch( return allErrs } -func validateFilter( - validator validation.HTTPFieldsValidator, - filter v1.HTTPRouteFilter, - filterPath *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - switch filter.Type { - case v1.HTTPRouteFilterRequestRedirect: - return validateFilterRedirect(validator, filter.RequestRedirect, filterPath) - case v1.HTTPRouteFilterURLRewrite: - return validateFilterRewrite(validator, filter.URLRewrite, filterPath) - case v1.HTTPRouteFilterRequestHeaderModifier: - return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath.Child(string(filter.Type))) - case v1.HTTPRouteFilterResponseHeaderModifier: - return validateFilterResponseHeaderModifier( - validator, filter.ResponseHeaderModifier, filterPath.Child(string(filter.Type)), - ) - default: - valErr := field.NotSupported( - filterPath.Child("type"), - filter.Type, - []string{ - string(v1.HTTPRouteFilterRequestRedirect), - string(v1.HTTPRouteFilterURLRewrite), - string(v1.HTTPRouteFilterRequestHeaderModifier), - string(v1.HTTPRouteFilterResponseHeaderModifier), - }, - ) - allErrs = append(allErrs, valErr) - return allErrs - } -} - func validateFilterRedirect( validator validation.HTTPFieldsValidator, redirect *v1.HTTPRequestRedirectFilter, diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 667e7b8338..a655364e60 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -11,8 +11,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) @@ -91,6 +93,21 @@ func TestBuildHTTPRoutes(t *testing.T) { gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} hr := createHTTPRoute("hr-1", gwNsName.Name, "example.com", "/") + snippetsFilterRef := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Name: "sf", + Kind: kinds.SnippetsFilter, + Group: ngfAPI.GroupName, + }, + } + requestRedirectFilter := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + } + + addFilterToPath(hr, "/", snippetsFilterRef) + addFilterToPath(hr, "/", requestRedirectFilter) hrWrongGateway := createHTTPRoute("hr-2", "some-gateway", "example.com", "/") @@ -99,6 +116,21 @@ func TestBuildHTTPRoutes(t *testing.T) { client.ObjectKeyFromObject(hrWrongGateway): hrWrongGateway, } + sf := &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "sf", + }, + Spec: ngfAPI.SnippetsFilterSpec{ + Snippets: []ngfAPI.Snippet{ + { + Context: ngfAPI.NginxContextHTTP, + Value: "http snippet", + }, + }, + }, + } + tests := []struct { expected map[RouteKey]*L7Route name string @@ -123,8 +155,33 @@ func TestBuildHTTPRoutes(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{ + { + ExtensionRef: snippetsFilterRef.ExtensionRef, + ResolvedExtensionRef: &ExtensionRefFilter{ + SnippetsFilter: &SnippetsFilter{ + Source: sf, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTP: "http snippet", + }, + Valid: true, + Referenced: true, + }, + Valid: true, + }, + RouteType: RouteTypeHTTP, + FilterType: FilterExtensionRef, + }, + { + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + RouteType: RouteTypeHTTP, + FilterType: FilterRequestRedirect, + }, + }, + }, Matches: hr.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{}, }, @@ -147,12 +204,24 @@ func TestBuildHTTPRoutes(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) + + snippetsFilters := map[types.NamespacedName]*SnippetsFilter{ + client.ObjectKeyFromObject(sf): { + Source: sf, + Valid: true, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTP: "http snippet", + }, + }, + } + routes := buildRoutesForGateways( validator, hrRoutes, map[types.NamespacedName]*gatewayv1.GRPCRoute{}, test.gwNsNames, nil, + snippetsFilters, ) g.Expect(helpers.Diff(test.expected, routes)).To(BeEmpty()) }) @@ -168,49 +237,96 @@ func TestBuildHTTPRoute(t *testing.T) { gatewayNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} + // route with valid filter validFilter := gatewayv1.HTTPRouteFilter{ Type: gatewayv1.HTTPRouteFilterRequestRedirect, RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, } - invalidFilter := gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname](invalidRedirectHostname), - }, - } - hr := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/", "/filter") addFilterToPath(hr, "/filter", validFilter) + // invalid routes without filters hrInvalidHostname := createHTTPRoute("hr", gatewayNsName.Name, "", "/") hrNotNGF := createHTTPRoute("hr", "some-gateway", "example.com", "/") hrInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath) - hrInvalidMatchesEmptyPathType := createHTTPRoute("hr", gatewayNsName.Name, "example.com", emptyPathType) hrInvalidMatchesEmptyPathValue := createHTTPRoute("hr", gatewayNsName.Name, "example.com", emptyPathValue) + hrDroppedInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/") + // route with invalid filter + invalidFilter := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname](invalidRedirectHostname), + }, + } hrInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") addFilterToPath(hrInvalidFilters, "/filter", invalidFilter) - hrDroppedInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/") - + // route with invalid matches and filters hrDroppedInvalidMatchesAndInvalidFilters := createHTTPRoute( "hr", gatewayNsName.Name, "example.com", - invalidPath, "/filter", "/") + invalidPath, + "/filter", + "/", + ) addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter) + // route with both invalid and valid filters in the same rule hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) addFilterToPath(hrDroppedInvalidFilters, "/", invalidFilter) + // route with duplicate section names hrDuplicateSectionName := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") hrDuplicateSectionName.Spec.ParentRefs = append( hrDuplicateSectionName.Spec.ParentRefs, hrDuplicateSectionName.Spec.ParentRefs[0], ) + // route with valid snippets filter extension ref + hrValidSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") + validSnippetsFilterExtRef := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + } + addFilterToPath(hrValidSnippetsFilter, "/filter", validSnippetsFilterExtRef) + + // route with invalid snippets filter extension ref + hrInvalidSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") + invalidSnippetsFilterExtRef := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Group: "wrong", + Kind: kinds.SnippetsFilter, + Name: "sf", + }, + } + addFilterToPath(hrInvalidSnippetsFilter, "/filter", invalidSnippetsFilterExtRef) + + // route with unresolvable snippets filter extension ref + hrUnresolvableSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") + unresolvableSnippetsFilterExtRef := gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterExtensionRef, + ExtensionRef: &gatewayv1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: "does-not-exist", + }, + } + addFilterToPath(hrUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef) + + // route with two invalid snippets filter extensions refs: (1) invalid group (2) unresolvable + hrInvalidAndUnresolvableSnippetsFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") + addFilterToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", invalidSnippetsFilterExtRef) + addFilterToPath(hrInvalidAndUnresolvableSnippetsFilter, "/filter", unresolvableSnippetsFilterExtRef) + validatorInvalidFieldsInRule := &validationfakes.FakeHTTPFieldsValidator{ ValidatePathInMatchStub: func(path string) error { if path == invalidPath { @@ -251,16 +367,21 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hr.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: convertHTTPRouteFilters(hr.Spec.Rules[1].Filters), + }, Matches: hr.Spec.Rules[1].Matches, - Filters: hr.Spec.Rules[1].Filters, RouteBackendRefs: []RouteBackendRef{}, }, }, @@ -292,8 +413,11 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hrInvalidMatchesEmptyPathType.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, RouteBackendRefs: []RouteBackendRef{}, Matches: hrInvalidMatchesEmptyPathType.Spec.Rules[0].Matches, }, @@ -335,8 +459,11 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, RouteBackendRefs: []RouteBackendRef{}, Matches: hrInvalidMatchesEmptyPathValue.Spec.Rules[0].Matches, }, @@ -398,8 +525,11 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hrInvalidMatches.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{}, }, @@ -433,10 +563,12 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: false, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertHTTPRouteFilters(hrInvalidFilters.Spec.Rules[0].Filters), + }, Matches: hrInvalidFilters.Spec.Rules[0].Matches, - Filters: hrInvalidFilters.Spec.Rules[0].Filters, RouteBackendRefs: []RouteBackendRef{}, }, }, @@ -468,14 +600,20 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hrDroppedInvalidMatches.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hrDroppedInvalidMatches.Spec.Rules[1].Matches, RouteBackendRefs: []RouteBackendRef{}, }, @@ -511,21 +649,31 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: false, - ValidFilters: true, + ValidMatches: false, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[0].Matches, RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: false, - Matches: hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[1].Matches, - Filters: hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[1].Filters, + ValidMatches: true, + Matches: hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[1].Matches, + Filters: RouteRuleFilters{ + Valid: false, + Filters: convertHTTPRouteFilters( + hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[1].Filters, + ), + }, RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: true, + ValidMatches: true, + Filters: RouteRuleFilters{ + Valid: true, + Filters: []Filter{}, + }, Matches: hrDroppedInvalidMatchesAndInvalidFilters.Spec.Rules[2].Matches, RouteBackendRefs: []RouteBackendRef{}, }, @@ -559,17 +707,21 @@ func TestBuildHTTPRoute(t *testing.T) { Hostnames: hr.Spec.Hostnames, Rules: []RouteRule{ { - ValidMatches: true, - ValidFilters: true, - Matches: hrDroppedInvalidFilters.Spec.Rules[0].Matches, - Filters: hrDroppedInvalidFilters.Spec.Rules[0].Filters, + ValidMatches: true, + Matches: hrDroppedInvalidFilters.Spec.Rules[0].Matches, + Filters: RouteRuleFilters{ + Filters: convertHTTPRouteFilters(hrDroppedInvalidFilters.Spec.Rules[0].Filters), + Valid: true, + }, RouteBackendRefs: []RouteBackendRef{}, }, { - ValidMatches: true, - ValidFilters: false, - Matches: hrDroppedInvalidFilters.Spec.Rules[1].Matches, - Filters: hrDroppedInvalidFilters.Spec.Rules[1].Filters, + ValidMatches: true, + Matches: hrDroppedInvalidFilters.Spec.Rules[1].Matches, + Filters: RouteRuleFilters{ + Filters: convertHTTPRouteFilters(hrDroppedInvalidFilters.Spec.Rules[1].Filters), + Valid: false, + }, RouteBackendRefs: []RouteBackendRef{}, }, }, @@ -577,6 +729,170 @@ func TestBuildHTTPRoute(t *testing.T) { }, name: "dropped invalid rule with invalid filters", }, + { + validator: validatorInvalidFieldsInRule, + hr: hrValidSnippetsFilter, + expected: &L7Route{ + RouteType: RouteTypeHTTP, + Source: hrValidSnippetsFilter, + Valid: true, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: hrValidSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Spec: L7RouteSpec{ + Hostnames: hrValidSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Matches: hrValidSnippetsFilter.Spec.Rules[0].Matches, + Filters: RouteRuleFilters{ + Filters: []Filter{ + { + RouteType: RouteTypeHTTP, + FilterType: FilterExtensionRef, + ExtensionRef: validSnippetsFilterExtRef.ExtensionRef, + ResolvedExtensionRef: &ExtensionRefFilter{ + Valid: true, + SnippetsFilter: &SnippetsFilter{Valid: true, Referenced: true}, + }, + }, + }, + Valid: true, + }, + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + name: "rule with valid snippets filter extension ref filter", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrInvalidSnippetsFilter, + expected: &L7Route{ + RouteType: RouteTypeHTTP, + Source: hrInvalidSnippetsFilter, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: hrInvalidSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + "All rules are invalid: spec.rules[0].filters[0].extensionRef: " + + "Unsupported value: \"wrong\": supported values: \"gateway.nginx.org\"", + ), + }, + Spec: L7RouteSpec{ + Hostnames: hrInvalidSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Matches: hrInvalidSnippetsFilter.Spec.Rules[0].Matches, + Filters: RouteRuleFilters{ + Filters: convertHTTPRouteFilters(hrInvalidSnippetsFilter.Spec.Rules[0].Filters), + Valid: false, + }, + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + name: "rule with invalid snippets filter extension ref filter", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrUnresolvableSnippetsFilter, + expected: &L7Route{ + RouteType: RouteTypeHTTP, + Source: hrUnresolvableSnippetsFilter, + Valid: true, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: hrUnresolvableSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteResolvedRefsInvalidFilter( + "spec.rules[0].filters[0].extensionRef: Not found: " + + "v1.LocalObjectReference{Group:\"gateway.nginx.org\", Kind:\"SnippetsFilter\", " + + "Name:\"does-not-exist\"}", + ), + }, + Spec: L7RouteSpec{ + Hostnames: hrUnresolvableSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Matches: hrUnresolvableSnippetsFilter.Spec.Rules[0].Matches, + Filters: RouteRuleFilters{ + Filters: convertHTTPRouteFilters(hrUnresolvableSnippetsFilter.Spec.Rules[0].Filters), + Valid: false, + }, + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + name: "rule with unresolvable snippets filter extension ref filter", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrInvalidAndUnresolvableSnippetsFilter, + expected: &L7Route{ + RouteType: RouteTypeHTTP, + Source: hrInvalidAndUnresolvableSnippetsFilter, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: hrInvalidAndUnresolvableSnippetsFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + "All rules are invalid: spec.rules[0].filters[0].extensionRef: " + + "Unsupported value: \"wrong\": supported values: \"gateway.nginx.org\"", + ), + staticConds.NewRouteResolvedRefsInvalidFilter( + "spec.rules[0].filters[1].extensionRef: Not found: " + + "v1.LocalObjectReference{Group:\"gateway.nginx.org\", Kind:\"SnippetsFilter\", " + + "Name:\"does-not-exist\"}", + ), + }, + Spec: L7RouteSpec{ + Hostnames: hrInvalidAndUnresolvableSnippetsFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + Matches: hrInvalidAndUnresolvableSnippetsFilter.Spec.Rules[0].Matches, + Filters: RouteRuleFilters{ + Filters: convertHTTPRouteFilters( + hrInvalidAndUnresolvableSnippetsFilter.Spec.Rules[0].Filters, + ), + Valid: false, + }, + RouteBackendRefs: []RouteBackendRef{}, + }, + }, + }, + }, + name: "rule with one invalid and one unresolvable snippets filter extension ref filter", + }, } gatewayNsNames := []types.NamespacedName{gatewayNsName} @@ -586,7 +902,11 @@ func TestBuildHTTPRoute(t *testing.T) { t.Parallel() g := NewWithT(t) - route := buildHTTPRoute(test.validator, test.hr, gatewayNsNames) + snippetsFilters := map[types.NamespacedName]*SnippetsFilter{ + {Namespace: "test", Name: "sf"}: {Valid: true}, + } + + route := buildHTTPRoute(test.validator, test.hr, gatewayNsNames, snippetsFilters) g.Expect(helpers.Diff(test.expected, route)).To(BeEmpty()) }) } @@ -857,66 +1177,6 @@ func TestValidateMatch(t *testing.T) { } } -func TestValidateFilter(t *testing.T) { - t.Parallel() - tests := []struct { - filter gatewayv1.HTTPRouteFilter - name string - expectErrCount int - }{ - { - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, - }, - expectErrCount: 0, - name: "valid redirect filter", - }, - { - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, - }, - expectErrCount: 0, - name: "valid rewrite filter", - }, - { - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, - }, - expectErrCount: 0, - name: "valid request header modifiers filter", - }, - { - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{}, - }, - expectErrCount: 0, - name: "valid response header modifiers filter", - }, - { - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestMirror, - }, - expectErrCount: 1, - name: "unsupported filter", - }, - } - - filterPath := field.NewPath("test") - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - allErrs := validateFilter(&validationfakes.FakeHTTPFieldsValidator{}, test.filter, filterPath) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - }) - } -} - func TestValidateFilterRedirect(t *testing.T) { t.Parallel() createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 4db7807d71..9a4dbe3a14 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -127,16 +127,14 @@ type L7RouteSpec struct { type RouteRule struct { // Matches define the predicate used to match requests to a given action. Matches []v1.HTTPRouteMatch - // Filters define processing steps that must be completed during the request or response lifecycle. - Filters []v1.HTTPRouteFilter // RouteBackendRefs are a wrapper for v1.BackendRef and any BackendRef filters from the HTTPRoute or GRPCRoute. RouteBackendRefs []RouteBackendRef // BackendRefs is an internal representation of a backendRef in a Route. BackendRefs []BackendRef + // Filters define processing steps that must be completed during the request or response lifecycle. + Filters RouteRuleFilters // ValidMatches indicates if the matches are valid and accepted by the Route. ValidMatches bool - // ValidFilters indicates if the filters are valid and accepted by the Route. - ValidFilters bool } // RouteBackendRef is a wrapper for v1.BackendRef and any BackendRef filters from the HTTPRoute or GRPCRoute. @@ -173,6 +171,18 @@ func CreateRouteKeyL4(obj client.Object) L4RouteKey { } } +type routeRuleErrors struct { + invalid field.ErrorList + resolve field.ErrorList +} + +func (e routeRuleErrors) append(newErrors routeRuleErrors) routeRuleErrors { + return routeRuleErrors{ + invalid: append(e.invalid, newErrors.invalid...), + resolve: append(e.resolve, newErrors.resolve...), + } +} + func buildL4RoutesForGateways( tlsRoutes map[types.NamespacedName]*v1alpha.TLSRoute, gatewayNsNames []types.NamespacedName, @@ -207,6 +217,7 @@ func buildRoutesForGateways( grpcRoutes map[types.NamespacedName]*v1.GRPCRoute, gatewayNsNames []types.NamespacedName, npCfg *NginxProxy, + snippetsFilters map[types.NamespacedName]*SnippetsFilter, ) map[RouteKey]*L7Route { if len(gatewayNsNames) == 0 { return nil @@ -217,14 +228,14 @@ func buildRoutesForGateways( http2disabled := isHTTP2Disabled(npCfg) for _, route := range httpRoutes { - r := buildHTTPRoute(validator, route, gatewayNsNames) + r := buildHTTPRoute(validator, route, gatewayNsNames, snippetsFilters) if r != nil { routes[CreateRouteKey(route)] = r } } for _, route := range grpcRoutes { - r := buildGRPCRoute(validator, route, gatewayNsNames, http2disabled) + r := buildGRPCRoute(validator, route, gatewayNsNames, http2disabled, snippetsFilters) if r != nil { routes[CreateRouteKey(route)] = r } @@ -458,7 +469,7 @@ func tryToAttachL4RouteToListeners( allowed, attached, hostnamesUnique bool ) - // Sorting the listeners from most specific hostname to least specific hostname + // Sorting the listeners from most specific hostname to the least specific hostname sort.Slice(attachableListeners, func(i, j int) bool { h1 := "" h2 := "" @@ -870,166 +881,6 @@ func validateHeaderMatch( return allErrs } -func validateFilterHeaderModifier( - validator validation.HTTPFieldsValidator, - headerModifier *v1.HTTPHeaderFilter, - filterPath *field.Path, -) field.ErrorList { - if headerModifier == nil { - return field.ErrorList{field.Required(filterPath, "cannot be nil")} - } - - return validateFilterHeaderModifierFields(validator, headerModifier, filterPath) -} - -func validateFilterHeaderModifierFields( - validator validation.HTTPFieldsValidator, - headerModifier *v1.HTTPHeaderFilter, - headerModifierPath *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - // Ensure that the header names are case-insensitive unique - allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( - headerModifier.Add, - headerModifierPath.Child(add))..., - ) - allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( - headerModifier.Set, - headerModifierPath.Child(set))..., - ) - allErrs = append(allErrs, validateRequestHeaderStringCaseInsensitiveUnique( - headerModifier.Remove, - headerModifierPath.Child(remove))..., - ) - - for _, h := range headerModifier.Add { - if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { - valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) - allErrs = append(allErrs, valErr) - } - if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { - valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - for _, h := range headerModifier.Set { - if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { - valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) - allErrs = append(allErrs, valErr) - } - if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { - valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - for _, h := range headerModifier.Remove { - if err := validator.ValidateFilterHeaderName(h); err != nil { - valErr := field.Invalid(headerModifierPath.Child(remove), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - - return allErrs -} - -func validateFilterResponseHeaderModifier( - validator validation.HTTPFieldsValidator, - responseHeaderModifier *v1.HTTPHeaderFilter, - filterPath *field.Path, -) field.ErrorList { - if errList := validateFilterHeaderModifier(validator, responseHeaderModifier, filterPath); errList != nil { - return errList - } - var allErrs field.ErrorList - - allErrs = append(allErrs, validateResponseHeaders( - responseHeaderModifier.Add, - filterPath.Child(add))..., - ) - - allErrs = append(allErrs, validateResponseHeaders( - responseHeaderModifier.Set, - filterPath.Child(set))..., - ) - - var removeHeaders []v1.HTTPHeader - for _, h := range responseHeaderModifier.Remove { - removeHeaders = append(removeHeaders, v1.HTTPHeader{Name: v1.HTTPHeaderName(h)}) - } - - allErrs = append(allErrs, validateResponseHeaders( - removeHeaders, - filterPath.Child(remove))..., - ) - - return allErrs -} - -func validateResponseHeaders( - headers []v1.HTTPHeader, - path *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - disallowedResponseHeaderSet := map[string]struct{}{ - "server": {}, - "date": {}, - "x-pad": {}, - "content-type": {}, - "content-length": {}, - "connection": {}, - } - invalidPrefix := "x-accel" - - for _, h := range headers { - valErr := field.Invalid(path, h, "header name is not allowed") - name := strings.ToLower(string(h.Name)) - if _, exists := disallowedResponseHeaderSet[name]; exists || - strings.HasPrefix(name, strings.ToLower(invalidPrefix)) { - allErrs = append(allErrs, valErr) - } - } - - return allErrs -} - -func validateRequestHeadersCaseInsensitiveUnique( - headers []v1.HTTPHeader, - path *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - seen := make(map[string]struct{}) - - for _, h := range headers { - name := strings.ToLower(string(h.Name)) - if _, exists := seen[name]; exists { - valErr := field.Invalid(path, h, "header name is not unique") - allErrs = append(allErrs, valErr) - } - seen[name] = struct{}{} - } - - return allErrs -} - -func validateRequestHeaderStringCaseInsensitiveUnique(headers []string, path *field.Path) field.ErrorList { - var allErrs field.ErrorList - - seen := make(map[string]struct{}) - - for _, h := range headers { - name := strings.ToLower(h) - if _, exists := seen[name]; exists { - valErr := field.Invalid(path, h, "header name is not unique") - allErrs = append(allErrs, valErr) - } - seen[name] = struct{}{} - } - - return allErrs -} - func routeKeyForKind(kind v1.Kind, nsname types.NamespacedName) RouteKey { key := RouteKey{NamespacedName: nsname} switch kind { diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index f96e820bb7..e0d3db46df 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -18,7 +18,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) func TestBuildSectionNameRefs(t *testing.T) { @@ -1406,332 +1405,6 @@ func TestValidateHostnames(t *testing.T) { } } -func TestValidateFilterRequestHeaderModifier(t *testing.T) { - t.Parallel() - createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { - v := &validationfakes.FakeHTTPFieldsValidator{} - return v - } - - tests := []struct { - filter gatewayv1.HTTPRouteFilter - validator *validationfakes.FakeHTTPFieldsValidator - name string - expectErrCount int - }{ - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, - Remove: []string{"Cache-Control"}, - }, - }, - expectErrCount: 0, - name: "valid request header modifier filter", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: nil, - }, - expectErrCount: 1, - name: "nil request header modifier filter", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "$var_name", Value: "gzip"}, - }, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid add", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Remove: []string{"$var-name"}, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid remove", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "yhu$"}, - }, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid header value", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Host", Value: "my_host"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "}90yh&$", Value: "gzip$"}, - {Name: "}67yh&$", Value: "compress$"}, - }, - Remove: []string{"Cache-Control$}"}, - }, - }, - expectErrCount: 7, - name: "request header modifier filter all fields invalid", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - {Name: "mYbespokeHEader", Value: "duplicate"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - {Name: "accept-encodING", Value: "gzip"}, - }, - Remove: []string{"Cache-Control", "cache-control"}, - }, - }, - expectErrCount: 3, - name: "request header modifier filter not unique names", - }, - } - - filterPath := field.NewPath("test") - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - allErrs := validateFilterHeaderModifier( - test.validator, test.filter.RequestHeaderModifier, filterPath, - ) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - }) - } -} - -func TestValidateFilterResponseHeaderModifier(t *testing.T) { - t.Parallel() - createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { - v := &validationfakes.FakeHTTPFieldsValidator{} - return v - } - - tests := []struct { - filter gatewayv1.HTTPRouteFilter - validator *validationfakes.FakeHTTPFieldsValidator - name string - expectErrCount int - }{ - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, - Remove: []string{"Cache-Control"}, - }, - }, - expectErrCount: 0, - name: "valid response header modifier filter", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: nil, - }, - expectErrCount: 1, - name: "nil response header modifier filter", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "$var_name", Value: "gzip"}, - }, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid add", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Remove: []string{"$var-name"}, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid remove", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "yhu$"}, - }, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid header value", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Host", Value: "my_host"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "}90yh&$", Value: "gzip$"}, - {Name: "}67yh&$", Value: "compress$"}, - }, - Remove: []string{"Cache-Control$}"}, - }, - }, - expectErrCount: 7, - name: "response header modifier filter all fields invalid", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - {Name: "mYbespokeHEader", Value: "duplicate"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - {Name: "accept-encodING", Value: "gzip"}, - }, - Remove: []string{"Cache-Control", "cache-control"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter not unique names", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Content-Length", Value: "163"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Content-Type", Value: "text/plain"}, - }, - Remove: []string{"X-Pad"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter with disallowed header name", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "X-Accel-Redirect", Value: "/protected/iso.img"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "X-Accel-Limit-Rate", Value: "1024"}, - }, - Remove: []string{"X-Accel-Charset"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter with disallowed header name prefix", - }, - } - - filterPath := field.NewPath("test") - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - allErrs := validateFilterResponseHeaderModifier( - test.validator, test.filter.ResponseHeaderModifier, filterPath, - ) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - }) - } -} - func TestRouteKeyForKind(t *testing.T) { t.Parallel() nsname := types.NamespacedName{Namespace: testNs, Name: "route"} diff --git a/internal/mode/static/state/graph/snippets_filter.go b/internal/mode/static/state/graph/snippets_filter.go index f3541d7341..58020345ea 100644 --- a/internal/mode/static/state/graph/snippets_filter.go +++ b/internal/mode/static/state/graph/snippets_filter.go @@ -3,9 +3,11 @@ package graph import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" + v1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) @@ -13,10 +15,41 @@ import ( type SnippetsFilter struct { // Source is the SnippetsFilter. Source *ngfAPI.SnippetsFilter + // Snippets stored as a map of nginx context to snippet value. + Snippets map[ngfAPI.NginxContext]string // Conditions define the conditions to be reported in the status of the SnippetsFilter. Conditions []conditions.Condition // Valid indicates whether the SnippetsFilter is semantically and syntactically valid. Valid bool + // Referenced indicates whether the SnippetsFilter is referenced by a Route. + Referenced bool +} + +// getSnippetsFilterResolverForNamespace returns a resolveExtRefFilter function. +// This function resolves a LocalObjectReference to a SnippetsFilter in the given namespace. +// If the SnippetsFilter exists, it is marked as referenced and returned as an ExtensionRefFilter. +func getSnippetsFilterResolverForNamespace( + snippetsFilters map[types.NamespacedName]*SnippetsFilter, + ns string, +) resolveExtRefFilter { + return func(ref v1.LocalObjectReference) *ExtensionRefFilter { + if len(snippetsFilters) == 0 { + return nil + } + + if ref.Group != ngfAPI.GroupName || ref.Kind != kinds.SnippetsFilter { + return nil + } + + sf := snippetsFilters[types.NamespacedName{Namespace: ns, Name: string(ref.Name)}] + if sf == nil { + return nil + } + + sf.Referenced = true + + return &ExtensionRefFilter{SnippetsFilter: sf, Valid: sf.Valid} + } } func processSnippetsFilters( @@ -29,22 +62,37 @@ func processSnippetsFilters( processed := make(map[types.NamespacedName]*SnippetsFilter) for nsname, sf := range snippetsFilters { - processedSf := &SnippetsFilter{ - Source: sf, - Valid: true, - } - if cond := validateSnippetsFilter(sf); cond != nil { - processedSf.Valid = false - processedSf.Conditions = []conditions.Condition{*cond} + processed[nsname] = &SnippetsFilter{ + Source: sf, + Conditions: []conditions.Condition{*cond}, + Valid: false, + } + + continue } - processed[nsname] = processedSf + processed[nsname] = &SnippetsFilter{ + Source: sf, + Valid: true, + Snippets: createSnippetsMap(sf.Spec.Snippets), + } } return processed } +func createSnippetsMap(snippets []ngfAPI.Snippet) map[ngfAPI.NginxContext]string { + snippetsMap := make(map[ngfAPI.NginxContext]string) + + // snippets are already validated, so we can assume there's max one snippet per context. + for _, snippet := range snippets { + snippetsMap[snippet.Context] = snippet.Value + } + + return snippetsMap +} + func validateSnippetsFilter(filter *ngfAPI.SnippetsFilter) *conditions.Condition { var allErrs field.ErrorList snippetsPath := field.NewPath("spec.snippets") diff --git a/internal/mode/static/state/graph/snippets_filter_test.go b/internal/mode/static/state/graph/snippets_filter_test.go index 9c3116488a..c5c8545552 100644 --- a/internal/mode/static/state/graph/snippets_filter_test.go +++ b/internal/mode/static/state/graph/snippets_filter_test.go @@ -4,14 +4,19 @@ import ( "testing" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + v1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) func TestProcessSnippetsFilters(t *testing.T) { + t.Parallel() + filter1NsName := types.NamespacedName{Namespace: "test", Name: "filter-1"} filter2NsName := types.NamespacedName{Namespace: "other", Name: "filter-2"} invalidFilterNsName := types.NamespacedName{Namespace: "default", Name: "invalid"} @@ -79,18 +84,29 @@ func TestProcessSnippetsFilters(t *testing.T) { Source: filter1, Conditions: nil, Valid: true, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextMain: "main snippet", + ngfAPI.NginxContextHTTP: "http snippet", + }, }, filter2NsName: { Source: filter2, Conditions: nil, Valid: true, + Referenced: false, + Snippets: map[ngfAPI.NginxContext]string{ + ngfAPI.NginxContextHTTPServerLocation: "location snippet", + }, }, invalidFilterNsName: { Source: invalidFilter, - Conditions: []conditions.Condition{staticConds.NewSnippetsFilterInvalid( - "spec.snippets[1].context: Unsupported value: \"invalid context\": " + - "supported values: \"main\", \"http\", \"http.server\", \"http.server.location\"", - )}, + Conditions: []conditions.Condition{ + staticConds.NewSnippetsFilterInvalid( + "spec.snippets[1].context: Unsupported value: \"invalid context\": " + + "supported values: \"main\", \"http\", \"http.server\", \"http.server.location\"", + ), + }, Valid: false, }, }, @@ -99,6 +115,7 @@ func TestProcessSnippetsFilters(t *testing.T) { for _, test := range tests { t.Run(test.msg, func(t *testing.T) { + t.Parallel() g := NewWithT(t) processedSnippetsFilters := processSnippetsFilters(test.snippetsFilters) @@ -108,6 +125,8 @@ func TestProcessSnippetsFilters(t *testing.T) { } func TestValidateSnippetsFilter(t *testing.T) { + t.Parallel() + tests := []struct { msg string filter *ngfAPI.SnippetsFilter @@ -268,6 +287,7 @@ func TestValidateSnippetsFilter(t *testing.T) { for _, test := range tests { t.Run(test.msg, func(t *testing.T) { + t.Parallel() g := NewWithT(t) cond := validateSnippetsFilter(test.filter) @@ -280,3 +300,148 @@ func TestValidateSnippetsFilter(t *testing.T) { }) } } + +func TestGetSnippetsFilterResolverForNamespace(t *testing.T) { + t.Parallel() + + defaultSf1NsName := types.NamespacedName{Name: "sf1", Namespace: "default"} + fooSf1NsName := types.NamespacedName{Name: "sf1", Namespace: "foo"} + fooSf2InvalidNsName := types.NamespacedName{Name: "sf2-invalid", Namespace: "foo"} + + createSnippetsFilter := func(nsname types.NamespacedName, valid bool) *SnippetsFilter { + return &SnippetsFilter{ + Source: &ngfAPI.SnippetsFilter{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsname.Name, + Namespace: nsname.Namespace, + }, + }, + Valid: valid, + } + } + + createSnippetsFilterMap := func() map[types.NamespacedName]*SnippetsFilter { + return map[types.NamespacedName]*SnippetsFilter{ + defaultSf1NsName: createSnippetsFilter(defaultSf1NsName, true), + fooSf1NsName: createSnippetsFilter(fooSf1NsName, true), + fooSf2InvalidNsName: createSnippetsFilter(fooSf2InvalidNsName, false), + } + } + + tests := []struct { + name string + extRef v1.LocalObjectReference + snippetsFilterMap map[types.NamespacedName]*SnippetsFilter + resolveInNamespace string + expResolve bool + expValid bool + }{ + { + name: "empty ref", + extRef: v1.LocalObjectReference{}, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "default", + expResolve: false, + }, + { + name: "no snippets filters", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName(fooSf1NsName.Name), + }, + snippetsFilterMap: nil, + resolveInNamespace: "default", + expResolve: false, + }, + { + name: "invalid group", + extRef: v1.LocalObjectReference{ + Group: "invalid", + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName(defaultSf1NsName.Name), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "default", + expResolve: false, + }, + { + name: "invalid kind", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.Gateway, + Name: v1.ObjectName(defaultSf1NsName.Name), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "default", + expResolve: false, + }, + { + name: "snippets filter does not exist", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName("dne"), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "default", + expResolve: false, + }, + { + name: "valid snippets filter exists - namespace default", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName(defaultSf1NsName.Name), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "default", + expResolve: true, + expValid: true, + }, + { + name: "valid snippets filter exists - namespace foo", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName(fooSf1NsName.Name), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "foo", + expResolve: true, + expValid: true, + }, + { + name: "invalid snippets filter exists - namespace foo", + extRef: v1.LocalObjectReference{ + Group: ngfAPI.GroupName, + Kind: kinds.SnippetsFilter, + Name: v1.ObjectName(fooSf2InvalidNsName.Name), + }, + snippetsFilterMap: createSnippetsFilterMap(), + resolveInNamespace: "foo", + expResolve: true, + expValid: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + resolve := getSnippetsFilterResolverForNamespace(test.snippetsFilterMap, test.resolveInNamespace) + resolvedSf := resolve(test.extRef) + if test.expResolve { + g.Expect(resolvedSf).ToNot(BeNil()) + g.Expect(resolvedSf.SnippetsFilter).ToNot(BeNil()) + g.Expect(resolvedSf.SnippetsFilter.Referenced).To(BeTrue()) + g.Expect(resolvedSf.SnippetsFilter.Source.Name).To(BeEquivalentTo(test.extRef.Name)) + g.Expect(resolvedSf.SnippetsFilter.Source.Namespace).To(Equal(test.resolveInNamespace)) + g.Expect(resolvedSf.Valid).To(BeEquivalentTo(test.expValid)) + } else { + g.Expect(resolvedSf).To(BeNil()) + } + }) + } +}