Delete resources applied by controller only

pkg/helper/helper_test.go

@@ -348,7 +348,6 @@ func TestDeleteAppliedResourcess(t *testing.T) {
 				{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
 			},
 			expectedResourcesPendingFinalization: []workapiv1.AppliedManifestResourceMeta{
-				{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
 				{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
 			},
 		},

pkg/helper/helpers.go

@@ -18,6 +18,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog"
@@ -264,6 +265,7 @@ func UpdateManifestWorkStatus(
 }
 
 // DeleteAppliedResources deletes all given applied resources and returns those pending for finalization
+// If the uid recorded in resources is different from what we get by client, ignore the deletion.
 func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, dynamicClient dynamic.Interface) ([]workapiv1.AppliedManifestResourceMeta, []error) {
 	var resourcesPendingFinalization []workapiv1.AppliedManifestResourceMeta
 	var errs []error
@@ -286,28 +288,18 @@ func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, d
 			continue
 		}
 
-		pendingFinalization := u.GetDeletionTimestamp() != nil && !u.GetDeletionTimestamp().IsZero()
-		waitingForFinalization := len(resource.UID) > 0
-		if pendingFinalization {
-			if waitingForFinalization && resource.UID != string(u.GetUID()) {
-				// the instance is deleted, so do not add to the resourcesPendingDeletion list
-				continue
-			}
-
-			// if we aren't waiting for finalization or the UID does match, then we want to ensure we add it to the pending list
-			newResource := copyAppliedResource(resource)
-			newResource.UID = string(u.GetUID())
-			resourcesPendingFinalization = append(resourcesPendingFinalization, newResource)
+		if resource.UID != string(u.GetUID()) {
+			// the traced instance has been deleted, and forget this item.
 			continue
 		}
 
-		// forget this item if the UID does not match the resource UID we previously deleted
-		if waitingForFinalization && resource.UID != string(u.GetUID()) {
+		if u.GetDeletionTimestamp() != nil && !u.GetDeletionTimestamp().IsZero() {
+			resourcesPendingFinalization = append(resourcesPendingFinalization, resource)
 			continue
 		}
 
 		// delete the resource which is not deleted yet
-		uid := u.GetUID()
+		uid := types.UID(resource.UID)
 		err = dynamicClient.
 			Resource(gvr).
 			Namespace(resource.Namespace).
@@ -330,27 +322,13 @@ func DeleteAppliedResources(resources []workapiv1.AppliedManifestResourceMeta, d
 			continue
 		}
 
-		// set UID of applied resource once deletion is successful
-		newResource := copyAppliedResource(resource)
-		newResource.UID = string(u.GetUID())
-		resourcesPendingFinalization = append(resourcesPendingFinalization, newResource)
+		resourcesPendingFinalization = append(resourcesPendingFinalization, resource)
 		klog.V(2).Infof("Delete resource %v with key %s/%s", gvr, resource.Namespace, resource.Name)
 	}
 
 	return resourcesPendingFinalization, errs
 }
 
-func copyAppliedResource(resource workapiv1.AppliedManifestResourceMeta) workapiv1.AppliedManifestResourceMeta {
-	return workapiv1.AppliedManifestResourceMeta{
-		Group:     resource.Group,
-		Version:   resource.Version,
-		Resource:  resource.Resource,
-		Name:      resource.Name,
-		Namespace: resource.Namespace,
-		UID:       resource.UID,
-	}
-}
-
 // GuessObjectGroupVersionKind returns GVK for the passed runtime object.
 func GuessObjectGroupVersionKind(object runtime.Object) (*schema.GroupVersionKind, error) {
 	gvk := resourcehelper.GuessObjectGroupVersionKind(object)

pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go

@@ -94,6 +94,10 @@ func (m *AppliedManifestWorkController) sync(ctx context.Context, controllerCont
 	if err != nil {
 		return err
 	}
+	// no work to do if we're deleted
+	if !appliedManifestWork.DeletionTimestamp.IsZero() {
+		return nil
+	}
 
 	return m.syncManifestWork(ctx, controllerContext, manifestWork, appliedManifestWork)
 }
@@ -111,20 +115,43 @@ func (m *AppliedManifestWorkController) syncManifestWork(
 	// spec because manifests in spec are only resource templates, while resource status records the real resources
 	// maintained by the manifest work.
 	var appliedResources []workapiv1.AppliedManifestResourceMeta
+	var errs []error
 	for _, resourceStatus := range manifestWork.Status.ResourceStatus.Manifests {
 		gvr := schema.GroupVersionResource{Group: resourceStatus.ResourceMeta.Group, Version: resourceStatus.ResourceMeta.Version, Resource: resourceStatus.ResourceMeta.Resource}
 		if len(gvr.Resource) == 0 || len(gvr.Version) == 0 || len(resourceStatus.ResourceMeta.Name) == 0 {
 			continue
 		}
 
+		u, err := m.spokeDynamicClient.
+			Resource(gvr).
+			Namespace(resourceStatus.ResourceMeta.Namespace).
+			Get(context.TODO(), resourceStatus.ResourceMeta.Name, metav1.GetOptions{})
+		if errors.IsNotFound(err) {
+			klog.V(2).Infof(
+				"Resource %v with key %s/%s does not exist",
+				gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name)
+			continue
+		}
+
+		if err != nil {
+			errs = append(errs, fmt.Errorf(
+				"Failed to get resource %v with key %s/%s: %w",
+				gvr, resourceStatus.ResourceMeta.Namespace, resourceStatus.ResourceMeta.Name, err))
+			continue
+		}
+
 		appliedResources = append(appliedResources, workapiv1.AppliedManifestResourceMeta{
 			Group:     resourceStatus.ResourceMeta.Group,
 			Version:   resourceStatus.ResourceMeta.Version,
 			Resource:  resourceStatus.ResourceMeta.Resource,
 			Namespace: resourceStatus.ResourceMeta.Namespace,
 			Name:      resourceStatus.ResourceMeta.Name,
+			UID:       string(u.GetUID()),
 		})
 	}
+	if len(errs) != 0 {
+		return utilerrors.NewAggregate(errs)
+	}
 
 	// delete applied resources which are no longer maintained by manifest work
 	noLongerMaintainedResources := findUntrackedResources(appliedManifestWork.Status.AppliedResources, appliedResources)
@@ -178,7 +205,9 @@ func findUntrackedResources(appliedResources, newAppliedResources []workapiv1.Ap
 
 	resourceIndex := map[workapiv1.AppliedManifestResourceMeta]struct{}{}
 	for _, resource := range newAppliedResources {
-		resourceIndex[resource] = struct{}{}
+		key := resource.DeepCopy()
+		key.UID = ""
+		resourceIndex[*key] = struct{}{}
 	}
 
 	for _, resource := range appliedResources {

pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go

@@ -38,39 +38,46 @@ func TestSyncManifestWork(t *testing.T) {
 		appliedResources                   []workapiv1.AppliedManifestResourceMeta
 		manifests                          []workapiv1.ManifestCondition
 		validateAppliedManifestWorkActions func(t *testing.T, actions []clienttesting.Action)
-		validateDynamicActions             func(t *testing.T, actions []clienttesting.Action)
+		expectedDeleteActions              []clienttesting.DeleteActionImpl
 		expectedQueueLen                   int
 	}{
 		{
 			name: "skip when no applied resource changed",
-			appliedResources: []workapiv1.AppliedManifestResourceMeta{
-				{Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"},
+			existingResources: []runtime.Object{
+				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
 			},
-			manifests: []workapiv1.ManifestCondition{newManifest("g1", "v1", "r1", "ns1", "n1")},
+			appliedResources: []workapiv1.AppliedManifestResourceMeta{
+				{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+			},
+			manifests: []workapiv1.ManifestCondition{newManifest("", "v1", "secrets", "ns1", "n1")},
 			validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) {
 				if len(actions) > 0 {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
-			validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) {
-				if len(actions) > 0 {
-					t.Fatal(spew.Sdump(actions))
-				}
-			},
+			expectedDeleteActions: []clienttesting.DeleteActionImpl{},
 		},
 		{
 			name: "delete untracked resources",
+			existingResources: []runtime.Object{
+				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
+				spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2"),
+				spoketesting.NewUnstructuredSecret("ns3", "n3", false, "ns3-n3"),
+				spoketesting.NewUnstructuredSecret("ns4", "n4", false, "ns4-n4"),
+				spoketesting.NewUnstructuredSecret("ns5", "n5", false, "ns5-n5"),
+				spoketesting.NewUnstructuredSecret("ns6", "n6", false, "ns6-n6"),
+			},
 			appliedResources: []workapiv1.AppliedManifestResourceMeta{
-				{Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"},
-				{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
-				{Group: "g3", Version: "v3", Resource: "r3", Namespace: "ns3", Name: "n3"},
-				{Group: "g4", Version: "v4", Resource: "r4", Namespace: "ns4", Name: "n4"},
+				{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+				{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
+				{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"},
+				{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns4", Name: "n4", UID: "ns4-n4"},
 			},
 			manifests: []workapiv1.ManifestCondition{
-				newManifest("g1", "v1", "r1", "ns1", "n1"),
-				newManifest("g2", "v2", "r2", "ns2", "n2"),
-				newManifest("g5", "v5", "r5", "ns5", "n5"),
-				newManifest("g6", "v6", "r6", "ns6", "n6"),
+				newManifest("", "v1", "secrets", "ns1", "n1"),
+				newManifest("", "v1", "secrets", "ns2", "n2"),
+				newManifest("", "v1", "secrets", "ns5", "n5"),
+				newManifest("", "v1", "secrets", "ns6", "n6"),
 			},
 			validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) {
 				if len(actions) != 1 {
@@ -78,39 +85,31 @@ func TestSyncManifestWork(t *testing.T) {
 				}
 				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork)
 				if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{
-					{Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"},
-					{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
-					{Group: "g5", Version: "v5", Resource: "r5", Namespace: "ns5", Name: "n5"},
-					{Group: "g6", Version: "v6", Resource: "r6", Namespace: "ns6", Name: "n6"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns4", Name: "n4", UID: "ns4-n4"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5", UID: "ns5-n5"},
+					{Group: "", Version: "v1", Resource: "secrets", Namespace: "ns6", Name: "n6", UID: "ns6-n6"},
 				}) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
-			validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) {
-				if len(actions) != 2 {
-					t.Fatal(spew.Sdump(actions))
-				}
-
-				action := actions[0].(clienttesting.GetAction)
-				resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName()
-				if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g3", Version: "v3", Resource: "r3"}) || namespace != "ns3" || name != "n3" {
-					t.Fatal(spew.Sdump(actions))
-				}
-				action = actions[1].(clienttesting.GetAction)
-				resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName()
-				if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "g4", Version: "v4", Resource: "r4"}) || namespace != "ns4" || name != "n4" {
-					t.Fatal(spew.Sdump(actions))
-				}
+			expectedDeleteActions: []clienttesting.DeleteActionImpl{
+				clienttesting.NewDeleteAction(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, "ns3", "n3"),
+				clienttesting.NewDeleteAction(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, "ns4", "n4"),
 			},
 		},
 		{
 			name: "requeue work when applied resource for stale manifest is deleting",
 			existingResources: []runtime.Object{
+				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
+				spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2"),
 				spoketesting.NewUnstructuredSecret("ns3", "n3", true, "ns3-n3"),
 			},
 			appliedResources: []workapiv1.AppliedManifestResourceMeta{
-				{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1"},
-				{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2"},
+				{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+				{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
 				{Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"},
 			},
 			manifests: []workapiv1.ManifestCondition{
@@ -122,23 +121,15 @@ func TestSyncManifestWork(t *testing.T) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
-			validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) {
-				if len(actions) != 1 {
-					t.Fatal(spew.Sdump(actions))
-				}
-
-				action := actions[0].(clienttesting.GetAction)
-				resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName()
-				if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns3" || name != "n3" {
-					t.Fatal(spew.Sdump(actions))
-				}
-			},
-			expectedQueueLen: 1,
+			expectedDeleteActions: []clienttesting.DeleteActionImpl{},
+			expectedQueueLen:      1,
 		},
 		{
 			name: "ignore re-created resource",
 			existingResources: []runtime.Object{
 				spoketesting.NewUnstructuredSecret("ns3", "n3", false, "ns3-n3-recreated"),
+				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
+				spoketesting.NewUnstructuredSecret("ns5", "n5", false, "ns5-n5"),
 			},
 			appliedResources: []workapiv1.AppliedManifestResourceMeta{
 				{Version: "v1", Resource: "secrets", Namespace: "ns3", Name: "n3", UID: "ns3-n3"},
@@ -154,28 +145,41 @@ func TestSyncManifestWork(t *testing.T) {
 				}
 				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork)
 				if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{
-					{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1"},
-					{Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5"},
+					{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+					{Version: "v1", Resource: "secrets", Namespace: "ns5", Name: "n5", UID: "ns5-n5"},
 				}) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
-			validateDynamicActions: func(t *testing.T, actions []clienttesting.Action) {
-				if len(actions) != 2 {
+			expectedDeleteActions: []clienttesting.DeleteActionImpl{},
+		},
+		{
+			name: "update resource uid",
+			existingResources: []runtime.Object{
+				spoketesting.NewUnstructuredSecret("ns1", "n1", false, "ns1-n1"),
+				spoketesting.NewUnstructuredSecret("ns2", "n2", false, "ns2-n2-updated"),
+			},
+			appliedResources: []workapiv1.AppliedManifestResourceMeta{
+				{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+				{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2"},
+			},
+			manifests: []workapiv1.ManifestCondition{
+				newManifest("", "v1", "secrets", "ns1", "n1"),
+				newManifest("", "v1", "secrets", "ns2", "n2"),
+			},
+			validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) != 1 {
 					t.Fatal(spew.Sdump(actions))
 				}
-
-				action := actions[0].(clienttesting.GetAction)
-				resource, namespace, name := action.GetResource(), action.GetNamespace(), action.GetName()
-				if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns3" || name != "n3" {
-					t.Fatal(spew.Sdump(actions))
-				}
-				action = actions[1].(clienttesting.GetAction)
-				resource, namespace, name = action.GetResource(), action.GetNamespace(), action.GetName()
-				if !reflect.DeepEqual(resource, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}) || namespace != "ns4" || name != "n4" {
+				work := actions[0].(clienttesting.UpdateAction).GetObject().(*workapiv1.AppliedManifestWork)
+				if !reflect.DeepEqual(work.Status.AppliedResources, []workapiv1.AppliedManifestResourceMeta{
+					{Version: "v1", Resource: "secrets", Namespace: "ns1", Name: "n1", UID: "ns1-n1"},
+					{Version: "v1", Resource: "secrets", Namespace: "ns2", Name: "n2", UID: "ns2-n2-updated"},
+				}) {
 					t.Fatal(spew.Sdump(actions))
 				}
 			},
+			expectedDeleteActions: []clienttesting.DeleteActionImpl{},
 		},
 	}
 
@@ -207,7 +211,16 @@ func TestSyncManifestWork(t *testing.T) {
 				t.Fatal(err)
 			}
 			c.validateAppliedManifestWorkActions(t, fakeClient.Actions())
-			c.validateDynamicActions(t, fakeDynamicClient.Actions())
+
+			deleteActions := []clienttesting.DeleteActionImpl{}
+			for _, action := range fakeDynamicClient.Actions() {
+				if action.GetVerb() == "delete" {
+					deleteActions = append(deleteActions, action.(clienttesting.DeleteActionImpl))
+				}
+			}
+			if !reflect.DeepEqual(c.expectedDeleteActions, deleteActions) {
+				t.Fatal(spew.Sdump(deleteActions))
+			}
 
 			queueLen := controllerContext.Queue().Len()
 			if queueLen != c.expectedQueueLen {

pkg/spoke/controllers/finalizercontroller/manifestwork_finalize_controller_test.go

@@ -186,7 +186,6 @@ func TestSyncManifestWorkController(t *testing.T) {
 				},
 			},
 			validateAppliedManifestWorkActions: func(t *testing.T, actions []clienttesting.Action) {
-				fmt.Printf("Actions are %#v\n", actions)
 				if len(actions) != 1 {
 					t.Errorf("Expect 2 actions on appliedmanifestwork, but have %d", len(actions))
 				}

test/e2e/work_agent_test.go

@@ -214,7 +214,17 @@ var _ = ginkgo.Describe("Work agent", func() {
 				{Version: "v1", Resource: "configmaps", Namespace: ns2, Name: "cm3"},
 				{Version: "v1", Resource: "namespaces", Name: ns1},
 			}
-			gomega.Expect(reflect.DeepEqual(appliedManifestWork.Status.AppliedResources, expectedAppliedResources)).To(gomega.BeTrue())
+			actualAppliedResources := []workapiv1.AppliedManifestResourceMeta{}
+			for _, appliedResource := range appliedManifestWork.Status.AppliedResources {
+				actualAppliedResources = append(actualAppliedResources, workapiv1.AppliedManifestResourceMeta{
+					Group:     appliedResource.Group,
+					Version:   appliedResource.Version,
+					Resource:  appliedResource.Resource,
+					Namespace: appliedResource.Namespace,
+					Name:      appliedResource.Name,
+				})
+			}
+			gomega.Expect(reflect.DeepEqual(actualAppliedResources, expectedAppliedResources)).To(gomega.BeTrue())
 
 			ginkgo.By("update manifestwork")
 			cmData := map[string]string{"x": "y"}

test/integration/util/assertion.go

@@ -181,6 +181,18 @@ func AssertAppliedResources(hubHash, workName string, gvrs []schema.GroupVersion
 			return false
 		}
 
-		return reflect.DeepEqual(appliedManifestWork.Status.AppliedResources, appliedResources)
+		// remove uid from each AppliedManifestResourceMeta
+		var actualAppliedResources []workapiv1.AppliedManifestResourceMeta
+		for _, appliedResource := range appliedManifestWork.Status.AppliedResources {
+			actualAppliedResources = append(actualAppliedResources, workapiv1.AppliedManifestResourceMeta{
+				Group:     appliedResource.Group,
+				Version:   appliedResource.Version,
+				Resource:  appliedResource.Resource,
+				Namespace: appliedResource.Namespace,
+				Name:      appliedResource.Name,
+			})
+		}
+
+		return reflect.DeepEqual(actualAppliedResources, appliedResources)
 	}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
 }