From c1d959a21546079f5623523a42e13a84bde66901 Mon Sep 17 00:00:00 2001 From: splatypus-bot <282974456+splatypus-bot@users.noreply.github.com> Date: Fri, 8 May 2026 20:50:48 -0400 Subject: [PATCH 1/4] feat: implement per-component vCenter privilege validation (story #37) Add ValidateComponentPrivileges and privilegeRequirementsFor to the vsphere actuator package. Defines privilege requirement sets for all four CCO-managed components (machine-api, storage, cloud-controller, vsphere-problem-detector) and validates them through a PrivilegeChecker interface, enabling unit testing without a live vCenter. Authentication failures and missing-privilege errors are reported with exact condition-message formats per the story acceptance criteria. Validation is fail-fast across multiple vCenters; missing privileges are sorted for deterministic output. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/vsphere/actuator/privilege_validation.go | 135 ++++++ .../actuator/privilege_validation_test.go | 427 ++++++++++++++++++ 2 files changed, 562 insertions(+) create mode 100644 pkg/vsphere/actuator/privilege_validation.go create mode 100644 pkg/vsphere/actuator/privilege_validation_test.go diff --git a/pkg/vsphere/actuator/privilege_validation.go b/pkg/vsphere/actuator/privilege_validation.go new file mode 100644 index 0000000000..bb90f61b57 --- /dev/null +++ b/pkg/vsphere/actuator/privilege_validation.go @@ -0,0 +1,135 @@ +package actuator + +import ( + "context" + "fmt" + "sort" + "strings" +) + +// machineAPIPrivileges lists the 19 vCenter privileges required by the Machine API operator. +var machineAPIPrivileges = []string{ + "Datastore.AllocateSpace", + "Network.Assign", + "Resource.AssignVMToPool", + "VirtualMachine.Config.AddExistingDisk", + "VirtualMachine.Config.AddNewDisk", + "VirtualMachine.Config.AddRemoveDevice", + "VirtualMachine.Config.AdvancedConfig", + "VirtualMachine.Config.CPUCount", + "VirtualMachine.Config.DiskExtend", + "VirtualMachine.Config.EditDevice", + "VirtualMachine.Config.Memory", + "VirtualMachine.Config.RemoveDisk", + "VirtualMachine.Config.Resource", + "VirtualMachine.Config.Settings", + "VirtualMachine.Interact.PowerOff", + "VirtualMachine.Interact.PowerOn", + "VirtualMachine.Interact.Reset", + "VirtualMachine.Inventory.Create", + "VirtualMachine.Inventory.Delete", +} + +// storagePrivileges lists the 6 vCenter privileges required by the CSI driver. +var storagePrivileges = []string{ + "Datastore.AllocateSpace", + "Datastore.FileManagement", + "StoragePod.Config", + "VirtualMachine.Config.AddExistingDisk", + "VirtualMachine.Config.AddNewDisk", + "VirtualMachine.Config.RemoveDisk", +} + +// cloudControllerPrivileges lists the 3 vCenter privileges required by the Cloud Controller Manager. +var cloudControllerPrivileges = []string{ + "System.Read", + "System.View", + "VirtualMachine.Inventory.Create", +} + +// vsphereProblemDetectorPrivileges lists the 2 vCenter privileges required by the vSphere Problem Detector. +var vsphereProblemDetectorPrivileges = []string{ + "Sessions.ValidateSession", + "StorageProfile.View", +} + +// privilegeRequirementsFor returns the required vCenter privileges for the given component. +// Returns nil for unknown components (no requirement → validation skipped). +func privilegeRequirementsFor(component string) []string { + switch component { + case "machine-api": + return machineAPIPrivileges + case "storage": + return storagePrivileges + case "cloud-controller": + return cloudControllerPrivileges + case "vsphere-problem-detector": + return vsphereProblemDetectorPrivileges + default: + return nil + } +} + +// PrivilegeChecker retrieves the vCenter privileges granted to a user account. +type PrivilegeChecker interface { + // FetchUserPrivileges returns the privileges the named user holds on the given vCenter. + // Returns an authentication error (containing "authentication", "invalidlogin", or + // "unauthenticated", case-insensitive) when the credentials are rejected by vCenter. + FetchUserPrivileges(ctx context.Context, username, vcenterFQDN string) ([]string, error) +} + +// ComponentCredential holds the vCenter login information for one component on one vCenter. +type ComponentCredential struct { + Component string + Username string + Password string + VCenterFQDN string +} + +// ValidateComponentPrivileges validates that each credential satisfies the privilege +// requirements for its component role. Returns the first validation error encountered +// (fail-fast semantics across multiple credentials / vCenters). +func ValidateComponentPrivileges(ctx context.Context, checker PrivilegeChecker, creds []ComponentCredential) error { + for _, cred := range creds { + required := privilegeRequirementsFor(cred.Component) + if len(required) == 0 { + continue + } + + actual, err := checker.FetchUserPrivileges(ctx, cred.Username, cred.VCenterFQDN) + if err != nil { + if isAuthError(err) { + return fmt.Errorf("Authentication failed for '%s' on vCenter '%s'", cred.Username, cred.VCenterFQDN) + } + return err + } + + missing := missingPrivileges(required, actual) + if len(missing) > 0 { + sort.Strings(missing) + return fmt.Errorf("Account '%s' on vCenter '%s' missing privileges: %v", cred.Username, cred.VCenterFQDN, missing) + } + } + return nil +} + +func isAuthError(err error) bool { + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "authentication") || + strings.Contains(msg, "invalidlogin") || + strings.Contains(msg, "unauthenticated") +} + +func missingPrivileges(required, actual []string) []string { + have := make(map[string]struct{}, len(actual)) + for _, p := range actual { + have[p] = struct{}{} + } + var missing []string + for _, p := range required { + if _, ok := have[p]; !ok { + missing = append(missing, p) + } + } + return missing +} diff --git a/pkg/vsphere/actuator/privilege_validation_test.go b/pkg/vsphere/actuator/privilege_validation_test.go new file mode 100644 index 0000000000..cc1ea4f65d --- /dev/null +++ b/pkg/vsphere/actuator/privilege_validation_test.go @@ -0,0 +1,427 @@ +package actuator + +import ( + "context" + "fmt" + "strings" + "testing" +) + +// mockPrivilegeChecker implements PrivilegeChecker for unit tests. +type mockPrivilegeChecker struct { + privileges map[string][]string // "username@vcenter" → granted privileges + authErrors map[string]bool // "username@vcenter" → trigger auth failure + otherErrors map[string]error // "username@vcenter" → trigger non-auth error +} + +func newMock() *mockPrivilegeChecker { + return &mockPrivilegeChecker{ + privileges: make(map[string][]string), + authErrors: make(map[string]bool), + otherErrors: make(map[string]error), + } +} + +func (m *mockPrivilegeChecker) FetchUserPrivileges(_ context.Context, username, vcenterFQDN string) ([]string, error) { + key := username + "@" + vcenterFQDN + if m.authErrors[key] { + return nil, fmt.Errorf("authentication failed: InvalidLogin for %s", username) + } + if err, ok := m.otherErrors[key]; ok { + return nil, err + } + return m.privileges[key], nil +} + +func setPrivileges(m *mockPrivilegeChecker, username, vcenter string, privs []string) { + m.privileges[username+"@"+vcenter] = privs +} + +func setAuthError(m *mockPrivilegeChecker, username, vcenter string) { + m.authErrors[username+"@"+vcenter] = true +} + +func setOtherError(m *mockPrivilegeChecker, username, vcenter string, err error) { + m.otherErrors[username+"@"+vcenter] = err +} + +// allExcept returns privs with the named elements removed. +func allExcept(privs []string, exclude ...string) []string { + excSet := make(map[string]struct{}, len(exclude)) + for _, e := range exclude { + excSet[e] = struct{}{} + } + out := make([]string, 0, len(privs)) + for _, p := range privs { + if _, skip := excSet[p]; !skip { + out = append(out, p) + } + } + return out +} + +const ( + vcenter1 = "vcenter1.example.com" + vcenter2 = "vcenter2.example.com" +) + +// --------------------------------------------------------------------------- +// AC1: Missing privilege → CredentialsProvisionFailed condition +// --------------------------------------------------------------------------- + +func TestValidatePrivileges_MachineAPI_MissingOnePrivilege(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, + allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + const wantMsg = "Account 'ocp-machine-api@vsphere.local' on vCenter 'vcenter1.example.com' missing privileges: [VirtualMachine.Config.AddNewDisk]" + if err.Error() != wantMsg { + t.Errorf("got:\n %s\nwant:\n %s", err.Error(), wantMsg) + } +} + +func TestValidatePrivileges_MachineAPI_MissingMultiplePrivileges(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, + allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk", "VirtualMachine.Interact.PowerOn", "Network.Assign")) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + for _, priv := range []string{"Network.Assign", "VirtualMachine.Config.AddNewDisk", "VirtualMachine.Interact.PowerOn"} { + if !strings.Contains(err.Error(), priv) { + t.Errorf("expected %q in error message %q", priv, err.Error()) + } + } +} + +func TestValidatePrivileges_CSIDriver_MissingPrivilege(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, + allExcept(storagePrivileges, "Datastore.FileManagement")) + + creds := []ComponentCredential{ + {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "ocp-csi@vsphere.local") || + !strings.Contains(err.Error(), vcenter1) || + !strings.Contains(err.Error(), "Datastore.FileManagement") { + t.Errorf("unexpected error message: %s", err.Error()) + } +} + +func TestValidatePrivileges_CloudController_MissingPrivilege(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-cloud-controller@vsphere.local", vcenter1, + allExcept(cloudControllerPrivileges, "System.Read")) + + creds := []ComponentCredential{ + {Component: "cloud-controller", Username: "ocp-cloud-controller@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "System.Read") { + t.Errorf("expected System.Read in error: %s", err.Error()) + } +} + +func TestValidatePrivileges_VSphereProblemDetector_MissingPrivilege(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, + allExcept(vsphereProblemDetectorPrivileges, "Sessions.ValidateSession")) + + creds := []ComponentCredential{ + {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "Sessions.ValidateSession") { + t.Errorf("expected Sessions.ValidateSession in error: %s", err.Error()) + } +} + +// --------------------------------------------------------------------------- +// AC2: All privileges present → nil error, provisioning proceeds +// --------------------------------------------------------------------------- + +func TestValidatePrivileges_MachineAPI_FullPrivilegeSet(t *testing.T) { + privs := privilegeRequirementsFor("machine-api") + if len(privs) != 19 { + t.Errorf("machine-api privilege set: got %d; want 19", len(privs)) + } + + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil, got %v", err) + } +} + +func TestValidatePrivileges_CSIDriver_FullPrivilegeSet(t *testing.T) { + privs := privilegeRequirementsFor("storage") + if len(privs) != 6 { + t.Errorf("csi-driver privilege set: got %d; want 6", len(privs)) + } + + m := newMock() + setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, storagePrivileges) + + creds := []ComponentCredential{ + {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil, got %v", err) + } +} + +func TestValidatePrivileges_CloudController_FullPrivilegeSet(t *testing.T) { + privs := privilegeRequirementsFor("cloud-controller") + if len(privs) != 3 { + t.Errorf("cloud-controller privilege set: got %d; want 3", len(privs)) + } + + m := newMock() + setPrivileges(m, "ocp-cc@vsphere.local", vcenter1, cloudControllerPrivileges) + + creds := []ComponentCredential{ + {Component: "cloud-controller", Username: "ocp-cc@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil, got %v", err) + } +} + +func TestValidatePrivileges_VSphereProblemDetector_FullPrivilegeSet(t *testing.T) { + privs := privilegeRequirementsFor("vsphere-problem-detector") + if len(privs) != 2 { + t.Errorf("vsphere-problem-detector privilege set: got %d; want 2", len(privs)) + } + + m := newMock() + setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, vsphereProblemDetectorPrivileges) + + creds := []ComponentCredential{ + {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil, got %v", err) + } +} + +func TestValidatePrivileges_AllComponents_AllPrivilegesPresent(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) + setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, storagePrivileges) + setPrivileges(m, "ocp-cc@vsphere.local", vcenter1, cloudControllerPrivileges) + setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, vsphereProblemDetectorPrivileges) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, + {Component: "cloud-controller", Username: "ocp-cc@vsphere.local", VCenterFQDN: vcenter1}, + {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil, got %v", err) + } +} + +// --------------------------------------------------------------------------- +// AC3: Authentication failure → "Authentication failed for" message +// --------------------------------------------------------------------------- + +func TestValidatePrivileges_CSIDriver_AuthenticationFailure(t *testing.T) { + m := newMock() + setAuthError(m, "ocp-csi@vsphere.local", vcenter1) + + creds := []ComponentCredential{ + {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + const wantMsg = "Authentication failed for 'ocp-csi@vsphere.local' on vCenter 'vcenter1.example.com'" + if err.Error() != wantMsg { + t.Errorf("got:\n %s\nwant:\n %s", err.Error(), wantMsg) + } +} + +func TestValidatePrivileges_MachineAPI_AuthenticationFailure(t *testing.T) { + m := newMock() + setAuthError(m, "ocp-machine-api@vsphere.local", vcenter1) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "Authentication failed for") || + !strings.Contains(err.Error(), "ocp-machine-api@vsphere.local") || + !strings.Contains(err.Error(), vcenter1) { + t.Errorf("unexpected auth failure message: %s", err.Error()) + } +} + +// --------------------------------------------------------------------------- +// Adversarial cases +// --------------------------------------------------------------------------- + +func TestValidatePrivileges_AccountWithNoPrivileges(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, []string{}) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error when account has no privileges, got nil") + } + for _, priv := range machineAPIPrivileges { + if !strings.Contains(err.Error(), priv) { + t.Errorf("expected %q in error message", priv) + } + } +} + +func TestValidatePrivileges_UnknownComponent_NoRequirements(t *testing.T) { + m := newMock() + creds := []ComponentCredential{ + {Component: "unknown-component", Username: "ocp-unknown@vsphere.local", VCenterFQDN: vcenter1}, + } + if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { + t.Errorf("expected nil for unknown component, got %v", err) + } +} + +func TestValidatePrivileges_MultipleVCenters_OneVCenterFails(t *testing.T) { + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter2, + allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter2}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error for vcenter2 missing privilege, got nil") + } + if !strings.Contains(err.Error(), vcenter2) { + t.Errorf("error should identify vcenter2 as the failing vCenter: %s", err.Error()) + } +} + +func TestValidatePrivileges_MultipleVCenters_BothFail(t *testing.T) { + // ValidateComponentPrivileges uses fail-fast: returns on the first failure. + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, + allExcept(machineAPIPrivileges, "Network.Assign")) + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter2, + allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter2}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error when both vCenters fail, got nil") + } + if !strings.Contains(err.Error(), vcenter1) { + t.Errorf("fail-fast: expected error about vcenter1 (first), got: %s", err.Error()) + } +} + +func TestValidatePrivileges_FetchPrivileges_NonAuthError(t *testing.T) { + m := newMock() + networkErr := fmt.Errorf("connection refused: timeout connecting to vcenter1.example.com:443") + setOtherError(m, "ocp-machine-api@vsphere.local", vcenter1, networkErr) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if strings.Contains(err.Error(), "Authentication failed") || strings.Contains(err.Error(), "missing privileges") { + t.Errorf("non-auth error was misclassified: %s", err.Error()) + } + if !strings.Contains(err.Error(), "connection refused") { + t.Errorf("expected original error propagated, got: %s", err.Error()) + } +} + +func TestValidatePrivileges_ConditionMessage_DoesNotContainPassword(t *testing.T) { + const sensitivePassword = "s3cr3t-p@ssw0rd-unique" + + m := newMock() + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, + allExcept(machineAPIPrivileges, "Network.Assign")) + + creds := []ComponentCredential{ + { + Component: "machine-api", + Username: "ocp-machine-api@vsphere.local", + Password: sensitivePassword, + VCenterFQDN: vcenter1, + }, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + if strings.Contains(err.Error(), sensitivePassword) { + t.Errorf("condition message must not contain the password; got: %s", err.Error()) + } +} + +func TestValidatePrivileges_MissingPrivileges_SortedInMessage(t *testing.T) { + m := newMock() + // Remove three privileges that sort as: Datastore.AllocateSpace < Network.Assign < VirtualMachine.Inventory.Delete + setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, + allExcept(machineAPIPrivileges, "VirtualMachine.Inventory.Delete", "Datastore.AllocateSpace", "Network.Assign")) + + creds := []ComponentCredential{ + {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, + } + err := ValidateComponentPrivileges(context.Background(), m, creds) + if err == nil { + t.Fatal("expected an error, got nil") + } + msg := err.Error() + iDA := strings.Index(msg, "Datastore.AllocateSpace") + iNA := strings.Index(msg, "Network.Assign") + iVID := strings.Index(msg, "VirtualMachine.Inventory.Delete") + if !(iDA < iNA && iNA < iVID) { + t.Errorf("missing privileges not in sorted order in message: %s", msg) + } +} From 787864ae3c1f3ef65c92ebe0c259256d106898b1 Mon Sep 17 00:00:00 2001 From: splatypus-bot <282974456+splatypus-bot@users.noreply.github.com> Date: Fri, 8 May 2026 21:38:06 -0400 Subject: [PATCH 2/4] feat: implement per-component vSphere credential distribution with graceful fallback (story #38) Adds resolveVSphereCredentials() routing CredentialsRequest CRs annotated with cloudcredential.openshift.io/vsphere-component to per-component kube-system secrets. Falls back to shared credential with warning when component secret is absent or empty. Validates privileges via Story #37 ValidateComponentPrivileges before provisioning. Supports multi-vCenter clusters with FQDN-keyed secret entries. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../actuator/credential_distribution.go | 107 +++++ .../actuator/credential_distribution_test.go | 454 ++++++++++++++++++ 2 files changed, 561 insertions(+) create mode 100644 pkg/vsphere/actuator/credential_distribution.go create mode 100644 pkg/vsphere/actuator/credential_distribution_test.go diff --git a/pkg/vsphere/actuator/credential_distribution.go b/pkg/vsphere/actuator/credential_distribution.go new file mode 100644 index 0000000000..cb322ff684 --- /dev/null +++ b/pkg/vsphere/actuator/credential_distribution.go @@ -0,0 +1,107 @@ +package actuator + +import ( + "context" + "fmt" + + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" + corev1 "k8s.io/api/core/v1" +) + +const vsphereComponentAnnotationKey = "cloudcredential.openshift.io/vsphere-component" + +// ComponentSecretReader looks up a named secret from kube-system. +type ComponentSecretReader interface { + GetComponentSecret(ctx context.Context, name string) (*corev1.Secret, error) +} + +// componentSecretName maps an annotation value to the kube-system secret name. +// Returns "" for unrecognized components. +func componentSecretName(component string) string { + switch component { + case "machineAPI": + return "vsphere-machine-api-creds" + case "csiDriver": + return "vsphere-storage-creds" + case "cloudController": + return "vsphere-cloud-controller-creds" + case "vsphereProblemDetector": + return "vsphere-problem-detector-creds" + default: + return "" + } +} + +// componentPrivilegeSet maps an annotation value to the privilege-set name used by ValidateComponentPrivileges. +func componentPrivilegeSet(component string) string { + switch component { + case "machineAPI": + return "machine-api" + case "csiDriver": + return "storage" + case "cloudController": + return "cloud-controller" + case "vsphereProblemDetector": + return "vsphere-problem-detector" + default: + return "" + } +} + +// resolveVSphereCredentials routes a CredentialsRequest to per-component or shared credentials. +// +// If the CR has a vsphere-component annotation and a matching kube-system secret with non-empty +// data, the per-component credential is validated (via Story #37 ValidateComponentPrivileges) and +// returned. If the component secret is absent or empty, shared credentials are returned and a +// warning is set. A CR without the annotation is a passthrough — shared credentials, no warning. +func resolveVSphereCredentials( + ctx context.Context, + cr *minterv1.CredentialsRequest, + componentSecretReader ComponentSecretReader, + sharedSecretData map[string][]byte, + checker PrivilegeChecker, + vcenters []string, +) (secretData map[string][]byte, warning string, err error) { + component := cr.Annotations[vsphereComponentAnnotationKey] + if component == "" { + return sharedSecretData, "", nil + } + + secretName := componentSecretName(component) + if secretName != "" { + secret, err := componentSecretReader.GetComponentSecret(ctx, secretName) + if err != nil { + return nil, "", err + } + + if secret != nil && len(secret.Data) > 0 { + // Verify credentials exist for every cluster vCenter before validating. + for _, vcenter := range vcenters { + if _, ok := secret.Data[vcenter+".username"]; !ok { + return nil, "", fmt.Errorf("component secret %q missing credentials for vCenter %q", secretName, vcenter) + } + } + + // Build a ComponentCredential per vCenter for privilege validation. + creds := make([]ComponentCredential, 0, len(vcenters)) + privSet := componentPrivilegeSet(component) + for _, vcenter := range vcenters { + creds = append(creds, ComponentCredential{ + Component: privSet, + Username: string(secret.Data[vcenter+".username"]), + Password: string(secret.Data[vcenter+".password"]), + VCenterFQDN: vcenter, + }) + } + + if err := ValidateComponentPrivileges(ctx, checker, creds); err != nil { + return nil, "", err + } + + return secret.Data, "", nil + } + } + + // No component secret (or unrecognized component): fall back to shared credentials. + return sharedSecretData, fmt.Sprintf("Component '%s' using fallback shared credentials", component), nil +} diff --git a/pkg/vsphere/actuator/credential_distribution_test.go b/pkg/vsphere/actuator/credential_distribution_test.go new file mode 100644 index 0000000000..3585d8eb10 --- /dev/null +++ b/pkg/vsphere/actuator/credential_distribution_test.go @@ -0,0 +1,454 @@ +// credential_distribution_test.go tests Story #38: +// CCO Per-Component Credential Distribution with Graceful Fallback. +// +// Build guard prevents compile errors until the implementation exists. +// Remove //go:build ignore once credential_distribution.go is implemented. +// +// Implementation contract expected by these tests: +// +// func resolveVSphereCredentials( +// ctx context.Context, +// cr *minterv1.CredentialsRequest, +// componentSecretReader ComponentSecretReader, +// sharedSecretData map[string][]byte, +// checker PrivilegeChecker, +// vcenters []string, +// ) (secretData map[string][]byte, warning string, err error) +// +// ComponentSecretReader is an interface (or func) that looks up a named secret from kube-system. +// The component annotation key is: cloudcredential.openshift.io/vsphere-component +// Annotation values: machineAPI, csiDriver, cloudController, vsphereProblemDetector +// Component secret names: vsphere-machine-api-creds, vsphere-storage-creds, +// +// vsphere-cloud-controller-creds, vsphere-problem-detector-creds +// +// Per-component secret key format: {vcenter.fqdn}.username / {vcenter.fqdn}.password +// Warning format: Component '{component}' using fallback shared credentials +package actuator + +import ( + "context" + "fmt" + "testing" + + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// vsphereComponentAnnotation is the annotation key used to identify per-component CredentialsRequests. +const vsphereComponentAnnotation = "cloudcredential.openshift.io/vsphere-component" + +// stubComponentSecretReader is a test double for looking up kube-system component secrets. +type stubComponentSecretReader struct { + secrets map[string]*corev1.Secret // secretName → secret (nil means NotFound) +} + +func (s *stubComponentSecretReader) GetComponentSecret(_ context.Context, name string) (*corev1.Secret, error) { + if secret, ok := s.secrets[name]; ok { + return secret, nil // nil value means secret was explicitly absent + } + return nil, nil // not configured = absent +} + +// makeCredCR builds a minimal CredentialsRequest with the given component annotation value. +func makeCredCR(component string) *minterv1.CredentialsRequest { + cr := &minterv1.CredentialsRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cr", + Namespace: "openshift-cloud-credential-operator", + }, + } + if component != "" { + cr.Annotations = map[string]string{vsphereComponentAnnotation: component} + } + return cr +} + +// makeComponentSecret builds a kube-system Secret with the given vCenter credentials. +func makeComponentSecret(name string, creds map[string]string) *corev1.Secret { + data := make(map[string][]byte, len(creds)) + for k, v := range creds { + data[k] = []byte(v) + } + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "kube-system"}, + Data: data, + } +} + +var sharedSecretData = map[string][]byte{ + "vcenter.example.com.username": []byte("shared@vsphere.local"), + "vcenter.example.com.password": []byte("shared-password"), +} + +// ── AC1: Per-component routing and shared-credential fallback ───────────────── + +// TestResolveComponentCredential_AnnotatedCRWithSecret_UsesPerComponentCred verifies AC1 (first +// branch): a CredentialsRequest annotated machineAPI, whose component secret exists, receives the +// per-component credential — not the shared credential. +func TestResolveComponentCredential_AnnotatedCRWithSecret_UsesPerComponentCred(t *testing.T) { + cr := makeCredCR("machineAPI") + + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter.example.com.username": "machine-api@vsphere.local", + "vcenter.example.com.password": "machine-api-password", + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{ + "vsphere-machine-api-creds": componentSecret, + }, + } + + checker := newMock() + setPrivileges(checker, "machine-api@vsphere.local", "vcenter.example.com", machineAPIPrivileges) + + data, warning, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if warning != "" { + t.Errorf("expected no warning for annotated CR with existing component secret, got: %q", warning) + } + if string(data["vcenter.example.com.username"]) != "machine-api@vsphere.local" { + t.Errorf("expected per-component username, got: %q", data["vcenter.example.com.username"]) + } +} + +// TestResolveComponentCredential_AnnotatedCRWithoutSecret_FallsBackToShared verifies AC1 (fallback +// branch): a CredentialsRequest annotated csiDriver, with no component secret present, receives the +// shared credential and a warning is returned. +func TestResolveComponentCredential_AnnotatedCRWithoutSecret_FallsBackToShared(t *testing.T) { + cr := makeCredCR("csiDriver") + + // No component secret registered for csiDriver + reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} + + checker := newMock() + // Shared credential checker — should be queried for shared credential validation (if any) + + data, warning, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("expected no error during fallback, got: %v", err) + } + expectedWarning := "Component 'csiDriver' using fallback shared credentials" + if warning != expectedWarning { + t.Errorf("expected warning %q, got %q", expectedWarning, warning) + } + if string(data["vcenter.example.com.username"]) != "shared@vsphere.local" { + t.Errorf("expected shared username in fallback data, got: %q", data["vcenter.example.com.username"]) + } +} + +// ── AC2: Multi-vCenter credential distribution ──────────────────────────────── + +// TestResolveComponentCredential_MultiVCenter_BothVCentersPopulated verifies AC2: when a +// component secret contains credentials for two vCenters, both sets of keyed entries appear +// in the provisioned secret data. +func TestResolveComponentCredential_MultiVCenter_BothVCentersPopulated(t *testing.T) { + cr := makeCredCR("machineAPI") + + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter1.example.com.username": "machine-api@vc1.local", + "vcenter1.example.com.password": "password1", + "vcenter2.example.com.username": "machine-api@vc2.local", + "vcenter2.example.com.password": "password2", + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, + } + + checker := newMock() + setPrivileges(checker, "machine-api@vc1.local", "vcenter1.example.com", machineAPIPrivileges) + setPrivileges(checker, "machine-api@vc2.local", "vcenter2.example.com", machineAPIPrivileges) + + data, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter1.example.com", "vcenter2.example.com"}, + ) + + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if string(data["vcenter1.example.com.username"]) != "machine-api@vc1.local" { + t.Errorf("vcenter1 username missing or wrong: %q", data["vcenter1.example.com.username"]) + } + if string(data["vcenter2.example.com.username"]) != "machine-api@vc2.local" { + t.Errorf("vcenter2 username missing or wrong: %q", data["vcenter2.example.com.username"]) + } +} + +// ── AC3: Privilege validation invoked before provisioning ──────────────────── + +// TestResolveComponentCredential_PrivilegeValidationInvoked verifies AC3: the Story #37 +// ValidateComponentPrivileges call is made before the secret data is returned. A mock checker +// that records calls is used; provisioning succeeds only when the checker is exercised. +func TestResolveComponentCredential_PrivilegeValidationInvoked(t *testing.T) { + cr := makeCredCR("machineAPI") + + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter.example.com.username": "machine-api@vsphere.local", + "vcenter.example.com.password": "machine-api-password", + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, + } + + called := false + recordingChecker := &recordingPrivilegeChecker{ + inner: newMock(), + onCalled: func() { called = true }, + } + setPrivileges(recordingChecker.inner, "machine-api@vsphere.local", "vcenter.example.com", machineAPIPrivileges) + + _, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, recordingChecker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if !called { + t.Error("expected PrivilegeChecker.FetchUserPrivileges to be called (AC3 requires validation before provisioning)") + } +} + +// TestResolveComponentCredential_PrivilegeValidationFails_SecretNotProvisioned verifies AC3 (failure +// path): if privilege validation fails, an error is returned and no secret data is returned. +func TestResolveComponentCredential_PrivilegeValidationFails_SecretNotProvisioned(t *testing.T) { + cr := makeCredCR("machineAPI") + + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter.example.com.username": "machine-api@vsphere.local", + "vcenter.example.com.password": "machine-api-password", + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, + } + + // Checker grants only a subset of required privileges → validation fails + checker := newMock() + setPrivileges(checker, "machine-api@vsphere.local", "vcenter.example.com", + allExcept(machineAPIPrivileges, "VirtualMachine.Inventory.Create", "VirtualMachine.Inventory.Delete")) + + data, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err == nil { + t.Fatal("expected validation error, got nil") + } + if len(data) > 0 { + t.Errorf("expected no secret data when validation fails, got: %v", data) + } +} + +// ── Adversarial cases ───────────────────────────────────────────────────────── + +// TestResolveComponentCredential_NoAnnotation_Passthrough verifies that a CredentialsRequest +// with no vsphere-component annotation is not routed to per-component logic (passthrough). +func TestResolveComponentCredential_NoAnnotation_Passthrough(t *testing.T) { + cr := makeCredCR("") // no annotation + + reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} + checker := newMock() + + data, warning, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("expected no error for passthrough CR, got: %v", err) + } + if warning != "" { + t.Errorf("expected no warning for passthrough CR, got: %q", warning) + } + // Passthrough returns the shared secret data unchanged + if string(data["vcenter.example.com.username"]) != "shared@vsphere.local" { + t.Errorf("expected shared credential in passthrough, got: %q", data["vcenter.example.com.username"]) + } +} + +// TestResolveComponentCredential_NilAnnotations_Passthrough is an edge case ensuring nil +// annotations map (not just absent key) is handled without panic. +func TestResolveComponentCredential_NilAnnotations_Passthrough(t *testing.T) { + cr := &minterv1.CredentialsRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cr", + Namespace: "openshift-cloud-credential-operator", + Annotations: nil, // explicitly nil + }, + } + + reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} + checker := newMock() + + _, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("nil annotations must not panic or error, got: %v", err) + } +} + +// TestResolveComponentCredential_UnknownComponentAnnotation_FallsBackToShared verifies that an +// unrecognized annotation value (e.g. "diagnostics") does not panic and falls back to shared. +func TestResolveComponentCredential_UnknownComponentAnnotation_FallsBackToShared(t *testing.T) { + cr := makeCredCR("diagnostics") // not a known component + + reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} + checker := newMock() + + _, warning, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("unknown component should fall back gracefully, got error: %v", err) + } + if warning == "" { + t.Error("expected a fallback warning for unknown component") + } +} + +// TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error verifies that when a +// component secret is present but lacks credentials for one of the cluster's vCenters, an error +// is returned (not silent omission — the operator would fail to connect to that vCenter). +func TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error(t *testing.T) { + cr := makeCredCR("machineAPI") + + // Secret only has vcenter1; cluster has vcenter1 + vcenter2 + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter1.example.com.username": "machine-api@vc1.local", + "vcenter1.example.com.password": "password1", + // vcenter2 entries absent + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, + } + + checker := newMock() + setPrivileges(checker, "machine-api@vc1.local", "vcenter1.example.com", machineAPIPrivileges) + + _, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter1.example.com", "vcenter2.example.com"}, + ) + + if err == nil { + t.Error("expected error when component secret is missing credentials for a cluster vCenter") + } +} + +// TestResolveComponentCredential_AuthFailureDuringValidation_Error verifies that an authentication +// failure from the PrivilegeChecker is surfaced as an error (and not silently treated as +// insufficient privileges — the error type matters for operator status messages). +func TestResolveComponentCredential_AuthFailureDuringValidation_Error(t *testing.T) { + cr := makeCredCR("machineAPI") + + componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ + "vcenter.example.com.username": "machine-api@vsphere.local", + "vcenter.example.com.password": "wrong-password", + }) + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, + } + + checker := newMock() + setAuthError(checker, "machine-api@vsphere.local", "vcenter.example.com") + + _, _, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err == nil { + t.Fatal("expected error for authentication failure") + } + if !containsInsensitive(err.Error(), "authentication") { + t.Errorf("expected authentication error message, got: %v", err) + } +} + +// TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared verifies that a component +// secret with no keys (empty Data) is treated equivalently to a missing secret — fallback occurs. +func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *testing.T) { + cr := makeCredCR("machineAPI") + + emptySecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{}) // no keys + + reader := &stubComponentSecretReader{ + secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": emptySecret}, + } + + checker := newMock() + + _, warning, err := resolveVSphereCredentials( + context.Background(), cr, reader, sharedSecretData, checker, + []string{"vcenter.example.com"}, + ) + + if err != nil { + t.Fatalf("empty component secret should fall back, not error: %v", err) + } + if warning == "" { + t.Error("expected fallback warning for empty component secret") + } +} + +// ── Helpers used by the stubs above ────────────────────────────────────────── + +// recordingPrivilegeChecker wraps an inner checker and calls onCalled on the first invocation. +type recordingPrivilegeChecker struct { + inner *mockPrivilegeChecker + onCalled func() +} + +func (r *recordingPrivilegeChecker) FetchUserPrivileges(ctx context.Context, username, vcenterFQDN string) ([]string, error) { + if r.onCalled != nil { + r.onCalled() + } + return r.inner.FetchUserPrivileges(ctx, username, vcenterFQDN) +} + +func containsInsensitive(s, substr string) bool { + return fmt.Sprintf("%s", s) != "" && len(s) >= len(substr) && + (s == substr || len(s) > 0 && containsBytes([]byte(s), []byte(substr))) +} + +func containsBytes(haystack, needle []byte) bool { + for i := 0; i <= len(haystack)-len(needle); i++ { + match := true + for j, b := range needle { + hb := haystack[i+j] + if hb != b && hb != b-32 && hb != b+32 { + match = false + break + } + } + if match { + return true + } + } + return false +} From a3bcc3f0bbff0af4fc44a97525bbf37fbe871106 Mon Sep 17 00:00:00 2001 From: splatypus-bot <282974456+splatypus-bot@users.noreply.github.com> Date: Mon, 11 May 2026 09:27:07 -0400 Subject: [PATCH 3/4] refactor: remove privilege validation per review feedback Remove privilege_validation.go and its tests. Update resolveVSphereCredentials to drop the PrivilegeChecker parameter and the ValidateComponentPrivileges call. AC3 tests removed; credential routing and multi-vCenter coverage retained. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../actuator/credential_distribution.go | 41 +- .../actuator/credential_distribution_test.go | 176 +------- pkg/vsphere/actuator/privilege_validation.go | 135 ------ .../actuator/privilege_validation_test.go | 427 ------------------ 4 files changed, 12 insertions(+), 767 deletions(-) delete mode 100644 pkg/vsphere/actuator/privilege_validation.go delete mode 100644 pkg/vsphere/actuator/privilege_validation_test.go diff --git a/pkg/vsphere/actuator/credential_distribution.go b/pkg/vsphere/actuator/credential_distribution.go index cb322ff684..441bbbc4c2 100644 --- a/pkg/vsphere/actuator/credential_distribution.go +++ b/pkg/vsphere/actuator/credential_distribution.go @@ -32,34 +32,17 @@ func componentSecretName(component string) string { } } -// componentPrivilegeSet maps an annotation value to the privilege-set name used by ValidateComponentPrivileges. -func componentPrivilegeSet(component string) string { - switch component { - case "machineAPI": - return "machine-api" - case "csiDriver": - return "storage" - case "cloudController": - return "cloud-controller" - case "vsphereProblemDetector": - return "vsphere-problem-detector" - default: - return "" - } -} - // resolveVSphereCredentials routes a CredentialsRequest to per-component or shared credentials. // // If the CR has a vsphere-component annotation and a matching kube-system secret with non-empty -// data, the per-component credential is validated (via Story #37 ValidateComponentPrivileges) and -// returned. If the component secret is absent or empty, shared credentials are returned and a -// warning is set. A CR without the annotation is a passthrough — shared credentials, no warning. +// data, the per-component credential is returned. If the component secret is absent or empty, +// shared credentials are returned and a warning is set. A CR without the annotation is a +// passthrough — shared credentials, no warning. func resolveVSphereCredentials( ctx context.Context, cr *minterv1.CredentialsRequest, componentSecretReader ComponentSecretReader, sharedSecretData map[string][]byte, - checker PrivilegeChecker, vcenters []string, ) (secretData map[string][]byte, warning string, err error) { component := cr.Annotations[vsphereComponentAnnotationKey] @@ -75,29 +58,13 @@ func resolveVSphereCredentials( } if secret != nil && len(secret.Data) > 0 { - // Verify credentials exist for every cluster vCenter before validating. + // Verify credentials exist for every cluster vCenter before returning. for _, vcenter := range vcenters { if _, ok := secret.Data[vcenter+".username"]; !ok { return nil, "", fmt.Errorf("component secret %q missing credentials for vCenter %q", secretName, vcenter) } } - // Build a ComponentCredential per vCenter for privilege validation. - creds := make([]ComponentCredential, 0, len(vcenters)) - privSet := componentPrivilegeSet(component) - for _, vcenter := range vcenters { - creds = append(creds, ComponentCredential{ - Component: privSet, - Username: string(secret.Data[vcenter+".username"]), - Password: string(secret.Data[vcenter+".password"]), - VCenterFQDN: vcenter, - }) - } - - if err := ValidateComponentPrivileges(ctx, checker, creds); err != nil { - return nil, "", err - } - return secret.Data, "", nil } } diff --git a/pkg/vsphere/actuator/credential_distribution_test.go b/pkg/vsphere/actuator/credential_distribution_test.go index 3585d8eb10..930a8c6f03 100644 --- a/pkg/vsphere/actuator/credential_distribution_test.go +++ b/pkg/vsphere/actuator/credential_distribution_test.go @@ -1,9 +1,6 @@ // credential_distribution_test.go tests Story #38: // CCO Per-Component Credential Distribution with Graceful Fallback. // -// Build guard prevents compile errors until the implementation exists. -// Remove //go:build ignore once credential_distribution.go is implemented. -// // Implementation contract expected by these tests: // // func resolveVSphereCredentials( @@ -11,7 +8,6 @@ // cr *minterv1.CredentialsRequest, // componentSecretReader ComponentSecretReader, // sharedSecretData map[string][]byte, -// checker PrivilegeChecker, // vcenters []string, // ) (secretData map[string][]byte, warning string, err error) // @@ -28,7 +24,6 @@ package actuator import ( "context" - "fmt" "testing" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" @@ -101,11 +96,8 @@ func TestResolveComponentCredential_AnnotatedCRWithSecret_UsesPerComponentCred(t }, } - checker := newMock() - setPrivileges(checker, "machine-api@vsphere.local", "vcenter.example.com", machineAPIPrivileges) - data, warning, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -129,11 +121,8 @@ func TestResolveComponentCredential_AnnotatedCRWithoutSecret_FallsBackToShared(t // No component secret registered for csiDriver reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} - checker := newMock() - // Shared credential checker — should be queried for shared credential validation (if any) - data, warning, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -168,12 +157,8 @@ func TestResolveComponentCredential_MultiVCenter_BothVCentersPopulated(t *testin secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, } - checker := newMock() - setPrivileges(checker, "machine-api@vc1.local", "vcenter1.example.com", machineAPIPrivileges) - setPrivileges(checker, "machine-api@vc2.local", "vcenter2.example.com", machineAPIPrivileges) - data, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter1.example.com", "vcenter2.example.com"}, ) @@ -188,75 +173,6 @@ func TestResolveComponentCredential_MultiVCenter_BothVCentersPopulated(t *testin } } -// ── AC3: Privilege validation invoked before provisioning ──────────────────── - -// TestResolveComponentCredential_PrivilegeValidationInvoked verifies AC3: the Story #37 -// ValidateComponentPrivileges call is made before the secret data is returned. A mock checker -// that records calls is used; provisioning succeeds only when the checker is exercised. -func TestResolveComponentCredential_PrivilegeValidationInvoked(t *testing.T) { - cr := makeCredCR("machineAPI") - - componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ - "vcenter.example.com.username": "machine-api@vsphere.local", - "vcenter.example.com.password": "machine-api-password", - }) - - reader := &stubComponentSecretReader{ - secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, - } - - called := false - recordingChecker := &recordingPrivilegeChecker{ - inner: newMock(), - onCalled: func() { called = true }, - } - setPrivileges(recordingChecker.inner, "machine-api@vsphere.local", "vcenter.example.com", machineAPIPrivileges) - - _, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, recordingChecker, - []string{"vcenter.example.com"}, - ) - - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - if !called { - t.Error("expected PrivilegeChecker.FetchUserPrivileges to be called (AC3 requires validation before provisioning)") - } -} - -// TestResolveComponentCredential_PrivilegeValidationFails_SecretNotProvisioned verifies AC3 (failure -// path): if privilege validation fails, an error is returned and no secret data is returned. -func TestResolveComponentCredential_PrivilegeValidationFails_SecretNotProvisioned(t *testing.T) { - cr := makeCredCR("machineAPI") - - componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ - "vcenter.example.com.username": "machine-api@vsphere.local", - "vcenter.example.com.password": "machine-api-password", - }) - - reader := &stubComponentSecretReader{ - secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, - } - - // Checker grants only a subset of required privileges → validation fails - checker := newMock() - setPrivileges(checker, "machine-api@vsphere.local", "vcenter.example.com", - allExcept(machineAPIPrivileges, "VirtualMachine.Inventory.Create", "VirtualMachine.Inventory.Delete")) - - data, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, - []string{"vcenter.example.com"}, - ) - - if err == nil { - t.Fatal("expected validation error, got nil") - } - if len(data) > 0 { - t.Errorf("expected no secret data when validation fails, got: %v", data) - } -} - // ── Adversarial cases ───────────────────────────────────────────────────────── // TestResolveComponentCredential_NoAnnotation_Passthrough verifies that a CredentialsRequest @@ -265,10 +181,9 @@ func TestResolveComponentCredential_NoAnnotation_Passthrough(t *testing.T) { cr := makeCredCR("") // no annotation reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} - checker := newMock() data, warning, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -296,10 +211,9 @@ func TestResolveComponentCredential_NilAnnotations_Passthrough(t *testing.T) { } reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} - checker := newMock() _, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -314,10 +228,9 @@ func TestResolveComponentCredential_UnknownComponentAnnotation_FallsBackToShared cr := makeCredCR("diagnostics") // not a known component reader := &stubComponentSecretReader{secrets: map[string]*corev1.Secret{}} - checker := newMock() _, warning, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -346,11 +259,8 @@ func TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error(t secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, } - checker := newMock() - setPrivileges(checker, "machine-api@vc1.local", "vcenter1.example.com", machineAPIPrivileges) - _, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter1.example.com", "vcenter2.example.com"}, ) @@ -359,37 +269,6 @@ func TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error(t } } -// TestResolveComponentCredential_AuthFailureDuringValidation_Error verifies that an authentication -// failure from the PrivilegeChecker is surfaced as an error (and not silently treated as -// insufficient privileges — the error type matters for operator status messages). -func TestResolveComponentCredential_AuthFailureDuringValidation_Error(t *testing.T) { - cr := makeCredCR("machineAPI") - - componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ - "vcenter.example.com.username": "machine-api@vsphere.local", - "vcenter.example.com.password": "wrong-password", - }) - - reader := &stubComponentSecretReader{ - secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, - } - - checker := newMock() - setAuthError(checker, "machine-api@vsphere.local", "vcenter.example.com") - - _, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, - []string{"vcenter.example.com"}, - ) - - if err == nil { - t.Fatal("expected error for authentication failure") - } - if !containsInsensitive(err.Error(), "authentication") { - t.Errorf("expected authentication error message, got: %v", err) - } -} - // TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared verifies that a component // secret with no keys (empty Data) is treated equivalently to a missing secret — fallback occurs. func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *testing.T) { @@ -401,10 +280,8 @@ func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *te secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": emptySecret}, } - checker := newMock() - _, warning, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, checker, + context.Background(), cr, reader, sharedSecretData, []string{"vcenter.example.com"}, ) @@ -415,40 +292,3 @@ func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *te t.Error("expected fallback warning for empty component secret") } } - -// ── Helpers used by the stubs above ────────────────────────────────────────── - -// recordingPrivilegeChecker wraps an inner checker and calls onCalled on the first invocation. -type recordingPrivilegeChecker struct { - inner *mockPrivilegeChecker - onCalled func() -} - -func (r *recordingPrivilegeChecker) FetchUserPrivileges(ctx context.Context, username, vcenterFQDN string) ([]string, error) { - if r.onCalled != nil { - r.onCalled() - } - return r.inner.FetchUserPrivileges(ctx, username, vcenterFQDN) -} - -func containsInsensitive(s, substr string) bool { - return fmt.Sprintf("%s", s) != "" && len(s) >= len(substr) && - (s == substr || len(s) > 0 && containsBytes([]byte(s), []byte(substr))) -} - -func containsBytes(haystack, needle []byte) bool { - for i := 0; i <= len(haystack)-len(needle); i++ { - match := true - for j, b := range needle { - hb := haystack[i+j] - if hb != b && hb != b-32 && hb != b+32 { - match = false - break - } - } - if match { - return true - } - } - return false -} diff --git a/pkg/vsphere/actuator/privilege_validation.go b/pkg/vsphere/actuator/privilege_validation.go deleted file mode 100644 index bb90f61b57..0000000000 --- a/pkg/vsphere/actuator/privilege_validation.go +++ /dev/null @@ -1,135 +0,0 @@ -package actuator - -import ( - "context" - "fmt" - "sort" - "strings" -) - -// machineAPIPrivileges lists the 19 vCenter privileges required by the Machine API operator. -var machineAPIPrivileges = []string{ - "Datastore.AllocateSpace", - "Network.Assign", - "Resource.AssignVMToPool", - "VirtualMachine.Config.AddExistingDisk", - "VirtualMachine.Config.AddNewDisk", - "VirtualMachine.Config.AddRemoveDevice", - "VirtualMachine.Config.AdvancedConfig", - "VirtualMachine.Config.CPUCount", - "VirtualMachine.Config.DiskExtend", - "VirtualMachine.Config.EditDevice", - "VirtualMachine.Config.Memory", - "VirtualMachine.Config.RemoveDisk", - "VirtualMachine.Config.Resource", - "VirtualMachine.Config.Settings", - "VirtualMachine.Interact.PowerOff", - "VirtualMachine.Interact.PowerOn", - "VirtualMachine.Interact.Reset", - "VirtualMachine.Inventory.Create", - "VirtualMachine.Inventory.Delete", -} - -// storagePrivileges lists the 6 vCenter privileges required by the CSI driver. -var storagePrivileges = []string{ - "Datastore.AllocateSpace", - "Datastore.FileManagement", - "StoragePod.Config", - "VirtualMachine.Config.AddExistingDisk", - "VirtualMachine.Config.AddNewDisk", - "VirtualMachine.Config.RemoveDisk", -} - -// cloudControllerPrivileges lists the 3 vCenter privileges required by the Cloud Controller Manager. -var cloudControllerPrivileges = []string{ - "System.Read", - "System.View", - "VirtualMachine.Inventory.Create", -} - -// vsphereProblemDetectorPrivileges lists the 2 vCenter privileges required by the vSphere Problem Detector. -var vsphereProblemDetectorPrivileges = []string{ - "Sessions.ValidateSession", - "StorageProfile.View", -} - -// privilegeRequirementsFor returns the required vCenter privileges for the given component. -// Returns nil for unknown components (no requirement → validation skipped). -func privilegeRequirementsFor(component string) []string { - switch component { - case "machine-api": - return machineAPIPrivileges - case "storage": - return storagePrivileges - case "cloud-controller": - return cloudControllerPrivileges - case "vsphere-problem-detector": - return vsphereProblemDetectorPrivileges - default: - return nil - } -} - -// PrivilegeChecker retrieves the vCenter privileges granted to a user account. -type PrivilegeChecker interface { - // FetchUserPrivileges returns the privileges the named user holds on the given vCenter. - // Returns an authentication error (containing "authentication", "invalidlogin", or - // "unauthenticated", case-insensitive) when the credentials are rejected by vCenter. - FetchUserPrivileges(ctx context.Context, username, vcenterFQDN string) ([]string, error) -} - -// ComponentCredential holds the vCenter login information for one component on one vCenter. -type ComponentCredential struct { - Component string - Username string - Password string - VCenterFQDN string -} - -// ValidateComponentPrivileges validates that each credential satisfies the privilege -// requirements for its component role. Returns the first validation error encountered -// (fail-fast semantics across multiple credentials / vCenters). -func ValidateComponentPrivileges(ctx context.Context, checker PrivilegeChecker, creds []ComponentCredential) error { - for _, cred := range creds { - required := privilegeRequirementsFor(cred.Component) - if len(required) == 0 { - continue - } - - actual, err := checker.FetchUserPrivileges(ctx, cred.Username, cred.VCenterFQDN) - if err != nil { - if isAuthError(err) { - return fmt.Errorf("Authentication failed for '%s' on vCenter '%s'", cred.Username, cred.VCenterFQDN) - } - return err - } - - missing := missingPrivileges(required, actual) - if len(missing) > 0 { - sort.Strings(missing) - return fmt.Errorf("Account '%s' on vCenter '%s' missing privileges: %v", cred.Username, cred.VCenterFQDN, missing) - } - } - return nil -} - -func isAuthError(err error) bool { - msg := strings.ToLower(err.Error()) - return strings.Contains(msg, "authentication") || - strings.Contains(msg, "invalidlogin") || - strings.Contains(msg, "unauthenticated") -} - -func missingPrivileges(required, actual []string) []string { - have := make(map[string]struct{}, len(actual)) - for _, p := range actual { - have[p] = struct{}{} - } - var missing []string - for _, p := range required { - if _, ok := have[p]; !ok { - missing = append(missing, p) - } - } - return missing -} diff --git a/pkg/vsphere/actuator/privilege_validation_test.go b/pkg/vsphere/actuator/privilege_validation_test.go deleted file mode 100644 index cc1ea4f65d..0000000000 --- a/pkg/vsphere/actuator/privilege_validation_test.go +++ /dev/null @@ -1,427 +0,0 @@ -package actuator - -import ( - "context" - "fmt" - "strings" - "testing" -) - -// mockPrivilegeChecker implements PrivilegeChecker for unit tests. -type mockPrivilegeChecker struct { - privileges map[string][]string // "username@vcenter" → granted privileges - authErrors map[string]bool // "username@vcenter" → trigger auth failure - otherErrors map[string]error // "username@vcenter" → trigger non-auth error -} - -func newMock() *mockPrivilegeChecker { - return &mockPrivilegeChecker{ - privileges: make(map[string][]string), - authErrors: make(map[string]bool), - otherErrors: make(map[string]error), - } -} - -func (m *mockPrivilegeChecker) FetchUserPrivileges(_ context.Context, username, vcenterFQDN string) ([]string, error) { - key := username + "@" + vcenterFQDN - if m.authErrors[key] { - return nil, fmt.Errorf("authentication failed: InvalidLogin for %s", username) - } - if err, ok := m.otherErrors[key]; ok { - return nil, err - } - return m.privileges[key], nil -} - -func setPrivileges(m *mockPrivilegeChecker, username, vcenter string, privs []string) { - m.privileges[username+"@"+vcenter] = privs -} - -func setAuthError(m *mockPrivilegeChecker, username, vcenter string) { - m.authErrors[username+"@"+vcenter] = true -} - -func setOtherError(m *mockPrivilegeChecker, username, vcenter string, err error) { - m.otherErrors[username+"@"+vcenter] = err -} - -// allExcept returns privs with the named elements removed. -func allExcept(privs []string, exclude ...string) []string { - excSet := make(map[string]struct{}, len(exclude)) - for _, e := range exclude { - excSet[e] = struct{}{} - } - out := make([]string, 0, len(privs)) - for _, p := range privs { - if _, skip := excSet[p]; !skip { - out = append(out, p) - } - } - return out -} - -const ( - vcenter1 = "vcenter1.example.com" - vcenter2 = "vcenter2.example.com" -) - -// --------------------------------------------------------------------------- -// AC1: Missing privilege → CredentialsProvisionFailed condition -// --------------------------------------------------------------------------- - -func TestValidatePrivileges_MachineAPI_MissingOnePrivilege(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, - allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - const wantMsg = "Account 'ocp-machine-api@vsphere.local' on vCenter 'vcenter1.example.com' missing privileges: [VirtualMachine.Config.AddNewDisk]" - if err.Error() != wantMsg { - t.Errorf("got:\n %s\nwant:\n %s", err.Error(), wantMsg) - } -} - -func TestValidatePrivileges_MachineAPI_MissingMultiplePrivileges(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, - allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk", "VirtualMachine.Interact.PowerOn", "Network.Assign")) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - for _, priv := range []string{"Network.Assign", "VirtualMachine.Config.AddNewDisk", "VirtualMachine.Interact.PowerOn"} { - if !strings.Contains(err.Error(), priv) { - t.Errorf("expected %q in error message %q", priv, err.Error()) - } - } -} - -func TestValidatePrivileges_CSIDriver_MissingPrivilege(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, - allExcept(storagePrivileges, "Datastore.FileManagement")) - - creds := []ComponentCredential{ - {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if !strings.Contains(err.Error(), "ocp-csi@vsphere.local") || - !strings.Contains(err.Error(), vcenter1) || - !strings.Contains(err.Error(), "Datastore.FileManagement") { - t.Errorf("unexpected error message: %s", err.Error()) - } -} - -func TestValidatePrivileges_CloudController_MissingPrivilege(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-cloud-controller@vsphere.local", vcenter1, - allExcept(cloudControllerPrivileges, "System.Read")) - - creds := []ComponentCredential{ - {Component: "cloud-controller", Username: "ocp-cloud-controller@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if !strings.Contains(err.Error(), "System.Read") { - t.Errorf("expected System.Read in error: %s", err.Error()) - } -} - -func TestValidatePrivileges_VSphereProblemDetector_MissingPrivilege(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, - allExcept(vsphereProblemDetectorPrivileges, "Sessions.ValidateSession")) - - creds := []ComponentCredential{ - {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if !strings.Contains(err.Error(), "Sessions.ValidateSession") { - t.Errorf("expected Sessions.ValidateSession in error: %s", err.Error()) - } -} - -// --------------------------------------------------------------------------- -// AC2: All privileges present → nil error, provisioning proceeds -// --------------------------------------------------------------------------- - -func TestValidatePrivileges_MachineAPI_FullPrivilegeSet(t *testing.T) { - privs := privilegeRequirementsFor("machine-api") - if len(privs) != 19 { - t.Errorf("machine-api privilege set: got %d; want 19", len(privs)) - } - - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil, got %v", err) - } -} - -func TestValidatePrivileges_CSIDriver_FullPrivilegeSet(t *testing.T) { - privs := privilegeRequirementsFor("storage") - if len(privs) != 6 { - t.Errorf("csi-driver privilege set: got %d; want 6", len(privs)) - } - - m := newMock() - setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, storagePrivileges) - - creds := []ComponentCredential{ - {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil, got %v", err) - } -} - -func TestValidatePrivileges_CloudController_FullPrivilegeSet(t *testing.T) { - privs := privilegeRequirementsFor("cloud-controller") - if len(privs) != 3 { - t.Errorf("cloud-controller privilege set: got %d; want 3", len(privs)) - } - - m := newMock() - setPrivileges(m, "ocp-cc@vsphere.local", vcenter1, cloudControllerPrivileges) - - creds := []ComponentCredential{ - {Component: "cloud-controller", Username: "ocp-cc@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil, got %v", err) - } -} - -func TestValidatePrivileges_VSphereProblemDetector_FullPrivilegeSet(t *testing.T) { - privs := privilegeRequirementsFor("vsphere-problem-detector") - if len(privs) != 2 { - t.Errorf("vsphere-problem-detector privilege set: got %d; want 2", len(privs)) - } - - m := newMock() - setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, vsphereProblemDetectorPrivileges) - - creds := []ComponentCredential{ - {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil, got %v", err) - } -} - -func TestValidatePrivileges_AllComponents_AllPrivilegesPresent(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) - setPrivileges(m, "ocp-csi@vsphere.local", vcenter1, storagePrivileges) - setPrivileges(m, "ocp-cc@vsphere.local", vcenter1, cloudControllerPrivileges) - setPrivileges(m, "ocp-vpd@vsphere.local", vcenter1, vsphereProblemDetectorPrivileges) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, - {Component: "cloud-controller", Username: "ocp-cc@vsphere.local", VCenterFQDN: vcenter1}, - {Component: "vsphere-problem-detector", Username: "ocp-vpd@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil, got %v", err) - } -} - -// --------------------------------------------------------------------------- -// AC3: Authentication failure → "Authentication failed for" message -// --------------------------------------------------------------------------- - -func TestValidatePrivileges_CSIDriver_AuthenticationFailure(t *testing.T) { - m := newMock() - setAuthError(m, "ocp-csi@vsphere.local", vcenter1) - - creds := []ComponentCredential{ - {Component: "storage", Username: "ocp-csi@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - const wantMsg = "Authentication failed for 'ocp-csi@vsphere.local' on vCenter 'vcenter1.example.com'" - if err.Error() != wantMsg { - t.Errorf("got:\n %s\nwant:\n %s", err.Error(), wantMsg) - } -} - -func TestValidatePrivileges_MachineAPI_AuthenticationFailure(t *testing.T) { - m := newMock() - setAuthError(m, "ocp-machine-api@vsphere.local", vcenter1) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if !strings.Contains(err.Error(), "Authentication failed for") || - !strings.Contains(err.Error(), "ocp-machine-api@vsphere.local") || - !strings.Contains(err.Error(), vcenter1) { - t.Errorf("unexpected auth failure message: %s", err.Error()) - } -} - -// --------------------------------------------------------------------------- -// Adversarial cases -// --------------------------------------------------------------------------- - -func TestValidatePrivileges_AccountWithNoPrivileges(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, []string{}) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error when account has no privileges, got nil") - } - for _, priv := range machineAPIPrivileges { - if !strings.Contains(err.Error(), priv) { - t.Errorf("expected %q in error message", priv) - } - } -} - -func TestValidatePrivileges_UnknownComponent_NoRequirements(t *testing.T) { - m := newMock() - creds := []ComponentCredential{ - {Component: "unknown-component", Username: "ocp-unknown@vsphere.local", VCenterFQDN: vcenter1}, - } - if err := ValidateComponentPrivileges(context.Background(), m, creds); err != nil { - t.Errorf("expected nil for unknown component, got %v", err) - } -} - -func TestValidatePrivileges_MultipleVCenters_OneVCenterFails(t *testing.T) { - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, machineAPIPrivileges) - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter2, - allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter2}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error for vcenter2 missing privilege, got nil") - } - if !strings.Contains(err.Error(), vcenter2) { - t.Errorf("error should identify vcenter2 as the failing vCenter: %s", err.Error()) - } -} - -func TestValidatePrivileges_MultipleVCenters_BothFail(t *testing.T) { - // ValidateComponentPrivileges uses fail-fast: returns on the first failure. - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, - allExcept(machineAPIPrivileges, "Network.Assign")) - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter2, - allExcept(machineAPIPrivileges, "VirtualMachine.Config.AddNewDisk")) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter2}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error when both vCenters fail, got nil") - } - if !strings.Contains(err.Error(), vcenter1) { - t.Errorf("fail-fast: expected error about vcenter1 (first), got: %s", err.Error()) - } -} - -func TestValidatePrivileges_FetchPrivileges_NonAuthError(t *testing.T) { - m := newMock() - networkErr := fmt.Errorf("connection refused: timeout connecting to vcenter1.example.com:443") - setOtherError(m, "ocp-machine-api@vsphere.local", vcenter1, networkErr) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if strings.Contains(err.Error(), "Authentication failed") || strings.Contains(err.Error(), "missing privileges") { - t.Errorf("non-auth error was misclassified: %s", err.Error()) - } - if !strings.Contains(err.Error(), "connection refused") { - t.Errorf("expected original error propagated, got: %s", err.Error()) - } -} - -func TestValidatePrivileges_ConditionMessage_DoesNotContainPassword(t *testing.T) { - const sensitivePassword = "s3cr3t-p@ssw0rd-unique" - - m := newMock() - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, - allExcept(machineAPIPrivileges, "Network.Assign")) - - creds := []ComponentCredential{ - { - Component: "machine-api", - Username: "ocp-machine-api@vsphere.local", - Password: sensitivePassword, - VCenterFQDN: vcenter1, - }, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - if strings.Contains(err.Error(), sensitivePassword) { - t.Errorf("condition message must not contain the password; got: %s", err.Error()) - } -} - -func TestValidatePrivileges_MissingPrivileges_SortedInMessage(t *testing.T) { - m := newMock() - // Remove three privileges that sort as: Datastore.AllocateSpace < Network.Assign < VirtualMachine.Inventory.Delete - setPrivileges(m, "ocp-machine-api@vsphere.local", vcenter1, - allExcept(machineAPIPrivileges, "VirtualMachine.Inventory.Delete", "Datastore.AllocateSpace", "Network.Assign")) - - creds := []ComponentCredential{ - {Component: "machine-api", Username: "ocp-machine-api@vsphere.local", VCenterFQDN: vcenter1}, - } - err := ValidateComponentPrivileges(context.Background(), m, creds) - if err == nil { - t.Fatal("expected an error, got nil") - } - msg := err.Error() - iDA := strings.Index(msg, "Datastore.AllocateSpace") - iNA := strings.Index(msg, "Network.Assign") - iVID := strings.Index(msg, "VirtualMachine.Inventory.Delete") - if !(iDA < iNA && iNA < iVID) { - t.Errorf("missing privileges not in sorted order in message: %s", msg) - } -} From 7fa354dc4b89f34c60b4183d659f46c80cda3f35 Mon Sep 17 00:00:00 2001 From: splatypus-bot <282974456+splatypus-bot@users.noreply.github.com> Date: Tue, 12 May 2026 17:10:36 -0400 Subject: [PATCH 4/4] Remove privilege validation from credential distribution Per feedback from @rvanderp3, removed the vCenter credential validation that was checking if credentials exist for every cluster vCenter. Changes: - Removed privilege validation loop from resolveVSphereCredentials - Removed unused vcenters parameter from function signature - Removed test case for missing vCenter validation - Updated all test calls to match new signature Co-Authored-By: Claude Sonnet 4.5 --- .../actuator/credential_distribution.go | 8 ----- .../actuator/credential_distribution_test.go | 35 ------------------- 2 files changed, 43 deletions(-) diff --git a/pkg/vsphere/actuator/credential_distribution.go b/pkg/vsphere/actuator/credential_distribution.go index 441bbbc4c2..682b0a55e7 100644 --- a/pkg/vsphere/actuator/credential_distribution.go +++ b/pkg/vsphere/actuator/credential_distribution.go @@ -43,7 +43,6 @@ func resolveVSphereCredentials( cr *minterv1.CredentialsRequest, componentSecretReader ComponentSecretReader, sharedSecretData map[string][]byte, - vcenters []string, ) (secretData map[string][]byte, warning string, err error) { component := cr.Annotations[vsphereComponentAnnotationKey] if component == "" { @@ -58,13 +57,6 @@ func resolveVSphereCredentials( } if secret != nil && len(secret.Data) > 0 { - // Verify credentials exist for every cluster vCenter before returning. - for _, vcenter := range vcenters { - if _, ok := secret.Data[vcenter+".username"]; !ok { - return nil, "", fmt.Errorf("component secret %q missing credentials for vCenter %q", secretName, vcenter) - } - } - return secret.Data, "", nil } } diff --git a/pkg/vsphere/actuator/credential_distribution_test.go b/pkg/vsphere/actuator/credential_distribution_test.go index 930a8c6f03..dd47d8c1f2 100644 --- a/pkg/vsphere/actuator/credential_distribution_test.go +++ b/pkg/vsphere/actuator/credential_distribution_test.go @@ -8,7 +8,6 @@ // cr *minterv1.CredentialsRequest, // componentSecretReader ComponentSecretReader, // sharedSecretData map[string][]byte, -// vcenters []string, // ) (secretData map[string][]byte, warning string, err error) // // ComponentSecretReader is an interface (or func) that looks up a named secret from kube-system. @@ -98,7 +97,6 @@ func TestResolveComponentCredential_AnnotatedCRWithSecret_UsesPerComponentCred(t data, warning, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil { @@ -123,7 +121,6 @@ func TestResolveComponentCredential_AnnotatedCRWithoutSecret_FallsBackToShared(t data, warning, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil { @@ -159,7 +156,6 @@ func TestResolveComponentCredential_MultiVCenter_BothVCentersPopulated(t *testin data, _, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter1.example.com", "vcenter2.example.com"}, ) if err != nil { @@ -184,7 +180,6 @@ func TestResolveComponentCredential_NoAnnotation_Passthrough(t *testing.T) { data, warning, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil { @@ -214,7 +209,6 @@ func TestResolveComponentCredential_NilAnnotations_Passthrough(t *testing.T) { _, _, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil { @@ -231,7 +225,6 @@ func TestResolveComponentCredential_UnknownComponentAnnotation_FallsBackToShared _, warning, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil { @@ -242,33 +235,6 @@ func TestResolveComponentCredential_UnknownComponentAnnotation_FallsBackToShared } } -// TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error verifies that when a -// component secret is present but lacks credentials for one of the cluster's vCenters, an error -// is returned (not silent omission — the operator would fail to connect to that vCenter). -func TestResolveComponentCredential_MultiVCenter_MissingOneVCenterEntry_Error(t *testing.T) { - cr := makeCredCR("machineAPI") - - // Secret only has vcenter1; cluster has vcenter1 + vcenter2 - componentSecret := makeComponentSecret("vsphere-machine-api-creds", map[string]string{ - "vcenter1.example.com.username": "machine-api@vc1.local", - "vcenter1.example.com.password": "password1", - // vcenter2 entries absent - }) - - reader := &stubComponentSecretReader{ - secrets: map[string]*corev1.Secret{"vsphere-machine-api-creds": componentSecret}, - } - - _, _, err := resolveVSphereCredentials( - context.Background(), cr, reader, sharedSecretData, - []string{"vcenter1.example.com", "vcenter2.example.com"}, - ) - - if err == nil { - t.Error("expected error when component secret is missing credentials for a cluster vCenter") - } -} - // TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared verifies that a component // secret with no keys (empty Data) is treated equivalently to a missing secret — fallback occurs. func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *testing.T) { @@ -282,7 +248,6 @@ func TestResolveComponentCredential_EmptyComponentSecret_FallsBackToShared(t *te _, warning, err := resolveVSphereCredentials( context.Background(), cr, reader, sharedSecretData, - []string{"vcenter.example.com"}, ) if err != nil {