✨ Include metadata in WorkApplier cache to detect external changes

[mkolesnik]

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 and ManifestWorkEqual is never called.

This stores a hash of the desired work's metadata in the cache and compares it against the existing work's metadata on each apply, ensuring the cache invalidates when an external actor modifies owned metadata fields.

Fixes: #223

Summary by CodeRabbit

• Bug Fixes

  • Enhanced metadata drift detection to identify changes in labels, annotations, and owner references, ensuring patches are issued when external modifications occur.

• Tests

  • Added comprehensive test coverage for metadata drift detection across multiple change scenarios.

[coderabbitai]

Walkthrough

The PR extends the WorkApplier cache to detect external metadata modifications (labels, annotations, ownerReferences) that don't bump the generation field, solving the issue where the cache incorrectly skips re-applying work when metadata is tampered externally. It adds metadata hash tracking alongside existing spec hashing and validates the metadata hash in the skip-apply logic.

Changes

Metadata Drift Detection for WorkApplier Cache

Layer / File(s)	Summary
Metadata hash tracking contract and helper pkg/apis/work/v1/applier/workcache.go	Adds metav1 import, extends cachedResource with metadataHash field, and implements hashOfMetadata helper to compute hash from labels, annotations, and ownerReferences.
Cache metadata detection logic pkg/apis/work/v1/applier/workcache.go	Updates updateCache to compute and store metadataHash on cache entries, and modifies safeToSkipApply to require cached metadata hash to match current metadata hash before skipping applies.
Cache metadata change detection test pkg/apis/work/v1/applier/workcache_test.go	Unit test using table-driven approach to verify safeToSkipApply detects mutations to labels, annotations, and ownerReferences and correctly invalidates the cache.
Applier metadata drift integration test pkg/apis/work/v1/applier/workapplier_test.go	Integration test covering applier behavior when external actors modify ManifestWork metadata, asserting that Apply detects tampering and issues patches for label, annotation, and owner reference changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description explains the problem, solution, and links to the related issue, but lacks the recommended template structure with emoji/category prefix and explicit summary section.
Linked Issues check	✅ Passed	The code changes successfully address issue #223 by implementing metadata hashing in the cache and comparison logic to detect external metadata-only changes.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objective of detecting external metadata changes through cache invalidation; no unrelated modifications detected.
Title check	✅ Passed	The title 'Include metadata in WorkApplier cache to detect external changes' clearly summarizes the main change: adding metadata tracking to the WorkApplier cache to detect external modifications, which is directly supported by all three file changes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/apis/work/v1/applier/workapplier_test.go`:
- Around line 178-191: The helper assertExternalMetadataChangeDetected currently
builds tamperedWork from newWork.DeepCopy(), which can reintroduce a
stale/default Generation and make the "patch" assertion pass for the wrong
reason; instead, fetch the existing object from the informer store (via
workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Get/ByKey
for the same work), DeepCopy that informer object into tamperedWork, then apply
the tamper mutation so the live Generation/metadata from the informer is
preserved before calling Update and running workApplier.Apply; keep references
to assertExternalMetadataChangeDetected, newWork, tamperedWork,
workInformerFactory.Work().V1().ManifestWorks().Informer().GetStore().Update,
workApplier.Apply and fakeWorkClient.Actions when making this change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e726a16-1b50-4445-92bd-cad80f59e718

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbb933 and f95cb17.

📒 Files selected for processing (3)

• pkg/apis/work/v1/applier/workapplier_test.go
• pkg/apis/work/v1/applier/workcache.go
• pkg/apis/work/v1/applier/workcache_test.go

[qiujian16]

I think resourceHash has included metadata section already. Are you trying to detect if label/annotation of the exisiting mw is changed? If so, I would consider an annotatil/label merge when apply rather than replace.

[mkolesnik]

Thanks for the quick review!

You're right that resourceHash hashes the desired work, so it catches when we change our labels.
The problem is it doesn't catch when someone else changes labels on the existing work (see #223 which I reported).
When that happens, our desired hash stays the same and generation doesn't bump, so the cache skips and the external change is never corrected.

Re merge vs replace, that's a good point and worth discussing separately.
This PR just makes the cache consistent with what the applier already does when the cache misses.

[mikeshng]

@coderabbitai review

[mikeshng]

/lgtm

/assign @tesshuflower

PTAL as well. Thanks!

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikeshng, mkolesnik
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment