✨ Include metadata in WorkApplier cache to detect external changes

pkg/apis/work/v1/applier/workapplier_test.go

@@ -175,6 +175,38 @@ func TestWorkApplierWithTypedClient(t *testing.T) {
 	}
 	assertActions(t, fakeWorkClient.Actions(), "patch")
 
+	assertExternalMetadataChangeDetected := func(description string, tamper func(*workapiv1.ManifestWork)) {
+		obj, exists, getErr := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().GetByKey("test/test")
+		if getErr != nil || !exists {
+			t.Fatalf("%s: failed to get work from store: err=%v exists=%v", description, getErr, exists)
+		}
+
+		tamperedWork := obj.(*workapiv1.ManifestWork).DeepCopy()
+		tamper(tamperedWork)
+		if err := workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update(tamperedWork); err != nil {
+			t.Errorf("%s: failed to update work with err %v", description, err)
+		}
+
+		fakeWorkClient.ClearActions()
+		_, err = workApplier.Apply(context.TODO(), newWork)
+		if err != nil {
+			t.Errorf("%s: failed to apply work with err %v", description, err)
+		}
+		assertActions(t, fakeWorkClient.Actions(), "patch")
+	}
+
+	assertExternalMetadataChangeDetected("external label change", func(w *workapiv1.ManifestWork) {
+		w.Labels = map[string]string{"tampered": "true"}
+	})
+	assertExternalMetadataChangeDetected("external annotation change", func(w *workapiv1.ManifestWork) {
+		w.Annotations = map[string]string{"tampered": "true"}
+	})
+	assertExternalMetadataChangeDetected("external owner reference change", func(w *workapiv1.ManifestWork) {
+		w.OwnerReferences = []metav1.OwnerReference{
+			{APIVersion: "v1", Kind: "ConfigMap", Name: "rogue-owner", UID: "rogue"},
+		}
+	})
+
 	fakeWorkClient.ClearActions()
 	err = workApplier.Delete(context.TODO(), newWork.Namespace, newWork.Name)
 	if err != nil {

pkg/apis/work/v1/applier/workcache.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"sync"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog/v2"
 
 	workapiv1 "open-cluster-management.io/api/work/v1"
@@ -20,6 +21,7 @@ type workKey struct {
 type cachedResource struct {
 	resourceHash string
 	generation   int64
+	metadataHash string
 }
 
 type workCache struct {
@@ -48,6 +50,7 @@ func (w *workCache) updateCache(required, existing *workapiv1.ManifestWork) {
 	value := cachedResource{
 		resourceHash: hashOfResourceStruct(required),
 		generation:   existing.Generation,
+		metadataHash: hashOfMetadata(required),
 	}
 
 	w.cache[key] = value
@@ -79,12 +82,11 @@ func (w *workCache) safeToSkipApply(required, existing *workapiv1.ManifestWork)
 	generation := existing.Generation
 	w.mutex.RLock()
 	defer w.mutex.RUnlock()
-	var generationMatch, hashMatch bool
+	metadataHash := hashOfMetadata(existing)
+
 	if cached, exists := w.cache[cacheKey]; exists {
-		generationMatch = cached.generation == generation
-		hashMatch = cached.resourceHash == resourceHash
-		if generationMatch && hashMatch {
-			klog.V(4).Infof("found matching generation & manifest hash")
+		if cached.generation == generation && cached.resourceHash == resourceHash && cached.metadataHash == metadataHash {
+			klog.V(4).Infof("found matching generation, manifest hash & metadata hash")
 			return true
 		}
 	}
@@ -101,3 +103,16 @@ func hashOfResourceStruct(o interface{}) string {
 	rval := fmt.Sprintf("%x", h.Sum(nil))
 	return rval
 }
+
+func hashOfMetadata(work *workapiv1.ManifestWork) string {
+	meta := struct {
+		Labels          map[string]string
+		Annotations     map[string]string
+		OwnerReferences []metav1.OwnerReference
+	}{
+		Labels:          work.Labels,
+		Annotations:     work.Annotations,
+		OwnerReferences: work.OwnerReferences,
+	}
+	return hashOfResourceStruct(meta)
+}

pkg/apis/work/v1/applier/workcache_test.go

@@ -63,3 +63,51 @@ func TestCache(t *testing.T) {
 		t.Errorf("should update work if related cache is not found")
 	}
 }
+
+func TestCacheDetectsExternalMetadataChange(t *testing.T) {
+	cases := []struct {
+		name     string
+		required metav1.ObjectMeta
+		tamper   func(*workapiv1.ManifestWork)
+	}{
+		{
+			name:     "labels",
+			required: metav1.ObjectMeta{Name: "test", Namespace: "cluster1", Labels: map[string]string{"managed-by": "addon"}},
+			tamper:   func(w *workapiv1.ManifestWork) { w.Labels["managed-by"] = "tampered" },
+		},
+		{
+			name:     "annotations",
+			required: metav1.ObjectMeta{Name: "test", Namespace: "cluster1", Annotations: map[string]string{"config-hash": "abc123"}},
+			tamper:   func(w *workapiv1.ManifestWork) { w.Annotations["config-hash"] = "tampered" },
+		},
+		{
+			name: "owner references",
+			required: metav1.ObjectMeta{Name: "test", Namespace: "cluster1", OwnerReferences: []metav1.OwnerReference{
+				{APIVersion: "v1", Kind: "ConfigMap", Name: "owner", UID: "abc"},
+			}},
+			tamper: func(w *workapiv1.ManifestWork) { w.OwnerReferences[0].Name = "tampered" },
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			cache := newWorkCache()
+
+			requiredWork := &workapiv1.ManifestWork{ObjectMeta: c.required}
+			existingWork := requiredWork.DeepCopy()
+			existingWork.Generation = 1
+
+			cache.updateCache(requiredWork, existingWork)
+
+			if !cache.safeToSkipApply(requiredWork, existingWork) {
+				t.Errorf("should skip apply when nothing changed")
+			}
+
+			c.tamper(existingWork)
+
+			if cache.safeToSkipApply(requiredWork, existingWork) {
+				t.Errorf("should not skip apply when %s are externally modified", c.name)
+			}
+		})
+	}
+}