From 74780ccb36b3ac0f60c8fbd8983b7299d54d0c2d Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Fri, 19 Nov 2021 15:55:20 -0500 Subject: [PATCH] Add finalizer for SA SCCs when they're used and clean up on deletion Signed-off-by: Angel Misevski --- .../workspace/devworkspace_controller.go | 9 ++- controllers/workspace/finalize.go | 46 +++++++++++-- pkg/provision/workspace/serviceaccount.go | 67 +++++++++++++++++++ 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index af50194a1..9a317ec5a 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/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") diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index d28a17723..a7c1d3275 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -33,13 +33,23 @@ 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) @@ -47,6 +57,18 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, 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) { diff --git a/pkg/provision/workspace/serviceaccount.go b/pkg/provision/workspace/serviceaccount.go index d9db10fbe..3aafddaf0 100644 --- a/pkg/provision/workspace/serviceaccount.go +++ b/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 +}