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)