Code review

Author: sjberman

cmd/gateway/commands.go

@@ -186,7 +186,6 @@ func createStaticModeCommand() *cobra.Command {
 				GatewayClassName:         gatewayClassName.value,
 				GatewayNsName:            gwNsName,
 				UpdateGatewayClassStatus: updateGCStatus,
-				DisableProductTelemetry:  disableProductTelemetry,
 				GatewayPodConfig: config.GatewayPodConfig{
 					PodIP:       podIP,
 					ServiceName: serviceName.value,
@@ -207,12 +206,15 @@ func createStaticModeCommand() *cobra.Command {
 					LockName: leaderElectionLockName.String(),
 					Identity: podName,
 				},
-				UsageReportConfig:     usageReportConfig,
-				Plus:                  plus,
-				TelemetryReportPeriod: period,
-				Version:               version,
-				ExperimentalFeatures:  gwExperimentalFeatures,
-				ImageSource:           imageSource,
+				UsageReportConfig: usageReportConfig,
+				ProductTelemetryConfig: config.ProductTelemetryConfig{
+					TelemetryReportPeriod:   period,
+					DisableProductTelemetry: disableProductTelemetry,
+				},
+				Plus:                 plus,
+				Version:              version,
+				ExperimentalFeatures: gwExperimentalFeatures,
+				ImageSource:          imageSource,
 			}
 
 			if err := static.StartManager(conf); err != nil {
@@ -323,7 +325,7 @@ func createStaticModeCommand() *cobra.Command {
 		&disableProductTelemetry,
 		productTelemetryDisableFlag,
 		false,
-		"Disable the collection of product telemetry every 24 hours.",
+		"Disable the collection of product telemetry.",
 	)
 
 	cmd.Flags().BoolVar(

deploy/helm-chart/README.md

@@ -294,7 +294,7 @@ The following tables lists the configurable parameters of the NGINX Gateway Fabr
 | `nginxGateway.leaderElection.enable`              | Enable leader election. Leader election is used to avoid multiple replicas of the NGINX Gateway Fabric reporting the status of the Gateway API resources.                                                | true                                                                                                            |
 | `nginxGateway.leaderElection.lockName`            | The name of the leader election lock. A Lease object with this name will be created in the same Namespace as the controller.                                                                             | Autogenerated                                                                                                   |
 | `nginxGateway.securityContext.allowPrivilegeEscalation` | Some environments may need this set to true in order for the control plane to successfully reload NGINX. | false |
-| `nginxGateway.productTelemetry.enable`            | Enable the collection of product telemetry every 24 hours. | true |
+| `nginxGateway.productTelemetry.enable`            | Enable the collection of product telemetry. | true |
 | `nginxGateway.gwAPIExperimentalFeatures.enable`   | Enable the experimental features of Gateway API which are supported by NGINX Gateway Fabric. Requires the Gateway APIs installed from the experimental channel. | false |
 | `nginx.image.repository`                          | The repository for the NGINX image.                                                                                                                                                                      | ghcr.io/nginxinc/nginx-gateway-fabric/nginx                                                                     |
 | `nginx.image.tag`                                 | The tag for the NGINX image.                                                                                                                                                                             | edge                                                                                                            |

deploy/helm-chart/templates/rbac.yaml

@@ -39,25 +39,34 @@ rules:
   - get
   - list
   - watch
-{{- if or .Values.nginxGateway.productTelemetry.enable .Values.nginx.plus }}
+{{- if .Values.nginxGateway.productTelemetry.enable }}
 - apiGroups:
   - ""
   resources:
   - pods
   verbs:
   - get
 - apiGroups:
-  - ""
+  - apps
   resources:
-  - nodes
+  - replicasets
   verbs:
-  - list
+  - get
+{{- end }}
+{{- if .Values.nginx.plus }}
 - apiGroups:
   - apps
   resources:
   - replicasets
   verbs:
-  - get
+  - list
+{{- end }}
+{{- if or .Values.nginxGateway.productTelemetry.enable .Values.nginx.plus }}
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
   - list
 {{- end }}
 - apiGroups:

deploy/helm-chart/values.yaml

@@ -46,7 +46,7 @@ nginxGateway:
     allowPrivilegeEscalation: false
 
   productTelemetry:
-    ## Enable the collection of product telemetry every 24 hours.
+    ## Enable the collection of product telemetry.
     enable: true
 
   ## The lifecycle of the nginx-gateway container.

deploy/manifests/nginx-gateway-experimental.yaml

@@ -43,18 +43,17 @@ rules:
   - pods
   verbs:
   - get
-- apiGroups:
-  - ""
-  resources:
-  - nodes
-  verbs:
-  - list
 - apiGroups:
   - apps
   resources:
   - replicasets
   verbs:
   - get
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
   - list
 - apiGroups:
   - ""

deploy/manifests/nginx-gateway.yaml

@@ -42,18 +42,17 @@ rules:
   - pods
   verbs:
   - get
-- apiGroups:
-  - ""
-  resources:
-  - nodes
-  verbs:
-  - list
 - apiGroups:
   - apps
   resources:
   - replicasets
   verbs:
   - get
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
   - list
 - apiGroups:
   - ""

deploy/manifests/nginx-plus-gateway-experimental.yaml

@@ -44,17 +44,22 @@ rules:
   verbs:
   - get
 - apiGroups:
-  - ""
+  - apps
   resources:
-  - nodes
+  - replicasets
   verbs:
-  - list
+  - get
 - apiGroups:
   - apps
   resources:
   - replicasets
   verbs:
-  - get
+  - list
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
   - list
 - apiGroups:
   - ""

deploy/manifests/nginx-plus-gateway.yaml

@@ -43,17 +43,22 @@ rules:
   verbs:
   - get
 - apiGroups:
-  - ""
+  - apps
   resources:
-  - nodes
+  - replicasets
   verbs:
-  - list
+  - get
 - apiGroups:
   - apps
   resources:
   - replicasets
   verbs:
-  - get
+  - list
+- apiGroups:
+  - ""
+  resources:
+  - nodes
+  verbs:
   - list
 - apiGroups:
   - ""

internal/mode/static/config/config.go

@@ -36,12 +36,10 @@ type Config struct {
 	MetricsConfig MetricsConfig
 	// HealthConfig specifies the health probe config.
 	HealthConfig HealthConfig
-	// TelemetryReportPeriod is the period at which telemetry reports are sent.
-	TelemetryReportPeriod time.Duration
+	// ProductTelemetryConfig contains the configuration for collecting product telemetry.
+	ProductTelemetryConfig ProductTelemetryConfig
 	// UpdateGatewayClassStatus enables updating the status of the GatewayClass resource.
 	UpdateGatewayClassStatus bool
-	// DisableProductTelemetry disables the collection of product telemetry.
-	DisableProductTelemetry bool
 	// Plus indicates whether NGINX Plus is being used.
 	Plus bool
 	// ExperimentalFeatures indicates if experimental features are enabled.
@@ -88,6 +86,14 @@ type LeaderElectionConfig struct {
 	Enabled bool
 }
 
+// ProductTelemetryConfig contains the configuration for collecting product telemetry.
+type ProductTelemetryConfig struct {
+	// TelemetryReportPeriod is the period at which telemetry reports are sent.
+	TelemetryReportPeriod time.Duration
+	// DisableProductTelemetry disables the collection of product telemetry.
+	DisableProductTelemetry bool
+}
+
 // UsageReportConfig contains the configuration for NGINX Plus usage reporting.
 type UsageReportConfig struct {
 	// SecretNsName is the namespaced name of the Secret containing the server credentials.

internal/mode/static/manager.go

@@ -242,7 +242,7 @@ func StartManager(cfg config.Config) error {
 		return fmt.Errorf("cannot register status updater: %w", err)
 	}
 
-	if !cfg.DisableProductTelemetry {
+	if !cfg.ProductTelemetryConfig.DisableProductTelemetry {
 		dataCollector := telemetry.NewDataCollectorImpl(telemetry.DataCollectorConfig{
 			K8sClientReader:     mgr.GetAPIReader(),
 			GraphGetter:         processor,
@@ -475,7 +475,7 @@ func createTelemetryJob(
 			runnables.CronJobConfig{
 				Worker:       telemetry.CreateTelemetryJobWorker(logger, exporter, dataCollector),
 				Logger:       logger,
-				Period:       cfg.TelemetryReportPeriod,
+				Period:       cfg.ProductTelemetryConfig.TelemetryReportPeriod,
 				JitterFactor: telemetryJitterFactor,
 				ReadyCh:      readyCh,
 			},
@@ -503,7 +503,7 @@ func createUsageReporterJob(
 		Runnable: runnables.NewCronJob(runnables.CronJobConfig{
 			Worker:       usage.CreateUsageJobWorker(logger, k8sClient, reporter, cfg),
 			Logger:       logger,
-			Period:       cfg.TelemetryReportPeriod,
+			Period:       cfg.ProductTelemetryConfig.TelemetryReportPeriod,
 			JitterFactor: telemetryJitterFactor,
 			ReadyCh:      readyCh,
 		}),

site/content/reference/cli-help.md

@@ -36,7 +36,7 @@ _Usage_:
 | _health-port_                       | _int_    | Set the port where the health probe server is exposed. An integer between 1024 - 65535 (Default: `8081`).                                                                                                                                                                                                                                                                                |
 | _leader-election-disable_           | _bool_   | Disable leader election, which is used to avoid multiple replicas of the NGINX Gateway Fabric reporting the status of the Gateway API resources. If disabled, all replicas of NGINX Gateway Fabric will update the statuses of the Gateway API resources (Default: `false`).                                                                                                             |
 | _leader-election-lock-name_         | _string_ | The name of the leader election lock. A lease object with this name will be created in the same namespace as the controller (Default: `"nginx-gateway-leader-election-lock"`).                                                                                                                                                                                                           |
-| _product-telemetry-disable_  | _bool_   | Disable the collection of product telemetry every 24 hours (Default: `false`). |
+| _product-telemetry-disable_  | _bool_   | Disable the collection of product telemetry (Default: `false`). |
 | _usage-report-secret_        | _string_ | The namespace/name of the Secret containing the credentials for NGINX Plus usage reporting. |
 | _usage-report-server-url_    | _string_ | The base server URL of the NGINX Plus usage reporting server. |
 | _usage-report-cluster-name_  | _string_ | The display name of the Kubernetes cluster in the NGINX Plus usage reporting server. |