Support for Node-Specific Restrictions in Fluid #4508

xliuqq · 2025-02-19T05:05:50Z

Ⅰ. Describe what this PR does

Support for Node-Specific Restrictions in Fluid，and abstracting the relevant code for node modification using a new interface.

Ⅱ. Does this pull request fix one issue?

fixes #4488

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

Ⅳ. Describe how to verify it

test with k8s version 1.30.

Ⅴ. Special notes for reviews

fluid-e2e-bot · 2025-02-19T05:05:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zwwhdls for approval by writing /assign @zwwhdls 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.

Needs approval from an approver in each of these files:

OWNERS

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

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

sonarqubecloud · 2025-03-02T11:07:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

xliuqq · 2025-03-29T14:19:43Z

@cheyang @TrafalgarZZZ any progress ?

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

sonarqubecloud · 2025-05-18T01:55:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull Request Overview

This PR introduces support for node-specific restrictions in Fluid using Kubernetes 1.30+'s ServiceAccountTokenPodNodeInfo feature gate. The implementation provides an alternative to kubelet config-based node authorization by leveraging node binding tokens with ValidatingAdmissionPolicy for improved security.

Key changes:

Introduces version detection to determine if node binding token support is available (k8s >= 1.30)
Abstracts node operations through a new NodeAuthorizedClient interface with two implementations: restrictedNodeClient (using node binding tokens) and kubeletNodeClient (using kubelet config)
Adds ValidatingAdmissionPolicy resources to enforce node-specific access restrictions when the new method is enabled

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/utils/compatibility/node_restrict.go	Adds version detection logic to check if ServiceAccountTokenPodNodeInfo feature is supported
pkg/csi/plugins/register.go	Refactors client initialization to use the new abstraction and select appropriate authorization method
pkg/csi/plugins/nodeserver.go	Updates node operations to use the new `NodeAuthorizedClient` interface
pkg/csi/plugins/node_resource_operator.go	Defines the `NodeAuthorizedClient` interface and its two implementations
pkg/csi/plugins/driver.go	Updates driver struct to use the new interface type
charts/fluid/fluid/values.yaml	Updates documentation for the `useNodeAuthorization` configuration option
charts/fluid/fluid/templates/role/csi/rbac.yaml	Conditionally grants node permissions based on k8s version and configuration
charts/fluid/fluid/templates/csi/validatingadmissionpolicybinding.yaml	Adds policy binding for node-specific restrictions
charts/fluid/fluid/templates/csi/validatingadmissionpolicy.yaml	Defines validation rules for node access restrictions
charts/fluid/fluid/templates/csi/daemonset.yaml	Conditionally mounts kubelet config based on authorization method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/utils/compatibility/node_restrict.go

pkg/csi/plugins/nodeserver.go

charts/fluid/fluid/templates/csi/validatingadmissionpolicy.yaml

Signed-off-by: xliuqq <xlzq1992@gmail.com> Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>

sonarqubecloud · 2025-10-30T04:45:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

codecov · 2025-10-30T04:57:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.75%. Comparing base (930dc8d) to head (07e566f).
⚠️ Report is 385 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4508      +/-   ##
==========================================
+ Coverage   58.22%   58.75%   +0.52%     
==========================================
  Files         565      564       -1     
  Lines       30966    31062      +96     
==========================================
+ Hits        18030    18250     +220     
+ Misses      10744    10652      -92     
+ Partials     2192     2160      -32

☔ View full report in Codecov by Sentry.
📢 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T05:50:25Z

pkg/utils/compatibility/node_restrict.go

+// Beta release, default enabled. see https://github.com/kubernetes/enhancements/issues/4193
+const nodeBindingTokenSupportedVersion = "v1.30.0"
+
+// Checks the ServiceAccountTokenPodNodeInfo feature gate, whether the apiserver embeds the node name for the associated node when issuing service account tokens bound to Pod objects.


The comment is missing proper formatting. Go convention requires comments for exported functions to start with the function name. This comment should start with 'discoverNodeBindingTokenCompatibility checks...' However, since this function is unexported, consider making this comment more concise or moving it above the exported IsNodeBindingTokenSupported function where it would be more appropriate.

Copilot · 2025-11-06T05:50:25Z

pkg/utils/compatibility/node_restrict.go

+	discoveryClient := discovery.NewDiscoveryClientForConfigOrDie(restConfig)
+
+	serverVersion, err := discoveryClient.ServerVersion()
+	if err != nil && !errors.IsNotFound(err) {


The error handling logic is incorrect. If ServerVersion() returns an error (whether IsNotFound or any other error), serverVersion will be nil, causing a nil pointer dereference on line 49 when accessing serverVersion.GitVersion. The code should fatal on any error, not just non-NotFound errors. Change the condition to if err != nil to properly handle all error cases.

Suggested change

if err != nil && !errors.IsNotFound(err) {

if err != nil {

Copilot · 2025-11-06T05:50:25Z

pkg/csi/plugins/register.go

+// isUseKubeletConfig checks if the specified Kubelet configuration file exists. If the file does not exist, it returns an empty client without an error .
 // If the file exists, it attempts to initialize and return a node-authorized Kubernetes client.


The function documentation is inaccurate and contains issues: (1) There's an extra space before the period in 'without an error .', (2) The comment describes returning 'an empty client' but the function returns a bool, not a client, (3) The comment mentions 'attempts to initialize and return' but the function only checks file existence. Update the comment to accurately describe what the function does: 'isUseKubeletConfig checks if the specified Kubelet configuration file exists and returns true if it does, false otherwise.'

Suggested change

// isUseKubeletConfig checks if the specified Kubelet configuration file exists. If the file does not exist, it returns an empty client without an error .

// If the file exists, it attempts to initialize and return a node-authorized Kubernetes client.

// isUseKubeletConfig checks if the specified Kubelet configuration file exists and returns true if it does, false otherwise.

Copilot · 2025-11-06T05:50:25Z

pkg/csi/plugins/register.go

+		glog.Warningf("fail to stat kubelet config file %s", kubeletConfigPath)
 	}



When os.Stat fails with an error other than IsNotExist, the function logs a warning but continues execution and returns true on line 41. This means if there's a permission error or any other filesystem issue, the code will incorrectly assume the kubelet config should be used, leading to a failure when trying to initialize the client later. The function should either return false or include the error in the warning message with proper handling context.

Suggested change

glog.Warningf("fail to stat kubelet config file %s", kubeletConfigPath)

}

glog.Warningf("fail to stat kubelet config file %s: %v, continue without node authorization...", kubeletConfigPath, err)

return false

}

Copilot · 2025-11-06T05:50:26Z

pkg/csi/plugins/node_resource_operator.go

+	err := p.Client.Patch(context.TODO(), node, client.RawPatch(patchType, data))
+	return err
+}


[nitpick] The intermediate variable err is unnecessary. Return the error directly: return p.Client.Patch(context.TODO(), node, client.RawPatch(patchType, data))

Suggested change

err := p.Client.Patch(context.TODO(), node, client.RawPatch(patchType, data))

return err

}

return p.Client.Patch(context.TODO(), node, client.RawPatch(patchType, data))

}

}

Copilot · 2025-11-06T05:50:26Z

charts/fluid/fluid/values.yaml

  # See why Fluid's CSI Plugins need node-specific authorization at https://github.com/fluid-cloudnative/fluid/security/advisories/GHSA-93xx-cvmc-9w3v
  # See node authorization at https://kubernetes.io/docs/reference/access-authn-authz/node/
+  # If false, use NodeBinding Token with ValidatingAdmissionPolicy instead of kubelet config for Node-Specific Restrictions.
+  # can only be set false when k8s.version >= 1.30 and the below kubelet.kubeConfigFile is useless.


Line 65 has grammatical issues and unclear meaning. 'can only be set false' should be 'Can only be set to false'. Also, 'useless' is informal; consider 'not required' or 'ignored'. Suggested revision: 'Can only be set to false when k8s.version >= 1.30, and the kubelet.kubeConfigFile setting below will be ignored.'

Suggested change

# can only be set false when k8s.version >= 1.30 and the below kubelet.kubeConfigFile is useless.

# Can only be set to false when k8s.version >= 1.30, and the kubelet.kubeConfigFile setting below will be ignored.

cheyang requested a review from TrafalgarZZZ February 23, 2025 05:42

abstracting the relevant code for node modification using a new inter…

8e4c7ac

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

xliuqq force-pushed the policy branch from 5605c1a to 211cf53 Compare March 2, 2025 11:05

xliuqq requested a review from cheyang March 13, 2025 11:10

fix conflict with master

c89d18f

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

xliuqq force-pushed the policy branch from 211cf53 to c89d18f Compare May 18, 2025 01:54

RongGu requested a review from Copilot October 26, 2025 16:12

Copilot AI reviewed Oct 26, 2025

View reviewed changes

pkg/utils/compatibility/node_restrict.go Outdated Show resolved Hide resolved

pkg/csi/plugins/nodeserver.go Outdated Show resolved Hide resolved

charts/fluid/fluid/templates/csi/validatingadmissionpolicy.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

07e566f

Signed-off-by: xliuqq <xlzq1992@gmail.com> Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>

xliuqq force-pushed the policy branch from f3f9c27 to 07e566f Compare October 30, 2025 04:43

RongGu requested a review from Copilot November 6, 2025 05:42

Copilot AI reviewed Nov 6, 2025

View reviewed changes

		// isUseKubeletConfig checks if the specified Kubelet configuration file exists. If the file does not exist, it returns an empty client without an error .
		// If the file exists, it attempts to initialize and return a node-authorized Kubernetes client.

		glog.Warningf("fail to stat kubelet config file %s", kubeletConfigPath)
		}

-		glog.Warningf("fail to stat kubelet config file %s", kubeletConfigPath)
-	}
+		glog.Warningf("fail to stat kubelet config file %s: %v, continue without node authorization...", kubeletConfigPath, err)
+		return false
+	}

	# can only be set false when k8s.version >= 1.30 and the below kubelet.kubeConfigFile is useless.
	# Can only be set to false when k8s.version >= 1.30, and the kubelet.kubeConfigFile setting below will be ignored.

Support for Node-Specific Restrictions in Fluid #4508

Are you sure you want to change the base?

Support for Node-Specific Restrictions in Fluid #4508

Uh oh!

Conversation

xliuqq commented Feb 19, 2025

Ⅰ. Describe what this PR does

Ⅱ. 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

Ⅴ. Special notes for reviews

Uh oh!

fluid-e2e-bot bot commented Feb 19, 2025

Uh oh!

sonarqubecloud bot commented Mar 2, 2025

Quality Gate passed

Uh oh!

xliuqq commented Mar 29, 2025

Uh oh!

sonarqubecloud bot commented May 18, 2025

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 30, 2025

Quality Gate passed

Uh oh!

codecov bot commented Oct 30, 2025

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant