Merge pull request #2113 from hasheddan/keep-those-clusterroles

🐛 Only delete ClusterRoles and ClusterRoleBindings when Workspace finalizer is removed

Author: openshift-merge-robot

pkg/apis/tenancy/v1alpha1/helper/helper.go

@@ -24,8 +24,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/kcp-dev/kcp/pkg/apis/tenancy/v1alpha1"
+	"github.com/kcp-dev/kcp/pkg/apis/tenancy/v1beta1"
 )
 
+// IsValidCluster indicates whether a cluster is valid based on whether it
+// adheres to logical cluster naming requirements and is rooted at root or
+// system.
 func IsValidCluster(cluster logicalcluster.Name) bool {
 	if !cluster.IsValid() {
 		return false
@@ -34,9 +38,18 @@ func IsValidCluster(cluster logicalcluster.Name) bool {
 	return cluster.HasPrefix(v1alpha1.RootCluster) || cluster.HasPrefix(logicalcluster.New("system"))
 }
 
+// QualifiedObjectName builds a fully qualified identifier for an object
+// consisting of its logical cluster, namespace if applicable, and object
+// metadata name.
 func QualifiedObjectName(obj metav1.Object) string {
 	if len(obj.GetNamespace()) > 0 {
 		return fmt.Sprintf("%s|%s/%s", logicalcluster.From(obj), obj.GetNamespace(), obj.GetName())
 	}
 	return fmt.Sprintf("%s|%s", logicalcluster.From(obj), obj.GetName())
 }
+
+// WorkspaceLabelSelector builds a label selector for objects associated with a
+// given workspace.
+func WorkspaceLabelSelector(name string) string {
+	return fmt.Sprintf("%s=%s", v1beta1.WorkspaceNameLabel, name)
+}

pkg/apis/tenancy/v1alpha1/helper/helper_test.go

@@ -20,6 +20,8 @@ import (
 	"testing"
 
 	"github.com/kcp-dev/logicalcluster/v2"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestIsValidCluster(t *testing.T) {
@@ -60,7 +62,51 @@ func TestIsValidCluster(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.workspace, func(t *testing.T) {
 			if got := IsValidCluster(logicalcluster.New(tt.workspace)); got != tt.valid {
-				t.Errorf("isValid(%q) = %v, want %v", tt.workspace, got, tt.valid)
+				t.Errorf("IsValidCluster(%q) = %v, want %v", tt.workspace, got, tt.valid)
+			}
+		})
+	}
+}
+
+func TestQualifiedObjectName(t *testing.T) {
+	tests := []struct {
+		obj  metav1.Object
+		name string
+	}{
+		{&metav1.ObjectMeta{
+			Name: "cool-name",
+			Annotations: map[string]string{
+				logicalcluster.AnnotationKey: "cool-cluster",
+			},
+		}, "cool-cluster|cool-name"},
+		{&metav1.ObjectMeta{
+			Name:      "cool-name",
+			Namespace: "cool-namespace",
+			Annotations: map[string]string{
+				logicalcluster.AnnotationKey: "cool-cluster",
+			},
+		}, "cool-cluster|cool-namespace/cool-name"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.obj.GetName(), func(t *testing.T) {
+			if got := QualifiedObjectName(tt.obj); got != tt.name {
+				t.Errorf("QualifiedObjectName(%v) = %s, want %s", tt.obj, got, tt.name)
+			}
+		})
+	}
+}
+
+func TestWorkspaceLabelSelector(t *testing.T) {
+	tests := []struct {
+		ws       string
+		selector string
+	}{
+		{"cool-ws", "workspaces.kcp.dev/name=cool-ws"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.ws, func(t *testing.T) {
+			if got := WorkspaceLabelSelector(tt.ws); got != tt.selector {
+				t.Errorf("WorkspaceLabelSelector(%s) = %s, want %s", tt.ws, got, tt.selector)
 			}
 		})
 	}

pkg/apis/tenancy/v1beta1/types.go

@@ -23,6 +23,10 @@ import (
 	conditionsv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/apis/conditions/v1alpha1"
 )
 
+// WorkspaceNameLabel is a label indicating the workspace in which the given
+// object resides.
+const WorkspaceNameLabel = "workspaces.kcp.dev/name"
+
 // Workspace defines a generic Kubernetes-cluster-like endpoint, with standard Kubernetes
 // discovery APIs, OpenAPI and resource API endpoints.
 //

pkg/reconciler/tenancy/clusterworkspacedeletion/clusterworkspace_deletion_controller.go

@@ -33,12 +33,14 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
+	kubernetesclient "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/metadata"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 
 	tenancyv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/tenancy/v1alpha1"
+	"github.com/kcp-dev/kcp/pkg/apis/tenancy/v1alpha1/helper"
 	kcpclient "github.com/kcp-dev/kcp/pkg/client/clientset/versioned"
 	tenancyinformers "github.com/kcp-dev/kcp/pkg/client/informers/externalversions/tenancy/v1alpha1"
 	tenancylisters "github.com/kcp-dev/kcp/pkg/client/listers/tenancy/v1alpha1"
@@ -50,7 +52,13 @@ const (
 	controllerName = "kcp-clusterworkspacedeletion"
 )
 
+var (
+	background        = metav1.DeletePropagationBackground
+	backgroudDeletion = metav1.DeleteOptions{PropagationPolicy: &background}
+)
+
 func NewController(
+	kubeClusterClient kubernetesclient.ClusterInterface,
 	kcpClusterClient kcpclient.Interface,
 	metadataClusterClient metadata.Interface,
 	workspaceInformer tenancyinformers.ClusterWorkspaceInformer,
@@ -60,6 +68,7 @@ func NewController(
 
 	c := &Controller{
 		queue:                 queue,
+		kubeClusterClient:     kubeClusterClient,
 		kcpClusterClient:      kcpClusterClient,
 		metadataClusterClient: metadataClusterClient,
 		workspaceLister:       workspaceInformer.Lister(),
@@ -87,6 +96,7 @@ func NewController(
 type Controller struct {
 	queue workqueue.RateLimitingInterface
 
+	kubeClusterClient     kubernetesclient.ClusterInterface
 	kcpClusterClient      kcpclient.Interface
 	metadataClusterClient metadata.Interface
 
@@ -248,9 +258,23 @@ func (c *Controller) finalizeWorkspace(ctx context.Context, workspace *tenancyv1
 		if workspace.Finalizers[i] == deletion.WorkspaceFinalizer {
 			workspace.Finalizers = append(workspace.Finalizers[:i], workspace.Finalizers[i+1:]...)
 
+			clusterName := logicalcluster.From(workspace)
+			listOpts := metav1.ListOptions{
+				LabelSelector: helper.WorkspaceLabelSelector(workspace.Name),
+			}
+
+			// TODO(hasheddan): ClusterRole and ClusterRoleBinding cleanup
+			// should be handled by garbage collection when the controller is
+			// implemented.
+			if err := c.kubeClusterClient.Cluster(clusterName).RbacV1().ClusterRoles().DeleteCollection(ctx, backgroudDeletion, listOpts); err != nil && !apierrors.IsNotFound(err) {
+				return fmt.Errorf("could not delete clusterroles for workspace %s: %w", clusterName, err)
+			}
+			if err := c.kubeClusterClient.Cluster(clusterName).RbacV1().ClusterRoleBindings().DeleteCollection(ctx, backgroudDeletion, listOpts); err != nil && !apierrors.IsNotFound(err) {
+				return fmt.Errorf("could not delete clusterrolebindings for workspace %s: %w", clusterName, err)
+			}
 			logger.V(2).Info("removing finalizer from ClusterWorkspace")
 			_, err := c.kcpClusterClient.TenancyV1alpha1().ClusterWorkspaces().Update(
-				logicalcluster.WithCluster(ctx, logicalcluster.From(workspace)), workspace, metav1.UpdateOptions{})
+				logicalcluster.WithCluster(ctx, clusterName), workspace, metav1.UpdateOptions{})
 			return err
 		}
 	}

pkg/reconciler/tenancy/clusterworkspacedeletion/deletion/workspace_resource_deletor.go

@@ -169,7 +169,7 @@ func (d *workspacedResourcesDeleter) deleteCollection(ctx context.Context, clust
 	opts := metav1.DeleteOptions{PropagationPolicy: &background}
 	if err := d.metadataClusterClient.Resource(gvr).DeleteCollection(
 		logicalcluster.WithCluster(ctx, clusterName), opts, metav1.ListOptions{}); err != nil {
-		logger.V(5).Error(err, "unexpected deleteColection error")
+		logger.V(5).Error(err, "unexpected deleteCollection error")
 		return true, err
 	}
 

pkg/server/controllers.go

@@ -312,8 +312,12 @@ func (s *Server) installWorkspaceDeletionController(ctx context.Context, config
 		}
 		return discoveryClient.ServerPreferredResources()
 	}
-
+	kubeClusterClient, err := kubernetesclient.NewClusterForConfig(config)
+	if err != nil {
+		return err
+	}
 	workspaceDeletionController := clusterworkspacedeletion.NewController(
+		kubeClusterClient,
 		kcpClusterClient,
 		metadataClusterClient,
 		s.KcpSharedInformerFactory.Tenancy().V1alpha1().ClusterWorkspaces(),

pkg/virtual/workspaces/registry/rest.go

@@ -53,10 +53,6 @@ import (
 	workspaceutil "github.com/kcp-dev/kcp/pkg/virtual/workspaces/util"
 )
 
-const (
-	WorkspaceNameLabel string = "workspaces.kcp.dev/name"
-)
-
 // FilteredClusterWorkspaces allows to list and watch ClusterWorkspaces
 // filtered by authorizaation, i.e. a user only sees those object he has access to.
 type FilteredClusterWorkspaces interface {
@@ -354,7 +350,7 @@ func (s *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		ObjectMeta: metav1.ObjectMeta{
 			Name: ownerRoleBindingName,
 			Labels: map[string]string{
-				WorkspaceNameLabel: workspace.Name,
+				tenancyv1beta1.WorkspaceNameLabel: workspace.Name,
 			},
 		},
 		RoleRef: rbacv1.RoleRef{
@@ -388,7 +384,7 @@ func (s *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		ObjectMeta: metav1.ObjectMeta{
 			Name: ownerRoleBindingName,
 			Labels: map[string]string{
-				WorkspaceNameLabel: workspace.Name,
+				tenancyv1beta1.WorkspaceNameLabel: workspace.Name,
 			},
 		},
 		Rules: rules,
@@ -413,26 +409,12 @@ func (s *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va
 	logger := klog.FromContext(ctx).WithValues("parent", orgClusterName, "name", name)
 	ctx = klog.NewContext(ctx, logger)
 
-	errorToReturn := s.kcpClusterClient.Cluster(orgClusterName).TenancyV1alpha1().ClusterWorkspaces().Delete(ctx, name, *options)
-	if errorToReturn != nil && !kerrors.IsNotFound(errorToReturn) {
-		return nil, false, errorToReturn
-	}
-	if kerrors.IsNotFound(errorToReturn) {
-		errorToReturn = kerrors.NewNotFound(tenancyv1beta1.Resource("workspaces"), name)
-	}
-	workspaceNameLabelSelector := fmt.Sprintf("%s=%s", WorkspaceNameLabel, name)
-	if err := s.kubeClusterClient.Cluster(orgClusterName).RbacV1().ClusterRoles().DeleteCollection(ctx, *options, metav1.ListOptions{
-		LabelSelector: workspaceNameLabelSelector,
-	}); err != nil {
-		logger.Error(err, "could not delete clusterroles")
-	}
-	if err := s.kubeClusterClient.Cluster(orgClusterName).RbacV1().ClusterRoleBindings().DeleteCollection(ctx, *options, metav1.ListOptions{
-		LabelSelector: workspaceNameLabelSelector,
-	}); err != nil {
-		logger.Error(err, "could not delete clusterrolebindings")
+	err := s.kcpClusterClient.Cluster(orgClusterName).TenancyV1alpha1().ClusterWorkspaces().Delete(ctx, name, *options)
+	if kerrors.IsNotFound(err) {
+		err = kerrors.NewNotFound(tenancyv1beta1.Resource("workspaces"), name)
 	}
 
-	return nil, false, errorToReturn
+	return nil, false, err
 }
 
 type withProjection struct {