kms/health: address review comments

- guard against a (nil, nil) Status response instead of panicking
- reject duplicate --kms-sockets entries in validate
- move client construction from run into complete, following the
  complete/validate/run convention; run receives the operator client

Author: ibihim

pkg/operator/encryption/kms/health/cmd.go

@@ -60,7 +60,11 @@ func NewCommand(ctx context.Context, newOperatorClient func(*rest.Config) (v1hel
 			if err := o.validate(); err != nil {
 				return err
 			}
-			return o.run(ctx)
+			operatorClient, err := o.complete()
+			if err != nil {
+				return err
+			}
+			return o.run(ctx, operatorClient)
 		},
 	}
 	o.addFlags(cmd.Flags())
@@ -80,10 +84,15 @@ func (o *options) validate() error {
 	if len(o.KMSSockets) == 0 {
 		return fmt.Errorf("--kms-sockets is required, at least one")
 	}
+	socketSet := make(map[string]struct{}, len(o.KMSSockets))
 	for _, s := range o.KMSSockets {
 		if !kmsSocketPattern.MatchString(s) {
 			return fmt.Errorf("--kms-sockets entry %q must match %s", s, kmsSocketPattern)
 		}
+		if _, ok := socketSet[s]; ok {
+			return fmt.Errorf("--kms-sockets entry %q is duplicated", s)
+		}
+		socketSet[s] = struct{}{}
 	}
 
 	if o.Interval <= 0 {
@@ -102,20 +111,25 @@ func (o *options) validate() error {
 	return nil
 }
 
-func (o *options) run(ctx context.Context) error {
-	ctx = setupSignalContext(ctx)
-
+func (o *options) complete() (v1helpers.OperatorClient, error) {
 	// Empty kubeconfig falls back to the in-cluster config (service account
 	// token + KUBERNETES_SERVICE_HOST), which is the deployed path.
 	cfg, err := clientcmd.BuildConfigFromFlags("", o.Kubeconfig)
 	if err != nil {
-		return fmt.Errorf("build rest config: %w", err)
+		return nil, fmt.Errorf("build rest config: %w", err)
 	}
 
-	if _, err := o.newOperatorClient(cfg); err != nil {
-		return fmt.Errorf("build operator client: %w", err)
+	operatorClient, err := o.newOperatorClient(cfg)
+	if err != nil {
+		return nil, fmt.Errorf("build operator client: %w", err)
 	}
 
+	return operatorClient, nil
+}
+
+func (o *options) run(ctx context.Context, operatorClient v1helpers.OperatorClient) error {
+	ctx = setupSignalContext(ctx)
+
 	plugins, err := buildPlugins(ctx, o.KMSSockets, o.ReadTimeout)
 	if err != nil {
 		return err
@@ -128,7 +142,8 @@ func (o *options) run(ctx context.Context) error {
 		// Each Status RPC enforces o.ReadTimeout internally (set at dial time);
 		// ctx here only carries shutdown cancellation.
 		conditions := prober.probeAll(ctx)
-		// TODO: hand conditions to the writer once it lands; logging is a placeholder.
+		// TODO: hand conditions to the writer (via operatorClient) once it lands;
+		// logging is a placeholder.
 		klog.InfoS("kms plugin health", "conditions", conditions)
 	}, o.Interval, 0.1, false)
 

pkg/operator/encryption/kms/health/cmd_test.go

@@ -43,6 +43,11 @@ func TestValidate(t *testing.T) {
 			name:   "multiple valid sockets",
 			mutate: func(o *options) { o.KMSSockets = append(o.KMSSockets, "unix:///var/run/kmsplugin/kms-2.sock") },
 		},
+		{
+			name:    "duplicate sockets",
+			mutate:  func(o *options) { o.KMSSockets = append(o.KMSSockets, o.KMSSockets[0]) },
+			wantErr: true,
+		},
 		{
 			name:    "socket missing unix scheme",
 			mutate:  func(o *options) { o.KMSSockets = []string{"/var/run/kmsplugin/kms-1.sock"} },

pkg/operator/encryption/kms/health/prober.go

@@ -76,6 +76,11 @@ func (p *prober) probe(ctx context.Context, plugin pluginClient) pluginHealthRep
 	case err != nil:
 		report.Status = statusError
 		report.Detail = err.Error()
+	case resp == nil:
+		// The in-tree gRPC client never returns (nil, nil), but a misbehaving
+		// plugin must not panic the reporter.
+		report.Status = statusError
+		report.Detail = "kms plugin returned nil status response"
 	case resp.Healthz == healthzOK:
 		report.Status = statusHealthy
 		report.KEKID = resp.KeyID

pkg/operator/encryption/kms/health/prober_test.go

@@ -35,6 +35,7 @@ func TestProber_ProbeAll(t *testing.T) {
 			{keyID: "1", service: &fakeService{resp: &kmsservice.StatusResponse{Healthz: "ok", KeyID: "kek-abc"}}},
 			{keyID: "2", service: &fakeService{err: fmt.Errorf("connection refused")}},
 			{keyID: "3", service: &fakeService{resp: &kmsservice.StatusResponse{Healthz: "degraded"}}},
+			{keyID: "4", service: &fakeService{}},
 		},
 		now: func() time.Time { return fixed },
 	}
@@ -44,6 +45,7 @@ func TestProber_ProbeAll(t *testing.T) {
 		{KeyID: "1", KEKID: "kek-abc", Status: "healthy", LastChecked: fixed},
 		{KeyID: "2", Status: "error", Detail: "connection refused", LastChecked: fixed},
 		{KeyID: "3", Status: "unhealthy", Detail: "degraded", LastChecked: fixed},
+		{KeyID: "4", Status: "error", Detail: "kms plugin returned nil status response", LastChecked: fixed},
 	}
 	if !reflect.DeepEqual(have, want) {
 		t.Errorf("probeAll():\n have: %+v\n want: %+v", have, want)