From 30866a673848e9622bae074c1848002c3d555aaa Mon Sep 17 00:00:00 2001 From: Daniel Pena Docampo Date: Mon, 8 Jun 2026 16:57:50 +0200 Subject: [PATCH] fix(rbac): implement registry rolebinding finalization for OpenShift Signed-off-by: Daniel Pena Docampo --- pkg/provision/workspace/rbac/finalize.go | 25 +++++- pkg/provision/workspace/rbac/finalize_test.go | 89 +++++++++++++++++++ 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/pkg/provision/workspace/rbac/finalize.go b/pkg/provision/workspace/rbac/finalize.go index 04485d78d..181af482e 100644 --- a/pkg/provision/workspace/rbac/finalize.go +++ b/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{} diff --git a/pkg/provision/workspace/rbac/finalize_test.go b/pkg/provision/workspace/rbac/finalize_test.go index 788ae9e86..3debd486c 100644 --- a/pkg/provision/workspace/rbac/finalize_test.go +++ b/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 +}