Add finalizer for SA SCCs when they're used and clean up on deletion

Signed-off-by: Angel Misevski <[email protected]>

Author: amisevsk

controllers/workspace/devworkspace_controller.go

@@ -260,7 +260,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request
 
 	// Set finalizer on DevWorkspace if necessary
 	// Note: we need to check the flattened workspace to see if a finalizer is needed, as plugins could require storage
-	if isFinalizerNecessary(workspace, storageProvisioner) {
+	if storageProvisioner.NeedsStorage(&workspace.Spec.Template) {
 		coputil.AddFinalizer(clusterWorkspace, storageCleanupFinalizer)
 		if err := r.Update(ctx, clusterWorkspace); err != nil {
 			return reconcile.Result{}, err
@@ -367,6 +367,13 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		}
 		return reconcile.Result{Requeue: serviceAcctStatus.Requeue}, serviceAcctStatus.Err
 	}
+	if wsprovision.NeedsServiceAccountFinalizer(&workspace.Spec.Template) {
+		coputil.AddFinalizer(clusterWorkspace, serviceAccountCleanupFinalizer)
+		if err := r.Update(ctx, clusterWorkspace); err != nil {
+			return reconcile.Result{}, err
+		}
+	}
+
 	serviceAcctName := serviceAcctStatus.ServiceAccountName
 	reconcileStatus.setConditionTrue(dw.DevWorkspaceServiceAccountReady, "DevWorkspace serviceaccount ready")
 

controllers/workspace/finalize.go

@@ -33,20 +33,42 @@ import (
 )
 
 const (
-	storageCleanupFinalizer = "storage.controller.devfile.io"
+	storageCleanupFinalizer        = "storage.controller.devfile.io"
+	serviceAccountCleanupFinalizer = "serviceaccount.controller.devfile.io"
 )
 
-func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
-	if !coputil.HasFinalizer(workspace, storageCleanupFinalizer) {
-		return reconcile.Result{}, nil
+func (r *DevWorkspaceReconciler) workspaceNeedsFinalize(workspace *dw.DevWorkspace) bool {
+	for _, finalizer := range workspace.Finalizers {
+		if finalizer == storageCleanupFinalizer {
+			return true
+		}
+		if finalizer == serviceAccountCleanupFinalizer {
+			return true
+		}
 	}
+	return false
+}
+
+func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
 	workspace.Status.Message = "Cleaning up resources for deletion"
 	workspace.Status.Phase = devworkspacePhaseTerminating
 	err := r.Client.Status().Update(ctx, workspace)
 	if err != nil && !k8sErrors.IsConflict(err) {
 		return reconcile.Result{}, err
 	}
 
+	for _, finalizer := range workspace.Finalizers {
+		switch finalizer {
+		case storageCleanupFinalizer:
+			return r.finalizeStorage(ctx, log, workspace)
+		case serviceAccountCleanupFinalizer:
+			return r.finalizeServiceAccount(ctx, log, workspace)
+		}
+	}
+	return reconcile.Result{}, nil
+}
+
+func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
 	// Need to make sure Deployment is cleaned up before starting job to avoid mounting issues for RWO PVCs
 	wait, err := wsprovision.DeleteWorkspaceDeployment(ctx, workspace, r.Client)
 	if err != nil {
@@ -98,8 +120,20 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger,
 	return reconcile.Result{}, r.Update(ctx, workspace)
 }
 
-func isFinalizerNecessary(workspace *dw.DevWorkspace, provisioner storage.Provisioner) bool {
-	return provisioner.NeedsStorage(&workspace.Spec.Template)
+func (r *DevWorkspaceReconciler) finalizeServiceAccount(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (reconcile.Result, error) {
+	retry, err := wsprovision.FinalizeServiceAccount(workspace, ctx, r.NonCachingClient)
+	if err != nil {
+		log.Error(err, "Failed to finalize workspace ServiceAccount")
+		failedStatus := currentStatus{phase: dw.DevWorkspaceStatusError}
+		failedStatus.setConditionTrue(dw.DevWorkspaceError, err.Error())
+		return r.updateWorkspaceStatus(workspace, r.Log, &failedStatus, reconcile.Result{}, nil)
+	}
+	if retry {
+		return reconcile.Result{Requeue: true}, nil
+	}
+	log.Info("ServiceAccount clean up successful; clearing finalizer")
+	coputil.RemoveFinalizer(workspace, serviceAccountCleanupFinalizer)
+	return reconcile.Result{}, r.Update(ctx, workspace)
 }
 
 func (r *DevWorkspaceReconciler) namespaceIsTerminating(ctx context.Context, namespace string) (bool, error) {

pkg/provision/workspace/serviceaccount.go

@@ -16,6 +16,7 @@
 package workspace
 
 import (
+	"context"
 	"fmt"
 
 	dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -26,6 +27,7 @@ import (
 	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/devfile/devworkspace-operator/pkg/common"
@@ -103,6 +105,27 @@ func SyncServiceAccount(
 	}
 }
 
+func NeedsServiceAccountFinalizer(workspace *dw.DevWorkspaceTemplateSpec) bool {
+	if !workspace.Attributes.Exists(constants.WorkspaceSCCAttribute) {
+		return false
+	}
+	if workspace.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) == "" {
+		return false
+	}
+	return true
+}
+
+func FinalizeServiceAccount(workspace *dw.DevWorkspace, ctx context.Context, nonCachingClient crclient.Client) (retry bool, err error) {
+	saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId)
+	namespace := workspace.Namespace
+	if !workspace.Spec.Template.Attributes.Exists(constants.WorkspaceSCCAttribute) {
+		return false, nil
+	}
+	sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil)
+
+	return removeSCCFromServiceAccount(saName, namespace, sccName, ctx, nonCachingClient)
+}
+
 func addSCCToServiceAccount(saName, namespace, sccName string, clusterAPI sync.ClusterAPI) (retry bool, err error) {
 	serviceaccount := fmt.Sprintf("system:serviceaccount:%s:%s", namespace, saName)
 
@@ -139,3 +162,47 @@ func addSCCToServiceAccount(saName, namespace, sccName string, clusterAPI sync.C
 
 	return false, nil
 }
+
+func removeSCCFromServiceAccount(saName, namespace, sccName string, ctx context.Context, nonCachingClient crclient.Client) (retry bool, err error) {
+	serviceaccount := fmt.Sprintf("system:serviceaccount:%s:%s", namespace, saName)
+
+	scc := &securityv1.SecurityContextConstraints{}
+	if err := nonCachingClient.Get(ctx, types.NamespacedName{Name: sccName}, scc); err != nil {
+		switch {
+		case k8sErrors.IsForbidden(err):
+			return false, fmt.Errorf("operator does not have permissions to get the '%s' SecurityContextConstraints", sccName)
+		case k8sErrors.IsNotFound(err):
+			return false, fmt.Errorf("requested SecurityContextConstraints '%s' not found on cluster", sccName)
+		default:
+			return false, err
+		}
+	}
+
+	found := false
+	var newUsers []string
+	for _, user := range scc.Users {
+		if user == serviceaccount {
+			found = true
+			continue
+		}
+		newUsers = append(newUsers, user)
+	}
+	if !found {
+		return false, err
+	}
+
+	scc.Users = newUsers
+
+	if err := nonCachingClient.Update(ctx, scc); err != nil {
+		switch {
+		case k8sErrors.IsForbidden(err):
+			return false, fmt.Errorf("operator does not have permissions to update the '%s' SecurityContextConstraints", sccName)
+		case k8sErrors.IsConflict(err):
+			return true, nil
+		default:
+			return false, err
+		}
+	}
+
+	return false, nil
+}