You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(webhook): inject the runtime config into app pods of client-less cache runtimes - #6197
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign yangyuliufeng for approval by writing /assign @yangyuliufeng in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Details
Needs approval from an approver in each of these files:
I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
❌ Patch coverage is 90.85714% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.91%. Comparing base (e0fc4c1) to head (41b4714). ⚠️ Report is 15 commits behind head on master.
… cache runtimes
A cache system without a FUSE client, such as Mooncake, has its app talk to
the cache service through the system's own SDK. The app pod mounts no dataset
volume, so until now it could only learn the master's address by hardcoding
it.
Add a RuntimeConfigInjector webhook plugin. A pod labelled fluid.io/inject=true
gets, for every dataset listed in its fluid.io/datasets annotation, that
dataset's runtime config ConfigMap mounted read-only at
/etc/fluid/config/<dataset>/, and an env var FLUID_RUNTIME_CONFIG_PATH_<DATASET>
pointing at the runtime.sh in it. The dataset name in the env var is
upper-cased with '-' turned into '_', since a shell variable name can't hold a
hyphen.
The annotation is parsed by CollectRuntimeInfosFromAnnotations, next to the
PVC-based collector, which also checks that each dataset is bound. These pods
go to a new clientless handler group, so none of the FUSE-related plugins run
on them. The plugin checks every dataset before it touches the pod: a plugin
error is only logged and the pod is still admitted, so failing half way
through would leave a partially injected pod behind.
The annotation key is fluid.io/datasets rather than fluid.io/dataset, which is
already used as a worker pod label for anti-affinity. The config directory,
file names and env var prefix move to pkg/common so the engine and the webhook
share one definition.
Part of fluid-cloudnative#6176.
Signed-off-by: btxu-db <btxu-db@outlook.com>
Describe how an app pod of a cache system without a FUSE client asks for the
runtime config, what gets injected, and the limits that come with it, and list
the variables in runtime.sh. Its example is rendered by the actual generator.
The Mooncake sample gets an optional section that reads the master address
from the injected config instead of hardcoding it.
Part of fluid-cloudnative#6176.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The webhook's ClusterRole lists every runtime kind except cacheruntimes.
base.GetRuntimeInfo reads the CacheRuntime object for a CacheRuntime dataset,
so the cached client starts an informer that is never allowed to list, the
Get waits on a cache that never syncs, and the admission call times out after
15s. With failurePolicy Fail the pod is then rejected.
A pod that mounts a CacheRuntime dataset's PVC and carries one of the FUSE
injection labels would run into the same timeout; the existing e2e pods carry
no such label, so it went unnoticed. A pod asking for runtime config injection
always takes this path, and could not be created at all.
Part of fluid-cloudnative#6176.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The reason will be displayed to describe this comment to others. Learn more.
The premise checks out (stage-1 runtime.sh is merged and nothing delivered it to FUSE-less app pods; the cacheruntimes RBAC fix is real), and the plugin itself is in good shape — clean all-or-nothing injection, deterministic ordering, thorough unit tests. Two things should be resolved before merge, both about silent failure. First, on helm upgrade with default values, the chart's lookup preserves the old webhook-plugins ConfigMap, so the new clientless.fluid.io rule matches labeled pods but no plugin runs: they are admitted with zero injection and no visible error — verified independently by two test harnesses; the cleanest fix is merging the new clientless group into a preserved profile rather than relying on forceReplacePluginsProfile. Second, when the runtime config ConfigMap is missing — including the informer-lag window right after a dataset binds, since the plugin reads through the cached client — the error is logged and the pod is admitted without its config, permanently; we'd reject opted-in pods instead (retry via the direct reader, then fail with a clear message). The two of us debated the severity of that second item (minor vs major) and settled on major given how it hits the feature's primary scripted-pipeline use case. Smaller items: dataset names of 43–63 chars pass validation but are silently never injected; combining fluid.io/inject with the FUSE labels silently skips the injector; and the new handler routing and annotation collection have no tests (both of our harnesses are donate-able starting points). Copilot (advisory) independently flags the upgrade no-op and the mixed-label routing, plus two adjacent injector edges we spot-checked as real.
The reason will be displayed to describe this comment to others. Learn more.
Consensus finding (both reviewers, independently verified by two disjoint harnesses; Copilot agrees). On helm upgrade of an existing install with default values, plugins-profile.yaml re-emits the live webhook-plugins ConfigMap verbatim (lookup + forceReplacePluginsProfile: false), so the preserved profile has no clientless group — while webhookconfiguration.yaml unconditionally adds the clientless.fluid.io rule for fluid.io/inject: "true" pods. Labeled pods are intercepted, routed to an empty clientless plugin list, and admitted with zero injection, no error, and no log. The chart also has no checksum annotation and the profile is read once at webhook startup with no reload, so even a manual ConfigMap fix needs a pod restart nothing triggers; the new docs never mention forceReplacePluginsProfile. Preferred fix (absorbing Copilot's caveat that force-replacing clobbers user customizations): merge/render the new clientless group into a preserved profile rather than documenting forceReplacePluginsProfile=true; a startup warning when the loaded profile lacks clientless would also help.
The reason will be displayed to describe this comment to others. Learn more.
F3 (severity disputed in phase 1 — Codex rated it minor, Claude major; adjudicated major with Codex concurring in cross-critique that "major is fair"; not the sole basis of this review's verdict). When the ConfigMap is missing or lacks runtime.sh (e.g. an older cacheruntime-controller during rollout skew), the plugin returns a plain non-retryable error; Handle() (pkg/webhook/handler/mutating/mutating_handler.go:101) logs it and admits the pod unmutated — failurePolicy: Fail never fires because the webhook itself answers Allowed. This is inconsistent with the sibling path the PR does make fatal (dataset missing/not Bound → NeedRetryWithApiReaderError → retry → rejected). The failure is permanent for the pod: nothing re-triggers admission when the ConfigMap appears later. The plugin reads the ConfigMap through the cached manager client (RegisterMutatingHandlers(mgr.GetClient())), so "not found" also covers informer lag right after a dataset turns Bound — exactly when a scripted pipeline (wait for Bound → create app pod) creates its pod; that intermittency is reasoning-only (a fake client cannot lag), but the unit-level plain-error→admit-unmutated mechanism is test-proven. Recommended fix, on which both reviewers agree: fail closed for opted-in pods — wrap the plugin error in NeedRetryWithApiReaderError so the pod is retried once against the direct reader and then rejected with a clear message (a rejected create self-heals via the Deployment controller; an admitted-without-config pod never does). Note the direct-reader retry needs the MutatingHandler interface change the author mentioned.
The reason will be displayed to describe this comment to others. Learn more.
Found by Codex, confirmed by Claude in cross-critique. CollectRuntimeInfosFromAnnotations accepts any DNS-1035 label (≤63 chars, pkg/webhook/utils/runtime_info.go:111), but the plugin rejects names longer than 63 - len("fluid-runtime-config-") = 42 because the volume name embeds the ConfigMap name. In that window collection succeeds (dataset resolves, Bound, runtime info built), the plugin errors, Handle swallows the error, and the pod is created with no injection and no user-facing signal — while a dotted name is loudly rejected at admission. Verified: a 50-char Bound cache dataset with its ConfigMap present is admitted unmutated. Suggest moving the length check into the collector (fail closed with a clear message) or using a volume name that does not embed the full dataset name.
The reason will be displayed to describe this comment to others. Learn more.
Found by Codex, mechanism code-verified by Claude in cross-critique; Copilot flagged the same issue. A pod carrying fluid.io/inject: "true" together with serverless.fluid.io/inject or fuse.serverful.fluid.io/inject matches two webhook rules; the annotation collection at line 154 runs unconditionally so the annotation datasets are merged into runtimeInfos, but the switch's precedence (Serverless/Serverful tested before Inject) routes the pod to the FUSE plugin list, so RuntimeConfigInjector never runs: no config injection, no error, and the annotation datasets leak into serverful plugins' input (e.g. NodeAffinityWithCache affinity). Corner case — requires deliberately combining labels the docs never suggest combining. Worth documenting that the labels are mutually exclusive or making the routing precedence explicit.
The reason will be displayed to describe this comment to others. Learn more.
Flagged by Copilot (Reviewer C); mechanism spot-checked and confirmed real by both debaters, so recorded here as a minor risk. CollectRuntimeInfosFromPVCs keys runtimeInfos by PVC name (pkg/webhook/utils/runtime_info.go:36/60), while the annotation collector keys by dataset name; RuntimeConfigInjector.Mutate iterates map keys as dataset names. A clientless-labeled pod that also mounts a fluid-managed PVC whose name differs from its dataset (e.g. PVC input managed by Dataset demo) makes the plugin look up fluid-runtime-config-<pvcName>, miss, and abort the entire injection silently — landing in the same swallowed-error path as the fail-open finding above. Suggested fix: normalize PVC entries by runtimeInfo.GetName() for the clientless route while preserving PVC-keyed inputs for the FUSE handlers.
The reason will be displayed to describe this comment to others. Learn more.
Copilot-only flag, unconfirmed by the debaters (Codex called the code behavior real but did not adopt it as a finding) — advisory. AppendOrOverrideVolumeMounts matches only volume names, so a container already mounting another volume at /etc/fluid/config/<ds> would receive a duplicate mount path when dataset <ds> is injected, and Kubernetes rejects the otherwise valid pod. A preflight mount-path conflict check across init and regular containers would turn this into a clear error; note that under the current admission policy a plain plugin error is swallowed (see the fail-open finding), so the preflight should be paired with fail-closed handling.
The reason will be displayed to describe this comment to others. Learn more.
Consensus of both reviewers. The PR adds a real switch branch, annotation collection, a PVC-wins merge, and error wrapping in MutatePod, but mutating_handler_test.go (2200+ lines of serverless/serverful coverage) is untouched, and CollectRuntimeInfosFromAnnotations (76 lines of new pure parsing/dedup/validation logic) has no unit tests; only the plugin package is tested. The e2e deferral to #6175/#6176 is tracked, so this is not a coverage blocker — but the routing layer is exactly where the upgrade no-op, the name-length window, and the mixed-label routing above would be caught by regression if tests existed. Both verification harnesses (verify/runtime-config-injector-claude's clientless_harness_test.go, verify/clientless-runtime-config-inject-codex's verify_pr6197_codex_test.go) are donate-able starting points.
Address the review of fluid-cloudnative#6197: every way the injection could be skipped now
either works or rejects the pod with a message saying why, instead of
admitting it with nothing injected.
- Upgrade: the chart keeps an existing webhook-plugins profile, which has no
clientless group, so the new rule matched pods that no plugin handled. Add
the missing group to a preserved profile, and restart the webhook through a
checksum annotation when the profile changes.
- Missing config map or runtime.sh: the plugin read through the cached client,
which may lag behind a dataset that was just bound, and its error was only
logged. Plugins can now implement ReaderAwareHandler to get the client the
handler uses, and these errors ask for a retry with the API reader, so the
pod is rejected only if the config is still missing.
- Mixed labels: a pod with fluid.io/inject and a FUSE injection label was
routed to the FUSE plugins only. Reject the combination.
- PVC datasets on the clientless route were keyed by PVC name, so a PVC
managed by a dataset of another name made the lookup miss. Key them by
dataset name, and only take cache runtimes, which are the ones with a
runtime config.
- Dataset names of 43-63 characters passed collection but cannot form a
volume name; reject them while collecting the annotation.
- A container already mounting another volume at the config's path would get
a duplicate mount path; reject it before mutating.
Failures the pod spec itself causes return the new RejectPodError, which the
handler turns into a denial without retrying.
Part of fluid-cloudnative#6176.
Signed-off-by: btxu-db <btxu-db@outlook.com>
Matching the ConfigMap name does not make an existing volume equivalent to the injected one. A volume projecting runtime.json as app.json at the expected mount path passes this check, but AppendOrOverrideVolume subsequently replaces it, removing app.json. Custom file permissions and Optional settings are also reset. Reject incompatible projections and settings before overwriting, while still accepting API-defaulted versions of the generated volume. Add regression coverage for these conflicts.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up review at 6d3a6d7e (delta from the previous debate round at c5ea086b).
The three majors from last round are all fixed, and each re-verified the hard way: the webhook-plugins upgrade path now merges the clientless profile into a preserved ConfigMap, and the new checksum/plugins-profile annotation rolls the webhook so it actually reloads — checked with the real helm CLI against an envtest API server seeded with a legacy profile (red on the old head, green on this one). Over-long dataset names are now rejected up front instead of being silently admitted unmutated. And the failure semantics are uniformly fail-closed: a missing ConfigMap or a ConfigMap without runtime.sh denies the pod after one API-reader retry, with the docs updated to match. The PVC merge path also now keys by dataset name, so inject-labeled pods mounting a cache-runtime PVC actually get injected.
No new blockers. CI is fully green at this head (build, lint, staticcheck, unittest, CodeQL, kind-e2e and backward-compat across k8s 1.22–1.33, DCO, codecov; the only pending status is tide, which is the merge queue itself, not a check).
One process caveat: the second reviewer's structured pass failed this round, so the cross-critique never ran — the two reviews are independent but uncrossed. They happened to converge (neither found an open blocker or major at this head), and Copilot also reviewed this head and raised nothing.
Next round
[MUST] none — no open PR-introduced blocker/major at this head
[SHOULD] runtime-type check in the injector so a fuse-served dataset in fluid.io/datasets gets a denial that names the type instead of "may not be ready yet" (A-F1); DNS-1123 volume-name guard on the PVC-resolved path (A-F2); carry the "too long" reason into the over-long-name denial (B-F5); trim before indenting to avoid the one-off checksum churn on the second upgrade (B-F6); consider a webhook.clientless.enabled values gate and a values-side profile merge (A-F3); exact-name match instead of the fluid-fuse prefix skip (A-N1) — all in the folded appendix below
[Filed] none
Non-blocking observations (9) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
pkg/webhook/plugins/runtimeconfiginjector/runtime_config_injector.go:127 — Misleading "may not be ready yet" denial when a non-cache dataset is listed in fluid.io/datasets (no runtim... [minor]
pkg/webhook/plugins/runtimeconfiginjector/runtime_config_injector.go:114 — PVC merge path bypasses the dataset-name shape check, producing an API-server-invalid volume name [minor]
charts/fluid/fluid/templates/webhook/plugins-profile.yaml:26 — Chart merge asymmetry: a user-supplied pluginsProfile gets no clientless merge, and the new webhook rule ha... [minor]
pkg/webhook/plugins/runtimeconfiginjector/runtime_config_injector.go:127 — First admission attempt reads ConfigMaps through the cached client, starting a cluster-wide ConfigMap informer [minor]
pkg/webhook/utils/runtime_info.go:154 — Over-long annotation dataset name denied with a generic aggregate message that does not state the reason [minor]
test/gha-e2e/mooncake/ — e2e coverage for the injection path deferred to the #6175-dependent mooncake e2e work [minor]
charts/fluid/fluid/templates/webhook/plugins-profile.yaml:30 — Profile checksum re-renders once on the second upgrade (one spurious webhook rollout) [nit]
pkg/webhook/plugins/runtimeconfiginjector/runtime_config_injector.go:186 — Container-name prefix skip can catch user containers named fluid-fuse-* [nit]
Verified review delta
A-F1: open (initial-pr)
A-F2: open (initial-pr)
A-F3: open (initial-pr)
A-F4: open (initial-pr)
B-F1: fixed (initial-pr)
B-F2: fixed (initial-pr)
B-F3: fixed (initial-pr)
B-F4: refuted (initial-pr)
B-F5: open (initial-pr)
A-Q1: open (initial-pr)
A-MT1: deferred (initial-pr)
B-F6: open (initial-pr)
A-N1: open (initial-pr)
Code: pending. CI: success; attribution: unknown. Required CI checks still apply.
…cheRuntime
CacheRuntime has not shipped in a release yet, so no released
cacheruntime-controller lacks runtime.sh. Point at the controller that
generates the ConfigMap instead.
Signed-off-by: btxu-db <btxu-db@outlook.com>
…d for injection
A dataset served by a fuse runtime has no runtime config map, so listing
one in fluid.io/datasets was denied as a config map that "may not be
ready yet". Reject it up front with a message naming the runtime type
and pointing at the PVC.
Signed-off-by: btxu-db <btxu-db@outlook.com>
…dataset
The annotation path rejects a dataset name that is not a DNS-1035
label, but a dataset resolved from a PVC skips that check, and a dataset
name with a dot produced a volume name the API server rejects. Validate
the volume name as a DNS-1123 label before injecting.
Signed-off-by: btxu-db <btxu-db@outlook.com>
…runtime config
The prefix match skipped any container whose name starts with
fluid-fuse, including an app container such as fluid-fuse-proxy. Match
the names fluid gives its sidecars, fluid-fuse and fluid-fuse-<index>.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The annotation collector logged the reason but denied the pod with only
the dataset names, so an over-long or misspelled name read the same as
one that is not bound yet. Append each dataset's reason to the error.
Signed-off-by: btxu-db <btxu-db@outlook.com>
A profile kept from an earlier install ends with the newline its block
scalar preserves, and indent turned that into a whitespace-only line.
The second upgrade after adding the clientless group therefore rendered
the ConfigMap differently from the first and rolled the webhook once
for nothing.
Signed-off-by: btxu-db <btxu-db@outlook.com>
Document that a dataset of a non-cache runtime and a dataset name with
a dot are rejected, and that only fluid's own fuse sidecars are skipped.
Signed-off-by: btxu-db <btxu-db@outlook.com>
trim also stripped the first line's indent, so a profile indented as a
whole came out as invalid YAML and the webhook failed to parse it.
Remove only the newline the block scalar keeps, which is all the
checksum churn needed.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The comment still assumed a DNS-1035 label. A dataset resolved from a PVC
only has to form a DNS-1123 label volume name, which may start with a
digit; say so, and why the suffix stays safe.
Signed-off-by: btxu-db <btxu-db@outlook.com>
buildConfigMount calls envSuffix before the volume name check, so the
guarantee applies to the datasets that are injected, not to every name
that reaches the function.
Signed-off-by: btxu-db <btxu-db@outlook.com>
An existing volume referencing the same ConfigMap with items: [{key: runtime.json, path: config.json}] passes this check when mounted at the expected directory. AppendOrOverrideVolume then replaces the entire volume source, removing config.json and resetting any custom file permissions. Reject customized projections, modes, and optional settings before mutation, while accepting equivalent API defaults. Add a regression test confirming that rejection leaves the pod unchanged.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up review at 268012a5 (delta from 6d3a6d7e, 10 commits — this round lands the fixes for last round's SHOULD list).
Five of the six open items from last round are fixed, and each fix is verified differentially rather than by reading the diff (red on 6d3a6d7e, green at this head, on both harnesses): an Alluxio dataset listed in fluid.io/datasets is now denied naming the runtime type instead of the misleading "configmap … may not be ready yet"; a dotted dataset resolved from a PVC is denied naming the invalid volume name instead of producing an API-server-invalid pod; the annotation-path denial now carries the per-dataset reason ("too long", DNS-1035), so the docs' reason-in-the-error claim holds on every path; only fluid's own fluid-fuse / fluid-fuse-<index> containers are skipped, so a user container named fluid-fuse-proxy gets injected; and the plugins-profile checksum no longer churns once on the second helm upgrade — that last one checked with the real helm CLI against an envtest API server through two actual upgrades, storing the profile as the API server persists it. The three majors from round 1 stay fixed and re-verified green at head.
No new findings in the delta: both reviewers reviewed it independently and cross-critiqued each other's claims with zero disagreements (the only notes exchanged were evidence-strength comparisons and two citation corrections that change no conclusion), and Copilot also reviewed this head and raised nothing. The handler's retry-then-deny flow composes correctly with the new typed rejections, and the docs in both languages match the code. CI is fully green at this head (all 22 checks SUCCESS; the only pending status is tide, the merge queue itself).
Still open, non-blocking: the chart has no values-level gate for the new webhook rule and a user-supplied pluginsProfile still gets no clientless merge (A-F3); the first admission read of the runtime-config ConfigMap goes through the cached client, starting a cluster-wide ConfigMap informer for the webhook's lifetime — worth a conscious decision or an API-reader-only read (A-F4, hypothesis; the live layer was skipped); and the cluster e2e for the injection path remains deferred to the post-#6175 mooncake e2e PR.
Next round
[MUST] none — no open PR-introduced blocker/major at this head
[SHOULD] a values-level gate or values-side merge for the clientless plugin group, so a user who overrides webhook.pluginsProfile without a clientless key doesn't get the webhook rule with no plugin behind it (A-F3, mirrors pre-existing serverless/serverful behavior); a conscious decision on reading the runtime-config ConfigMap through the API reader instead of the cached client, to avoid a webhook-lifetime cluster-wide ConfigMap informer (A-F4); carry a tracking-issue link into the deferred mooncake e2e follow-up (A-MT1); naming taste for the maintainer — the completion marker says "sidecar" though nothing sidecar-like is injected, and fluid.io/inject is more generic than the scoped serverless/serverful label names (A-Q1)
[Filed] none
Verified review delta
A-F1: fixed (initial-pr)
A-F2: fixed (initial-pr)
B-F5: fixed (initial-pr)
B-F6: fixed (initial-pr)
A-N1: fixed (initial-pr)
Code: pending. CI: success; attribution: unknown. Required CI checks still apply.
…placing it
A pod may already carry a volume of the runtime config ConfigMap under the
injected name, with its own items, mode or optional setting. The name and
ConfigMap check let it through, and the injection then replaced the whole
volume source, dropping the files and permissions the app projected.
Accept such a volume only when it is what the injection builds, allowing
for the default mode the API server fills in, and reject it otherwise
before the pod is touched.
Signed-off-by: btxu-db <btxu-db@outlook.com>
…ues lack it
helm upgrade --reuse-values from a release that predates the clientless
group reuses that release's defaults, so the values carry no clientless
group. The profile merge then wrote "clientless: null" while the
clientless.fluid.io webhook rule was rendered, and pods matching the rule
were admitted with no plugin run.
Default the group to the chart's own RuntimeConfigInjector list whenever
the values leave it unset or empty, on every rendering path. A group set
explicitly, even with empty lists, is kept.
Signed-off-by: btxu-db <btxu-db@outlook.com>
be4e30d deep-copied webhook.pluginsProfile and read its plugins
unconditionally, so a pluginsProfile set to null failed to render with
"reflect: call of reflect.Value.Type on zero Value", even where an
existing profile used to be kept as it is.
Apply the clientless fallback only to values that are maps, and merge the
resolved group into an existing profile, so plugins: null no longer
renders a broken profile either. Note that the fallback mirrors the
clientless group in values.yaml.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
An unresolved validation issue remains, and the admission changes and live Helm profile migration need human verification of upgrade and rollback behavior.
[P2] This rejects valid Dataset names beginning with a digit, such as 1demo. Dataset metadata permits those names, and the generated volume (fluid-runtime-config-1demo), service and prefixed environment variable are all valid, so an otherwise ready runtime becomes unusable through the annotation-based path. Use validation.IsDNS1123Label here instead, update the expected validation error in the tests, and add a digit-leading-name regression case.
The reason will be displayed to describe this comment to others. Learn more.
Round 4 at 41b47141 (delta from 268012a5, 3 commits — the values-side chart fallback, a new webhook rejection class, and a null/non-map guard).
Both of the code items this round landed, and each is verified differentially rather than by reading the diff. The chart now injects its own clientless plugin group when the values lack one: the real trigger turned out to be helm upgrade --reuse-values from a pre-clientless release (a plain -f override deep-merges with the chart's values.yaml and was never exposed), and at the previous head that path actually wrote clientless: null into the stored webhook-plugins ConfigMap — verified with a real helm install of the base chart plus a real --reuse-values upgrade against an envtest API server, red on the previous head and green here, with null/non-map profiles and the default-values render byte-identical across the delta (same checksum/plugins-profile, so default users' webhook does not roll). And a pod that already projects the runtime-config ConfigMap under the injected volume name with its own items / defaultMode / optional: true used to have that volume silently replaced, dropping the app's projection — it's now denied naming the volume and the customization, while the API-server-defaulted shape (explicit defaultMode: 0644, optional: false) still passes, so re-mutating an already-injected or GitOps-restored spec keeps working. All twelve harness findings re-verify FIXED at this head on both reviewers' harnesses, and CI is fully green (22 checks; tide pending by design, it's the merge queue).
New this round, all minor and verified by both: a falseyplugins.clientless value — {} or null — is silently replaced by the chart's default group, because the fallback gates on Go-template truthiness rather than hasKey (the schema-valid empty group is respected; and the [] spelling, which used to crash-loop the webhook at startup, is actually repaired by this change — the silent-reversal case is {} and null). And the annotation path validates dataset names with IsDNS1035Label, so a digit-leading name like 1demo — legal as an object name, valid as the derived volume and env var, and accepted via the PVC path — is denied via fluid.io/datasets with a reason the docs don't list; IsDNS1123Label, already used for the volume name, would align the two paths. One question for the maintainer: forceReplacePluginsProfile no longer renders values verbatim (the clientless group is injected even under force-replace) — defensible given the unconditional webhook rule, but please confirm it's intended. One nit: the hard-coded fallback group duplicates values.yaml with nothing in the shipped chart's CI enforcing the equality.
The only disagreement between the reviewers this round was framing — whether the values-side fallback made the "no disable gate" concern moot (it doesn't quite: an explicit falsey value is silently reversed) and whether [] counted as a silent reversal (it was a crash-loop before) — both resolved in the ledger. Reviewer C (Copilot) raised no findings at this head.
Still open, non-blocking: the cached-client ConfigMap read that starts a webhook-lifetime cluster-wide informer (hypothesis; the live layer was skipped in this setup), and the cluster e2e deferred to the post-#6175 mooncake PR — please carry a tracking-issue link there.
Next round
[MUST] none — no open PR-introduced blocker/major at this head
[SHOULD] hasKey instead of truthiness for the clientless fallback, so an explicit falsey {}/null value is not silently reversed (A-F5); align the annotation-path name check with the volume-name constraint (IsDNS1123Label) or document the digit-leading rejection class, plus a regression case (A-F6); confirm the force-replace semantics change is intended, or exempt that branch (A-Q2); a conscious decision on reading the runtime-config ConfigMap through the API reader instead of the cached client (A-F4); keep the hard-coded fallback group in line with values.yaml — a cross-reference comment or helm-unittest (B-F8); carry a tracking-issue link into the deferred mooncake e2e follow-up (A-MT1); naming taste — the completion marker says "sidecar" though nothing sidecar-like is injected, and fluid.io/inject is more generic than the scoped serverless/serverful names (A-Q1)
[Filed] none
Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
charts/fluid/fluid/templates/webhook/plugins-profile.yaml:23 [minor] — A falsey plugins.clientless values value ({} or null) is silently replaced by the chart's default group
A-F5: The values-side fallback added by commit be4e30d gates on Go-template truthiness ({{ if get $valuesPlugins "clientless" }}), so an empty map or null is falsy: a user writing webhook.pluginsProfile.plugins.clientless: {} (or null) to mean "off" silently gets the chart's RuntimeConfigInjector group instead — attempted-and-silently-reversed, not "not offered". The schema-valid opt-out (a 2-key empty group with withDataset: [] / withoutDataset: []) IS respected. Adjudicated refinement (Reviewer B, confirmed by parse matrix): the [] spelling is unparseable by the webhook (yaml.v2 !!seq into the group struct errors; cmd/webhook os.Exit(1)), so at the previous head [] meant a crash-looping webhook — for [] the delta is a repair; the silent-reversal claim stands exactly for {} and null. Suggested fix: hasKey $valuesPlugins "clientless" instead of truthiness (the stored-profile merge branch already uses hasKey, so the two branches disagree today). Bounded impact: unreleased chart, opt-in rule (fluid.io/inject=true).
pkg/webhook/utils/runtime_info.go:114 [minor] — Annotation path rejects digit-leading dataset names (IsDNS1035Label) that the plugin/PVC path accepts
A-F6: CollectRuntimeInfosFromAnnotations validates each dataset name with IsDNS1035Label, whose first character must be a letter, while a Dataset object name may start with a digit and the injector path (annotation-resolved and PVC-resolved alike) only checks the length and the derived volume name — fluid-runtime-config-1demo is a valid DNS-1123 label and FLUID_RUNTIME_CONFIG_PATH_1DEMO a valid env name. A dataset named 1demo therefore works through a mounted PVC + fluid.io/inject but is denied via fluid.io/datasets, with a DNS-1035 reason the docs do not list as a rejection class. Suggested fix: IsDNS1123Label on the annotation path (the volume-name guard already covers the real API constraint), a doc line, and a digit-leading regression case. Late discovery on code unchanged since 6d3a6d7 — an initial-PR miss, not a new regression. Note: Reviewer A's phase-1 doc credits Copilot with flagging this at the same head, but the advisory Copilot document reports no findings this round; the finding stands on both debaters' independent verification (consensus A+B, not Copilot-only).
charts/fluid/fluid/templates/webhook/plugins-profile.yaml:30 [minor] — forceReplacePluginsProfile no longer renders the values verbatim — intentional?
A-Q2: New in round 4 (commit be4e30d). With forceReplacePluginsProfile: true and values lacking a clientless group, the rendered profile now gains the chart's group (the fallback applies to the force-replace branch too — at 268012a the branch rendered toYaml .Values.webhook.pluginsProfile verbatim; at head it renders the deep-copied, fallback-merged map), so the flag no longer means strict passthrough of webhook.pluginsProfile. Given the clientless.fluid.io rule is unconditional (no values gate exists), rule/plugin consistency is a defensible choice and the truthy empty-group opt-out still works under force-replace; a one-line values.yaml comment would close it. Please confirm this is the intended semantics, or exempt the force-replace branch. Both reviewers agree it is correctly framed as a question, not a defect.
charts/fluid/fluid/templates/webhook/plugins-profile.yaml:17 [nit] — Chart's hard-coded fallback clientless group can silently drift from values.yaml
B-F8: The A-F3 fallback (dict "withDataset" (list "RuntimeConfigInjector") "withoutDataset" (list)) hard-codes and duplicates values.yaml's webhook.pluginsProfile.plugins.clientless. The template comment says to keep them in line, but nothing in the shipped chart's CI enforces it — a future edit to the values.yaml group silently drifts the --reuse-values fallback. A cross-reference comment in values.yaml or a helm-unittest asserting the two render equal would close it.
Verified review delta
A-F5: open (initial-pr)
A-F6: open (initial-pr)
A-F3: fixed (initial-pr)
B-F7: fixed (initial-pr)
A-Q2: open (initial-pr)
B-F8: open (initial-pr)
Code: pending. CI: success; attribution: unknown. Required CI checks still apply.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cheyang
left a comment
Ⅰ. Describe what this PR does
#6176 的阶段二。阶段一（在运行时配置的 ConfigMap 里生成 runtime.sh）已经在 #6191 合进去了，这个 PR 把它送到业务 Pod 里。
像 Mooncake 这样没有 FUSE client 的缓存系统，应用用自己的 SDK 直连 master，业务 Pod 不挂数据集的 PVC，之前只能把 master 地址写死在应用里。现在业务 Pod 加上标签和注解：
webhook 会为注解里的每个数据集，把它的运行时配置 ConfigMap 只读挂到
/etc/fluid/config/<数据集名>/，并注入环境变量FLUID_RUNTIME_CONFIG_PATH_<数据集名>指向里面的 runtime.sh。应用在 entrypoint 里. "$FLUID_RUNTIME_CONFIG_PATH_MOONCAKE_DEMO"之后，就有了$MASTER_NAME、$MASTER_SERVICE_NAME这些变量。三个提交：
feat(webhook)：新插件RuntimeConfigInjector；CollectRuntimeInfosFromAnnotations从注解解析数据集；mutating handler 里加fluid.io/inject的路由，走新的 clientless 插件组，不会触发 FUSE 相关的插件；chart 里的插件组和 webhook 规则。docs：中英文接入指南（怎么声明、注入了什么、runtime.sh 里有哪些变量），Mooncake 示例里加了注入的用法。fix(chart)：webhook 的 ClusterRole 里漏了cacheruntimes，见第 Ⅴ 节。CRD 没有改动。
Ⅱ. Does this pull request fix one issue?
part of #6176
e2e 等 #6175 合了以后，在
test/gha-e2e/mooncake/上加注入相关的检查，另外提。Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/webhook/plugins/runtimeconfiginjector/runtime_config_injector_test.go：-；多个数据集时注入顺序固定（重复跑几次确认）Ⅳ. Describe how to verify it
我在 kind（v1.30）上用这个分支构建的镜像跑了一遍，Mooncake 用的是 #6163 示例里的 CacheRuntimeClass：
clientlessPodWithDatasetHandler装上了RuntimeConfigInjectorFLUID_RUNTIME_CONFIG_PATH_MOONCAKE_DEMO；容器里 source 之后，用${MASTER_NAME}-0.${MASTER_SERVICE_NAME}连上 master，Python client 写入再读回 4 MiB，md5 一致第一次跑的时候业务 Pod 创建超时，查下来是 webhook 没有读 cacheruntimes 的权限，就是第三个提交修的。
按
docs/zh/samples/cacheruntime/mooncake_cache_runtime.md里“可选：让 Fluid 注入运行时配置”一节可以复现。Ⅴ. Special notes for reviews
先说跑集群时发现的 RBAC 问题。webhook 的 ClusterRole 里列了其他所有 runtime，唯独没有
cacheruntimes。base.GetRuntimeInfo碰到 CacheRuntime 数据集要读 CacheRuntime 对象，带缓存的 client 起的 informer 一直 list 不了，Get 就一直等到 15 秒超时，Pod 创建失败。带 FUSE 注入标签、又挂了 CacheRuntime 数据集 PVC 的 Pod 其实也会碰到，只是现有 e2e 的 Pod 都没打这些标签，所以之前没发现。和 #6176 里写的不一样的几处：
fluid.io/datasets，不用 issue 里的fluid.io/dataset。后者已经在transform_worker.go里当作 worker Pod 的 label（做 PodAntiAffinity，值是单个 dataset id），再拿同一个 key 放逗号分隔的列表容易混。CollectRuntimeInfosFromAnnotations里解析，插件只看传进来的 runtimeInfos。issue 的 2.3.3 和 2.3.4 在这点上说法不一样，我按 2.3.4 做，这样能顺带用checkDatasetBound确认数据集已经 Bound，插件也不用知道注解叫什么。-换成_。shell 变量名不能带-，FLUID_RUNTIME_CONFIG_PATH_mooncake-demo没法引用。数据集名里不允许出现_，所以不会有两个数据集撞到同一个变量名。fluid.io/datasets注解时，issue 里写的是直接 return，我改成走 clientless 组的 withoutDataset（values.yaml 里配成空），效果一样，写法和另外两组一致。pkg/common/cacheruntime.go，engine 里原来写死的/etc/fluid/config、runtime.json、runtime.sh、FLUID_RUNTIME_CONFIG_PATH也改成用这些常量，webhook 和 engine 用同一份定义。想听听意见的几点：
ports取，我觉得单开 issue 比较好。done.sidecar.fluid.io/inject。功能上没问题，fluidapp controller 虽然也看这个标记，但只处理带 privileged fuse sidecar 的 Pod，碰不到这里的 Pod。只是这里注入的不是 sidecar，要不要换个名字？MutatingHandler接口，这次没动。另外，runtime.sh 能进业务 Pod 以后，#6175 里
custom-entrypoint.sh的 worker 分支也可以直接 source，不用再装 jq 解析 runtime.json，这个放到 e2e 那个 PR 里一起改。