From 99a06756c3b3756dfcd67cbbcbee574bee002d3b Mon Sep 17 00:00:00 2001 From: Zhiwei Yin Date: Tue, 4 Jul 2023 17:51:50 +0800 Subject: [PATCH] refactor rbacfinalizercontroller to fix cluster ns is terminating after delete clustermanager Signed-off-by: Zhiwei Yin --- .gitignore | 1 + .../helpers/testing/testinghelpers.go | 6 +- pkg/registration/hub/manager.go | 5 +- .../hub/rbacfinalizerdeletion/controller.go | 116 ++++++-------- .../rbacfinalizerdeletion/controller_test.go | 148 ++++++------------ 5 files changed, 97 insertions(+), 179 deletions(-) diff --git a/.gitignore b/.gitignore index df4ba9c2b..eea4ddc89 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,7 @@ munge-csv # Output of the go coverage tool, specifically when used with LiteIDE *.out _output/ +.idea/ .kubeconfig .hub-kubeconfig diff --git a/pkg/registration/helpers/testing/testinghelpers.go b/pkg/registration/helpers/testing/testinghelpers.go index 5b53f2bb2..1b1eda1a0 100644 --- a/pkg/registration/helpers/testing/testinghelpers.go +++ b/pkg/registration/helpers/testing/testinghelpers.go @@ -204,11 +204,12 @@ 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 @@ -216,11 +217,12 @@ func NewRole(namespace, name string, finalizers []string, terminated bool) *rbac 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 diff --git a/pkg/registration/hub/manager.go b/pkg/registration/hub/manager.go index 13a014dea..6eee11cba 100644 --- a/pkg/registration/hub/manager.go +++ b/pkg/registration/hub/manager.go @@ -177,10 +177,9 @@ func (m *HubManagerOptions) RunControllerManager(ctx context.Context, controller ) rbacFinalizerController := rbacfinalizerdeletion.NewFinalizeController( - kubeInfomers.Rbac().V1().Roles(), kubeInfomers.Rbac().V1().RoleBindings(), - kubeInfomers.Core().V1().Namespaces().Lister(), - clusterInformers.Cluster().V1().ManagedClusters().Lister(), + kubeInfomers.Core().V1().Namespaces(), + clusterInformers.Cluster().V1().ManagedClusters(), workInformers.Work().V1().ManifestWorks().Lister(), kubeClient.RbacV1(), controllerContext.EventRecorder, diff --git a/pkg/registration/hub/rbacfinalizerdeletion/controller.go b/pkg/registration/hub/rbacfinalizerdeletion/controller.go index c8ee49f52..55c970a18 100644 --- a/pkg/registration/hub/rbacfinalizerdeletion/controller.go +++ b/pkg/registration/hub/rbacfinalizerdeletion/controller.go @@ -2,18 +2,19 @@ package rbacfinalizerdeletion import ( "context" - "fmt" "reflect" + "time" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" - 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" + "k8s.io/apimachinery/pkg/selection" + corev1informers "k8s.io/client-go/informers/core/v1" 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" @@ -21,6 +22,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" + informerv1 "open-cluster-management.io/api/client/cluster/informers/externalversions/cluster/v1" 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" @@ -45,125 +47,95 @@ 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, + 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, + namespaceLister: namespaceInformer.Lister(), + clusterLister: clusterInformer.Lister(), manifestWorkLister: manifestWorkLister, rbacClient: rbacClient, eventRecorder: eventRecorder, } return factory.New(). - WithInformersQueueKeysFunc(queue.QueueKeyByMetaNamespaceName, roleInformer.Informer(), roleBindingInformer.Informer()). + WithInformersQueueKeysFunc(queue.QueueKeyByMetaNamespaceName, 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(clusterv1.ClusterNameLabelKey, 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 diff --git a/pkg/registration/hub/rbacfinalizerdeletion/controller_test.go b/pkg/registration/hub/rbacfinalizerdeletion/controller_test.go index 1ca74b3e6..33949cf9e 100644 --- a/pkg/registration/hub/rbacfinalizerdeletion/controller_test.go +++ b/pkg/registration/hub/rbacfinalizerdeletion/controller_test.go @@ -7,7 +7,6 @@ import ( "time" "github.com/openshift/library-go/pkg/operator/events" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -20,7 +19,6 @@ import ( fakeworkclient "open-cluster-management.io/api/client/work/clientset/versioned/fake" workinformers "open-cluster-management.io/api/client/work/informers/externalversions" clusterv1 "open-cluster-management.io/api/cluster/v1" - workapiv1 "open-cluster-management.io/api/work/v1" testingcommon "open-cluster-management.io/ocm/pkg/common/testing" testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing" @@ -34,29 +32,37 @@ func TestSync(t *testing.T) { key string clusters []runtime.Object namespaces []runtime.Object - roles []runtime.Object roleBindings []runtime.Object works []runtime.Object expectedErr string }{ { name: "managed cluster namespace is not found", - key: fmt.Sprintf("%s/%s", testinghelpers.TestManagedClusterName, roleName), - expectedErr: "namespace \"testmanagedcluster\" not found", + key: testinghelpers.TestManagedClusterName, + expectedErr: "", }, + { - name: "there are no resources in managed cluster namespace", - key: fmt.Sprintf("%s/%s", testinghelpers.TestManagedClusterName, roleName), - namespaces: []runtime.Object{testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, true)}, + name: "there are no resources in managed cluster namespace", + key: testinghelpers.TestManagedClusterName, + namespaces: []runtime.Object{testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, true)}, + expectedErr: "", }, { - name: "still have works in deleting managed cluster namespace", - key: fmt.Sprintf("%s/%s", testinghelpers.TestManagedClusterName, roleName), - namespaces: []runtime.Object{testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, true)}, - roles: []runtime.Object{testinghelpers.NewRole(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true)}, - roleBindings: []runtime.Object{testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true)}, - works: []runtime.Object{testinghelpers.NewManifestWork(testinghelpers.TestManagedClusterName, "work1", []string{manifestWorkFinalizer}, nil)}, - expectedErr: "still having 1 works in the cluster namespace testmanagedcluster", + name: "cluster and ns are not deleting", + key: testinghelpers.TestManagedClusterName, + namespaces: []runtime.Object{testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, false)}, + clusters: []runtime.Object{testinghelpers.NewManagedCluster()}, + expectedErr: "", + }, + { + name: "still have works in deleting managed cluster namespace", + key: testinghelpers.TestManagedClusterName, + namespaces: []runtime.Object{testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, true)}, + roleBindings: []runtime.Object{testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, + []string{manifestWorkFinalizer}, map[string]string{clusterv1.ClusterNameLabelKey: testinghelpers.TestManagedClusterName}, true)}, + works: []runtime.Object{testinghelpers.NewManifestWork(testinghelpers.TestManagedClusterName, "work1", []string{manifestWorkFinalizer}, nil)}, + expectedErr: "", }, } for _, c := range cases { @@ -69,12 +75,7 @@ func TestSync(t *testing.T) { t.Fatal(err) } } - roleStore := kubeInformerFactory.Rbac().V1().Roles().Informer().GetStore() - for _, role := range c.roles { - if err := roleStore.Add(role); err != nil { - t.Fatal(err) - } - } + roleBindingStore := kubeInformerFactory.Rbac().V1().RoleBindings().Informer().GetStore() for _, roleBinding := range c.roleBindings { if err := roleBindingStore.Add(roleBinding); err != nil { @@ -95,7 +96,6 @@ func TestSync(t *testing.T) { } ctrl := &finalizeController{ - roleLister: kubeInformerFactory.Rbac().V1().Roles().Lister(), roleBindingLister: kubeInformerFactory.Rbac().V1().RoleBindings().Lister(), namespaceLister: kubeInformerFactory.Core().V1().Namespaces().Lister(), clusterLister: clusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), @@ -112,64 +112,38 @@ func TestSync(t *testing.T) { func TestSyncRoleAndRoleBinding(t *testing.T) { cases := []struct { name string - role *rbacv1.Role roleBinding *rbacv1.RoleBinding - cluster *clusterv1.ManagedCluster - namespace *corev1.Namespace - work *workapiv1.ManifestWork + namespace string expectedRoleFinalizers []string expectedRoleBindingFinalizers []string expectedWorkFinalizers []string - expectedQueueLen int validateRbacActions func(t *testing.T, actions []clienttesting.Action) }{ { - name: "skip if neither role nor rolebinding exists", - cluster: testinghelpers.NewManagedCluster(), - namespace: testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, false), - work: testinghelpers.NewManifestWork(testinghelpers.TestManagedClusterName, "work1", nil, nil), + name: "skip if rolebinding does not exists", + namespace: testinghelpers.TestManagedClusterName, validateRbacActions: testingcommon.AssertNoActions, }, { - name: "skip if neither role nor rolebinding has finalizer", - role: testinghelpers.NewRole(testinghelpers.TestManagedClusterName, roleName, nil, false), - roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, nil, false), - cluster: testinghelpers.NewManagedCluster(), - namespace: testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, false), - work: testinghelpers.NewManifestWork(testinghelpers.TestManagedClusterName, "work1", []string{manifestWorkFinalizer}, nil), - expectedWorkFinalizers: []string{manifestWorkFinalizer}, - validateRbacActions: testingcommon.AssertNoActions, + name: "skip if rolebinding has no finalizer", roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, nil, nil, false), + namespace: testinghelpers.TestManagedClusterName, + validateRbacActions: testingcommon.AssertNoActions, }, { - name: "remove finalizer from deleting role within non-terminating namespace", - role: testinghelpers.NewRole(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true), - roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, false), - cluster: testinghelpers.NewManagedCluster(), - namespace: testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, false), - work: testinghelpers.NewManifestWork(testinghelpers.TestManagedClusterName, "work1", []string{manifestWorkFinalizer}, nil), + name: "skip if rolebinding has no labels", + roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, nil, false), + namespace: testinghelpers.TestManagedClusterName, expectedRoleBindingFinalizers: []string{manifestWorkFinalizer}, - expectedWorkFinalizers: []string{manifestWorkFinalizer}, - validateRbacActions: func(t *testing.T, actions []clienttesting.Action) { - testingcommon.AssertActions(t, actions, "update") - }, - }, - { - name: "remove finalizer from role/rolebinding within terminating cluster", - role: testinghelpers.NewRole(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true), - roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true), - cluster: testinghelpers.NewDeletingManagedCluster(), - namespace: testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, false), - validateRbacActions: func(t *testing.T, actions []clienttesting.Action) { - testingcommon.AssertActions(t, actions, "update", "update") - }, + validateRbacActions: testingcommon.AssertNoActions, }, { - name: "remove finalizer from role/rolebinding within terminating ns", - role: testinghelpers.NewRole(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true), - roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, true), - namespace: testinghelpers.NewNamespace(testinghelpers.TestManagedClusterName, true), + name: "remove finalizer from deleting roleBinding", + roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, + map[string]string{clusterv1.ClusterNameLabelKey: testinghelpers.TestManagedClusterName}, true), + namespace: testinghelpers.TestManagedClusterName, + expectedRoleFinalizers: []string{manifestWorkFinalizer}, validateRbacActions: func(t *testing.T, actions []clienttesting.Action) { - testingcommon.AssertActions(t, actions, "update", "update") + testingcommon.AssertActions(t, actions, "update") }, }, } @@ -177,28 +151,18 @@ func TestSyncRoleAndRoleBinding(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { objects := []runtime.Object{} - if c.role != nil { - objects = append(objects, c.role) - } if c.roleBinding != nil { objects = append(objects, c.roleBinding) } fakeClient := fakeclient.NewSimpleClientset(objects...) - var fakeManifestWorkClient *fakeworkclient.Clientset - if c.work == nil { - fakeManifestWorkClient = fakeworkclient.NewSimpleClientset() - } else { - fakeManifestWorkClient = fakeworkclient.NewSimpleClientset(c.work) - } - - workInformerFactory := workinformers.NewSharedInformerFactory(fakeManifestWorkClient, 5*time.Minute) + kubeInformerFactory := kubeinformers.NewSharedInformerFactory(fakeClient, time.Minute*10) recorder := events.NewInMemoryRecorder("") controller := finalizeController{ - manifestWorkLister: workInformerFactory.Work().V1().ManifestWorks().Lister(), - eventRecorder: recorder, - rbacClient: fakeClient.RbacV1(), + roleBindingLister: kubeInformerFactory.Rbac().V1().RoleBindings().Lister(), + eventRecorder: recorder, + rbacClient: fakeClient.RbacV1(), } controllerContext := testingcommon.NewFakeSyncContext(t, "") @@ -207,23 +171,15 @@ func TestSyncRoleAndRoleBinding(t *testing.T) { ctx, cancel := context.WithTimeout(context.TODO(), 15*time.Second) defer cancel() - workInformerFactory.Start(ctx.Done()) - workInformerFactory.WaitForCacheSync(ctx.Done()) - - if err := controller.syncRoleAndRoleBinding(context.TODO(), controllerContext, c.role, c.roleBinding, c.namespace, c.cluster); err != nil { + kubeInformerFactory.Start(ctx.Done()) + kubeInformerFactory.WaitForCacheSync(ctx.Done()) + fakeClient.ClearActions() + if err := controller.syncRoleBindings(context.TODO(), controllerContext, c.namespace); err != nil { t.Fatal(err) } c.validateRbacActions(t, fakeClient.Actions()) - if c.role != nil { - role, err := fakeClient.RbacV1().Roles(c.role.Namespace).Get(context.TODO(), c.role.Name, metav1.GetOptions{}) - if err != nil { - t.Fatal(err) - } - testinghelpers.AssertFinalizers(t, role, c.expectedRoleFinalizers) - } - if c.roleBinding != nil { rolebinding, err := fakeClient.RbacV1().RoleBindings(c.roleBinding.Namespace).Get(context.TODO(), c.roleBinding.Name, metav1.GetOptions{}) if err != nil { @@ -232,18 +188,6 @@ func TestSyncRoleAndRoleBinding(t *testing.T) { testinghelpers.AssertFinalizers(t, rolebinding, c.expectedRoleBindingFinalizers) } - if c.work != nil { - work, err := fakeManifestWorkClient.WorkV1().ManifestWorks(c.work.Namespace).Get(context.TODO(), c.work.Name, metav1.GetOptions{}) - if err != nil { - t.Fatal(err) - } - testinghelpers.AssertFinalizers(t, work, c.expectedWorkFinalizers) - } - - actual := controllerContext.Queue().Len() - if actual != c.expectedQueueLen { - t.Errorf("Expect queue with length: %d, but got %d", c.expectedQueueLen, actual) - } }() }) }