fix(rbac): implement registry rolebinding finalization for OpenShift

Author: danielpenad

pkg/provision/workspace/rbac/finalize.go

@@ -16,6 +16,7 @@ package rbac
 import (
 	"github.com/devfile/devworkspace-operator/pkg/common"
 	"github.com/devfile/devworkspace-operator/pkg/constants"
+	"github.com/devfile/devworkspace-operator/pkg/infrastructure"
 	"github.com/devfile/devworkspace-operator/pkg/provision/sync"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -42,11 +43,16 @@ func FinalizeRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI)
 		if err := deleteRolebinding(rolebindingName, workspace.Namespace, api); err != nil {
 			return err
 		}
-		return nil
-	}
-	if err := removeServiceAccountFromRolebinding(saName, workspace.Namespace, rolebindingName, api); err != nil {
+	} else if err := removeServiceAccountFromRolebinding(saName, workspace.Namespace, rolebindingName, api); err != nil {
 		return err
 	}
+
+	if infrastructure.IsOpenShift() {
+		if err := finalizeRegistryRBAC(workspace, api); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -74,6 +80,19 @@ func finalizeSCCRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterA
 	return nil
 }
 
+func finalizeRegistryRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error {
+	saName := common.ServiceAccountName(workspace)
+	rolebindingName := common.RegistryImagePullerRolebindingName(workspace.Namespace)
+	numWorkspaces, err := countNonDeletedWorkspaces(workspace.Namespace, api)
+	if err != nil {
+		return err
+	}
+	if numWorkspaces == 0 {
+		return deleteRolebinding(rolebindingName, workspace.Namespace, api)
+	}
+	return removeServiceAccountFromRolebinding(saName, workspace.Namespace, rolebindingName, api)
+}
+
 func countNonDeletedWorkspaces(namespace string, api sync.ClusterAPI) (int, error) {
 	count := 0
 	allWorkspaces := &dw.DevWorkspaceList{}

pkg/provision/workspace/rbac/finalize_test.go

@@ -133,6 +133,85 @@ func TestFinalizeDoesNothingWhenRolebindingDoesNotExist(t *testing.T) {
 	}
 }
 
+func TestShouldRemoveWorkspaceSAFromRegistryRolebindingWhenDeletedOnOpenShift(t *testing.T) {
+	infrastructure.InitializeForTesting(infrastructure.OpenShiftv4)
+	testdw := getTestDevWorkspace("test-devworkspace")
+	testdw2 := getTestDevWorkspace("test-devworkspace2")
+	testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	testdw.DevWorkspace.ObjectMeta.Finalizers = []string{"test-finalizer"}
+	testdwSAName := common.ServiceAccountName(testdw)
+	testdw2SAName := common.ServiceAccountName(testdw2)
+	registryRolebinding := newRegistryImagePullerRolebinding(
+		rbacv1.Subject{
+			Kind:      rbacv1.ServiceAccountKind,
+			Name:      testdwSAName,
+			Namespace: testNamespace,
+		},
+		rbacv1.Subject{
+			Kind:      rbacv1.ServiceAccountKind,
+			Name:      testdw2SAName,
+			Namespace: testNamespace,
+		})
+	api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, registryRolebinding)
+	retryErr := &dwerrors.RetryError{}
+	err := FinalizeRBAC(testdw, api)
+	if assert.Error(t, err, "Should return error to indicate registry rolebinding updated") {
+		assert.ErrorAs(t, err, &retryErr, "Error should be RetryError")
+	}
+	err = FinalizeRBAC(testdw, api)
+	assert.NoError(t, err, "Should not return error once registry rolebinding is in sync")
+
+	actualRolebinding := &rbacv1.RoleBinding{}
+	err = api.Client.Get(api.Ctx, types.NamespacedName{
+		Name:      common.RegistryImagePullerRolebindingName(testNamespace),
+		Namespace: testNamespace,
+	}, actualRolebinding)
+	assert.NoError(t, err, "Unexpected test error getting registry rolebinding")
+	assert.False(t, testHasSubject(testdwSAName, testNamespace, actualRolebinding), "Should remove deleted workspace SA from registry rolebinding subjects")
+	assert.True(t, testHasSubject(testdw2SAName, testNamespace, actualRolebinding), "Should leave other workspace SA in registry rolebinding subjects")
+}
+
+func TestDeletesRegistryRolebindingWhenLastWorkspaceIsDeletedOnOpenShift(t *testing.T) {
+	infrastructure.InitializeForTesting(infrastructure.OpenShiftv4)
+	testdw := getTestDevWorkspace("test-devworkspace")
+	testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	testdw.DevWorkspace.ObjectMeta.Finalizers = []string{"test-finalizer"}
+	registryRolebinding := newRegistryImagePullerRolebinding(rbacv1.Subject{
+		Kind:      rbacv1.ServiceAccountKind,
+		Name:      common.ServiceAccountName(testdw),
+		Namespace: testNamespace,
+	})
+	api := getTestClusterAPI(t, testdw.DevWorkspace, registryRolebinding)
+	retryErr := &dwerrors.RetryError{}
+	err := FinalizeRBAC(testdw, api)
+	if assert.Error(t, err, "Should return error to indicate registry rolebinding deleted") {
+		assert.ErrorAs(t, err, &retryErr, "Error should be RetryError")
+		assert.Regexp(t, fmt.Sprintf("deleted rolebinding .* in namespace %s", testNamespace), err.Error())
+	}
+	err = FinalizeRBAC(testdw, api)
+	assert.NoError(t, err, "Should not return error once registry rolebinding is deleted")
+
+	actualRolebinding := &rbacv1.RoleBinding{}
+	err = api.Client.Get(api.Ctx, types.NamespacedName{
+		Name:      common.RegistryImagePullerRolebindingName(testNamespace),
+		Namespace: testNamespace,
+	}, actualRolebinding)
+	if assert.Error(t, err, "Expect error when getting deleted registry rolebinding") {
+		assert.True(t, k8sErrors.IsNotFound(err), "Error should have IsNotFound type")
+	}
+}
+
+func TestFinalizeDoesNothingWhenRegistryRolebindingDoesNotExistOnOpenShift(t *testing.T) {
+	infrastructure.InitializeForTesting(infrastructure.OpenShiftv4)
+	testdw := getTestDevWorkspace("test-devworkspace")
+	testdw2 := getTestDevWorkspace("test-devworkspace2")
+	testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	testdw.DevWorkspace.ObjectMeta.Finalizers = []string{"test-finalizer"}
+	api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace)
+	err := FinalizeRBAC(testdw, api)
+	assert.NoError(t, err, "Should not return error when registry rolebinding does not exist")
+}
+
 func TestDeletesSCCRoleAndRolebindingWhenLastWorkspaceIsDeleted(t *testing.T) {
 	infrastructure.InitializeForTesting(infrastructure.Kubernetes)
 	testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName)
@@ -237,3 +316,13 @@ func TestFinalizeDoesNothingWhenSCCRolebindingDoesNotExist(t *testing.T) {
 		assert.True(t, k8sErrors.IsNotFound(err), "Error should have IsNotFound type")
 	}
 }
+
+func newRegistryImagePullerRolebinding(subjects ...rbacv1.Subject) *rbacv1.RoleBinding {
+	rolebinding := generateDefaultRolebinding(
+		common.RegistryImagePullerRolebindingName(testNamespace),
+		testNamespace,
+		common.RegistryImagePullerRoleName(),
+		constants.RbacClusterRoleKind)
+	rolebinding.Subjects = append(rolebinding.Subjects, subjects...)
+	return rolebinding
+}