From 4bd0bc5267605338823754feacd81dfd8ca25e1f Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 14:50:10 -0600 Subject: [PATCH 01/10] Begin process of deprecating previous role and rolebinding functionality The default way to provision roles and rolebindings in DevWorkspace namespaces was by creating roles and bindings with static names. This needs to be extended and so the previous way of provisioning needs to be safely deprecated. Signed-off-by: Angel Misevski --- pkg/cache/cache.go | 4 ++-- pkg/common/naming.go | 10 +++++----- pkg/provision/workspace/rbac.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index baaeec5fa..3caacab50 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -75,10 +75,10 @@ func GetCacheFunc() (cache.NewCacheFunc, error) { Label: secretObjectSelector, }, &rbacv1.Role{}: { - Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.WorkspaceRoleName()}), + Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.OldWorkspaceRoleName()}), }, &rbacv1.RoleBinding{}: { - Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.WorkspaceRolebindingName()}), + Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.OldWorkspaceRolebindingName()}), }, } diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 853e7a347..76c69fe34 100644 --- a/pkg/common/naming.go +++ b/pkg/common/naming.go @@ -119,21 +119,21 @@ 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 { +func OldWorkspaceRoleName() string { return "workspace" } -func WorkspaceRolebindingName() string { +func OldWorkspaceRolebindingName() string { return constants.ServiceAccount + "dw" } diff --git a/pkg/provision/workspace/rbac.go b/pkg/provision/workspace/rbac.go index 096015e51..73393719d 100644 --- a/pkg/provision/workspace/rbac.go +++ b/pkg/provision/workspace/rbac.go @@ -51,7 +51,7 @@ func generateRBAC(namespace string) []client.Object { return []client.Object{ &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ - Name: common.WorkspaceRoleName(), + Name: common.OldWorkspaceRoleName(), Namespace: namespace, }, Rules: []rbacv1.PolicyRule{ @@ -106,7 +106,7 @@ func generateRBAC(namespace string) []client.Object { }, &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: common.WorkspaceRolebindingName(), + Name: common.OldWorkspaceRolebindingName(), Namespace: namespace, }, RoleRef: rbacv1.RoleRef{ From 9a16d1ff3dd244b8b46d11d167d51d8ef48de034 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 17:45:28 -0600 Subject: [PATCH 02/10] Add new way of handling workspace RBAC that supports SCCs Add package pkg/provision/workspace/rbac to handle workspace roles and rolebindings more carefully. For regular workspaces, each namespace gets one role and rolebinding. The rolebinding is updated to have all workspace serviceaccounts in the namespace as subjects, and this list is updated when workspaces are deleted. If all workspaces in a namespace are deleted, then the role and rolebinding are deleted as well. If a workspaces uses the 'controller.devfile.io/scc' attribute, an additional role+binding for that SCC name is created and managed in the same way. Signed-off-by: Angel Misevski --- pkg/common/naming.go | 16 ++ pkg/provision/workspace/rbac/common.go | 56 +++++++ pkg/provision/workspace/rbac/finalize.go | 116 ++++++++++++++ pkg/provision/workspace/rbac/role.go | 147 ++++++++++++++++++ pkg/provision/workspace/rbac/rolebinding.go | 160 ++++++++++++++++++++ 5 files changed, 495 insertions(+) create mode 100644 pkg/provision/workspace/rbac/common.go create mode 100644 pkg/provision/workspace/rbac/finalize.go create mode 100644 pkg/provision/workspace/rbac/role.go create mode 100644 pkg/provision/workspace/rbac/rolebinding.go diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 76c69fe34..7873d8755 100644 --- a/pkg/common/naming.go +++ b/pkg/common/naming.go @@ -130,6 +130,22 @@ func AutoMountPVCVolumeName(pvcName string) string { return pvcName } +func WorkspaceRoleName() string { + 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) +} + func OldWorkspaceRoleName() string { return "workspace" } diff --git a/pkg/provision/workspace/rbac/common.go b/pkg/provision/workspace/rbac/common.go new file mode 100644 index 000000000..442257db4 --- /dev/null +++ b/pkg/provision/workspace/rbac/common.go @@ -0,0 +1,56 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" +) + +type RetryError struct { + Err error +} + +func (e *RetryError) Error() string { + return e.Err.Error() +} + +type FailError struct { + Err error +} + +func (e *FailError) Error() string { + return e.Err.Error() +} + +func wrapSyncError(err error) error { + switch syncErr := err.(type) { + case *sync.NotInSyncError: + return &RetryError{syncErr} + case *sync.UnrecoverableSyncError: + return &FailError{syncErr} + default: + return err + } +} + +func SyncRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + if err := syncRoles(workspace, api); err != nil { + return err + } + if err := syncRolebindings(workspace, api); err != nil { + return err + } + return nil +} diff --git a/pkg/provision/workspace/rbac/finalize.go b/pkg/provision/workspace/rbac/finalize.go new file mode 100644 index 000000000..da71f28bf --- /dev/null +++ b/pkg/provision/workspace/rbac/finalize.go @@ -0,0 +1,116 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" + "sigs.k8s.io/controller-runtime/pkg/client" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +) + +func FinalizeRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + if workspace.Spec.Template.Attributes.Exists(constants.WorkspaceSCCAttribute) { + if err := finalizeSCCRBAC(workspace, api); err != nil { + return err + } + } + saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + roleName := common.WorkspaceRoleName() + rolebindingName := common.WorkspaceRolebindingName() + numWorkspaces, err := countNonDeletedWorkspaces(workspace.Namespace, api) + if err != nil { + return err + } + if numWorkspaces == 0 { + if err := deleteRole(roleName, workspace.Namespace, api); err != nil { + return err + } + if err := deleteRolebinding(rolebindingName, workspace.Namespace, api); err != nil { + return err + } + return nil + } + if err := removeServiceAccountFromRolebinding(saName, workspace.Namespace, rolebindingName, api); err != nil { + return err + } + return nil +} + +func finalizeSCCRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) + saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + roleName := common.WorkspaceSCCRoleName(sccName) + rolebindingName := common.WorkspaceSCCRolebindingName(sccName) + numWorkspaces, err := countNonDeletedWorkspacesUsingSCC(sccName, workspace.Namespace, api) + if err != nil { + return err + } + if numWorkspaces == 0 { + if err := deleteRole(roleName, workspace.Namespace, api); err != nil { + return err + } + if err := deleteRolebinding(rolebindingName, workspace.Namespace, api); err != nil { + return err + } + return nil + } + if err := removeServiceAccountFromRolebinding(saName, workspace.Namespace, rolebindingName, api); err != nil { + return err + } + return nil +} + +func countNonDeletedWorkspaces(namespace string, api sync.ClusterAPI) (int, error) { + count := 0 + allWorkspaces := &dw.DevWorkspaceList{} + allWorkspacesListOptions := &client.ListOptions{ + Namespace: namespace, + } + if err := api.Client.List(api.Ctx, allWorkspaces, allWorkspacesListOptions); err != nil { + return -1, err + } + for _, workspace := range allWorkspaces.Items { + if workspace.DeletionTimestamp != nil { + // ignore workspaces that are being deleted + continue + } + count = count + 1 + } + return count, nil +} + +func countNonDeletedWorkspacesUsingSCC(sccName, namespace string, api sync.ClusterAPI) (int, error) { + count := 0 + allWorkspaces := &dw.DevWorkspaceList{} + allWorkspacesListOptions := &client.ListOptions{ + Namespace: namespace, + } + if err := api.Client.List(api.Ctx, allWorkspaces, allWorkspacesListOptions); err != nil { + return -1, err + } + for _, workspace := range allWorkspaces.Items { + if workspace.DeletionTimestamp != nil { + // ignore workspaces that are being deleted + continue + } + attrs := workspace.Spec.Template.Attributes + if attrs.Exists(constants.WorkspaceSCCAttribute) && attrs.GetString(constants.WorkspaceSCCAttribute, nil) == sccName { + count = count + 1 + } + } + return count, nil +} diff --git a/pkg/provision/workspace/rbac/role.go b/pkg/provision/workspace/rbac/role.go new file mode 100644 index 000000000..12d946e5b --- /dev/null +++ b/pkg/provision/workspace/rbac/role.go @@ -0,0 +1,147 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "fmt" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" + + rbacv1 "k8s.io/api/rbac/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func syncRoles(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + defaultRole := generateDefaultRole(workspace.Namespace) + if _, err := sync.SyncObjectWithCluster(defaultRole, api); err != nil { + return wrapSyncError(err) + } + if !workspace.Spec.Template.Attributes.Exists(constants.WorkspaceSCCAttribute) { + return nil + } + sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) + sccRole := generateUseRoleForSCC(workspace.Namespace, sccName) + if _, err := sync.SyncObjectWithCluster(sccRole, api); err != nil { + return wrapSyncError(err) + } + return nil +} + +func deleteRole(name, namespace string, api sync.ClusterAPI) error { + role := &rbacv1.Role{} + namespacedName := types.NamespacedName{ + Name: name, + Namespace: namespace, + } + err := api.Client.Get(api.Ctx, namespacedName, role) + switch { + case err == nil: + if err := api.Client.Delete(api.Ctx, role); err != nil { + return &RetryError{fmt.Errorf("failed to delete role %s in namespace %s: %w", name, namespace, err)} + } + return &RetryError{fmt.Errorf("deleted role %s in namespace %s", name, namespace)} + case k8sErrors.IsNotFound(err): + // Already deleted + return nil + default: + return err + } +} + +func generateDefaultRole(namespace string) *rbacv1.Role { + return &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceRoleName(), + Namespace: namespace, + Labels: map[string]string{ + "app.kubernetes.io/name": "devworkspace-workspaces", + "app.kubernetes.io/part-of": "devworkspace-operator", + }, + }, + Rules: []rbacv1.PolicyRule{ + { + Resources: []string{"pods/exec"}, + APIGroups: []string{""}, + Verbs: []string{"create"}, + }, + { + Resources: []string{"pods"}, + APIGroups: []string{""}, + Verbs: []string{"get", "list", "watch"}, + }, + { + Resources: []string{"pods"}, + APIGroups: []string{"metrics.k8s.io"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + Resources: []string{"deployments", "replicasets"}, + APIGroups: []string{"apps", "extensions"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + Resources: []string{"secrets"}, + APIGroups: []string{""}, + Verbs: []string{"get", "create", "patch", "delete"}, + ResourceNames: []string{"workspace-credentials-secret"}, + }, + { + Resources: []string{"configmaps"}, + APIGroups: []string{""}, + Verbs: []string{"get", "create", "patch", "delete"}, + ResourceNames: []string{"workspace-preferences-configmap"}, + }, + { + Resources: []string{"devworkspaces"}, + APIGroups: []string{"workspace.devfile.io"}, + Verbs: []string{"get", "watch", "list", "patch", "update"}, + }, + { + Resources: []string{"devworkspaceroutings"}, + APIGroups: []string{"controller.devfile.io"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + Resources: []string{"devworkspacetemplates"}, + APIGroups: []string{"workspace.devfile.io"}, + Verbs: []string{"get", "create", "patch", "update", "delete", "list", "watch"}, + }, + }, + } +} + +func generateUseRoleForSCC(namespace, sccName string) *rbacv1.Role { + return &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceSCCRoleName(sccName), + Namespace: namespace, + Labels: map[string]string{ + "app.kubernetes.io/name": "devworkspace-workspaces", + "app.kubernetes.io/part-of": "devworkspace-operator", + }, + }, + Rules: []rbacv1.PolicyRule{ + { + Resources: []string{"securitycontextconstraints"}, + APIGroups: []string{"security.openshift.io"}, + Verbs: []string{"use"}, + ResourceNames: []string{sccName}, + }, + }, + } +} diff --git a/pkg/provision/workspace/rbac/rolebinding.go b/pkg/provision/workspace/rbac/rolebinding.go new file mode 100644 index 000000000..88fb1da99 --- /dev/null +++ b/pkg/provision/workspace/rbac/rolebinding.go @@ -0,0 +1,160 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "fmt" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" + + rbacv1 "k8s.io/api/rbac/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func syncRolebindings(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + defaultRoleName := common.WorkspaceRoleName() + defaultRolebindingName := common.WorkspaceRolebindingName() + if err := addServiceAccountToRolebinding(saName, workspace.Namespace, defaultRoleName, defaultRolebindingName, api); err != nil { + return err + } + if !workspace.Spec.Template.Attributes.Exists(constants.WorkspaceSCCAttribute) { + return nil + } + sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) + sccRoleName := common.WorkspaceSCCRoleName(sccName) + sccRolebindingName := common.WorkspaceSCCRolebindingName(sccName) + if err := addServiceAccountToRolebinding(saName, workspace.Namespace, sccRoleName, sccRolebindingName, api); err != nil { + return err + } + return nil +} + +func deleteRolebinding(name, namespace string, api sync.ClusterAPI) error { + rolebinding := &rbacv1.RoleBinding{} + namespacedName := types.NamespacedName{ + Name: name, + Namespace: namespace, + } + err := api.Client.Get(api.Ctx, namespacedName, rolebinding) + switch { + case err == nil: + if err := api.Client.Delete(api.Ctx, rolebinding); err != nil { + return &RetryError{fmt.Errorf("failed to delete rolebinding %s in namespace %s: %w", name, namespace, err)} + } + return &RetryError{fmt.Errorf("deleted rolebinding %s in namespace %s", name, namespace)} + case k8sErrors.IsNotFound(err): + // Already deleted + return nil + default: + return err + } +} + +func addServiceAccountToRolebinding(saName, namespace, roleName, rolebindingName string, api sync.ClusterAPI) error { + rolebinding := &rbacv1.RoleBinding{} + namespacedName := types.NamespacedName{ + Name: rolebindingName, + Namespace: namespace, + } + err := api.Client.Get(api.Ctx, namespacedName, rolebinding) + switch { + case err == nil: + // Got existing rolebinding, need to make sure SA is part of it + break + case k8sErrors.IsNotFound(err): + // Rolebinding not created yet, initiailize default rolebinding and add SA to it + rolebinding = generateDefaultRolebinding(rolebindingName, namespace, roleName) + default: + return err + } + if !rolebindingHasSubject(rolebinding, saName, namespace) { + rolebinding.Subjects = append(rolebinding.Subjects, rbacv1.Subject{ + Kind: rbacv1.ServiceAccountKind, + Name: saName, + Namespace: namespace, + }) + } + + if _, err = sync.SyncObjectWithCluster(rolebinding, api); err != nil { + return wrapSyncError(err) + } + return nil +} + +func removeServiceAccountFromRolebinding(saName, namespace, roleBindingName string, api sync.ClusterAPI) error { + rolebinding := &rbacv1.RoleBinding{} + namespacedName := types.NamespacedName{ + Name: roleBindingName, + Namespace: namespace, + } + err := api.Client.Get(api.Ctx, namespacedName, rolebinding) + switch { + case err == nil: + // Found rolebinding, ensure saName is not a subject + break + case k8sErrors.IsNotFound(err): + // Rolebinding does not exist; nothing to do + return nil + default: + return err + } + if !rolebindingHasSubject(rolebinding, saName, namespace) { + return nil + } + var newSubjects []rbacv1.Subject + for _, subject := range rolebinding.Subjects { + if subject.Kind == rbacv1.ServiceAccountKind && subject.Name == saName && subject.Namespace == namespace { + continue + } + newSubjects = append(newSubjects, subject) + } + rolebinding.Subjects = newSubjects + if _, err := sync.SyncObjectWithCluster(rolebinding, api); err != nil { + return wrapSyncError(err) + } + return nil +} + +func generateDefaultRolebinding(name, namespace, roleName string) *rbacv1.RoleBinding { + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{ + "app.kubernetes.io/name": "devworkspace-workspaces", + "app.kubernetes.io/part-of": "devworkspace-operator", + }, + }, + RoleRef: rbacv1.RoleRef{ + Kind: "Role", + Name: roleName, + }, + // Subjects added for each workspace ServiceAccount + Subjects: []rbacv1.Subject{}, + } +} + +func rolebindingHasSubject(rolebinding *rbacv1.RoleBinding, saName, namespace string) bool { + for _, subject := range rolebinding.Subjects { + if subject.Kind == rbacv1.ServiceAccountKind && subject.Name == saName && subject.Namespace == namespace { + return true + } + } + return false +} From d77a80bf21dd05e6417fc26726bfa762bf9ec3f2 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 18:29:04 -0600 Subject: [PATCH 03/10] Add function to clean up old roles and rolebindings Add functionality to automatically delete old roles and rolebindings when a workspace is processed. This removes the previously-used 'workspace' role and 'devworkspacedw' rolebinding, which have no normal mechanism for their removal. Signed-off-by: Angel Misevski --- pkg/common/naming.go | 8 +++ pkg/provision/workspace/rbac/common.go | 3 ++ pkg/provision/workspace/rbac/migrate.go | 66 +++++++++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 pkg/provision/workspace/rbac/migrate.go diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 7873d8755..a692cbf5d 100644 --- a/pkg/common/naming.go +++ b/pkg/common/naming.go @@ -146,10 +146,18 @@ 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" } diff --git a/pkg/provision/workspace/rbac/common.go b/pkg/provision/workspace/rbac/common.go index 442257db4..44cc594b5 100644 --- a/pkg/provision/workspace/rbac/common.go +++ b/pkg/provision/workspace/rbac/common.go @@ -46,6 +46,9 @@ func wrapSyncError(err error) error { } func SyncRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { + if err := cleanupDeprecatedRBAC(workspace.Namespace, api); err != nil { + return err + } if err := syncRoles(workspace, api); err != nil { return err } diff --git a/pkg/provision/workspace/rbac/migrate.go b/pkg/provision/workspace/rbac/migrate.go new file mode 100644 index 000000000..fd4a909e5 --- /dev/null +++ b/pkg/provision/workspace/rbac/migrate.go @@ -0,0 +1,66 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "fmt" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" + rbacv1 "k8s.io/api/rbac/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" +) + +// cleanupDeprecatedRBAC removes old Roles and RoleBindings created by an earlier version +// of the DevWorkspace Operator. These earlier roles and rolebindings are no longer used +// and need to be removed directly as there is no usual mechanism for their removal. +// TODO: Remove this functionality for DevWorkspace Operator v0.19 +func cleanupDeprecatedRBAC(namespace string, api sync.ClusterAPI) error { + role := &rbacv1.Role{} + roleNN := types.NamespacedName{ + Name: common.OldWorkspaceRoleName(), + Namespace: namespace, + } + err := api.Client.Get(api.Ctx, roleNN, role) + switch { + case err == nil: + if err := api.Client.Delete(api.Ctx, role); err != nil { + return err + } + return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace Role")} + case k8sErrors.IsNotFound(err): + break + default: + return err + } + rolebinding := &rbacv1.RoleBinding{} + rolebindingNN := types.NamespacedName{ + Name: common.OldWorkspaceRolebindingName(), + Namespace: namespace, + } + err = api.Client.Get(api.Ctx, rolebindingNN, rolebinding) + switch { + case err == nil: + if err := api.Client.Delete(api.Ctx, rolebinding); err != nil { + return err + } + return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace RoleBinding")} + case k8sErrors.IsNotFound(err): + break + default: + return err + } + return nil +} From 08a0106e2b298d60d10c108fa99bcff8c79cd055 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 20:28:25 -0600 Subject: [PATCH 04/10] Switch to using updated RBAC syncing process Switch main reconcile loop to use new RBAC sync functions and remove old rbac files. Signed-off-by: Angel Misevski --- .../workspace/devworkspace_controller.go | 24 +++- pkg/constants/finalizers.go | 5 + pkg/provision/workspace/rbac.go | 124 ------------------ 3 files changed, 25 insertions(+), 128 deletions(-) delete mode 100644 pkg/provision/workspace/rbac.go diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index ae42aee5f..887288c10 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/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" @@ -285,7 +286,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 @@ -340,9 +341,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 diff --git a/pkg/constants/finalizers.go b/pkg/constants/finalizers.go index 487cab7f2..79d6533bb 100644 --- a/pkg/constants/finalizers.go +++ b/pkg/constants/finalizers.go @@ -21,4 +21,9 @@ const ( // necessary to clean up additional non-workspace roles added to the workspace // serviceaccount 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" ) diff --git a/pkg/provision/workspace/rbac.go b/pkg/provision/workspace/rbac.go deleted file mode 100644 index 73393719d..000000000 --- a/pkg/provision/workspace/rbac.go +++ /dev/null @@ -1,124 +0,0 @@ -// -// Copyright (c) 2019-2022 Red Hat, Inc. -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -package workspace - -import ( - "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/provision/sync" - - rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// SyncRBAC generates RBAC and synchronizes the runtime objects -func SyncRBAC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) ProvisioningStatus { - rbacs := generateRBAC(workspace.Namespace) - requeue := false - for _, rbac := range rbacs { - _, err := sync.SyncObjectWithCluster(rbac, clusterAPI) - switch t := err.(type) { - case nil: - break - case *sync.NotInSyncError: - requeue = true - case *sync.UnrecoverableSyncError: - return ProvisioningStatus{FailStartup: true, Err: t.Cause} - default: - return ProvisioningStatus{Err: err} - } - } - - return ProvisioningStatus{Continue: !requeue} -} - -func generateRBAC(namespace string) []client.Object { - // TODO: The rolebindings here are created namespace-wide; find a way to limit this, given that each workspace is only - // interested in a specific set of resources (e.g. the workspace shouldn't need to watch other pods, etc.) - return []client.Object{ - &rbacv1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.OldWorkspaceRoleName(), - Namespace: namespace, - }, - Rules: []rbacv1.PolicyRule{ - { - Resources: []string{"pods/exec"}, - APIGroups: []string{""}, - Verbs: []string{"create"}, - }, - { - Resources: []string{"pods"}, - APIGroups: []string{""}, - Verbs: []string{"get", "list", "watch"}, - }, - { - Resources: []string{"pods"}, - APIGroups: []string{"metrics.k8s.io"}, - Verbs: []string{"get", "list", "watch"}, - }, - { - Resources: []string{"deployments", "replicasets"}, - APIGroups: []string{"apps", "extensions"}, - Verbs: []string{"get", "list", "watch"}, - }, - { - Resources: []string{"secrets"}, - APIGroups: []string{""}, - Verbs: []string{"get", "create", "patch", "delete"}, - ResourceNames: []string{"workspace-credentials-secret"}, - }, - { - Resources: []string{"configmaps"}, - APIGroups: []string{""}, - Verbs: []string{"get", "create", "patch", "delete"}, - ResourceNames: []string{"workspace-preferences-configmap"}, - }, - { - Resources: []string{"devworkspaces"}, - APIGroups: []string{"workspace.devfile.io"}, - Verbs: []string{"get", "watch", "list", "patch", "update"}, - }, - { - Resources: []string{"devworkspaceroutings"}, - APIGroups: []string{"controller.devfile.io"}, - Verbs: []string{"get", "list", "watch"}, - }, - { - Resources: []string{"devworkspacetemplates"}, - APIGroups: []string{"workspace.devfile.io"}, - Verbs: []string{"get", "create", "patch", "update", "delete", "list", "watch"}, - }, - }, - }, - &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.OldWorkspaceRolebindingName(), - Namespace: namespace, - }, - RoleRef: rbacv1.RoleRef{ - Kind: "Role", - Name: "workspace", - }, - Subjects: []rbacv1.Subject{ - { - Kind: "Group", - Name: "system:serviceaccounts:" + namespace, - }, - }, - }, - } -} From 776dd456f206ae1145bb269709d1674a7883d333 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 20:42:15 -0600 Subject: [PATCH 05/10] Stop adding workspace SA to SCC and don't use SA finalizer 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 --- .../workspace/devworkspace_controller.go | 6 -- controllers/workspace/finalize.go | 43 ++++++++++++ pkg/constants/finalizers.go | 3 + pkg/provision/workspace/serviceaccount.go | 65 ++----------------- 4 files changed, 52 insertions(+), 65 deletions(-) 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) From 46743882ed0d23a84f5c515e32721f5ee5faf6e5 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 21:31:21 -0600 Subject: [PATCH 06/10] Update DevWorkspace operator clusterrole Update cluster role used by DWO to add verbs 'delete', 'deletecollection' for roles/rolebindings Signed-off-by: Angel Misevski --- controllers/workspace/devworkspace_controller.go | 3 ++- .../devworkspace-operator.clusterserviceversion.yaml | 10 ++++++++++ deploy/deployment/kubernetes/combined.yaml | 10 ++++++++++ .../devworkspace-controller-role.ClusterRole.yaml | 10 ++++++++++ deploy/deployment/openshift/combined.yaml | 10 ++++++++++ .../devworkspace-controller-role.ClusterRole.yaml | 10 ++++++++++ deploy/templates/components/rbac/role.yaml | 10 ++++++++++ 7 files changed, 62 insertions(+), 1 deletion(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 8117be1d1..dbb65c3fa 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -86,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 diff --git a/deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml b/deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml index 91762c767..2392ab7f4 100644 --- a/deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml +++ b/deploy/bundle/manifests/devworkspace-operator.clusterserviceversion.yaml @@ -277,10 +277,20 @@ spec: resources: - clusterrolebindings - clusterroles + verbs: + - create + - get + - list + - update + - watch + - apiGroups: + - rbac.authorization.k8s.io + resources: - rolebindings - roles verbs: - create + - delete - get - list - update diff --git a/deploy/deployment/kubernetes/combined.yaml b/deploy/deployment/kubernetes/combined.yaml index 1ec1e17ff..f0173e812 100644 --- a/deploy/deployment/kubernetes/combined.yaml +++ b/deploy/deployment/kubernetes/combined.yaml @@ -23534,10 +23534,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 diff --git a/deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml b/deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml index a292a209e..67111535b 100644 --- a/deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml +++ b/deploy/deployment/kubernetes/objects/devworkspace-controller-role.ClusterRole.yaml @@ -201,10 +201,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 diff --git a/deploy/deployment/openshift/combined.yaml b/deploy/deployment/openshift/combined.yaml index ac1a43f4c..d2f669b59 100644 --- a/deploy/deployment/openshift/combined.yaml +++ b/deploy/deployment/openshift/combined.yaml @@ -23534,10 +23534,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 diff --git a/deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml b/deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml index a292a209e..67111535b 100644 --- a/deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml +++ b/deploy/deployment/openshift/objects/devworkspace-controller-role.ClusterRole.yaml @@ -201,10 +201,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 diff --git a/deploy/templates/components/rbac/role.yaml b/deploy/templates/components/rbac/role.yaml index c9b8621a0..ed33239a4 100644 --- a/deploy/templates/components/rbac/role.yaml +++ b/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 From 930b7bc6d7f0d0caf6cf6dea9a49a24df111f1bc Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 18 Oct 2022 22:22:38 -0600 Subject: [PATCH 07/10] Update cache filter for roles and rolebindings, update migrate process Update the cache filter for roles and rolebindings to check for appropriate label. As a result, the old workspace role/rolebinding are invisible to the controller, so the process for cleaning up old resources needs to be updated as well. Signed-off-by: Angel Misevski --- pkg/cache/cache.go | 10 +-- pkg/provision/workspace/rbac/common.go | 6 ++ pkg/provision/workspace/rbac/migrate.go | 72 +++++++++++++++------ pkg/provision/workspace/rbac/role.go | 10 +-- pkg/provision/workspace/rbac/rolebinding.go | 5 +- 5 files changed, 69 insertions(+), 34 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 3caacab50..4af5236fc 100644 --- a/pkg/cache/cache.go +++ b/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.OldWorkspaceRoleName()}), + Label: rbacObjectSelector, }, &rbacv1.RoleBinding{}: { - Field: fields.SelectorFromSet(fields.Set{"metadata.name": common.OldWorkspaceRolebindingName()}), + Label: rbacObjectSelector, }, } diff --git a/pkg/provision/workspace/rbac/common.go b/pkg/provision/workspace/rbac/common.go index 44cc594b5..58fa9925e 100644 --- a/pkg/provision/workspace/rbac/common.go +++ b/pkg/provision/workspace/rbac/common.go @@ -18,6 +18,12 @@ import ( "github.com/devfile/devworkspace-operator/pkg/provision/sync" ) +var rbacLabels = map[string]string{ + "app.kubernetes.io/name": "devworkspace-workspaces", + "app.kubernetes.io/part-of": "devworkspace-operator", + "controller.devfile.io/workspace-rbac": "true", +} + type RetryError struct { Err error } diff --git a/pkg/provision/workspace/rbac/migrate.go b/pkg/provision/workspace/rbac/migrate.go index fd4a909e5..85128bab8 100644 --- a/pkg/provision/workspace/rbac/migrate.go +++ b/pkg/provision/workspace/rbac/migrate.go @@ -20,47 +20,83 @@ import ( "github.com/devfile/devworkspace-operator/pkg/provision/sync" rbacv1 "k8s.io/api/rbac/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) // cleanupDeprecatedRBAC removes old Roles and RoleBindings created by an earlier version // of the DevWorkspace Operator. These earlier roles and rolebindings are no longer used // and need to be removed directly as there is no usual mechanism for their removal. +// +// Since the cache filters used for the operator are label-based and the old roles/bindings +// do not have the appropriate labels, the old role/binding are "invisible" to the controller +// This means we have to delete the object without reading it first. To avoid submitting many +// delete requests to the API, we only do this if the new role/binding are not present. // TODO: Remove this functionality for DevWorkspace Operator v0.19 func cleanupDeprecatedRBAC(namespace string, api sync.ClusterAPI) error { - role := &rbacv1.Role{} - roleNN := types.NamespacedName{ - Name: common.OldWorkspaceRoleName(), + newRole := &rbacv1.Role{} + newRoleNN := types.NamespacedName{ + Name: common.WorkspaceRoleName(), Namespace: namespace, } - err := api.Client.Get(api.Ctx, roleNN, role) + oldRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.OldWorkspaceRoleName(), + Namespace: namespace, + }, + } + err := api.Client.Get(api.Ctx, newRoleNN, newRole) switch { case err == nil: - if err := api.Client.Delete(api.Ctx, role); err != nil { - return err - } - return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace Role")} - case k8sErrors.IsNotFound(err): + // New role exists, don't try to delete old role break + case k8sErrors.IsNotFound(err): + // Try to delete old role + deleteErr := api.Client.Delete(api.Ctx, oldRole) + switch { + case deleteErr == nil: + return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace Role")} + case k8sErrors.IsNotFound(err): + // Already deleted + break + default: + return deleteErr + } default: return err } - rolebinding := &rbacv1.RoleBinding{} - rolebindingNN := types.NamespacedName{ - Name: common.OldWorkspaceRolebindingName(), + + newRolebinding := &rbacv1.RoleBinding{} + newRolebindingNN := types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), Namespace: namespace, } - err = api.Client.Get(api.Ctx, rolebindingNN, rolebinding) + oldRolebinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.OldWorkspaceRolebindingName(), + Namespace: namespace, + }, + } + err = api.Client.Get(api.Ctx, newRolebindingNN, newRolebinding) switch { case err == nil: - if err := api.Client.Delete(api.Ctx, rolebinding); err != nil { - return err - } - return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace RoleBinding")} - case k8sErrors.IsNotFound(err): + // New role exists, don't try to delete old role break + case k8sErrors.IsNotFound(err): + // Try to delete old role + deleteErr := api.Client.Delete(api.Ctx, oldRolebinding) + switch { + case deleteErr == nil: + return &RetryError{fmt.Errorf("deleted deprecated DevWorkspace RoleBinding")} + case k8sErrors.IsNotFound(err): + // Already deleted + break + default: + return deleteErr + } default: return err } + return nil } diff --git a/pkg/provision/workspace/rbac/role.go b/pkg/provision/workspace/rbac/role.go index 12d946e5b..033e4bf4d 100644 --- a/pkg/provision/workspace/rbac/role.go +++ b/pkg/provision/workspace/rbac/role.go @@ -68,10 +68,7 @@ func generateDefaultRole(namespace string) *rbacv1.Role { ObjectMeta: metav1.ObjectMeta{ Name: common.WorkspaceRoleName(), Namespace: namespace, - Labels: map[string]string{ - "app.kubernetes.io/name": "devworkspace-workspaces", - "app.kubernetes.io/part-of": "devworkspace-operator", - }, + Labels: rbacLabels, }, Rules: []rbacv1.PolicyRule{ { @@ -130,10 +127,7 @@ func generateUseRoleForSCC(namespace, sccName string) *rbacv1.Role { ObjectMeta: metav1.ObjectMeta{ Name: common.WorkspaceSCCRoleName(sccName), Namespace: namespace, - Labels: map[string]string{ - "app.kubernetes.io/name": "devworkspace-workspaces", - "app.kubernetes.io/part-of": "devworkspace-operator", - }, + Labels: rbacLabels, }, Rules: []rbacv1.PolicyRule{ { diff --git a/pkg/provision/workspace/rbac/rolebinding.go b/pkg/provision/workspace/rbac/rolebinding.go index 88fb1da99..c3f0258c8 100644 --- a/pkg/provision/workspace/rbac/rolebinding.go +++ b/pkg/provision/workspace/rbac/rolebinding.go @@ -136,10 +136,7 @@ func generateDefaultRolebinding(name, namespace, roleName string) *rbacv1.RoleBi ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, - Labels: map[string]string{ - "app.kubernetes.io/name": "devworkspace-workspaces", - "app.kubernetes.io/part-of": "devworkspace-operator", - }, + Labels: rbacLabels, }, RoleRef: rbacv1.RoleRef{ Kind: "Role", From 6ba10cdea217051104a9615cd65e5eebde9999c1 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Wed, 19 Oct 2022 11:27:05 -0600 Subject: [PATCH 08/10] Add tests for role/rolebinding provisioning Signed-off-by: Angel Misevski --- pkg/provision/workspace/rbac/common_test.go | 222 +++++++++++++++++ pkg/provision/workspace/rbac/finalize_test.go | 228 ++++++++++++++++++ pkg/provision/workspace/rbac/migrate_test.go | 130 ++++++++++ pkg/provision/workspace/rbac/role_test.go | 113 +++++++++ .../workspace/rbac/rolebinding_test.go | 154 ++++++++++++ 5 files changed, 847 insertions(+) create mode 100644 pkg/provision/workspace/rbac/common_test.go create mode 100644 pkg/provision/workspace/rbac/finalize_test.go create mode 100644 pkg/provision/workspace/rbac/migrate_test.go create mode 100644 pkg/provision/workspace/rbac/role_test.go create mode 100644 pkg/provision/workspace/rbac/rolebinding_test.go diff --git a/pkg/provision/workspace/rbac/common_test.go b/pkg/provision/workspace/rbac/common_test.go new file mode 100644 index 000000000..3b3fb7242 --- /dev/null +++ b/pkg/provision/workspace/rbac/common_test.go @@ -0,0 +1,222 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "context" + "fmt" + "testing" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/api/v2/pkg/attributes" + "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" + testlog "github.com/go-logr/logr/testing" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +const ( + testNamespace = "test-namespace" + testSCCName = "test-scc" +) + +var ( + scheme = runtime.NewScheme() +) + +func init() { + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(v1alpha1.AddToScheme(scheme)) + utilruntime.Must(dw.AddToScheme(scheme)) +} + +var ( + oldRole = &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.OldWorkspaceRoleName(), + Namespace: testNamespace, + }, + Rules: []rbacv1.PolicyRule{}, + } + oldRolebinding = &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.OldWorkspaceRolebindingName(), + Namespace: testNamespace, + }, + } + newRole = &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceRoleName(), + Namespace: testNamespace, + }, + Rules: []rbacv1.PolicyRule{}, + } + newRolebinding = &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, + RoleRef: rbacv1.RoleRef{ + Kind: "Role", + Name: common.WorkspaceRoleName(), + }, + } + newSCCRole = &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceSCCRoleName(testSCCName), + Namespace: testNamespace, + }, + Rules: []rbacv1.PolicyRule{}, + } + newSCCRolebinding = &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, + RoleRef: rbacv1.RoleRef{ + Kind: "Role", + Name: common.WorkspaceSCCRoleName(testSCCName), + }, + } +) + +func TestSyncRBAC(t *testing.T) { + testdw1 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) + testdw1SAName := common.ServiceAccountName(testdw1.Status.DevWorkspaceId) + testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + api := getTestClusterAPI(t, testdw1.DevWorkspace, testdw2.DevWorkspace, oldRole, oldRolebinding) + // Keep calling SyncRBAC until error returned is nil, to account for multiple steps + iterCount := 0 + maxIters := 30 + retryErr := &RetryError{} + for err := SyncRBAC(testdw1, api); err != nil; err = SyncRBAC(testdw1, api) { + iterCount += 1 + if err == nil { + break + } + if !assert.ErrorAs(t, err, &retryErr, "Unexpected error from SyncRBAC: %s", err) { + return + } + if !assert.LessOrEqual(t, iterCount, maxIters, fmt.Sprintf("SyncRBAC did not sync everything within %d iterations", maxIters)) { + return + } + } + for err := SyncRBAC(testdw2, api); err != nil; err = SyncRBAC(testdw2, api) { + iterCount += 1 + if err == nil { + break + } + if !assert.ErrorAs(t, err, &retryErr, "Unexpected error from SyncRBAC: %s", err) { + return + } + if !assert.LessOrEqual(t, iterCount, maxIters, fmt.Sprintf("SyncRBAC did not sync everything within %d iterations", maxIters)) { + return + } + } + actualRole := &rbacv1.Role{} + err := api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRoleName(), + Namespace: testNamespace, + }, actualRole) + assert.NoError(t, err, "Role should be created") + + actualSCCRole := &rbacv1.Role{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRoleName(testSCCName), + Namespace: testNamespace, + }, actualSCCRole) + assert.NoError(t, err, "SCC Role should be created") + + actualRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, actualRolebinding) + assert.NoError(t, err, "Role should be created") + assert.True(t, testHasSubject(testdw1SAName, testNamespace, actualRolebinding), "Should have testdw1 SA as subject") + assert.True(t, testHasSubject(testdw2SAName, testNamespace, actualRolebinding), "Should have testdw2 SA as subject") + + actualSCCRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, actualSCCRolebinding) + assert.NoError(t, err, "SCC Rolebindind should be created") + assert.True(t, testHasSubject(testdw1SAName, testNamespace, actualSCCRolebinding), "Should have testdw1 SA as subject") + assert.True(t, testHasSubject(testdw2SAName, testNamespace, actualSCCRolebinding), "Should have testdw2 SA as subject") +} + +func getTestClusterAPI(t *testing.T, initialObjects ...client.Object) sync.ClusterAPI { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initialObjects...).Build() + return sync.ClusterAPI{ + Ctx: context.Background(), + Client: fakeClient, + Scheme: scheme, + Logger: testlog.TestLogger{T: t}, + } +} + +func getTestDevWorkspace(id string) *common.DevWorkspaceWithConfig { + return &common.DevWorkspaceWithConfig{ + DevWorkspace: &dw.DevWorkspace{ + ObjectMeta: metav1.ObjectMeta{ + Name: id, + Namespace: testNamespace, + }, + + Status: dw.DevWorkspaceStatus{ + DevWorkspaceId: id, + }, + }, + } +} + +func getTestDevWorkspaceWithAttributes(t *testing.T, id string, keysAndValues ...string) *common.DevWorkspaceWithConfig { + attr := attributes.Attributes{} + if len(keysAndValues)%2 != 0 { + t.Fatalf("Invalid keysAndValues for getTestDevWorkspaceWithAttributes") + } + for i := 0; i < len(keysAndValues); i += 2 { + attr.PutString(keysAndValues[i], keysAndValues[i+1]) + } + return &common.DevWorkspaceWithConfig{ + DevWorkspace: &dw.DevWorkspace{ + ObjectMeta: metav1.ObjectMeta{ + Name: id, + Namespace: testNamespace, + }, + Spec: dw.DevWorkspaceSpec{ + Template: dw.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: dw.DevWorkspaceTemplateSpecContent{ + Attributes: attr, + }, + }, + }, + Status: dw.DevWorkspaceStatus{ + DevWorkspaceId: id, + }, + }, + } +} diff --git a/pkg/provision/workspace/rbac/finalize_test.go b/pkg/provision/workspace/rbac/finalize_test.go new file mode 100644 index 000000000..546250325 --- /dev/null +++ b/pkg/provision/workspace/rbac/finalize_test.go @@ -0,0 +1,228 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "fmt" + "testing" + "time" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func TestDeletesRoleAndRolebindingWhenLastWorkspaceIsDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, newRole, newRolebinding) + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate role deleted") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + assert.Regexp(t, fmt.Sprintf("deleted role .* in namespace %s", testNamespace), err.Error()) + } + err = FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate 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 role and rolebinding deleted") +} + +func TestDeletesRoleAndRolebindingWhenAllWorkspacesAreDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + testdw2 := getTestDevWorkspace("test-devworkspace2") + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + testdw2.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, newRole, newRolebinding) + + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate role deleted") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + assert.Regexp(t, fmt.Sprintf("deleted role .* in namespace %s", testNamespace), err.Error()) + } + err = FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate 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 role and rolebinding deleted") +} + +func TestShouldRemoveWorkspaceSAFromRolebindingWhenDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + testdw2 := getTestDevWorkspace("test-devworkspace2") + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + testdwSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + testrb := newRolebinding.DeepCopy() + testrb.Subjects = append(testrb.Subjects, + 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, testrb, newRole) + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate rolebinding updated") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + } + err = FinalizeRBAC(testdw, api) + assert.NoError(t, err, "Should not return error once rolebinding is in sync") + + actualRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, actualRolebinding) + assert.NoError(t, err, "Unexpected test error getting rolebinding") + assert.False(t, testHasSubject(testdwSAName, testNamespace, actualRolebinding), "Should remove delete workspace SA from rolebinding subjects") + assert.True(t, testHasSubject(testdw2SAName, testNamespace, actualRolebinding), "Should leave workspace SA in rolebinding subjects") +} + +func TestFinalizeDoesNothingWhenRolebindingDoesNotExist(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + testdw2 := getTestDevWorkspace("test-devworkspace2") + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, newRole) + err := FinalizeRBAC(testdw, api) + assert.NoError(t, err, "Should not return error once rolebinding is in sync") + + actualRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, actualRolebinding) + if assert.Error(t, err, "Expect error when getting non-existent rolebinding") { + assert.True(t, k8sErrors.IsNotFound(err), "Error should have IsNotFound type") + } +} + +func TestDeletesSCCRoleAndRolebindingWhenLastWorkspaceIsDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspace("test-devworkspace2") + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, newSCCRole, newSCCRolebinding) + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate role deleted") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + assert.Regexp(t, fmt.Sprintf("deleted role .* in namespace %s", testNamespace), err.Error()) + } + err = FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate 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 role and rolebinding deleted") +} + +func TestDeletesSCCRoleAndRolebindingWhenAllWorkspacesAreDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + testdw2.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, newSCCRole, newSCCRolebinding) + + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate role deleted") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + assert.Regexp(t, fmt.Sprintf("deleted role .* in namespace %s", testNamespace), err.Error()) + } + err = FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate 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 role and rolebinding deleted") +} + +func TestShouldRemoveWorkspaceSAFromSCCRolebindingWhenDeleted(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + testdwSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + testrb := newSCCRolebinding.DeepCopy() + testrb.Subjects = append(testrb.Subjects, + 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, testrb, newSCCRole) + retryErr := &RetryError{} + err := FinalizeRBAC(testdw, api) + if assert.Error(t, err, "Should return error to indicate rolebinding updated") { + assert.ErrorAs(t, err, &retryErr, "Error should be RetryError") + } + err = FinalizeRBAC(testdw, api) + assert.NoError(t, err, "Should not return error once rolebinding is in sync") + + actualRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, actualRolebinding) + assert.NoError(t, err, "Unexpected test error getting rolebinding") + assert.False(t, testHasSubject(testdwSAName, testNamespace, actualRolebinding), "Should remove delete workspace SA from rolebinding subjects") + assert.True(t, testHasSubject(testdw2SAName, testNamespace, actualRolebinding), "Should leave workspace SA in rolebinding subjects") +} + +func TestFinalizeDoesNothingWhenSCCRolebindingDoesNotExist(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) + testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} + api := getTestClusterAPI(t, testdw.DevWorkspace, testdw2.DevWorkspace, newSCCRole) + err := FinalizeRBAC(testdw, api) + assert.NoError(t, err, "Should not return error once rolebinding is in sync") + + actualRolebinding := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, actualRolebinding) + if assert.Error(t, err, "Expect error when getting non-existent rolebinding") { + assert.True(t, k8sErrors.IsNotFound(err), "Error should have IsNotFound type") + } +} diff --git a/pkg/provision/workspace/rbac/migrate_test.go b/pkg/provision/workspace/rbac/migrate_test.go new file mode 100644 index 000000000..de4e724ba --- /dev/null +++ b/pkg/provision/workspace/rbac/migrate_test.go @@ -0,0 +1,130 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "testing" + + "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" +) + +func TestRemovesOldRBACWhenNewRBACNotPresent(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + api := getTestClusterAPI(t, oldRole, oldRolebinding) + // Expect three calls to be required: 1. delete role, 2. delete rolebinding, 3. return nil + err := cleanupDeprecatedRBAC(testNamespace, api) + retryErr := &RetryError{} + if assert.ErrorAs(t, err, &retryErr, "Error should be of type RetryErr") { + assert.Contains(t, err.Error(), "deleted deprecated DevWorkspace Role") + } + err = cleanupDeprecatedRBAC(testNamespace, api) + if assert.ErrorAs(t, err, &retryErr, "Error should be of type RetryErr") { + assert.Contains(t, err.Error(), "deleted deprecated DevWorkspace RoleBinding") + } + err = cleanupDeprecatedRBAC(testNamespace, api) + assert.NoError(t, err, "Should not return error if old rbac does not exist") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRole.Name, + Namespace: testNamespace, + }, &rbacv1.Role{}) + if assert.Error(t, err, "Expect get old role to return IsNotFound error") { + assert.True(t, k8sErrors.IsNotFound(err), "Expect error to have IsNotFound type") + } + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRolebinding.Name, + Namespace: testNamespace, + }, &rbacv1.RoleBinding{}) + if assert.Error(t, err, "Expect get old role to return IsNotFound error") { + assert.True(t, k8sErrors.IsNotFound(err), "Expect error to have IsNotFound type") + } +} + +func TestDoesNotRemoveOldRBACWhenNewRBACPresent(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + api := getTestClusterAPI(t, oldRole, oldRolebinding, newRole, newRolebinding) + err := cleanupDeprecatedRBAC(testNamespace, api) + assert.NoError(t, err, "Should do nothing if new RBAC role/rolebinding are present") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRole.Name, + Namespace: testNamespace, + }, &rbacv1.Role{}) + assert.NoError(t, err, "Old role should still exist if present initially") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRolebinding.Name, + Namespace: testNamespace, + }, &rbacv1.RoleBinding{}) + assert.NoError(t, err, "Old rolebinding should still exist if present initially") +} + +func TestRemovesOldRolebindingWhenNewRolebindingNotPresent(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + api := getTestClusterAPI(t, oldRole, oldRolebinding, newRole) + // Expect two calls to be required: 1. delete rolebinding, 2. return nil + err := cleanupDeprecatedRBAC(testNamespace, api) + retryErr := &RetryError{} + if assert.ErrorAs(t, err, &retryErr, "Error should be of type RetryErr") { + assert.Contains(t, err.Error(), "deleted deprecated DevWorkspace RoleBinding") + } + err = cleanupDeprecatedRBAC(testNamespace, api) + assert.NoError(t, err, "Should not return error if old rbac does not exist") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRole.Name, + Namespace: testNamespace, + }, &rbacv1.Role{}) + assert.NoError(t, err, "Old role should still exist if present initially") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRolebinding.Name, + Namespace: testNamespace, + }, &rbacv1.RoleBinding{}) + if assert.Error(t, err, "Expect get old role to return IsNotFound error") { + assert.True(t, k8sErrors.IsNotFound(err), "Expect error to have IsNotFound type") + } +} + +func TestRemovesOldRoleWhenNewRoleNotPresent(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + api := getTestClusterAPI(t, oldRole, oldRolebinding, newRolebinding) + // Expect two calls to be required: 1. delete role, 2. return nil + err := cleanupDeprecatedRBAC(testNamespace, api) + retryErr := &RetryError{} + if assert.ErrorAs(t, err, &retryErr, "Error should be of type RetryErr") { + assert.Contains(t, err.Error(), "deleted deprecated DevWorkspace Role") + } + err = cleanupDeprecatedRBAC(testNamespace, api) + assert.NoError(t, err, "Should not return error if old rbac does not exist") + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRole.Name, + Namespace: testNamespace, + }, &rbacv1.Role{}) + if assert.Error(t, err, "Expect get old role to return IsNotFound error") { + assert.True(t, k8sErrors.IsNotFound(err), "Expect error to have IsNotFound type") + } + + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: oldRolebinding.Name, + Namespace: testNamespace, + }, &rbacv1.RoleBinding{}) + assert.NoError(t, err, "Old rolebinding should still exist if present initially") +} diff --git a/pkg/provision/workspace/rbac/role_test.go b/pkg/provision/workspace/rbac/role_test.go new file mode 100644 index 000000000..389e98f1a --- /dev/null +++ b/pkg/provision/workspace/rbac/role_test.go @@ -0,0 +1,113 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "testing" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/types" +) + +func TestCreatesRoleIfNotExists(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + api := getTestClusterAPI(t, testdw.DevWorkspace) + err := syncRoles(testdw, api) + retryErr := &RetryError{} + if assert.Error(t, err, "Should return RetryError to indicate that role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if role is in sync") + actualRole := &rbacv1.Role{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRoleName(), + Namespace: testNamespace, + }, actualRole) + assert.NoError(t, err, "Role should be created") +} + +func TestDoesNothingIfRoleAlreadyInSync(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + api := getTestClusterAPI(t, testdw.DevWorkspace) + err := syncRoles(testdw, api) + retryErr := &RetryError{} + if assert.Error(t, err, "Should return RetryError to indicate that role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if role is in sync") + actualRole := &rbacv1.Role{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRoleName(), + Namespace: testNamespace, + }, actualRole) + assert.NoError(t, err, "Role should be created") + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if role is in sync") +} + +func TestCreatesSCCRoleIfNotExists(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + api := getTestClusterAPI(t, testdw.DevWorkspace) + retryErr := &RetryError{} + err := syncRoles(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that SCC role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if roles are in sync") + actualRole := &rbacv1.Role{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRoleName(testSCCName), + Namespace: testNamespace, + }, actualRole) + assert.NoError(t, err, "Role should be created") +} + +func TestDoesNothingIfSCCRoleAlreadyInSync(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + api := getTestClusterAPI(t, testdw.DevWorkspace) + retryErr := &RetryError{} + err := syncRoles(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that SCC role was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if roles are in sync") + actualRole := &rbacv1.Role{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRoleName(testSCCName), + Namespace: testNamespace, + }, actualRole) + assert.NoError(t, err, "Role should be created") + err = syncRoles(testdw, api) + assert.NoError(t, err, "Should not return error if role is in sync") +} diff --git a/pkg/provision/workspace/rbac/rolebinding_test.go b/pkg/provision/workspace/rbac/rolebinding_test.go new file mode 100644 index 000000000..4f3144252 --- /dev/null +++ b/pkg/provision/workspace/rbac/rolebinding_test.go @@ -0,0 +1,154 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "testing" + + "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/types" +) + +func TestCreatesRolebindingIfNotExists(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + api := getTestClusterAPI(t, testdw.DevWorkspace) + err := syncRolebindings(testdw, api) + retryErr := &RetryError{} + if assert.Error(t, err, "Should return RetryError to indicate that rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + assert.NoError(t, err, "Should not return error if rolebinding is in sync") + actualRB := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, actualRB) + assert.NoError(t, err, "Rolebinding should be created") + assert.Equal(t, common.WorkspaceRoleName(), actualRB.RoleRef.Name, "Rolebinding shold reference default role") + expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have workspace SA as subject") +} + +func TestAddsMultipleSubjectsToRolebinding(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.Kubernetes) + testdw := getTestDevWorkspace("test-devworkspace") + testdw2 := getTestDevWorkspace("test-devworkspace-2") + api := getTestClusterAPI(t, testdw.DevWorkspace) + err := syncRolebindings(testdw, api) + retryErr := &RetryError{} + if assert.Error(t, err, "Should return RetryError to indicate that rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + assert.NoError(t, err, "Should not return error if rolebinding is in sync") + err = syncRolebindings(testdw2, api) + if assert.Error(t, err, "Should return RetryError to indicate that rolebinding was updated") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw2, api) + assert.NoError(t, err, "Should not return error if rolebinding is in sync") + + actualRB := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceRolebindingName(), + Namespace: testNamespace, + }, actualRB) + assert.NoError(t, err, "Rolebinding should be created") + assert.Equal(t, common.WorkspaceRoleName(), actualRB.RoleRef.Name, "Rolebinding shold reference default role") + expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have both workspace SAs as subjects") + expectedSAName2 := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName2, testNamespace, actualRB), "Created rolebinding should have both workspace SAs as subjects") +} + +func TestCreatesSCCRolebindingIfNotExists(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + api := getTestClusterAPI(t, testdw.DevWorkspace) + retryErr := &RetryError{} + err := syncRolebindings(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that default rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that SCC rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + assert.NoError(t, err, "Should not return error if rolebindings are in sync") + actualRB := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, actualRB) + assert.NoError(t, err, "Rolebinding should be created") + assert.Equal(t, common.WorkspaceSCCRoleName(testSCCName), actualRB.RoleRef.Name, "Rolebinding shold reference default role") + expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have workspace SA as subject") +} + +func TestAddsMultipleSubjectsToSCCRolebinding(t *testing.T) { + infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) + testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) + testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace-2", constants.WorkspaceSCCAttribute, testSCCName) + api := getTestClusterAPI(t, testdw.DevWorkspace) + retryErr := &RetryError{} + err := syncRolebindings(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that default rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + if assert.Error(t, err, "Should return RetryError to indicate that SCC rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw, api) + assert.NoError(t, err, "Should not return error if rolebindings are in sync") + err = syncRolebindings(testdw2, api) + if assert.Error(t, err, "Should return RetryError to indicate that default rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw2, api) + if assert.Error(t, err, "Should return RetryError to indicate that SCC rolebinding was created") { + assert.ErrorAs(t, err, &retryErr, "Error should have RetryError type") + } + err = syncRolebindings(testdw2, api) + assert.NoError(t, err, "Should not return error if rolebindings are in sync") + + actualRB := &rbacv1.RoleBinding{} + err = api.Client.Get(api.Ctx, types.NamespacedName{ + Name: common.WorkspaceSCCRolebindingName(testSCCName), + Namespace: testNamespace, + }, actualRB) + assert.NoError(t, err, "Rolebinding should be created") + assert.Equal(t, common.WorkspaceSCCRoleName(testSCCName), actualRB.RoleRef.Name, "Rolebinding shold reference default role") + expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created SCC rolebinding should have both workspace SAs as subjects") + expectedSAName2 := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + assert.True(t, testHasSubject(expectedSAName2, testNamespace, actualRB), "Created SCC rolebinding should have both workspace SAs as subjects") +} + +func testHasSubject(subjName, namespace string, rolebinding *rbacv1.RoleBinding) bool { + for _, subject := range rolebinding.Subjects { + if subject.Name == subjName && subject.Namespace == namespace && subject.Kind == rbacv1.ServiceAccountKind { + return true + } + } + return false +} From 8c9c62443d0036c65061adecc52050b6f30337a7 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 1 Nov 2022 15:07:52 -0400 Subject: [PATCH 09/10] Fix RBAC provisioning after rebase (changes to SA name package) Signed-off-by: Angel Misevski --- pkg/provision/workspace/rbac/common_test.go | 4 ++-- pkg/provision/workspace/rbac/finalize.go | 4 ++-- pkg/provision/workspace/rbac/finalize_test.go | 8 ++++---- pkg/provision/workspace/rbac/rolebinding.go | 2 +- pkg/provision/workspace/rbac/rolebinding_test.go | 12 ++++++------ 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/provision/workspace/rbac/common_test.go b/pkg/provision/workspace/rbac/common_test.go index 3b3fb7242..4a163aed0 100644 --- a/pkg/provision/workspace/rbac/common_test.go +++ b/pkg/provision/workspace/rbac/common_test.go @@ -104,8 +104,8 @@ var ( func TestSyncRBAC(t *testing.T) { testdw1 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) - testdw1SAName := common.ServiceAccountName(testdw1.Status.DevWorkspaceId) - testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + testdw1SAName := common.ServiceAccountName(testdw1) + testdw2SAName := common.ServiceAccountName(testdw2) api := getTestClusterAPI(t, testdw1.DevWorkspace, testdw2.DevWorkspace, oldRole, oldRolebinding) // Keep calling SyncRBAC until error returned is nil, to account for multiple steps iterCount := 0 diff --git a/pkg/provision/workspace/rbac/finalize.go b/pkg/provision/workspace/rbac/finalize.go index da71f28bf..1e94d92d2 100644 --- a/pkg/provision/workspace/rbac/finalize.go +++ b/pkg/provision/workspace/rbac/finalize.go @@ -28,7 +28,7 @@ func FinalizeRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) return err } } - saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + saName := common.ServiceAccountName(workspace) roleName := common.WorkspaceRoleName() rolebindingName := common.WorkspaceRolebindingName() numWorkspaces, err := countNonDeletedWorkspaces(workspace.Namespace, api) @@ -52,7 +52,7 @@ func FinalizeRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) func finalizeSCCRBAC(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { sccName := workspace.Spec.Template.Attributes.GetString(constants.WorkspaceSCCAttribute, nil) - saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + saName := common.ServiceAccountName(workspace) roleName := common.WorkspaceSCCRoleName(sccName) rolebindingName := common.WorkspaceSCCRolebindingName(sccName) numWorkspaces, err := countNonDeletedWorkspacesUsingSCC(sccName, workspace.Namespace, api) diff --git a/pkg/provision/workspace/rbac/finalize_test.go b/pkg/provision/workspace/rbac/finalize_test.go index 546250325..446791dd5 100644 --- a/pkg/provision/workspace/rbac/finalize_test.go +++ b/pkg/provision/workspace/rbac/finalize_test.go @@ -76,8 +76,8 @@ func TestShouldRemoveWorkspaceSAFromRolebindingWhenDeleted(t *testing.T) { testdw := getTestDevWorkspace("test-devworkspace") testdw2 := getTestDevWorkspace("test-devworkspace2") testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} - testdwSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) - testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + testdwSAName := common.ServiceAccountName(testdw) + testdw2SAName := common.ServiceAccountName(testdw2) testrb := newRolebinding.DeepCopy() testrb.Subjects = append(testrb.Subjects, rbacv1.Subject{ @@ -176,8 +176,8 @@ func TestShouldRemoveWorkspaceSAFromSCCRolebindingWhenDeleted(t *testing.T) { testdw := getTestDevWorkspaceWithAttributes(t, "test-devworkspace", constants.WorkspaceSCCAttribute, testSCCName) testdw2 := getTestDevWorkspaceWithAttributes(t, "test-devworkspace2", constants.WorkspaceSCCAttribute, testSCCName) testdw.DeletionTimestamp = &metav1.Time{Time: time.Now()} - testdwSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) - testdw2SAName := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + testdwSAName := common.ServiceAccountName(testdw) + testdw2SAName := common.ServiceAccountName(testdw2) testrb := newSCCRolebinding.DeepCopy() testrb.Subjects = append(testrb.Subjects, rbacv1.Subject{ diff --git a/pkg/provision/workspace/rbac/rolebinding.go b/pkg/provision/workspace/rbac/rolebinding.go index c3f0258c8..f4e0ab0e8 100644 --- a/pkg/provision/workspace/rbac/rolebinding.go +++ b/pkg/provision/workspace/rbac/rolebinding.go @@ -27,7 +27,7 @@ import ( ) func syncRolebindings(workspace *common.DevWorkspaceWithConfig, api sync.ClusterAPI) error { - saName := common.ServiceAccountName(workspace.Status.DevWorkspaceId) + saName := common.ServiceAccountName(workspace) defaultRoleName := common.WorkspaceRoleName() defaultRolebindingName := common.WorkspaceRolebindingName() if err := addServiceAccountToRolebinding(saName, workspace.Namespace, defaultRoleName, defaultRolebindingName, api); err != nil { diff --git a/pkg/provision/workspace/rbac/rolebinding_test.go b/pkg/provision/workspace/rbac/rolebinding_test.go index 4f3144252..0a19c9a91 100644 --- a/pkg/provision/workspace/rbac/rolebinding_test.go +++ b/pkg/provision/workspace/rbac/rolebinding_test.go @@ -42,7 +42,7 @@ func TestCreatesRolebindingIfNotExists(t *testing.T) { }, actualRB) assert.NoError(t, err, "Rolebinding should be created") assert.Equal(t, common.WorkspaceRoleName(), actualRB.RoleRef.Name, "Rolebinding shold reference default role") - expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + expectedSAName := common.ServiceAccountName(testdw) assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have workspace SA as subject") } @@ -72,9 +72,9 @@ func TestAddsMultipleSubjectsToRolebinding(t *testing.T) { }, actualRB) assert.NoError(t, err, "Rolebinding should be created") assert.Equal(t, common.WorkspaceRoleName(), actualRB.RoleRef.Name, "Rolebinding shold reference default role") - expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + expectedSAName := common.ServiceAccountName(testdw) assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have both workspace SAs as subjects") - expectedSAName2 := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + expectedSAName2 := common.ServiceAccountName(testdw2) assert.True(t, testHasSubject(expectedSAName2, testNamespace, actualRB), "Created rolebinding should have both workspace SAs as subjects") } @@ -100,7 +100,7 @@ func TestCreatesSCCRolebindingIfNotExists(t *testing.T) { }, actualRB) assert.NoError(t, err, "Rolebinding should be created") assert.Equal(t, common.WorkspaceSCCRoleName(testSCCName), actualRB.RoleRef.Name, "Rolebinding shold reference default role") - expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + expectedSAName := common.ServiceAccountName(testdw) assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created rolebinding should have workspace SA as subject") } @@ -138,9 +138,9 @@ func TestAddsMultipleSubjectsToSCCRolebinding(t *testing.T) { }, actualRB) assert.NoError(t, err, "Rolebinding should be created") assert.Equal(t, common.WorkspaceSCCRoleName(testSCCName), actualRB.RoleRef.Name, "Rolebinding shold reference default role") - expectedSAName := common.ServiceAccountName(testdw.Status.DevWorkspaceId) + expectedSAName := common.ServiceAccountName(testdw) assert.True(t, testHasSubject(expectedSAName, testNamespace, actualRB), "Created SCC rolebinding should have both workspace SAs as subjects") - expectedSAName2 := common.ServiceAccountName(testdw2.Status.DevWorkspaceId) + expectedSAName2 := common.ServiceAccountName(testdw2) assert.True(t, testHasSubject(expectedSAName2, testNamespace, actualRB), "Created SCC rolebinding should have both workspace SAs as subjects") } From da3ce11cf85368e36524d96cbfd40e0ae3a4c4d2 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 1 Nov 2022 16:44:27 -0400 Subject: [PATCH 10/10] Fix RBAC tests to accomodate serviceaccount changes. Signed-off-by: Angel Misevski --- pkg/provision/workspace/rbac/common_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/provision/workspace/rbac/common_test.go b/pkg/provision/workspace/rbac/common_test.go index 4a163aed0..affa543fa 100644 --- a/pkg/provision/workspace/rbac/common_test.go +++ b/pkg/provision/workspace/rbac/common_test.go @@ -22,6 +22,7 @@ import ( "github.com/devfile/api/v2/pkg/attributes" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" + "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" testlog "github.com/go-logr/logr/testing" @@ -190,6 +191,7 @@ func getTestDevWorkspace(id string) *common.DevWorkspaceWithConfig { DevWorkspaceId: id, }, }, + Config: config.GetConfigForTesting(nil), } } @@ -202,6 +204,7 @@ func getTestDevWorkspaceWithAttributes(t *testing.T, id string, keysAndValues .. attr.PutString(keysAndValues[i], keysAndValues[i+1]) } return &common.DevWorkspaceWithConfig{ + Config: config.GetConfigForTesting(nil), DevWorkspace: &dw.DevWorkspace{ ObjectMeta: metav1.ObjectMeta{ Name: id,