diff --git a/internal/controller/cli_download_controller.go b/internal/controller/cli_download_controller.go index 8313950baa..537804347b 100644 --- a/internal/controller/cli_download_controller.go +++ b/internal/controller/cli_download_controller.go @@ -22,12 +22,13 @@ import ( ) const ( - cliServerDeploymentName = "openshift-adp-oadp-cli-server" - cliServerServiceName = "openshift-adp-cli-server" - cliServerRouteName = "oadp-cli-server-route" - cliDownloadName = "openshift-adp-oadp-cli" - managedByLabel = "app.kubernetes.io/managed-by" - operatorName = "oadp-operator" + cliServerDeploymentName = "openshift-adp-oadp-cli-server" + cliServerServiceName = "openshift-adp-cli-server" + cliServerServiceAccountName = "openshift-adp-cli-server" + cliServerRouteName = "oadp-cli-server-route" + cliDownloadName = "openshift-adp-oadp-cli" + managedByLabel = "app.kubernetes.io/managed-by" + operatorName = "oadp-operator" ) // CLIDownloadSetup is a runnable that sets up CLI download resources when the operator starts @@ -74,9 +75,80 @@ func (c *CLIDownloadSetup) Start(ctx context.Context) error { // reconcileCLIResources creates or updates all CLI-related resources // This is idempotent - it will reuse existing resources if they already exist func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDeployment *appsv1.Deployment, cliServerImage string) error { - // 1. Create or update the deployment - deployment := &appsv1.Deployment{} + // 1. Create or reconcile the dedicated ServiceAccount used by the CLI server pod. + // A non-default ServiceAccount is required (rather than relying on the + // namespace's "default" SA) to satisfy access-control best practices, + // even though this workload needs no Kubernetes API permissions. + serviceAccount := &corev1.ServiceAccount{} err := c.Client.Get(ctx, client.ObjectKey{ + Name: cliServerServiceAccountName, + Namespace: c.Namespace, + }, serviceAccount) + + if errors.IsNotFound(err) { + serviceAccount = buildCLIServerServiceAccount(c.Namespace) + if err := controllerutil.SetOwnerReference(operatorDeployment, serviceAccount, c.Client.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference on service account: %w", err) + } + err = c.Client.Create(ctx, serviceAccount) + if err != nil && !errors.IsAlreadyExists(err) { + return fmt.Errorf("failed to create CLI server service account: %w", err) + } + c.Log.Info("Created CLI server service account") + } else if err != nil { + return fmt.Errorf("failed to get CLI server service account: %w", err) + } else { + // ServiceAccount already exists — reconcile it to ensure desired state. + desired := buildCLIServerServiceAccount(c.Namespace) + needsUpdate := false + + // Ensure AutomountServiceAccountToken is explicitly false. + if serviceAccount.AutomountServiceAccountToken == nil || + *serviceAccount.AutomountServiceAccountToken != *desired.AutomountServiceAccountToken { + serviceAccount.AutomountServiceAccountToken = desired.AutomountServiceAccountToken + needsUpdate = true + } + + // Ensure required labels are present. + if serviceAccount.Labels == nil { + serviceAccount.Labels = make(map[string]string) + } + for k, v := range desired.Labels { + if serviceAccount.Labels[k] != v { + serviceAccount.Labels[k] = v + needsUpdate = true + } + } + + // Check for the pre-existing owner reference BEFORE mutating. + // SetOwnerReference is idempotent and will add the reference if it's + // missing, so checking the list *after* calling it would always find + // a match and mask the fact that an update was actually needed. + hasOwnerRef := false + for _, ref := range serviceAccount.OwnerReferences { + if ref.UID == operatorDeployment.UID { + hasOwnerRef = true + break + } + } + if err := controllerutil.SetOwnerReference(operatorDeployment, serviceAccount, c.Client.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference on service account: %w", err) + } + if !hasOwnerRef { + needsUpdate = true + } + + if needsUpdate { + if err := c.Client.Update(ctx, serviceAccount); err != nil { + return fmt.Errorf("failed to update CLI server service account: %w", err) + } + c.Log.Info("Updated CLI server service account") + } + } + + // 2. Create or update the deployment + deployment := &appsv1.Deployment{} + err = c.Client.Get(ctx, client.ObjectKey{ Name: cliServerDeploymentName, Namespace: c.Namespace, }, deployment) @@ -95,19 +167,35 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe c.Log.Info("Created CLI server deployment", "image", cliServerImage) } else if err != nil { return fmt.Errorf("failed to get CLI server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - // Deployment exists from a version before probes were added; backfill them. + } else { + // Deployment exists from a version before probes/service account were added; backfill them. desired := buildCLIServerDeployment(c.Namespace, cliServerImage) - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe - deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe - if err := c.Client.Update(ctx, deployment); err != nil { - return fmt.Errorf("failed to update CLI server deployment with probes: %w", err) + needsUpdate := false + if len(deployment.Spec.Template.Spec.Containers) > 0 && + deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { + deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe + deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe + needsUpdate = true + } + if deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName { + deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName + needsUpdate = true + } + desiredAutomount := desired.Spec.Template.Spec.AutomountServiceAccountToken + currentAutomount := deployment.Spec.Template.Spec.AutomountServiceAccountToken + if desiredAutomount != nil && (currentAutomount == nil || *currentAutomount != *desiredAutomount) { + deployment.Spec.Template.Spec.AutomountServiceAccountToken = desiredAutomount + needsUpdate = true + } + if needsUpdate { + if err := c.Client.Update(ctx, deployment); err != nil { + return fmt.Errorf("failed to update CLI server deployment: %w", err) + } + c.Log.Info("Updated CLI server deployment with readiness/liveness probes and/or service account") } - c.Log.Info("Updated CLI server deployment with readiness/liveness probes") } - // 2. Create or update the service + // 3. Create or update the service service := &corev1.Service{} err = c.Client.Get(ctx, client.ObjectKey{ Name: cliServerServiceName, @@ -130,7 +218,7 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe return fmt.Errorf("failed to get CLI server service: %w", err) } - // 3. Create or get the route + // 4. Create or get the route route := &routev1.Route{} err = c.Client.Get(ctx, client.ObjectKey{ Name: cliServerRouteName, @@ -200,7 +288,7 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe } } - // 4. Create or update ConsoleCLIDownload (cluster-scoped) + // 5. Create or update ConsoleCLIDownload (cluster-scoped) // Note: This is idempotent. If the ConsoleCLIDownload already exists from a previous // installation, we'll reuse it and update the URL to point to our route. // We use a fixed name to prevent duplicates across operator reinstalls. @@ -278,6 +366,7 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { runAsNonRoot := true allowPrivilegeEscalation := false readOnlyRootFilesystem := true + automountServiceAccountToken := false return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -302,6 +391,8 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { }, }, Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + AutomountServiceAccountToken: &automountServiceAccountToken, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: &runAsNonRoot, }, @@ -362,6 +453,21 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { } } +func buildCLIServerServiceAccount(namespace string) *corev1.ServiceAccount { + automountServiceAccountToken := false + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: cliServerServiceAccountName, + Namespace: namespace, + Labels: map[string]string{ + "app": "oadp-cli", + managedByLabel: operatorName, + }, + }, + AutomountServiceAccountToken: &automountServiceAccountToken, + } +} + func buildCLIServerService(namespace string) *corev1.Service { return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/cli_download_controller_test.go b/internal/controller/cli_download_controller_test.go new file mode 100644 index 0000000000..60bd7b095b --- /dev/null +++ b/internal/controller/cli_download_controller_test.go @@ -0,0 +1,236 @@ +package controller + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestBuildCLIServerDeployment_ServiceAccount(t *testing.T) { + deployment := buildCLIServerDeployment("openshift-adp", "test-image") + podSpec := deployment.Spec.Template.Spec + + if podSpec.ServiceAccountName != cliServerServiceAccountName { + t.Errorf("expected serviceAccountName %q, got %q", cliServerServiceAccountName, podSpec.ServiceAccountName) + } + if podSpec.AutomountServiceAccountToken == nil || *podSpec.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be false") + } +} + +func TestBuildCLIServerServiceAccount(t *testing.T) { + const testNamespace = "openshift-adp" + + sa := buildCLIServerServiceAccount(testNamespace) + + if sa.Name != cliServerServiceAccountName { + t.Errorf("expected name %q, got %q", cliServerServiceAccountName, sa.Name) + } + if sa.Namespace != testNamespace { + t.Errorf("expected namespace %q, got %q", testNamespace, sa.Namespace) + } + if sa.AutomountServiceAccountToken == nil || *sa.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be false") + } + if sa.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q, got %q", managedByLabel, operatorName, sa.Labels[managedByLabel]) + } +} + +// newCLITestScheme registers only corev1 and appsv1 — enough for the +// ServiceAccount and Deployment steps of reconcileCLIResources to succeed, +// but not routev1/consolev1. This means reconcileCLIResources will return an +// error once it reaches the Route step (step 4), which is expected and +// ignored: by that point the ServiceAccount step (step 1) has already run +// and persisted its result, which is what these tests verify. +func newCLITestScheme(t *testing.T) *runtime.Scheme { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := appsv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + return scheme +} + +func newCLITestOperatorDeployment() *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oadp-operator", + Namespace: "openshift-adp", + UID: types.UID("test-operator-uid"), + }, + } +} + +// TestReconcileCLIResources_CreatesServiceAccountWhenMissing verifies the +// ServiceAccount is created with the desired state when it doesn't exist. +func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) { + const ns = "openshift-adp" + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy).Build() + + setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + // Expect an error once reconcileCLIResources reaches the unregistered + // Route/ConsoleCLIDownload steps — irrelevant to this test. + if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get created SA: %v", err) + } + if got.AutomountServiceAccountToken == nil || *got.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken=false on created SA") + } + if got.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q, got %q", managedByLabel, operatorName, got.Labels[managedByLabel]) + } + if len(got.OwnerReferences) == 0 || got.OwnerReferences[0].UID != operatorDeploy.UID { + t.Error("expected owner reference to be set on created SA") + } +} + +// TestReconcileCLIResources_FixesExistingServiceAccountDrift verifies that +// when the ServiceAccount already exists with AutomountServiceAccountToken +// unset/true, missing labels, and a missing owner reference, all three are +// corrected by reconcileCLIResources. +func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) { + const ns = "openshift-adp" + trueVal := true + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + existing := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: cliServerServiceAccountName, + Namespace: ns, + // no labels, no owner reference + }, + AutomountServiceAccountToken: &trueVal, // wrong: should be false + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + + setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + if got.AutomountServiceAccountToken == nil || *got.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be corrected to false") + } + if got.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q to be added, got %q", managedByLabel, operatorName, got.Labels[managedByLabel]) + } + found := false + for _, ref := range got.OwnerReferences { + if ref.UID == operatorDeploy.UID { + found = true + break + } + } + if !found { + t.Error("expected owner reference to be added") + } +} + +// TestReconcileCLIResources_FixesMissingOwnerReferenceOnly is the key +// regression test: an existing SA that already has the correct automount +// setting and labels, but is MISSING the owner reference, must still be +// updated. This guards against a bug where SetOwnerReference was called +// before checking whether the reference was already present, which always +// found a match (since SetOwnerReference had just added it) and silently +// skipped the Update() call. +func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { + const ns = "openshift-adp" + falseVal := false + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + existing := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: cliServerServiceAccountName, + Namespace: ns, + Labels: map[string]string{ + "app": "oadp-cli", + managedByLabel: operatorName, + }, + // everything else already matches desired state — only the + // owner reference is missing + }, + AutomountServiceAccountToken: &falseVal, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + + setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + found := false + for _, ref := range got.OwnerReferences { + if ref.UID == operatorDeploy.UID { + found = true + break + } + } + if !found { + t.Error("expected owner reference to operatorDeployment to be added, but it was missing after reconcile") + } +} + +// TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect verifies a +// ServiceAccount already matching the desired state is not unnecessarily +// updated (no ResourceVersion bump). +func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { + const ns = "openshift-adp" + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + desired := buildCLIServerServiceAccount(ns) + desired.OwnerReferences = []metav1.OwnerReference{ + {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() + + before := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, before); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + after := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, after); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + if before.ResourceVersion != after.ResourceVersion { + t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + } +} diff --git a/internal/controller/vmdp_download_controller.go b/internal/controller/vmdp_download_controller.go index 32c1e8e8e8..45b9619611 100644 --- a/internal/controller/vmdp_download_controller.go +++ b/internal/controller/vmdp_download_controller.go @@ -22,10 +22,11 @@ import ( ) const ( - vmdpServerDeploymentName = "openshift-adp-oadp-vmdp-server" - vmdpServerServiceName = "openshift-adp-vmdp-server" - vmdpServerRouteName = "oadp-vmdp-server-route" - vmdpDownloadName = "openshift-adp-oadp-vmdp" + vmdpServerDeploymentName = "openshift-adp-oadp-vmdp-server" + vmdpServerServiceName = "openshift-adp-vmdp-server" + vmdpServerServiceAccountName = "openshift-adp-vmdp-server" + vmdpServerRouteName = "oadp-vmdp-server-route" + vmdpDownloadName = "openshift-adp-oadp-vmdp" ) // VMDPDownloadSetup is a runnable that sets up VMDP download resources when the operator starts @@ -71,9 +72,80 @@ func (v *VMDPDownloadSetup) Start(ctx context.Context) error { // reconcileVMDPResources creates or updates all VMDP download-related resources func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operatorDeployment *appsv1.Deployment, vmdpServerImage string) error { - // 1. Create or update the deployment - deployment := &appsv1.Deployment{} + // 1. Create or reconcile the dedicated ServiceAccount used by the VMDP server pod. + // A non-default ServiceAccount is required (rather than relying on the + // namespace's "default" SA) to satisfy access-control best practices, + // even though this workload needs no Kubernetes API permissions. + serviceAccount := &corev1.ServiceAccount{} err := v.Client.Get(ctx, client.ObjectKey{ + Name: vmdpServerServiceAccountName, + Namespace: v.Namespace, + }, serviceAccount) + + if errors.IsNotFound(err) { + serviceAccount = buildVMDPServerServiceAccount(v.Namespace) + if err := controllerutil.SetOwnerReference(operatorDeployment, serviceAccount, v.Client.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference on service account: %w", err) + } + err = v.Client.Create(ctx, serviceAccount) + if err != nil && !errors.IsAlreadyExists(err) { + return fmt.Errorf("failed to create VMDP server service account: %w", err) + } + v.Log.Info("Created VMDP server service account") + } else if err != nil { + return fmt.Errorf("failed to get VMDP server service account: %w", err) + } else { + // ServiceAccount already exists — reconcile it to ensure desired state. + desired := buildVMDPServerServiceAccount(v.Namespace) + needsUpdate := false + + // Ensure AutomountServiceAccountToken is explicitly false. + if serviceAccount.AutomountServiceAccountToken == nil || + *serviceAccount.AutomountServiceAccountToken != *desired.AutomountServiceAccountToken { + serviceAccount.AutomountServiceAccountToken = desired.AutomountServiceAccountToken + needsUpdate = true + } + + // Ensure required labels are present. + if serviceAccount.Labels == nil { + serviceAccount.Labels = make(map[string]string) + } + for k, val := range desired.Labels { + if serviceAccount.Labels[k] != val { + serviceAccount.Labels[k] = val + needsUpdate = true + } + } + + // Check for the pre-existing owner reference BEFORE mutating. + // SetOwnerReference is idempotent and will add the reference if it's + // missing, so checking the list *after* calling it would always find + // a match and mask the fact that an update was actually needed. + hasOwnerRef := false + for _, ref := range serviceAccount.OwnerReferences { + if ref.UID == operatorDeployment.UID { + hasOwnerRef = true + break + } + } + if err := controllerutil.SetOwnerReference(operatorDeployment, serviceAccount, v.Client.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference on service account: %w", err) + } + if !hasOwnerRef { + needsUpdate = true + } + + if needsUpdate { + if err := v.Client.Update(ctx, serviceAccount); err != nil { + return fmt.Errorf("failed to update VMDP server service account: %w", err) + } + v.Log.Info("Updated VMDP server service account") + } + } + + // 2. Create or update the deployment + deployment := &appsv1.Deployment{} + err = v.Client.Get(ctx, client.ObjectKey{ Name: vmdpServerDeploymentName, Namespace: v.Namespace, }, deployment) @@ -90,19 +162,35 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator v.Log.Info("Created VMDP server deployment", "image", vmdpServerImage) } else if err != nil { return fmt.Errorf("failed to get VMDP server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - // Deployment exists from a version before probes were added; backfill them. + } else { + // Deployment exists from a version before probes/service account were added; backfill them. desired := buildVMDPServerDeployment(v.Namespace, vmdpServerImage) - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe - deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe - if err := v.Client.Update(ctx, deployment); err != nil { - return fmt.Errorf("failed to update VMDP server deployment with probes: %w", err) + needsUpdate := false + if len(deployment.Spec.Template.Spec.Containers) > 0 && + deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { + deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe + deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe + needsUpdate = true + } + if deployment.Spec.Template.Spec.ServiceAccountName != vmdpServerServiceAccountName { + deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName + needsUpdate = true + } + desiredAutomount := desired.Spec.Template.Spec.AutomountServiceAccountToken + currentAutomount := deployment.Spec.Template.Spec.AutomountServiceAccountToken + if desiredAutomount != nil && (currentAutomount == nil || *currentAutomount != *desiredAutomount) { + deployment.Spec.Template.Spec.AutomountServiceAccountToken = desiredAutomount + needsUpdate = true + } + if needsUpdate { + if err := v.Client.Update(ctx, deployment); err != nil { + return fmt.Errorf("failed to update VMDP server deployment: %w", err) + } + v.Log.Info("Updated VMDP server deployment with readiness/liveness probes and/or service account") } - v.Log.Info("Updated VMDP server deployment with readiness/liveness probes") } - // 2. Create or update the service + // 3. Create or update the service service := &corev1.Service{} err = v.Client.Get(ctx, client.ObjectKey{ Name: vmdpServerServiceName, @@ -123,7 +211,7 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator return fmt.Errorf("failed to get VMDP server service: %w", err) } - // 3. Create or get the route + // 4. Create or get the route route := &routev1.Route{} err = v.Client.Get(ctx, client.ObjectKey{ Name: vmdpServerRouteName, @@ -188,7 +276,7 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator } } - // 4. Create or update ConsoleCLIDownload (cluster-scoped) + // 5. Create or update ConsoleCLIDownload (cluster-scoped) downloadURL := fmt.Sprintf("https://%s/", hostname) consoleCLIDownload := &consolev1.ConsoleCLIDownload{} @@ -260,6 +348,7 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { runAsNonRoot := true allowPrivilegeEscalation := false readOnlyRootFilesystem := true + automountServiceAccountToken := false return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -284,6 +373,8 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { }, }, Spec: corev1.PodSpec{ + ServiceAccountName: vmdpServerServiceAccountName, + AutomountServiceAccountToken: &automountServiceAccountToken, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: &runAsNonRoot, }, @@ -344,6 +435,21 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { } } +func buildVMDPServerServiceAccount(namespace string) *corev1.ServiceAccount { + automountServiceAccountToken := false + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmdpServerServiceAccountName, + Namespace: namespace, + Labels: map[string]string{ + "app": "oadp-vmdp", + managedByLabel: operatorName, + }, + }, + AutomountServiceAccountToken: &automountServiceAccountToken, + } +} + func buildVMDPServerService(namespace string) *corev1.Service { return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/vmdp_download_controller_test.go b/internal/controller/vmdp_download_controller_test.go new file mode 100644 index 0000000000..3212079365 --- /dev/null +++ b/internal/controller/vmdp_download_controller_test.go @@ -0,0 +1,210 @@ +package controller + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestBuildVMDPServerDeployment_ServiceAccount(t *testing.T) { + deployment := buildVMDPServerDeployment("openshift-adp", "test-image") + podSpec := deployment.Spec.Template.Spec + + if podSpec.ServiceAccountName != vmdpServerServiceAccountName { + t.Errorf("expected serviceAccountName %q, got %q", vmdpServerServiceAccountName, podSpec.ServiceAccountName) + } + if podSpec.AutomountServiceAccountToken == nil || *podSpec.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be false") + } +} + +func TestBuildVMDPServerServiceAccount(t *testing.T) { + const testNamespace = "openshift-adp" + + sa := buildVMDPServerServiceAccount(testNamespace) + + if sa.Name != vmdpServerServiceAccountName { + t.Errorf("expected name %q, got %q", vmdpServerServiceAccountName, sa.Name) + } + if sa.Namespace != testNamespace { + t.Errorf("expected namespace %q, got %q", testNamespace, sa.Namespace) + } + if sa.AutomountServiceAccountToken == nil || *sa.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be false") + } + if sa.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q, got %q", managedByLabel, operatorName, sa.Labels[managedByLabel]) + } +} + +// TestReconcileVMDPResources_CreatesServiceAccountWhenMissing verifies the +// ServiceAccount is created with the desired state when it doesn't exist. +// Uses newCLITestScheme/newCLITestOperatorDeployment (defined in +// cli_download_controller_test.go) since the same scheme applies here: +// corev1+appsv1 registered, routev1/consolev1 deliberately omitted so +// reconcileVMDPResources errors out cleanly at the Route step, after the +// ServiceAccount step under test has already run. +func TestReconcileVMDPResources_CreatesServiceAccountWhenMissing(t *testing.T) { + const ns = "openshift-adp" + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy).Build() + + setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get created SA: %v", err) + } + if got.AutomountServiceAccountToken == nil || *got.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken=false on created SA") + } + if got.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q, got %q", managedByLabel, operatorName, got.Labels[managedByLabel]) + } + if len(got.OwnerReferences) == 0 || got.OwnerReferences[0].UID != operatorDeploy.UID { + t.Error("expected owner reference to be set on created SA") + } +} + +// TestReconcileVMDPResources_FixesExistingServiceAccountDrift verifies that +// when the ServiceAccount already exists with AutomountServiceAccountToken +// unset/true, missing labels, and a missing owner reference, all three are +// corrected by reconcileVMDPResources. +func TestReconcileVMDPResources_FixesExistingServiceAccountDrift(t *testing.T) { + const ns = "openshift-adp" + trueVal := true + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + existing := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmdpServerServiceAccountName, + Namespace: ns, + // no labels, no owner reference + }, + AutomountServiceAccountToken: &trueVal, // wrong: should be false + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + + setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + if got.AutomountServiceAccountToken == nil || *got.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be corrected to false") + } + if got.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q to be added, got %q", managedByLabel, operatorName, got.Labels[managedByLabel]) + } + found := false + for _, ref := range got.OwnerReferences { + if ref.UID == operatorDeploy.UID { + found = true + break + } + } + if !found { + t.Error("expected owner reference to be added") + } +} + +// TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly is the key +// regression test: an existing SA that already has the correct automount +// setting and labels, but is MISSING the owner reference, must still be +// updated. This guards against a bug where SetOwnerReference was called +// before checking whether the reference was already present, which always +// found a match (since SetOwnerReference had just added it) and silently +// skipped the Update() call. +func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { + const ns = "openshift-adp" + falseVal := false + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + existing := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmdpServerServiceAccountName, + Namespace: ns, + Labels: map[string]string{ + "app": "oadp-vmdp", + managedByLabel: operatorName, + }, + // everything else already matches desired state — only the + // owner reference is missing + }, + AutomountServiceAccountToken: &falseVal, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + + setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + got := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, got); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + found := false + for _, ref := range got.OwnerReferences { + if ref.UID == operatorDeploy.UID { + found = true + break + } + } + if !found { + t.Error("expected owner reference to operatorDeployment to be added, but it was missing after reconcile") + } +} + +// TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect verifies a +// ServiceAccount already matching the desired state is not unnecessarily +// updated (no ResourceVersion bump). +func TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { + const ns = "openshift-adp" + scheme := newCLITestScheme(t) + operatorDeploy := newCLITestOperatorDeployment() + + desired := buildVMDPServerServiceAccount(ns) + desired.OwnerReferences = []metav1.OwnerReference{ + {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() + + before := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, before); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} + if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { + t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + } + + after := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, after); err != nil { + t.Fatalf("failed to get SA: %v", err) + } + + if before.ResourceVersion != after.ResourceVersion { + t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + } +}