Skip to content

fix(sanitize): strip the kcp.io/cluster annotation from Git writes#227

Merged
sunib merged 1 commit into
mainfrom
fix/sanitize-kcp-cluster-annotation
Jul 11, 2026
Merged

fix(sanitize): strip the kcp.io/cluster annotation from Git writes#227
sunib merged 1 commit into
mainfrom
fix/sanitize-kcp-cluster-annotation

Conversation

@sunib

@sunib sunib commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Why

kcp stamps kcp.io/cluster on every object it serves, naming the logical cluster the object was read from. That makes it an address, not intent: it describes where a copy of the object happened to live, not anything the user asked for.

Committed to Git it is actively wrong, and in the way that is hardest to notice — the manifest is pinned to one workspace, and then travels with that pin into every other workspace it is applied to.

What

One key added to the operational-annotation set in internal/sanitize/types.go:

switch key {
case "argocd.argoproj.io/tracking-id", "argocd.argoproj.io/installation-id", "kcp.io/cluster":
    return true
}

Why it is an exact key and not a kcp.io/ prefix strip

Deliberately, and for the same reason the Argo CD keys next to it are exact.

A prefix strip is the tempting shape and the wrong one: it assumes every sibling key under the vendor prefix is bookkeeping too. That assumption is what the existing comment on isOperationalAnnotation already warns about for argocd.argoproj.io/ — a blanket strip there would silently drop sync-wave, sync-options, hook and other genuine user intent. Nothing tells us kcp.io/ is safer, so it gets the same treatment.

Two tests pin the asymmetry rather than leaving it to the comment:

  • remove kcp logical-cluster annotationkcp.io/cluster is stripped
  • keep other kcp annotationskcp.io/path survives

If a second kcp key ever turns out to be bookkeeping, it should be added by name, and its own test should say so.

Scope

Annotations only — kcp carries this as an annotation, so isOperationalLabel is untouched. No behavior change for any object that does not carry the key.

docs/bi-directional.md's stripped-metadata table is a Flux-vs-Argo comparison, so kcp has no row there; the rationale lives in the godoc on isOperationalAnnotation, where the sibling reasoning already is.

Validation

task fmt ✅ · task vet ✅ · task lint ✅ (0 issues) · task test ✅ (coverage 75.4%, within the 0.5% tolerance of the 75.5% baseline — no baseline change) · task test-e2e ✅ (54 passed, 0 failed, 10 skipped — the skips are the opt-in bi-directional corner)

🤖 Generated with Claude Code

kcp stamps `kcp.io/cluster` on every object to name the logical cluster the
object was read from. That is an address, not user intent: committed to Git it
pins a manifest to one workspace and then travels with it into every other.

Matched as an exact key, beside the Argo CD tracking annotations, rather than as
a `kcp.io/` prefix strip — sibling keys under that prefix are not assumed to be
bookkeeping. A test pins that asymmetry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The sanitizer now removes the exact kcp.io/cluster annotation while preserving other kcp.io/* annotations. Tests cover both behaviors and the all-operational-annotations case.

Changes

KCP annotation sanitization

Layer / File(s) Summary
Annotation cleanup and validation
internal/sanitize/types.go, internal/sanitize/types_test.go
kcp.io/cluster is treated as operational bookkeeping and removed exactly, while sibling kcp.io/* annotations remain preserved; tests verify both cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is well written but does not follow the repository's required template sections or checklists. Reformat the PR body to match the template: Description, Type of Change, Testing, Checklist, Related Issues, Screenshots, and Additional Notes.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: stripping the exact kcp.io/cluster annotation from Git writes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sanitize-kcp-cluster-annotation

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

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sunib
sunib merged commit d9b6d9d into main Jul 11, 2026
32 of 33 checks passed
@sunib
sunib deleted the fix/sanitize-kcp-cluster-annotation branch July 11, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant