fix(etcd): fail the CRD vendoring when the keep stamp misses - #3595
Conversation
`make update` stamps helm.sh/resource-policy: keep onto each vendored CRD with an awk step anchored on the controller-gen version line, which upstream owns. If upstream stops emitting that line the step matches nothing, writes the CRD out unstamped and exits zero, so the release quietly loses the protection that keeps a chart uninstall from deleting the CRDs and every EtcdCluster, EtcdMember and EtcdSnapshot under them. Count the annotations the step produced and require exactly one per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Stage every CRD in a temporary directory and replace templates/ only once all of them are counted, so a failure on any one leaves the vendored files exactly as they were instead of wiping the directory and refilling part of the set. Verified by substituting the upstream response: a CRD with no controller-gen line fails with "stamped 0 keep annotations, expected 1", one carrying the line twice fails with 2, a failure on the second CRD leaves all three committed files untouched, and the unmodified upstream still vendors byte-identical output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
📝 WalkthroughWalkthroughThe CRD update target now stages generated files, validates the required ChangesCRD update workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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 `@packages/system/etcd-operator-crds/Makefile`:
- Around line 13-16: Update the Makefile replacement flow around the validation
loop and the templates update commands so the complete generated CRD set is
first staged in a sibling directory. Swap the staged directory into place only
after all files are prepared, retaining the existing templates directory until
installation succeeds and restoring it if the swap fails; avoid deleting the
current directory before the replacement is safely installed.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e57c5f34-2aeb-4ead-90ce-f9df7556a54f
📒 Files selected for processing (1)
packages/system/etcd-operator-crds/Makefile
| # The stamp is anchored on a line upstream controls, so count the result: one | ||
| # annotation per CRD, or the update fails. templates/ is replaced only after | ||
| # every CRD has been counted, so a failure leaves the vendored files as they | ||
| # were. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the templates/ replacement transactional.
The validation loop preserves the existing files until Line [26]. However, rm -rf templates deletes the current CRDs before the mv loop at Line [27] completes. An interruption, permission error, disk-full condition, or cross-filesystem move can leave templates/ missing or partially populated. This breaks the hack/package.mk show, diff, and apply inputs and makes the guarantee in Lines [13-16] incomplete.
Stage the complete replacement in a sibling directory, then perform a rollback-safe swap. Keep the old directory until the new directory is installed successfully.
Also applies to: 26-27
🤖 Prompt for 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.
In `@packages/system/etcd-operator-crds/Makefile` around lines 13 - 16, Update the
Makefile replacement flow around the validation loop and the templates update
commands so the complete generated CRD set is first staged in a sibling
directory. Swap the staged directory into place only after all files are
prepared, retaining the existing templates directory until installation succeeds
and restoring it if the swap fails; avoid deleting the current directory before
the replacement is safely installed.
|
Reopening to pick up the multus fix on main. |
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. The fail-open on merge-base is confirmed against the code, and the fix (count + strict -eq 1) closes all three unhappy paths fail-closed. Worth noting the || true here is not a fail-open: the decision is strict equality, not "empty == clean", so zero matches, an empty value, and a grep read error all fail closed. Atomicity is improved — staging in mktemp -d, replacing templates/ only after every CRD passes.
Non-blocking notes:
- No automated regression test for the guard itself (manual reproduction + #3590 covers the symptom on a different layer, not yet merged).
- The final templates/ swap isn't fully atomic (narrow window between rm -rf and the last mv); still strictly better than the old recipe.
What this PR does
make updateinpackages/system/etcd-operator-crdsvendors three CRDs and stampshelm.sh/resource-policy: keeponto each with an awk step anchored on thecontroller-gen.kubebuilder.io/version:line. That line is upstream's to emit. If a future controller-gen stops writing it, the awk matches nothing, writes the CRD out unstamped and exits zero, so the regeneration reports success and the CRDs land without the annotation that keeps a chart uninstall from deleting them, and with them every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster.The step now counts what it produced and requires exactly one annotation per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Every CRD is staged in a temporary directory and
templates/is replaced only after all of them have been counted, so a failure on any one leaves the vendored files exactly as they were rather than wiping the directory and refilling part of the set.I checked it by substituting the upstream response, so what ran was the recipe that ships rather than a copy of its logic: a CRD with no controller-gen line fails with
stamped 0 keep annotations, expected 1, a CRD carrying that line twice fails with 2, a failure on the second of the three leaves all three committed files untouched, and the real upstream still vendors byte-identical output, withmake updateleavinggit statusshowing nothing but this Makefile.On
maintoday this is the only thing guarding those three CRDs. A helm-unittest suite that fails when one of them renders without the annotation is in #3590, which is open and not merged. Once that lands the two catch the same loss at different moments: the suite on a test run, this step in front of whoever ran the regeneration.The same stamping shape exists in three more packages, but only on the branch of #3586, which adds it to
gateway-api-crds,vsnap-crdandvertical-pod-autoscaler-crds. There is nothing to guard there onmainyet, so they are deliberately out of this change and need the same step once #3586 merges.Screenshots
Not a UI change.
Downstream repositories
I walked the trigger map in
docs/agents/contributing.mdagainst the file list of this diff, which is one package Makefile and nothing else.hack/package.mkis untouched, so theccpandexternal-apps-exampletriggers that key on it do not fire, and the website's "developer tooling (the package Makefiles)" line is the only near miss:content/en/docs/next/development.mddescribesmake updatein general terms and does not go stale from a package validating its own output.Release note
Summary by CodeRabbit