From 57e50eb60f5630841b9e74e7999fa2c30a36cee6 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 28 Jul 2025 15:31:59 -0700 Subject: [PATCH 01/15] WIP just need a couple more tests --- charts/nginx-gateway-fabric/README.md | 2 +- .../templates/deployment.yaml | 12 +++ .../nginx-gateway-fabric/values.schema.json | 36 +++++++ charts/nginx-gateway-fabric/values.yaml | 14 +++ cmd/gateway/commands.go | 101 ++++++++++++------ cmd/gateway/commands_test.go | 73 +++++++++++-- cmd/gateway/validation.go | 8 ++ cmd/gateway/validation_test.go | 39 +++++++ internal/controller/config/config.go | 14 +++ internal/controller/manager.go | 16 +++ internal/controller/provisioner/objects.go | 56 +++++++++- .../controller/provisioner/provisioner.go | 4 + .../provisioner/provisioner_test.go | 15 +++ internal/controller/provisioner/templates.go | 31 ++++-- internal/controller/telemetry/agent_labels.go | 82 ++++++++++++++ 15 files changed, 452 insertions(+), 51 deletions(-) create mode 100644 internal/controller/telemetry/agent_labels.go diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index 3bcafee80e..e4f24a93ab 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -264,7 +264,7 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `certGenerator.ttlSecondsAfterFinished` | How long to wait after the cert generator job has finished before it is removed by the job controller. | int | `30` | | `clusterDomain` | The DNS cluster domain of your Kubernetes cluster. | string | `"cluster.local"` | | `gateways` | A list of Gateway objects. View https://gateway-api.sigs.k8s.io/reference/spec/#gateway for full Gateway reference. | list | `[]` | -| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | +| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","nginxOneConsole":{"dataplaneKeySecretName":"","endpointHost":"product.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false},"plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | | `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]}` | | `nginx.container.hostPorts` | A list of HostPorts to expose on the host. This configuration allows containers to bind to a specific port on the host node, enabling external network traffic to reach the container directly through the host's IP address and port. Use this option when you need to expose container ports on the host for direct access, such as for debugging, legacy integrations, or when NodePort/LoadBalancer services are not suitable. Note: Using hostPort may have security and scheduling implications, as it ties pods to specific nodes and ports. | list | `[]` | diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index 6b34a7e97c..e20f8b3dd8 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -103,6 +103,18 @@ spec: {{- if .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" }} - --nginx-scc={{ include "nginx-gateway.scc-name" . }}-nginx {{- end}} + {{- if .Values.nginx.nginxOneConsole.dataplaneKeySecretName }} + - --nginx-one-console-dataplane-key-secret={{ .Values.nginx.nginxOneConsole.dataplaneKeySecretName }} + {{- if .Values.nginx.nginxOneConsole.endpointHost }} + - --nginx-one-console-telemetry-endpoint-host={{ .Values.nginx.nginxOneConsole.endpointHost }} + {{- end }} + {{- if .Values.nginx.nginxOneConsole.endpointPort }} + - --nginx-one-console-telemetry-endpoint-port={{ .Values.nginx.nginxOneConsole.endpointPort }} + {{- end }} + {{- if .Values.nginx.nginxOneConsole.tlsSkipVerify }} + - --nginx-one-console-tls-skip-verify + {{- end }} + {{- end }} env: - name: POD_NAMESPACE valueFrom: diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index 88edebfdbf..49bcd0be0f 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -445,6 +445,42 @@ "required": [], "title": "kind" }, + "nginxOneConsole": { + "description": "Configuration for NGINX One Console.", + "properties": { + "dataplaneKeySecretName": { + "default": "", + "description": "Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console.", + "required": [], + "title": "dataplaneKeySecretName", + "type": "string" + }, + "endpointHost": { + "default": "product.connect.nginx.com", + "description": "The Endpoint host that the NGINX One Console telemetry metrics will be sent to. ", + "required": [], + "title": "endpointHost", + "type": "string" + }, + "endpointPort": { + "default": 443, + "description": "The endpoint port that the NGINX One Console telemetry metrics will be sent to.", + "required": [], + "title": "endpointPort", + "type": "integer" + }, + "tlsSkipVerify": { + "default": false, + "description": "NGINX One Console configuration specifying tls skip verify.", + "required": [], + "title": "tlsSkipVerify", + "type": "boolean" + } + }, + "required": [], + "title": "nginxOneConsole", + "type": "object" + }, "plus": { "default": false, "description": "Is NGINX Plus image being used.", diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index 361a507616..13b9b8dec4 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -212,6 +212,20 @@ nginx: # -- Is NGINX Plus image being used. plus: false + # Configuration for NGINX One Console. + nginxOneConsole: + # Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console. + dataplaneKeySecretName: "" + + # The Endpoint host that the NGINX One Console telemetry metrics will be sent to. + endpointHost: "product.connect.nginx.com" + + # The endpoint port that the NGINX One Console telemetry metrics will be sent to. + endpointPort: 443 + + # NGINX One Console configuration specifying tls skip verify. + tlsSkipVerify: false + # -- The name of the secret containing docker registry credentials. # Secret must exist in the same namespace as the helm release. The control # plane will copy this secret into any namespace where NGINX is deployed. diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 37a3000631..d828f943aa 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -6,7 +6,6 @@ import ( "os" "runtime/debug" "strconv" - "strings" "time" "github.com/spf13/cobra" @@ -58,27 +57,31 @@ func createRootCommand() *cobra.Command { func createControllerCommand() *cobra.Command { // flag names const ( - configFlag = "config" - serviceFlag = "service" - agentTLSSecretFlag = "agent-tls-secret" - metricsDisableFlag = "metrics-disable" - metricsSecureFlag = "metrics-secure-serving" - metricsPortFlag = "metrics-port" - healthDisableFlag = "health-disable" - healthPortFlag = "health-port" - leaderElectionDisableFlag = "leader-election-disable" - leaderElectionLockNameFlag = "leader-election-lock-name" - productTelemetryDisableFlag = "product-telemetry-disable" - gwAPIExperimentalFlag = "gateway-api-experimental-features" - nginxDockerSecretFlag = "nginx-docker-secret" //nolint:gosec // not credentials - usageReportSecretFlag = "usage-report-secret" - usageReportEndpointFlag = "usage-report-endpoint" - usageReportResolverFlag = "usage-report-resolver" - usageReportSkipVerifyFlag = "usage-report-skip-verify" - usageReportClientSSLSecretFlag = "usage-report-client-ssl-secret" //nolint:gosec // not credentials - usageReportCASecretFlag = "usage-report-ca-secret" //nolint:gosec // not credentials - snippetsFiltersFlag = "snippets-filters" - nginxSCCFlag = "nginx-scc" + configFlag = "config" + serviceFlag = "service" + agentTLSSecretFlag = "agent-tls-secret" + nginxOneConsoleDataplaneKeySecretFlag = "nginx-one-console-dataplane-key-secret" + nginxOneConsoleTelemetryEndpointHostFlag = "nginx-one-console-telemetry-endpoint-host" + nginxOneConsoleTelemetryEndpointPortFlag = "nginx-one-console-telemetry-endpoint-port" + nginxOneConsoleTLSSkipVerifyFlag = "nginx-one-console-tls-skip-verify" + metricsDisableFlag = "metrics-disable" + metricsSecureFlag = "metrics-secure-serving" + metricsPortFlag = "metrics-port" + healthDisableFlag = "health-disable" + healthPortFlag = "health-port" + leaderElectionDisableFlag = "leader-election-disable" + leaderElectionLockNameFlag = "leader-election-lock-name" + productTelemetryDisableFlag = "product-telemetry-disable" + gwAPIExperimentalFlag = "gateway-api-experimental-features" + nginxDockerSecretFlag = "nginx-docker-secret" //nolint:gosec // not credentials + usageReportSecretFlag = "usage-report-secret" + usageReportEndpointFlag = "usage-report-endpoint" + usageReportResolverFlag = "usage-report-resolver" + usageReportSkipVerifyFlag = "usage-report-skip-verify" + usageReportClientSSLSecretFlag = "usage-report-client-ssl-secret" //nolint:gosec // not credentials + usageReportCASecretFlag = "usage-report-ca-secret" //nolint:gosec // not credentials + snippetsFiltersFlag = "snippets-filters" + nginxSCCFlag = "nginx-scc" ) // flag values @@ -101,7 +104,19 @@ func createControllerCommand() *cobra.Command { validator: validateResourceName, value: agentTLSSecret, } - nginxSCCName = stringValidatingValue{ + nginxOneConsoleDataplaneKeySecretName = stringValidatingValue{ + validator: validateResourceName, + } + nginxOneConsoleTelemetryEndpointHost = stringValidatingValue{ + validator: validateResourceName, + value: "product.connect.nginx.com", + } + nginxOneConsoleTelemetryEndpointPort = intValidatingValue{ + validator: validateProtocolPort, + value: 443, + } + nginxOneConsoleTLSSkipVerify bool + nginxSCCName = stringValidatingValue{ validator: validateResourceName, } disableMetrics bool @@ -257,6 +272,12 @@ func createControllerCommand() *cobra.Command { NginxDockerSecretNames: nginxDockerSecrets.values, AgentTLSSecretName: agentTLSSecretName.value, NGINXSCCName: nginxSCCName.value, + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: nginxOneConsoleDataplaneKeySecretName.value, + EndpointHost: nginxOneConsoleTelemetryEndpointHost.value, + EndpointPort: nginxOneConsoleTelemetryEndpointPort.value, + EndpointTLSSkipVerify: nginxOneConsoleTLSSkipVerify, + }, } if err := controller.StartManager(conf); err != nil { @@ -304,6 +325,32 @@ func createControllerCommand() *cobra.Command { `NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).`, ) + cmd.Flags().Var( + &nginxOneConsoleDataplaneKeySecretName, + nginxOneConsoleDataplaneKeySecretFlag, + `The name of the Secret containing the NGINX One Console's dataplane key. Must exist in the same namespace that `+ + `the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).`, + ) + + cmd.Flags().Var( + &nginxOneConsoleTelemetryEndpointHost, + nginxOneConsoleTelemetryEndpointHostFlag, + `The host of the NGINX One Console's telemetry endpoint.`, + ) + + cmd.Flags().Var( + &nginxOneConsoleTelemetryEndpointPort, + nginxOneConsoleTelemetryEndpointPortFlag, + `The port of the NGINX One Console's telemetry endpoint.`, + ) + + cmd.Flags().BoolVar( + &nginxOneConsoleTLSSkipVerify, + nginxOneConsoleTLSSkipVerifyFlag, + false, + "Disable client verification of the NGINX One Console's telemetry endpoint server certificate.", + ) + cmd.Flags().BoolVar( &disableMetrics, metricsDisableFlag, @@ -741,19 +788,13 @@ func createGatewayPodConfig(version, svcName string) (config.GatewayPodConfig, e return config.GatewayPodConfig{}, err } - // use image tag version if set, otherwise fall back to binary version - ngfVersion := version - if imageParts := strings.Split(image, ":"); len(imageParts) == 2 { - ngfVersion = imageParts[1] - } - c := config.GatewayPodConfig{ ServiceName: svcName, Namespace: ns, Name: name, UID: podUID, InstanceName: instance, - Version: ngfVersion, + Version: version, Image: image, } diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index ba65d2fa49..0460de8038 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -156,6 +156,10 @@ func TestControllerCmdFlagValidation(t *testing.T) { "--usage-report-client-ssl-secret=client-secret", "--snippets-filters", "--nginx-scc=nginx-sscc-name", + "--nginx-one-console-dataplane-key-secret=dataplane-key-secret", + "--nginx-one-console-telemetry-endpoint-host=telemetry-endpoint-host", + "--nginx-one-console-telemetry-endpoint-port=443", + "--nginx-one-console-tls-skip-verify", }, wantErr: false, }, @@ -426,6 +430,65 @@ func TestControllerCmdFlagValidation(t *testing.T) { wantErr: true, expectedErrPrefix: `invalid argument "!@#$" for "--nginx-scc" flag: invalid format: `, }, + { + name: "nginx-one-console-dataplane-key-secret is set to empty string", + args: []string{ + "--nginx-one-console-dataplane-key-secret=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--nginx-one-console-dataplane-key-secret" flag: must be set`, + }, + { + name: "nginx-one-console-dataplane-key-secret is invalid", + args: []string{ + "--nginx-one-console-dataplane-key-secret=!@#$", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-dataplane-key-secret" flag: invalid format: `, + }, + { + name: "nginx-one-console-telemetry-endpoint-host is set to empty string", + args: []string{ + "--nginx-one-console-telemetry-endpoint-host=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--nginx-one-console-telemetry-endpoint-host" flag: must be set`, + }, + { + name: "nginx-one-console-telemetry-endpoint-host is invalid", + args: []string{ + "--nginx-one-console-telemetry-endpoint-host=!@#$", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-telemetry-endpoint-host" flag: invalid format: `, + }, + { + name: "nginx-one-console-telemetry-endpoint-port is invalid type", + args: []string{ + "--nginx-one-console-telemetry-endpoint-port=invalid", // not an int + }, + wantErr: true, + expectedErrPrefix: `invalid argument "invalid" for "--nginx-one-console-telemetry-endpoint-port" flag: failed to parse int value:` + + ` strconv.ParseInt: parsing "invalid": invalid syntax`, + }, + { + name: "nginx-one-console-telemetry-endpoint-port is outside of range", + args: []string{ + "--nginx-one-console-telemetry-endpoint-port=65536", // outside of range + }, + wantErr: true, + expectedErrPrefix: `invalid argument "65536" for "--nginx-one-console-telemetry-endpoint-port" flag:` + + ` port outside of valid port range [1 - 65535]: 65536`, + }, + { + name: "nginx-one-console-tls-skip-verify is not a bool", + expectedErrPrefix: `invalid argument "not-a-bool" for "--nginx-one-console-tls-skip-verify" flag: strconv.ParseBool:` + + ` parsing "not-a-bool": invalid syntax`, + args: []string{ + "--nginx-one-console-tls-skip-verify=not-a-bool", + }, + wantErr: true, + }, } // common flags validation is tested separately @@ -753,21 +816,13 @@ func TestCreateGatewayPodConfig(t *testing.T) { Name: "my-pod", UID: "1234", InstanceName: "my-pod-xyz", - Version: "tag", + Version: "0.0.0", Image: "my-pod-image:tag", } cfg, err := createGatewayPodConfig(version, "svc") g.Expect(err).To(Not(HaveOccurred())) g.Expect(cfg).To(Equal(expCfg)) - // unset image tag and use provided version - g.Expect(os.Setenv("IMAGE_NAME", "my-pod-image")).To(Succeed()) - expCfg.Version = version - expCfg.Image = "my-pod-image" - cfg, err = createGatewayPodConfig(version, "svc") - g.Expect(err).To(Not(HaveOccurred())) - g.Expect(cfg).To(Equal(expCfg)) - // unset image name g.Expect(os.Unsetenv("IMAGE_NAME")).To(Succeed()) cfg, err = createGatewayPodConfig(version, "svc") diff --git a/cmd/gateway/validation.go b/cmd/gateway/validation.go index a953c522c1..9ba2deaa36 100644 --- a/cmd/gateway/validation.go +++ b/cmd/gateway/validation.go @@ -157,6 +157,14 @@ func validatePort(port int) error { return nil } +// validateProtocolPort makes sure a given port is inside the valid port range for its usage. This also includes well known ports. +func validateProtocolPort(port int) error { + if port < 1 || port > 65535 { + return fmt.Errorf("port outside of valid port range [1 - 65535]: %v", port) + } + return nil +} + // ensureNoPortCollisions checks if the same port has been defined multiple times. func ensureNoPortCollisions(ports ...int) error { seen := make(map[int]struct{}) diff --git a/cmd/gateway/validation_test.go b/cmd/gateway/validation_test.go index 665bd91582..a38be109fb 100644 --- a/cmd/gateway/validation_test.go +++ b/cmd/gateway/validation_test.go @@ -414,6 +414,45 @@ func TestValidatePort(t *testing.T) { } } +func TestProtocolPort(t *testing.T) { + t.Parallel() + tests := []struct { + name string + port int + expErr bool + }{ + { + name: "port under minimum allowed value", + port: 0, + expErr: true, + }, + { + name: "port over maximum allowed value", + port: 65536, + expErr: true, + }, + { + name: "valid port", + port: 443, + expErr: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + err := validateProtocolPort(tc.port) + if !tc.expErr { + g.Expect(err).ToNot(HaveOccurred()) + } else { + g.Expect(err).To(HaveOccurred()) + } + }) + } +} + func TestEnsureNoPortCollisions(t *testing.T) { t.Parallel() g := NewWithT(t) diff --git a/internal/controller/config/config.go b/internal/controller/config/config.go index 25630eb8f0..aace63c6c8 100644 --- a/internal/controller/config/config.go +++ b/internal/controller/config/config.go @@ -42,6 +42,8 @@ type Config struct { MetricsConfig MetricsConfig // HealthConfig specifies the health probe config. HealthConfig HealthConfig + // NginxOneConsoleTelemetryConfig contains the configuration for NGINX One Console telemetry. + NginxOneConsoleTelemetryConfig NginxOneConsoleTelemetryConfig // Plus indicates whether NGINX Plus is being used. Plus bool // ExperimentalFeatures indicates if experimental features are enabled. @@ -134,3 +136,15 @@ type Flags struct { // Each Value will be either true or false for boolean flags and default or user-defined for non-boolean flags. Values []string } + +// NginxOneConsoleTelemetryConfig contains the configuration for NGINX One Console telemetry. +type NginxOneConsoleTelemetryConfig struct { + // DataplaneKeySecretName is the name of the Secret containing the dataplane key. + DataplaneKeySecretName string + // EndpointHost is the host of the telemetry endpoint. + EndpointHost string + // EndpointPort is the port of the telemetry endpoint. + EndpointPort int + // EndpointTLSSkipVerify specifies whether to skip TLS verification for the telemetry endpoint. + EndpointTLSSkipVerify bool +} diff --git a/internal/controller/manager.go b/internal/controller/manager.go index bb95a3660f..eb86673b71 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -200,6 +200,20 @@ func StartManager(cfg config.Config) error { return fmt.Errorf("cannot register grpc server: %w", err) } + agentLabelCollector := telemetry.NewLabelCollectorConfigImpl(telemetry.LabelCollectorConfig{ + K8sClientReader: mgr.GetAPIReader(), + Version: cfg.GatewayPodConfig.Version, + PodNSName: types.NamespacedName{ + Namespace: cfg.GatewayPodConfig.Namespace, + Name: cfg.GatewayPodConfig.Name, + }, + }) + + agentLabels, err := agentLabelCollector.Collect(ctx) + if err != nil { + return fmt.Errorf("failed to collect agent labels: %w", err) + } + nginxProvisioner, provLoop, err := provisioner.NewNginxProvisioner( ctx, mgr, @@ -215,6 +229,8 @@ func StartManager(cfg config.Config) error { Plus: cfg.Plus, NginxDockerSecretNames: cfg.NginxDockerSecretNames, PlusUsageConfig: &cfg.UsageReportConfig, + AgentLabels: agentLabels, + NginxOneConsoleTelemetryConfig: cfg.NginxOneConsoleTelemetryConfig, }, ) if err != nil { diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 47cd221256..40d9f9276e 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -25,6 +25,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/config" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/dataplane" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -71,6 +72,11 @@ func (p *NginxProvisioner) buildNginxResourceObjects( } } + var dataplaneKeySecretName string + if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { + dataplaneKeySecretName = controller.CreateNginxResourceName(resourceName, p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName) + } + // map key is the new name, value is the original name dockerSecretNames := make(map[string]string) for _, name := range p.cfg.NginxDockerSecretNames { @@ -112,6 +118,7 @@ func (p *NginxProvisioner) buildNginxResourceObjects( jwtSecretName, caSecretName, clientSSLSecretName, + dataplaneKeySecretName, ) if err != nil { errs = append(errs, err) @@ -158,6 +165,7 @@ func (p *NginxProvisioner) buildNginxResourceObjects( jwtSecretName, caSecretName, clientSSLSecretName, + dataplaneKeySecretName, ) if err != nil { errs = append(errs, err) @@ -190,6 +198,7 @@ func (p *NginxProvisioner) buildNginxSecrets( jwtSecretName string, caSecretName string, clientSSLSecretName string, + dataplaneKeySecretName string, ) ([]client.Object, error) { var secrets []client.Object var errs []error @@ -290,6 +299,24 @@ func (p *NginxProvisioner) buildNginxSecrets( } } + if dataplaneKeySecretName != "" { + newSecret, err := p.getAndUpdateSecret( + p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, + metav1.ObjectMeta{ + Name: dataplaneKeySecretName, + Namespace: objectMeta.Namespace, + Labels: objectMeta.Labels, + Annotations: objectMeta.Annotations, + }, + corev1.SecretTypeOpaque, + ) + if err != nil { + errs = append(errs, err) + } else { + secrets = append(secrets, newSecret) + } + } + return secrets, errors.Join(errs...) } @@ -379,10 +406,18 @@ func (p *NginxProvisioner) buildNginxConfigMaps( "Namespace": p.cfg.GatewayPodConfig.Namespace, "EnableMetrics": enableMetrics, "MetricsPort": metricsPort, + "AgentLabels": telemetry.AgentLabelsToMap(p.cfg.AgentLabels), } if logging != nil && logging.AgentLevel != nil { - agentFields["LogLevel"] = *logging.AgentLevel + agentFields["LogLevel"] = logging.AgentLevel + } + + if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { + agentFields["DataplaneKeySecretName"] = p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName + agentFields["EndpointHost"] = p.cfg.NginxOneConsoleTelemetryConfig.EndpointHost + agentFields["EndpointPort"] = strconv.Itoa(p.cfg.NginxOneConsoleTelemetryConfig.EndpointPort) + agentFields["EndpointTLSSkipVerify"] = p.cfg.NginxOneConsoleTelemetryConfig.EndpointTLSSkipVerify } agentCM := &corev1.ConfigMap{ @@ -540,6 +575,7 @@ func (p *NginxProvisioner) buildNginxDeployment( jwtSecretName string, caSecretName string, clientSSLSecretName string, + dataplaneKeySecretName string, ) (client.Object, error) { podTemplateSpec := p.buildNginxPodTemplateSpec( objectMeta, @@ -552,6 +588,7 @@ func (p *NginxProvisioner) buildNginxDeployment( jwtSecretName, caSecretName, clientSSLSecretName, + dataplaneKeySecretName, ) if nProxyCfg != nil && nProxyCfg.Kubernetes != nil && nProxyCfg.Kubernetes.DaemonSet != nil { @@ -671,6 +708,7 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec( jwtSecretName string, caSecretName string, clientSSLSecretName string, + dataplaneKeySecretName string, ) corev1.PodTemplateSpec { containerPorts := make([]corev1.ContainerPort, 0, len(ports)) for port := range ports { @@ -983,6 +1021,22 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec( spec.Spec.Containers[0].VolumeMounts = volumeMounts } + if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { + volumeMounts := spec.Spec.Containers[0].VolumeMounts + + volumeMounts = append(volumeMounts, corev1.VolumeMount{ + Name: "agent-dataplane-key", + MountPath: "/etc/nginx-agent/secrets/dataplane.key", + SubPath: "dataplane.key", + }) + spec.Spec.Volumes = append(spec.Spec.Volumes, corev1.Volume{ + Name: "agent-dataplane-key", + VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: dataplaneKeySecretName}}, + }) + + spec.Spec.Containers[0].VolumeMounts = volumeMounts + } + return spec } diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 7cfb91f355..7e78a4ed20 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -28,6 +28,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/provisioner/openshift" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/status" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/events" ) @@ -55,6 +56,9 @@ type Config struct { Logger logr.Logger NginxDockerSecretNames []string + NginxOneConsoleTelemetryConfig config.NginxOneConsoleTelemetryConfig + AgentLabels telemetry.AgentLabels + Plus bool } diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index 07de8ff56c..0dce82d80b 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -24,6 +24,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/agentfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/provisioner/openshift/openshiftfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -183,6 +184,20 @@ func defaultNginxProvisioner( }, NginxDockerSecretNames: []string{dockerTestSecretName}, AgentTLSSecretName: agentTLSTestSecretName, + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: "dataplane-key", + EndpointHost: "product.connect.nginx.com", + EndpointPort: 443, + EndpointTLSSkipVerify: false, + }, + AgentLabels: telemetry.AgentLabels{ + ProductType: "ngf", + ProductVersion: "ngf-version", + ClusterID: "my-cluster-id", + ControlName: "my-control-plane-name", + ControlID: "my-control-plane-id", + ControlNamespace: "my-control-plane-namespace", + }, }, leader: true, }, fakeClient, deploymentStore diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index 9e557ffe90..c333a8ebbc 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -61,21 +61,32 @@ features: log: level: {{ .LogLevel }} {{- end }} +labels: + {{- range $key, $value := .AgentLabels }} + {{ $key }}: {{ $value }} + {{- end }} + +{{- if .DataplaneKeySecretName }} +auxiliary_command: + server: + host: {{ .EndpointHost }} + port: {{ .EndpointPort }} + type: grpc + auth: + tokenpath: /etc/nginx-agent/secrets/dataplane.key + tls: + skip_verify: {{ .EndpointTLSSkipVerify }} +{{- end }} {{- if .EnableMetrics }} collector: - receivers: - container_metrics: - collection_interval: 1m0s - host_metrics: - collection_interval: 1m0s - initial_delay: 1s - scrapers: - network: {} - processors: - batch: {} exporters: prometheus: server: host: "0.0.0.0" port: {{ .MetricsPort }} + pipelines: + metrics: + "ngf": + receivers: ["host_metrics", "nginx_metrics"] + exporters: ["prometheus"] {{- end }}` diff --git a/internal/controller/telemetry/agent_labels.go b/internal/controller/telemetry/agent_labels.go new file mode 100644 index 0000000000..2cab61c6dc --- /dev/null +++ b/internal/controller/telemetry/agent_labels.go @@ -0,0 +1,82 @@ +package telemetry + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// AgentLabels contains the metadata information needed for reporting to Agent v3. +type AgentLabels struct { + ProductType string `json:"product-type"` + ProductVersion string `json:"product-version"` + ClusterID string `json:"cluster-id"` + ControlName string `json:"control-name"` + ControlID string `json:"control-id"` + ControlNamespace string `json:"control-namespace"` +} + +// LabelCollectorConfig holds configuration parameters for LabelCollectorImpl. +type LabelCollectorConfig struct { + // K8sClientReader is a Kubernetes API client Reader. + K8sClientReader client.Reader + // Version is the NGF version. + Version string + // PodNSName is the NamespacedName of the NGF Pod. + PodNSName types.NamespacedName +} + +// LabelCollectorConfigImpl is an implementation of LabelCollectorConfig. +type LabelCollectorConfigImpl struct { + cfg LabelCollectorConfig +} + +// NewLabelCollectorConfigImpl creates a new LabelCollectorConfigImpl for a telemetry Job. +func NewLabelCollectorConfigImpl( + cfg LabelCollectorConfig, +) *LabelCollectorConfigImpl { + return &LabelCollectorConfigImpl{ + cfg: cfg, + } +} + +func (l *LabelCollectorConfigImpl) Collect(ctx context.Context) (AgentLabels, error) { + clusterID, err := collectClusterID(ctx, l.cfg.K8sClientReader) + if err != nil { + return AgentLabels{}, fmt.Errorf("failed to collect cluster information: %w", err) + } + + replicaSet, err := getPodReplicaSet(ctx, l.cfg.K8sClientReader, l.cfg.PodNSName) + if err != nil { + return AgentLabels{}, fmt.Errorf("failed to get replica set for pod %v: %w", l.cfg.PodNSName, err) + } + + deploymentID, err := getDeploymentID(replicaSet) + if err != nil { + return AgentLabels{}, fmt.Errorf("failed to get NGF deploymentID: %w", err) + } + + agentLabels := AgentLabels{ + ProductType: "ngf", + ProductVersion: l.cfg.Version, + ClusterID: clusterID, + ControlName: l.cfg.PodNSName.Name, + ControlNamespace: l.cfg.PodNSName.Namespace, + ControlID: deploymentID, + } + + return agentLabels, nil +} + +func AgentLabelsToMap(labels AgentLabels) map[string]string { + return map[string]string{ + "product-type": labels.ProductType, + "product-version": labels.ProductVersion, + "cluster-id": labels.ClusterID, + "control-name": labels.ControlName, + "control-namespace": labels.ControlNamespace, + "control-id": labels.ControlID, + } +} From 6e6902b5961115265e2fba38a7c5d62e0bf86329 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Thu, 31 Jul 2025 14:38:40 -0700 Subject: [PATCH 02/15] Mostly fix lint issues --- charts/nginx-gateway-fabric/README.md | 6 +++- .../nginx-gateway-fabric/values.schema.json | 10 +++--- charts/nginx-gateway-fabric/values.yaml | 12 ++++--- cmd/gateway/commands.go | 6 ++-- cmd/gateway/commands_test.go | 13 +++---- cmd/gateway/validation.go | 5 +-- cmd/gateway/validation_test.go | 2 +- internal/controller/config/config.go | 36 +++++++++---------- internal/controller/manager.go | 26 +++++++------- internal/controller/provisioner/objects.go | 5 ++- .../controller/provisioner/provisioner.go | 27 +++++++------- .../provisioner/provisioner_test.go | 2 +- 12 files changed, 79 insertions(+), 71 deletions(-) diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index e4f24a93ab..4408363566 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -264,7 +264,7 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `certGenerator.ttlSecondsAfterFinished` | How long to wait after the cert generator job has finished before it is removed by the job controller. | int | `30` | | `clusterDomain` | The DNS cluster domain of your Kubernetes cluster. | string | `"cluster.local"` | | `gateways` | A list of Gateway objects. View https://gateway-api.sigs.k8s.io/reference/spec/#gateway for full Gateway reference. | list | `[]` | -| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","nginxOneConsole":{"dataplaneKeySecretName":"","endpointHost":"product.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false},"plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | +| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","nginxOneConsole":{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false},"plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | | `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]}` | | `nginx.container.hostPorts` | A list of HostPorts to expose on the host. This configuration allows containers to bind to a specific port on the host node, enabling external network traffic to reach the container directly through the host's IP address and port. Use this option when you need to expose container ports on the host for direct access, such as for debugging, legacy integrations, or when NodePort/LoadBalancer services are not suitable. Note: Using hostPort may have security and scheduling implications, as it ties pods to specific nodes and ports. | list | `[]` | @@ -276,6 +276,10 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `nginx.imagePullSecret` | The name of the secret containing docker registry credentials. Secret must exist in the same namespace as the helm release. The control plane will copy this secret into any namespace where NGINX is deployed. | string | `""` | | `nginx.imagePullSecrets` | A list of secret names containing docker registry credentials. Secrets must exist in the same namespace as the helm release. The control plane will copy these secrets into any namespace where NGINX is deployed. | list | `[]` | | `nginx.kind` | The kind of NGINX deployment. | string | `"deployment"` | +| `nginx.nginxOneConsole` | Configuration for NGINX One Console. | object | `{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false}` | +| `nginx.nginxOneConsole.endpointHost` | The Endpoint host that the NGINX One Console telemetry metrics will be sent to. | string | `"agent.connect.nginx.com"` | +| `nginx.nginxOneConsole.endpointPort` | The endpoint port that the NGINX One Console telemetry metrics will be sent to. | int | `443` | +| `nginx.nginxOneConsole.tlsSkipVerify` | Skip TLS verification for NGINX One Console connections. | bool | `false` | | `nginx.plus` | Is NGINX Plus image being used. | bool | `false` | | `nginx.pod` | The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.replicas` | The number of replicas of the NGINX Deployment. | int | `1` | diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index 49bcd0be0f..82b85b5fb2 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -450,28 +450,28 @@ "properties": { "dataplaneKeySecretName": { "default": "", - "description": "Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console.", + "description": "Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console.\nSecret must exist in the same namespace that the NGINX Gateway Fabric control plane is running in\n(default namespace: nginx-gateway).", "required": [], "title": "dataplaneKeySecretName", "type": "string" }, "endpointHost": { - "default": "product.connect.nginx.com", - "description": "The Endpoint host that the NGINX One Console telemetry metrics will be sent to. ", + "default": "agent.connect.nginx.com", + "description": "The Endpoint host that the NGINX One Console telemetry metrics will be sent to.", "required": [], "title": "endpointHost", "type": "string" }, "endpointPort": { "default": 443, - "description": "The endpoint port that the NGINX One Console telemetry metrics will be sent to.", + "description": "The endpoint port that the NGINX One Console telemetry metrics will be sent to.", "required": [], "title": "endpointPort", "type": "integer" }, "tlsSkipVerify": { "default": false, - "description": "NGINX One Console configuration specifying tls skip verify.", + "description": "Skip TLS verification for NGINX One Console connections.", "required": [], "title": "tlsSkipVerify", "type": "boolean" diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index 13b9b8dec4..6f167842ae 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -212,18 +212,20 @@ nginx: # -- Is NGINX Plus image being used. plus: false - # Configuration for NGINX One Console. + # -- Configuration for NGINX One Console. nginxOneConsole: # Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console. + # Secret must exist in the same namespace that the NGINX Gateway Fabric control plane is running in + # (default namespace: nginx-gateway). dataplaneKeySecretName: "" - # The Endpoint host that the NGINX One Console telemetry metrics will be sent to. - endpointHost: "product.connect.nginx.com" + # -- The Endpoint host that the NGINX One Console telemetry metrics will be sent to. + endpointHost: "agent.connect.nginx.com" - # The endpoint port that the NGINX One Console telemetry metrics will be sent to. + # -- The endpoint port that the NGINX One Console telemetry metrics will be sent to. endpointPort: 443 - # NGINX One Console configuration specifying tls skip verify. + # -- Skip TLS verification for NGINX One Console connections. tlsSkipVerify: false # -- The name of the secret containing docker registry credentials. diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index d828f943aa..7b2ea6e335 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -60,7 +60,7 @@ func createControllerCommand() *cobra.Command { configFlag = "config" serviceFlag = "service" agentTLSSecretFlag = "agent-tls-secret" - nginxOneConsoleDataplaneKeySecretFlag = "nginx-one-console-dataplane-key-secret" + nginxOneConsoleDataplaneKeySecretFlag = "nginx-one-console-dataplane-key-secret" //nolint:gosec // not credentials nginxOneConsoleTelemetryEndpointHostFlag = "nginx-one-console-telemetry-endpoint-host" nginxOneConsoleTelemetryEndpointPortFlag = "nginx-one-console-telemetry-endpoint-port" nginxOneConsoleTLSSkipVerifyFlag = "nginx-one-console-tls-skip-verify" @@ -109,10 +109,10 @@ func createControllerCommand() *cobra.Command { } nginxOneConsoleTelemetryEndpointHost = stringValidatingValue{ validator: validateResourceName, - value: "product.connect.nginx.com", + value: "agent.connect.nginx.com", } nginxOneConsoleTelemetryEndpointPort = intValidatingValue{ - validator: validateProtocolPort, + validator: validateAnyPort, value: 443, } nginxOneConsoleTLSSkipVerify bool diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 0460de8038..99d51492d3 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -459,8 +459,9 @@ func TestControllerCmdFlagValidation(t *testing.T) { args: []string{ "--nginx-one-console-telemetry-endpoint-host=!@#$", }, - wantErr: true, - expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-telemetry-endpoint-host" flag: invalid format: `, + wantErr: true, + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-telemetry-endpoint-host" ` + + `flag: invalid format: `, }, { name: "nginx-one-console-telemetry-endpoint-port is invalid type", @@ -468,8 +469,8 @@ func TestControllerCmdFlagValidation(t *testing.T) { "--nginx-one-console-telemetry-endpoint-port=invalid", // not an int }, wantErr: true, - expectedErrPrefix: `invalid argument "invalid" for "--nginx-one-console-telemetry-endpoint-port" flag: failed to parse int value:` + - ` strconv.ParseInt: parsing "invalid": invalid syntax`, + expectedErrPrefix: `invalid argument "invalid" for "--nginx-one-console-telemetry-endpoint-port" ` + + `flag: failed to parse int value: strconv.ParseInt: parsing "invalid": invalid syntax`, }, { name: "nginx-one-console-telemetry-endpoint-port is outside of range", @@ -482,8 +483,8 @@ func TestControllerCmdFlagValidation(t *testing.T) { }, { name: "nginx-one-console-tls-skip-verify is not a bool", - expectedErrPrefix: `invalid argument "not-a-bool" for "--nginx-one-console-tls-skip-verify" flag: strconv.ParseBool:` + - ` parsing "not-a-bool": invalid syntax`, + expectedErrPrefix: `invalid argument "not-a-bool" for "--nginx-one-console-tls-skip-verify" flag:` + + `strconv.ParseBool: parsing "not-a-bool": invalid syntax`, args: []string{ "--nginx-one-console-tls-skip-verify=not-a-bool", }, diff --git a/cmd/gateway/validation.go b/cmd/gateway/validation.go index 9ba2deaa36..26b16d8bbd 100644 --- a/cmd/gateway/validation.go +++ b/cmd/gateway/validation.go @@ -157,8 +157,9 @@ func validatePort(port int) error { return nil } -// validateProtocolPort makes sure a given port is inside the valid port range for its usage. This also includes well known ports. -func validateProtocolPort(port int) error { +// validateAnyPort makes sure a given port is inside the valid range for all ports. +// This includes protected ports (1-1023) and unprivileged ports (1024-65535). +func validateAnyPort(port int) error { if port < 1 || port > 65535 { return fmt.Errorf("port outside of valid port range [1 - 65535]: %v", port) } diff --git a/cmd/gateway/validation_test.go b/cmd/gateway/validation_test.go index a38be109fb..ab5c94e67f 100644 --- a/cmd/gateway/validation_test.go +++ b/cmd/gateway/validation_test.go @@ -443,7 +443,7 @@ func TestProtocolPort(t *testing.T) { t.Parallel() g := NewWithT(t) - err := validateProtocolPort(tc.port) + err := validateAnyPort(tc.port) if !tc.expErr { g.Expect(err).ToNot(HaveOccurred()) } else { diff --git a/internal/controller/config/config.go b/internal/controller/config/config.go index aace63c6c8..ff6744b101 100644 --- a/internal/controller/config/config.go +++ b/internal/controller/config/config.go @@ -12,38 +12,38 @@ const DefaultNginxMetricsPort = int32(9113) type Config struct { // AtomicLevel is an atomically changeable, dynamic logging level. AtomicLevel zap.AtomicLevel - // UsageReportConfig specifies the NGINX Plus usage reporting configuration. - UsageReportConfig UsageReportConfig - // ImageSource is the source of the NGINX Gateway image. - ImageSource string - // Flags contains the NGF command-line flag names and values. - Flags Flags // GatewayPodConfig contains information about this Pod. GatewayPodConfig GatewayPodConfig // Logger is the Zap Logger used by all components. Logger logr.Logger - // GatewayCtlrName is the name of this controller. - GatewayCtlrName string + // NGINXSCCName is the name of the SecurityContextConstraints for the NGINX Pods. Only applicable in OpenShift. + NGINXSCCName string // ConfigName is the name of the NginxGateway resource for this controller. ConfigName string - // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. - GatewayClassName string // AgentTLSSecretName is the name of the TLS Secret used by NGINX Agent to communicate with the control plane. AgentTLSSecretName string - // NGINXSCCName is the name of the SecurityContextConstraints for the NGINX Pods. Only applicable in OpenShift. - NGINXSCCName string - // NginxDockerSecretNames are the names of any Docker registry Secrets for the NGINX container. - NginxDockerSecretNames []string + // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. + GatewayClassName string + // ImageSource is the source of the NGINX Gateway image. + ImageSource string + // GatewayCtlrName is the name of this controller. + GatewayCtlrName string + // UsageReportConfig specifies the NGINX Plus usage reporting configuration. + UsageReportConfig UsageReportConfig + // Flags contains the NGF command-line flag names and values. + Flags Flags // LeaderElection contains the configuration for leader election. LeaderElection LeaderElectionConfig + // NginxDockerSecretNames are the names of any Docker registry Secrets for the NGINX container. + NginxDockerSecretNames []string + // NginxOneConsoleTelemetryConfig contains the configuration for NGINX One Console telemetry. + NginxOneConsoleTelemetryConfig NginxOneConsoleTelemetryConfig // ProductTelemetryConfig contains the configuration for collecting product telemetry. ProductTelemetryConfig ProductTelemetryConfig - // MetricsConfig specifies the metrics config. - MetricsConfig MetricsConfig // HealthConfig specifies the health probe config. HealthConfig HealthConfig - // NginxOneConsoleTelemetryConfig contains the configuration for NGINX One Console telemetry. - NginxOneConsoleTelemetryConfig NginxOneConsoleTelemetryConfig + // MetricsConfig specifies the metrics config. + MetricsConfig MetricsConfig // Plus indicates whether NGINX Plus is being used. Plus bool // ExperimentalFeatures indicates if experimental features are enabled. diff --git a/internal/controller/manager.go b/internal/controller/manager.go index eb86673b71..c9a61cb1df 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -218,19 +218,19 @@ func StartManager(cfg config.Config) error { ctx, mgr, provisioner.Config{ - DeploymentStore: nginxUpdater.NginxDeployments, - StatusQueue: statusQueue, - Logger: cfg.Logger.WithName("provisioner"), - EventRecorder: recorder, - GatewayPodConfig: &cfg.GatewayPodConfig, - GCName: cfg.GatewayClassName, - AgentTLSSecretName: cfg.AgentTLSSecretName, - NGINXSCCName: cfg.NGINXSCCName, - Plus: cfg.Plus, - NginxDockerSecretNames: cfg.NginxDockerSecretNames, - PlusUsageConfig: &cfg.UsageReportConfig, - AgentLabels: agentLabels, - NginxOneConsoleTelemetryConfig: cfg.NginxOneConsoleTelemetryConfig, + DeploymentStore: nginxUpdater.NginxDeployments, + StatusQueue: statusQueue, + Logger: cfg.Logger.WithName("provisioner"), + EventRecorder: recorder, + GatewayPodConfig: &cfg.GatewayPodConfig, + GCName: cfg.GatewayClassName, + AgentTLSSecretName: cfg.AgentTLSSecretName, + NGINXSCCName: cfg.NGINXSCCName, + Plus: cfg.Plus, + NginxDockerSecretNames: cfg.NginxDockerSecretNames, + PlusUsageConfig: &cfg.UsageReportConfig, + AgentLabels: agentLabels, + NginxOneConsoleTelemetryConfig: cfg.NginxOneConsoleTelemetryConfig, }, ) if err != nil { diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 40d9f9276e..03906dbddf 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -74,7 +74,10 @@ func (p *NginxProvisioner) buildNginxResourceObjects( var dataplaneKeySecretName string if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { - dataplaneKeySecretName = controller.CreateNginxResourceName(resourceName, p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName) + dataplaneKeySecretName = controller.CreateNginxResourceName( + resourceName, + p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, + ) } // map key is the new name, value is the original name diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 7e78a4ed20..9700d784de 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -44,22 +44,19 @@ type Provisioner interface { // Config is the configuration for the Provisioner. type Config struct { - GCName string - AgentTLSSecretName string - NGINXSCCName string - - DeploymentStore agent.DeploymentStorer - StatusQueue *status.Queue - GatewayPodConfig *config.GatewayPodConfig - PlusUsageConfig *config.UsageReportConfig - EventRecorder record.EventRecorder - Logger logr.Logger - NginxDockerSecretNames []string - - NginxOneConsoleTelemetryConfig config.NginxOneConsoleTelemetryConfig + DeploymentStore agent.DeploymentStorer + EventRecorder record.EventRecorder + PlusUsageConfig *config.UsageReportConfig + StatusQueue *status.Queue + GatewayPodConfig *config.GatewayPodConfig AgentLabels telemetry.AgentLabels - - Plus bool + Logger logr.Logger + NGINXSCCName string + GCName string + AgentTLSSecretName string + NginxDockerSecretNames []string + NginxOneConsoleTelemetryConfig config.NginxOneConsoleTelemetryConfig + Plus bool } // NginxProvisioner handles provisioning nginx kubernetes resources. diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index 0dce82d80b..caa809bdd1 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -186,7 +186,7 @@ func defaultNginxProvisioner( AgentTLSSecretName: agentTLSTestSecretName, NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ DataplaneKeySecretName: "dataplane-key", - EndpointHost: "product.connect.nginx.com", + EndpointHost: "agent.connect.nginx.com", EndpointPort: 443, EndpointTLSSkipVerify: false, }, From 71e6cbdf775109dba0a82995f5fc4e3413d2c385 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Thu, 31 Jul 2025 15:11:05 -0700 Subject: [PATCH 03/15] Add small feedback --- .../templates/deployment.yaml | 8 +-- .../nginx-gateway-fabric/values.schema.json | 2 + charts/nginx-gateway-fabric/values.yaml | 5 ++ cmd/gateway/commands.go | 58 +++++++++---------- cmd/gateway/commands_test.go | 52 ++++++++--------- 5 files changed, 66 insertions(+), 59 deletions(-) diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index e20f8b3dd8..6f9aec008b 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -104,15 +104,15 @@ spec: - --nginx-scc={{ include "nginx-gateway.scc-name" . }}-nginx {{- end}} {{- if .Values.nginx.nginxOneConsole.dataplaneKeySecretName }} - - --nginx-one-console-dataplane-key-secret={{ .Values.nginx.nginxOneConsole.dataplaneKeySecretName }} + - --nginx-one-dataplane-key-secret={{ .Values.nginx.nginxOneConsole.dataplaneKeySecretName }} {{- if .Values.nginx.nginxOneConsole.endpointHost }} - - --nginx-one-console-telemetry-endpoint-host={{ .Values.nginx.nginxOneConsole.endpointHost }} + - --nginx-one-telemetry-endpoint-host={{ .Values.nginx.nginxOneConsole.endpointHost }} {{- end }} {{- if .Values.nginx.nginxOneConsole.endpointPort }} - - --nginx-one-console-telemetry-endpoint-port={{ .Values.nginx.nginxOneConsole.endpointPort }} + - --nginx-one-telemetry-endpoint-port={{ .Values.nginx.nginxOneConsole.endpointPort }} {{- end }} {{- if .Values.nginx.nginxOneConsole.tlsSkipVerify }} - - --nginx-one-console-tls-skip-verify + - --nginx-one-tls-skip-verify {{- end }} {{- end }} env: diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index 82b85b5fb2..89fa0ff8fa 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -465,6 +465,8 @@ "endpointPort": { "default": 443, "description": "The endpoint port that the NGINX One Console telemetry metrics will be sent to.", + "maximum": 65535, + "minimum": 1, "required": [], "title": "endpointPort", "type": "integer" diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index 6f167842ae..41ebf23526 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -222,6 +222,11 @@ nginx: # -- The Endpoint host that the NGINX One Console telemetry metrics will be sent to. endpointHost: "agent.connect.nginx.com" + # @schema + # type: integer + # minimum: 1 + # maximum: 65535 + # @schema # -- The endpoint port that the NGINX One Console telemetry metrics will be sent to. endpointPort: 443 diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 7b2ea6e335..dcbb240d0a 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -57,31 +57,31 @@ func createRootCommand() *cobra.Command { func createControllerCommand() *cobra.Command { // flag names const ( - configFlag = "config" - serviceFlag = "service" - agentTLSSecretFlag = "agent-tls-secret" - nginxOneConsoleDataplaneKeySecretFlag = "nginx-one-console-dataplane-key-secret" //nolint:gosec // not credentials - nginxOneConsoleTelemetryEndpointHostFlag = "nginx-one-console-telemetry-endpoint-host" - nginxOneConsoleTelemetryEndpointPortFlag = "nginx-one-console-telemetry-endpoint-port" - nginxOneConsoleTLSSkipVerifyFlag = "nginx-one-console-tls-skip-verify" - metricsDisableFlag = "metrics-disable" - metricsSecureFlag = "metrics-secure-serving" - metricsPortFlag = "metrics-port" - healthDisableFlag = "health-disable" - healthPortFlag = "health-port" - leaderElectionDisableFlag = "leader-election-disable" - leaderElectionLockNameFlag = "leader-election-lock-name" - productTelemetryDisableFlag = "product-telemetry-disable" - gwAPIExperimentalFlag = "gateway-api-experimental-features" - nginxDockerSecretFlag = "nginx-docker-secret" //nolint:gosec // not credentials - usageReportSecretFlag = "usage-report-secret" - usageReportEndpointFlag = "usage-report-endpoint" - usageReportResolverFlag = "usage-report-resolver" - usageReportSkipVerifyFlag = "usage-report-skip-verify" - usageReportClientSSLSecretFlag = "usage-report-client-ssl-secret" //nolint:gosec // not credentials - usageReportCASecretFlag = "usage-report-ca-secret" //nolint:gosec // not credentials - snippetsFiltersFlag = "snippets-filters" - nginxSCCFlag = "nginx-scc" + configFlag = "config" + serviceFlag = "service" + agentTLSSecretFlag = "agent-tls-secret" + nginxOneDataplaneKeySecretFlag = "nginx-one-dataplane-key-secret" //nolint:gosec // not credentials + nginxOneTelemetryEndpointHostFlag = "nginx-one-telemetry-endpoint-host" + nginxOneTelemetryEndpointPortFlag = "nginx-one-telemetry-endpoint-port" + nginxOneTLSSkipVerifyFlag = "nginx-one-tls-skip-verify" + metricsDisableFlag = "metrics-disable" + metricsSecureFlag = "metrics-secure-serving" + metricsPortFlag = "metrics-port" + healthDisableFlag = "health-disable" + healthPortFlag = "health-port" + leaderElectionDisableFlag = "leader-election-disable" + leaderElectionLockNameFlag = "leader-election-lock-name" + productTelemetryDisableFlag = "product-telemetry-disable" + gwAPIExperimentalFlag = "gateway-api-experimental-features" + nginxDockerSecretFlag = "nginx-docker-secret" //nolint:gosec // not credentials + usageReportSecretFlag = "usage-report-secret" + usageReportEndpointFlag = "usage-report-endpoint" + usageReportResolverFlag = "usage-report-resolver" + usageReportSkipVerifyFlag = "usage-report-skip-verify" + usageReportClientSSLSecretFlag = "usage-report-client-ssl-secret" //nolint:gosec // not credentials + usageReportCASecretFlag = "usage-report-ca-secret" //nolint:gosec // not credentials + snippetsFiltersFlag = "snippets-filters" + nginxSCCFlag = "nginx-scc" ) // flag values @@ -327,26 +327,26 @@ func createControllerCommand() *cobra.Command { cmd.Flags().Var( &nginxOneConsoleDataplaneKeySecretName, - nginxOneConsoleDataplaneKeySecretFlag, + nginxOneDataplaneKeySecretFlag, `The name of the Secret containing the NGINX One Console's dataplane key. Must exist in the same namespace that `+ `the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).`, ) cmd.Flags().Var( &nginxOneConsoleTelemetryEndpointHost, - nginxOneConsoleTelemetryEndpointHostFlag, + nginxOneTelemetryEndpointHostFlag, `The host of the NGINX One Console's telemetry endpoint.`, ) cmd.Flags().Var( &nginxOneConsoleTelemetryEndpointPort, - nginxOneConsoleTelemetryEndpointPortFlag, + nginxOneTelemetryEndpointPortFlag, `The port of the NGINX One Console's telemetry endpoint.`, ) cmd.Flags().BoolVar( &nginxOneConsoleTLSSkipVerify, - nginxOneConsoleTLSSkipVerifyFlag, + nginxOneTLSSkipVerifyFlag, false, "Disable client verification of the NGINX One Console's telemetry endpoint server certificate.", ) diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 99d51492d3..0ecd4111d9 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -156,10 +156,10 @@ func TestControllerCmdFlagValidation(t *testing.T) { "--usage-report-client-ssl-secret=client-secret", "--snippets-filters", "--nginx-scc=nginx-sscc-name", - "--nginx-one-console-dataplane-key-secret=dataplane-key-secret", - "--nginx-one-console-telemetry-endpoint-host=telemetry-endpoint-host", - "--nginx-one-console-telemetry-endpoint-port=443", - "--nginx-one-console-tls-skip-verify", + "--nginx-one-dataplane-key-secret=dataplane-key-secret", + "--nginx-one-telemetry-endpoint-host=telemetry-endpoint-host", + "--nginx-one-telemetry-endpoint-port=443", + "--nginx-one-tls-skip-verify", }, wantErr: false, }, @@ -431,62 +431,62 @@ func TestControllerCmdFlagValidation(t *testing.T) { expectedErrPrefix: `invalid argument "!@#$" for "--nginx-scc" flag: invalid format: `, }, { - name: "nginx-one-console-dataplane-key-secret is set to empty string", + name: "nginx-one-dataplane-key-secret is set to empty string", args: []string{ - "--nginx-one-console-dataplane-key-secret=", + "--nginx-one-dataplane-key-secret=", }, wantErr: true, - expectedErrPrefix: `invalid argument "" for "--nginx-one-console-dataplane-key-secret" flag: must be set`, + expectedErrPrefix: `invalid argument "" for "--nginx-one-dataplane-key-secret" flag: must be set`, }, { - name: "nginx-one-console-dataplane-key-secret is invalid", + name: "nginx-one-dataplane-key-secret is invalid", args: []string{ - "--nginx-one-console-dataplane-key-secret=!@#$", + "--nginx-one-dataplane-key-secret=!@#$", }, wantErr: true, - expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-dataplane-key-secret" flag: invalid format: `, + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-dataplane-key-secret" flag: invalid format: `, }, { - name: "nginx-one-console-telemetry-endpoint-host is set to empty string", + name: "nginx-one-telemetry-endpoint-host is set to empty string", args: []string{ - "--nginx-one-console-telemetry-endpoint-host=", + "--nginx-one-telemetry-endpoint-host=", }, wantErr: true, - expectedErrPrefix: `invalid argument "" for "--nginx-one-console-telemetry-endpoint-host" flag: must be set`, + expectedErrPrefix: `invalid argument "" for "--nginx-one-telemetry-endpoint-host" flag: must be set`, }, { - name: "nginx-one-console-telemetry-endpoint-host is invalid", + name: "nginx-one-telemetry-endpoint-host is invalid", args: []string{ - "--nginx-one-console-telemetry-endpoint-host=!@#$", + "--nginx-one-telemetry-endpoint-host=!@#$", }, wantErr: true, - expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-console-telemetry-endpoint-host" ` + + expectedErrPrefix: `invalid argument "!@#$" for "--nginx-one-telemetry-endpoint-host" ` + `flag: invalid format: `, }, { - name: "nginx-one-console-telemetry-endpoint-port is invalid type", + name: "nginx-one-telemetry-endpoint-port is invalid type", args: []string{ - "--nginx-one-console-telemetry-endpoint-port=invalid", // not an int + "--nginx-one-telemetry-endpoint-port=invalid", // not an int }, wantErr: true, - expectedErrPrefix: `invalid argument "invalid" for "--nginx-one-console-telemetry-endpoint-port" ` + + expectedErrPrefix: `invalid argument "invalid" for "--nginx-one-telemetry-endpoint-port" ` + `flag: failed to parse int value: strconv.ParseInt: parsing "invalid": invalid syntax`, }, { - name: "nginx-one-console-telemetry-endpoint-port is outside of range", + name: "nginx-one-telemetry-endpoint-port is outside of range", args: []string{ - "--nginx-one-console-telemetry-endpoint-port=65536", // outside of range + "--nginx-one-telemetry-endpoint-port=65536", // outside of range }, wantErr: true, - expectedErrPrefix: `invalid argument "65536" for "--nginx-one-console-telemetry-endpoint-port" flag:` + + expectedErrPrefix: `invalid argument "65536" for "--nginx-one-telemetry-endpoint-port" flag:` + ` port outside of valid port range [1 - 65535]: 65536`, }, { - name: "nginx-one-console-tls-skip-verify is not a bool", - expectedErrPrefix: `invalid argument "not-a-bool" for "--nginx-one-console-tls-skip-verify" flag:` + - `strconv.ParseBool: parsing "not-a-bool": invalid syntax`, + name: "nginx-one-tls-skip-verify is not a bool", + expectedErrPrefix: `invalid argument "not-a-bool" for "--nginx-one-tls-skip-verify" flag:` + + ` strconv.ParseBool: parsing "not-a-bool": invalid syntax`, args: []string{ - "--nginx-one-console-tls-skip-verify=not-a-bool", + "--nginx-one-tls-skip-verify=not-a-bool", }, wantErr: true, }, From 0bf5a6d73be2990291f015398a961631c1cb43b5 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Sun, 3 Aug 2025 19:24:21 -0700 Subject: [PATCH 04/15] Add some more review changes --- internal/controller/manager.go | 15 ------------ internal/controller/provisioner/objects.go | 4 ++-- .../controller/provisioner/provisioner.go | 24 +++++++++++++++++++ .../provisioner/provisioner_test.go | 10 ++++++++ internal/controller/provisioner/templates.go | 6 ++--- 5 files changed, 39 insertions(+), 20 deletions(-) diff --git a/internal/controller/manager.go b/internal/controller/manager.go index c9a61cb1df..94c929d493 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -200,20 +200,6 @@ func StartManager(cfg config.Config) error { return fmt.Errorf("cannot register grpc server: %w", err) } - agentLabelCollector := telemetry.NewLabelCollectorConfigImpl(telemetry.LabelCollectorConfig{ - K8sClientReader: mgr.GetAPIReader(), - Version: cfg.GatewayPodConfig.Version, - PodNSName: types.NamespacedName{ - Namespace: cfg.GatewayPodConfig.Namespace, - Name: cfg.GatewayPodConfig.Name, - }, - }) - - agentLabels, err := agentLabelCollector.Collect(ctx) - if err != nil { - return fmt.Errorf("failed to collect agent labels: %w", err) - } - nginxProvisioner, provLoop, err := provisioner.NewNginxProvisioner( ctx, mgr, @@ -229,7 +215,6 @@ func StartManager(cfg config.Config) error { Plus: cfg.Plus, NginxDockerSecretNames: cfg.NginxDockerSecretNames, PlusUsageConfig: &cfg.UsageReportConfig, - AgentLabels: agentLabels, NginxOneConsoleTelemetryConfig: cfg.NginxOneConsoleTelemetryConfig, }, ) diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 03906dbddf..7b535382c8 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -413,11 +413,11 @@ func (p *NginxProvisioner) buildNginxConfigMaps( } if logging != nil && logging.AgentLevel != nil { - agentFields["LogLevel"] = logging.AgentLevel + agentFields["LogLevel"] = *logging.AgentLevel } if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { - agentFields["DataplaneKeySecretName"] = p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName + agentFields["NginxOneReporting"] = true agentFields["EndpointHost"] = p.cfg.NginxOneConsoleTelemetryConfig.EndpointHost agentFields["EndpointPort"] = strconv.Itoa(p.cfg.NginxOneConsoleTelemetryConfig.EndpointPort) agentFields["EndpointTLSSkipVerify"] = p.cfg.NginxOneConsoleTelemetryConfig.EndpointTLSSkipVerify diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 9700d784de..a82909c886 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -76,6 +76,23 @@ type NginxProvisioner struct { var apiChecker openshift.APIChecker = &openshift.APICheckerImpl{} +var labelCollectorFactory func(mgr manager.Manager, cfg Config) AgentLabelCollector = defaultLabelCollectorFactory + +func defaultLabelCollectorFactory(mgr manager.Manager, cfg Config) AgentLabelCollector { + return telemetry.NewLabelCollectorConfigImpl(telemetry.LabelCollectorConfig{ + K8sClientReader: mgr.GetAPIReader(), + Version: cfg.GatewayPodConfig.Version, + PodNSName: types.NamespacedName{ + Namespace: cfg.GatewayPodConfig.Namespace, + Name: cfg.GatewayPodConfig.Name, + }, + }) +} + +type AgentLabelCollector interface { + Collect(ctx context.Context) (telemetry.AgentLabels, error) +} + // NewNginxProvisioner returns a new instance of a Provisioner that will deploy nginx resources. func NewNginxProvisioner( ctx context.Context, @@ -112,6 +129,13 @@ func NewNginxProvisioner( cfg.Logger.Error(err, "could not determine if running in openshift, will not create Role/RoleBinding") } + agentLabelCollector := labelCollectorFactory(mgr, cfg) + agentLabels, err := agentLabelCollector.Collect(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to collect agent labels: %w", err) + } + cfg.AgentLabels = agentLabels + provisioner := &NginxProvisioner{ k8sClient: mgr.GetClient(), store: store, diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index caa809bdd1..1db6704757 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -203,6 +203,12 @@ func defaultNginxProvisioner( }, fakeClient, deploymentStore } +type fakeLabelCollector struct{} + +func (f *fakeLabelCollector) Collect(_ context.Context) (telemetry.AgentLabels, error) { + return telemetry.AgentLabels{ProductType: "fake"}, nil +} + func TestNewNginxProvisioner(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -219,6 +225,10 @@ func TestNewNginxProvisioner(t *testing.T) { } apiChecker = &openshiftfakes.FakeAPIChecker{} + labelCollectorFactory = func(_ manager.Manager, _ Config) AgentLabelCollector { + return &fakeLabelCollector{} + } + provisioner, eventLoop, err := NewNginxProvisioner(context.TODO(), mgr, cfg) g.Expect(err).ToNot(HaveOccurred()) g.Expect(provisioner).NotTo(BeNil()) diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index c333a8ebbc..4b921a7744 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -66,13 +66,13 @@ labels: {{ $key }}: {{ $value }} {{- end }} -{{- if .DataplaneKeySecretName }} +{{- if .NginxOneReporting }} auxiliary_command: - server: + server: host: {{ .EndpointHost }} port: {{ .EndpointPort }} type: grpc - auth: + auth: tokenpath: /etc/nginx-agent/secrets/dataplane.key tls: skip_verify: {{ .EndpointTLSSkipVerify }} From 0b522a7be1b2043365ad1ad287f4197da04f7f1b Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Sun, 3 Aug 2025 19:28:56 -0700 Subject: [PATCH 05/15] Change name to LabelCollector --- internal/controller/provisioner/provisioner.go | 2 +- internal/controller/telemetry/agent_labels.go | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index a82909c886..371396a006 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -79,7 +79,7 @@ var apiChecker openshift.APIChecker = &openshift.APICheckerImpl{} var labelCollectorFactory func(mgr manager.Manager, cfg Config) AgentLabelCollector = defaultLabelCollectorFactory func defaultLabelCollectorFactory(mgr manager.Manager, cfg Config) AgentLabelCollector { - return telemetry.NewLabelCollectorConfigImpl(telemetry.LabelCollectorConfig{ + return telemetry.NewLabelCollector(telemetry.LabelCollectorConfig{ K8sClientReader: mgr.GetAPIReader(), Version: cfg.GatewayPodConfig.Version, PodNSName: types.NamespacedName{ diff --git a/internal/controller/telemetry/agent_labels.go b/internal/controller/telemetry/agent_labels.go index 2cab61c6dc..15401019ba 100644 --- a/internal/controller/telemetry/agent_labels.go +++ b/internal/controller/telemetry/agent_labels.go @@ -18,7 +18,7 @@ type AgentLabels struct { ControlNamespace string `json:"control-namespace"` } -// LabelCollectorConfig holds configuration parameters for LabelCollectorImpl. +// LabelCollectorConfig holds configuration parameters for LabelCollector. type LabelCollectorConfig struct { // K8sClientReader is a Kubernetes API client Reader. K8sClientReader client.Reader @@ -28,21 +28,21 @@ type LabelCollectorConfig struct { PodNSName types.NamespacedName } -// LabelCollectorConfigImpl is an implementation of LabelCollectorConfig. -type LabelCollectorConfigImpl struct { +// LabelCollector is an implementation of AgentLabelCollector. +type LabelCollector struct { cfg LabelCollectorConfig } -// NewLabelCollectorConfigImpl creates a new LabelCollectorConfigImpl for a telemetry Job. -func NewLabelCollectorConfigImpl( +// NewLabelCollector creates a new LabelCollector. +func NewLabelCollector( cfg LabelCollectorConfig, -) *LabelCollectorConfigImpl { - return &LabelCollectorConfigImpl{ +) *LabelCollector { + return &LabelCollector{ cfg: cfg, } } -func (l *LabelCollectorConfigImpl) Collect(ctx context.Context) (AgentLabels, error) { +func (l *LabelCollector) Collect(ctx context.Context) (AgentLabels, error) { clusterID, err := collectClusterID(ctx, l.cfg.K8sClientReader) if err != nil { return AgentLabels{}, fmt.Errorf("failed to collect cluster information: %w", err) From 92d45639c67d2dd5a531fd990fb18fc05797d2d0 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Sun, 3 Aug 2025 20:35:32 -0700 Subject: [PATCH 06/15] Refactor AgentLabels to be a map --- internal/controller/provisioner/objects.go | 3 +- .../controller/provisioner/provisioner.go | 4 +- .../provisioner/provisioner_test.go | 19 ++++---- internal/controller/telemetry/agent_labels.go | 46 ++++++------------- 4 files changed, 25 insertions(+), 47 deletions(-) diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 7b535382c8..6a23b179f6 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -25,7 +25,6 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/config" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/dataplane" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" - "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -409,7 +408,7 @@ func (p *NginxProvisioner) buildNginxConfigMaps( "Namespace": p.cfg.GatewayPodConfig.Namespace, "EnableMetrics": enableMetrics, "MetricsPort": metricsPort, - "AgentLabels": telemetry.AgentLabelsToMap(p.cfg.AgentLabels), + "AgentLabels": p.cfg.AgentLabels, } if logging != nil && logging.AgentLevel != nil { diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 371396a006..b5a04d4121 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -49,7 +49,7 @@ type Config struct { PlusUsageConfig *config.UsageReportConfig StatusQueue *status.Queue GatewayPodConfig *config.GatewayPodConfig - AgentLabels telemetry.AgentLabels + AgentLabels map[string]string Logger logr.Logger NGINXSCCName string GCName string @@ -90,7 +90,7 @@ func defaultLabelCollectorFactory(mgr manager.Manager, cfg Config) AgentLabelCol } type AgentLabelCollector interface { - Collect(ctx context.Context) (telemetry.AgentLabels, error) + Collect(ctx context.Context) (map[string]string, error) } // NewNginxProvisioner returns a new instance of a Provisioner that will deploy nginx resources. diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index 1db6704757..1bc95f7f54 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -24,7 +24,6 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/agentfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/provisioner/openshift/openshiftfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" - "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -190,13 +189,13 @@ func defaultNginxProvisioner( EndpointPort: 443, EndpointTLSSkipVerify: false, }, - AgentLabels: telemetry.AgentLabels{ - ProductType: "ngf", - ProductVersion: "ngf-version", - ClusterID: "my-cluster-id", - ControlName: "my-control-plane-name", - ControlID: "my-control-plane-id", - ControlNamespace: "my-control-plane-namespace", + AgentLabels: map[string]string{ + "product-type": "ngf", + "product-version": "ngf-version", + "cluster-id": "my-cluster-id", + "control-name": "my-control-plane-name", + "control-id": "my-control-plane-id", + "control-namespace": "my-control-plane-namespace", }, }, leader: true, @@ -205,8 +204,8 @@ func defaultNginxProvisioner( type fakeLabelCollector struct{} -func (f *fakeLabelCollector) Collect(_ context.Context) (telemetry.AgentLabels, error) { - return telemetry.AgentLabels{ProductType: "fake"}, nil +func (f *fakeLabelCollector) Collect(_ context.Context) (map[string]string, error) { + return map[string]string{"product-type": "fake"}, nil } func TestNewNginxProvisioner(t *testing.T) { diff --git a/internal/controller/telemetry/agent_labels.go b/internal/controller/telemetry/agent_labels.go index 15401019ba..d6a8d8b78c 100644 --- a/internal/controller/telemetry/agent_labels.go +++ b/internal/controller/telemetry/agent_labels.go @@ -8,16 +8,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// AgentLabels contains the metadata information needed for reporting to Agent v3. -type AgentLabels struct { - ProductType string `json:"product-type"` - ProductVersion string `json:"product-version"` - ClusterID string `json:"cluster-id"` - ControlName string `json:"control-name"` - ControlID string `json:"control-id"` - ControlNamespace string `json:"control-namespace"` -} - // LabelCollectorConfig holds configuration parameters for LabelCollector. type LabelCollectorConfig struct { // K8sClientReader is a Kubernetes API client Reader. @@ -42,41 +32,31 @@ func NewLabelCollector( } } -func (l *LabelCollector) Collect(ctx context.Context) (AgentLabels, error) { +// Collect gathers metadata labels needed for reporting to Agent v3. +func (l *LabelCollector) Collect(ctx context.Context) (map[string]string, error) { + agentLabels := make(map[string]string) + clusterID, err := collectClusterID(ctx, l.cfg.K8sClientReader) if err != nil { - return AgentLabels{}, fmt.Errorf("failed to collect cluster information: %w", err) + return nil, fmt.Errorf("failed to collect cluster information: %w", err) } replicaSet, err := getPodReplicaSet(ctx, l.cfg.K8sClientReader, l.cfg.PodNSName) if err != nil { - return AgentLabels{}, fmt.Errorf("failed to get replica set for pod %v: %w", l.cfg.PodNSName, err) + return nil, fmt.Errorf("failed to get replica set for pod %v: %w", l.cfg.PodNSName, err) } deploymentID, err := getDeploymentID(replicaSet) if err != nil { - return AgentLabels{}, fmt.Errorf("failed to get NGF deploymentID: %w", err) + return nil, fmt.Errorf("failed to get NGF deploymentID: %w", err) } - agentLabels := AgentLabels{ - ProductType: "ngf", - ProductVersion: l.cfg.Version, - ClusterID: clusterID, - ControlName: l.cfg.PodNSName.Name, - ControlNamespace: l.cfg.PodNSName.Namespace, - ControlID: deploymentID, - } + agentLabels["product-type"] = "ngf" + agentLabels["product-version"] = l.cfg.Version + agentLabels["cluster-id"] = clusterID + agentLabels["control-name"] = l.cfg.PodNSName.Name + agentLabels["control-namespace"] = l.cfg.PodNSName.Namespace + agentLabels["control-id"] = deploymentID return agentLabels, nil } - -func AgentLabelsToMap(labels AgentLabels) map[string]string { - return map[string]string{ - "product-type": labels.ProductType, - "product-version": labels.ProductVersion, - "cluster-id": labels.ClusterID, - "control-name": labels.ControlName, - "control-namespace": labels.ControlNamespace, - "control-id": labels.ControlID, - } -} From 2c4221a612c7bc13b20fa58d4bf39dfb277cf3f4 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Mon, 4 Aug 2025 14:51:49 -0700 Subject: [PATCH 07/15] Add missing pieces for new dataplane key secret --- internal/controller/provisioner/handler.go | 6 ++++ .../controller/provisioner/handler_test.go | 3 +- .../controller/provisioner/provisioner.go | 1 + .../provisioner/provisioner_test.go | 14 ++++---- internal/controller/provisioner/store.go | 35 ++++++++++++++----- internal/controller/provisioner/store_test.go | 31 +++++++++++----- 6 files changed, 67 insertions(+), 23 deletions(-) diff --git a/internal/controller/provisioner/handler.go b/internal/controller/provisioner/handler.go index 037954575f..e3327cab68 100644 --- a/internal/controller/provisioner/handler.go +++ b/internal/controller/provisioner/handler.go @@ -256,6 +256,8 @@ func (h *eventHandler) provisionResourceForAllGateways( // deprovisionSecretsForAllGateways cleans up any secrets that a user deleted that were duplicated // for all Gateways. For example, NGINX Plus secrets. +// +//nolint:gocyclo // will refactor at some point func (h *eventHandler) deprovisionSecretsForAllGateways(ctx context.Context, secret string) error { var allErrs []error @@ -283,6 +285,10 @@ func (h *eventHandler) deprovisionSecretsForAllGateways(ctx context.Context, sec if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.PlusClientSSLSecret}); err != nil { allErrs = append(allErrs, err) } + case strings.HasSuffix(resources.DataplaneKeySecret.Name, secret): + if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.DataplaneKeySecret}); err != nil { + allErrs = append(allErrs, err) + } default: for _, dockerSecret := range resources.DockerSecrets { if strings.HasSuffix(dockerSecret.Name, secret) { diff --git a/internal/controller/provisioner/handler_test.go b/internal/controller/provisioner/handler_test.go index 45fd0cc435..fef621c094 100644 --- a/internal/controller/provisioner/handler_test.go +++ b/internal/controller/provisioner/handler_test.go @@ -23,7 +23,7 @@ func TestHandleEventBatch_Upsert(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore([]string{dockerTestSecretName}, "", jwtTestSecretName, "", "") + store := newStore([]string{dockerTestSecretName}, "", jwtTestSecretName, "", "", "") provisioner, fakeClient, _ := defaultNginxProvisioner() provisioner.cfg.StatusQueue = status.NewQueue() @@ -213,6 +213,7 @@ func TestHandleEventBatch_Delete(t *testing.T) { jwtTestSecretName, caTestSecretName, clientTestSecretName, + nginxOneDataplaneKeySecretName, ) provisioner, fakeClient, _ := defaultNginxProvisioner() provisioner.cfg.StatusQueue = status.NewQueue() diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index b5a04d4121..0580d7c2bc 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -112,6 +112,7 @@ func NewNginxProvisioner( jwtSecretName, caSecretName, clientSSLSecretName, + cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, ) selector := metav1.LabelSelector{ diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index 1bc95f7f54..07590e65de 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -29,12 +29,13 @@ import ( ) const ( - agentTLSTestSecretName = "agent-tls-secret" - jwtTestSecretName = "jwt-secret" - caTestSecretName = "ca-secret" - clientTestSecretName = "client-secret" - dockerTestSecretName = "docker-secret" - ngfNamespace = "nginx-gateway" + agentTLSTestSecretName = "agent-tls-secret" + jwtTestSecretName = "jwt-secret" + caTestSecretName = "ca-secret" + clientTestSecretName = "client-secret" + dockerTestSecretName = "docker-secret" + ngfNamespace = "nginx-gateway" + nginxOneDataplaneKeySecretName = "dataplane-key" ) func createScheme() *runtime.Scheme { @@ -164,6 +165,7 @@ func defaultNginxProvisioner( jwtTestSecretName, caTestSecretName, clientTestSecretName, + nginxOneDataplaneKeySecretName, ), k8sClient: fakeClient, cfg: Config{ diff --git a/internal/controller/provisioner/store.go b/internal/controller/provisioner/store.go index 2eb0d154cf..19d0a19419 100644 --- a/internal/controller/provisioner/store.go +++ b/internal/controller/provisioner/store.go @@ -31,6 +31,7 @@ type NginxResources struct { PlusJWTSecret metav1.ObjectMeta PlusClientSSLSecret metav1.ObjectMeta PlusCASecret metav1.ObjectMeta + DataplaneKeySecret metav1.ObjectMeta DockerSecrets []metav1.ObjectMeta } @@ -50,6 +51,9 @@ type store struct { caSecretName string clientSSLSecretName string + // NGINX One Dataplane key secret + dataplaneKeySecretName string + lock sync.RWMutex } @@ -58,7 +62,8 @@ func newStore( agentTLSSecretName, jwtSecretName, caSecretName, - clientSSLSecretName string, + clientSSLSecretName, + dataplaneKeySecretName string, ) *store { dockerSecretNamesMap := make(map[string]struct{}) for _, name := range dockerSecretNames { @@ -66,13 +71,14 @@ func newStore( } return &store{ - gateways: make(map[types.NamespacedName]*gatewayv1.Gateway), - nginxResources: make(map[types.NamespacedName]*NginxResources), - dockerSecretNames: dockerSecretNamesMap, - agentTLSSecretName: agentTLSSecretName, - jwtSecretName: jwtSecretName, - caSecretName: caSecretName, - clientSSLSecretName: clientSSLSecretName, + gateways: make(map[types.NamespacedName]*gatewayv1.Gateway), + nginxResources: make(map[types.NamespacedName]*NginxResources), + dockerSecretNames: dockerSecretNamesMap, + agentTLSSecretName: agentTLSSecretName, + jwtSecretName: jwtSecretName, + caSecretName: caSecretName, + clientSSLSecretName: clientSSLSecretName, + dataplaneKeySecretName: dataplaneKeySecretName, } } @@ -226,6 +232,10 @@ func (s *store) registerSecretInGatewayConfig(obj *corev1.Secret, gatewayNSName s.nginxResources[gatewayNSName] = &NginxResources{ PlusClientSSLSecret: obj.ObjectMeta, } + case hasSuffix(obj.GetName(), s.dataplaneKeySecretName): + s.nginxResources[gatewayNSName] = &NginxResources{ + DataplaneKeySecret: obj.ObjectMeta, + } } for secret := range s.dockerSecretNames { @@ -246,6 +256,8 @@ func (s *store) registerSecretInGatewayConfig(obj *corev1.Secret, gatewayNSName cfg.PlusCASecret = obj.ObjectMeta case hasSuffix(obj.GetName(), s.clientSSLSecretName): cfg.PlusClientSSLSecret = obj.ObjectMeta + case hasSuffix(obj.GetName(), s.dataplaneKeySecretName): + cfg.DataplaneKeySecret = obj.ObjectMeta } for secret := range s.dockerSecretNames { @@ -357,6 +369,10 @@ func secretResourceMatches(resources *NginxResources, nsName types.NamespacedNam return true } + if resourceMatches(resources.DataplaneKeySecret, nsName) { + return true + } + return resourceMatches(resources.PlusCASecret, nsName) } @@ -437,6 +453,9 @@ func getResourceVersionForSecret(resources *NginxResources, secret *corev1.Secre if resources.PlusCASecret.GetName() == secret.GetName() { return resources.PlusCASecret.GetResourceVersion() } + if resources.DataplaneKeySecret.GetName() == secret.GetName() { + return resources.DataplaneKeySecret.GetResourceVersion() + } return "" } diff --git a/internal/controller/provisioner/store_test.go b/internal/controller/provisioner/store_test.go index 06b1a33c50..c38da7d7fe 100644 --- a/internal/controller/provisioner/store_test.go +++ b/internal/controller/provisioner/store_test.go @@ -23,7 +23,14 @@ func TestNewStore(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore([]string{"docker-secret"}, "agent-tls-secret", "jwt-secret", "ca-secret", "client-ssl-secret") + store := newStore( + []string{"docker-secret"}, + "agent-tls-secret", + "jwt-secret", + "ca-secret", + "client-ssl-secret", + "dataplane-key", + ) g.Expect(store).NotTo(BeNil()) g.Expect(store.dockerSecretNames).To(HaveKey("docker-secret")) @@ -31,13 +38,14 @@ func TestNewStore(t *testing.T) { g.Expect(store.jwtSecretName).To(Equal("jwt-secret")) g.Expect(store.caSecretName).To(Equal("ca-secret")) g.Expect(store.clientSSLSecretName).To(Equal("client-ssl-secret")) + g.Expect(store.dataplaneKeySecretName).To(Equal("dataplane-key")) } func TestUpdateGateway(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") gateway := &gatewayv1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Name: "test-gateway", @@ -56,7 +64,7 @@ func TestDeleteGateway(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"} store.gateways[nsName] = &gatewayv1.Gateway{} @@ -70,7 +78,7 @@ func TestGetGateways(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") gateway1 := &gatewayv1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Name: "test-gateway-1", @@ -101,7 +109,14 @@ func TestRegisterResourceInGatewayConfig(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore([]string{"docker-secret"}, "agent-tls-secret", "jwt-secret", "ca-secret", "client-ssl-secret") + store := newStore( + []string{"docker-secret"}, + "agent-tls-secret", + "jwt-secret", + "ca-secret", + "client-ssl-secret", + "dataplane-key", + ) nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"} registerAndGetResources := func(obj any) *NginxResources { @@ -415,7 +430,7 @@ func TestDeleteResourcesForGateway(t *testing.T) { t.Parallel() g := NewWithT(t) - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"} store.nginxResources[nsName] = &NginxResources{} @@ -427,7 +442,7 @@ func TestDeleteResourcesForGateway(t *testing.T) { func TestGatewayExistsForResource(t *testing.T) { t.Parallel() - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") gateway := &graph.Gateway{} store.nginxResources[types.NamespacedName{Name: "test-gateway", Namespace: "default"}] = &NginxResources{ Gateway: gateway, @@ -648,7 +663,7 @@ func TestGatewayExistsForResource(t *testing.T) { func TestGetResourceVersionForObject(t *testing.T) { t.Parallel() - store := newStore(nil, "", "", "", "") + store := newStore(nil, "", "", "", "", "") nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"} store.nginxResources[nsName] = &NginxResources{ Deployment: metav1.ObjectMeta{ From 39e30630eb3310d16e083dc0a7713b1a12a5497e Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Mon, 4 Aug 2025 16:04:21 -0700 Subject: [PATCH 08/15] Add some more missing pieces for provisioning secret --- internal/controller/provisioner/eventloop.go | 4 +++- internal/controller/provisioner/handler.go | 2 ++ internal/controller/provisioner/handler_test.go | 9 +++++++++ internal/controller/provisioner/objects.go | 15 +++++++++++++++ internal/controller/provisioner/provisioner.go | 5 +++++ 5 files changed, 34 insertions(+), 1 deletion(-) diff --git a/internal/controller/provisioner/eventloop.go b/internal/controller/provisioner/eventloop.go index 7e076e21db..c7fb485f20 100644 --- a/internal/controller/provisioner/eventloop.go +++ b/internal/controller/provisioner/eventloop.go @@ -31,14 +31,16 @@ func newEventLoop( ngfNamespace string, dockerSecrets []string, agentTLSSecret string, + dataplaneKeySecret string, usageConfig *config.UsageReportConfig, isOpenshift bool, ) (*events.EventLoop, error) { nginxResourceLabelPredicate := predicate.NginxLabelPredicate(selector) - secretsToWatch := make([]string, 0, len(dockerSecrets)+4) + secretsToWatch := make([]string, 0, len(dockerSecrets)+5) secretsToWatch = append(secretsToWatch, agentTLSSecret) secretsToWatch = append(secretsToWatch, dockerSecrets...) + secretsToWatch = append(secretsToWatch, dataplaneKeySecret) if usageConfig != nil { if usageConfig.SecretName != "" { diff --git a/internal/controller/provisioner/handler.go b/internal/controller/provisioner/handler.go index e3327cab68..1436be7fc4 100644 --- a/internal/controller/provisioner/handler.go +++ b/internal/controller/provisioner/handler.go @@ -124,10 +124,12 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, } case *corev1.Secret: if h.provisioner.isUserSecret(e.NamespacedName.Name) { + fmt.Println("This should be a user secret") if err := h.deprovisionSecretsForAllGateways(ctx, e.NamespacedName.Name); err != nil { logger.Error(err, "error removing secrets") } } else { + fmt.Println("aparently we are reprovisioning") if err := h.reprovisionResources(ctx, e); err != nil { logger.Error(err, "error re-provisioning nginx resources") } diff --git a/internal/controller/provisioner/handler_test.go b/internal/controller/provisioner/handler_test.go index fef621c094..4c705cd070 100644 --- a/internal/controller/provisioner/handler_test.go +++ b/internal/controller/provisioner/handler_test.go @@ -299,6 +299,14 @@ func TestHandleEventBatch_Delete(t *testing.T) { } g.Expect(fakeClient.Create(ctx, userDockerSecret)).To(Succeed()) + userDataplaneKeySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nginxOneDataplaneKeySecretName, + Namespace: ngfNamespace, + }, + } + g.Expect(fakeClient.Create(ctx, userDataplaneKeySecret)).To(Succeed()) + upsertEvent := &events.UpsertEvent{Resource: gateway} batch := events.EventBatch{upsertEvent} handler.HandleEventBatch(ctx, logger, batch) @@ -342,6 +350,7 @@ func TestHandleEventBatch_Delete(t *testing.T) { verifySecret(caTestSecretName, userCASecret) verifySecret(clientTestSecretName, userClientSSLSecret) verifySecret(dockerTestSecretName, userDockerSecret) + verifySecret(nginxOneDataplaneKeySecretName, userDataplaneKeySecret) // delete Gateway when provisioner is not leader provisioner.leader = false diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 6a23b179f6..4f9f71e2c8 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -1190,6 +1190,21 @@ func (p *NginxProvisioner) buildNginxResourceObjectsForDeletion(deploymentNSName objects = append(objects, jwtSecret) } + var dataplaneKeySecretName string + if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { + dataplaneKeySecretName = controller.CreateNginxResourceName( + deploymentNSName.Name, + p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, + ) + dataplaneKeySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: dataplaneKeySecretName, + Namespace: deploymentNSName.Namespace, + }, + } + objects = append(objects, dataplaneKeySecret) + } + return objects } diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 0580d7c2bc..6e55a7e623 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -160,6 +160,7 @@ func NewNginxProvisioner( cfg.GatewayPodConfig.Namespace, cfg.NginxDockerSecretNames, cfg.AgentTLSSecretName, + cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, cfg.PlusUsageConfig, isOpenshift, ) @@ -443,6 +444,10 @@ func (p *NginxProvisioner) isUserSecret(name string) bool { return true } + if p.cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName == name { + return true + } + if p.cfg.PlusUsageConfig != nil { return name == p.cfg.PlusUsageConfig.SecretName || name == p.cfg.PlusUsageConfig.CASecretName || From 3633d5085eb30a3eb8802e30dfabbc491cfdf296 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Mon, 4 Aug 2025 16:10:48 -0700 Subject: [PATCH 09/15] Add more checks around dataplaneKeySecretName --- internal/controller/provisioner/eventloop.go | 5 ++++- internal/controller/provisioner/handler.go | 2 -- internal/controller/provisioner/provisioner.go | 11 ++++++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/controller/provisioner/eventloop.go b/internal/controller/provisioner/eventloop.go index c7fb485f20..983251156c 100644 --- a/internal/controller/provisioner/eventloop.go +++ b/internal/controller/provisioner/eventloop.go @@ -40,7 +40,10 @@ func newEventLoop( secretsToWatch := make([]string, 0, len(dockerSecrets)+5) secretsToWatch = append(secretsToWatch, agentTLSSecret) secretsToWatch = append(secretsToWatch, dockerSecrets...) - secretsToWatch = append(secretsToWatch, dataplaneKeySecret) + + if dataplaneKeySecret != "" { + secretsToWatch = append(secretsToWatch, dataplaneKeySecret) + } if usageConfig != nil { if usageConfig.SecretName != "" { diff --git a/internal/controller/provisioner/handler.go b/internal/controller/provisioner/handler.go index 1436be7fc4..e3327cab68 100644 --- a/internal/controller/provisioner/handler.go +++ b/internal/controller/provisioner/handler.go @@ -124,12 +124,10 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, } case *corev1.Secret: if h.provisioner.isUserSecret(e.NamespacedName.Name) { - fmt.Println("This should be a user secret") if err := h.deprovisionSecretsForAllGateways(ctx, e.NamespacedName.Name); err != nil { logger.Error(err, "error removing secrets") } } else { - fmt.Println("aparently we are reprovisioning") if err := h.reprovisionResources(ctx, e); err != nil { logger.Error(err, "error re-provisioning nginx resources") } diff --git a/internal/controller/provisioner/provisioner.go b/internal/controller/provisioner/provisioner.go index 6e55a7e623..6ebc5dc178 100644 --- a/internal/controller/provisioner/provisioner.go +++ b/internal/controller/provisioner/provisioner.go @@ -106,13 +106,18 @@ func NewNginxProvisioner( clientSSLSecretName = cfg.PlusUsageConfig.ClientSSLSecretName } + var dataplaneKeySecretName string + if cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName != "" { + dataplaneKeySecretName = cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName + } + store := newStore( cfg.NginxDockerSecretNames, cfg.AgentTLSSecretName, jwtSecretName, caSecretName, clientSSLSecretName, - cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, + dataplaneKeySecretName, ) selector := metav1.LabelSelector{ @@ -133,7 +138,7 @@ func NewNginxProvisioner( agentLabelCollector := labelCollectorFactory(mgr, cfg) agentLabels, err := agentLabelCollector.Collect(ctx) if err != nil { - return nil, nil, fmt.Errorf("failed to collect agent labels: %w", err) + cfg.Logger.Error(err, "failed to collect agent labels") } cfg.AgentLabels = agentLabels @@ -160,7 +165,7 @@ func NewNginxProvisioner( cfg.GatewayPodConfig.Namespace, cfg.NginxDockerSecretNames, cfg.AgentTLSSecretName, - cfg.NginxOneConsoleTelemetryConfig.DataplaneKeySecretName, + dataplaneKeySecretName, cfg.PlusUsageConfig, isOpenshift, ) From d540c509da974fadc3b668ae27a0f25e152a5d49 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 11:56:17 -0700 Subject: [PATCH 10/15] Finish adding objects and agent labels tests --- .../controller/provisioner/objects_test.go | 164 +++++++++++++ .../controller/telemetry/agent_labels_test.go | 226 ++++++++++++++++++ .../controller/telemetry/collector_test.go | 4 +- 3 files changed, 391 insertions(+), 3 deletions(-) create mode 100644 internal/controller/telemetry/agent_labels_test.go diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go index 247e20ba0d..6b1b86fe40 100644 --- a/internal/controller/provisioner/objects_test.go +++ b/internal/controller/provisioner/objects_test.go @@ -785,6 +785,86 @@ func TestBuildNginxResourceObjects_OpenShift(t *testing.T) { g.Expect(roleBinding.GetLabels()).To(Equal(expLabels)) } +func TestBuildNginxResourceObjects_DataplaneKeySecret(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + agentTLSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: agentTLSTestSecretName, + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"tls.crt": []byte("tls")}, + } + dataplaneKeySecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dataplane-key-secret", + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"dataplane.key": []byte("keydata")}, + } + fakeClient := fake.NewFakeClient(agentTLSSecret, dataplaneKeySecret) + + dataplaneKeySecretName := "dataplane-key-secret" //nolint:gosec // not credentials + + provisioner := &NginxProvisioner{ + cfg: Config{ + GatewayPodConfig: &config.GatewayPodConfig{ + Namespace: ngfNamespace, + }, + AgentTLSSecretName: agentTLSTestSecretName, + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: dataplaneKeySecretName, + EndpointHost: "my.endpoint.com", + EndpointPort: 443, + EndpointTLSSkipVerify: false, + }, + }, + k8sClient: fakeClient, + baseLabelSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + }, + } + + gateway := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "default", + }, + } + + resourceName := "gw-nginx" + objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, &graph.EffectiveNginxProxy{}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(objects).To(HaveLen(7)) // 2 secrets, 2 configmaps, serviceaccount, service, deployment + + // Find the dataplane key secret + var found bool + for _, obj := range objects { + if s, ok := obj.(*corev1.Secret); ok { + if s.GetName() == controller.CreateNginxResourceName(resourceName, dataplaneKeySecretName) { + found = true + g.Expect(s.Data).To(HaveKey("dataplane.key")) + g.Expect(s.Data["dataplane.key"]).To(Equal([]byte("keydata"))) + } + } + } + g.Expect(found).To(BeTrue()) + + // Check deployment mounts the secret + dep, ok := objects[6].(*appsv1.Deployment) + g.Expect(ok).To(BeTrue()) + g.Expect(dep).ToNot(BeNil()) + container := dep.Spec.Template.Spec.Containers[0] + g.Expect(container.VolumeMounts).To(ContainElement(corev1.VolumeMount{ + Name: "agent-dataplane-key", + MountPath: "/etc/nginx-agent/secrets/dataplane.key", + SubPath: "dataplane.key", + })) +} + func TestGetAndUpdateSecret_NotFound(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -989,6 +1069,50 @@ func TestBuildNginxResourceObjectsForDeletion_OpenShift(t *testing.T) { validateMeta(roleBinding, deploymentNSName.Name) } +func TestBuildNginxResourceObjectsForDeletion_DataplaneKeySecret(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + dataplaneKeySecretName := "dataplane-key-secret" //nolint:gosec // not credentials + + provisioner := &NginxProvisioner{ + cfg: Config{ + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: dataplaneKeySecretName, + }, + AgentTLSSecretName: agentTLSTestSecretName, + }, + } + + deploymentNSName := types.NamespacedName{ + Name: "gw-nginx", + Namespace: "default", + } + + objects := provisioner.buildNginxResourceObjectsForDeletion(deploymentNSName) + + // Should include the dataplane key secret in the objects list + // Default: deployment, daemonset, service, serviceaccount, 2 configmaps, agentTLSSecret, dataplaneKeySecret + g.Expect(objects).To(HaveLen(8)) + + validateMeta := func(obj client.Object, name string) { + g.Expect(obj.GetName()).To(Equal(name)) + g.Expect(obj.GetNamespace()).To(Equal(deploymentNSName.Namespace)) + } + + // Validate the dataplane key secret is present + found := false + for _, obj := range objects { + if s, ok := obj.(*corev1.Secret); ok { + if s.GetName() == controller.CreateNginxResourceName(deploymentNSName.Name, dataplaneKeySecretName) { + validateMeta(s, controller.CreateNginxResourceName(deploymentNSName.Name, dataplaneKeySecretName)) + found = true + } + } + } + g.Expect(found).To(BeTrue()) +} + func TestSetIPFamily(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -1070,6 +1194,46 @@ func TestBuildNginxConfigMaps_WorkerConnections(t *testing.T) { g.Expect(bootstrapCM.Data["main.conf"]).To(ContainSubstring("worker_connections 2048;")) } +func TestBuildNginxConfigMaps_AgentFields(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + provisioner := &NginxProvisioner{ + cfg: Config{ + GatewayPodConfig: &config.GatewayPodConfig{ + Namespace: "default", + ServiceName: "test-service", + }, + AgentLabels: map[string]string{ + "key1": "val1", + "key2": "val2", + }, + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: "dataplane-key-secret", + EndpointHost: "console.example.com", + EndpointPort: 443, + EndpointTLSSkipVerify: false, + }, + }, + } + objectMeta := metav1.ObjectMeta{Name: "test", Namespace: "default"} + + nProxyCfgEmpty := &graph.EffectiveNginxProxy{} + + configMaps := provisioner.buildNginxConfigMaps(objectMeta, nProxyCfgEmpty, "test-bootstrap", "test-agent", true, true) + g.Expect(configMaps).To(HaveLen(2)) + + agentCM, ok := configMaps[1].(*corev1.ConfigMap) + g.Expect(ok).To(BeTrue()) + data := agentCM.Data["nginx-agent.conf"] + + g.Expect(data).To(ContainSubstring("key1: val1")) + g.Expect(data).To(ContainSubstring("key2: val2")) + g.Expect(data).To(ContainSubstring("host: console.example.com")) + g.Expect(data).To(ContainSubstring("port: 443")) + g.Expect(data).To(ContainSubstring("skip_verify: false")) +} + func TestBuildReadinessProbe(t *testing.T) { t.Parallel() diff --git a/internal/controller/telemetry/agent_labels_test.go b/internal/controller/telemetry/agent_labels_test.go new file mode 100644 index 0000000000..de61c21029 --- /dev/null +++ b/internal/controller/telemetry/agent_labels_test.go @@ -0,0 +1,226 @@ +package telemetry_test + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/telemetry" + "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kubernetes/kubernetesfakes" +) + +func TestCollect_Success(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + ngfPod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-pod", + Namespace: "nginx-gateway", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + }, + } + + replicas := int32(1) + ngfReplicaSet := &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: &replicas, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "replica", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "Deployment1", + UID: "test-uid-replicaSet", + }, + }, + }, + } + + kubeNamespace := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: metav1.NamespaceSystem, + UID: "test-uid", + }, + } + + k8sClientReader := &kubernetesfakes.FakeReader{} + + cfg := telemetry.LabelCollectorConfig{ + K8sClientReader: k8sClientReader, + Version: "my-version", + PodNSName: types.NamespacedName{ + Name: "ngf-pod", + Namespace: "nginx-gateway", + }, + } + + baseGetCalls := createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) + k8sClientReader.GetCalls(baseGetCalls) + + c := telemetry.NewLabelCollector(cfg) + labels, err := c.Collect(context.TODO()) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(labels).To(Equal(map[string]string{ + "product-type": "ngf", + "product-version": "my-version", + "cluster-id": "test-uid", + "control-name": "ngf-pod", + "control-namespace": "nginx-gateway", + "control-id": "test-uid-replicaSet", + })) +} + +func TestCollect_Errors(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + ngfPod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-pod", + Namespace: "nginx-gateway", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + }, + } + + replicas := int32(1) + ngfReplicaSet := &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: &replicas, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "replica", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "Deployment1", + UID: "test-uid-replicaSet", + }, + }, + }, + } + + kubeNamespace := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: metav1.NamespaceSystem, + UID: "test-uid", + }, + } + + baseGetCalls := createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) + + mergeGetCallsWithBase := func(f getCallsFunc) getCallsFunc { + return func( + ctx context.Context, + nsName types.NamespacedName, + object client.Object, + option ...client.GetOption, + ) error { + err := baseGetCalls(ctx, nsName, object, option...) + g.Expect(err).ToNot(HaveOccurred()) + + return f(ctx, nsName, object, option...) + } + } + + tests := []struct { + name string + getCallsFunc getCallsFunc + wantErrContain string + }{ + { + name: "collectClusterID error", + getCallsFunc: mergeGetCallsWithBase(func( + _ context.Context, + _ types.NamespacedName, + object client.Object, + _ ...client.GetOption, + ) error { + if _, ok := object.(*v1.Namespace); ok { + return errors.New("clusterID fail") + } + return nil + }), + wantErrContain: "failed to collect cluster information", + }, + { + name: "getPodReplicaSet error", + getCallsFunc: mergeGetCallsWithBase(func( + _ context.Context, + _ types.NamespacedName, + object client.Object, + _ ...client.GetOption, + ) error { + if _, ok := object.(*appsv1.ReplicaSet); ok { + return errors.New("replicaSet fail") + } + return nil + }), + wantErrContain: "failed to get replica set for pod", + }, + { + name: "getDeploymentID error", + getCallsFunc: mergeGetCallsWithBase(createGetCallsFunc(&appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: &replicas, + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Name: "replica", + Kind: "Deployment", + }, + }, + }, + })), + wantErrContain: "failed to get NGF deploymentID", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + getCalls := tt.getCallsFunc + + k8sClientReader := &kubernetesfakes.FakeReader{} + k8sClientReader.GetCalls(getCalls) + + cfg := telemetry.LabelCollectorConfig{ + K8sClientReader: k8sClientReader, + Version: "my-version", + PodNSName: types.NamespacedName{ + Name: "ngf-pod", + Namespace: "nginx-gateway", + }, + } + + c := telemetry.NewLabelCollector(cfg) + + labels, err := c.Collect(context.TODO()) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrContain)) + g.Expect(labels).To(BeNil()) + }) + } +} diff --git a/internal/controller/telemetry/collector_test.go b/internal/controller/telemetry/collector_test.go index 850b5756be..ea90045032 100644 --- a/internal/controller/telemetry/collector_test.go +++ b/internal/controller/telemetry/collector_test.go @@ -59,9 +59,7 @@ type getCallsFunc = func( ) error func createGetCallsFunc(objects ...client.Object) getCallsFunc { - return func(_ context.Context, _ types.NamespacedName, object client.Object, option ...client.GetOption) error { - Expect(option).To(BeEmpty()) - + return func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { for _, obj := range objects { if reflect.TypeOf(obj) == reflect.TypeOf(object) { reflect.ValueOf(object).Elem().Set(reflect.ValueOf(obj).Elem()) From 47a5ff262fb0b2b526e14c5ff0f039d66dc54878 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 12:06:23 -0700 Subject: [PATCH 11/15] Fix indentation --- internal/controller/provisioner/templates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index 4b921a7744..c6eb828377 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -12,7 +12,7 @@ const mainTemplateText = ` error_log stderr {{ .ErrorLevel }}; events { - worker_connections {{ .WorkerConnections }}; + worker_connections {{ .WorkerConnections }}; }` const mgmtTemplateText = `mgmt { From a10e389561be5cee77079495c04652ead083fdf9 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 12:16:25 -0700 Subject: [PATCH 12/15] Add more store.go tests --- internal/controller/provisioner/store_test.go | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/internal/controller/provisioner/store_test.go b/internal/controller/provisioner/store_test.go index c38da7d7fe..b9ccb68b63 100644 --- a/internal/controller/provisioner/store_test.go +++ b/internal/controller/provisioner/store_test.go @@ -327,6 +327,22 @@ func TestRegisterResourceInGatewayConfig(t *testing.T) { // Docker Secret again, already exists resources = registerAndGetResources(dockerSecret) g.Expect(resources.DockerSecrets).To(ContainElement(dockerSecretMeta)) + + // clear out resources before next test + store.deleteResourcesForGateway(nsName) + + // Dataplane Key Secret + dataplaneKeySecretMeta := metav1.ObjectMeta{ + Name: controller.CreateNginxResourceName(defaultMeta.Name, store.dataplaneKeySecretName), + Namespace: defaultMeta.Namespace, + } + dataplaneKeySecret := &corev1.Secret{ObjectMeta: dataplaneKeySecretMeta} + resources = registerAndGetResources(dataplaneKeySecret) + g.Expect(resources.DataplaneKeySecret).To(Equal(dataplaneKeySecretMeta)) + + // Dataplane Key Secret again, already exists + resources = registerAndGetResources(dataplaneKeySecret) + g.Expect(resources.DataplaneKeySecret).To(Equal(dataplaneKeySecretMeta)) } func TestGatewayChanged(t *testing.T) { @@ -500,6 +516,10 @@ func TestGatewayExistsForResource(t *testing.T) { Namespace: "default", }, }, + DataplaneKeySecret: metav1.ObjectMeta{ + Name: "test-dataplane-key-secret", + Namespace: "default", + }, } tests := []struct { @@ -637,6 +657,16 @@ func TestGatewayExistsForResource(t *testing.T) { }, expected: gateway, }, + { + name: "Dataplane Key Secret exists", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dataplane-key-secret", + Namespace: "default", + }, + }, + expected: gateway, + }, { name: "Resource does not exist", object: &corev1.Service{ @@ -733,6 +763,11 @@ func TestGetResourceVersionForObject(t *testing.T) { ResourceVersion: "13", }, }, + DataplaneKeySecret: metav1.ObjectMeta{ + Name: "test-dataplane-key-secret", + Namespace: "default", + ResourceVersion: "14", + }, } tests := []struct { @@ -870,6 +905,16 @@ func TestGetResourceVersionForObject(t *testing.T) { }, expectedResult: "13", }, + { + name: "Dataplane Key Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dataplane-key-secret", + Namespace: "default", + }, + }, + expectedResult: "14", + }, { name: "Non-existent resource", object: &corev1.Service{ From d90a5dc15bdd7491ecf320622268321246c4ef57 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 12:45:16 -0700 Subject: [PATCH 13/15] Add another test --- .../provisioner/provisioner_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index 07590e65de..f727d3e2e8 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -25,6 +25,7 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/provisioner/openshift/openshiftfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" + "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller/controllerfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -558,3 +559,21 @@ func TestProvisionerRestartsDaemonSet(t *testing.T) { g.Expect(fakeClient.Get(context.TODO(), key, ds)).To(Succeed()) g.Expect(ds.Spec.Template.GetAnnotations()).To(HaveKey(controller.RestartedAnnotation)) } + +func TestDefaultLabelCollectorFactory(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + mgr := &controllerfakes.FakeManager{} + + cfg := Config{ + GatewayPodConfig: &config.GatewayPodConfig{ + Namespace: "pod-namespace", + Name: "pod-name", + Version: "my-version", + }, + } + + collector := defaultLabelCollectorFactory(mgr, cfg) + g.Expect(collector).NotTo(BeNil()) +} From 21c8dbc0967ca0d984a6d82835fa71870c71b922 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 12:52:45 -0700 Subject: [PATCH 14/15] Add another test for coverage --- internal/controller/provisioner/provisioner_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/controller/provisioner/provisioner_test.go b/internal/controller/provisioner/provisioner_test.go index f727d3e2e8..1e1842b000 100644 --- a/internal/controller/provisioner/provisioner_test.go +++ b/internal/controller/provisioner/provisioner_test.go @@ -224,6 +224,9 @@ func TestNewNginxProvisioner(t *testing.T) { InstanceName: "test-instance", }, Logger: logr.Discard(), + NginxOneConsoleTelemetryConfig: config.NginxOneConsoleTelemetryConfig{ + DataplaneKeySecretName: "dataplane-key", + }, } apiChecker = &openshiftfakes.FakeAPIChecker{} @@ -243,6 +246,8 @@ func TestNewNginxProvisioner(t *testing.T) { }, } g.Expect(provisioner.baseLabelSelector).To(Equal(labelSelector)) + + g.Expect(provisioner.store.dataplaneKeySecretName).To(Equal("dataplane-key")) } func TestEnable(t *testing.T) { From 4a2262bff4991f24e327a4a937e437b867256086 Mon Sep 17 00:00:00 2001 From: Ben Jee Date: Tue, 5 Aug 2025 14:05:11 -0700 Subject: [PATCH 15/15] Add some feedback review --- charts/nginx-gateway-fabric/README.md | 7 ++++--- charts/nginx-gateway-fabric/templates/deployment.yaml | 2 +- charts/nginx-gateway-fabric/values.schema.json | 4 ++-- charts/nginx-gateway-fabric/values.yaml | 4 ++-- cmd/gateway/commands.go | 7 ++++--- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index 4408363566..d740c649eb 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -264,7 +264,7 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `certGenerator.ttlSecondsAfterFinished` | How long to wait after the cert generator job has finished before it is removed by the job controller. | int | `30` | | `clusterDomain` | The DNS cluster domain of your Kubernetes cluster. | string | `"cluster.local"` | | `gateways` | A list of Gateway objects. View https://gateway-api.sigs.k8s.io/reference/spec/#gateway for full Gateway reference. | list | `[]` | -| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","nginxOneConsole":{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false},"plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | +| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","nginxOneConsole":{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"skipVerify":false},"plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | | `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{"hostPorts":[],"lifecycle":{},"readinessProbe":{},"resources":{},"volumeMounts":[]}` | | `nginx.container.hostPorts` | A list of HostPorts to expose on the host. This configuration allows containers to bind to a specific port on the host node, enabling external network traffic to reach the container directly through the host's IP address and port. Use this option when you need to expose container ports on the host for direct access, such as for debugging, legacy integrations, or when NodePort/LoadBalancer services are not suitable. Note: Using hostPort may have security and scheduling implications, as it ties pods to specific nodes and ports. | list | `[]` | @@ -276,10 +276,11 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `nginx.imagePullSecret` | The name of the secret containing docker registry credentials. Secret must exist in the same namespace as the helm release. The control plane will copy this secret into any namespace where NGINX is deployed. | string | `""` | | `nginx.imagePullSecrets` | A list of secret names containing docker registry credentials. Secrets must exist in the same namespace as the helm release. The control plane will copy these secrets into any namespace where NGINX is deployed. | list | `[]` | | `nginx.kind` | The kind of NGINX deployment. | string | `"deployment"` | -| `nginx.nginxOneConsole` | Configuration for NGINX One Console. | object | `{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"tlsSkipVerify":false}` | +| `nginx.nginxOneConsole` | Configuration for NGINX One Console. | object | `{"dataplaneKeySecretName":"","endpointHost":"agent.connect.nginx.com","endpointPort":443,"skipVerify":false}` | +| `nginx.nginxOneConsole.dataplaneKeySecretName` | Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console. Secret must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` | | `nginx.nginxOneConsole.endpointHost` | The Endpoint host that the NGINX One Console telemetry metrics will be sent to. | string | `"agent.connect.nginx.com"` | | `nginx.nginxOneConsole.endpointPort` | The endpoint port that the NGINX One Console telemetry metrics will be sent to. | int | `443` | -| `nginx.nginxOneConsole.tlsSkipVerify` | Skip TLS verification for NGINX One Console connections. | bool | `false` | +| `nginx.nginxOneConsole.skipVerify` | Skip TLS verification for NGINX One Console connections. | bool | `false` | | `nginx.plus` | Is NGINX Plus image being used. | bool | `false` | | `nginx.pod` | The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.replicas` | The number of replicas of the NGINX Deployment. | int | `1` | diff --git a/charts/nginx-gateway-fabric/templates/deployment.yaml b/charts/nginx-gateway-fabric/templates/deployment.yaml index 6f9aec008b..9be1b13f16 100644 --- a/charts/nginx-gateway-fabric/templates/deployment.yaml +++ b/charts/nginx-gateway-fabric/templates/deployment.yaml @@ -111,7 +111,7 @@ spec: {{- if .Values.nginx.nginxOneConsole.endpointPort }} - --nginx-one-telemetry-endpoint-port={{ .Values.nginx.nginxOneConsole.endpointPort }} {{- end }} - {{- if .Values.nginx.nginxOneConsole.tlsSkipVerify }} + {{- if .Values.nginx.nginxOneConsole.skipVerify }} - --nginx-one-tls-skip-verify {{- end }} {{- end }} diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index 89fa0ff8fa..1661185878 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -471,11 +471,11 @@ "title": "endpointPort", "type": "integer" }, - "tlsSkipVerify": { + "skipVerify": { "default": false, "description": "Skip TLS verification for NGINX One Console connections.", "required": [], - "title": "tlsSkipVerify", + "title": "skipVerify", "type": "boolean" } }, diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index 41ebf23526..a3c0bdcc55 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -214,7 +214,7 @@ nginx: # -- Configuration for NGINX One Console. nginxOneConsole: - # Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console. + # -- Name of the secret which holds the dataplane key that is required to authenticate with the NGINX One Console. # Secret must exist in the same namespace that the NGINX Gateway Fabric control plane is running in # (default namespace: nginx-gateway). dataplaneKeySecretName: "" @@ -231,7 +231,7 @@ nginx: endpointPort: 443 # -- Skip TLS verification for NGINX One Console connections. - tlsSkipVerify: false + skipVerify: false # -- The name of the secret containing docker registry credentials. # Secret must exist in the same namespace as the helm release. The control diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index dcbb240d0a..9f83cbcfb3 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -37,8 +37,9 @@ const ( `The controller name must be of the form: DOMAIN/PATH. The controller's domain is '%s'` plusFlag = "nginx-plus" - serverTLSSecret = "server-tls" - agentTLSSecret = "agent-tls" + serverTLSSecret = "server-tls" + agentTLSSecret = "agent-tls" + nginxOneTelemetryEndpointHost = "agent.connect.nginx.com" ) func createRootCommand() *cobra.Command { @@ -109,7 +110,7 @@ func createControllerCommand() *cobra.Command { } nginxOneConsoleTelemetryEndpointHost = stringValidatingValue{ validator: validateResourceName, - value: "agent.connect.nginx.com", + value: nginxOneTelemetryEndpointHost, } nginxOneConsoleTelemetryEndpointPort = intValidatingValue{ validator: validateAnyPort,