diff --git a/internal/controller/cli_download_controller.go b/internal/controller/cli_download_controller.go index 8313950baa..3a44e5c99c 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,33 @@ 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 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) + } + + // 2. Create or update the deployment + deployment := &appsv1.Deployment{} + err = c.Client.Get(ctx, client.ObjectKey{ Name: cliServerDeploymentName, Namespace: c.Namespace, }, deployment) @@ -95,19 +120,30 @@ 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 + deployment.Spec.Template.Spec.AutomountServiceAccountToken = desired.Spec.Template.Spec.AutomountServiceAccountToken + 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 +166,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 +236,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 +314,7 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { runAsNonRoot := true allowPrivilegeEscalation := false readOnlyRootFilesystem := true + automountServiceAccountToken := false return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -302,6 +339,8 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { }, }, Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + AutomountServiceAccountToken: &automountServiceAccountToken, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: &runAsNonRoot, }, @@ -362,6 +401,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..ae0f953454 --- /dev/null +++ b/internal/controller/cli_download_controller_test.go @@ -0,0 +1,34 @@ +package controller + +import "testing" + +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]) + } +} diff --git a/internal/controller/vmdp_download_controller.go b/internal/controller/vmdp_download_controller.go index 32c1e8e8e8..087a6974e0 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,33 @@ 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 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) + } + + // 2. Create or update the deployment + deployment := &appsv1.Deployment{} + err = v.Client.Get(ctx, client.ObjectKey{ Name: vmdpServerDeploymentName, Namespace: v.Namespace, }, deployment) @@ -90,19 +115,30 @@ 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 + deployment.Spec.Template.Spec.AutomountServiceAccountToken = desired.Spec.Template.Spec.AutomountServiceAccountToken + 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 +159,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 +224,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 +296,7 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { runAsNonRoot := true allowPrivilegeEscalation := false readOnlyRootFilesystem := true + automountServiceAccountToken := false return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -284,6 +321,8 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { }, }, Spec: corev1.PodSpec{ + ServiceAccountName: vmdpServerServiceAccountName, + AutomountServiceAccountToken: &automountServiceAccountToken, SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: &runAsNonRoot, }, @@ -344,6 +383,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..0566129bfb --- /dev/null +++ b/internal/controller/vmdp_download_controller_test.go @@ -0,0 +1,34 @@ +package controller + +import "testing" + +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]) + } +}