Repository navigation
Conversation
…pplied in place Add a validating admission webhook (update.cacheruntime.fluid.io) that rejects a CacheRuntime UPDATE changing any spec field other than master/worker replicas, runtimeVersion and resources. Such fields are only read when the component is created, so a change was silently ignored or, for tieredStore, half-applied (ConfigMap quota updated while the worker tmpfs sizeLimit kept its creation-time value). - Only spec is compared; metadata/status writes and the scale subresource are never checked. - Add the ValidatingWebhookConfiguration to the chart with a configurable failurePolicy (webhook.cacheRuntimeValidation.failurePolicy, default Fail) and RBAC for the webhook to patch it. - Extend the webhook controller and certificate builder to keep the caBundle of the validating webhook configuration in sync. - Add unit tests, a curvine e2e check, and update the en/zh docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: btxu-db <btxu-db@outlook.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6243 +/- ##
==========================================
+ Coverage 65.81% 65.90% +0.08%
==========================================
Files 486 489 +3
Lines 34454 34604 +150
==========================================
+ Hits 22677 22806 +129
- Misses 10024 10044 +20
- Partials 1753 1754 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Validation permits master/worker imagePullPolicy updates that are silently ignored while blocking client.cleanPolicy changes that already take effect dynamically.
2 open issues
What changed in this PR
Adds CacheRuntime update validation to Fluid’s admission webhook, rejecting changes to fields the controller cannot apply in place rather than silently accepting them.
Changes:
- Adds semantic spec comparison, admission handling, and validation tests.
- Extends certificate injection, configuration watches, and Helm permissions to support the validating webhook.
- Documents update restrictions and failure-policy behavior, with end-to-end coverage.
| File | Description |
|---|---|
test/gha-e2e/curvine/test.sh |
Checks immutable-field rejection and metadata updates. |
pkg/webhook/handler/validating/webhook.go |
Maps the validation endpoint to its handler. |
pkg/webhook/handler/validating/cacheruntime/spec_update.go |
Defines immutable-field validation. |
pkg/webhook/handler/validating/cacheruntime/spec_update_test.go |
Tests field restrictions and semantic equality. |
pkg/webhook/handler/validating/cacheruntime/handler.go |
Handles CacheRuntime update admission requests. |
pkg/webhook/handler/validating/cacheruntime/handler_test.go |
Tests admission responses and request exemptions. |
pkg/webhook/handler/validating/cacheruntime/cacheruntime_suite_test.go |
Registers the validation test suite. |
pkg/webhook/handler/register.go |
Enables validating handler registration. |
pkg/webhook/certificate_builder.go |
Adds validating webhook CA injection. |
pkg/webhook/certificate_builder_test.go |
Tests CA injection and configuration isolation. |
pkg/ctrl/watch/validatingwebhookconfiguration.go |
Filters validating configuration events by name. |
pkg/ctrl/watch/validatingwebhookconfiguration_test.go |
Tests configuration event filtering. |
pkg/ctrl/watch/manager.go |
Registers the validating configuration watch. |
pkg/controllers/v1alpha1/webhook/webhook_controller.go |
Routes CA reconciliation by configuration name. |
pkg/controllers/v1alpha1/webhook/webhook_controller_test.go |
Tests reconciliation routing and retries. |
pkg/common/webhook.go |
Defines configuration and endpoint names. |
docs/zh/samples/cacheruntime/cacheruntime_spec_update.md |
Documents restrictions and availability behavior in Chinese. |
docs/en/samples/cacheruntime/cacheruntime_spec_update.md |
Documents restrictions and availability behavior in English. |
cmd/webhook/app/webhook.go |
Restricts the validating configuration cache by name. |
charts/fluid/fluid/values.yaml |
Adds the validation failure-policy setting. |
charts/fluid/fluid/templates/webhook/webhookconfiguration.yaml |
Installs the validating webhook configuration. |
charts/fluid/fluid/templates/role/webhook/rabc.yaml |
Grants name-scoped configuration access. |
Review effort: Balanced
| errs = append(errs, requireUnchanged(client.Child("tieredStore"), oldSpec.Client.TieredStore, newSpec.Client.TieredStore)...) | ||
| errs = append(errs, requireUnchanged(client.Child("cleanPolicy"), oldSpec.Client.CleanPolicy, newSpec.Client.CleanPolicy)...) |
| if !syncedInPlace { | ||
| errs = append(errs, requireUnchanged(path.Child("runtimeVersion"), oldSpec.RuntimeVersion, newSpec.RuntimeVersion)...) | ||
| errs = append(errs, requireUnchanged(path.Child("resources"), oldSpec.Resources, newSpec.Resources)...) | ||
| } |
The *.out artifacts are gitignored; force-added as review evidence. Co-Authored-By: Claude Code <noreply@anthropic.com>
cheyang
left a comment
There was a problem hiding this comment.
The premise is real — on base, non-synced CacheRuntime spec changes are silently ignored and tieredStore updates half-apply — and the guard is carefully built (semantic equality, subresource exclusion, name-filtered watch). But two PR-introduced issues block merge.
1. The webhook denies the controller's own metadata writes for client-less CacheRuntimes (stored form without spec.client, e.g. the mooncake e2e fixture). Structural defaulting doesn't materialize absent parent objects, but the typed client always serializes client:{}, so the API server defaults cleanPolicy: OnRuntimeDeleted onto the update body only — a phantom immutable-field diff. The controller's finalizer/owner-reference writes are then rejected forever: datasets stay NotBound and deletion hangs. This is exactly what fails in this PR's kind-e2e on all five K8s versions, and both of us reproduced it under envtest with a real API server (control and guard subtests green, a demo fix flips it green). Please fix it generally — normalize CRD defaults before comparing (or exempt cleanPolicy, which CSI reads dynamically via NodeUnstageVolume) — not as a phantom-diff special case, and land the stored-form/defaulting regression test with it; the harnesses have runnable examples.
2. config/webhook/manifests.yaml was not regenerated after adding the +kubebuilder:webhook marker, so the required lint gate fails. make update-crd and commit.
Meaningful disagreement in the debate was minor and resolved in cross-critique (one severity label; one e2e-coverage claim narrowed rather than upheld). Reviewer C (Copilot) flagged two items, both independently verified by us: cleanPolicy is a live CSI knob (widens the F1 fix space) and imagePullPolicy is admitted-but-never-synced.
Next round
[MUST] F1: make the webhook admit metadata-only updates of client-less CacheRuntimes via a general CRD-default normalization (or cleanPolicy exemption); add the stored-form/defaulting regression test; mooncake e2e green on all five K8s versions.
[MUST] F2: run make update-crd and commit config/webhook/manifests.yaml; lint gate green.
[SHOULD] F3: imagePullPolicy — make it immutable (keep image/tag updatable) or actually propagate it in updateImage; invert the canaries.
[SHOULD] F4: state the deletion-hang consequence of failurePolicy: Fail (incl. the helm-upgrade 404 window) in cacheruntime_spec_update.md; consider Ignore or a controller-SA exemption.
[SHOULD] F5: document the supported upgrade ordering for the new VWC RBAC watch, or tolerate a missing grant.
[SHOULD] F7: add a reflection-based completeness tripwire (or a leaf-granular fail-closed comparison) so future spec fields can't silently reopen the ignored-update class.
[SHOULD] M2: add workload-sync e2e assertions that run on multi-node clusters (currently skipped in single-node kind CI).
[Filed] none.
Non-blocking observations (6) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/webhook/handler/validating/cacheruntime/spec_update.go:75 [minor] — runtimeVersion.imagePullPolicy is admitted by the webhook but never synced — the exact no-op-update class this PR exists to reject (one subfield)
F3 (Claude's phase-1 catch, missed by Codex in phase 1, independently verified by Codex in cross-critique, and independently flagged by Reviewer C as C4575767913): validateComponentCommonSpecUpdate treats the whole master/worker runtimeVersion as updatable for synced components, but AdvancedStatefulSetManager.updateImage (pkg/ddc/cache/component/advanced_statefulset_manager.go:279) patches only container.Image = image:tag — the ImagePullPolicy in the ASTS pod template keeps its creation-time value, and the code comment at :197 claiming ImagePullPolicy is in-place supported is wrong (pre-existing on base). So a spec.master.runtimeVersion.imagePullPolicy: Never update is admitted and silently ignored: the PR's own contract ('only fields that take effect in place may change') is inaccurate for this subfield. Attribution note (Codex): the underlying non-sync is pre-existing (identical updateImage at base); the PR-introduced part is the new admission decision that over-claims the contract. Fix: either make imagePullPolicy immutable (Reviewer C's suggestion, keeping image/tag updatable) or actually propagate it in updateImage; then invert the canaries. - charts/fluid/fluid/values.yaml:372 [minor] — failurePolicy=Fail fail-closes controller finalizer writes during webhook outages and the helm-upgrade window — CacheRuntime deletion hangs
F4 (consensus, both reviewers, minor): with failurePolicy Fail (chart default, values.yaml:372; rendered at webhookconfiguration.yaml:72, no namespace/objectSelector so cluster-wide), every CacheRuntime UPDATE is rejected while the webhook is unreachable — including the controller's finalizer add/remove, which are main-resource Updates (pkg/controllers/runtime_controller.go:243/330/354) — so CacheRuntime creation AND deletion stall until the webhook recovers; DELETE itself is not matched but finalizer removal is an UPDATE. Beyond the documented install window, two windows matter (Claude's added helm-upgrade window verified sound by Codex): webhook pod crash/rollout, and helm upgrade where the new VWC becomes active while old webhook pods 404 the new path (a 404 counts as fail-closed under Fail). Folded in (Q1): whether Fail is the right default for a UX-validation webhook — the handler only rejects silent no-ops, not an integrity requirement; Ignore is one values.yaml switch away, and exempting the controller SA would moot the controller-written case. Ask: state the deletion-hang consequence explicitly in cacheruntime_spec_update.md so operators choose knowingly. - cmd/webhook/app/webhook.go:136 [minor] — Upgrade skew: new webhook binary with old chart RBAC cannot list/watch ValidatingWebhookConfiguration — whole serving path (incl. the Fail mutating webhook) degrades
F5 (raised as a question by Codex; every code link upgraded from hypothesis to code-path verified by Claude in cross-critique): the new source.Kind watch and the cache ByObject entry make the webhook process list/watch ValidatingWebhookConfiguration at manager start (cmd/webhook/app/webhook.go:136-138, pkg/ctrl/watch/manager.go:260-271); the RBAC grant is new in this PR (charts/fluid/fluid/templates/role/webhook/rabc.yaml, commit 98d5eac), so an old chart has no such grant — the informer's list/watch returns 403 and can never sync. Under controller-runtime v0.17.5, CacheSyncTimeout (2 min) fails Controller.Start; mgr.Start propagates; webhook.go:182-185 logs 'start webhook handler failed' and os.Exit(1) — the whole process, including the mutating webhook (itself failurePolicy=Fail for pod injection), crash-loops for the duration of the skew. Helm ships RBAC in the same release so only an image-only upgrade/rollback is exposed. Policy question for maintainers: is image-only skew a supported upgrade ordering? Cheap mitigations: a chart/upgrade-docs note, or tolerating a missing RBAC grant instead of hard-failing cache sync. - pkg/webhook/handler/validating/cacheruntime/spec_update.go:45 [minor] — Immutable-field list is fail-open: future CacheRuntimeSpec fields silently become updatable again
F7 (Codex's F3 merged with Claude's O1 — same finding, same fix, trivial severity delta between nit and minor): ValidateSpecUpdate enumerates immutable fields one by one; any field later added to CacheRuntimeSpec is updatable by default unless its author also edits this function — silently re-opening the exact bug class this webhook closes (the author discloses this in 'Special notes'). Preferred shape (Claude's critique of Codex's sketch): the low-risk variant is a reflection-based tripwire test that walks CacheRuntimeSpec and the component specs and fails on any leaf not classified as updatable-or-immutable. If the fail-closed rewrite is taken instead (copy new spec, overwrite updatable fields from old, semantic-DeepEqual the whole spec, report differing paths), it must classify at leaf granularity — overwriting whole runtimeVersion inherits the F3 imagePullPolicy hole, overwriting nothing breaks image updates — and it centralizes the F1 class of phantom diffs, so the defaulting normalization is still required. - pkg/webhook/handler/validating/cacheruntime/handler_test.go:58 [minor] — Handler tests never model the stored-form/CRD-defaulting asymmetry that produced F1
F6 (consensus, both reviewers): all handler/spec tests build old and new from the same fully-populated baseSpec() (e.g. spec_update_test.go:66 bakes CleanPolicy: OnRuntimeDeleted into both), so no test constructs the old object from a stored form omitting spec.client while the new one carries the API-server-defaulted cleanPolicy — precisely the asymmetry that let the PR's own suite stay green while CI went red on five versions. Must land WITH the F1 fix, not after: a regression case where the old object is decoded from a stored form without spec.client and the new one carries the defaulted cleanPolicy, plus the anti-overfix guard (a real change to OnDemand stays denied). Runnable examples exist on both verify branches (verify_pr6243_defaulting_test.go, TestVerifyPR6243ClientlessMetadataUpdateAllowed). - test/gha-e2e/curvine/test.sh:550 [minor] — Workload-sync assertions are skipped in single-node CI — no e2e verifies a supported update reaches the AdvancedStatefulSet
M2 (narrow residual after Codex's rebuttal of Claude's broader M1; the residual is agreed by both): the curvine e2e already covers the admission half end-to-end — scale_worker_and_verify 1 patch (test/gha-e2e/curvine/test.sh:550/554, pre-existing on base) issues kubectl patch of spec.worker.replicas on the main resource and asserts the value lands, and it passed in this PR's CI. But the kind clusters are single-node (default helm/kind-action, no extra nodes in .github/workflows/kind-e2e.yml), so verify_pods=false and the sync-to-AdvancedStatefulSet assertions are skipped — equally for the main-resource and the scale-subresource paths. Unit tests cover the sync logic; the real-cluster propagation is unverified in CI (A10).
Verified review delta
- F1: open (initial-pr)
- F2: open (initial-pr)
- F3: open (initial-pr)
- F4: open (initial-pr)
- F5: open (initial-pr)
- F7: open (initial-pr)
- F6: open (initial-pr)
- M2: open (initial-pr)
Code: needs-work. CI: failure; attribution: pr-regression. Required CI checks still apply.
| client := root.Child("client") | ||
| errs = append(errs, validateComponentCommonSpecUpdate(client, &oldSpec.Client.RuntimeComponentCommonSpec, &newSpec.Client.RuntimeComponentCommonSpec, false)...) | ||
| errs = append(errs, requireUnchanged(client.Child("tieredStore"), oldSpec.Client.TieredStore, newSpec.Client.TieredStore)...) | ||
| errs = append(errs, requireUnchanged(client.Child("cleanPolicy"), oldSpec.Client.CleanPolicy, newSpec.Client.CleanPolicy)...) |
There was a problem hiding this comment.
F1 (consensus, both reviewers independently, confirmed in both cross-critiques): for a CacheRuntime whose stored form has no spec.client section (any client-less topology applied from YAML, e.g. test/gha-e2e/mooncake/cacheruntime.yaml), EVERY full-object Update is denied — including the controller's own finalizer/owner-reference writes — because: (1) CRD structural defaulting does not materialize absent parent objects in the stored/old object; (2) a typed Go client marshals the runtime with a materialized client object (encoding/json never omits zero-value structs, api/v1alpha1/cacheruntime_types.go:153); (3) the API server then applies the CRD default spec.client.cleanPolicy=OnRuntimeDeleted to the new object only, producing a phantom immutable-field diff that ValidateSpecUpdate forbids. Consequences: the CacheRuntime never reconciles (dataset stuck NotBound — mooncake e2e fails on all five K8s versions in this PR's CI) and deletion hangs because finalizer removal is also a full-object Update. The PR's claim that metadata writes are never affected is false for exactly these CRs. Fix direction (harness-bites verified, then reverted): make it GENERAL — normalize CRD defaults before comparing / treat old-zero-new-CRD-default as unchanged / re-default the old object, so any future defaulted immutable field (Q2) is covered; an identity exemption for the controller SA alone is not general (a user doing kubectl replace or a typed write of a pre-existing client-less CacheRuntime would still be denied). Reviewer C (C4575767901), verified by both debaters, widens the fix space: cleanPolicy is dynamically consumed by CSI NodeUnstageVolume (pkg/ddc/base/runtime.go -> SetupFuseCleanPolicy, pkg/csi/plugins/nodeserver.go:719-730), so exempting cleanPolicy from the immutable list is a legitimate fix that also preserves a genuinely dynamic knob. A real change (e.g. -> OnDemand) must stay denied; the harnesses guard that.
| "github.com/fluid-cloudnative/fluid/pkg/webhook/handler/validating/cacheruntime" | ||
| ) | ||
|
|
||
| // +kubebuilder:webhook:path=/validate-data-fluid-io-v1alpha1-cacheruntime,mutating=false,failurePolicy=fail,sideEffects=None,admissionReviewVersions=v1;v1beta1,groups=data.fluid.io,resources=cacheruntimes,verbs=update,versions=v1alpha1,name=update.cacheruntime.fluid.io |
There was a problem hiding this comment.
F2 (consensus; severity label adjudicated from Claude's 'major' vs Codex's 'blocker' — a required CI gate failing on the PR head is merge-blocking by definition, and both reviewers list it as must-fix): the PR adds the +kubebuilder:webhook marker but does not commit the regenerated config/webhook/manifests.yaml. The lint gate runs make update-crd (manifests via the repo-pinned controller-gen v0.19.0) and fails on the dirty tree: 'CRD validation failed. Please use update-crd to keep CRDs latest'. The generated config no longer reflects the shipped webhook. Fix: run make update-crd / make manifests and commit the output.
Ⅰ. Describe what this PR does
Adds a validating admission webhook for CacheRuntime (
update.cacheruntime.fluid.io). An UPDATE of a CacheRuntime that changes a spec field which cannot take effect in place is now rejected at admission time.Background
The CacheRuntime controller syncs only the
replicas,runtimeVersionandresourcesof the master and worker to their AdvancedStatefulSets (seeAdvancedStatefulSetManager.SyncComponentSpec). The client is a DaemonSet and is never synced. Every other field is read once, when the component is created, so after a change:tieredStore: the update is applied halfway. The quota in the runtime ConfigMap follows the spec, while the tmpfssizeLimitof the Worker Pod keeps its creation-time value, and the mismatch only surfaces when the Worker restarts.Changes
Validation (
pkg/webhook/handler/validating/cacheruntime)ValidateSpecUpdatecompares the old and new spec field by field. Any changed field other than master/workerreplicas,runtimeVersionandresourcesyields afield.Forbidden; the client'sruntimeVersionandresourcesare immutable too.equality.Semantic.DeepEqual): nil equals an empty list, and2Giequals2048Mi.specis compared, so writes tometadataandstatus(such as the controller's own finalizer and condition updates) are never affected. The webhook rule matches only the main resource, so writes through thestatusandscalesubresources (e.g.kubectl scale) are not checked.validating.HandlerMapis enabled inpkg/webhook/handler/register.go, serving/validate-data-fluid-io-v1alpha1-cacheruntime.caBundle injection
The webhook serves a self-signed certificate generated at startup, so the chart installs the configuration with a placeholder
caBundleand the webhook process patches in the real CA. The existing mechanism only handled the MutatingWebhookConfiguration; this PR extends it to the new ValidatingWebhookConfiguration.CertificateBuildergainsPatchValidatingCABundle, which injects the caBundle into aValidatingWebhookConfigurationand skips the patch when nothing changed.WebhookReconcilergains aValidatingWebhookNamefield and picks the configuration to patch by the request name.pkg/ctrl/watchwatchesfluid-cacheruntime-validating-webhook(by that name only) and re-injects the caBundle whenever it is created or updated, e.g. whenhelm upgraderesets the caBundle to the placeholder. The webhook's cache lists/watches this object by name only.Helm chart
ValidatingWebhookConfigurationfluid-cacheruntime-validating-webhook, reusing the existingfluid-pod-admission-webhookService.webhook.cacheRuntimeValidation.failurePolicy(defaultFail), which decides whether a CacheRuntime update is rejected or let through unchecked while the webhook is unreachable.get/patch/list/watchon that configuration, restricted byresourceNames.The chart and the code must agree on a few names: the configuration name (
common.ValidatingWebhookName), the path (common.WebhookValidateCacheRuntimePath), and the RBACresourceNames. Because RBAC withresourceNamesonly allows list/watch with ametadata.namefield selector, the webhook's cache incmd/webhook/app/webhook.gofilters this object by name.Docs: updates the English and Chinese
cacheruntime_spec_update.mdto explain that an update to an unsupported field is rejected, that the CacheRuntime must be deleted and recreated to change such a field, and whatfailurePolicymeans.Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/webhook/handler/validating/cacheruntime/spec_update_test.go: the updatable fields (master/worker replicas/runtimeVersion/resources), each immutable field, several violations at once, and semantic equality (nil vs. empty, quantities in different units).pkg/webhook/handler/validating/cacheruntime/handler_test.go: CREATE/DELETE/CONNECT are allowed, subresource writes are allowed, a missing or malformed old/new object returns 400, metadata/status-only writes are allowed, and an update changing immutable fields is denied with every offending field listed.pkg/webhook/certificate_builder_test.go:PatchValidatingCABundleinjects the caBundle into every webhook, leaves an already patched configuration alone, fails when the configuration does not exist, and does not touch a MutatingWebhookConfiguration of the same name.pkg/controllers/v1alpha1/webhook/webhook_controller_test.go: patches only the Mutating or Validating configuration the request names, and requeues after 10 seconds when the patch fails.pkg/ctrl/watch/validatingwebhookconfiguration_test.go: the event filter passes create/update events for the target name only; delete events do not trigger a reconcile.test/gha-e2e/curvine/test.sh): newverify_immutable_spec_update_rejected. Patchingspec.worker.tieredStoremust be rejected by the webhook and leave the spec unchanged; annotating the CacheRuntime must succeed.Ⅳ. Describe how to verify it
webhook.enabled=true, the default) and create a CacheRuntime and a Dataset.kubectl patch cacheruntime demo --type merge -p '{"spec":{"worker":{"replicas":2}}}' kubectl scale cacheruntime demo --replicas=1Ⅴ. Special notes for reviews
ValidateSpecUpdateenumerates the fields that cannot change. A field added toCacheRuntimeSpeclater is updatable unless it is added here as well, so this list needs to be kept in sync with the spec.failurePolicydefaults toFail: while the webhook is unreachable, or before its caBundle is injected, CacheRuntime updates are rejected. The controller's status writes go through the subresource and are unaffected, but its writes to the main resource (such as finalizers) fail too. Set it toIgnoreto favor availability.Fail); the e2e test retries for this window.webhook.enabled=false) nothing is checked, and the behavior is as before: the update is accepted but does not take effect.🤖 Generated with Claude Code