Skip to content

Commit

Permalink
Stop adding workspace SA to SCC and don't use SA finalizer
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@amisevsk
amisevsk committed Nov 1, 2022
1 parent 08a0106 commit 776dd45
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 65 deletions.
6 changes: 0 additions & 6 deletions controllers/workspace/devworkspace_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
43 changes: 43 additions & 0 deletions controllers/workspace/finalize.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions pkg/constants/finalizers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
65 changes: 6 additions & 59 deletions pkg/provision/workspace/serviceaccount.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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)

Expand Down

0 comments on commit 776dd45

Please sign in to comment.