feat(apps): mark stateful-app storageClass fields as immutable - #2639
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CEL immutability validations (rule: self == oldSelf) for storageClass across CRD Go types, Helm values schemas, and ApplicationDefinition openAPISchema; refactors Application REST spec parsing into buildSpecSchema; adds tests and a docs page describing the contract. ChangesstorageClass immutability enforcement across CRDs, Helm charts, and ApplicationDefinitions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
e2676c8 to
2dd4407
Compare
2dd4407 to
5f71dae
Compare
4c2a7c3 to
ad99cbc
Compare
f419939 to
84c0ffd
Compare
84c0ffd to
25721d8
Compare
There was a problem hiding this comment.
Code Review
This pull request enforces the immutability of the storageClass field across multiple stateful applications by adding CEL validation rules to API types and Helm chart schemas. It also includes documentation updates and a refactor of the schema building logic in the application registry, accompanied by new unit tests. Feedback was provided regarding a violation of the repository style guide where errors were being discarded using blank identifiers during scheme initialization.
| _ = internalapiext.AddToScheme(scheme) | ||
| _ = apiextv1.AddToScheme(scheme) |
There was a problem hiding this comment.
Discarding errors with _ when adding types to the scheme violates the repository style guide. Although these calls are unlikely to fail in this context, they should be handled explicitly to ensure the scheme is correctly initialized before conversion.
| _ = internalapiext.AddToScheme(scheme) | |
| _ = apiextv1.AddToScheme(scheme) | |
| if err := internalapiext.AddToScheme(scheme); err != nil { | |
| return nil, fmt.Errorf("add internalapiext to scheme: %w", err) | |
| } | |
| if err := apiextv1.AddToScheme(scheme); err != nil { | |
| return nil, fmt.Errorf("add apiextv1 to scheme: %w", err) | |
| } |
References
- Handle errors explicitly. Discarding meaningful errors with
_is a bug. (link)
25721d8 to
06b6dc9
Compare
06b6dc9 to
440a059
Compare
There was a problem hiding this comment.
Code Review
This pull request enforces the immutability of the storageClass field across multiple stateful applications by introducing CEL validation rules in API types, Helm schemas, and application definitions. It also refactors the OpenAPI schema parsing logic and adds unit tests to ensure validation rules survive structural schema conversion. Review feedback highlighted a violation of the repository's error handling policy regarding discarded errors and noted an inconsistent validation message in the PostgreSQL configuration.
| _ = internalapiext.AddToScheme(scheme) | ||
| _ = apiextv1.AddToScheme(scheme) |
There was a problem hiding this comment.
Discarding errors from AddToScheme calls violates the repository style guide (Rule 88), which states that discarding meaningful errors with _ is a bug. Although these calls are unlikely to fail in practice, they should be handled explicitly to ensure the scheme is correctly initialized before conversion.
| _ = internalapiext.AddToScheme(scheme) | |
| _ = apiextv1.AddToScheme(scheme) | |
| if err := internalapiext.AddToScheme(scheme); err != nil { | |
| return nil, fmt.Errorf("add internal apiextensions to scheme: %w", err) | |
| } | |
| if err := apiextv1.AddToScheme(scheme); err != nil { | |
| return nil, fmt.Errorf("add v1 apiextensions to scheme: %w", err) | |
| } |
References
- Handle errors explicitly. Discarding meaningful errors with
_is a bug. (link)
| Size resource.Quantity `json:"size"` | ||
| // StorageClass used to store the data. | ||
| // +kubebuilder:default:="" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storageClass is immutable" |
There was a problem hiding this comment.
The validation message for postgresql is inconsistent with other applications in this PR (e.g., mariadb, clickhouse, harbor), which use "storageClass is immutable after creation". Maintaining a consistent message across all stateful apps improves the user experience and ensures uniform schema-aware validation feedback.
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storageClass is immutable" | |
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storageClass is immutable after creation" |
440a059 to
b09b44d
Compare
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/apps/postgres/values.schema.json`:
- Around line 115-120: Update the source annotation that generates the JSON
validation so the message matches other apps: locate the
"x-kubernetes-validations" entry that contains the rule "self == oldSelf" and
change its "message" value from "storageClass is immutable" to "storageClass is
immutable after creation" in packages/apps/postgres/values.yaml so the generated
values.schema.json uses the standardized, more descriptive message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f87c129b-350d-4c6e-b595-fa48b21bbf79
📒 Files selected for processing (83)
api/apps/v1alpha1/clickhouse/types.goapi/apps/v1alpha1/foundationdb/types.goapi/apps/v1alpha1/harbor/types.goapi/apps/v1alpha1/httpcache/types.goapi/apps/v1alpha1/kafka/types.goapi/apps/v1alpha1/kubernetes/types.goapi/apps/v1alpha1/mariadb/types.goapi/apps/v1alpha1/mongodb/types.goapi/apps/v1alpha1/nats/types.goapi/apps/v1alpha1/openbao/types.goapi/apps/v1alpha1/opensearch/types.goapi/apps/v1alpha1/postgresql/types.goapi/apps/v1alpha1/qdrant/types.goapi/apps/v1alpha1/rabbitmq/types.goapi/apps/v1alpha1/redis/types.goapi/apps/v1alpha1/vmdisk/types.godocs/storage-immutability.mdpackages/apps/clickhouse/README.mdpackages/apps/clickhouse/values.schema.jsonpackages/apps/clickhouse/values.yamlpackages/apps/foundationdb/README.mdpackages/apps/foundationdb/values.schema.jsonpackages/apps/foundationdb/values.yamlpackages/apps/harbor/README.mdpackages/apps/harbor/values.schema.jsonpackages/apps/harbor/values.yamlpackages/apps/http-cache/README.mdpackages/apps/http-cache/values.schema.jsonpackages/apps/http-cache/values.yamlpackages/apps/kafka/README.mdpackages/apps/kafka/values.schema.jsonpackages/apps/kafka/values.yamlpackages/apps/kubernetes/README.mdpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/apps/mariadb/README.mdpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/apps/mongodb/README.mdpackages/apps/mongodb/values.schema.jsonpackages/apps/mongodb/values.yamlpackages/apps/nats/README.mdpackages/apps/nats/values.schema.jsonpackages/apps/nats/values.yamlpackages/apps/openbao/README.mdpackages/apps/openbao/values.schema.jsonpackages/apps/openbao/values.yamlpackages/apps/opensearch/README.mdpackages/apps/opensearch/values.schema.jsonpackages/apps/opensearch/values.yamlpackages/apps/postgres/README.mdpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/apps/qdrant/README.mdpackages/apps/qdrant/values.schema.jsonpackages/apps/qdrant/values.yamlpackages/apps/rabbitmq/README.mdpackages/apps/rabbitmq/values.schema.jsonpackages/apps/rabbitmq/values.yamlpackages/apps/redis/README.mdpackages/apps/redis/values.schema.jsonpackages/apps/redis/values.yamlpackages/apps/vm-disk/README.mdpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-disk/values.yamlpackages/system/clickhouse-rd/cozyrds/clickhouse.yamlpackages/system/foundationdb-rd/cozyrds/foundationdb.yamlpackages/system/harbor-rd/cozyrds/harbor.yamlpackages/system/http-cache-rd/cozyrds/http-cache.yamlpackages/system/kafka-rd/cozyrds/kafka.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yamlpackages/system/mongodb-rd/cozyrds/mongodb.yamlpackages/system/nats-rd/cozyrds/nats.yamlpackages/system/openbao-rd/cozyrds/openbao.yamlpackages/system/opensearch-rd/cozyrds/opensearch.yamlpackages/system/postgres-rd/cozyrds/postgres.yamlpackages/system/qdrant-rd/cozyrds/qdrant.yamlpackages/system/rabbitmq-rd/cozyrds/rabbitmq.yamlpackages/system/redis-rd/cozyrds/redis.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yamlpkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_spec_schema_test.go
✅ Files skipped from review due to trivial changes (29)
- packages/apps/foundationdb/values.yaml
- packages/apps/harbor/values.yaml
- packages/apps/rabbitmq/values.yaml
- packages/apps/kafka/values.yaml
- packages/apps/http-cache/values.yaml
- packages/apps/mongodb/README.md
- api/apps/v1alpha1/harbor/types.go
- packages/apps/http-cache/README.md
- packages/apps/rabbitmq/README.md
- packages/apps/kafka/README.md
- packages/apps/opensearch/README.md
- packages/apps/vm-disk/README.md
- packages/apps/harbor/README.md
- packages/system/postgres-rd/cozyrds/postgres.yaml
- packages/apps/redis/README.md
- docs/storage-immutability.md
- packages/apps/postgres/README.md
- packages/apps/openbao/values.yaml
- packages/apps/clickhouse/README.md
- packages/apps/nats/README.md
- packages/apps/redis/values.yaml
- packages/apps/nats/values.yaml
- packages/apps/mariadb/values.yaml
- packages/apps/qdrant/README.md
- packages/apps/opensearch/values.yaml
- packages/apps/mongodb/values.yaml
- packages/apps/kubernetes/README.md
- packages/apps/foundationdb/README.md
- packages/apps/qdrant/values.yaml
v1.5.0 adds the @immutable annotation support (cozystack/cozyvalues-gen#24). Without the bump, pre-commit re-runs make generate with the v1.4.0 binary, silently strips any x-kubernetes-validations entries that downstream PRs add to values.schema.json / embedded openAPISchema, and reports drift against the committed artefacts. This is a no-op for the current main: no chart currently uses @immutable, so regeneration with v1.5.0 produces identical output to v1.4.0. Unblocks #2639 (apply @immutable to storageClass across stateful apps). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
## Summary Bump the `cozyvalues-gen` binary pinned in `.github/workflows/pre-commit.yml` from `v1.4.0` to `v1.5.0`. `v1.5.0` (cozystack/cozyvalues-gen#24) adds support for the `@immutable` annotation. Without this bump, pre-commit re-runs `make generate` with the old `v1.4.0` binary on every PR; the old binary silently ignores `## @immutable` directives and regenerates `values.schema.json` / embedded `openAPISchema` without the corresponding `x-kubernetes-validations` entries — which then shows up as committed-vs-regenerated drift on any PR that tries to use the new annotation. ## No-op for main today No chart on main currently uses `@immutable` (zero hits for `## @immutable` under `packages/apps/*/values.yaml`), so this bump regenerates byte-identical output. The change is purely lifting the floor for downstream PRs that will start using the annotation. ## Unblocks - #2639 — apply `@immutable` to `storageClass` across every stateful Cozystack app (currently fails pre-commit on the v1.4.0 pin). ## Release info - v1.5.0 release notes: https://github.com/cozystack/cozyvalues-gen/releases/tag/v1.5.0 - Tag signed by F20B7CCBF00A5CAE8F9F8B8C7988329FDF395282 (Aleksey Sviridkin) - Goreleaser uploaded `cozyvalues-gen-linux-amd64.tar.gz` (the artefact this workflow downloads) along with the rest of the OS/arch matrix. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated development tooling dependencies for build process improvements. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2730?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
18dd65d to
9a6313b
Compare
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM. Coverage is complete across all 16 stateful apps, the buildSpecSchema refactor is a good call, and the new tests pass. nodeGroups exclusion is correct and the v1.5.0 dependency is already satisfied on main.
Two non-blocking doc nits:
- The PR description says
kubernetes (top-level + nodeGroups[]), butnodeGroups[].storageClassis intentionally excluded. The "Dependencies" section is also stale — cozyvalues-gen v1.5.0 is already pinned on main and CI is green. Worth syncing the body to match.
| ## @field {string} instanceType="u1.medium" - Virtual machine instance type. | ||
| ## @field {quantity} diskSize="20Gi" - Persistent disk size for kubelet and containerd data. | ||
| ## @field {string} [storageClass] - StorageClass for worker node persistent disks. When empty, uses the management cluster default StorageClass (the one annotated storageclass.kubernetes.io/is-default-class: true). | ||
| ## @field {string} [storageClass] - StorageClass for worker node persistent disks. When empty, uses the management cluster default StorageClass (the one annotated storageclass.kubernetes.io/is-default-class: true). NOTE: deliberately NOT @immutable — the field is optional and undefaulted, so a strict `self == oldSelf` rule would block any future attempt to set it on an existing node group. |
There was a problem hiding this comment.
The description embeds the literal @immutable token ("deliberately NOT Immutable (@immutable) …"). v1.5.0 parses it correctly today (the generated schema has no rule on this field), but it's fragile against future parser changes. Rewording to "deliberately not marked immutable" sidesteps it.
Annotate storageClass as immutable across every stateful app whose chart exposes one — clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level only), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. Without coverage across the catalogue the UI would render storageClass read-only on some apps and editable on others — a worse mental model than uniform behaviour. The annotation lands at the schema layer (CEL XValidation rule 'self == oldSelf' on values.schema.json / types.go / embedded openAPISchema in cozyrds/<app>.yaml). The cozystack aggregated apiserver does not yet evaluate CEL on application-backed resources; that is tracked at #2657. The UI (cozystack/cozystack-ui#6) consumes the marker today. The PVC-pinning argument is identical for every chart: Kubernetes fixes a PVC's storageClassName at creation time, so editing this field on an existing resource never migrated data. The schema annotation makes that contract explicit. kubernetes.nodeGroups[name].storageClass is intentionally NOT annotated: the field is optional and undefaulted, so a strict 'self == oldSelf' rule would block any future attempt to set it on an existing node group. A richer ratcheting form is needed there — see docs/storage-immutability.md. A refactor in pkg/registry/apps/application/rest.go extracts the inline schema-build path into buildSpecSchema so it can be unit-tested. Three regression tests in rest_spec_schema_test.go pin the contract that x-kubernetes-validations survives the v1->internal->structural conversion (loaded from the actual MariaDB ApplicationDefinition fixture on disk, not a hand-edited mock). Each per-app README gains a one-line blockquote pointing to a new docs/storage-immutability.md that explains the contract and which consumers enforce it today. Requires cozystack/cozyvalues-gen with @immutable support — bump the pre-commit pin in .github/workflows/pre-commit.yml once the release exists. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
9a6313b to
5af5962
Compare
Merge the AI-generated v1.5.0 changelog with a fuller pass: - Add an "Upgrade Notes and Required Actions" section (K8s 1.33+, removal of upgrade.force, GPU permittedHostDevices ownership, MetalLB FRR-K8s / HTTPS metrics, automatic TLS on externally-published services). - Expand "Platform Components" with per-component upstream highlights and CVE references (Flux v2.8 CVE-2026-23990, MetalLB v0.16 CVE-2025-22874), plus the new frr-k8s subchart and kuberture package. - Scope the storageClass-immutability entry (#2639) to UI-only enforcement, with apiserver enforcement tracked in #2657. - Broaden Development/Testing/CI coverage and add the v1.4.x website docs backports (#565, #566, #567, #580). - Correct the loop-device LVM-filter attribution to talm#215 (the prior ansible-cozystack#51 reference was a dependency bump, not this change). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Summary
Annotate
storageClassas immutable across every stateful chart that exposes one: clickhouse, foundationdb, harbor, http-cache, kafka (Kafka + ZooKeeper), kubernetes (top-level + nodeGroups[]), mariadb, mongodb, nats, openbao, opensearch, postgres, qdrant, rabbitmq, redis, vm-disk. The PVC-pinning argument is identical for every chart — Kubernetes fixesstorageClassNameon each PVC at creation, so editing this field on an existing resource never migrated data; the schema annotation makes that contract explicit and lets schema-aware consumers refuse the edit up-front.Schema-aware consumers today: the Cozystack UI (cozystack/cozystack-ui#6) reads the rule from the chart's openAPISchema and renders the field read-only on edit forms.
This PR is schema-side only. The cozystack aggregated apiserver does not yet evaluate CEL rules in
openAPISchema(MariaDB/Postgres/ etc. are application-backed, not CRDs); tracking issue #2657 covers wiring CEL intopkg/registry/apps/application/rest.go::Update. Until that lands, an apiserver-levelkubectl patchthat changesstorageClassis still accepted — but the underlying StatefulSet PVCs do not move, so there's no data-corruption risk, only a confusing UX. The UI catches that case today; the apiserver gate closes it permanently.What changes
For each app in the list above:
packages/apps/<app>/values.yaml— gain a## @immutabledirective directly above (or below, for fields inside a@typedef) thestorageClassline.make generatein each package:api/apps/v1alpha1/<app>/types.go— kubebuilderXValidationmarker on the relevant field(s).packages/apps/<app>/values.schema.json—x-kubernetes-validations: [{rule: "self == oldSelf", message: …}]onstorageClass.packages/apps/<app>/README.md— regenerated parameter table.packages/system/<app>-rd/cozyrds/<app>.yaml— the embeddedopenAPISchemaon the ApplicationDefinition reflects the change.Refactor:
pkg/registry/apps/application/rest.gofactors the inline schema-build path into a newbuildSpecSchemahelper so it can be unit-tested without standing up a fake apiserver. Three regression tests inpkg/registry/apps/application/rest_spec_schema_test.gopin the contract thatx-kubernetes-validationssurvives the v1→internal→structural conversion (test loads the actual MariaDB ApplicationDefinition fixture on disk, asserts the rule reaches the structural schema). Empty-input and malformed-JSON paths are also pinned.No e2e admission test is added because the apiserver does not yet enforce the rule — see #2657.
No changelog entry: schema-side annotation is invisible at the apiserver layer today (UI-only consumer) and the release-cut PR will add the entry when admission enforcement lands.
Dependencies
Requires cozystack/cozyvalues-gen#24 to be merged and released, and the
cozyvalues-genbinary pin in.github/workflows/pre-commit.ymlline 50 bumped to the release that includes that PR. Pre-commit CI will fail until both happen — the stockv1.3.0binary silently ignores## @immutableand regenerates artefacts without the rule, producing a drift against what's committed.Once that ordering is satisfied, rebase this branch and the pre-commit job will be green.
Summary by CodeRabbit
New Features
Documentation
Tests