Code review; validate secrets

Author: sjberman

charts/nginx-gateway-fabric/README.md

@@ -268,8 +268,8 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri
 | `nginx.image.tag` |  | string | `"edge"` |
 | `nginx.lifecycle` | The lifecycle of the nginx container. | object | `{}` |
 | `nginx.plus` | Is NGINX Plus image being used | bool | `false` |
-| `nginx.usage.caSecretName` | The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
-| `nginx.usage.clientSSLSecretName` | The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
+| `nginx.usage.caSecretName` | The name of the Secret containing the NGINX Instance Manager CA certificate. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
+| `nginx.usage.clientSSLSecretName` | The name of the Secret containing the client certificate and key for authenticating with NGINX Instance Manager. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
 | `nginx.usage.endpoint` | The endpoint of the NGINX Plus usage reporting server. Default: product.connect.nginx.com | string | `""` |
 | `nginx.usage.resolver` | The nameserver used to resolve the NGINX Plus usage reporting endpoint. Used with NGINX Instance Manager. | string | `""` |
 | `nginx.usage.secretName` | The name of the Secret containing the JWT for NGINX Plus usage reporting. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `"nplus-license"` |

charts/nginx-gateway-fabric/values.schema.json

@@ -264,14 +264,14 @@
           "properties": {
             "caSecretName": {
               "default": "",
-              "description": "The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
+              "description": "The name of the Secret containing the NGINX Instance Manager CA certificate.\nMust exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
               "required": [],
               "title": "caSecretName",
               "type": "string"
             },
             "clientSSLSecretName": {
               "default": "",
-              "description": "The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
+              "description": "The name of the Secret containing the client certificate and key for authenticating with NGINX Instance Manager.\nMust exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
               "required": [],
               "title": "clientSSLSecretName",
               "type": "string"

charts/nginx-gateway-fabric/values.yaml

@@ -149,12 +149,12 @@ nginx:
     # -- Disable client verification of the NGINX Plus usage reporting server certificate.
     skipVerify: false
 
-    # -- The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting
-    # server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
+    # -- The name of the Secret containing the NGINX Instance Manager CA certificate.
+    # Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
     caSecretName: ""
 
-    # -- The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting
-    # server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
+    # -- The name of the Secret containing the client certificate and key for authenticating with NGINX Instance Manager.
+    # Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
     clientSSLSecretName: ""
 
   # @schema

cmd/gateway/commands.go

@@ -47,6 +47,7 @@ func createRootCommand() *cobra.Command {
 	return rootCmd
 }
 
+//nolint:gocyclo
 func createStaticModeCommand() *cobra.Command {
 	// flag names
 	const (
@@ -200,6 +201,10 @@ func createStaticModeCommand() *cobra.Command {
 			}
 
 			var usageReportConfig config.UsageReportConfig
+			if plus && usageReportSecretName.value == "" {
+				return errors.New("usage-report-secret is required when using NGINX Plus")
+			}
+
 			if plus {
 				usageReportConfig = config.UsageReportConfig{
 					SecretName:          usageReportSecretName.value,
@@ -392,8 +397,6 @@ func createStaticModeCommand() *cobra.Command {
 			"that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
 	)
 
-	cmd.MarkFlagsRequiredTogether(plusFlag, usageReportSecretFlag)
-
 	cmd.Flags().Var(
 		&usageReportEndpoint,
 		usageReportEndpointFlag,
@@ -416,16 +419,16 @@ func createStaticModeCommand() *cobra.Command {
 	cmd.Flags().Var(
 		&usageReportClientSSLSecretName,
 		usageReportClientSSLSecretFlag,
-		"The name of the Secret containing the client cert/key for communicating with the NGINX Plus usage reporting "+
-			"server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in "+
+		"The name of the Secret containing the client certificate and key for authenticating with NGINX Instance Manager. "+
+			"Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in "+
 			"(default namespace: nginx-gateway).",
 	)
 
 	cmd.Flags().Var(
 		&usageReportCASecretName,
 		usageReportCASecretFlag,
-		"The name of the Secret containing the CA cert for verifying the NGINX Plus usage reporting "+
-			"server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in "+
+		"The name of the Secret containing the NGINX Instance Manager CA certificate. "+
+			"Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in "+
 			"(default namespace: nginx-gateway).",
 	)
 
@@ -533,11 +536,8 @@ func createCopyCommand() *cobra.Command {
 		Use:   "copy",
 		Short: "Copy files to another directory",
 		RunE: func(_ *cobra.Command, _ []string) error {
-			if len(srcFiles) == 0 {
-				return errors.New("source must not be empty")
-			}
-			if len(dest) == 0 {
-				return errors.New("destination must not be empty")
+			if err := validateSleepArgs(srcFiles, dest); err != nil {
+				return err
 			}
 
 			for _, src := range srcFiles {

cmd/gateway/validation.go

@@ -205,3 +205,15 @@ func ensureNoPortCollisions(ports ...int) error {
 
 	return nil
 }
+
+// validateSleepArgs ensures that arguments to the sleep command are set.
+func validateSleepArgs(srcFiles []string, dest string) error {
+	if len(srcFiles) == 0 {
+		return errors.New("source must not be empty")
+	}
+	if len(dest) == 0 {
+		return errors.New("destination must not be empty")
+	}
+
+	return nil
+}

cmd/gateway/validation_test.go

@@ -553,3 +553,47 @@ func TestEnsureNoPortCollisions(t *testing.T) {
 	g.Expect(ensureNoPortCollisions(9113, 8081)).To(Succeed())
 	g.Expect(ensureNoPortCollisions(9113, 9113)).ToNot(Succeed())
 }
+
+func TestValidateSleepArgs(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		dest     string
+		srcFiles []string
+		expErr   bool
+	}{
+		{
+			name:     "valid values",
+			dest:     "/dest/file",
+			srcFiles: []string{"/src/file"},
+			expErr:   false,
+		},
+		{
+			name:     "invalid dest",
+			dest:     "",
+			srcFiles: []string{"/src/file"},
+			expErr:   true,
+		},
+		{
+			name:     "invalid src",
+			dest:     "/dest/file",
+			srcFiles: []string{},
+			expErr:   true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+
+			err := validateSleepArgs(tc.srcFiles, tc.dest)
+			if !tc.expErr {
+				g.Expect(err).ToNot(HaveOccurred())
+			} else {
+				g.Expect(err).To(HaveOccurred())
+			}
+		})
+	}
+}

internal/mode/static/handler.go

@@ -173,8 +173,10 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 	case state.EndpointsOnlyChange:
 		h.version++
 		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
-		if err := h.setDeploymentCtx(ctx, &cfg); err != nil {
+		if depCtx, err := h.setDeploymentCtx(ctx, logger); err != nil {
 			logger.Error(err, "error setting deployment context for usage reporting")
+		} else {
+			cfg.DeploymentContext = depCtx
 		}
 
 		h.setLatestConfiguration(&cfg)
@@ -187,8 +189,10 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 	case state.ClusterStateChange:
 		h.version++
 		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
-		if err := h.setDeploymentCtx(ctx, &cfg); err != nil {
+		if depCtx, err := h.setDeploymentCtx(ctx, logger); err != nil {
 			logger.Error(err, "error setting deployment context for usage reporting")
+		} else {
+			cfg.DeploymentContext = depCtx
 		}
 
 		h.setLatestConfiguration(&cfg)
@@ -497,9 +501,12 @@ func getGatewayAddresses(
 }
 
 // setDeploymentCtx sets the deployment context metadata for nginx plus reporting.
-func (h *eventHandlerImpl) setDeploymentCtx(ctx context.Context, cfg *dataplane.Configuration) error {
+func (h *eventHandlerImpl) setDeploymentCtx(
+	ctx context.Context,
+	logger logr.Logger,
+) (dataplane.DeploymentContext, error) {
 	if !h.cfg.plus {
-		return nil
+		return dataplane.DeploymentContext{}, nil
 	}
 
 	podNSName := types.NamespacedName{
@@ -509,27 +516,29 @@ func (h *eventHandlerImpl) setDeploymentCtx(ctx context.Context, cfg *dataplane.
 
 	clusterInfo, err := telemetry.CollectClusterInformation(ctx, h.cfg.k8sReader)
 	if err != nil {
-		return fmt.Errorf("error getting cluster information")
+		return dataplane.DeploymentContext{}, fmt.Errorf("error getting cluster information")
 	}
 
+	var installationID string
+	// InstallationID is not required by the usage API, so if we can't get it, don't return an error
 	replicaSet, err := telemetry.GetPodReplicaSet(ctx, h.cfg.k8sReader, podNSName)
 	if err != nil {
-		return fmt.Errorf("failed to get replica set for pod %v: %w", podNSName, err)
-	}
-
-	deploymentID, err := telemetry.GetDeploymentID(replicaSet)
-	if err != nil {
-		return fmt.Errorf("failed to get NGF deploymentID: %w", err)
+		logger.Error(err, fmt.Sprintf("failed to get replica set for pod %v", podNSName))
+	} else {
+		installationID, err = telemetry.GetDeploymentID(replicaSet)
+		if err != nil {
+			logger.Error(err, "failed to get NGF installationID")
+		}
 	}
 
-	cfg.DeploymentContext = dataplane.DeploymentContext{
+	depCtx := dataplane.DeploymentContext{
 		Integration:      "ngf",
 		ClusterID:        clusterInfo.ClusterID,
 		ClusterNodeCount: clusterInfo.NodeCount,
-		InstallationID:   deploymentID,
+		InstallationID:   installationID,
 	}
 
-	return nil
+	return depCtx, nil
 }
 
 // GetLatestConfiguration gets the latest configuration.