Ignore version when finding untracked resource

Signed-off-by: Jian Qiu <jqiu@redhat.com>

pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller.go

@@ -201,19 +201,26 @@ func (m *AppliedManifestWorkController) syncManifestWork(
 }
 
 // findUntrackedResources returns applied resources which are no longer tracked by manifestwork
+// API version should be ignored when checking if a resource is no longer tracked by a manifestwork.
+// This is because we treat resources of same GroupResource but different version equivalent.
+// It also compares UID of the appliedResources to identify the untracked appliedResources because
+// 1. The UID should keep the same for resources with different versions.
+// 2. The UID in the newAppliedResources is always the latest updated one. The only possibility that UID
+// in appliedResources differs from what in newAppliedResources is that this resource is recreated.
+// Its UID in appliedResources is invalid hence recording it as untracked applied resource and delete it is safe.
 func findUntrackedResources(appliedResources, newAppliedResources []workapiv1.AppliedManifestResourceMeta) []workapiv1.AppliedManifestResourceMeta {
 	var untracked []workapiv1.AppliedManifestResourceMeta
 
 	resourceIndex := map[workapiv1.AppliedManifestResourceMeta]struct{}{}
 	for _, resource := range newAppliedResources {
 		key := resource.DeepCopy()
-		key.UID = ""
+		key.UID, key.Version = "", ""
 		resourceIndex[*key] = struct{}{}
 	}
 
 	for _, resource := range appliedResources {
 		key := resource.DeepCopy()
-		key.UID = ""
+		key.UID, key.Version = "", ""
 		if _, ok := resourceIndex[*key]; !ok {
 			untracked = append(untracked, resource)
 		}

pkg/spoke/controllers/appliedmanifestcontroller/appliedmanifestwork_controller_test.go

@@ -275,6 +275,20 @@ func TestFindUntrackedResources(t *testing.T) {
 				{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
 			},
 		},
+		{
+			name: "changing version of original resources does not make it untracked",
+			appliedResources: []workapiv1.AppliedManifestResourceMeta{
+				{Group: "g1", Version: "v1", Resource: "r1", Namespace: "ns1", Name: "n1"},
+				{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
+			},
+			newAppliedResources: []workapiv1.AppliedManifestResourceMeta{
+				{Group: "g1", Version: "v2", Resource: "r1", Namespace: "ns1", Name: "n1"},
+				{Group: "g4", Version: "v4", Resource: "r4", Namespace: "ns4", Name: "n4"},
+			},
+			expectedUntrackedResources: []workapiv1.AppliedManifestResourceMeta{
+				{Group: "g2", Version: "v2", Resource: "r2", Namespace: "ns2", Name: "n2"},
+			},
+		},
 	}
 
 	for _, c := range cases {

test/e2e/work_agent_test.go

@@ -18,6 +18,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 )
 
@@ -25,8 +26,8 @@ const (
 	eventuallyTimeout  = 60 // seconds
 	eventuallyInterval = 1  // seconds
 
-	guestbookCrdJson = `{
-		"apiVersion": "apiextensions.k8s.io/v1beta1",
+	guestBookCRDJson = `{
+		"apiVersion": "apiextensions.k8s.io/v1",
 		"kind": "CustomResourceDefinition",
 		"metadata": {
 			"name": "guestbooks.my.domain"
@@ -42,39 +43,70 @@ const (
 				"plural": "guestbooks",
 				"singular": "guestbook"
 			},
-			"preserveUnknownFields": true,
+			"preserveUnknownFields": false,
 			"scope": "Namespaced",
-			"validation": {
-				"openAPIV3Schema": {
-					"properties": {
-						"apiVersion": {
-							"type": "string"
-						},
-						"kind": {
-							"type": "string"
-						},
-						"metadata": {
-							"type": "object"
-						},
-						"spec": {
-							"properties": {
-								"foo": {
-									"type": "string"
-								}
-							},
-							"type": "object"
-						},
-						"status": {
-							"type": "object"
-						}
-					},
-					"type": "object"
-				}
-			},
-			"version": "v1",
 			"versions": [
 				{
 					"name": "v1",
+					"schema": {
+						"openAPIV3Schema": {
+							"properties": {
+								"apiVersion": {
+									"type": "string"
+								},
+								"kind": {
+									"type": "string"
+								},
+								"metadata": {
+									"type": "object"
+								},
+								"spec": {
+									"properties": {
+										"foo": {
+											"type": "string"
+										}
+									},
+									"type": "object"
+								},
+								"status": {
+									"type": "object"
+								}
+							},
+							"type": "object"
+						}
+					},
+					"served": true,
+					"storage": false
+				},
+				{
+					"name": "v2",
+					"schema": {
+						"openAPIV3Schema": {
+							"properties": {
+								"apiVersion": {
+									"type": "string"
+								},
+								"kind": {
+									"type": "string"
+								},
+								"metadata": {
+									"type": "object"
+								},
+								"spec": {
+									"properties": {
+										"foo": {
+											"type": "string"
+										}
+									},
+									"type": "object"
+								},
+								"status": {
+									"type": "object"
+								}
+							},
+							"type": "object"
+						}
+					},
 					"served": true,
 					"storage": true
 				}
@@ -82,7 +114,7 @@ const (
 		}
 	}`
 
-	guestbookCrJson = `{
+	guestbookCRJson = `{
 		"apiVersion": "my.domain/v1",
 		"kind": "Guestbook",
 		"metadata": {
@@ -93,6 +125,18 @@ const (
 			"foo": "bar"
 		}
 	}`
+
+	upgradedGuestBookCRJson = `{
+		"apiVersion": "my.domain/v2",
+		"kind": "Guestbook",
+		"metadata": {
+			"name": "guestbook1",
+			"namespace": "default"
+		},
+		"spec": {
+			"foo": "foo"
+		}
+	}`
 )
 
 var _ = ginkgo.Describe("Work agent", func() {
@@ -366,13 +410,13 @@ var _ = ginkgo.Describe("Work agent", func() {
 		})
 
 		ginkgo.It("should create crd/cr with aggregated cluster role successfully", func() {
-			crd, err := newCrd()
+			crd, err := newCrd(guestBookCRDJson)
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
 			clusterRoleName := fmt.Sprintf("cr-%s", nameSuffix)
 			clusterRole := newAggregatedClusterRole(clusterRoleName, "my.domain", "guestbooks")
 
-			cr, err := newCr(crNamespace, "cr1")
+			cr, err := newCr(guestbookCRJson, crNamespace, "cr1")
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
 			objects := []runtime.Object{crd, clusterRole, cr}
@@ -400,6 +444,50 @@ var _ = ginkgo.Describe("Work agent", func() {
 				return meta.IsStatusConditionPresentAndEqual(work.Status.Conditions, workapiv1.WorkApplied, metav1.ConditionTrue) &&
 					meta.IsStatusConditionPresentAndEqual(work.Status.Conditions, workapiv1.WorkAvailable, metav1.ConditionTrue)
 			}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+
+			// Upgrade crd/cr and check if cr resource is recreated.
+			// Get UID of cr resource at first.
+			guestbook, err := spokeDynamicClient.Resource(schema.GroupVersionResource{
+				Resource: "guestbooks",
+				Version:  "v1",
+				Group:    "my.domain",
+			}).Namespace(crNamespace).Get(context.Background(), "cr1", metav1.GetOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			currentUID := guestbook.GetUID()
+
+			// Upgrade crd/cr in the work
+			cr, err = newCr(upgradedGuestBookCRJson, crNamespace, "cr1")
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			objects = []runtime.Object{crd, clusterRole, cr}
+			work = newManifestWork(clusterName, workName, objects...)
+
+			// Update work
+			existingWork, err := hubWorkClient.WorkV1().ManifestWorks(clusterName).Get(context.Background(), work.Name, metav1.GetOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			work.ResourceVersion = existingWork.ResourceVersion
+			_, err = hubWorkClient.WorkV1().ManifestWorks(clusterName).Update(context.Background(), work, metav1.UpdateOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+
+			// check if v2 cr is applied
+			gomega.Eventually(func() error {
+				guestbook, err := spokeDynamicClient.Resource(schema.GroupVersionResource{
+					Resource: "guestbooks",
+					Version:  "v2",
+					Group:    "my.domain",
+				}).Namespace(crNamespace).Get(context.Background(), "cr1", metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+				// Check if the spec in the cr is updated
+				if !reflect.DeepEqual(guestbook.Object["spec"], cr.Object["spec"]) {
+					return fmt.Errorf("Expect CR spec is updated, expected: %v, actual %v", cr.Object["spec"], guestbook.Object["spec"])
+				}
+				// Ensure that the CR is updated rather than recreated
+				if currentUID != guestbook.GetUID() {
+					return fmt.Errorf("Expect UID to be the same, expected: %q, actual %q", currentUID, guestbook.GetUID())
+				}
+				return nil
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
 		})
 	})
 })
@@ -472,13 +560,13 @@ func haveManifestCondition(conditions []workapiv1.ManifestCondition, expectedTyp
 	return true
 }
 
-func newCrd() (crd *unstructured.Unstructured, err error) {
-	crd, err = loadResourceFromJSON(guestbookCrdJson)
+func newCrd(content string) (crd *unstructured.Unstructured, err error) {
+	crd, err = loadResourceFromJSON(content)
 	return crd, err
 }
 
-func newCr(namespace, name string) (cr *unstructured.Unstructured, err error) {
-	cr, err = loadResourceFromJSON(guestbookCrJson)
+func newCr(content, namespace, name string) (cr *unstructured.Unstructured, err error) {
+	cr, err = loadResourceFromJSON(content)
 	if err != nil {
 		return nil, err
 	}

test/integration/work_test.go

@@ -541,7 +541,7 @@ var _ = ginkgo.Describe("ManifestWork", func() {
 		ginkgo.It("should delete applied manifest work if it is orphan", func() {
 			appliedManifestWork := &workapiv1.AppliedManifestWork{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: fmt.Sprintf("%s-fakeworrk", hubHash),
+					Name: fmt.Sprintf("%s-fakework", hubHash),
 				},
 				Spec: workapiv1.AppliedManifestWorkSpec{
 					HubHash:          hubHash,
@@ -554,4 +554,218 @@ var _ = ginkgo.Describe("ManifestWork", func() {
 			util.AssertAppliedManifestWorkDeleted(appliedManifestWork.Name, spokeWorkClient, eventuallyTimeout, eventuallyInterval)
 		})
 	})
+
+	ginkgo.Context("Resource sharing and adoption between manifestworks", func() {
+		var anotherWork *workapiv1.ManifestWork
+		var anotherAppliedManifestWorkName string
+		ginkgo.BeforeEach(func() {
+			manifests = []workapiv1.Manifest{
+				util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm1", map[string]string{"a": "b"}, []string{})),
+				util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm2", map[string]string{"c": "d"}, []string{})),
+			}
+		})
+
+		ginkgo.JustBeforeEach(func() {
+			// Create another manifestworks with one shared resource.
+			// TODO We might not want the sharing in this cases, since the content of a resource in two manifestworks
+			// can be different.
+			anotherWork = util.NewManifestWork(o.SpokeClusterName, "sharing-resource-work", []workapiv1.Manifest{manifests[0]})
+			anotherWork, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Create(context.Background(), anotherWork, metav1.CreateOptions{})
+			anotherAppliedManifestWorkName = fmt.Sprintf("%s-%s", hubHash, anotherWork.Name)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+		})
+
+		ginkgo.It("shared resource between the manifestwork should be recreated when one manifestwork is deleted", func() {
+			// Ensure two manifestworks are all applied
+			util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+
+			// ensure configmap exists and get its uid
+			util.AssertExistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
+			curentConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			currentUID := curentConfigMap.UID
+
+			// Ensure that uid recorded in the appliedmanifestwork and anotherappliedmanifestwork is correct.
+			gomega.Eventually(func() error {
+				appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range appliedManifestWork.Status.AppliedResources {
+					if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) {
+						return nil
+					}
+				}
+
+				return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match")
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			gomega.Eventually(func() error {
+				anotherappliedmanifestwork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range anotherappliedmanifestwork.Status.AppliedResources {
+					if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) {
+						return nil
+					}
+				}
+
+				return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match")
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// Delete one manifestwork
+			err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Delete(context.Background(), work.Name, metav1.DeleteOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+
+			// Ensure the configmap is recreated and tracked by anotherappliedmanifestwork.
+			gomega.Eventually(func() error {
+				recreatedConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				if currentUID == recreatedConfigMap.UID {
+					return fmt.Errorf("UID should not be equal")
+				}
+
+				anotherappliedmanifestwork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range anotherappliedmanifestwork.Status.AppliedResources {
+					if appliedResource.Name != "cm1" {
+						return fmt.Errorf("Resource Name should be cm1")
+					}
+
+					if appliedResource.UID == string(currentUID) {
+						return fmt.Errorf("UID should not be equal")
+					}
+				}
+
+				return nil
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// Ensure the appliedmanifestwork of deleted manifestwork is removed so it won't try to delete shared resource
+			gomega.Eventually(func() bool {
+				_, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{})
+				if errors.IsNotFound(err) {
+					return true
+				}
+				return false
+			}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+
+		})
+
+		ginkgo.It("shared resource between the manifestwork should be recreated when the shared resource is removed from one manifestwork", func() {
+			// Ensure two manifestworks are all applied
+			util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkApplied), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+			util.AssertWorkCondition(anotherWork.Namespace, anotherWork.Name, hubWorkClient, string(workapiv1.WorkAvailable), metav1.ConditionTrue,
+				[]metav1.ConditionStatus{metav1.ConditionTrue}, eventuallyTimeout, eventuallyInterval)
+
+			// ensure configmap exists and get its uid
+			util.AssertExistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
+			curentConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			currentUID := curentConfigMap.UID
+
+			// Ensure that uid recorded in the appliedmanifestwork and anotherappliedmanifestwork is correct.
+			gomega.Eventually(func() error {
+				appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range appliedManifestWork.Status.AppliedResources {
+					if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) {
+						return nil
+					}
+				}
+
+				return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match")
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			gomega.Eventually(func() error {
+				anotherAppliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range anotherAppliedManifestWork.Status.AppliedResources {
+					if appliedResource.Name == "cm1" && appliedResource.UID == string(currentUID) {
+						return nil
+					}
+				}
+
+				return fmt.Errorf("Resource name or uid in appliedmanifestwork does not match")
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// Update one manifestwork to remove the shared resource
+			work, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Get(context.Background(), work.Name, metav1.GetOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			work.Spec.Workload.Manifests = []workapiv1.Manifest{manifests[1]}
+			_, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Update(context.Background(), work, metav1.UpdateOptions{})
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+
+			// Ensure the configmap is recreated and tracked by anotherappliedmanifestwork
+			gomega.Eventually(func() error {
+				recreatedConfigMap, err := spokeKubeClient.CoreV1().ConfigMaps(o.SpokeClusterName).Get(context.Background(), "cm1", metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				if currentUID == recreatedConfigMap.UID {
+					return fmt.Errorf("UID should not be equal")
+				}
+
+				anotherAppliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), anotherAppliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				for _, appliedResource := range anotherAppliedManifestWork.Status.AppliedResources {
+					if appliedResource.Name != "cm1" {
+						return fmt.Errorf("Resource Name should be cm1")
+					}
+
+					if appliedResource.UID == string(currentUID) {
+						return fmt.Errorf("UID should not be equal")
+					}
+				}
+
+				return nil
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// ensure the resource is not tracked by the appliedmanifestwork.
+			gomega.Eventually(func() bool {
+				appliedManifestWork, err := spokeWorkClient.WorkV1().AppliedManifestWorks().Get(context.Background(), appliedManifestWorkName, metav1.GetOptions{})
+				if err != nil {
+					return false
+				}
+
+				for _, appliedResource := range appliedManifestWork.Status.AppliedResources {
+					if appliedResource.Name == "cm1" {
+						return false
+					}
+				}
+
+				return true
+			}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+		})
+	})
 })