[oadp-1.3] OADP-8450: Reconciling the ServiceAccount

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 {

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")
+	}
+}