refactor rbacfinalizercontroller to fix cluster ns terminating (#40)

Signed-off-by: Zhiwei Yin <[email protected]>
Co-authored-by: Zhiwei Yin <[email protected]>

pkg/helpers/testing/testinghelpers.go

@@ -233,23 +233,25 @@ func NewManifestWork(namespace, name string, finalizers []string, deletionTimest
 	return work
 }
 
-func NewRole(namespace, name string, finalizers []string, terminated bool) *rbacv1.Role {
+func NewRole(namespace, name string, finalizers []string, labels map[string]string, terminated bool) *rbacv1.Role {
 	role := &rbacv1.Role{}
 	role.Namespace = namespace
 	role.Name = name
 	role.Finalizers = finalizers
+	role.Labels = labels
 	if terminated {
 		now := metav1.Now()
 		role.DeletionTimestamp = &now
 	}
 	return role
 }
 
-func NewRoleBinding(namespace, name string, finalizers []string, terminated bool) *rbacv1.RoleBinding {
+func NewRoleBinding(namespace, name string, finalizers []string, labels map[string]string, terminated bool) *rbacv1.RoleBinding {
 	rolebinding := &rbacv1.RoleBinding{}
 	rolebinding.Namespace = namespace
 	rolebinding.Name = name
 	rolebinding.Finalizers = finalizers
+	rolebinding.Labels = labels
 	if terminated {
 		now := metav1.Now()
 		rolebinding.DeletionTimestamp = &now

pkg/hub/managedcluster/controller.go

@@ -5,6 +5,7 @@ import (
 	"embed"
 	"encoding/json"
 	"fmt"
+	corev1 "k8s.io/api/core/v1"
 
 	clientset "open-cluster-management.io/api/client/cluster/clientset/versioned"
 	informerv1 "open-cluster-management.io/api/client/cluster/informers/externalversions/cluster/v1"
@@ -142,8 +143,21 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
 
 	// TODO consider to add the managedcluster-namespace.yaml back to staticFiles,
 	// currently, we keep the namespace after the managed cluster is deleted.
-	applyFiles := []string{"manifests/managedcluster-namespace.yaml"}
-	applyFiles = append(applyFiles, staticFiles...)
+	// apply namespace at first
+	namespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: managedClusterName,
+			Labels: map[string]string{
+				"open-cluster-management.io/cluster-name": managedClusterName,
+			},
+		},
+	}
+
+	errs := []error{}
+	_, _, err = resourceapply.ApplyNamespace(ctx, c.kubeClient.CoreV1(), syncCtx.Recorder(), namespace)
+	if err != nil {
+		errs = append(errs, err)
+	}
 
 	// Hub cluster-admin accepts the spoke cluster, we apply
 	// 1. clusterrole and clusterrolebinding for this spoke cluster.
@@ -155,9 +169,9 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
 		syncCtx.Recorder(),
 		c.cache,
 		helpers.ManagedClusterAssetFn(manifestFiles, managedClusterName),
-		applyFiles...,
+		staticFiles...,
 	)
-	errs := []error{}
+
 	for _, result := range resourceResults {
 		if result.Error != nil {
 			errs = append(errs, fmt.Errorf("%q (%T): %v", result.File, result.Type, result.Error))

pkg/hub/managedcluster/manifests/managedcluster-registration-rolebinding.yaml

@@ -3,6 +3,8 @@ kind: RoleBinding
 metadata:
   name: open-cluster-management:managedcluster:{{ .ManagedClusterName }}:registration
   namespace: "{{ .ManagedClusterName }}"
+  labels:
+    open-cluster-management.io/cluster-name: {{ .ManagedClusterName }}
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole

pkg/hub/managedcluster/manifests/managedcluster-work-rolebinding.yaml

@@ -3,6 +3,8 @@ kind: RoleBinding
 metadata:
   name: open-cluster-management:managedcluster:{{ .ManagedClusterName }}:work
   namespace: "{{ .ManagedClusterName }}"
+  labels:
+    open-cluster-management.io/cluster-name: {{ .ManagedClusterName }}
   finalizers:
   - cluster.open-cluster-management.io/manifest-work-cleanup
 roleRef:

pkg/hub/manager.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	certv1 "k8s.io/api/certificates/v1"
 	certv1beta1 "k8s.io/api/certificates/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"time"
 
 	ocmfeature "open-cluster-management.io/api/feature"
@@ -154,11 +155,22 @@ func (m *HubManagerOptions) RunControllerManager(ctx context.Context, controller
 		controllerContext.EventRecorder,
 	)
 
+	rbacFinalizerKubeInfomers := kubeinformers.NewSharedInformerFactoryWithOptions(kubeClient, 10*time.Minute, kubeinformers.WithTweakListOptions(
+		func(listOptions *metav1.ListOptions) {
+			selector := &metav1.LabelSelector{
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{
+						Key:      "open-cluster-management.io/cluster-name",
+						Operator: metav1.LabelSelectorOpExists,
+					},
+				},
+			}
+			listOptions.LabelSelector = metav1.FormatLabelSelector(selector)
+		}))
 	rbacFinalizerController := rbacfinalizerdeletion.NewFinalizeController(
-		kubeInfomers.Rbac().V1().Roles(),
-		kubeInfomers.Rbac().V1().RoleBindings(),
-		kubeInfomers.Core().V1().Namespaces().Lister(),
-		clusterInformers.Cluster().V1().ManagedClusters().Lister(),
+		rbacFinalizerKubeInfomers.Rbac().V1().RoleBindings().Lister(),
+		rbacFinalizerKubeInfomers.Core().V1().Namespaces(),
+		clusterInformers.Cluster().V1().ManagedClusters(),
 		workInformers.Work().V1().ManifestWorks().Lister(),
 		kubeClient.RbacV1(),
 		controllerContext.EventRecorder,
@@ -217,6 +229,7 @@ func (m *HubManagerOptions) RunControllerManager(ctx context.Context, controller
 	go workInformers.Start(ctx.Done())
 	go kubeInfomers.Start(ctx.Done())
 	go addOnInformers.Start(ctx.Done())
+	go rbacFinalizerKubeInfomers.Start(ctx.Done())
 
 	go managedClusterController.Run(ctx, 1)
 	go taintController.Run(ctx, 1)

pkg/hub/rbacfinalizerdeletion/controller.go

@@ -2,37 +2,36 @@ package rbacfinalizerdeletion
 
 import (
 	"context"
-	"fmt"
+	"k8s.io/apimachinery/pkg/selection"
+	corev1informers "k8s.io/client-go/informers/core/v1"
+	"k8s.io/klog/v2"
+	informerv1 "open-cluster-management.io/api/client/cluster/informers/externalversions/cluster/v1"
 	"reflect"
-
-	clusterv1listers "open-cluster-management.io/api/client/cluster/listers/cluster/v1"
-	worklister "open-cluster-management.io/api/client/work/listers/work/v1"
-	clusterv1 "open-cluster-management.io/api/cluster/v1"
+	"time"
 
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
+	clusterv1listers "open-cluster-management.io/api/client/cluster/listers/cluster/v1"
+	worklister "open-cluster-management.io/api/client/work/listers/work/v1"
 
-	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
-	rbacv1informers "k8s.io/client-go/informers/rbac/v1"
 	rbacv1client "k8s.io/client-go/kubernetes/typed/rbac/v1"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	rbacv1listers "k8s.io/client-go/listers/rbac/v1"
 	"k8s.io/client-go/tools/cache"
-	"k8s.io/klog/v2"
 )
 
 const (
 	manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
+	clusterLabelKey       = "open-cluster-management.io/cluster-name"
 )
 
 type finalizeController struct {
-	roleLister         rbacv1listers.RoleLister
 	roleBindingLister  rbacv1listers.RoleBindingLister
 	rbacClient         rbacv1client.RbacV1Interface
 	clusterLister      clusterv1listers.ManagedClusterLister
@@ -44,20 +43,18 @@ type finalizeController struct {
 // NewFinalizeController ensures all manifestworks are deleted before role/rolebinding for work
 // agent are deleted in a terminating cluster namespace.
 func NewFinalizeController(
-	roleInformer rbacv1informers.RoleInformer,
-	roleBindingInformer rbacv1informers.RoleBindingInformer,
-	namespaceLister corelisters.NamespaceLister,
-	clusterLister clusterv1listers.ManagedClusterLister,
+	roleBindingLister rbacv1listers.RoleBindingLister,
+	namespaceInformer corev1informers.NamespaceInformer,
+	clusterInformer informerv1.ManagedClusterInformer,
 	manifestWorkLister worklister.ManifestWorkLister,
 	rbacClient rbacv1client.RbacV1Interface,
 	eventRecorder events.Recorder,
 ) factory.Controller {
 
 	controller := &finalizeController{
-		roleLister:         roleInformer.Lister(),
-		roleBindingLister:  roleBindingInformer.Lister(),
-		namespaceLister:    namespaceLister,
-		clusterLister:      clusterLister,
+		roleBindingLister:  roleBindingLister,
+		namespaceLister:    namespaceInformer.Lister(),
+		clusterLister:      clusterInformer.Lister(),
 		manifestWorkLister: manifestWorkLister,
 		rbacClient:         rbacClient,
 		eventRecorder:      eventRecorder,
@@ -67,105 +64,77 @@ func NewFinalizeController(
 		WithInformersQueueKeyFunc(func(obj runtime.Object) string {
 			key, _ := cache.MetaNamespaceKeyFunc(obj)
 			return key
-		}, roleInformer.Informer(), roleBindingInformer.Informer()).
+		}, clusterInformer.Informer(), namespaceInformer.Informer()).
 		WithSync(controller.sync).ToController("FinalizeController", eventRecorder)
 }
 
 func (m *finalizeController) sync(ctx context.Context, controllerContext factory.SyncContext) error {
 	key := controllerContext.QueueKey()
-	namespace, name, err := cache.SplitMetaNamespaceKey(key)
-	if err != nil {
-		// ignore role/rolebinding whose key is not in format: namespace/name
+	if key == "" {
 		return nil
 	}
 
-	cluster, err := m.clusterLister.Get(namespace)
-	if err != nil && !errors.IsNotFound(err) {
-		return err
-	}
-	ns, err := m.namespaceLister.Get(namespace)
+	_, clusterName, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
-		return err
+		return nil
 	}
 
-	// cluster is deleted or being deleted
-	role, rolebinding, err := m.getRoleAndRoleBinding(namespace, name)
-	if err != nil {
+	cluster, err := m.clusterLister.Get(clusterName)
+	if err != nil && !errors.IsNotFound(err) {
 		return err
 	}
 
-	err = m.syncRoleAndRoleBinding(ctx, controllerContext, role, rolebinding, ns, cluster)
-
-	if err != nil {
-		klog.Errorf("Reconcile role/rolebinding %s fails with err: %v", key, err)
-	}
-	return err
-}
-
-func (m *finalizeController) syncRoleAndRoleBinding(ctx context.Context, controllerContext factory.SyncContext,
-	role *rbacv1.Role, rolebinding *rbacv1.RoleBinding, ns *corev1.Namespace, cluster *clusterv1.ManagedCluster) error {
-	// Skip if neither role nor rolebinding has the finalizer
-	if !hasFinalizer(role, manifestWorkFinalizer) && !hasFinalizer(rolebinding, manifestWorkFinalizer) {
+	ns, err := m.namespaceLister.Get(clusterName)
+	if errors.IsNotFound(err) {
 		return nil
 	}
+	if err != nil {
+		return err
+	}
 
 	// There are two possible cases that we need to remove finalizers on role/rolebindings based on
 	// clean of manifestworks.
 	// 1. The namespace is finalizing.
-	// 2. The cluster is finalizing but namespace fails to be deleted.
-	if !ns.DeletionTimestamp.IsZero() || (cluster != nil && !cluster.DeletionTimestamp.IsZero()) {
+	// 2. The cluster is finalizing or not found.
+	if !ns.DeletionTimestamp.IsZero() || cluster == nil ||
+		(cluster != nil && !cluster.DeletionTimestamp.IsZero()) {
 		works, err := m.manifestWorkLister.ManifestWorks(ns.Name).List(labels.Everything())
 		if err != nil {
 			return err
 		}
 
 		if len(works) != 0 {
-			return fmt.Errorf("still having %d works in the cluster namespace %s", len(works), ns.Name)
-		}
-	}
-
-	// remove finalizer from role/rolebinding
-	if pendingFinalization(role) {
-		if err := m.removeFinalizerFromRole(ctx, role, manifestWorkFinalizer); err != nil {
-			return err
-		}
-	}
-
-	if pendingFinalization(rolebinding) {
-		if err := m.removeFinalizerFromRoleBinding(ctx, rolebinding, manifestWorkFinalizer); err != nil {
-			return err
+			controllerContext.Queue().AddAfter(clusterName, 10*time.Second)
+			klog.Warningf("still having %d works in the cluster namespace %s", len(works), ns.Name)
+			return nil
 		}
+		return m.syncRoleBindings(ctx, controllerContext, clusterName)
 	}
 	return nil
 }
 
-func (m *finalizeController) getRoleAndRoleBinding(namespace, name string) (*rbacv1.Role, *rbacv1.RoleBinding, error) {
-	role, err := m.roleLister.Roles(namespace).Get(name)
-	if err != nil && !errors.IsNotFound(err) {
-		return nil, nil, err
-	}
-
-	rolebinding, err := m.roleBindingLister.RoleBindings(namespace).Get(name)
+func (m *finalizeController) syncRoleBindings(ctx context.Context, controllerContext factory.SyncContext,
+	namespace string) error {
+	requirement, _ := labels.NewRequirement(clusterLabelKey, selection.Exists, []string{})
+	selector := labels.NewSelector().Add(*requirement)
+	roleBindings, err := m.roleBindingLister.RoleBindings(namespace).List(selector)
 	if err != nil && !errors.IsNotFound(err) {
-		return nil, nil, err
-	}
-
-	return role, rolebinding, nil
-}
-
-// removeFinalizerFromRole removes the particular finalizer from role
-func (m *finalizeController) removeFinalizerFromRole(ctx context.Context, role *rbacv1.Role, finalizer string) error {
-	if role == nil {
-		return nil
+		return err
 	}
 
-	role = role.DeepCopy()
-	if changed := removeFinalizer(role, finalizer); !changed {
-		return nil
+	for _, roleBinding := range roleBindings {
+		// Skip if roleBinding has no the finalizer
+		if !hasFinalizer(roleBinding, manifestWorkFinalizer) {
+			continue
+		}
+		// remove finalizer from roleBinding
+		if pendingFinalization(roleBinding) {
+			if err := m.removeFinalizerFromRoleBinding(ctx, roleBinding, manifestWorkFinalizer); err != nil {
+				return err
+			}
+		}
 	}
-
-	_, err := m.rbacClient.Roles(role.Namespace).Update(ctx, role, metav1.UpdateOptions{})
-	return err
+	return nil
 }
 
 // removeFinalizerFromRoleBinding removes the particular finalizer from rolebinding