Skip to content

Use feature gates for host pid - #6072

Merged
RongGu merged 4 commits into
fluid-cloudnative:masterfrom
xliuqq:cve
Jul 4, 2026
Merged

Use feature gates for host pid #6072
RongGu merged 4 commits into
fluid-cloudnative:masterfrom
xliuqq:cve

Conversation

@xliuqq

@xliuqq xliuqq commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Ⅰ. Describe what this PR does

Add a RuntimeFuseHostPID feature gate to control whether the runtime.fluid.io/fuse.hostpid annotation on Runtime CRs can enable hostPID: true in FUSE DaemonSets. Each runtime engine has its own featureGates configuration in Helm values, allowing administrators to enable/disable this feature per engine. Defaults to false (disabled) for security.

Ⅱ. Does this pull request fix one issue?

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

  • Unit tests for HostPIDEnabled with feature gate on/off
  • Verify all 5 runtime controllers using hostPid register the flag
  • Verify Helm values and templates for each engine

xliuqq added 3 commits June 28, 2026 16:55
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
@xliuqq
xliuqq requested a review from cheyang June 28, 2026 09:36

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new feature gate, RuntimeFuseHostPID, which controls whether the runtime.fluid.io/fuse.hostpid annotation can enable hostPID in FUSE DaemonSets. It adds --feature-gates command-line flags to alluxio, jindo, juicefs, thin, and vineyard runtime controllers, registers the feature gate flag in the controller binaries, and updates the Helm chart templates and values.yaml accordingly. The feedback suggests wrapping the --feature-gates flag in conditional Helm blocks for all controllers to prevent rendering empty flags when the feature gate value is undefined, which could cause parsing errors or container startup failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread charts/fluid/fluid/templates/controller/thinruntime_controller.yaml
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.77%. Comparing base (795a7a1) to head (f0da9ee).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
cmd/alluxio/app/alluxio.go 0.00% 1 Missing ⚠️
cmd/jindo/app/jindo.go 0.00% 1 Missing ⚠️
cmd/juicefs/app/juicefs.go 0.00% 1 Missing ⚠️
cmd/thin/app/thin.go 0.00% 1 Missing ⚠️
cmd/vineyard/app/vineyard.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6072      +/-   ##
==========================================
- Coverage   64.77%   64.77%   -0.01%     
==========================================
  Files         484      485       +1     
  Lines       33892    33902      +10     
==========================================
+ Hits        21954    21959       +5     
- Misses      10215    10220       +5     
  Partials     1723     1723              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/common/hostpid_test.go
Comment thread pkg/common/hostpid.go

// HostPIDEnabled check if HostPID is true for runtime fuse pod.
func HostPIDEnabled(annotations map[string]string) bool {
if !utilfeature.DefaultFeatureGate.Enabled(features.RuntimeFuseHostPID) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After this PR, any cluster that today relies on runtime.fluid.io/fuse.hostpid: "true" to start FUSE pods with hostPID will silently stop doing so on upgrade unless the operator also flips RuntimeFuseHostPID=true via the Helm chart. The change is intentional (this is the CVE mitigation) and the secure default is correct, but please make sure a release note / upgrade-notes entry calls this out prominently so existing users aren't surprised. Linking the CVE ID in the PR description would also help downstream consumers triage faster.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

where to document it ? what's the CVE ID?

Comment thread charts/fluid/fluid/values.yaml
}

func init() {
runtime.Must(utilfeature.DefaultMutableFeatureGate.Add(defaultFeatureGates))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Each runtime controller (alluxio, jindo, juicefs, thin, vineyard) blank-imports pkg/common/features and exposes the same --feature-gates flag with an identical default. That works fine, but if more runtime-shared gates land later, the operator-facing surface multiplies (one flag per binary). Did you consider keeping a single shared gates package with a registration helper, or is the per-controller binding intentional so each runtime can ship divergent gates down the road? Not blocking — just want to understand the design intent before more gates pile on.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The current design is reasonable because:

  • Fluid's runtimes are truly independent components, each evolving at its own pace.
  • In terms of Feature Gate semantics, each runtime controller is deployment-isolated (different Deployments, different Helm values keys).
  • If there truly is a cross-runtime shared gate in the future, it can be defined once in pkg/common/features, and each runtime continues to register it independently (only one definition needed, not a shared registration entry point).

…ature gate test recoverable

Signed-off-by: xliuqq <xlzq1992@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

@cheyang

cheyang commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Re-reviewed at f0da9ee. Thanks for the quick follow-up — the prior notes are addressed:

  • Helm templates now guard --feature-gates rendering with {{- if .Values.runtime.<engine>.featureGates }} across all five controllers, so empty values no longer emit a bare --feature-gates= flag.
  • pkg/common/hostpid_test.go introduces a setFeatureGate helper that snapshots the current value and restores it via DeferCleanup, removing the residual global state between contexts.
  • Every values.yaml entry documents the gate as Alpha ("Alpha; may change or be removed in future releases."), and the PR description's "Special notes for reviews" already calls out the default-behavior change as a release note.

No new blockers from my side. Heads-up: kind-e2e-test (v1.33.2) failed on this push; treating it as a flake and re-triggering the workflow. Will keep an eye on the rerun.

@xliuqq
xliuqq requested a review from cheyang July 1, 2026 00:11
@cheyang
cheyang requested review from Syspretor and TrafalgarZZZ July 3, 2026 15:15

@RongGu RongGu 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
/approve

@fluid-e2e-bot

fluid-e2e-bot Bot commented Jul 4, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RongGu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@RongGu
RongGu merged commit b3c0e8e into fluid-cloudnative:master Jul 4, 2026
26 of 28 checks passed
@xliuqq
xliuqq deleted the cve branch July 6, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants