Check if finalizer exists before applying manifests

pkg/spoke/controllers/constants.go

@@ -0,0 +1,7 @@
+package controllers
+
+const (
+	// ManifestWorkFinalizer is the name of the finalizer added to manifestworks. It is used to ensure
+	// all maintained resources of a manifestwork are deleted before the manifestwork itself is deleted
+	ManifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
+)

pkg/spoke/controllers/finalizercontroller/add_finalizer_controller.go

@@ -14,6 +14,7 @@ import (
 	workv1client "github.com/open-cluster-management/api/client/work/clientset/versioned/typed/work/v1"
 	workinformer "github.com/open-cluster-management/api/client/work/informers/externalversions/work/v1"
 	worklister "github.com/open-cluster-management/api/client/work/listers/work/v1"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
 )
@@ -24,10 +25,6 @@ type AddFinalizerController struct {
 	manifestWorkLister worklister.ManifestWorkNamespaceLister
 }
 
-const (
-	manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
-)
-
 // NewAddFinalizerController returns a ManifestWorkController
 func NewAddFinalizerController(
 	recorder events.Recorder,
@@ -74,12 +71,12 @@ func (m *AddFinalizerController) syncManifestWork(ctx context.Context, originalM
 
 	// don't add finalizer to instances that already have it
 	for i := range manifestWork.Finalizers {
-		if manifestWork.Finalizers[i] == manifestWorkFinalizer {
+		if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer {
 			return nil
 		}
 	}
 	// if this conflicts, we'll simply try again later
-	manifestWork.Finalizers = append(manifestWork.Finalizers, manifestWorkFinalizer)
+	manifestWork.Finalizers = append(manifestWork.Finalizers, controllers.ManifestWorkFinalizer)
 	_, err := m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{})
 	return err
 }

pkg/spoke/controllers/finalizercontroller/add_finalizer_controller_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/davecgh/go-spew/spew"
 	fakeworkclient "github.com/open-cluster-management/api/client/work/clientset/versioned/fake"
 	workapiv1 "github.com/open-cluster-management/api/work/v1"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/open-cluster-management/work/pkg/spoke/spoketesting"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clienttesting "k8s.io/client-go/testing"
@@ -28,7 +29,7 @@ func TestAddFinalizer(t *testing.T) {
 					t.Fatal(spew.Sdump(actions))
 				}
 				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork)
-				if !reflect.DeepEqual(work.Finalizers, []string{manifestWorkFinalizer}) {
+				if !reflect.DeepEqual(work.Finalizers, []string{controllers.ManifestWorkFinalizer}) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
@@ -41,7 +42,7 @@ func TestAddFinalizer(t *testing.T) {
 					t.Fatal(spew.Sdump(actions))
 				}
 				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.ManifestWork)
-				if !reflect.DeepEqual(work.Finalizers, []string{"other", manifestWorkFinalizer}) {
+				if !reflect.DeepEqual(work.Finalizers, []string{"other", controllers.ManifestWorkFinalizer}) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
@@ -57,7 +58,7 @@ func TestAddFinalizer(t *testing.T) {
 		},
 		{
 			name:               "skip when present",
-			existingFinalizers: []string{manifestWorkFinalizer},
+			existingFinalizers: []string{controllers.ManifestWorkFinalizer},
 			validateActions: func(t *testing.T, actions []clienttesting.Action) {
 				if len(actions) > 0 {
 					t.Fatal(spew.Sdump(actions))

pkg/spoke/controllers/finalizercontroller/finalize_controller.go

@@ -10,6 +10,7 @@ import (
 	worklister "github.com/open-cluster-management/api/client/work/listers/work/v1"
 	workapiv1 "github.com/open-cluster-management/api/work/v1"
 	"github.com/open-cluster-management/work/pkg/helper"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -82,7 +83,7 @@ func (m *FinalizeController) syncManifestWork(ctx context.Context, controllerCon
 	// don't do work if the finalizer is not present
 	found := false
 	for i := range manifestWork.Finalizers {
-		if manifestWork.Finalizers[i] == manifestWorkFinalizer {
+		if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer {
 			found = true
 			break
 		}
@@ -122,7 +123,7 @@ func (m *FinalizeController) syncManifestWork(ctx context.Context, controllerCon
 	// reset the rate limiter for the manifest work
 	m.rateLimiter.Forget(manifestWork.Name)
 
-	removeFinalizer(manifestWork, manifestWorkFinalizer)
+	removeFinalizer(manifestWork, controllers.ManifestWorkFinalizer)
 	_, err = m.manifestWorkClient.Update(ctx, manifestWork, metav1.UpdateOptions{})
 	if err != nil {
 		return fmt.Errorf("Failed to remove finalizer from ManifestWork %s/%s: %w", manifestWork.Namespace, manifestWork.Name, err)

pkg/spoke/controllers/finalizercontroller/finalize_controller_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/davecgh/go-spew/spew"
 	fakeworkclient "github.com/open-cluster-management/api/client/work/clientset/versioned/fake"
 	workapiv1 "github.com/open-cluster-management/api/work/v1"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/open-cluster-management/work/pkg/spoke/spoketesting"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -31,7 +32,7 @@ func TestFinalize(t *testing.T) {
 	}{
 		{
 			name:                        "skip when not delete",
-			existingFinalizers:          []string{manifestWorkFinalizer},
+			existingFinalizers:          []string{controllers.ManifestWorkFinalizer},
 			validateManifestWorkActions: noAction,
 			validateDynamicActions:      noAction,
 		},
@@ -45,7 +46,7 @@ func TestFinalize(t *testing.T) {
 		{
 			name:               "delete resources and remove finalizer",
 			terminated:         true,
-			existingFinalizers: []string{"a", manifestWorkFinalizer, "b"},
+			existingFinalizers: []string{"a", controllers.ManifestWorkFinalizer, "b"},
 			resourcesToRemove: []workapiv1.AppliedManifestResourceMeta{
 				{Group: "g1", Version: "v1", Resource: "r1", Namespace: "", Name: "n1"},
 				{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
@@ -96,7 +97,7 @@ func TestFinalize(t *testing.T) {
 		{
 			name:               "requeue work when deleting resources are still visiable",
 			terminated:         true,
-			existingFinalizers: []string{manifestWorkFinalizer},
+			existingFinalizers: []string{controllers.ManifestWorkFinalizer},
 			existingResources: []runtime.Object{
 				spoketesting.NewUnstructuredSecret("ns1", "n1", true, "ns1-n1"),
 				spoketesting.NewUnstructuredSecret("ns2", "n2", true, "ns2-n2"),
@@ -127,7 +128,7 @@ func TestFinalize(t *testing.T) {
 		{
 			name:               "ignore re-created resource and remove finalizer",
 			terminated:         true,
-			existingFinalizers: []string{manifestWorkFinalizer},
+			existingFinalizers: []string{controllers.ManifestWorkFinalizer},
 			existingResources: []runtime.Object{
 				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
 			},

pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go

@@ -30,6 +30,7 @@ import (
 	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 
 	"github.com/open-cluster-management/work/pkg/helper"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/open-cluster-management/work/pkg/spoke/resource"
 )
 
@@ -45,10 +46,6 @@ type ManifestWorkController struct {
 	restMapper *resource.Mapper
 }
 
-const (
-	manifestWorkFinalizer = "cluster.open-cluster-management.io/manifest-work-cleanup"
-)
-
 // NewManifestWorkController returns a ManifestWorkController
 func NewManifestWorkController(
 	ctx context.Context,
@@ -100,6 +97,19 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac
 		return nil
 	}
 
+	// don't do work if the finalizer is not present
+	// it ensures all maintained resources will be cleaned once manifestwork is deleted
+	found := false
+	for i := range manifestWork.Finalizers {
+		if manifestWork.Finalizers[i] == controllers.ManifestWorkFinalizer {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return nil
+	}
+
 	errs := []error{}
 	// Apply resources on spoke cluster.
 	resourceResults := m.applyManifest(manifestWork.Spec.Workload.Manifests, controllerContext.Recorder())

pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go

@@ -8,6 +8,7 @@ import (
 	fakeworkclient "github.com/open-cluster-management/api/client/work/clientset/versioned/fake"
 	workinformers "github.com/open-cluster-management/api/client/work/informers/externalversions"
 	workapiv1 "github.com/open-cluster-management/api/work/v1"
+	"github.com/open-cluster-management/work/pkg/spoke/controllers"
 	"github.com/open-cluster-management/work/pkg/spoke/resource"
 	"github.com/open-cluster-management/work/pkg/spoke/spoketesting"
 	corev1 "k8s.io/api/core/v1"
@@ -283,7 +284,7 @@ func TestSync(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
 			work, workKey := spoketesting.NewManifestWork(0, c.workManifest...)
-			work.Finalizers = []string{manifestWorkFinalizer}
+			work.Finalizers = []string{controllers.ManifestWorkFinalizer}
 			controller := newController(work, spoketesting.NewFakeRestMapper()).
 				withKubeObject(c.spokeObject...).
 				withUnstructuredObject(c.spokeDynamicObject...)
@@ -309,7 +310,7 @@ func TestFailedToApplyResource(t *testing.T) {
 		withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionFalse})
 
 	work, workKey := spoketesting.NewManifestWork(0, tc.workManifest...)
-	work.Finalizers = []string{manifestWorkFinalizer}
+	work.Finalizers = []string{controllers.ManifestWorkFinalizer}
 	controller := newController(work, spoketesting.NewFakeRestMapper()).withKubeObject(tc.spokeObject...).withUnstructuredObject()
 
 	// Add a reactor on fake client to throw error when creating secret on namespace ns2