From 68811f94cd6f4c8b38e6cc0a514803abb34ee6d5 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 22 Apr 2024 12:44:15 -0600 Subject: [PATCH 01/15] Support NginxProxy CRD and global tracing settings Problem: As a user of NGF I want to set the collection point for my traces for my installation of NGF So that I can ensure all my traces are sent to the same collection platform. Solution: Implement the NginxProxy CRD which contains the fields required to configure the collection point for tracing. This resource is attached to the GatewayClass. If the resource is not found, a condition will be set on the GatewayClass to indicate this. The GatewayClass will continue to be Accepted even if the parametersRef is invalid. This configuration sets the `http` context-level otel directives. The otel module is loaded conditionally based on the existence of this configuration. Note: tracing is not enabled by this configuration, this only sets high level options. https://github.com/nginxinc/nginx-gateway-fabric/issues/1828 is required to actually enable tracing on a per-route basis. --- build/Dockerfile.nginx | 9 +- build/Dockerfile.nginxplus | 2 +- charts/nginx-gateway-fabric/README.md | 2 +- .../templates/deployment.yaml | 6 + .../nginx-gateway-fabric/templates/rbac.yaml | 3 +- .../provisioner/static-deployment.yaml | 6 + .../manifests/nginx-gateway-experimental.yaml | 7 + deploy/manifests/nginx-gateway.yaml | 7 + .../nginx-plus-gateway-experimental.yaml | 7 + deploy/manifests/nginx-plus-gateway.yaml | 7 + docs/proposals/gateway-settings.md | 13 +- internal/mode/static/manager.go | 7 + internal/mode/static/manager_test.go | 4 + .../mode/static/nginx/conf/nginx-plus.conf | 1 + internal/mode/static/nginx/conf/nginx.conf | 1 + .../mode/static/nginx/config/generator.go | 20 +++ .../static/nginx/config/generator_test.go | 23 +++- .../mode/static/nginx/config/maps_template.go | 2 +- .../static/nginx/config/servers_template.go | 2 +- .../nginx/config/split_clients_template.go | 2 +- .../mode/static/nginx/config/telemetry.go | 22 +++ .../static/nginx/config/telemetry_template.go | 22 +++ .../static/nginx/config/telemetry_test.go | 60 ++++++++ .../static/nginx/config/upstreams_template.go | 2 +- .../static/nginx/config/version_template.go | 2 +- .../mode/static/state/change_processor.go | 7 + .../static/state/change_processor_test.go | 56 ++++++++ .../static/state/conditions/conditions.go | 47 ++++++- .../static/state/dataplane/configuration.go | 33 +++++ .../state/dataplane/configuration_test.go | 121 ++++++++++++++++ internal/mode/static/state/dataplane/types.go | 19 +++ .../mode/static/state/graph/gatewayclass.go | 29 +++- .../static/state/graph/gatewayclass_test.go | 65 +++++++-- internal/mode/static/state/graph/graph.go | 8 +- .../mode/static/state/graph/graph_test.go | 32 ++++- .../mode/static/state/graph/nginxproxy.go | 23 ++++ .../static/state/graph/nginxproxy_test.go | 129 ++++++++++++++++++ .../overview/gateway-api-compatibility.md | 2 +- 38 files changed, 770 insertions(+), 40 deletions(-) create mode 100644 internal/mode/static/nginx/config/telemetry.go create mode 100644 internal/mode/static/nginx/config/telemetry_template.go create mode 100644 internal/mode/static/nginx/config/telemetry_test.go create mode 100644 internal/mode/static/state/graph/nginxproxy.go create mode 100644 internal/mode/static/state/graph/nginxproxy_test.go diff --git a/build/Dockerfile.nginx b/build/Dockerfile.nginx index 2a6cae2287..6152c8d567 100644 --- a/build/Dockerfile.nginx +++ b/build/Dockerfile.nginx @@ -1,11 +1,18 @@ # syntax=docker/dockerfile:1.6 +FROM scratch as nginx-files + +# the following links can be replaced with local files if needed, i.e. ADD --chown=101:1001 +ADD --link --chown=101:1001 https://cs.nginx.com/static/keys/nginx_signing.rsa.pub nginx_signing.rsa.pub + FROM nginx:1.25.5-alpine ARG NJS_DIR ARG NGINX_CONF_DIR ARG BUILD_AGENT -RUN apk add --no-cache libcap \ +RUN --mount=type=bind,from=nginx-files,src=nginx_signing.rsa.pub,target=/etc/apk/keys/nginx_signing.rsa.pub \ + printf "%s\n" "http://nginx.org/packages/mainline/alpine/v$(egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \ + && apk add --no-cache libcap nginx-module-otel \ && mkdir -p /var/lib/nginx /usr/lib/nginx/modules \ && setcap 'cap_net_bind_service=+ep' /usr/sbin/nginx \ && setcap -v 'cap_net_bind_service=+ep' /usr/sbin/nginx \ diff --git a/build/Dockerfile.nginxplus b/build/Dockerfile.nginxplus index b27b7f70f1..dd4ba09fca 100644 --- a/build/Dockerfile.nginxplus +++ b/build/Dockerfile.nginxplus @@ -18,7 +18,7 @@ RUN --mount=type=secret,id=nginx-repo.crt,dst=/etc/apk/cert.pem,mode=0644 \ addgroup -g 1001 -S nginx \ && adduser -S -D -H -u 101 -h /var/cache/nginx -s /sbin/nologin -G nginx -g nginx nginx \ && printf "%s\n" "https://pkgs.nginx.com/plus/${NGINX_PLUS_VERSION}/alpine/v$(grep -E -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \ - && apk add --no-cache nginx-plus nginx-plus-module-njs libcap \ + && apk add --no-cache nginx-plus nginx-plus-module-njs nginx-plus-module-otel libcap \ && mkdir -p /var/lib/nginx /usr/lib/nginx/modules \ && setcap 'cap_net_bind_service=+ep' /usr/sbin/nginx \ && setcap -v 'cap_net_bind_service=+ep' /usr/sbin/nginx \ diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index bc6871d5ed..2d3b7bba9f 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -224,7 +224,7 @@ To uninstall/delete the release `ngf`: ```shell helm uninstall ngf -n nginx-gateway kubectl delete ns nginx-gateway -kubectl delete crd nginxgateways.gateway.nginx.org +for crd in `kubectl get crds -oname | grep gateway.nginx.org | awk -F / '{ print $2 }'`; do kubectl delete crd $crd; done ``` These commands remove all the Kubernetes components associated with the release and deletes the release. diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index dd272d91e9..10f629b459 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -118,6 +118,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -149,6 +151,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -181,6 +185,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/charts/nginx-gateway-fabric/templates/rbac.yaml b/charts/nginx-gateway-fabric/templates/rbac.yaml index 851ddb42b7..eb1c472416 100644 --- a/charts/nginx-gateway-fabric/templates/rbac.yaml +++ b/charts/nginx-gateway-fabric/templates/rbac.yaml @@ -10,7 +10,7 @@ metadata: {{- if or .Values.serviceAccount.imagePullSecret .Values.serviceAccount.imagePullSecrets }} imagePullSecrets: {{- if .Values.serviceAccount.imagePullSecret }} - - name: {{ .Values.serviceAccount.imagePullSecret}} + - name: {{ .Values.serviceAccount.imagePullSecret }} {{- end }} {{- if .Values.serviceAccount.imagePullSecrets }} {{- range .Values.serviceAccount.imagePullSecrets }} @@ -111,6 +111,7 @@ rules: - gateway.nginx.org resources: - nginxgateways + - nginxproxies verbs: - get - list diff --git a/conformance/provisioner/static-deployment.yaml b/conformance/provisioner/static-deployment.yaml index 58396a2a0a..8b2713585e 100644 --- a/conformance/provisioner/static-deployment.yaml +++ b/conformance/provisioner/static-deployment.yaml @@ -70,6 +70,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -94,6 +96,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -111,6 +115,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/deploy/manifests/nginx-gateway-experimental.yaml b/deploy/manifests/nginx-gateway-experimental.yaml index 7cf2912885..1a8b2e5780 100644 --- a/deploy/manifests/nginx-gateway-experimental.yaml +++ b/deploy/manifests/nginx-gateway-experimental.yaml @@ -93,6 +93,7 @@ rules: - gateway.nginx.org resources: - nginxgateways + - nginxproxies verbs: - get - list @@ -213,6 +214,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -237,6 +240,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -254,6 +259,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 933e66d4ec..928dedd147 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -90,6 +90,7 @@ rules: - gateway.nginx.org resources: - nginxgateways + - nginxproxies verbs: - get - list @@ -209,6 +210,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -233,6 +236,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -250,6 +255,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/deploy/manifests/nginx-plus-gateway-experimental.yaml b/deploy/manifests/nginx-plus-gateway-experimental.yaml index 88b5249c34..08b66daef4 100644 --- a/deploy/manifests/nginx-plus-gateway-experimental.yaml +++ b/deploy/manifests/nginx-plus-gateway-experimental.yaml @@ -99,6 +99,7 @@ rules: - gateway.nginx.org resources: - nginxgateways + - nginxproxies verbs: - get - list @@ -220,6 +221,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -244,6 +247,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -261,6 +266,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/deploy/manifests/nginx-plus-gateway.yaml b/deploy/manifests/nginx-plus-gateway.yaml index e0de54f541..4d0f72d4b2 100644 --- a/deploy/manifests/nginx-plus-gateway.yaml +++ b/deploy/manifests/nginx-plus-gateway.yaml @@ -96,6 +96,7 @@ rules: - gateway.nginx.org resources: - nginxgateways + - nginxproxies verbs: - get - list @@ -216,6 +217,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -240,6 +243,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d + - name: nginx-includes + mountPath: /etc/nginx/includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -257,6 +262,8 @@ spec: volumes: - name: nginx-conf emptyDir: {} + - name: nginx-includes + emptyDir: {} - name: nginx-secrets emptyDir: {} - name: nginx-run diff --git a/docs/proposals/gateway-settings.md b/docs/proposals/gateway-settings.md index e910f47fb7..cd0b0e8103 100644 --- a/docs/proposals/gateway-settings.md +++ b/docs/proposals/gateway-settings.md @@ -1,7 +1,7 @@ # Enhancement Proposal-1775: Gateway Settings - Issue: https://github.com/nginxinc/nginx-gateway-fabric/issues/1775 -- Status: Implementable +- Status: Completed ## Summary @@ -93,7 +93,7 @@ type Telemetry struct { // SpanAttributes are custom key/value attributes that are added to each span. // // +optional - SpanAttributes map[string]string `json:"spanAttributes,omitempty"` + SpanAttributes []SpanAttribute `json:"spanAttributes,omitempty"` } // TelemetryExporter specifies OpenTelemetry export parameters. @@ -122,6 +122,15 @@ type TelemetryExporter struct { // The format is a subset of the syntax parsed by Golang time.ParseDuration. // Examples: 1h, 12m, 30s, 150ms. type Duration string + +// SpanAttribute is a key value pair to be added to a tracing span. +type SpanAttribute struct { + // Key is the key for a span attribute. + Key string `json:"key"` + + // Value is the value for a span attribute. + Value string `json:"value"` +} ``` ### Status diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index d810584a69..f1a9ff2451 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -414,6 +414,12 @@ func registerControllers( ), }, }, + { + objectType: &ngfAPI.NginxProxy{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, + }, } if cfg.ExperimentalFeatures { @@ -592,6 +598,7 @@ func prepareFirstEventBatchPreparerArgs( &discoveryV1.EndpointSliceList{}, &gatewayv1.HTTPRouteList{}, &gatewayv1beta1.ReferenceGrantList{}, + &ngfAPI.NginxProxyList{}, partialObjectMetadataList, } diff --git a/internal/mode/static/manager_test.go b/internal/mode/static/manager_test.go index 532807d374..e28fa3c7bb 100644 --- a/internal/mode/static/manager_test.go +++ b/internal/mode/static/manager_test.go @@ -16,6 +16,7 @@ import ( gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) @@ -52,6 +53,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1.HTTPRouteList{}, &gatewayv1.GatewayList{}, &gatewayv1beta1.ReferenceGrantList{}, + &ngfAPI.NginxProxyList{}, partialObjectMetadataList, }, }, @@ -72,6 +74,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &discoveryV1.EndpointSliceList{}, &gatewayv1.HTTPRouteList{}, &gatewayv1beta1.ReferenceGrantList{}, + &ngfAPI.NginxProxyList{}, partialObjectMetadataList, }, }, @@ -93,6 +96,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &discoveryV1.EndpointSliceList{}, &gatewayv1.HTTPRouteList{}, &gatewayv1beta1.ReferenceGrantList{}, + &ngfAPI.NginxProxyList{}, partialObjectMetadataList, &gatewayv1alpha2.BackendTLSPolicyList{}, }, diff --git a/internal/mode/static/nginx/conf/nginx-plus.conf b/internal/mode/static/nginx/conf/nginx-plus.conf index f3e1da0806..e41ecbba07 100644 --- a/internal/mode/static/nginx/conf/nginx-plus.conf +++ b/internal/mode/static/nginx/conf/nginx-plus.conf @@ -1,4 +1,5 @@ load_module /usr/lib/nginx/modules/ngx_http_js_module.so; +include /etc/nginx/includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/conf/nginx.conf b/internal/mode/static/nginx/conf/nginx.conf index 1abe5c879a..5df383c8a2 100644 --- a/internal/mode/static/nginx/conf/nginx.conf +++ b/internal/mode/static/nginx/conf/nginx.conf @@ -1,4 +1,5 @@ load_module /usr/lib/nginx/modules/ngx_http_js_module.so; +include /etc/nginx/includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 6127ffa094..b755bca5f6 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -15,6 +15,7 @@ const ( // httpFolder is the folder where NGINX HTTP configuration files are stored. httpFolder = configFolder + "/conf.d" + // secretsFolder is the folder where secrets (like TLS certs/keys) are stored. secretsFolder = configFolder + "/secrets" @@ -26,6 +27,9 @@ 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 = configFolder + "/includes/load_modules.conf" ) // ConfigFolders is a list of folders where NGINX configuration files are stored. @@ -82,6 +86,8 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { files = append(files, generateCertBundle(id, bundle)) } + files = append(files, generateLoadModulesConf(conf)) + return files } @@ -142,6 +148,7 @@ func (g GeneratorImpl) getExecuteFuncs() []executeFunc { g.executeUpstreams, executeSplitClients, executeMaps, + executeTelemetry, } } @@ -155,3 +162,16 @@ func generateConfigVersion(configVersion int) file.File { Type: file.TypeRegular, } } + +func generateLoadModulesConf(conf dataplane.Configuration) file.File { + var c []byte + if conf.Telemetry.Endpoint != "" { + c = []byte("load_module modules/ngx_otel_module.so;") + } + + return file.File{ + Content: c, + Path: loadModulesFile, + Type: file.TypeRegular, + } +} diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index a4e334ea74..ef834d25e5 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -63,6 +63,13 @@ func TestGenerate(t *testing.T) { CertBundles: map[dataplane.CertBundleID]dataplane.CertBundle{ "test-certbundle": []byte("test-cert"), }, + Telemetry: dataplane.Telemetry{ + Endpoint: "1.2.3.4:123", + ServiceName: "ngf:gw-ns:gw-name:my-name", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + }, } g := NewWithT(t) @@ -71,7 +78,7 @@ func TestGenerate(t *testing.T) { files := generator.Generate(conf) - g.Expect(files).To(HaveLen(5)) + g.Expect(files).To(HaveLen(6)) arrange := func(i, j int) bool { return files[i].Path < files[j].Path } @@ -92,16 +99,26 @@ func TestGenerate(t *testing.T) { g.Expect(httpCfg).To(ContainSubstring("upstream")) g.Expect(httpCfg).To(ContainSubstring("split_clients")) + g.Expect(httpCfg).To(ContainSubstring("endpoint 1.2.3.4:123;")) + g.Expect(httpCfg).To(ContainSubstring("interval 5s;")) + g.Expect(httpCfg).To(ContainSubstring("batch_size 512;")) + 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(files[2].Path).To(Equal("/etc/nginx/conf.d/matches.json")) + g.Expect(files[2].Type).To(Equal(file.TypeRegular)) expString := "{}" g.Expect(string(files[2].Content)).To(Equal(expString)) - g.Expect(files[3].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt")) + 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[3].Content) g.Expect(certBundle).To(Equal("test-cert")) - g.Expect(files[4]).To(Equal(file.File{ + g.Expect(files[5]).To(Equal(file.File{ Type: file.TypeSecret, Path: "/etc/nginx/secrets/test-keypair.pem", Content: []byte("test-cert\ntest-key"), diff --git a/internal/mode/static/nginx/config/maps_template.go b/internal/mode/static/nginx/config/maps_template.go index 9233b0e403..4b4407a1a4 100644 --- a/internal/mode/static/nginx/config/maps_template.go +++ b/internal/mode/static/nginx/config/maps_template.go @@ -1,6 +1,6 @@ package config -var mapsTemplateText = ` +const mapsTemplateText = ` {{ range $m := . }} map {{ $m.Source }} {{ $m.Variable }} { {{ range $p := $m.Parameters }} diff --git a/internal/mode/static/nginx/config/servers_template.go b/internal/mode/static/nginx/config/servers_template.go index 8d272e05a7..c05fa7cea7 100644 --- a/internal/mode/static/nginx/config/servers_template.go +++ b/internal/mode/static/nginx/config/servers_template.go @@ -1,6 +1,6 @@ package config -var serversTemplateText = ` +const serversTemplateText = ` js_preload_object matches from /etc/nginx/conf.d/matches.json; {{- range $s := . -}} {{ if $s.IsDefaultSSL -}} diff --git a/internal/mode/static/nginx/config/split_clients_template.go b/internal/mode/static/nginx/config/split_clients_template.go index da51dc5cf1..6d2a0e208f 100644 --- a/internal/mode/static/nginx/config/split_clients_template.go +++ b/internal/mode/static/nginx/config/split_clients_template.go @@ -1,6 +1,6 @@ package config -var splitClientsTemplateText = ` +const splitClientsTemplateText = ` {{ range $sc := . }} split_clients $request_id ${{ $sc.VariableName }} { {{- range $d := $sc.Distributions }} diff --git a/internal/mode/static/nginx/config/telemetry.go b/internal/mode/static/nginx/config/telemetry.go new file mode 100644 index 0000000000..fef1e4ec27 --- /dev/null +++ b/internal/mode/static/nginx/config/telemetry.go @@ -0,0 +1,22 @@ +package config + +import ( + gotemplate "text/template" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +var otelTemplate = gotemplate.Must(gotemplate.New("otel").Parse(otelTemplateText)) + +func executeTelemetry(conf dataplane.Configuration) []executeResult { + if conf.Telemetry.Endpoint != "" { + result := executeResult{ + dest: httpConfigFile, + data: execute(otelTemplate, conf.Telemetry), + } + + return []executeResult{result} + } + + return nil +} diff --git a/internal/mode/static/nginx/config/telemetry_template.go b/internal/mode/static/nginx/config/telemetry_template.go new file mode 100644 index 0000000000..7d41b5352d --- /dev/null +++ b/internal/mode/static/nginx/config/telemetry_template.go @@ -0,0 +1,22 @@ +package config + +const otelTemplateText = ` +otel_exporter { + endpoint {{ .Endpoint }}; + {{- if .Interval }} + interval {{ .Interval }}; + {{- end }} + {{- if .BatchSize }} + batch_size {{ .BatchSize }}; + {{- end }} + {{- if .BatchCount }} + batch_count {{ .BatchCount }}; + {{- end }} +} + +otel_service_name {{ .ServiceName }}; + +{{- range $attr := .SpanAttributes }} +otel_span_attr {{ $attr.Key }} {{ $attr.Value }}; +{{- end }} +` diff --git a/internal/mode/static/nginx/config/telemetry_test.go b/internal/mode/static/nginx/config/telemetry_test.go new file mode 100644 index 0000000000..b260a0f0c9 --- /dev/null +++ b/internal/mode/static/nginx/config/telemetry_test.go @@ -0,0 +1,60 @@ +package config + +import ( + "strings" + "testing" + + . "github.com/onsi/gomega" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +func TestExecuteTelemetry(t *testing.T) { + conf := dataplane.Configuration{ + Telemetry: dataplane.Telemetry{ + Endpoint: "1.2.3.4:123", + ServiceName: "ngf:gw-ns:gw-name:my-name", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + SpanAttributes: []ngfAPI.SpanAttribute{ + { + Key: "key1", + Value: "val1", + }, + { + Key: "key2", + Value: "val2", + }, + }, + }, + } + + g := NewWithT(t) + expSubStrings := map[string]int{ + "endpoint 1.2.3.4:123;": 1, + "otel_service_name ngf:gw-ns:gw-name:my-name;": 1, + "interval 5s;": 1, + "batch_size 512;": 1, + "batch_count 4;": 1, + "otel_span_attr": 2, + } + + for expSubStr, expCount := range expSubStrings { + res := executeTelemetry(conf) + g.Expect(res).To(HaveLen(1)) + g.Expect(expCount).To(Equal(strings.Count(string(res[0].data), expSubStr))) + } +} + +func TestExecuteTelemetryNil(t *testing.T) { + conf := dataplane.Configuration{ + Telemetry: dataplane.Telemetry{}, + } + + g := NewWithT(t) + + res := executeTelemetry(conf) + g.Expect(res).To(BeEmpty()) +} diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index c2d9a95490..e57b98cb37 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -4,7 +4,7 @@ package config // 512k will support up to 648 upstream servers for OSS. // NGINX Plus needs 1m to support roughly the same amount of servers (556 upstream servers). // https://github.com/nginxinc/nginx-gateway-fabric/issues/483 -var upstreamsTemplateText = ` +const upstreamsTemplateText = ` {{ range $u := . }} upstream {{ $u.Name }} { random two least_conn; diff --git a/internal/mode/static/nginx/config/version_template.go b/internal/mode/static/nginx/config/version_template.go index 3f0d3ea544..ccf46e02cc 100644 --- a/internal/mode/static/nginx/config/version_template.go +++ b/internal/mode/static/nginx/config/version_template.go @@ -1,6 +1,6 @@ package config -var versionTemplateText = ` +const versionTemplateText = ` server { listen unix:/var/run/nginx/nginx-config-version.sock; access_log off; diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index db035746c4..533c2c5876 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -19,6 +19,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1beta1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -105,6 +106,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { CRDMetadata: make(map[types.NamespacedName]*metav1.PartialObjectMetadata), BackendTLSPolicies: make(map[types.NamespacedName]*v1alpha2.BackendTLSPolicy), ConfigMaps: make(map[types.NamespacedName]*apiv1.ConfigMap), + NginxProxies: make(map[types.NamespacedName]*ngfAPI.NginxProxy), } extractGVK := func(obj client.Object) schema.GroupVersionKind { @@ -182,6 +184,11 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { store: newObjectStoreMapAdapter(clusterStore.CRDMetadata), predicate: annotationChangedPredicate{annotation: gatewayclass.BundleVersionAnnotation}, }, + { + gvk: extractGVK(&ngfAPI.NginxProxy{}), + store: newObjectStoreMapAdapter(clusterStore.NginxProxies), + predicate: nil, + }, }, ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index fab678497e..09e6c2f434 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -17,6 +17,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1beta1" + 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/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" @@ -187,6 +188,7 @@ func createScheme() *runtime.Scheme { utilruntime.Must(apiv1.AddToScheme(scheme)) utilruntime.Must(discoveryV1.AddToScheme(scheme)) utilruntime.Must(apiext.AddToScheme(scheme)) + utilruntime.Must(ngfAPI.AddToScheme(scheme)) return scheme } @@ -1523,6 +1525,60 @@ var _ = Describe("ChangeProcessor", func() { }) }) }) + + Describe("NginxProxy resource changes", Ordered, func() { + paramGC := gc.DeepCopy() + paramGC.Spec.ParametersRef = &v1beta1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1beta1.Kind("NginxProxy"), + Name: "np", + } + + np := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np", + }, + } + + npUpdated := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np", + }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-svc:123", + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + }, + }, + }, + } + It("handles upserts for an NginxProxy", func() { + processor.CaptureUpsertChange(np) + processor.CaptureUpsertChange(paramGC) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.NginxProxy).To(Equal(np)) + }) + It("captures changes for an NginxProxy", func() { + processor.CaptureUpsertChange(npUpdated) + processor.CaptureUpsertChange(paramGC) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.NginxProxy).To(Equal(npUpdated)) + }) + It("handles deletes for an NginxProxy", func() { + processor.CaptureDeleteChange(np, client.ObjectKeyFromObject(np)) + + changed, graph := processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.NginxProxy).To(BeNil()) + }) + }) }) Describe("Ensuring non-changing changes don't override previously changing changes", func() { diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 894fb3596c..9675112b40 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -31,11 +31,11 @@ const ( // RouteReasonBackendRefUnsupportedValue is used with the "ResolvedRefs" condition when one of the // Route rules has a backendRef with an unsupported value. - RouteReasonBackendRefUnsupportedValue = "UnsupportedValue" + RouteReasonBackendRefUnsupportedValue v1.RouteConditionReason = "UnsupportedValue" // RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route // references is invalid. - RouteReasonInvalidGateway = "InvalidGateway" + RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway" // RouteReasonInvalidListener is used with the "Accepted" condition when the Route references an invalid listener. RouteReasonInvalidListener v1.RouteConditionReason = "InvalidListener" @@ -66,6 +66,17 @@ const ( RouteMessageFailedNginxReload = GatewayMessageFailedNginxReload + ". NGINX may still be configured " + "for this HTTPRoute. However, future updates to this resource will not be configured until the Gateway " + "is programmed again" + + // GatewayClassResolvedRefs condition indicates whether the controller was able to resolve the + // parametersRef on the GatewayClass. + GatewayClassResolvedRefs v1.GatewayClassConditionType = "ResolvedRefs" + + // GatewayClassReasonResolvedRefs is used with the "GatewayClassResolvedRefs" condition when the condition is true. + GatewayClassReasonResolvedRefs v1.GatewayClassConditionReason = "ResolvedRefs" + + // GatewayClassReasonParamsRefNotFound is used with the "GatewayClassResolvedRefs" condition when the + // parametersRef resource does not exist. + GatewayClassReasonParamsRefNotFound v1.GatewayClassConditionReason = "ParametersRefNotFound" ) // NewTODO returns a Condition that can be used as a placeholder for a condition that is not yet implemented. @@ -203,7 +214,7 @@ func NewRouteBackendRefUnsupportedValue(msg string) conditions.Condition { return conditions.Condition{ Type: string(v1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, - Reason: RouteReasonBackendRefUnsupportedValue, + Reason: string(RouteReasonBackendRefUnsupportedValue), Message: msg, } } @@ -214,7 +225,7 @@ func NewRouteInvalidGateway() conditions.Condition { return conditions.Condition{ Type: string(v1.RouteConditionAccepted), Status: metav1.ConditionFalse, - Reason: RouteReasonInvalidGateway, + Reason: string(RouteReasonInvalidGateway), Message: "Gateway is invalid", } } @@ -402,13 +413,37 @@ func NewListenerRefNotPermitted(msg string) []conditions.Condition { } } +// NewGatewayClassResolvedRefs returns a Condition that indicates that the parametersRef +// on the GatewayClass is resolved. +func NewGatewayClassResolvedRefs() conditions.Condition { + return conditions.Condition{ + Type: string(GatewayClassResolvedRefs), + Status: metav1.ConditionTrue, + Reason: string(GatewayClassReasonResolvedRefs), + Message: "parametersRef resource is resolved", + } +} + +// NewGatewayClassRefNotFound returns a Condition that indicates that the parametersRef +// on the GatewayClass could not be resolved. +func NewGatewayClassRefNotFound() conditions.Condition { + return conditions.Condition{ + Type: string(GatewayClassResolvedRefs), + Status: metav1.ConditionFalse, + Reason: string(GatewayClassReasonParamsRefNotFound), + Message: "parametersRef resource could not be found", + } +} + // NewGatewayClassInvalidParameters returns a Condition that indicates that the GatewayClass has invalid parameters. +// We are allowing Accepted to still be true to prevent nullifying the entire config tree if a parametersRef +// is updated to something invalid. func NewGatewayClassInvalidParameters(msg string) conditions.Condition { return conditions.Condition{ Type: string(v1.GatewayClassConditionStatusAccepted), - Status: metav1.ConditionFalse, + Status: metav1.ConditionTrue, Reason: string(v1.GatewayClassReasonInvalidParameters), - Message: msg, + Message: fmt.Sprintf("GatewayClass is accepted, but parametersRef is ignored due to an error: %s", msg), } } diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index 25ed7e217a..123a7eb83b 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -40,6 +40,7 @@ func BuildConfiguration( backendGroups := buildBackendGroups(append(httpServers, sslServers...)) keyPairs := buildSSLKeyPairs(g.ReferencedSecrets, g.Gateway.Listeners) certBundles := buildCertBundles(g.ReferencedCaCertConfigMaps, backendGroups) + telemetry := buildTelemetry(g) config := Configuration{ HTTPServers: httpServers, @@ -49,6 +50,7 @@ func BuildConfiguration( SSLKeyPairs: keyPairs, Version: configVersion, CertBundles: certBundles, + Telemetry: telemetry, } return config @@ -559,3 +561,34 @@ func generateSSLKeyPairID(secret types.NamespacedName) SSLKeyPairID { func generateCertBundleID(configMap types.NamespacedName) CertBundleID { return CertBundleID(fmt.Sprintf("cert_bundle_%s_%s", configMap.Namespace, configMap.Name)) } + +// buildTelemetry generates the Otel configuration. +func buildTelemetry(g *graph.Graph) Telemetry { + if g.NginxProxy != nil && g.NginxProxy.Spec.Telemetry != nil && g.NginxProxy.Spec.Telemetry.Exporter != nil { + serviceName := fmt.Sprintf("ngf:%s:%s", g.Gateway.Source.Namespace, g.Gateway.Source.Name) + telemetry := g.NginxProxy.Spec.Telemetry + if telemetry.ServiceName != nil { + serviceName = serviceName + ":" + *telemetry.ServiceName + } + + tel := Telemetry{ + Endpoint: telemetry.Exporter.Endpoint, + ServiceName: serviceName, + SpanAttributes: telemetry.SpanAttributes, + } + + if telemetry.Exporter.BatchCount != nil { + tel.BatchCount = *telemetry.Exporter.BatchCount + } + if telemetry.Exporter.BatchSize != nil { + tel.BatchSize = *telemetry.Exporter.BatchSize + } + if telemetry.Exporter.Interval != nil { + tel.Interval = string(*telemetry.Exporter.Interval) + } + + return tel + } + + return Telemetry{} +} diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 4a44b5c5b7..b162accae0 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -14,6 +14,7 @@ import ( v1 "sigs.k8s.io/gateway-api/apis/v1" v1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + 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/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" @@ -539,6 +540,20 @@ func TestBuildConfiguration(t *testing.T) { }, } + nginxProxy := &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-otel.svc:4563", + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + }, + ServiceName: helpers.GetPointer("my-svc"), + }, + }, + } + tests := []struct { graph *graph.Graph msg string @@ -1858,6 +1873,51 @@ func TestBuildConfiguration(t *testing.T) { }, msg: "https listener with httproute with backend that has a backend TLS policy with binaryData attached", }, + { + graph: &graph.Graph{ + GatewayClass: &graph.GatewayClass{ + Source: &v1.GatewayClass{}, + Valid: true, + }, + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + Listeners: []*graph.Listener{ + { + Name: "listener-80-1", + Source: listener80, + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{}, + }, + }, + }, + Routes: map[types.NamespacedName]*graph.Route{}, + NginxProxy: nginxProxy, + }, + expConf: Configuration{ + HTTPServers: []VirtualServer{ + { + IsDefault: true, + Port: 80, + }, + }, + SSLServers: []VirtualServer{}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, + CertBundles: map[CertBundleID]CertBundle{}, + Telemetry: Telemetry{ + Endpoint: "my-otel.svc:4563", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + ServiceName: "ngf:ns:gw:my-svc", + }, + }, + msg: "NginxProxy with tracing config", + }, } for _, test := range tests { @@ -1873,6 +1933,7 @@ func TestBuildConfiguration(t *testing.T) { g.Expect(result.SSLKeyPairs).To(Equal(test.expConf.SSLKeyPairs)) g.Expect(result.Version).To(Equal(1)) g.Expect(result.CertBundles).To(Equal(test.expConf.CertBundles)) + g.Expect(result.Telemetry).To(Equal(test.expConf.Telemetry)) }) } } @@ -2511,3 +2572,63 @@ func TestConvertBackendTLS(t *testing.T) { }) } } + +func TestBuildTelemetry(t *testing.T) { + telemetryConfigured := &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-otel.svc:4563", + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + }, + ServiceName: helpers.GetPointer("my-svc"), + }, + }, + } + + expTelemetryConfigured := Telemetry{ + Endpoint: "my-otel.svc:4563", + ServiceName: "ngf:ns:gw:my-svc", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + } + + tests := []struct { + g *graph.Graph + msg string + expTelemetry Telemetry + }{ + { + g: &graph.Graph{ + NginxProxy: &ngfAPI.NginxProxy{}, + }, + expTelemetry: Telemetry{}, + msg: "No telemetry configured", + }, + { + g: &graph.Graph{ + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + }, + NginxProxy: telemetryConfigured, + }, + expTelemetry: expTelemetryConfigured, + msg: "Telemetry configured", + }, + } + + for _, tc := range tests { + t.Run(tc.msg, func(t *testing.T) { + g := NewWithT(t) + g.Expect(buildTelemetry(tc.g)).To(Equal(tc.expTelemetry)) + }) + } +} diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index da1b00afaa..2d6e1071e8 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -6,6 +6,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -33,6 +34,8 @@ type Configuration struct { Upstreams []Upstream // BackendGroups holds all unique BackendGroups. BackendGroups []BackendGroup + // Telemetry holds the Otel configuration. + Telemetry Telemetry // Version represents the version of the generated configuration. Version int } @@ -249,3 +252,19 @@ type VerifyTLS struct { Hostname string RootCAPath string } + +// Telemetry represents Otel configuration for the dataplane. +type Telemetry struct { + // Endpoint specifies the address of OTLP/gRPC endpoint that will accept telemetry data. + Endpoint string + // ServiceName is the “service.name” attribute of the OTel resource. + ServiceName string + // Interval specifies the export interval. + Interval string + // SpanAttributes are custom key/value attributes that are added to each span. + SpanAttributes []ngfAPI.SpanAttribute + // BatchSize specifies the maximum number of spans to be sent in one batch per worker. + BatchSize int32 + // BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. + BatchCount int32 +} diff --git a/internal/mode/static/state/graph/gatewayclass.go b/internal/mode/static/state/graph/gatewayclass.go index fc3313f269..f18db455c6 100644 --- a/internal/mode/static/state/graph/gatewayclass.go +++ b/internal/mode/static/state/graph/gatewayclass.go @@ -7,6 +7,7 @@ import ( "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/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" @@ -60,13 +61,14 @@ func processGatewayClasses( func buildGatewayClass( gc *v1.GatewayClass, + npCfg *ngfAPI.NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, ) *GatewayClass { if gc == nil { return nil } - conds, valid := validateGatewayClass(gc, crdVersions) + conds, valid := validateGatewayClass(gc, npCfg, crdVersions) return &GatewayClass{ Source: gc, @@ -77,20 +79,33 @@ func buildGatewayClass( func validateGatewayClass( gc *v1.GatewayClass, + npCfg *ngfAPI.NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, ) ([]conditions.Condition, bool) { var conds []conditions.Condition - valid := true - if gc.Spec.ParametersRef != nil { + var err error path := field.NewPath("spec").Child("parametersRef") - err := field.Forbidden(path, "parametersRef is not supported") - conds = append(conds, staticConds.NewGatewayClassInvalidParameters(err.Error())) - valid = false + if _, ok := supportedParamKinds[string(gc.Spec.ParametersRef.Kind)]; !ok { + err = field.NotSupported(path.Child("kind"), string(gc.Spec.ParametersRef.Kind), []string{"NginxProxy"}) + } else if npCfg == nil { + err = field.NotFound(path.Child("name"), gc.Spec.ParametersRef.Name) + conds = append(conds, staticConds.NewGatewayClassRefNotFound()) + } + + if err != nil { + conds = append(conds, staticConds.NewGatewayClassInvalidParameters(err.Error())) + } else { + conds = append(conds, staticConds.NewGatewayClassResolvedRefs()) + } } supportedVersionConds, versionsValid := gatewayclass.ValidateCRDVersions(crdVersions) - return append(conds, supportedVersionConds...), valid && versionsValid + return append(conds, supportedVersionConds...), versionsValid +} + +var supportedParamKinds = map[string]struct{}{ + "NginxProxy": {}, } diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index 11c4feb321..965656f0fe 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -9,6 +9,7 @@ import ( "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/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" @@ -122,9 +123,22 @@ func TestProcessGatewayClasses(t *testing.T) { func TestBuildGatewayClass(t *testing.T) { validGC := &v1.GatewayClass{} - invalidGC := &v1.GatewayClass{ + gcWithParams := &v1.GatewayClass{ Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{}, + ParametersRef: &v1.ParametersReference{ + Kind: v1.Kind("NginxProxy"), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Name: "nginx-proxy", + }, + }, + } + + gcWithInvalidKind := &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Kind: v1.Kind("Invalid"), + Namespace: helpers.GetPointer(v1.Namespace("test")), + }, }, } @@ -150,6 +164,7 @@ func TestBuildGatewayClass(t *testing.T) { tests := []struct { gc *v1.GatewayClass + np *ngfAPI.NginxProxy crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata expected *GatewayClass name string @@ -169,18 +184,50 @@ func TestBuildGatewayClass(t *testing.T) { name: "no gatewayclass", }, { - gc: invalidGC, - crdMetadata: validCRDs, + gc: gcWithParams, + np: &ngfAPI.NginxProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: "NginxProxy", + }, + }, expected: &GatewayClass{ - Source: invalidGC, - Valid: false, + Source: gcWithParams, + Valid: true, + Conditions: []conditions.Condition{staticConds.NewGatewayClassResolvedRefs()}, + }, + name: "valid gatewayclass with paramsRef", + }, + { + gc: gcWithInvalidKind, + np: &ngfAPI.NginxProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: "NginxProxy", + }, + }, + expected: &GatewayClass{ + Source: gcWithInvalidKind, + Valid: true, + Conditions: []conditions.Condition{ + staticConds.NewGatewayClassInvalidParameters( + "spec.parametersRef.kind: Unsupported value: \"Invalid\": supported values: \"NginxProxy\"", + ), + }, + }, + name: "invalid gatewayclass with unsupported paramsRef Kind", + }, + { + gc: gcWithParams, + expected: &GatewayClass{ + Source: gcWithParams, + Valid: true, Conditions: []conditions.Condition{ + staticConds.NewGatewayClassRefNotFound(), staticConds.NewGatewayClassInvalidParameters( - "spec.parametersRef: Forbidden: parametersRef is not supported", + "spec.parametersRef.name: Not found: \"nginx-proxy\"", ), }, }, - name: "invalid gatewayclass; parameters ref", + name: "invalid gatewayclass with paramsRef resource that doesn't exist", }, { gc: validGC, @@ -198,7 +245,7 @@ func TestBuildGatewayClass(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result := buildGatewayClass(test.gc, test.crdMetadata) + result := buildGatewayClass(test.gc, test.np, test.crdMetadata) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index f6ee5598b9..14d3f25216 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -10,6 +10,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1beta1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -26,6 +27,7 @@ type ClusterState struct { CRDMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata BackendTLSPolicies map[types.NamespacedName]*v1alpha2.BackendTLSPolicy ConfigMaps map[types.NamespacedName]*v1.ConfigMap + NginxProxies map[types.NamespacedName]*ngfAPI.NginxProxy } // Graph is a Graph-like representation of Gateway API resources. @@ -58,6 +60,8 @@ type Graph struct { ReferencedCaCertConfigMaps map[types.NamespacedName]*CaCertConfigMap // BackendTLSPolicies holds BackendTLSPolicy resources. BackendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy + // NginxProxy holds the NginxProxy config for the GatewayClass. + NginxProxy *ngfAPI.NginxProxy } // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. @@ -118,7 +122,8 @@ func BuildGraph( return &Graph{} } - gc := buildGatewayClass(processedGwClasses.Winner, state.CRDMetadata) + npCfg := getNginxProxy(state.NginxProxies, processedGwClasses.Winner) + gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata) secretResolver := newSecretResolver(state.Secrets) configMapResolver := newConfigMapResolver(state.ConfigMaps) @@ -154,6 +159,7 @@ func BuildGraph( ReferencedServices: referencedServices, ReferencedCaCertConfigMaps: configMapResolver.getResolvedConfigMaps(), BackendTLSPolicies: processedBackendTLSPolicies, + NginxProxy: npCfg, } return g diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 8a278d673a..f4c1f7ddb2 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1beta1" + 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/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" @@ -288,6 +289,23 @@ func TestBuildGraph(t *testing.T) { }, } + proxy := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "1.2.3.4:123", + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + }, + ServiceName: helpers.GetPointer("my-svc"), + }, + }, + } + createStateWithGatewayClass := func(gc *gatewayv1.GatewayClass) ClusterState { return ClusterState{ GatewayClasses: map[types.NamespacedName]*gatewayv1.GatewayClass{ @@ -321,6 +339,9 @@ func TestBuildGraph(t *testing.T) { ConfigMaps: map[types.NamespacedName]*v1.ConfigMap{ client.ObjectKeyFromObject(cm): cm, }, + NginxProxies: map[types.NamespacedName]*ngfAPI.NginxProxy{ + client.ObjectKeyFromObject(proxy): proxy, + }, } } @@ -361,8 +382,9 @@ func TestBuildGraph(t *testing.T) { createExpectedGraphWithGatewayClass := func(gc *gatewayv1.GatewayClass) *Graph { return &Graph{ GatewayClass: &GatewayClass{ - Source: gc, - Valid: true, + Source: gc, + Valid: true, + Conditions: []conditions.Condition{staticConds.NewGatewayClassResolvedRefs()}, }, Gateway: &Gateway{ Source: gw1, @@ -419,6 +441,7 @@ func TestBuildGraph(t *testing.T) { BackendTLSPolicies: map[types.NamespacedName]*BackendTLSPolicy{ client.ObjectKeyFromObject(btp.Source): &btp, }, + NginxProxy: proxy, } } @@ -428,6 +451,11 @@ func TestBuildGraph(t *testing.T) { }, Spec: gatewayv1.GatewayClassSpec{ ControllerName: controllerName, + ParametersRef: &gatewayv1.ParametersReference{ + Group: gatewayv1.Group("gateway.nginx.org"), + Kind: gatewayv1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, }, } differentControllerGC := &gatewayv1.GatewayClass{ diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go new file mode 100644 index 0000000000..992d1648ab --- /dev/null +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -0,0 +1,23 @@ +package graph + +import ( + "k8s.io/apimachinery/pkg/types" + v1 "sigs.k8s.io/gateway-api/apis/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" +) + +// getNginxProxy returns the NginxProxy associated with the GatewayClass (if it exists). +func getNginxProxy( + nps map[types.NamespacedName]*ngfAPI.NginxProxy, + gc *v1.GatewayClass, +) *ngfAPI.NginxProxy { + if gc != nil { + ref := gc.Spec.ParametersRef + if ref != nil && ref.Group == ngfAPI.GroupName && ref.Kind == v1.Kind("NginxProxy") { + return nps[types.NamespacedName{Name: ref.Name}] + } + } + + return nil +} diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go new file mode 100644 index 0000000000..7376003230 --- /dev/null +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -0,0 +1,129 @@ +package graph + +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" +) + +func TestGetNginxProxy(t *testing.T) { + tests := []struct { + nps map[types.NamespacedName]*ngfAPI.NginxProxy + gc *v1.GatewayClass + expNP *ngfAPI.NginxProxy + name string + }{ + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ + {Name: "np1"}: {}, + }, + gc: nil, + expNP: nil, + name: "nil gatewayclass", + }, + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ + {Name: "np1"}: {}, + }, + gc: &v1.GatewayClass{}, + expNP: nil, + name: "nil paramsRef", + }, + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{}, + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "np1", + }, + }, + }, + expNP: nil, + name: "no nginxproxy resources", + }, + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ + {Name: "np1"}: { + ObjectMeta: metav1.ObjectMeta{ + Name: "np1", + }, + }, + }, + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: v1.Group("wrong-group"), + Kind: v1.Kind("NginxProxy"), + Name: "wrong-group", + }, + }, + }, + expNP: nil, + name: "wrong group", + }, + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ + {Name: "np1"}: { + ObjectMeta: metav1.ObjectMeta{ + Name: "np1", + }, + }, + }, + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("WrongKind"), + Name: "wrong-kind", + }, + }, + }, + expNP: nil, + name: "wrong kind", + }, + { + nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ + {Name: "np1"}: { + ObjectMeta: metav1.ObjectMeta{ + Name: "np1", + }, + }, + {Name: "np2"}: { + ObjectMeta: metav1.ObjectMeta{ + Name: "np2", + }, + }, + }, + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "np2", + }, + }, + }, + expNP: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np2", + }, + }, + name: "returns correct resource", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(getNginxProxy(test.nps, test.gc)).To(Equal(test.expNP)) + }) + } +} diff --git a/site/content/overview/gateway-api-compatibility.md b/site/content/overview/gateway-api-compatibility.md index 2673543e91..2b411b7b2f 100644 --- a/site/content/overview/gateway-api-compatibility.md +++ b/site/content/overview/gateway-api-compatibility.md @@ -58,7 +58,7 @@ NGINX Gateway Fabric supports a single GatewayClass resource configured with the - `spec` - `controllerName` - supported. - - `parametersRef` - not supported. + - `parametersRef` - NginxProxy resource supported. - `description` - supported. - `status` - `conditions` - supported (Condition/Status/Reason): From a0e24b78424365690ec51aa7f509967ca0e105ca Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 24 Apr 2024 09:01:45 -0600 Subject: [PATCH 02/15] Remove unsupported architectures --- build/Dockerfile.nginx | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/build/Dockerfile.nginx b/build/Dockerfile.nginx index 6152c8d567..ad12ffe2b3 100644 --- a/build/Dockerfile.nginx +++ b/build/Dockerfile.nginx @@ -1,18 +1,11 @@ # syntax=docker/dockerfile:1.6 -FROM scratch as nginx-files - -# the following links can be replaced with local files if needed, i.e. ADD --chown=101:1001 -ADD --link --chown=101:1001 https://cs.nginx.com/static/keys/nginx_signing.rsa.pub nginx_signing.rsa.pub - -FROM nginx:1.25.5-alpine +FROM nginx:1.25.5-alpine-otel ARG NJS_DIR ARG NGINX_CONF_DIR ARG BUILD_AGENT -RUN --mount=type=bind,from=nginx-files,src=nginx_signing.rsa.pub,target=/etc/apk/keys/nginx_signing.rsa.pub \ - printf "%s\n" "http://nginx.org/packages/mainline/alpine/v$(egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \ - && apk add --no-cache libcap nginx-module-otel \ +RUN apk add --no-cache libcap \ && mkdir -p /var/lib/nginx /usr/lib/nginx/modules \ && setcap 'cap_net_bind_service=+ep' /usr/sbin/nginx \ && setcap -v 'cap_net_bind_service=+ep' /usr/sbin/nginx \ From 9c9f72a5502fe93fce487f166b5a6b01b8ba0364 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 24 Apr 2024 14:34:40 -0600 Subject: [PATCH 03/15] Quote string variables in conf --- internal/mode/static/nginx/config/generator_test.go | 6 +++--- .../mode/static/nginx/config/telemetry_template.go | 8 ++++---- internal/mode/static/nginx/config/telemetry_test.go | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index ef834d25e5..09a071a625 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -99,11 +99,11 @@ func TestGenerate(t *testing.T) { g.Expect(httpCfg).To(ContainSubstring("upstream")) g.Expect(httpCfg).To(ContainSubstring("split_clients")) - g.Expect(httpCfg).To(ContainSubstring("endpoint 1.2.3.4:123;")) - g.Expect(httpCfg).To(ContainSubstring("interval 5s;")) + g.Expect(httpCfg).To(ContainSubstring(`endpoint "1.2.3.4:123";`)) + g.Expect(httpCfg).To(ContainSubstring(`interval "5s";`)) g.Expect(httpCfg).To(ContainSubstring("batch_size 512;")) 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(`otel_service_name "ngf:gw-ns:gw-name:my-name";`)) g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/matches.json")) diff --git a/internal/mode/static/nginx/config/telemetry_template.go b/internal/mode/static/nginx/config/telemetry_template.go index 7d41b5352d..6fcf297642 100644 --- a/internal/mode/static/nginx/config/telemetry_template.go +++ b/internal/mode/static/nginx/config/telemetry_template.go @@ -2,9 +2,9 @@ package config const otelTemplateText = ` otel_exporter { - endpoint {{ .Endpoint }}; + endpoint "{{ .Endpoint }}"; {{- if .Interval }} - interval {{ .Interval }}; + interval "{{ .Interval }}"; {{- end }} {{- if .BatchSize }} batch_size {{ .BatchSize }}; @@ -14,9 +14,9 @@ otel_exporter { {{- end }} } -otel_service_name {{ .ServiceName }}; +otel_service_name "{{ .ServiceName }}"; {{- range $attr := .SpanAttributes }} -otel_span_attr {{ $attr.Key }} {{ $attr.Value }}; +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; {{- end }} ` diff --git a/internal/mode/static/nginx/config/telemetry_test.go b/internal/mode/static/nginx/config/telemetry_test.go index b260a0f0c9..ecfa143a65 100644 --- a/internal/mode/static/nginx/config/telemetry_test.go +++ b/internal/mode/static/nginx/config/telemetry_test.go @@ -33,12 +33,12 @@ func TestExecuteTelemetry(t *testing.T) { g := NewWithT(t) expSubStrings := map[string]int{ - "endpoint 1.2.3.4:123;": 1, - "otel_service_name ngf:gw-ns:gw-name:my-name;": 1, - "interval 5s;": 1, - "batch_size 512;": 1, - "batch_count 4;": 1, - "otel_span_attr": 2, + `endpoint "1.2.3.4:123";`: 1, + `otel_service_name "ngf:gw-ns:gw-name:my-name";`: 1, + `interval "5s";`: 1, + "batch_size 512;": 1, + "batch_count 4;": 1, + "otel_span_attr": 2, } for expSubStr, expCount := range expSubStrings { From ca72108c84a1d5596aae49982a60d0f51a484737 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 09:56:48 -0600 Subject: [PATCH 04/15] Code review; add validation and helm template --- charts/nginx-gateway-fabric/README.md | 3 +- .../templates/_helpers.tpl | 8 + .../templates/deployment.yaml | 4 +- .../templates/gatewayclass.yaml | 6 + .../templates/nginxproxy.yaml | 10 + .../nginx-gateway-fabric/templates/rbac.yaml | 8 +- charts/nginx-gateway-fabric/values.yaml | 11 + .../provisioner/static-deployment.yaml | 4 +- .../manifests/nginx-gateway-experimental.yaml | 12 +- deploy/manifests/nginx-gateway.yaml | 12 +- .../nginx-plus-gateway-experimental.yaml | 12 +- deploy/manifests/nginx-plus-gateway.yaml | 12 +- internal/mode/static/manager.go | 1 + .../mode/static/nginx/conf/nginx-plus.conf | 2 +- internal/mode/static/nginx/conf/nginx.conf | 2 +- .../mode/static/nginx/config/generator.go | 7 +- .../static/nginx/config/telemetry_test.go | 3 +- .../static/nginx/config/validation/common.go | 9 + .../nginx/config/validation/common_test.go | 21 ++ .../nginx/config/validation/http_validator.go | 1 + .../mode/static/state/change_processor.go | 2 +- .../static/state/change_processor_test.go | 5 +- .../static/state/dataplane/configuration.go | 52 ++-- .../state/dataplane/configuration_test.go | 22 +- internal/mode/static/state/dataplane/types.go | 12 +- .../mode/static/state/graph/gatewayclass.go | 12 +- .../static/state/graph/gatewayclass_test.go | 54 +++- internal/mode/static/state/graph/graph.go | 7 +- .../mode/static/state/graph/graph_test.go | 63 +++- .../mode/static/state/graph/nginxproxy.go | 85 +++++- .../static/state/graph/nginxproxy_test.go | 278 +++++++++++++++--- .../validationfakes/fake_generic_validator.go | 111 +++++++ .../mode/static/state/validation/validator.go | 11 +- .../installation/installing-ngf/helm.md | 2 +- 34 files changed, 757 insertions(+), 107 deletions(-) create mode 100644 charts/nginx-gateway-fabric/templates/nginxproxy.yaml create mode 100644 internal/mode/static/state/validation/validationfakes/fake_generic_validator.go diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index 2d3b7bba9f..bc5084c842 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -224,7 +224,7 @@ To uninstall/delete the release `ngf`: ```shell helm uninstall ngf -n nginx-gateway kubectl delete ns nginx-gateway -for crd in `kubectl get crds -oname | grep gateway.nginx.org | awk -F / '{ print $2 }'`; do kubectl delete crd $crd; done +kubectl delete crd nginxgateways.gateway.nginx.org nginxproxies.gateway.nginx.org ``` These commands remove all the Kubernetes components associated with the release and deletes the release. @@ -269,6 +269,7 @@ The following tables lists the configurable parameters of the NGINX Gateway Fabr | `nginx.image.tag` | The tag for the NGINX image. | edge | | `nginx.image.pullPolicy` | The `imagePullPolicy` for the NGINX image. | Always | | `nginx.plus` | Is NGINX Plus image being used | false | +| `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. | [See nginx.config section](values.yaml) | | `nginx.usage.secretName` | The namespace/name of the Secret containing the credentials for NGINX Plus usage reporting. | | | `nginx.usage.serverURL` | The base server URL of the NGINX Plus usage reporting server. | | | `nginx.usage.clusterName` | The display name of the Kubernetes cluster in the NGINX Plus usage reporting server. | | diff --git a/charts/nginx-gateway-fabric/templates/_helpers.tpl b/charts/nginx-gateway-fabric/templates/_helpers.tpl index f94eeb379f..13d78128ae 100644 --- a/charts/nginx-gateway-fabric/templates/_helpers.tpl +++ b/charts/nginx-gateway-fabric/templates/_helpers.tpl @@ -31,6 +31,14 @@ Create control plane config name. {{- printf "%s-config" $name | trunc 63 | trimSuffix "-" }} {{- end }} +{{/* +Create data plane config name. +*/}} +{{- define "nginx-gateway.proxy-config-name" -}} +{{- $name := default .Release.Name .Values.nameOverride }} +{{- printf "%s-proxy-config" $name | trunc 63 | trimSuffix "-" }} +{{- end }} + {{/* Create chart name and version as used by the chart label. */}} diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index 10f629b459..aa6e9bed71 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -119,7 +119,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -152,7 +152,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/charts/nginx-gateway-fabric/templates/gatewayclass.yaml b/charts/nginx-gateway-fabric/templates/gatewayclass.yaml index 9bb6a01c17..4ecafd287b 100644 --- a/charts/nginx-gateway-fabric/templates/gatewayclass.yaml +++ b/charts/nginx-gateway-fabric/templates/gatewayclass.yaml @@ -6,3 +6,9 @@ metadata: {{- include "nginx-gateway.labels" . | nindent 4 }} spec: controllerName: {{ .Values.nginxGateway.gatewayControllerName }} + {{- if .Values.nginx.config }} + parametersRef: + group: gateway.nginx.org + kind: NginxProxy + name: {{ include "nginx-gateway.proxy-config-name" . }} + {{- end }} diff --git a/charts/nginx-gateway-fabric/templates/nginxproxy.yaml b/charts/nginx-gateway-fabric/templates/nginxproxy.yaml new file mode 100644 index 0000000000..4214158b75 --- /dev/null +++ b/charts/nginx-gateway-fabric/templates/nginxproxy.yaml @@ -0,0 +1,10 @@ +{{- if .Values.nginx.config }} +apiVersion: gateway.nginx.org/v1alpha1 +kind: NginxProxy +metadata: + name: {{ include "nginx-gateway.proxy-config-name" . }} + labels: + {{- include "nginx-gateway.labels" . | nindent 4 }} +spec: + {{- toYaml .Values.nginx.config | nindent 2 }} +{{- end }} diff --git a/charts/nginx-gateway-fabric/templates/rbac.yaml b/charts/nginx-gateway-fabric/templates/rbac.yaml index eb1c472416..8d29b828b9 100644 --- a/charts/nginx-gateway-fabric/templates/rbac.yaml +++ b/charts/nginx-gateway-fabric/templates/rbac.yaml @@ -111,11 +111,17 @@ rules: - gateway.nginx.org resources: - nginxgateways - - nginxproxies verbs: - get - list - watch +- apiGroups: + - gateway.nginx.org + resources: + - nginxproxies + verbs: + - list + - watch - apiGroups: - gateway.nginx.org resources: diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index c0dbe5eb6f..4b7430e9ae 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -70,6 +70,17 @@ nginx: ## Is NGINX Plus image being used plus: false + ## The configuration for the data plane that is contained in the NginxProxy resource. + config: {} + # telemetry: + # exporter: + # endpoint: otel-collector.default.svc:4317 + # interval: 5s + # batchSize: 512 + # batchCount: 4 + # serviceName: "" + # spanAttributes: [] + ## Configuration for NGINX Plus usage reporting. usage: ## The namespace/name of the Secret containing the credentials for NGINX Plus usage reporting. diff --git a/conformance/provisioner/static-deployment.yaml b/conformance/provisioner/static-deployment.yaml index 8b2713585e..f67a040272 100644 --- a/conformance/provisioner/static-deployment.yaml +++ b/conformance/provisioner/static-deployment.yaml @@ -71,7 +71,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -97,7 +97,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/deploy/manifests/nginx-gateway-experimental.yaml b/deploy/manifests/nginx-gateway-experimental.yaml index 1a8b2e5780..79a9de476a 100644 --- a/deploy/manifests/nginx-gateway-experimental.yaml +++ b/deploy/manifests/nginx-gateway-experimental.yaml @@ -93,11 +93,17 @@ rules: - gateway.nginx.org resources: - nginxgateways - - nginxproxies verbs: - get - list - watch +- apiGroups: + - gateway.nginx.org + resources: + - nginxproxies + verbs: + - list + - watch - apiGroups: - gateway.nginx.org resources: @@ -215,7 +221,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -241,7 +247,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 928dedd147..f57bf1d39a 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -90,11 +90,17 @@ rules: - gateway.nginx.org resources: - nginxgateways - - nginxproxies verbs: - get - list - watch +- apiGroups: + - gateway.nginx.org + resources: + - nginxproxies + verbs: + - list + - watch - apiGroups: - gateway.nginx.org resources: @@ -211,7 +217,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -237,7 +243,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/deploy/manifests/nginx-plus-gateway-experimental.yaml b/deploy/manifests/nginx-plus-gateway-experimental.yaml index 08b66daef4..e55cc622ee 100644 --- a/deploy/manifests/nginx-plus-gateway-experimental.yaml +++ b/deploy/manifests/nginx-plus-gateway-experimental.yaml @@ -99,11 +99,17 @@ rules: - gateway.nginx.org resources: - nginxgateways - - nginxproxies verbs: - get - list - watch +- apiGroups: + - gateway.nginx.org + resources: + - nginxproxies + verbs: + - list + - watch - apiGroups: - gateway.nginx.org resources: @@ -222,7 +228,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -248,7 +254,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/deploy/manifests/nginx-plus-gateway.yaml b/deploy/manifests/nginx-plus-gateway.yaml index 4d0f72d4b2..f711c32c6e 100644 --- a/deploy/manifests/nginx-plus-gateway.yaml +++ b/deploy/manifests/nginx-plus-gateway.yaml @@ -96,11 +96,17 @@ rules: - gateway.nginx.org resources: - nginxgateways - - nginxproxies verbs: - get - list - watch +- apiGroups: + - gateway.nginx.org + resources: + - nginxproxies + verbs: + - list + - watch - apiGroups: - gateway.nginx.org resources: @@ -218,7 +224,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -244,7 +250,7 @@ spec: - name: nginx-conf mountPath: /etc/nginx/conf.d - name: nginx-includes - mountPath: /etc/nginx/includes + mountPath: /etc/nginx/modules-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index f1a9ff2451..cb68557165 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -116,6 +116,7 @@ func StartManager(cfg config.Config) error { Logger: cfg.Logger.WithName("changeProcessor"), Validators: validation.Validators{ HTTPFieldsValidator: ngxvalidation.HTTPValidator{}, + GenericValidator: ngxvalidation.GenericValidator{}, }, EventRecorder: recorder, Scheme: scheme, diff --git a/internal/mode/static/nginx/conf/nginx-plus.conf b/internal/mode/static/nginx/conf/nginx-plus.conf index e41ecbba07..08f8ee104d 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/includes/*.conf; +include /etc/nginx/modules-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/conf/nginx.conf b/internal/mode/static/nginx/conf/nginx.conf index 5df383c8a2..c93e5d364d 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/includes/*.conf; +include /etc/nginx/modules-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index b755bca5f6..cb9f5b29f1 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -16,6 +16,9 @@ const ( // httpFolder is the folder where NGINX HTTP configuration files are stored. httpFolder = configFolder + "/conf.d" + // modulesIncludesFolder is the folder where the included "load_module" file is stored. + modulesIncludesFolder = configFolder + "/modules-includes" + // secretsFolder is the folder where secrets (like TLS certs/keys) are stored. secretsFolder = configFolder + "/secrets" @@ -29,11 +32,11 @@ const ( httpMatchVarsFile = httpFolder + "/matches.json" // loadModulesFile is the path to the file containing any load_module directives. - loadModulesFile = configFolder + "/includes/load_modules.conf" + loadModulesFile = modulesIncludesFolder + "/load-modules.conf" ) // ConfigFolders is a list of folders where NGINX configuration files are stored. -var ConfigFolders = []string{httpFolder, secretsFolder} +var ConfigFolders = []string{httpFolder, secretsFolder, modulesIncludesFolder} // Generator generates NGINX configuration files. // This interface is used for testing purposes only. diff --git a/internal/mode/static/nginx/config/telemetry_test.go b/internal/mode/static/nginx/config/telemetry_test.go index ecfa143a65..ddaf0d5814 100644 --- a/internal/mode/static/nginx/config/telemetry_test.go +++ b/internal/mode/static/nginx/config/telemetry_test.go @@ -6,7 +6,6 @@ import ( . "github.com/onsi/gomega" - ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -18,7 +17,7 @@ func TestExecuteTelemetry(t *testing.T) { Interval: "5s", BatchSize: 512, BatchCount: 4, - SpanAttributes: []ngfAPI.SpanAttribute{ + SpanAttributes: []dataplane.SpanAttribute{ { Key: "key1", Value: "val1", diff --git a/internal/mode/static/nginx/config/validation/common.go b/internal/mode/static/nginx/config/validation/common.go index bca6a97095..df85999234 100644 --- a/internal/mode/static/nginx/config/validation/common.go +++ b/internal/mode/static/nginx/config/validation/common.go @@ -84,3 +84,12 @@ func validateHeaderName(name string) error { } return nil } + +// GenericValidator validates values for generic cases in the nginx conf. +type GenericValidator struct{} + +// ValidateEscapedStringNoVarExpansion ensures that no invalid characters are included in the string value that +// could lead to unwanted nginx behavior. +func (GenericValidator) ValidateEscapedStringNoVarExpansion(value string) error { + return validateEscapedStringNoVarExpansion(value, nil) +} diff --git a/internal/mode/static/nginx/config/validation/common_test.go b/internal/mode/static/nginx/config/validation/common_test.go index f7c2f3be74..5b1f108640 100644 --- a/internal/mode/static/nginx/config/validation/common_test.go +++ b/internal/mode/static/nginx/config/validation/common_test.go @@ -71,3 +71,24 @@ func TestValidateValidHeaderName(t *testing.T) { strings.Repeat("very-long-header", 16)+"1", ) } + +func TestGenericValidator_ValidateEscapedStringNoVarExpansion(t *testing.T) { + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateEscapedStringNoVarExpansion, + `test`, + `test test`, + `\"`, + `\\`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateEscapedStringNoVarExpansion, + `\`, + `test"test`, + `$test`, + ) +} diff --git a/internal/mode/static/nginx/config/validation/http_validator.go b/internal/mode/static/nginx/config/validation/http_validator.go index f33adb8434..37512da468 100644 --- a/internal/mode/static/nginx/config/validation/http_validator.go +++ b/internal/mode/static/nginx/config/validation/http_validator.go @@ -12,6 +12,7 @@ type HTTPValidator struct { HTTPRedirectValidator HTTPURLRewriteValidator HTTPRequestHeaderValidator + GenericValidator } var _ validation.HTTPFieldsValidator = HTTPValidator{} diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 533c2c5876..e5ae41cc52 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -187,7 +187,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&ngfAPI.NginxProxy{}), store: newObjectStoreMapAdapter(clusterStore.NginxProxies), - predicate: nil, + predicate: funcPredicate{stateChanged: isReferenced}, }, }, ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 09e6c2f434..dbb0659bb7 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -172,10 +172,9 @@ func createBackendRef( } func createAlwaysValidValidators() validation.Validators { - http := &validationfakes.FakeHTTPFieldsValidator{} - return validation.Validators{ - HTTPFieldsValidator: http, + HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}, + GenericValidator: &validationfakes.FakeGenericValidator{}, } } diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index 123a7eb83b..d9f0e4264e 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -564,31 +564,41 @@ func generateCertBundleID(configMap types.NamespacedName) CertBundleID { // buildTelemetry generates the Otel configuration. func buildTelemetry(g *graph.Graph) Telemetry { - if g.NginxProxy != nil && g.NginxProxy.Spec.Telemetry != nil && g.NginxProxy.Spec.Telemetry.Exporter != nil { - serviceName := fmt.Sprintf("ngf:%s:%s", g.Gateway.Source.Namespace, g.Gateway.Source.Name) - telemetry := g.NginxProxy.Spec.Telemetry - if telemetry.ServiceName != nil { - serviceName = serviceName + ":" + *telemetry.ServiceName - } + if g.NginxProxy == nil || g.NginxProxy.Spec.Telemetry == nil || g.NginxProxy.Spec.Telemetry.Exporter == nil { + return Telemetry{} + } - tel := Telemetry{ - Endpoint: telemetry.Exporter.Endpoint, - ServiceName: serviceName, - SpanAttributes: telemetry.SpanAttributes, - } + serviceName := fmt.Sprintf("ngf:%s:%s", g.Gateway.Source.Namespace, g.Gateway.Source.Name) + telemetry := g.NginxProxy.Spec.Telemetry + if telemetry.ServiceName != nil { + serviceName = serviceName + ":" + *telemetry.ServiceName + } - if telemetry.Exporter.BatchCount != nil { - tel.BatchCount = *telemetry.Exporter.BatchCount - } - if telemetry.Exporter.BatchSize != nil { - tel.BatchSize = *telemetry.Exporter.BatchSize - } - if telemetry.Exporter.Interval != nil { - tel.Interval = string(*telemetry.Exporter.Interval) + tel := Telemetry{ + Endpoint: telemetry.Exporter.Endpoint, + ServiceName: serviceName, + } + + spanAttrs := make([]SpanAttribute, 0, len(g.NginxProxy.Spec.Telemetry.SpanAttributes)) + for _, spanAttr := range g.NginxProxy.Spec.Telemetry.SpanAttributes { + sa := SpanAttribute{ + Key: spanAttr.Key, + Value: spanAttr.Value, } + spanAttrs = append(spanAttrs, sa) + } - return tel + tel.SpanAttributes = spanAttrs + + if telemetry.Exporter.BatchCount != nil { + tel.BatchCount = *telemetry.Exporter.BatchCount + } + if telemetry.Exporter.BatchSize != nil { + tel.BatchSize = *telemetry.Exporter.BatchSize + } + if telemetry.Exporter.Interval != nil { + tel.Interval = string(*telemetry.Exporter.Interval) } - return Telemetry{} + return tel } diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index b162accae0..c59c8f8694 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -1909,11 +1909,12 @@ func TestBuildConfiguration(t *testing.T) { SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, CertBundles: map[CertBundleID]CertBundle{}, Telemetry: Telemetry{ - Endpoint: "my-otel.svc:4563", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, - ServiceName: "ngf:ns:gw:my-svc", + Endpoint: "my-otel.svc:4563", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + ServiceName: "ngf:ns:gw:my-svc", + SpanAttributes: []SpanAttribute{}, }, }, msg: "NginxProxy with tracing config", @@ -2589,11 +2590,12 @@ func TestBuildTelemetry(t *testing.T) { } expTelemetryConfigured := Telemetry{ - Endpoint: "my-otel.svc:4563", - ServiceName: "ngf:ns:gw:my-svc", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, + Endpoint: "my-otel.svc:4563", + ServiceName: "ngf:ns:gw:my-svc", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + SpanAttributes: []SpanAttribute{}, } tests := []struct { diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index 2d6e1071e8..80662715d4 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -6,7 +6,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -262,9 +261,18 @@ type Telemetry struct { // Interval specifies the export interval. Interval string // SpanAttributes are custom key/value attributes that are added to each span. - SpanAttributes []ngfAPI.SpanAttribute + SpanAttributes []SpanAttribute // BatchSize specifies the maximum number of spans to be sent in one batch per worker. BatchSize int32 // BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. BatchCount int32 } + +// SpanAttribute is a key value pair to be added to a tracing span. +type SpanAttribute struct { + // Key is the key for a span attribute. + Key string + + // Value is the value for a span attribute. + Value string +} diff --git a/internal/mode/static/state/graph/gatewayclass.go b/internal/mode/static/state/graph/gatewayclass.go index f18db455c6..23bbb078e6 100644 --- a/internal/mode/static/state/graph/gatewayclass.go +++ b/internal/mode/static/state/graph/gatewayclass.go @@ -1,6 +1,8 @@ package graph import ( + "errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -11,6 +13,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) // GatewayClass represents the GatewayClass resource. @@ -63,12 +66,13 @@ func buildGatewayClass( gc *v1.GatewayClass, npCfg *ngfAPI.NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, + validator validation.GenericValidator, ) *GatewayClass { if gc == nil { return nil } - conds, valid := validateGatewayClass(gc, npCfg, crdVersions) + conds, valid := validateGatewayClass(gc, npCfg, crdVersions, validator) return &GatewayClass{ Source: gc, @@ -81,6 +85,7 @@ func validateGatewayClass( gc *v1.GatewayClass, npCfg *ngfAPI.NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, + validator validation.GenericValidator, ) ([]conditions.Condition, bool) { var conds []conditions.Condition @@ -92,6 +97,11 @@ func validateGatewayClass( } else if npCfg == nil { err = field.NotFound(path.Child("name"), gc.Spec.ParametersRef.Name) conds = append(conds, staticConds.NewGatewayClassRefNotFound()) + } else { + nginxProxyErrs := validateNginxProxy(validator, npCfg) + if len(nginxProxyErrs) > 0 { + err = errors.New(nginxProxyErrs.ToAggregate().Error()) + } } if err != nil { diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index 965656f0fe..b90ac0fb54 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -1,6 +1,7 @@ package graph import ( + "errors" "testing" . "github.com/onsi/gomega" @@ -14,6 +15,8 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "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" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) func TestProcessGatewayClasses(t *testing.T) { @@ -162,9 +165,24 @@ func TestBuildGatewayClass(t *testing.T) { }, } + createValidNPValidator := func() *validationfakes.FakeGenericValidator { + v := &validationfakes.FakeGenericValidator{} + v.ValidateEscapedStringNoVarExpansionReturns(nil) + + return v + } + + createInvalidNPValidator := func() *validationfakes.FakeGenericValidator { + v := &validationfakes.FakeGenericValidator{} + v.ValidateEscapedStringNoVarExpansionReturns(errors.New("error")) + + return v + } + tests := []struct { gc *v1.GatewayClass np *ngfAPI.NginxProxy + validator validation.GenericValidator crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata expected *GatewayClass name string @@ -189,7 +207,13 @@ func TestBuildGatewayClass(t *testing.T) { TypeMeta: metav1.TypeMeta{ Kind: "NginxProxy", }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), + }, + }, }, + validator: createValidNPValidator(), expected: &GatewayClass{ Source: gcWithParams, Valid: true, @@ -229,6 +253,34 @@ func TestBuildGatewayClass(t *testing.T) { }, name: "invalid gatewayclass with paramsRef resource that doesn't exist", }, + { + gc: gcWithParams, + np: &ngfAPI.NginxProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: "NginxProxy", + }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-endpoint", + }, + }, + }, + }, + validator: createInvalidNPValidator(), + expected: &GatewayClass{ + Source: gcWithParams, + Valid: true, + Conditions: []conditions.Condition{ + staticConds.NewGatewayClassInvalidParameters( + "[spec.telemetry.serviceName: Invalid value: \"my-svc\": error" + + ", spec.telemetry.exporter.endpoint: Invalid value: \"my-endpoint\": error]", + ), + }, + }, + name: "invalid gatewayclass with invalid paramsRef resource", + }, { gc: validGC, crdMetadata: invalidCRDs, @@ -245,7 +297,7 @@ func TestBuildGatewayClass(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result := buildGatewayClass(test.gc, test.np, test.crdMetadata) + result := buildGatewayClass(test.gc, test.np, test.crdMetadata, test.validator) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 14d3f25216..f94510c908 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -103,6 +103,11 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced // Service Namespace should be the same Namespace as the EndpointSlice _, exists := g.ReferencedServices[types.NamespacedName{Namespace: nsname.Namespace, Name: svcName}] return exists + // Similar to Namespace above, NginxProxy reference exists if it once was or currently is linked to a GatewayClass. + case *ngfAPI.NginxProxy: + existed := client.ObjectKeyFromObject(obj) == client.ObjectKeyFromObject(g.NginxProxy) + exists := isNginxProxyReferenced(obj, g.GatewayClass) + return existed || exists default: return false } @@ -123,7 +128,7 @@ func BuildGraph( } npCfg := getNginxProxy(state.NginxProxies, processedGwClasses.Winner) - gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata) + gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata, validators.GenericValidator) secretResolver := newSecretResolver(state.Secrets) configMapResolver := newConfigMapResolver(state.ConfigMaps) diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index f4c1f7ddb2..180372f23e 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -492,7 +492,10 @@ func TestBuildGraph(t *testing.T) { test.store, controllerName, gcName, - validation.Validators{HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}}, + validation.Validators{ + HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}, + GenericValidator: &validationfakes.FakeGenericValidator{}, + }, protectedPorts, ) @@ -610,6 +613,36 @@ func TestIsReferenced(t *testing.T) { }, } + gcWithNginxProxy := &GatewayClass{ + Source: &gatewayv1.GatewayClass{ + Spec: gatewayv1.GatewayClassSpec{ + ParametersRef: &gatewayv1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: gatewayv1.Kind("NginxProxy"), + Name: "nginx-proxy-in-gc", + }, + }, + }, + } + + npInGraph := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + } + + npNotInGraphButInGatewayClass := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy-in-gc", + }, + } + + npNotInGraph := &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy-not-referenced", + }, + } + graph := &Graph{ Gateway: gw, ReferencedSecrets: map[types.NamespacedName]*Secret{ @@ -629,10 +662,12 @@ func TestIsReferenced(t *testing.T) { CACert: []byte(caBlock), }, }, + NginxProxy: npInGraph, } tests := []struct { graph *Graph + gc *GatewayClass resource client.Object name string expected bool @@ -724,7 +759,7 @@ func TestIsReferenced(t *testing.T) { expected: false, }, - // ConfigMap cases + // ConfigMap tests { name: "ConfigMap in graph's ReferencedConfigMaps is referenced", resource: baseConfigMap, @@ -744,6 +779,28 @@ func TestIsReferenced(t *testing.T) { expected: false, }, + // NginxProxy tests + { + name: "NginxProxy in the Graph is referenced", + resource: npInGraph, + graph: graph, + expected: true, + }, + { + name: "NginxProxy is not yet in Graph but is referenced in GatewayClass", + resource: npNotInGraphButInGatewayClass, + gc: gcWithNginxProxy, + graph: graph, + expected: true, + }, + { + name: "NginxProxy not in Graph or referenced in GatewayClass", + resource: npNotInGraph, + gc: gcWithNginxProxy, + graph: graph, + expected: false, + }, + // Edge cases { name: "Resource is not supported by IsReferenced", @@ -755,6 +812,8 @@ func TestIsReferenced(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) + + test.graph.GatewayClass = test.gc result := test.graph.IsReferenced(test.resource, client.ObjectKeyFromObject(test.resource)) g.Expect(result).To(Equal(test.expected)) }) diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index 992d1648ab..e79b7c753e 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -2,9 +2,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/mode/static/state/validation" ) // getNginxProxy returns the NginxProxy associated with the GatewayClass (if it exists). @@ -12,12 +14,89 @@ func getNginxProxy( nps map[types.NamespacedName]*ngfAPI.NginxProxy, gc *v1.GatewayClass, ) *ngfAPI.NginxProxy { + if gcReferencesAnyNginxProxy(gc) { + return nps[types.NamespacedName{Name: gc.Spec.ParametersRef.Name}] + } + + return nil +} + +// isNginxProxyReferenced returns whether or not a specific NginxProxy is referenced in the GatewayClass. +func isNginxProxyReferenced(np *ngfAPI.NginxProxy, gc *GatewayClass) bool { + return np != nil && gc != nil && + gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == np.Name +} + +// gcReferencesNginxProxy returns whether a GatewayClass references any NginxProxy resource. +func gcReferencesAnyNginxProxy(gc *v1.GatewayClass) bool { if gc != nil { ref := gc.Spec.ParametersRef - if ref != nil && ref.Group == ngfAPI.GroupName && ref.Kind == v1.Kind("NginxProxy") { - return nps[types.NamespacedName{Name: ref.Name}] + return ref != nil && ref.Group == ngfAPI.GroupName && ref.Kind == v1.Kind("NginxProxy") + } + + return false +} + +// validateNginxProxy performs re-validation on string values in the case of CRD validation failure. +func validateNginxProxy( + validator validation.GenericValidator, + npCfg *ngfAPI.NginxProxy, +) field.ErrorList { + var allErrs field.ErrorList + spec := field.NewPath("spec") + + validateStringValue := func( + value, + valueName string, + path *field.Path, + validator validation.GenericValidator, + ) *field.Error { + if err := validator.ValidateEscapedStringNoVarExpansion(value); err != nil { + return field.Invalid(path.Child(valueName), value, err.Error()) } + + return nil } - return nil + telemetry := npCfg.Spec.Telemetry + if telemetry != nil { + telPath := spec.Child("telemetry") + if telemetry.ServiceName != nil { + if err := validateStringValue(*telemetry.ServiceName, "serviceName", telPath, validator); err != nil { + allErrs = append(allErrs, err) + } + } + + if telemetry.Exporter != nil { + exp := telemetry.Exporter + expPath := telPath.Child("exporter") + + if exp.Endpoint != "" { + if err := validateStringValue(exp.Endpoint, "endpoint", expPath, validator); err != nil { + allErrs = append(allErrs, err) + } + } + + if exp.Interval != nil { + if err := validateStringValue(string(*exp.Interval), "interval", expPath, validator); err != nil { + allErrs = append(allErrs, err) + } + } + } + + if telemetry.SpanAttributes != nil { + spanAttrPath := telPath.Child("spanAttributes") + for _, spanAttr := range telemetry.SpanAttributes { + if err := validateStringValue(spanAttr.Key, "key", spanAttrPath, validator); err != nil { + allErrs = append(allErrs, err) + } + + if err := validateStringValue(spanAttr.Value, "value", spanAttrPath, validator); err != nil { + allErrs = append(allErrs, err) + } + } + } + } + + return allErrs } diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go index 7376003230..ea3fb5fbc3 100644 --- a/internal/mode/static/state/graph/nginxproxy_test.go +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -1,6 +1,7 @@ package graph import ( + "errors" "testing" . "github.com/onsi/gomega" @@ -9,6 +10,8 @@ import ( 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/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) func TestGetNginxProxy(t *testing.T) { @@ -26,14 +29,6 @@ func TestGetNginxProxy(t *testing.T) { expNP: nil, name: "nil gatewayclass", }, - { - nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ - {Name: "np1"}: {}, - }, - gc: &v1.GatewayClass{}, - expNP: nil, - name: "nil paramsRef", - }, { nps: map[types.NamespacedName]*ngfAPI.NginxProxy{}, gc: &v1.GatewayClass{ @@ -55,67 +50,274 @@ func TestGetNginxProxy(t *testing.T) { Name: "np1", }, }, + {Name: "np2"}: { + ObjectMeta: metav1.ObjectMeta{ + Name: "np2", + }, + }, }, gc: &v1.GatewayClass{ Spec: v1.GatewayClassSpec{ ParametersRef: &v1.ParametersReference{ - Group: v1.Group("wrong-group"), + Group: ngfAPI.GroupName, Kind: v1.Kind("NginxProxy"), - Name: "wrong-group", + Name: "np2", }, }, }, - expNP: nil, - name: "wrong group", + expNP: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np2", + }, + }, + name: "returns correct resource", }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(getNginxProxy(test.nps, test.gc)).To(Equal(test.expNP)) + }) + } +} + +func TestIsNginxProxyReferenced(t *testing.T) { + tests := []struct { + gc *GatewayClass + np *ngfAPI.NginxProxy + name string + expRes bool + }{ { - nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ - {Name: "np1"}: { - ObjectMeta: metav1.ObjectMeta{ - Name: "np1", + gc: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, }, }, }, + np: nil, + expRes: false, + name: "nil nginxproxy", + }, + { + gc: nil, + np: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + }, + expRes: false, + name: "nil gatewayclass", + }, + { + gc: &GatewayClass{ + Source: nil, + }, + np: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + }, + expRes: false, + name: "nil gatewayclass source", + }, + { + gc: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, + }, + }, + }, + np: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + }, + expRes: true, + name: "references the NginxProxy", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(isNginxProxyReferenced(test.np, test.gc)).To(Equal(test.expRes)) + }) + } +} + +func TestGCReferencesAnyNginxProxy(t *testing.T) { + tests := []struct { + gc *v1.GatewayClass + name string + expRes bool + }{ + { + gc: nil, + expRes: false, + name: "nil gatewayclass", + }, + { + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{}, + }, + expRes: false, + name: "nil paramsRef", + }, + { gc: &v1.GatewayClass{ Spec: v1.GatewayClassSpec{ ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("WrongKind"), - Name: "wrong-kind", + Group: v1.Group("wrong-group"), + Kind: v1.Kind("NginxProxy"), + Name: "wrong-group", }, }, }, - expNP: nil, - name: "wrong kind", + expRes: false, + name: "wrong group name", }, { - nps: map[types.NamespacedName]*ngfAPI.NginxProxy{ - {Name: "np1"}: { - ObjectMeta: metav1.ObjectMeta{ - Name: "np1", - }, - }, - {Name: "np2"}: { - ObjectMeta: metav1.ObjectMeta{ - Name: "np2", + gc: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("WrongKind"), + Name: "wrong-kind", }, }, }, + expRes: false, + name: "wrong kind", + }, + { gc: &v1.GatewayClass{ Spec: v1.GatewayClassSpec{ ParametersRef: &v1.ParametersReference{ Group: ngfAPI.GroupName, Kind: v1.Kind("NginxProxy"), - Name: "np2", + Name: "nginx-proxy", }, }, }, - expNP: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "np2", + expRes: true, + name: "references an NginxProxy", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(gcReferencesAnyNginxProxy(test.gc)).To(Equal(test.expRes)) + }) + } +} + +func TestValidateNginxProxy(t *testing.T) { + createValidValidator := func() *validationfakes.FakeGenericValidator { + v := &validationfakes.FakeGenericValidator{} + v.ValidateEscapedStringNoVarExpansionReturns(nil) + + return v + } + + createInvalidValidator := func() *validationfakes.FakeGenericValidator { + v := &validationfakes.FakeGenericValidator{} + v.ValidateEscapedStringNoVarExpansionReturns(errors.New("error")) + + return v + } + + tests := []struct { + np *ngfAPI.NginxProxy + validator *validationfakes.FakeGenericValidator + name string + expErrSubstring string + expectErrCount int + }{ + { + name: "valid nginxproxy", + validator: createValidValidator(), + np: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), + }, }, }, - name: "returns correct resource", + expectErrCount: 0, + }, + { + name: "invalid serviceName", + validator: createInvalidValidator(), + np: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), // any value is invalid by the validator + }, + }, + }, + expErrSubstring: "telemetry.serviceName", + expectErrCount: 1, + }, + { + name: "invalid endpoint", + validator: createInvalidValidator(), + np: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-endpoint", // any value is invalid by the validator + }, + }, + }, + }, + expErrSubstring: "telemetry.exporter.endpoint", + expectErrCount: 1, + }, + { + name: "invalid interval", + validator: createInvalidValidator(), + np: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Interval: helpers.GetPointer[ngfAPI.Duration]("my-interval"), // any value is invalid by the validator + }, + }, + }, + }, + expErrSubstring: "telemetry.exporter.interval", + expectErrCount: 1, + }, + { + name: "invalid spanAttributes", + validator: createInvalidValidator(), + np: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "my-key", Value: "my-value"}, // any value is invalid by the validator + }, + }, + }, + }, + expErrSubstring: "telemetry.spanAttributes", + expectErrCount: 2, }, } @@ -123,7 +325,11 @@ func TestGetNginxProxy(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(getNginxProxy(test.nps, test.gc)).To(Equal(test.expNP)) + allErrs := validateNginxProxy(test.validator, test.np) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + if len(allErrs) > 0 { + g.Expect(allErrs.ToAggregate().Error()).To(ContainSubstring(test.expErrSubstring)) + } }) } } diff --git a/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go b/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go new file mode 100644 index 0000000000..8898251e51 --- /dev/null +++ b/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go @@ -0,0 +1,111 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package validationfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" +) + +type FakeGenericValidator struct { + ValidateEscapedStringNoVarExpansionStub func(string) error + validateEscapedStringNoVarExpansionMutex sync.RWMutex + validateEscapedStringNoVarExpansionArgsForCall []struct { + arg1 string + } + validateEscapedStringNoVarExpansionReturns struct { + result1 error + } + validateEscapedStringNoVarExpansionReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansion(arg1 string) error { + fake.validateEscapedStringNoVarExpansionMutex.Lock() + ret, specificReturn := fake.validateEscapedStringNoVarExpansionReturnsOnCall[len(fake.validateEscapedStringNoVarExpansionArgsForCall)] + fake.validateEscapedStringNoVarExpansionArgsForCall = append(fake.validateEscapedStringNoVarExpansionArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateEscapedStringNoVarExpansionStub + fakeReturns := fake.validateEscapedStringNoVarExpansionReturns + fake.recordInvocation("ValidateEscapedStringNoVarExpansion", []interface{}{arg1}) + fake.validateEscapedStringNoVarExpansionMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionCallCount() int { + fake.validateEscapedStringNoVarExpansionMutex.RLock() + defer fake.validateEscapedStringNoVarExpansionMutex.RUnlock() + return len(fake.validateEscapedStringNoVarExpansionArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionCalls(stub func(string) error) { + fake.validateEscapedStringNoVarExpansionMutex.Lock() + defer fake.validateEscapedStringNoVarExpansionMutex.Unlock() + fake.ValidateEscapedStringNoVarExpansionStub = stub +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionArgsForCall(i int) string { + fake.validateEscapedStringNoVarExpansionMutex.RLock() + defer fake.validateEscapedStringNoVarExpansionMutex.RUnlock() + argsForCall := fake.validateEscapedStringNoVarExpansionArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionReturns(result1 error) { + fake.validateEscapedStringNoVarExpansionMutex.Lock() + defer fake.validateEscapedStringNoVarExpansionMutex.Unlock() + fake.ValidateEscapedStringNoVarExpansionStub = nil + fake.validateEscapedStringNoVarExpansionReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionReturnsOnCall(i int, result1 error) { + fake.validateEscapedStringNoVarExpansionMutex.Lock() + defer fake.validateEscapedStringNoVarExpansionMutex.Unlock() + fake.ValidateEscapedStringNoVarExpansionStub = nil + if fake.validateEscapedStringNoVarExpansionReturnsOnCall == nil { + fake.validateEscapedStringNoVarExpansionReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateEscapedStringNoVarExpansionReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.validateEscapedStringNoVarExpansionMutex.RLock() + defer fake.validateEscapedStringNoVarExpansionMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeGenericValidator) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ validation.GenericValidator = new(FakeGenericValidator) diff --git a/internal/mode/static/state/validation/validator.go b/internal/mode/static/state/validation/validator.go index d6433ad363..86a9f302b2 100644 --- a/internal/mode/static/state/validation/validator.go +++ b/internal/mode/static/state/validation/validator.go @@ -1,11 +1,12 @@ package validation -// Validators include validators for Gateway API resources from the perspective of a data-plane. +// Validators include validators for API resources from the perspective of a data-plane. // It is used for fields that propagate into the data plane configuration. For example, the path in a routing rule. // However, not all such fields are validated: NGF will not validate a field using Validators if it is confident that // the field is valid. type Validators struct { HTTPFieldsValidator HTTPFieldsValidator + GenericValidator GenericValidator } // HTTPFieldsValidator validates the HTTP-related fields of Gateway API resources from the perspective of @@ -27,3 +28,11 @@ type HTTPFieldsValidator interface { ValidateRequestHeaderName(name string) error ValidateRequestHeaderValue(value string) error } + +// GenericValidator validates any generic values from NGF API resources from the perspective of a data-plane. +// These could be values that we want to re-validate in case of any CRD schema manipulation. +// +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . GenericValidator +type GenericValidator interface { + ValidateEscapedStringNoVarExpansion(value string) error +} diff --git a/site/content/installation/installing-ngf/helm.md b/site/content/installation/installing-ngf/helm.md index 8c061ce3c1..3daf082fb1 100644 --- a/site/content/installation/installing-ngf/helm.md +++ b/site/content/installation/installing-ngf/helm.md @@ -256,7 +256,7 @@ Follow these steps to uninstall NGINX Gateway Fabric and Gateway API from your K ```shell kubectl delete ns nginx-gateway - kubectl delete crd nginxgateways.gateway.nginx.org + kubectl delete crd nginxgateways.gateway.nginx.org nginxproxies.gateway.nginx.org ``` 3. **Remove the Gateway API resources:** From b994dc20eeb3b90f07af20732c1b443a84f525ea Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 09:59:40 -0600 Subject: [PATCH 05/15] Revert arch removal --- .github/workflows/ci.yml | 2 +- .goreleaser.yml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a158f1bad..7e5c84b88c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,7 +172,7 @@ jobs: fail-fast: false matrix: image: [ngf, nginx] - platforms: ["linux/arm64, linux/amd64"] + platforms: ["linux/arm64, linux/amd64, linux/s390x, linux/ppc64le"] uses: ./.github/workflows/build.yml with: image: ${{ matrix.image }} diff --git a/.goreleaser.yml b/.goreleaser.yml index 117e2ac3e4..86c1d5671b 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -8,6 +8,8 @@ builds: goarch: - amd64 - arm64 + - s390x + - ppc64le flags: - -trimpath gcflags: From 219bccfb046d7cd0904db7f32e3a684654e70e57 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 10:04:46 -0600 Subject: [PATCH 06/15] Fix nil check --- internal/mode/static/state/graph/graph.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index f94510c908..e2707b7683 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -105,7 +105,10 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced return exists // Similar to Namespace above, NginxProxy reference exists if it once was or currently is linked to a GatewayClass. case *ngfAPI.NginxProxy: - existed := client.ObjectKeyFromObject(obj) == client.ObjectKeyFromObject(g.NginxProxy) + var existed bool + if g.NginxProxy != nil { + existed = client.ObjectKeyFromObject(obj) == client.ObjectKeyFromObject(g.NginxProxy) + } exists := isNginxProxyReferenced(obj, g.GatewayClass) return existed || exists default: From e7d953c91ed2d4b4787047da8c0e8944b840a563 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 14:41:25 -0600 Subject: [PATCH 07/15] Code review --- .../templates/deployment.yaml | 10 +- .../provisioner/static-deployment.yaml | 10 +- .../manifests/nginx-gateway-experimental.yaml | 10 +- deploy/manifests/nginx-gateway.yaml | 10 +- .../nginx-plus-gateway-experimental.yaml | 10 +- deploy/manifests/nginx-plus-gateway.yaml | 10 +- .../mode/static/nginx/conf/nginx-plus.conf | 2 +- internal/mode/static/nginx/conf/nginx.conf | 2 +- .../mode/static/nginx/config/generator.go | 2 +- .../static/nginx/config/generator_test.go | 5 + .../state/dataplane/configuration_test.go | 17 ++- internal/mode/static/state/dataplane/types.go | 1 - internal/mode/static/state/graph/graph.go | 7 +- .../mode/static/state/graph/nginxproxy.go | 9 +- .../static/state/graph/nginxproxy_test.go | 111 ++++++++++++------ 15 files changed, 131 insertions(+), 85 deletions(-) diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index aa6e9bed71..83aeafde21 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -118,8 +118,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -151,8 +151,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -185,7 +185,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/conformance/provisioner/static-deployment.yaml b/conformance/provisioner/static-deployment.yaml index f67a040272..dc59c6a929 100644 --- a/conformance/provisioner/static-deployment.yaml +++ b/conformance/provisioner/static-deployment.yaml @@ -70,8 +70,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -96,8 +96,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -115,7 +115,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/deploy/manifests/nginx-gateway-experimental.yaml b/deploy/manifests/nginx-gateway-experimental.yaml index 79a9de476a..5d6449a98f 100644 --- a/deploy/manifests/nginx-gateway-experimental.yaml +++ b/deploy/manifests/nginx-gateway-experimental.yaml @@ -220,8 +220,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -246,8 +246,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -265,7 +265,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index f57bf1d39a..f10dff697a 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -216,8 +216,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -242,8 +242,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -261,7 +261,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/deploy/manifests/nginx-plus-gateway-experimental.yaml b/deploy/manifests/nginx-plus-gateway-experimental.yaml index e55cc622ee..f43819b427 100644 --- a/deploy/manifests/nginx-plus-gateway-experimental.yaml +++ b/deploy/manifests/nginx-plus-gateway-experimental.yaml @@ -227,8 +227,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -253,8 +253,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -272,7 +272,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/deploy/manifests/nginx-plus-gateway.yaml b/deploy/manifests/nginx-plus-gateway.yaml index f711c32c6e..c94be584ba 100644 --- a/deploy/manifests/nginx-plus-gateway.yaml +++ b/deploy/manifests/nginx-plus-gateway.yaml @@ -223,8 +223,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -249,8 +249,8 @@ spec: volumeMounts: - name: nginx-conf mountPath: /etc/nginx/conf.d - - name: nginx-includes - mountPath: /etc/nginx/modules-includes + - name: module-includes + mountPath: /etc/nginx/module-includes - name: nginx-secrets mountPath: /etc/nginx/secrets - name: nginx-run @@ -268,7 +268,7 @@ spec: volumes: - name: nginx-conf emptyDir: {} - - name: nginx-includes + - name: module-includes emptyDir: {} - name: nginx-secrets emptyDir: {} diff --git a/internal/mode/static/nginx/conf/nginx-plus.conf b/internal/mode/static/nginx/conf/nginx-plus.conf index 08f8ee104d..b8b9cc77d8 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/modules-includes/*.conf; +include /etc/nginx/module-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/conf/nginx.conf b/internal/mode/static/nginx/conf/nginx.conf index c93e5d364d..b70e857e0d 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/modules-includes/*.conf; +include /etc/nginx/module-includes/*.conf; worker_processes auto; diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index cb9f5b29f1..6ed5afb94d 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -17,7 +17,7 @@ const ( httpFolder = configFolder + "/conf.d" // modulesIncludesFolder is the folder where the included "load_module" file is stored. - modulesIncludesFolder = configFolder + "/modules-includes" + modulesIncludesFolder = configFolder + "/module-includes" // secretsFolder is the folder where secrets (like TLS certs/keys) are stored. secretsFolder = configFolder + "/secrets" diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 09a071a625..707f1ca862 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -118,9 +118,14 @@ func TestGenerate(t *testing.T) { certBundle := string(files[3].Content) g.Expect(certBundle).To(Equal("test-cert")) +<<<<<<< HEAD g.Expect(files[5]).To(Equal(file.File{ Type: file.TypeSecret, Path: "/etc/nginx/secrets/test-keypair.pem", Content: []byte("test-cert\ntest-key"), })) +======= + g.Expect(files[4].Path).To(Equal("/etc/nginx/module-includes/load-modules.conf")) + g.Expect(files[4].Content).To(Equal([]byte("load_module modules/ngx_otel_module.so;"))) +>>>>>>> dfc23a2 (Code review) } diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index c59c8f8694..5ffde9f604 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -2585,17 +2585,22 @@ func TestBuildTelemetry(t *testing.T) { Interval: helpers.GetPointer(ngfAPI.Duration("5s")), }, ServiceName: helpers.GetPointer("my-svc"), + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, }, }, } expTelemetryConfigured := Telemetry{ - Endpoint: "my-otel.svc:4563", - ServiceName: "ngf:ns:gw:my-svc", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, - SpanAttributes: []SpanAttribute{}, + Endpoint: "my-otel.svc:4563", + ServiceName: "ngf:ns:gw:my-svc", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + SpanAttributes: []SpanAttribute{ + {Key: "key", Value: "value"}, + }, } tests := []struct { diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index 80662715d4..21c5749dfb 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -272,7 +272,6 @@ type Telemetry struct { type SpanAttribute struct { // Key is the key for a span attribute. Key string - // Value is the value for a span attribute. Value string } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index e2707b7683..a08d00de86 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -105,12 +105,7 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced return exists // Similar to Namespace above, NginxProxy reference exists if it once was or currently is linked to a GatewayClass. case *ngfAPI.NginxProxy: - var existed bool - if g.NginxProxy != nil { - existed = client.ObjectKeyFromObject(obj) == client.ObjectKeyFromObject(g.NginxProxy) - } - exists := isNginxProxyReferenced(obj, g.GatewayClass) - return existed || exists + return isNginxProxyReferenced(nsname, g) default: return false } diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index e79b7c753e..a367fffad0 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -3,6 +3,7 @@ package graph import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" @@ -22,9 +23,11 @@ func getNginxProxy( } // isNginxProxyReferenced returns whether or not a specific NginxProxy is referenced in the GatewayClass. -func isNginxProxyReferenced(np *ngfAPI.NginxProxy, gc *GatewayClass) bool { - return np != nil && gc != nil && - gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == np.Name +func isNginxProxyReferenced(npNSName types.NamespacedName, g *Graph) bool { + existed := g.NginxProxy != nil && npNSName == client.ObjectKeyFromObject(g.NginxProxy) + gc := g.GatewayClass + exists := gc != nil && gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == npNSName.Name + return existed || exists } // gcReferencesNginxProxy returns whether a GatewayClass references any NginxProxy resource. diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go index ea3fb5fbc3..fd99b246a9 100644 --- a/internal/mode/static/state/graph/nginxproxy_test.go +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -85,68 +85,107 @@ func TestGetNginxProxy(t *testing.T) { func TestIsNginxProxyReferenced(t *testing.T) { tests := []struct { - gc *GatewayClass - np *ngfAPI.NginxProxy + g *Graph + npName types.NamespacedName name string expRes bool }{ { - gc: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("NginxProxy"), - Name: "nginx-proxy", + g: &Graph{ + GatewayClass: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, }, }, }, }, - np: nil, + npName: types.NamespacedName{Name: "not referenced"}, expRes: false, - name: "nil nginxproxy", + name: "nginxproxy not in graph and not referenced in gatewayclass", }, { - gc: nil, - np: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy", - }, + g: &Graph{ + GatewayClass: nil, + NginxProxy: nil, }, + npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: false, - name: "nil gatewayclass", + name: "nil gatewayclass and nginxproxy not in graph", }, { - gc: &GatewayClass{ - Source: nil, - }, - np: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy", + g: &Graph{ + GatewayClass: &GatewayClass{ + Source: nil, }, + NginxProxy: nil, }, + npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: false, - name: "nil gatewayclass source", + name: "nil gatewayclass source and nginxproxy not in graph", }, { - gc: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("NginxProxy"), - Name: "nginx-proxy", + g: &Graph{ + GatewayClass: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, }, }, }, + NginxProxy: nil, }, - np: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy", + npName: types.NamespacedName{Name: "nginx-proxy"}, + expRes: true, + name: "nginxproxy not in graph but referenced in gatewayclass", + }, + { + g: &Graph{ + GatewayClass: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{}, + }, + }, + NginxProxy: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, + }, + }, + npName: types.NamespacedName{Name: "nginx-proxy"}, + expRes: true, + name: "nginxproxy in graph but not referenced in gatewayclass", + }, + { + g: &Graph{ + GatewayClass: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", + }, + }, + }, + }, + NginxProxy: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-proxy", + }, }, }, + npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: true, - name: "references the NginxProxy", + name: "references the nginxproxy and exists in graph", }, } @@ -154,7 +193,7 @@ func TestIsNginxProxyReferenced(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(isNginxProxyReferenced(test.np, test.gc)).To(Equal(test.expRes)) + g.Expect(isNginxProxyReferenced(test.npName, test.g)).To(Equal(test.expRes)) }) } } From 585375f945a15b26ffad9199e73d7a5350e6e906 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 15:06:12 -0600 Subject: [PATCH 08/15] Remove validator --- internal/mode/static/nginx/config/validation/http_validator.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/mode/static/nginx/config/validation/http_validator.go b/internal/mode/static/nginx/config/validation/http_validator.go index 37512da468..f33adb8434 100644 --- a/internal/mode/static/nginx/config/validation/http_validator.go +++ b/internal/mode/static/nginx/config/validation/http_validator.go @@ -12,7 +12,6 @@ type HTTPValidator struct { HTTPRedirectValidator HTTPURLRewriteValidator HTTPRequestHeaderValidator - GenericValidator } var _ validation.HTTPFieldsValidator = HTTPValidator{} From 1fbe4a6c2ec0a9b493a076ab6c7a8c44774d07ed Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Fri, 26 Apr 2024 15:55:57 -0600 Subject: [PATCH 09/15] Revert reference check --- internal/mode/static/state/graph/graph.go | 4 +- .../mode/static/state/graph/graph_test.go | 25 ++--- .../mode/static/state/graph/nginxproxy.go | 8 +- .../static/state/graph/nginxproxy_test.go | 99 +++++-------------- 4 files changed, 34 insertions(+), 102 deletions(-) diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index a08d00de86..af60f41a20 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -103,9 +103,9 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced // Service Namespace should be the same Namespace as the EndpointSlice _, exists := g.ReferencedServices[types.NamespacedName{Namespace: nsname.Namespace, Name: svcName}] return exists - // Similar to Namespace above, NginxProxy reference exists if it once was or currently is linked to a GatewayClass. + // NginxProxy reference exists if it is linked to a GatewayClass. case *ngfAPI.NginxProxy: - return isNginxProxyReferenced(nsname, g) + return isNginxProxyReferenced(nsname, g.GatewayClass) default: return false } diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 180372f23e..12bfb4f3ca 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -625,24 +625,18 @@ func TestIsReferenced(t *testing.T) { }, } - npInGraph := &ngfAPI.NginxProxy{ + npNotInGatewayClass := &ngfAPI.NginxProxy{ ObjectMeta: metav1.ObjectMeta{ Name: "nginx-proxy", }, } - npNotInGraphButInGatewayClass := &ngfAPI.NginxProxy{ + npInGatewayClass := &ngfAPI.NginxProxy{ ObjectMeta: metav1.ObjectMeta{ Name: "nginx-proxy-in-gc", }, } - npNotInGraph := &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy-not-referenced", - }, - } - graph := &Graph{ Gateway: gw, ReferencedSecrets: map[types.NamespacedName]*Secret{ @@ -662,7 +656,6 @@ func TestIsReferenced(t *testing.T) { CACert: []byte(caBlock), }, }, - NginxProxy: npInGraph, } tests := []struct { @@ -781,21 +774,15 @@ func TestIsReferenced(t *testing.T) { // NginxProxy tests { - name: "NginxProxy in the Graph is referenced", - resource: npInGraph, - graph: graph, - expected: true, - }, - { - name: "NginxProxy is not yet in Graph but is referenced in GatewayClass", - resource: npNotInGraphButInGatewayClass, + name: "NginxProxy is referenced in GatewayClass", + resource: npInGatewayClass, gc: gcWithNginxProxy, graph: graph, expected: true, }, { - name: "NginxProxy not in Graph or referenced in GatewayClass", - resource: npNotInGraph, + name: "NginxProxy is not referenced in GatewayClass", + resource: npNotInGatewayClass, gc: gcWithNginxProxy, graph: graph, expected: false, diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index a367fffad0..73427594ee 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -3,7 +3,6 @@ package graph import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" @@ -23,11 +22,8 @@ func getNginxProxy( } // isNginxProxyReferenced returns whether or not a specific NginxProxy is referenced in the GatewayClass. -func isNginxProxyReferenced(npNSName types.NamespacedName, g *Graph) bool { - existed := g.NginxProxy != nil && npNSName == client.ObjectKeyFromObject(g.NginxProxy) - gc := g.GatewayClass - exists := gc != nil && gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == npNSName.Name - return existed || exists +func isNginxProxyReferenced(npNSName types.NamespacedName, gc *GatewayClass) bool { + return gc != nil && gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == npNSName.Name } // gcReferencesNginxProxy returns whether a GatewayClass references any NginxProxy resource. diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go index fd99b246a9..add1c55204 100644 --- a/internal/mode/static/state/graph/nginxproxy_test.go +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -85,107 +85,56 @@ func TestGetNginxProxy(t *testing.T) { func TestIsNginxProxyReferenced(t *testing.T) { tests := []struct { - g *Graph + gc *GatewayClass npName types.NamespacedName name string expRes bool }{ { - g: &Graph{ - GatewayClass: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("NginxProxy"), - Name: "nginx-proxy", - }, + gc: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", }, }, }, }, - npName: types.NamespacedName{Name: "not referenced"}, + npName: types.NamespacedName{}, expRes: false, - name: "nginxproxy not in graph and not referenced in gatewayclass", + name: "nil nginxproxy", }, { - g: &Graph{ - GatewayClass: nil, - NginxProxy: nil, - }, + gc: nil, npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: false, - name: "nil gatewayclass and nginxproxy not in graph", + name: "nil gatewayclass", }, { - g: &Graph{ - GatewayClass: &GatewayClass{ - Source: nil, - }, - NginxProxy: nil, + gc: &GatewayClass{ + Source: nil, }, npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: false, - name: "nil gatewayclass source and nginxproxy not in graph", + name: "nil gatewayclass source", }, { - g: &Graph{ - GatewayClass: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("NginxProxy"), - Name: "nginx-proxy", - }, + gc: &GatewayClass{ + Source: &v1.GatewayClass{ + Spec: v1.GatewayClassSpec{ + ParametersRef: &v1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: v1.Kind("NginxProxy"), + Name: "nginx-proxy", }, }, }, - NginxProxy: nil, - }, - npName: types.NamespacedName{Name: "nginx-proxy"}, - expRes: true, - name: "nginxproxy not in graph but referenced in gatewayclass", - }, - { - g: &Graph{ - GatewayClass: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{}, - }, - }, - NginxProxy: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy", - }, - }, - }, - npName: types.NamespacedName{Name: "nginx-proxy"}, - expRes: true, - name: "nginxproxy in graph but not referenced in gatewayclass", - }, - { - g: &Graph{ - GatewayClass: &GatewayClass{ - Source: &v1.GatewayClass{ - Spec: v1.GatewayClassSpec{ - ParametersRef: &v1.ParametersReference{ - Group: ngfAPI.GroupName, - Kind: v1.Kind("NginxProxy"), - Name: "nginx-proxy", - }, - }, - }, - }, - NginxProxy: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-proxy", - }, - }, }, npName: types.NamespacedName{Name: "nginx-proxy"}, expRes: true, - name: "references the nginxproxy and exists in graph", + name: "references the NginxProxy", }, } @@ -193,7 +142,7 @@ func TestIsNginxProxyReferenced(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(isNginxProxyReferenced(test.npName, test.g)).To(Equal(test.expRes)) + g.Expect(isNginxProxyReferenced(test.npName, test.gc)).To(Equal(test.expRes)) }) } } From 7453b0b70eceea9fda4db425253d84f933172aca Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 09:00:45 -0600 Subject: [PATCH 10/15] More specific validators --- .../static/nginx/config/validation/common.go | 9 - .../nginx/config/validation/common_test.go | 21 -- .../static/nginx/config/validation/generic.go | 83 +++++++ .../nginx/config/validation/generic_test.go | 86 +++++++ .../mode/static/state/graph/nginxproxy.go | 33 +-- .../static/state/graph/nginxproxy_test.go | 13 + .../validationfakes/fake_generic_validator.go | 222 ++++++++++++++++++ .../mode/static/state/validation/validator.go | 3 + 8 files changed, 417 insertions(+), 53 deletions(-) create mode 100644 internal/mode/static/nginx/config/validation/generic.go create mode 100644 internal/mode/static/nginx/config/validation/generic_test.go diff --git a/internal/mode/static/nginx/config/validation/common.go b/internal/mode/static/nginx/config/validation/common.go index df85999234..bca6a97095 100644 --- a/internal/mode/static/nginx/config/validation/common.go +++ b/internal/mode/static/nginx/config/validation/common.go @@ -84,12 +84,3 @@ func validateHeaderName(name string) error { } return nil } - -// GenericValidator validates values for generic cases in the nginx conf. -type GenericValidator struct{} - -// ValidateEscapedStringNoVarExpansion ensures that no invalid characters are included in the string value that -// could lead to unwanted nginx behavior. -func (GenericValidator) ValidateEscapedStringNoVarExpansion(value string) error { - return validateEscapedStringNoVarExpansion(value, nil) -} diff --git a/internal/mode/static/nginx/config/validation/common_test.go b/internal/mode/static/nginx/config/validation/common_test.go index 5b1f108640..f7c2f3be74 100644 --- a/internal/mode/static/nginx/config/validation/common_test.go +++ b/internal/mode/static/nginx/config/validation/common_test.go @@ -71,24 +71,3 @@ func TestValidateValidHeaderName(t *testing.T) { strings.Repeat("very-long-header", 16)+"1", ) } - -func TestGenericValidator_ValidateEscapedStringNoVarExpansion(t *testing.T) { - validator := GenericValidator{} - - testValidValuesForSimpleValidator( - t, - validator.ValidateEscapedStringNoVarExpansion, - `test`, - `test test`, - `\"`, - `\\`, - ) - - testInvalidValuesForSimpleValidator( - t, - validator.ValidateEscapedStringNoVarExpansion, - `\`, - `test"test`, - `$test`, - ) -} diff --git a/internal/mode/static/nginx/config/validation/generic.go b/internal/mode/static/nginx/config/validation/generic.go new file mode 100644 index 0000000000..5568650f25 --- /dev/null +++ b/internal/mode/static/nginx/config/validation/generic.go @@ -0,0 +1,83 @@ +package validation + +import ( + "errors" + "regexp" + + k8svalidation "k8s.io/apimachinery/pkg/util/validation" +) + +// GenericValidator validates values for generic cases in the nginx conf. +type GenericValidator struct{} + +// ValidateEscapedStringNoVarExpansion ensures that no invalid characters are included in the string value that +// could lead to unwanted nginx behavior. +func (GenericValidator) ValidateEscapedStringNoVarExpansion(value string) error { + return validateEscapedStringNoVarExpansion(value, nil) +} + +const ( + alphaNumericStringFmt = `[a-zA-Z0-9_-]+` + alphaNumericStringErrMsg = "must contain only alphanumeric characters or '-' or '_'" +) + +var alphaNumericStringFmtRegexp = regexp.MustCompile("^" + alphaNumericStringFmt + "$") + +// ValidateServiceName validates a k8s service name that can only use alphanumeric characters. +func (GenericValidator) ValidateServiceName(name string) error { + if !alphaNumericStringFmtRegexp.MatchString(name) { + examples := []string{ + "svc1", + "svc-1", + "svc_1", + } + + return errors.New(k8svalidation.RegexError(alphaNumericStringErrMsg, alphaNumericStringFmt, examples...)) + } + + return nil +} + +const ( + durationStringFmt = `\d{1,4}(ms|s)?` + durationStringErrMsg = "must contain a number followed by 'ms' or 's'" +) + +var durationStringFmtRegexp = regexp.MustCompile("^" + durationStringFmt + "$") + +// ValidateNginxDuration validates a duration string that nginx can understand. +func (GenericValidator) ValidateNginxDuration(duration string) error { + if !durationStringFmtRegexp.MatchString(duration) { + examples := []string{ + "5ms", + "10s", + } + + return errors.New(k8svalidation.RegexError(durationStringFmt, durationStringErrMsg, examples...)) + } + + return nil +} + +const ( + //nolint:lll + endpointStringFmt = `(?:http?:\/\/)?[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(?:\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*(?::\d{1,5})?` + endpointStringErrMsg = "must be an alphanumeric hostname with optional http scheme and optional port" +) + +var endpointStringFmtRegexp = regexp.MustCompile("^" + endpointStringFmt + "$") + +// ValidateEndpoint validates an alphanumeric endpoint, with optional http scheme and port. +func (GenericValidator) ValidateEndpoint(endpoint string) error { + if !endpointStringFmtRegexp.MatchString(endpoint) { + examples := []string{ + "my-endpoint", + "my.endpoint:5678", + "http://my-endpoint", + } + + return errors.New(k8svalidation.RegexError(endpointStringFmt, endpointStringErrMsg, examples...)) + } + + return nil +} diff --git a/internal/mode/static/nginx/config/validation/generic_test.go b/internal/mode/static/nginx/config/validation/generic_test.go new file mode 100644 index 0000000000..b07131ccf0 --- /dev/null +++ b/internal/mode/static/nginx/config/validation/generic_test.go @@ -0,0 +1,86 @@ +package validation + +import "testing" + +func TestGenericValidator_ValidateEscapedStringNoVarExpansion(t *testing.T) { + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateEscapedStringNoVarExpansion, + `test`, + `test test`, + `\"`, + `\\`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateEscapedStringNoVarExpansion, + `\`, + `test"test`, + `$test`, + ) +} + +func TestValidateServiceName(t *testing.T) { + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateServiceName, + `test`, + `Test-test`, + `test_Test`, + `test123`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateServiceName, + `test#$%`, + `test test`, + `test.test`, + ) +} + +func TestValidateNginxDuration(t *testing.T) { + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateNginxDuration, + `5ms`, + `10s`, + `123ms`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateNginxDuration, + `test`, + `12345`, + `5m`, + ) +} + +func TestValidateEndpoint(t *testing.T) { + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateEndpoint, + `http://my-endpoint:5678`, + `my.endpoint`, + `myendpoint:123`, + `my-endpoint123:456`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateEndpoint, + `https://my-endpoint`, + `my_endpoint`, + `my$endpoint`, + ) +} diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index 73427594ee..7088d0c4bb 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -44,25 +44,12 @@ func validateNginxProxy( var allErrs field.ErrorList spec := field.NewPath("spec") - validateStringValue := func( - value, - valueName string, - path *field.Path, - validator validation.GenericValidator, - ) *field.Error { - if err := validator.ValidateEscapedStringNoVarExpansion(value); err != nil { - return field.Invalid(path.Child(valueName), value, err.Error()) - } - - return nil - } - telemetry := npCfg.Spec.Telemetry if telemetry != nil { telPath := spec.Child("telemetry") if telemetry.ServiceName != nil { - if err := validateStringValue(*telemetry.ServiceName, "serviceName", telPath, validator); err != nil { - allErrs = append(allErrs, err) + if err := validator.ValidateEscapedStringNoVarExpansion(*telemetry.ServiceName); err != nil { + allErrs = append(allErrs, field.Invalid(telPath.Child("serviceName"), *telemetry.ServiceName, err.Error())) } } @@ -71,14 +58,14 @@ func validateNginxProxy( expPath := telPath.Child("exporter") if exp.Endpoint != "" { - if err := validateStringValue(exp.Endpoint, "endpoint", expPath, validator); err != nil { - allErrs = append(allErrs, err) + if err := validator.ValidateEscapedStringNoVarExpansion(exp.Endpoint); err != nil { + allErrs = append(allErrs, field.Invalid(expPath.Child("endpoint"), exp.Endpoint, err.Error())) } } if exp.Interval != nil { - if err := validateStringValue(string(*exp.Interval), "interval", expPath, validator); err != nil { - allErrs = append(allErrs, err) + if err := validator.ValidateEscapedStringNoVarExpansion(string(*exp.Interval)); err != nil { + allErrs = append(allErrs, field.Invalid(expPath.Child("interval"), *exp.Interval, err.Error())) } } } @@ -86,12 +73,12 @@ func validateNginxProxy( if telemetry.SpanAttributes != nil { spanAttrPath := telPath.Child("spanAttributes") for _, spanAttr := range telemetry.SpanAttributes { - if err := validateStringValue(spanAttr.Key, "key", spanAttrPath, validator); err != nil { - allErrs = append(allErrs, err) + if err := validator.ValidateEscapedStringNoVarExpansion(spanAttr.Key); err != nil { + allErrs = append(allErrs, field.Invalid(spanAttrPath.Child("key"), spanAttr.Key, err.Error())) } - if err := validateStringValue(spanAttr.Value, "value", spanAttrPath, validator); err != nil { - allErrs = append(allErrs, err) + if err := validator.ValidateEscapedStringNoVarExpansion(spanAttr.Value); err != nil { + allErrs = append(allErrs, field.Invalid(spanAttrPath.Child("value"), spanAttr.Value, err.Error())) } } } diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go index add1c55204..80e66da56a 100644 --- a/internal/mode/static/state/graph/nginxproxy_test.go +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -219,6 +219,9 @@ func TestValidateNginxProxy(t *testing.T) { createValidValidator := func() *validationfakes.FakeGenericValidator { v := &validationfakes.FakeGenericValidator{} v.ValidateEscapedStringNoVarExpansionReturns(nil) + v.ValidateEndpointReturns(nil) + v.ValidateServiceNameReturns(nil) + v.ValidateNginxDurationReturns(nil) return v } @@ -226,6 +229,9 @@ func TestValidateNginxProxy(t *testing.T) { createInvalidValidator := func() *validationfakes.FakeGenericValidator { v := &validationfakes.FakeGenericValidator{} v.ValidateEscapedStringNoVarExpansionReturns(errors.New("error")) + v.ValidateEndpointReturns(errors.New("error")) + v.ValidateServiceNameReturns(errors.New("error")) + v.ValidateNginxDurationReturns(errors.New("error")) return v } @@ -244,6 +250,13 @@ func TestValidateNginxProxy(t *testing.T) { Spec: ngfAPI.NginxProxySpec{ Telemetry: &ngfAPI.Telemetry{ ServiceName: helpers.GetPointer("my-svc"), + Exporter: &ngfAPI.TelemetryExporter{ + Interval: helpers.GetPointer[ngfAPI.Duration]("5ms"), + Endpoint: "my-endpoint", + }, + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, }, }, }, diff --git a/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go b/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go index 8898251e51..294b7dc526 100644 --- a/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go +++ b/internal/mode/static/state/validation/validationfakes/fake_generic_validator.go @@ -8,6 +8,17 @@ import ( ) type FakeGenericValidator struct { + ValidateEndpointStub func(string) error + validateEndpointMutex sync.RWMutex + validateEndpointArgsForCall []struct { + arg1 string + } + validateEndpointReturns struct { + result1 error + } + validateEndpointReturnsOnCall map[int]struct { + result1 error + } ValidateEscapedStringNoVarExpansionStub func(string) error validateEscapedStringNoVarExpansionMutex sync.RWMutex validateEscapedStringNoVarExpansionArgsForCall []struct { @@ -19,10 +30,93 @@ type FakeGenericValidator struct { validateEscapedStringNoVarExpansionReturnsOnCall map[int]struct { result1 error } + ValidateNginxDurationStub func(string) error + validateNginxDurationMutex sync.RWMutex + validateNginxDurationArgsForCall []struct { + arg1 string + } + validateNginxDurationReturns struct { + result1 error + } + validateNginxDurationReturnsOnCall map[int]struct { + result1 error + } + ValidateServiceNameStub func(string) error + validateServiceNameMutex sync.RWMutex + validateServiceNameArgsForCall []struct { + arg1 string + } + validateServiceNameReturns struct { + result1 error + } + validateServiceNameReturnsOnCall map[int]struct { + result1 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } +func (fake *FakeGenericValidator) ValidateEndpoint(arg1 string) error { + fake.validateEndpointMutex.Lock() + ret, specificReturn := fake.validateEndpointReturnsOnCall[len(fake.validateEndpointArgsForCall)] + fake.validateEndpointArgsForCall = append(fake.validateEndpointArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateEndpointStub + fakeReturns := fake.validateEndpointReturns + fake.recordInvocation("ValidateEndpoint", []interface{}{arg1}) + fake.validateEndpointMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateEndpointCallCount() int { + fake.validateEndpointMutex.RLock() + defer fake.validateEndpointMutex.RUnlock() + return len(fake.validateEndpointArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateEndpointCalls(stub func(string) error) { + fake.validateEndpointMutex.Lock() + defer fake.validateEndpointMutex.Unlock() + fake.ValidateEndpointStub = stub +} + +func (fake *FakeGenericValidator) ValidateEndpointArgsForCall(i int) string { + fake.validateEndpointMutex.RLock() + defer fake.validateEndpointMutex.RUnlock() + argsForCall := fake.validateEndpointArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenericValidator) ValidateEndpointReturns(result1 error) { + fake.validateEndpointMutex.Lock() + defer fake.validateEndpointMutex.Unlock() + fake.ValidateEndpointStub = nil + fake.validateEndpointReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateEndpointReturnsOnCall(i int, result1 error) { + fake.validateEndpointMutex.Lock() + defer fake.validateEndpointMutex.Unlock() + fake.ValidateEndpointStub = nil + if fake.validateEndpointReturnsOnCall == nil { + fake.validateEndpointReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateEndpointReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansion(arg1 string) error { fake.validateEscapedStringNoVarExpansionMutex.Lock() ret, specificReturn := fake.validateEscapedStringNoVarExpansionReturnsOnCall[len(fake.validateEscapedStringNoVarExpansionArgsForCall)] @@ -84,11 +178,139 @@ func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionReturnsOnCa }{result1} } +func (fake *FakeGenericValidator) ValidateNginxDuration(arg1 string) error { + fake.validateNginxDurationMutex.Lock() + ret, specificReturn := fake.validateNginxDurationReturnsOnCall[len(fake.validateNginxDurationArgsForCall)] + fake.validateNginxDurationArgsForCall = append(fake.validateNginxDurationArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateNginxDurationStub + fakeReturns := fake.validateNginxDurationReturns + fake.recordInvocation("ValidateNginxDuration", []interface{}{arg1}) + fake.validateNginxDurationMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateNginxDurationCallCount() int { + fake.validateNginxDurationMutex.RLock() + defer fake.validateNginxDurationMutex.RUnlock() + return len(fake.validateNginxDurationArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateNginxDurationCalls(stub func(string) error) { + fake.validateNginxDurationMutex.Lock() + defer fake.validateNginxDurationMutex.Unlock() + fake.ValidateNginxDurationStub = stub +} + +func (fake *FakeGenericValidator) ValidateNginxDurationArgsForCall(i int) string { + fake.validateNginxDurationMutex.RLock() + defer fake.validateNginxDurationMutex.RUnlock() + argsForCall := fake.validateNginxDurationArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenericValidator) ValidateNginxDurationReturns(result1 error) { + fake.validateNginxDurationMutex.Lock() + defer fake.validateNginxDurationMutex.Unlock() + fake.ValidateNginxDurationStub = nil + fake.validateNginxDurationReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateNginxDurationReturnsOnCall(i int, result1 error) { + fake.validateNginxDurationMutex.Lock() + defer fake.validateNginxDurationMutex.Unlock() + fake.ValidateNginxDurationStub = nil + if fake.validateNginxDurationReturnsOnCall == nil { + fake.validateNginxDurationReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateNginxDurationReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateServiceName(arg1 string) error { + fake.validateServiceNameMutex.Lock() + ret, specificReturn := fake.validateServiceNameReturnsOnCall[len(fake.validateServiceNameArgsForCall)] + fake.validateServiceNameArgsForCall = append(fake.validateServiceNameArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateServiceNameStub + fakeReturns := fake.validateServiceNameReturns + fake.recordInvocation("ValidateServiceName", []interface{}{arg1}) + fake.validateServiceNameMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateServiceNameCallCount() int { + fake.validateServiceNameMutex.RLock() + defer fake.validateServiceNameMutex.RUnlock() + return len(fake.validateServiceNameArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateServiceNameCalls(stub func(string) error) { + fake.validateServiceNameMutex.Lock() + defer fake.validateServiceNameMutex.Unlock() + fake.ValidateServiceNameStub = stub +} + +func (fake *FakeGenericValidator) ValidateServiceNameArgsForCall(i int) string { + fake.validateServiceNameMutex.RLock() + defer fake.validateServiceNameMutex.RUnlock() + argsForCall := fake.validateServiceNameArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenericValidator) ValidateServiceNameReturns(result1 error) { + fake.validateServiceNameMutex.Lock() + defer fake.validateServiceNameMutex.Unlock() + fake.ValidateServiceNameStub = nil + fake.validateServiceNameReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateServiceNameReturnsOnCall(i int, result1 error) { + fake.validateServiceNameMutex.Lock() + defer fake.validateServiceNameMutex.Unlock() + fake.ValidateServiceNameStub = nil + if fake.validateServiceNameReturnsOnCall == nil { + fake.validateServiceNameReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateServiceNameReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeGenericValidator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.validateEndpointMutex.RLock() + defer fake.validateEndpointMutex.RUnlock() fake.validateEscapedStringNoVarExpansionMutex.RLock() defer fake.validateEscapedStringNoVarExpansionMutex.RUnlock() + fake.validateNginxDurationMutex.RLock() + defer fake.validateNginxDurationMutex.RUnlock() + fake.validateServiceNameMutex.RLock() + defer fake.validateServiceNameMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/mode/static/state/validation/validator.go b/internal/mode/static/state/validation/validator.go index 86a9f302b2..52e20bb47f 100644 --- a/internal/mode/static/state/validation/validator.go +++ b/internal/mode/static/state/validation/validator.go @@ -35,4 +35,7 @@ type HTTPFieldsValidator interface { //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . GenericValidator type GenericValidator interface { ValidateEscapedStringNoVarExpansion(value string) error + ValidateServiceName(name string) error + ValidateNginxDuration(duration string) error + ValidateEndpoint(endpoint string) error } From 5ef075dae1fa464ecd04235a3df5f3cd6b2d0dda Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 09:25:22 -0600 Subject: [PATCH 11/15] Rebase and add test --- .github/workflows/ci.yml | 2 +- .goreleaser.yml | 2 - .../static/state/change_processor_test.go | 42 +++++++++++++------ 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e5c84b88c..3a158f1bad 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,7 +172,7 @@ jobs: fail-fast: false matrix: image: [ngf, nginx] - platforms: ["linux/arm64, linux/amd64, linux/s390x, linux/ppc64le"] + platforms: ["linux/arm64, linux/amd64"] uses: ./.github/workflows/build.yml with: image: ${{ matrix.image }} diff --git a/.goreleaser.yml b/.goreleaser.yml index 86c1d5671b..117e2ac3e4 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -8,8 +8,6 @@ builds: goarch: - amd64 - arm64 - - s390x - - ppc64le flags: - -trimpath gcflags: diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index dbb0659bb7..aeb8d9e0bc 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -1586,18 +1586,19 @@ var _ = Describe("ChangeProcessor", func() { //nolint:lll var ( - processor *state.ChangeProcessorImpl - gcNsName, gwNsName, hrNsName, hr2NsName, rgNsName, svcNsName, sliceNsName, secretNsName, cmNsName, btlsNsName types.NamespacedName - gc, gcUpdated *v1.GatewayClass - gw1, gw1Updated, gw2 *v1.Gateway - hr1, hr1Updated, hr2 *v1.HTTPRoute - rg1, rg1Updated, rg2 *v1beta1.ReferenceGrant - svc, barSvc, unrelatedSvc *apiv1.Service - slice, barSlice, unrelatedSlice *discoveryV1.EndpointSlice - ns, unrelatedNS, testNs, barNs *apiv1.Namespace - secret, secretUpdated, unrelatedSecret, barSecret, barSecretUpdated *apiv1.Secret - cm, cmUpdated, unrelatedCM *apiv1.ConfigMap - btls, btlsUpdated *v1alpha2.BackendTLSPolicy + processor *state.ChangeProcessorImpl + gcNsName, gwNsName, hrNsName, hr2NsName, rgNsName, svcNsName, sliceNsName, secretNsName, cmNsName, btlsNsName, npNsName types.NamespacedName + gc, gcUpdated *v1.GatewayClass + gw1, gw1Updated, gw2 *v1.Gateway + hr1, hr1Updated, hr2 *v1.HTTPRoute + rg1, rg1Updated, rg2 *v1beta1.ReferenceGrant + svc, barSvc, unrelatedSvc *apiv1.Service + slice, barSlice, unrelatedSlice *discoveryV1.EndpointSlice + ns, unrelatedNS, testNs, barNs *apiv1.Namespace + secret, secretUpdated, unrelatedSecret, barSecret, barSecretUpdated *apiv1.Secret + cm, cmUpdated, unrelatedCM *apiv1.ConfigMap + btls, btlsUpdated *v1alpha2.BackendTLSPolicy + np, npUpdated *ngfAPI.NginxProxy ) BeforeEach(OncePerOrdered, func() { @@ -1889,6 +1890,19 @@ var _ = Describe("ChangeProcessor", func() { }, } btlsUpdated = btls.DeepCopy() + + npNsName = types.NamespacedName{Name: "np-1"} + np = &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: npNsName.Name, + }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), + }, + }, + } + npUpdated = np.DeepCopy() }) // Changing change - a change that makes processor.Process() report changed // Non-changing change - a change that doesn't do that @@ -1906,6 +1920,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(rg1) processor.CaptureUpsertChange(btls) processor.CaptureUpsertChange(cm) + processor.CaptureUpsertChange(np) changed, _ := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) @@ -1919,6 +1934,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(rg1Updated) processor.CaptureUpsertChange(btlsUpdated) processor.CaptureUpsertChange(cmUpdated) + processor.CaptureUpsertChange(npUpdated) // there are non-changing changes processor.CaptureUpsertChange(gcUpdated) @@ -1927,6 +1943,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(rg1Updated) processor.CaptureUpsertChange(btlsUpdated) processor.CaptureUpsertChange(cmUpdated) + processor.CaptureUpsertChange(npUpdated) changed, _ := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) @@ -1950,6 +1967,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureDeleteChange(&v1beta1.ReferenceGrant{}, rgNsName) processor.CaptureDeleteChange(&v1alpha2.BackendTLSPolicy{}, btlsNsName) processor.CaptureDeleteChange(&apiv1.ConfigMap{}, cmNsName) + processor.CaptureDeleteChange(&ngfAPI.NginxProxy{}, npNsName) // these are non-changing changes processor.CaptureUpsertChange(gw2) From 43e79d8430eb57f4425c0ccade9cc35a3dc04bf6 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 09:33:07 -0600 Subject: [PATCH 12/15] Fix validation calls --- internal/mode/static/state/graph/nginxproxy.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index 7088d0c4bb..327d6cca89 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -48,7 +48,7 @@ func validateNginxProxy( if telemetry != nil { telPath := spec.Child("telemetry") if telemetry.ServiceName != nil { - if err := validator.ValidateEscapedStringNoVarExpansion(*telemetry.ServiceName); err != nil { + if err := validator.ValidateServiceName(*telemetry.ServiceName); err != nil { allErrs = append(allErrs, field.Invalid(telPath.Child("serviceName"), *telemetry.ServiceName, err.Error())) } } @@ -58,13 +58,13 @@ func validateNginxProxy( expPath := telPath.Child("exporter") if exp.Endpoint != "" { - if err := validator.ValidateEscapedStringNoVarExpansion(exp.Endpoint); err != nil { + if err := validator.ValidateEndpoint(exp.Endpoint); err != nil { allErrs = append(allErrs, field.Invalid(expPath.Child("endpoint"), exp.Endpoint, err.Error())) } } if exp.Interval != nil { - if err := validator.ValidateEscapedStringNoVarExpansion(string(*exp.Interval)); err != nil { + if err := validator.ValidateNginxDuration(string(*exp.Interval)); err != nil { allErrs = append(allErrs, field.Invalid(expPath.Child("interval"), *exp.Interval, err.Error())) } } From 80818786d8c4ede01e5abaa128e10bef3b2b143a Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 09:41:59 -0600 Subject: [PATCH 13/15] Fix failing unit test --- internal/mode/static/state/graph/gatewayclass_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index b90ac0fb54..a60da7eecc 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -167,14 +167,16 @@ func TestBuildGatewayClass(t *testing.T) { createValidNPValidator := func() *validationfakes.FakeGenericValidator { v := &validationfakes.FakeGenericValidator{} - v.ValidateEscapedStringNoVarExpansionReturns(nil) + v.ValidateServiceNameReturns(nil) + v.ValidateEndpointReturns(nil) return v } createInvalidNPValidator := func() *validationfakes.FakeGenericValidator { v := &validationfakes.FakeGenericValidator{} - v.ValidateEscapedStringNoVarExpansionReturns(errors.New("error")) + v.ValidateServiceNameReturns(errors.New("error")) + v.ValidateEndpointReturns(errors.New("error")) return v } From 879fa7b143553d047acd82981025cc668c1133ab Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 12:31:25 -0600 Subject: [PATCH 14/15] Fix test after rebase --- internal/mode/static/nginx/config/generator_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 707f1ca862..f6bf1f9517 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -115,17 +115,12 @@ func TestGenerate(t *testing.T) { 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[3].Content) + certBundle := string(files[4].Content) g.Expect(certBundle).To(Equal("test-cert")) -<<<<<<< HEAD g.Expect(files[5]).To(Equal(file.File{ Type: file.TypeSecret, Path: "/etc/nginx/secrets/test-keypair.pem", Content: []byte("test-cert\ntest-key"), })) -======= - g.Expect(files[4].Path).To(Equal("/etc/nginx/module-includes/load-modules.conf")) - g.Expect(files[4].Content).To(Equal([]byte("load_module modules/ngx_otel_module.so;"))) ->>>>>>> dfc23a2 (Code review) } From 30df54e483c99ce32168f5ce6edb73f4f12182d8 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 29 Apr 2024 15:55:55 -0600 Subject: [PATCH 15/15] Code review --- internal/mode/static/nginx/config/generator_test.go | 6 +++--- .../mode/static/nginx/config/telemetry_template.go | 6 +++--- internal/mode/static/nginx/config/telemetry_test.go | 12 ++++++------ .../mode/static/nginx/config/validation/generic.go | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index f6bf1f9517..7f2bc0f7a4 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -99,11 +99,11 @@ func TestGenerate(t *testing.T) { g.Expect(httpCfg).To(ContainSubstring("upstream")) g.Expect(httpCfg).To(ContainSubstring("split_clients")) - g.Expect(httpCfg).To(ContainSubstring(`endpoint "1.2.3.4:123";`)) - g.Expect(httpCfg).To(ContainSubstring(`interval "5s";`)) + g.Expect(httpCfg).To(ContainSubstring("endpoint 1.2.3.4:123;")) + g.Expect(httpCfg).To(ContainSubstring("interval 5s;")) g.Expect(httpCfg).To(ContainSubstring("batch_size 512;")) 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("otel_service_name ngf:gw-ns:gw-name:my-name;")) g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/matches.json")) diff --git a/internal/mode/static/nginx/config/telemetry_template.go b/internal/mode/static/nginx/config/telemetry_template.go index 6fcf297642..5152075835 100644 --- a/internal/mode/static/nginx/config/telemetry_template.go +++ b/internal/mode/static/nginx/config/telemetry_template.go @@ -2,9 +2,9 @@ package config const otelTemplateText = ` otel_exporter { - endpoint "{{ .Endpoint }}"; + endpoint {{ .Endpoint }}; {{- if .Interval }} - interval "{{ .Interval }}"; + interval {{ .Interval }}; {{- end }} {{- if .BatchSize }} batch_size {{ .BatchSize }}; @@ -14,7 +14,7 @@ otel_exporter { {{- end }} } -otel_service_name "{{ .ServiceName }}"; +otel_service_name {{ .ServiceName }}; {{- range $attr := .SpanAttributes }} otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; diff --git a/internal/mode/static/nginx/config/telemetry_test.go b/internal/mode/static/nginx/config/telemetry_test.go index ddaf0d5814..27226cfbaa 100644 --- a/internal/mode/static/nginx/config/telemetry_test.go +++ b/internal/mode/static/nginx/config/telemetry_test.go @@ -32,12 +32,12 @@ func TestExecuteTelemetry(t *testing.T) { g := NewWithT(t) expSubStrings := map[string]int{ - `endpoint "1.2.3.4:123";`: 1, - `otel_service_name "ngf:gw-ns:gw-name:my-name";`: 1, - `interval "5s";`: 1, - "batch_size 512;": 1, - "batch_count 4;": 1, - "otel_span_attr": 2, + "endpoint 1.2.3.4:123;": 1, + "otel_service_name ngf:gw-ns:gw-name:my-name;": 1, + "interval 5s;": 1, + "batch_size 512;": 1, + "batch_count 4;": 1, + "otel_span_attr": 2, } for expSubStr, expCount := range expSubStrings { diff --git a/internal/mode/static/nginx/config/validation/generic.go b/internal/mode/static/nginx/config/validation/generic.go index 5568650f25..bac2ecf827 100644 --- a/internal/mode/static/nginx/config/validation/generic.go +++ b/internal/mode/static/nginx/config/validation/generic.go @@ -23,7 +23,7 @@ const ( var alphaNumericStringFmtRegexp = regexp.MustCompile("^" + alphaNumericStringFmt + "$") -// ValidateServiceName validates a k8s service name that can only use alphanumeric characters. +// ValidateServiceName validates a service name that can only use alphanumeric characters. func (GenericValidator) ValidateServiceName(name string) error { if !alphaNumericStringFmtRegexp.MatchString(name) { examples := []string{