Reapply reverted changes from PR devfile#954

Reapply changes from PR devfile#954 (commits ac944d5..25d533b) that had been
reverted (PR devfile#972)

The original PR was reverted due to required changes in the Che
Operator. As those are now applied, the original PR can be re-applied.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

controllers/workspace/devworkspace_controller.go

@@ -39,6 +39,7 @@ import (
 	"github.com/devfile/devworkspace-operator/pkg/provision/storage"
 	"github.com/devfile/devworkspace-operator/pkg/provision/sync"
 	wsprovision "github.com/devfile/devworkspace-operator/pkg/provision/workspace"
+	"github.com/devfile/devworkspace-operator/pkg/provision/workspace/rbac"
 	"github.com/devfile/devworkspace-operator/pkg/timing"
 	"github.com/go-logr/logr"
 	"github.com/google/uuid"
@@ -85,7 +86,8 @@ type DevWorkspaceReconciler struct {
 // +kubebuilder:rbac:groups="batch",resources=jobs,verbs=get;create;list;watch;update;patch;delete
 // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=mutatingwebhookconfigurations;validatingwebhookconfigurations,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews;localsubjectaccessreviews,verbs=create
-// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings;clusterroles;clusterrolebindings,verbs=get;list;watch;create;update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;delete
 // +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch;delete;deletecollection
 // +kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;create
 // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get,resourceNames=cluster
@@ -285,7 +287,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 storageProvisioner.NeedsStorage(&workspace.Spec.Template) {
+	if storageProvisioner.NeedsStorage(&workspace.Spec.Template) && !coputil.HasFinalizer(clusterWorkspace, constants.StorageCleanupFinalizer) {
 		coputil.AddFinalizer(clusterWorkspace, constants.StorageCleanupFinalizer)
 		if err := r.Update(ctx, clusterWorkspace.DevWorkspace); err != nil {
 			return reconcile.Result{}, err
@@ -343,9 +345,24 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request
 
 	timing.SetTime(timingInfo, timing.ComponentsReady)
 
-	rbacStatus := wsprovision.SyncRBAC(workspace, clusterAPI)
-	if rbacStatus.Err != nil || !rbacStatus.Continue {
-		return reconcile.Result{Requeue: true}, rbacStatus.Err
+	// Add finalizer to ensure workspace rolebinding gets cleaned up when workspace
+	// is deleted.
+	if !coputil.HasFinalizer(clusterWorkspace, constants.RBACCleanupFinalizer) {
+		coputil.AddFinalizer(clusterWorkspace, constants.RBACCleanupFinalizer)
+		if err := r.Update(ctx, clusterWorkspace.DevWorkspace); err != nil {
+			return reconcile.Result{}, err
+		}
+	}
+	if err := rbac.SyncRBAC(workspace, clusterAPI); err != nil {
+		switch rbacErr := err.(type) {
+		case *rbac.RetryError:
+			reqLogger.Info(rbacErr.Error())
+			return reconcile.Result{Requeue: true}, nil
+		case *rbac.FailError:
+			return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning rbac: %s", rbacErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus)
+		default:
+			return reconcile.Result{}, err
+		}
 	}
 
 	// Step two: Create routing, and wait for routing to be ready
@@ -434,12 +451,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")
 	}

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 {

deploy/templates/components/rbac/role.yaml

@@ -200,10 +200,20 @@ rules:
   resources:
   - clusterrolebindings
   - clusterroles
+  verbs:
+  - create
+  - get
+  - list
+  - update
+  - watch
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
   - rolebindings
   - roles
   verbs:
   - create
+  - delete
   - get
   - list
   - update

pkg/cache/cache.go

@@ -16,7 +16,6 @@ package cache
 import (
 	"fmt"
 
-	"github.com/devfile/devworkspace-operator/pkg/common"
 	"github.com/devfile/devworkspace-operator/pkg/constants"
 	"github.com/devfile/devworkspace-operator/pkg/infrastructure"
 	routev1 "github.com/openshift/api/route/v1"
@@ -25,7 +24,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
-	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 )
@@ -48,6 +46,10 @@ func GetCacheFunc() (cache.NewCacheFunc, error) {
 	if err != nil {
 		return nil, err
 	}
+	rbacObjectSelector, err := labels.Parse("controller.devfile.io/workspace-rbac=true")
+	if err != nil {
+		return nil, err
+	}
 
 	selectors := cache.SelectorsByObject{
 		&appsv1.Deployment{}: {
@@ -75,10 +77,10 @@ func GetCacheFunc() (cache.NewCacheFunc, error) {
 			Label: secretObjectSelector,
 		},
 		&rbacv1.Role{}: {
-			Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.WorkspaceRoleName()}),
+			Label: rbacObjectSelector,
 		},
 		&rbacv1.RoleBinding{}: {
-			Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.WorkspaceRolebindingName()}),
+			Label: rbacObjectSelector,
 		},
 	}
 

pkg/common/naming.go

@@ -119,21 +119,45 @@ func MetadataConfigMapName(workspaceId string) string {
 // can potentially push the name over the 63 character limit (if the original
 // object has a long name)
 func AutoMountConfigMapVolumeName(volumeName string) string {
-	return fmt.Sprintf("%s", volumeName)
+	return volumeName
 }
 
 func AutoMountSecretVolumeName(volumeName string) string {
-	return fmt.Sprintf("%s", volumeName)
+	return volumeName
 }
 
 func AutoMountPVCVolumeName(pvcName string) string {
-	return fmt.Sprintf("%s", pvcName)
+	return pvcName
 }
 
 func WorkspaceRoleName() string {
-	return "workspace"
+	return "devworkspace-default-role"
 }
 
 func WorkspaceRolebindingName() string {
+	return "devworkspace-default-rolebinding"
+}
+
+func WorkspaceSCCRoleName(sccName string) string {
+	return fmt.Sprintf("devworkspace-use-%s", sccName)
+}
+
+func WorkspaceSCCRolebindingName(sccName string) string {
+	return fmt.Sprintf("devworkspace-use-%s", sccName)
+}
+
+// OldWorkspaceRoleName returns the name used for the workspace serviceaccount role
+//
+// Deprecated: use WorkspaceRoleName() instead.
+// TODO: remove for DevWorkspace Operator v0.19
+func OldWorkspaceRoleName() string {
+	return "workspace"
+}
+
+// OldWorkspaceRolebindingName returns the name used for the workspace serviceaccount rolebinding
+//
+// Deprecated: use WorkspaceRoleBindingName() instead.
+// TODO: remove for DevWorkspace Operator v0.19
+func OldWorkspaceRolebindingName() string {
 	return constants.ServiceAccount + "dw"
 }

pkg/constants/finalizers.go

@@ -20,5 +20,13 @@ 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
+	// serviceaccount is added to the workspace rolebinding, it is necessary to remove it
+	// when a workspace is deleted
+	RBACCleanupFinalizer = "rbac.controller.devfile.io"
 )