diff --git a/controllers/cli_download_controller.go b/controllers/cli_download_controller.go index d6001d517d..73dedb3216 100644 --- a/controllers/cli_download_controller.go +++ b/controllers/cli_download_controller.go @@ -75,7 +75,10 @@ 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 the dedicated ServiceAccount used by the CLI server pod. + // 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, @@ -93,6 +96,53 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe 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 @@ -125,7 +175,12 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe } if deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName { deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName - deployment.Spec.Template.Spec.AutomountServiceAccountToken = desired.Spec.Template.Spec.AutomountServiceAccountToken + 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 { diff --git a/controllers/cli_download_controller_test.go b/controllers/cli_download_controller_test.go index ea2e956e06..99d6dffec2 100644 --- a/controllers/cli_download_controller_test.go +++ b/controllers/cli_download_controller_test.go @@ -1,12 +1,44 @@ package controllers import ( + "context" "testing" + "github.com/go-logr/logr" + consolev1 "github.com/openshift/api/console/v1" + routev1 "github.com/openshift/api/route/v1" + 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/util/intstr" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// getCLIDownloadTestScheme returns a scheme with the console/route API groups +// registered, suitable for use with a fake client in reconciliation tests. +func getCLIDownloadTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + if err := consolev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add consolev1 to scheme: %v", err) + } + if err := routev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add routev1 to scheme: %v", err) + } + return scheme.Scheme +} + +// newTestCLIRoute returns a CLI server route with a hostname already +// assigned, so reconcileCLIResources doesn't hit its hostname-assignment +// retry/backoff path. +func newTestCLIRoute(namespace string) *routev1.Route { + route := buildCLIServerRoute(namespace) + route.Spec.Host = "cli.example.com" + return route +} + func TestBuildCLIServerDeployment(t *testing.T) { const ( testNamespace = "openshift-adp" @@ -112,3 +144,56 @@ func TestBuildCLIServerDeployment_SecurityContext(t *testing.T) { t.Error("expected ALL capabilities to be dropped") } } + +func TestReconcileCLIResources_ReconcilesExistingServiceAccount(t *testing.T) { + namespace := "openshift-adp" + testScheme := getCLIDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + // Simulate a ServiceAccount that was created before this reconciliation + // logic existed: token automounting is left at its default (true), + // required labels are missing, and there's no owner reference. + automountTrue := true + existingServiceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: cliServerServiceAccountName, + Namespace: namespace, + }, + AutomountServiceAccountToken: &automountTrue, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingServiceAccount, newTestCLIRoute(namespace)). + Build() + + setup := &CLIDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileCLIResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileCLIResources returned error: %v", err) + } + + updated := &corev1.ServiceAccount{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: cliServerServiceAccountName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated service account: %v", err) + } + + if updated.AutomountServiceAccountToken == nil || *updated.AutomountServiceAccountToken { + t.Error("expected AutomountServiceAccountToken to be reconciled to false") + } + if updated.Labels[managedByLabel] != operatorName { + t.Errorf("expected label %q=%q to be backfilled, got %q", managedByLabel, operatorName, updated.Labels[managedByLabel]) + } + if len(updated.OwnerReferences) == 0 { + t.Error("expected an owner reference to be added") + } +}