Include metadata in WorkApplier cache to detect external changes

The WorkApplier cache uses the desired work hash and the existing work's
generation to skip redundant updates. However, metadata-only changes
(labels, annotations, owner references) do not bump generation, so the
cache misses external modifications to these fields.

Store a hash of the desired work's metadata (labels, annotations, owner
references) in the cache and compare it against the existing work's
metadata on each apply. This ensures the cache invalidates when an
external actor modifies metadata fields that the applier owns.

Fixes: #223

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mike Kolesnik <mkolesni@redhat.com>

Author: mkolesnik

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)
+			}
+		})
+	}
+}