Skip to content

Commit

Permalink
Use finalizer in api repo (#241)
Browse files Browse the repository at this point in the history
Signed-off-by: Jian Qiu <[email protected]>
  • Loading branch information
qiujian16 authored Aug 4, 2023
1 parent 6d6a6f1 commit 3167826
Show file tree
Hide file tree
Showing 29 changed files with 62 additions and 85 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
clienttesting "k8s.io/client-go/testing"
"k8s.io/klog/v2"

workv1 "open-cluster-management.io/api/work/v1"

testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
"open-cluster-management.io/ocm/pkg/operator/helpers"
)
Expand All @@ -25,8 +27,8 @@ func TestSyncDelete(t *testing.T) {
namespace := newNamespace("testns")
appliedManifestWorks := []runtime.Object{
newAppliedManifestWorks("testhost", nil, false),
newAppliedManifestWorks("testhost", []string{appliedManifestWorkFinalizer}, true),
newAppliedManifestWorks("testhost-2", []string{appliedManifestWorkFinalizer}, false),
newAppliedManifestWorks("testhost", []string{workv1.AppliedManifestWorkFinalizer}, true),
newAppliedManifestWorks("testhost-2", []string{workv1.AppliedManifestWorkFinalizer}, false),
}
controller := newTestController(t, klusterlet, appliedManifestWorks, namespace, bootstrapKubeConfigSecret)
syncContext := testingcommon.NewFakeSyncContext(t, "klusterlet")
Expand Down Expand Up @@ -83,8 +85,8 @@ func TestSyncDeleteHosted(t *testing.T) {
namespace := newNamespace(agentNamespace)
appliedManifestWorks := []runtime.Object{
newAppliedManifestWorks("testhost", nil, false),
newAppliedManifestWorks("testhost", []string{appliedManifestWorkFinalizer}, true),
newAppliedManifestWorks("testhost-2", []string{appliedManifestWorkFinalizer}, false),
newAppliedManifestWorks("testhost", []string{workv1.AppliedManifestWorkFinalizer}, true),
newAppliedManifestWorks("testhost-2", []string{workv1.AppliedManifestWorkFinalizer}, false),
}
controller := newTestControllerHosted(t, klusterlet, appliedManifestWorks, bootstrapKubeConfigSecret, namespace /*externalManagedSecret*/)
syncContext := testingcommon.NewFakeSyncContext(t, klusterlet.Name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ const (
klusterletReadyToApply = "ReadyToApply"
hubConnectionDegraded = "HubConnectionDegraded"
hubKubeConfigSecretMissing = "HubKubeConfigSecretMissing" // #nosec G101
appliedManifestWorkFinalizer = "cluster.open-cluster-management.io/applied-manifest-work-cleanup"
managedResourcesEvictionTimestampAnno = "operator.open-cluster-management.io/managed-resources-eviction-timestamp"
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func (r *managedReconcile) cleanUpAppliedManifestWorks(ctx context.Context, klus
}

// remove finalizer if exists
if err := mwpatcher.RemoveFinalizer(ctx, &appliedManifestWorks.Items[index], appliedManifestWorkFinalizer); err != nil {
if err := mwpatcher.RemoveFinalizer(ctx, &appliedManifestWorks.Items[index], workapiv1.AppliedManifestWorkFinalizer); err != nil {
errs = append(errs, fmt.Errorf("unable to remove finalizer from AppliedManifestWork %q: %w", appliedManifestWorks.Items[index].Name, err))
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/registration/helpers/testing/testinghelpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func NewManagedCluster() *clusterv1.ManagedCluster {

func NewAcceptingManagedCluster() *clusterv1.ManagedCluster {
managedCluster := NewManagedCluster()
managedCluster.Finalizers = []string{"cluster.open-cluster-management.io/api-resource-cleanup"}
managedCluster.Finalizers = []string{clusterv1.ManagedClusterFinalizer}
managedCluster.Spec.HubAcceptsClient = true
return managedCluster
}
Expand Down Expand Up @@ -140,7 +140,7 @@ func NewDeletingManagedCluster() *clusterv1.ManagedCluster {
ObjectMeta: metav1.ObjectMeta{
Name: TestManagedClusterName,
DeletionTimestamp: &now,
Finalizers: []string{"cluster.open-cluster-management.io/api-resource-cleanup"},
Finalizers: []string{clusterv1.ManagedClusterFinalizer},
},
}
}
Expand Down
8 changes: 2 additions & 6 deletions pkg/registration/hub/managedcluster/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ import (
"open-cluster-management.io/ocm/pkg/registration/helpers"
)

const (
managedClusterFinalizer = "cluster.open-cluster-management.io/api-resource-cleanup"
)

//go:embed manifests
var manifestFiles embed.FS

Expand Down Expand Up @@ -103,7 +99,7 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn

newManagedCluster := managedCluster.DeepCopy()
if managedCluster.DeletionTimestamp.IsZero() {
updated, err := c.patcher.AddFinalizer(ctx, managedCluster, managedClusterFinalizer)
updated, err := c.patcher.AddFinalizer(ctx, managedCluster, v1.ManagedClusterFinalizer)
if err != nil || updated {
return err
}
Expand All @@ -114,7 +110,7 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
if err := c.removeManagedClusterResources(ctx, managedClusterName); err != nil {
return err
}
return c.patcher.RemoveFinalizer(ctx, managedCluster, managedClusterFinalizer)
return c.patcher.RemoveFinalizer(ctx, managedCluster, v1.ManagedClusterFinalizer)
}

if !managedCluster.Spec.HubAcceptsClient {
Expand Down
2 changes: 1 addition & 1 deletion pkg/registration/hub/managedcluster/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestSyncManagedCluster(t *testing.T) {
if err != nil {
t.Fatal(err)
}
testinghelpers.AssertFinalizers(t, managedCluster, []string{managedClusterFinalizer})
testinghelpers.AssertFinalizers(t, managedCluster, []string{v1.ManagedClusterFinalizer})
},
},
{
Expand Down
9 changes: 3 additions & 6 deletions pkg/registration/hub/rbacfinalizerdeletion/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,11 @@ import (
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"
workapiv1 "open-cluster-management.io/api/work/v1"

"open-cluster-management.io/ocm/pkg/common/queue"
)

const (
manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
)

type finalizeController struct {
roleBindingLister rbacv1listers.RoleBindingLister
rbacClient rbacv1client.RbacV1Interface
Expand Down Expand Up @@ -123,12 +120,12 @@ func (m *finalizeController) syncRoleBindings(ctx context.Context, controllerCon

for _, roleBinding := range roleBindings {
// Skip if roleBinding has no the finalizer
if !hasFinalizer(roleBinding, manifestWorkFinalizer) {
if !hasFinalizer(roleBinding, workapiv1.ManifestWorkFinalizer) {
continue
}
// remove finalizer from roleBinding
if pendingFinalization(roleBinding) {
if err := m.removeFinalizerFromRoleBinding(ctx, roleBinding, manifestWorkFinalizer); err != nil {
if err := m.removeFinalizerFromRoleBinding(ctx, roleBinding, workapiv1.ManifestWorkFinalizer); err != nil {
return err
}
}
Expand Down
21 changes: 13 additions & 8 deletions pkg/registration/hub/rbacfinalizerdeletion/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ 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"
Expand Down Expand Up @@ -66,8 +67,10 @@ func TestSync(t *testing.T) {
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)},
[]string{workapiv1.ManifestWorkFinalizer}, map[string]string{clusterv1.ClusterNameLabelKey: testinghelpers.TestManagedClusterName}, true)},
works: []runtime.Object{
testinghelpers.NewManifestWork(
testinghelpers.TestManagedClusterName, "work1", []string{workapiv1.ManifestWorkFinalizer}, nil)},
expectedErr: "",
},
}
Expand Down Expand Up @@ -145,23 +148,25 @@ func TestSyncRoleAndRoleBinding(t *testing.T) {
validateRbacActions: testingcommon.AssertNoActions,
},
{
name: "skip if rolebinding has no finalizer", roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, nil, nil, false),
name: "skip if rolebinding has no finalizer",
roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, nil, nil, false),
namespace: testinghelpers.TestManagedClusterName,
validateRbacActions: testingcommon.AssertNoActions,
},
{
name: "skip if rolebinding has no labels",
roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer}, nil, false),
name: "skip if rolebinding has no labels",
roleBinding: testinghelpers.NewRoleBinding(
testinghelpers.TestManagedClusterName, roleName, []string{workapiv1.ManifestWorkFinalizer}, nil, false),
namespace: testinghelpers.TestManagedClusterName,
expectedRoleBindingFinalizers: []string{manifestWorkFinalizer},
expectedRoleBindingFinalizers: []string{workapiv1.ManifestWorkFinalizer},
validateRbacActions: testingcommon.AssertNoActions,
},
{
name: "remove finalizer from deleting roleBinding",
roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{manifestWorkFinalizer},
roleBinding: testinghelpers.NewRoleBinding(testinghelpers.TestManagedClusterName, roleName, []string{workapiv1.ManifestWorkFinalizer},
map[string]string{clusterv1.ClusterNameLabelKey: testinghelpers.TestManagedClusterName}, true),
namespace: testinghelpers.TestManagedClusterName,
expectedRoleFinalizers: []string{manifestWorkFinalizer},
expectedRoleFinalizers: []string{workapiv1.ManifestWorkFinalizer},
validateRbacActions: func(t *testing.T, actions []clienttesting.Action) {
testingcommon.AssertActions(t, actions, "update")
},
Expand Down
11 changes: 0 additions & 11 deletions pkg/work/spoke/controllers/constants.go

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"open-cluster-management.io/ocm/pkg/common/patcher"
"open-cluster-management.io/ocm/pkg/common/queue"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
)

// AddFinalizerController is to add the cluster.open-cluster-management.io/manifest-work-cleanup finalizer to manifestworks.
Expand Down Expand Up @@ -68,7 +67,7 @@ func (m *AddFinalizerController) syncManifestWork(ctx context.Context, originalM
}

// if this conflicts, we'll simply try again later
_, err := m.patcher.AddFinalizer(ctx, manifestWork, controllers.ManifestWorkFinalizer)
_, err := m.patcher.AddFinalizer(ctx, manifestWork, workapiv1.ManifestWorkFinalizer)

return err
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"open-cluster-management.io/ocm/pkg/common/patcher"
testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
"open-cluster-management.io/ocm/pkg/work/spoke/spoketesting"
)

Expand All @@ -36,7 +35,7 @@ func TestAddFinalizer(t *testing.T) {
if err := json.Unmarshal(p, work); err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(work.Finalizers, []string{controllers.ManifestWorkFinalizer}) {
if !reflect.DeepEqual(work.Finalizers, []string{workapiv1.ManifestWorkFinalizer}) {
t.Fatal(spew.Sdump(actions))
}
},
Expand All @@ -51,7 +50,7 @@ func TestAddFinalizer(t *testing.T) {
if err := json.Unmarshal(p, work); err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(work.Finalizers, []string{"other", controllers.ManifestWorkFinalizer}) {
if !reflect.DeepEqual(work.Finalizers, []string{"other", workapiv1.ManifestWorkFinalizer}) {
t.Fatal(spew.Sdump(actions))
}
},
Expand All @@ -63,7 +62,7 @@ func TestAddFinalizer(t *testing.T) {
},
{
name: "skip when present",
existingFinalizers: []string{controllers.ManifestWorkFinalizer},
existingFinalizers: []string{workapiv1.ManifestWorkFinalizer},
validateActions: testingcommon.AssertNoActions,
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"open-cluster-management.io/ocm/pkg/common/patcher"
"open-cluster-management.io/ocm/pkg/common/queue"
"open-cluster-management.io/ocm/pkg/work/helper"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
)

// AppliedManifestWorkFinalizeController handles cleanup of appliedmanifestwork resources before deletion is allowed.
Expand Down Expand Up @@ -84,7 +83,7 @@ func (m *AppliedManifestWorkFinalizeController) syncAppliedManifestWork(ctx cont
}

// don't do work if the finalizer is not present
if !helper.HasFinalizer(appliedManifestWork.Finalizers, controllers.AppliedManifestWorkFinalizer) {
if !helper.HasFinalizer(appliedManifestWork.Finalizers, workapiv1.AppliedManifestWorkFinalizer) {
return nil
}

Expand Down Expand Up @@ -118,7 +117,7 @@ func (m *AppliedManifestWorkFinalizeController) syncAppliedManifestWork(ctx cont
// reset the rate limiter for the appliedmanifestwork
m.rateLimiter.Forget(appliedManifestWork.Name)

if err := m.patcher.RemoveFinalizer(ctx, appliedManifestWork, controllers.AppliedManifestWorkFinalizer); err != nil {
if err := m.patcher.RemoveFinalizer(ctx, appliedManifestWork, workapiv1.AppliedManifestWorkFinalizer); err != nil {
return fmt.Errorf("failed to remove finalizer from AppliedManifestWork %s: %w", appliedManifestWork.Name, err)
}
return nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"open-cluster-management.io/ocm/pkg/common/patcher"
testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
"open-cluster-management.io/ocm/pkg/work/helper"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
"open-cluster-management.io/ocm/pkg/work/spoke/spoketesting"
)

Expand All @@ -43,7 +42,7 @@ func TestFinalize(t *testing.T) {
}{
{
name: "skip when not delete",
existingFinalizers: []string{controllers.ManifestWorkFinalizer},
existingFinalizers: []string{workapiv1.ManifestWorkFinalizer},
validateAppliedManifestWorkActions: testingcommon.AssertNoActions,
validateDynamicActions: testingcommon.AssertNoActions,
},
Expand All @@ -57,7 +56,7 @@ func TestFinalize(t *testing.T) {
{
name: "get resources and remove finalizer",
terminated: true,
existingFinalizers: []string{"a", controllers.AppliedManifestWorkFinalizer, "b"},
existingFinalizers: []string{"a", workapiv1.AppliedManifestWorkFinalizer, "b"},
resourcesToRemove: []workapiv1.AppliedManifestResourceMeta{
{Version: "v1", ResourceIdentifier: workapiv1.ResourceIdentifier{Group: "g1", Resource: "r1", Namespace: "", Name: "n1"}},
{Version: "v2", ResourceIdentifier: workapiv1.ResourceIdentifier{Group: "g2", Resource: "r2", Namespace: "ns2", Name: "n2"}},
Expand Down Expand Up @@ -105,7 +104,7 @@ func TestFinalize(t *testing.T) {
{
name: "requeue work when deleting resources are still visiable",
terminated: true,
existingFinalizers: []string{controllers.AppliedManifestWorkFinalizer},
existingFinalizers: []string{workapiv1.AppliedManifestWorkFinalizer},
existingResources: []runtime.Object{
spoketesting.NewUnstructuredSecret("ns1", "n1", true, "ns1-n1", *owner),
spoketesting.NewUnstructuredSecret("ns2", "n2", true, "ns2-n2", *owner),
Expand Down Expand Up @@ -136,7 +135,7 @@ func TestFinalize(t *testing.T) {
{
name: "ignore re-created resource and remove finalizer",
terminated: true,
existingFinalizers: []string{controllers.AppliedManifestWorkFinalizer},
existingFinalizers: []string{workapiv1.AppliedManifestWorkFinalizer},
existingResources: []runtime.Object{
spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1", *owner),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"open-cluster-management.io/ocm/pkg/common/patcher"
"open-cluster-management.io/ocm/pkg/common/queue"
"open-cluster-management.io/ocm/pkg/work/helper"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
)

// ManifestWorkFinalizeController handles cleanup of manifestwork resources before deletion is allowed.
Expand Down Expand Up @@ -107,7 +106,7 @@ func (m *ManifestWorkFinalizeController) sync(ctx context.Context, controllerCon

m.rateLimiter.Forget(manifestWorkName)
manifestWork = manifestWork.DeepCopy()
if err := m.patcher.RemoveFinalizer(ctx, manifestWork, controllers.ManifestWorkFinalizer); err != nil {
if err := m.patcher.RemoveFinalizer(ctx, manifestWork, workapiv1.ManifestWorkFinalizer); err != nil {
return fmt.Errorf("failed to remove finalizer from ManifestWork %s/%s: %w", manifestWork.Namespace, manifestWork.Name, err)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (

"open-cluster-management.io/ocm/pkg/common/patcher"
testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
)

func TestSyncManifestWorkController(t *testing.T) {
Expand Down Expand Up @@ -78,7 +77,7 @@ func TestSyncManifestWorkController(t *testing.T) {
Name: "work",
Namespace: "cluster1",
DeletionTimestamp: &now,
Finalizers: []string{controllers.ManifestWorkFinalizer},
Finalizers: []string{workapiv1.ManifestWorkFinalizer},
},
},
appliedWork: &workapiv1.AppliedManifestWork{
Expand All @@ -100,7 +99,7 @@ func TestSyncManifestWorkController(t *testing.T) {
Name: "work",
Namespace: "cluster1",
DeletionTimestamp: &now,
Finalizers: []string{controllers.ManifestWorkFinalizer},
Finalizers: []string{workapiv1.ManifestWorkFinalizer},
},
},
appliedWork: &workapiv1.AppliedManifestWork{
Expand All @@ -121,7 +120,7 @@ func TestSyncManifestWorkController(t *testing.T) {
Name: "work",
Namespace: "cluster1",
DeletionTimestamp: &now,
Finalizers: []string{controllers.ManifestWorkFinalizer},
Finalizers: []string{workapiv1.ManifestWorkFinalizer},
},
},
appliedWork: &workapiv1.AppliedManifestWork{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"open-cluster-management.io/ocm/pkg/work/spoke/apply"
"open-cluster-management.io/ocm/pkg/work/spoke/auth"
"open-cluster-management.io/ocm/pkg/work/spoke/auth/basic"
"open-cluster-management.io/ocm/pkg/work/spoke/controllers"
)

var (
Expand Down Expand Up @@ -129,7 +128,7 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac

// don't do work if the finalizer is not present
// it ensures all maintained resources will be cleaned once manifestwork is deleted
if !helper.HasFinalizer(manifestWork.Finalizers, controllers.ManifestWorkFinalizer) {
if !helper.HasFinalizer(manifestWork.Finalizers, workapiv1.ManifestWorkFinalizer) {
return nil
}

Expand Down Expand Up @@ -235,7 +234,7 @@ func (m *ManifestWorkController) applyAppliedManifestWork(ctx context.Context, w
requiredAppliedWork := &workapiv1.AppliedManifestWork{
ObjectMeta: metav1.ObjectMeta{
Name: appliedManifestWorkName,
Finalizers: []string{controllers.AppliedManifestWorkFinalizer},
Finalizers: []string{workapiv1.AppliedManifestWorkFinalizer},
},
Spec: workapiv1.AppliedManifestWorkSpec{
HubHash: hubHash,
Expand Down
Loading

0 comments on commit 3167826

Please sign in to comment.