From 776dd456f206ae1145bb269709d1674a7883d333 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 20:42:15 -0600 Subject: [PATCH] Stop adding workspace SA to SCC and don't use SA finalizer Since SCCs are added to serviceaccounts via roles and rolebindings, it's no longer necessary to update SCCs to add workspace serviceaccounts as users. This step is removed, as is functionality for automatically adding the SA finalizer to workspaces. However, code related to finalizing workspaces and cleaning up changes made to SCCs is kept intact in order to ensure any workspaces that previously used the finalizer can still be deleted cleanly. Signed-off-by: Angel Misevski --- .../workspace/devworkspace_controller.go | 6 -- controllers/workspace/finalize.go | 43 ++++++++++++ pkg/constants/finalizers.go | 3 + pkg/provision/workspace/serviceaccount.go | 65 ++----------------- 4 files changed, 52 insertions(+), 65 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 887288c10..8117be1d1 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -447,12 +447,6 @@ 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, constants.ServiceAccountCleanupFinalizer) - if err := r.Update(ctx, clusterWorkspace.DevWorkspace); 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 2a3e3549a..1151858d1 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -24,6 +24,7 @@ import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/provision/sync" + "github.com/devfile/devworkspace-operator/pkg/provision/workspace/rbac" "github.com/go-logr/logr" coputil "github.com/redhat-cop/operator-utils/pkg/util" @@ -70,6 +71,8 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, return r.finalizeStorage(ctx, log, workspace, finalizeStatus) case constants.ServiceAccountCleanupFinalizer: return r.finalizeServiceAccount(ctx, log, workspace, finalizeStatus) + case constants.RBACCleanupFinalizer: + return r.finalizeRBAC(ctx, log, workspace, finalizeStatus) } } return reconcile.Result{}, nil @@ -130,6 +133,46 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L return reconcile.Result{}, r.Update(ctx, workspace.DevWorkspace) } +func (r *DevWorkspaceReconciler) finalizeRBAC(ctx context.Context, log logr.Logger, workspace *common.DevWorkspaceWithConfig, finalizeStatus *currentStatus) (reconcile.Result, error) { + terminating, err := r.namespaceIsTerminating(ctx, workspace.Namespace) + if err != nil { + return reconcile.Result{}, err + } else if terminating { + // Namespace is terminating, it's redundant to update roles/rolebindings since they will be removed with the workspace + log.Info("Namespace is terminating; clearing storage finalizer") + coputil.RemoveFinalizer(workspace, constants.RBACCleanupFinalizer) + return reconcile.Result{}, r.Update(ctx, workspace.DevWorkspace) + } + + if err := rbac.FinalizeRBAC(workspace, sync.ClusterAPI{ + Ctx: ctx, + Client: r.Client, + Scheme: r.Scheme, + Logger: log, + }); err != nil { + switch rbacErr := err.(type) { + case *rbac.RetryError: + log.Info(rbacErr.Error()) + return reconcile.Result{Requeue: true}, nil + case *rbac.FailError: + if workspace.Status.Phase != dw.DevWorkspaceStatusError { + // Avoid repeatedly logging error unless it's relevant + log.Error(rbacErr, "Failed to finalize workspace RBAC") + } + finalizeStatus.phase = dw.DevWorkspaceStatusError + finalizeStatus.setConditionTrue(dw.DevWorkspaceError, err.Error()) + return reconcile.Result{}, nil + default: + return reconcile.Result{}, err + } + } + log.Info("RBAC cleanup successful; clearing finalizer") + coputil.RemoveFinalizer(workspace, constants.RBACCleanupFinalizer) + return reconcile.Result{}, r.Update(ctx, workspace.DevWorkspace) +} + +// Deprecated: Only required to support old workspaces that use the service account finalizer. The service account finalizer should +// not be added to new workspaces. func (r *DevWorkspaceReconciler) finalizeServiceAccount(ctx context.Context, log logr.Logger, workspace *common.DevWorkspaceWithConfig, finalizeStatus *currentStatus) (reconcile.Result, error) { retry, err := wsprovision.FinalizeServiceAccount(workspace, ctx, r.NonCachingClient) if err != nil { diff --git a/pkg/constants/finalizers.go b/pkg/constants/finalizers.go index 79d6533bb..d098b7267 100644 --- a/pkg/constants/finalizers.go +++ b/pkg/constants/finalizers.go @@ -20,6 +20,9 @@ const ( // ServiceAccountCleanupFinalizer is used to block DevWorkspace deletion when it is // necessary to clean up additional non-workspace roles added to the workspace // serviceaccount + // + // Deprecated: Will not be added to new workspaces but needs to be tracked for + // removal to ensure workspaces that used it previously will be cleaned up. ServiceAccountCleanupFinalizer = "serviceaccount.controller.devfile.io" // RBACCleanupFinalizer is used to block DevWorkspace deletion in order to ensure // the workspace role and rolebinding are cleaned up correctly. Since each workspace diff --git a/pkg/provision/workspace/serviceaccount.go b/pkg/provision/workspace/serviceaccount.go index 637961aa2..fa0793136 100644 --- a/pkg/provision/workspace/serviceaccount.go +++ b/pkg/provision/workspace/serviceaccount.go @@ -19,7 +19,6 @@ import ( "context" "fmt" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" securityv1 "github.com/openshift/api/security/v1" @@ -94,17 +93,6 @@ func SyncServiceAccount( return ServiceAcctProvisioningStatus{ProvisioningStatus: ProvisioningStatus{Err: err}} } - if workspace.Spec.Template.Attributes.Exists(constants.WorkspaceSCCAttribute) { - sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) - retry, err := addSCCToServiceAccount(specSA.Name, specSA.Namespace, sccName, clusterAPI) - if err != nil { - return ServiceAcctProvisioningStatus{ProvisioningStatus: ProvisioningStatus{FailStartup: true, Message: err.Error()}} - } - if retry { - return ServiceAcctProvisioningStatus{ProvisioningStatus: ProvisioningStatus{Requeue: true}} - } - } - return ServiceAcctProvisioningStatus{ ProvisioningStatus: ProvisioningStatus{ Continue: true, @@ -113,16 +101,10 @@ 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 -} - +// FinalizeServiceAccount removes the workspace service account from the SCC specified by the controller.devfile.io/scc attribute. +// +// Deprecated: This should no longer be needed as the serviceaccount finalizer is no longer added to workspaces (and workspaces +// do not update SCCs) but is kept here in order to clear finalizers from existing workspaces on deletion. func FinalizeServiceAccount(workspace *common.DevWorkspaceWithConfig, ctx context.Context, nonCachingClient crclient.Client) (retry bool, err error) { saName := common.ServiceAccountName(workspace) namespace := workspace.Namespace @@ -134,43 +116,8 @@ func FinalizeServiceAccount(workspace *common.DevWorkspaceWithConfig, ctx contex 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) - - scc := &securityv1.SecurityContextConstraints{} - if err := clusterAPI.NonCachingClient.Get(clusterAPI.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 - } - } - - for _, user := range scc.Users { - if user == serviceaccount { - // This serviceaccount is already added to the SCC - return false, nil - } - } - - scc.Users = append(scc.Users, serviceaccount) - if err := clusterAPI.NonCachingClient.Update(clusterAPI.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 -} - +// Deprecated: This function is left in place ot ensure changes to SCCs can be undone when a workspace is deleted. However, +// the DevWorkspace Operator no longer updates SCCs, so this functionality is not required for new workspaces. 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)