Skip to content

test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs - #3590

Merged
Aleksei Sviridkin (lexfrei) merged 2 commits into
mainfrom
test/pin-crd-keep-annotation
Aug 7, 2026
Merged

test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs#3590
Aleksei Sviridkin (lexfrei) merged 2 commits into
mainfrom
test/pin-crd-keep-annotation

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What this PR does

cert-manager-crds and etcd-operator-crds are the two packages this repository points at as the in-tree precedent for protecting CRDs with helm.sh/resource-policy: keep. Both carry the annotation, neither had a test, so it could be lost with nothing reporting the loss. This adds a suite to each, plus the test: target that hack/helm-unit-tests.sh needs before it will run one. No annotation is added and no rendered output changes: the point is to close the way of losing what is already there.

The two are exposed differently, so the suites say so instead of sharing one explanation. In etcd-operator-crds the annotation is ours. make update stamps it with an awk step anchored on the controller-gen.kubebuilder.io/version: line that the upstream CRDs carry. If a future controller-gen stops emitting that line, the step matches nothing, writes the CRDs out unstamped and exits zero.

In cert-manager-crds the annotation comes from the upstream template, emitted under {{- if .Values.crds.keep }}, and the value that switches it on is ours. That is two independent ways to lose it. The templates are also not maintained in that package: packages/system/cert-manager/Makefile deletes and refills its templates/ directory during its own make update, so a cert-manager bump rewrites a neighbouring package that whoever runs the bump has no reason to open.

Each suite is scoped with the wildcard templates/*.yaml and asserts across every document rendered under it, rather than listing CRD names or pinning a document count, so a CRD that upstream starts shipping is covered without editing the test. Each also asserts the kind, because the annotation assertion on its own would accept a non-CRD document that happened to carry it. Both charts render only CRDs today, checked with helm template.

The test: target is load-bearing rather than boilerplate. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target. With the target present, breaking a chart makes the whole run exit 1 and name the directory. With the fixtures present but the target removed, the same broken chart gives exit 0 and "All Helm unit tests passed", having executed the suite zero times.

Screenshots

Not a UI change.

Downstream repositories

I walked the trigger map in docs/agents/contributing.md against the file list of this diff. It adds two test files and two test: targets under packages/system/, and touches nothing else: no hack/package.mk, no app values.schema.json, no platform values, no ApplicationDefinition, no release asset names. The website's "developer tooling (the package Makefiles)" trigger is the one worth checking, so I read content/en/docs/next/development.md there: it documents make test in general terms and does not go stale from a package gaining that target.

Release note

NONE

Summary by CodeRabbit

  • Tests

    • Added automated validation for cert-manager and etcd operator CRD charts.
    • Confirmed rendered resources are correctly identified as CustomResourceDefinitions.
    • Verified CRDs retain the required keep resource policy during Helm operations.
  • Chores

    • Added convenient test commands for running Helm chart validation.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The cert-manager and etcd-operator CRD charts now provide test Make targets and Helm unit tests. The tests verify that rendered CRDs have the helm.sh/resource-policy: keep annotation.

Changes

CRD Helm test coverage

Layer / File(s) Summary
cert-manager CRD test checkpoint
packages/system/cert-manager-crds/Makefile, packages/system/cert-manager-crds/tests/resource_policy_test.yaml
Adds a test target and validates all rendered cert-manager CRDs as CustomResourceDefinition objects with the keep resource policy.
etcd-operator CRD test checkpoint
packages/system/etcd-operator-crds/Makefile, packages/system/etcd-operator-crds/tests/resource_policy_test.yaml
Adds a test target and validates all rendered etcd-operator CRDs as CustomResourceDefinition objects with the keep resource policy.

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

Possibly related PRs

  • cozystack/cozystack#3586: Adds the same CRD resource-policy annotation and Helm unit test pattern to other CRD charts.

Suggested reviewers: ivanhunters

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding tests that enforce the keep annotation on cert-manager and etcd-operator CRDs.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/pin-crd-keep-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.

@github-actions github-actions Bot added area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/M This PR changes 30-99 lines, ignoring generated files labels Aug 6, 2026
@lexfrei

Copy link
Copy Markdown
Contributor Author

Reopening to pick up the multus fix on main.

@kvaps Andrei Kvapil (kvaps) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The suites are honest — I mutated the tree to check, and every failure mode I could think of turns the run red rather than passing on an empty document set:

  • dropping keep from a single CRD (etcd-operator-crds/templates/etcdsnapshots.yaml) fails and names that template, so the asserts really do run per document rather than only against the first one;
  • crds.keep: false in cert-manager-crds/values.yaml fails across all six CRDs;
  • crds.enabled: false, where the chart renders nothing at all, fails with no manifest found — no vacuous pass;
  • moving the CRDs out of templates/*.yaml entirely fails the same way;
  • moving one CRD into templates/sub/ and unstamping it still fails, so the glob reaches nested paths and a future refill into a subdirectory stays covered;
  • adding a non-CRD document fails on the kind assert, which is the invariant the header comment describes.

No merge-order dependency on #3586: that PR covers four different packages and ships its own suites for them, this one covers the two that already carry the annotation on main. The suites pass on this branch's base with #3586 absent. There is a textual overlap with #3595 in etcd-operator-crds/Makefile, but in a different hunk — whichever lands second may want a trivial rebase, and the two are complementary rather than redundant: #3595 fails the vendoring, this suite guards the committed files.

The red Unit & controller tests check is unrelated to this diff. It fails at the bats step on the EXIT-trap freeze list (multus-install-cni-plugins.bats=12 is not in the frozen set), which #3584 fixed on main after this branch's base. In that same run the helm suites ran and passed — the log has Running tests in packages/system/cert-manager-crds, the same for etcd-operator-crds, and All Helm unit tests passed. A rebase should clear it.

Two nits, neither blocking:

  1. This suite and the ones in #3586 share a filename and a purpose but not a shape: there it is documentSelector with matchMany: true, named per-CRD cases and a hasDocuments count; here it is a template glob with an equal on kind. The divergence is defensible — the gateway-api chart genuinely renders non-CRD documents, these two do not — but if both land the tree carries two idioms for one job. Worth a line in one of them saying why, or converging on one.
  2. The kind assert encodes "this chart renders only CRDs" as a second invariant. That is deliberate and the comment says so; just note that when it trips, the failure reads as a kind mismatch rather than pointing at the annotation.

One observation beyond this diff, since it is the same argument this PR makes about the test: target being load-bearing. Four packages have tests/ fixtures and no test: target, so hack/helm-unit-tests.sh has never executed them: packages/apps/mongodb (7 suites, 94 tests), packages/system/gpu-operator (3 tests) and packages/system/monitoring-agents (2 tests) — all three pass locally today — plus packages/system/nfs-driver, whose tests/ holds pvc.yaml/sc.yaml rather than helm-unittest fixtures. Separately, packages/system/piraeus-operator-crds (4 CRDs) and packages/system/application-definition-crd (1 CRD) render CRDs from templates/ with neither the annotation nor a suite; they are not tenant addons, so outside #3586's stated scope, but the same uninstall cascade applies. Both look like follow-ups rather than anything for this PR.

The CRDs carry helm.sh/resource-policy: keep, which is what stops a
release uninstall from deleting them and taking every Certificate,
Issuer and Order in the cluster with it. Nothing asserted that.

The annotation is not local to this package on either side: it comes
from the upstream template under a conditional, the value that switches
it on is ours, and the templates are refilled from the vendored upstream
chart by packages/system/cert-manager on its own update. The assertion
reads the rendered output, so it does not depend on which of those
moves.

Scope by wildcard rather than a list of filenames, so a CRD that
upstream starts shipping is covered without editing the test, and assert
the kind alongside the annotation, since the annotation assertion on its
own accepts a non-CRD document that carries it.

Add the test: target too. hack/helm-unit-tests.sh runs a package's suite
only when its Makefile defines that target, so without it the fixtures
sit unread and the run reports success on a chart that has already lost
the annotation.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The CRDs carry helm.sh/resource-policy: keep, which is what stops a
release uninstall from deleting them and taking every EtcdCluster,
EtcdMember and EtcdSnapshot in the cluster with it. Nothing asserted
that.

The annotation is stamped by the awk step in `make update`, anchored on
the controller-gen version line the upstream CRDs carry. If that line
stops being emitted, the step matches nothing and exits zero.

Scope by wildcard rather than a list of filenames, so a CRD added to the
vendoring list is covered without editing the test, and assert the kind
alongside the annotation, since the annotation assertion on its own
accepts a non-CRD document that carries it.

Add the test: target too. hack/helm-unit-tests.sh runs a package's suite
only when its Makefile defines that target, so without it the fixtures
sit unread and the run reports success on a chart that has already lost
the annotation.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 7, 2026
## What this PR does

`make update` in `packages/system/etcd-operator-crds` vendors three CRDs
and stamps `helm.sh/resource-policy: keep` onto each with an awk step
anchored on the `controller-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, with `make update` leaving
`git status` showing nothing but this Makefile.

On `main` today 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-crd` and
`vertical-pod-autoscaler-crds`. There is nothing to guard there on
`main` yet, so they are deliberately out of this change and need the
same step once #3586 merges.

### Screenshots

Not a UI change.

### Downstream repositories

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

I walked the trigger map in `docs/agents/contributing.md` against the
file list of this diff, which is one package Makefile and nothing else.
`hack/package.mk` is untouched, so the `ccp` and `external-apps-example`
triggers 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.md` describes `make update` in general
terms and does not go stale from a package validating its own output.

### Release note

```release-note
NONE
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved CRD update validation to ensure generated definitions include
the required preservation annotation.
* Prevented incomplete or invalid updates from replacing existing CRD
templates.
  * Added automatic cleanup of temporary update files.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit cab44d5 into main Aug 7, 2026
14 of 15 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the test/pin-crd-keep-annotation branch August 7, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants