feat(csi): add no-credentials (anonymous) connection mode#89
Open
ankitmaster08 wants to merge 1 commit into
Open
feat(csi): add no-credentials (anonymous) connection mode#89ankitmaster08 wants to merge 1 commit into
ankitmaster08 wants to merge 1 commit into
Conversation
Add authType=none (alias anonymous) to the CSI driver: no Secret, no IRSA, and no AWS env vars are injected, and per-workload identity is bypassed. For S3 the generated msfs.yaml sets anonymous: true; for AIStore the token is left empty (anonymous access, e.g. a local AIS cluster). Wire S3 anonymous support in the core: a new S3 "anonymous" config field (the parser skips the access-key requirement) drives aws.AnonymousCredentials so the SDK issues unsigned requests for public / no-auth endpoints. The field is rejected for change via SIGHUP, consistent with the other S3 fields. Adds unit tests (CSI none-mode resolution / config rendering / env, and anonymous S3 config parsing) and documents the mode in the CSI README and Helm chart. NGCDP-9029
📝 WalkthroughWalkthroughAdds a new ChangesAnonymous/none credential mode for S3
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@multi-storage-file-system/config.go`:
- Around line 1776-1783: The anonymous branch in the S3 config normalization
leaves useCredentialsEnv enabled even after clearing credentials, so update the
same backendConfigS3AsStruct handling to force useCredentialsEnv false whenever
anonymous is true. Make this change in the logic around
anonymous/useCredentialsEnv so setupS3Context and SIGHUP validation no longer
treat credential-env settings as active for anonymous mode.
In `@multi-storage-file-system/csi/pkg/driver/node_test.go`:
- Around line 193-215: Add a regression test in
TestWriteConfig_NoneModeAIStoreOmitsAnonymousAndCreds that populates
aisAuthnToken and aisAuthnTokenFile while using credentialModeNone, and assert
that writeConfig does not emit authn_token or authn_token_file for AIStore. Then
update writeConfig in node.go so the AIStore path skips both token fields when
authType is none, matching the none-mode contract and the existing
anonymous/token omission behavior in the test.
In `@multi-storage-file-system/csi/pkg/driver/node.go`:
- Around line 523-528: The credentialModeNone path still lets inherited
driver-pod AWS identity variables leak into the child process because buildEnv
returns os.Environ() unchanged. Update buildEnv to add an explicit none-mode
branch that strips AWS credential/IRSA-related environment variables before
returning, and use the existing credentialModeNone and buildEnv symbols to
locate the change.
🪄 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: Enterprise
Run ID: b0d95ca4-2992-4aa2-8a39-99a2942d79ee
📒 Files selected for processing (9)
multi-storage-file-system/backend_s3.gomulti-storage-file-system/config.gomulti-storage-file-system/config_test.gomulti-storage-file-system/csi/README.mdmulti-storage-file-system/csi/charts/msfs-csi/README.mdmulti-storage-file-system/csi/charts/msfs-csi/values.yamlmulti-storage-file-system/csi/pkg/driver/node.gomulti-storage-file-system/csi/pkg/driver/node_test.gomulti-storage-file-system/globals.go
Comment on lines
+1776
to
+1783
| if backendConfigS3AsStruct.anonymous { | ||
| // Anonymous access: no credentials are used (unsigned requests). | ||
| // Ignore any credentials file / access keys so a public or | ||
| // no-auth endpoint can be configured without them. | ||
| backendConfigS3AsStruct.credentialsFilePath = "" | ||
| backendConfigS3AsStruct.accessKeyID = "" | ||
| backendConfigS3AsStruct.secretAccessKey = "" | ||
| } else if backendConfigS3AsStruct.useCredentialsEnv { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize useCredentialsEnv when anonymous is enabled.
This branch clears credential values, but leaves useCredentialsEnv true if the config set both flags. setupS3Context still observes that flag when adding shared-profile options, and SIGHUP validation can reject changes to a value that anonymous mode is supposed to ignore.
Proposed fix
if backendConfigS3AsStruct.anonymous {
// Anonymous access: no credentials are used (unsigned requests).
// Ignore any credentials file / access keys so a public or
// no-auth endpoint can be configured without them.
+ backendConfigS3AsStruct.useCredentialsEnv = false
backendConfigS3AsStruct.credentialsFilePath = ""
backendConfigS3AsStruct.accessKeyID = ""
backendConfigS3AsStruct.secretAccessKey = ""
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if backendConfigS3AsStruct.anonymous { | |
| // Anonymous access: no credentials are used (unsigned requests). | |
| // Ignore any credentials file / access keys so a public or | |
| // no-auth endpoint can be configured without them. | |
| backendConfigS3AsStruct.credentialsFilePath = "" | |
| backendConfigS3AsStruct.accessKeyID = "" | |
| backendConfigS3AsStruct.secretAccessKey = "" | |
| } else if backendConfigS3AsStruct.useCredentialsEnv { | |
| if backendConfigS3AsStruct.anonymous { | |
| // Anonymous access: no credentials are used (unsigned requests). | |
| // Ignore any credentials file / access keys so a public or | |
| // no-auth endpoint can be configured without them. | |
| backendConfigS3AsStruct.useCredentialsEnv = false | |
| backendConfigS3AsStruct.credentialsFilePath = "" | |
| backendConfigS3AsStruct.accessKeyID = "" | |
| backendConfigS3AsStruct.secretAccessKey = "" | |
| } else if backendConfigS3AsStruct.useCredentialsEnv { |
🤖 Prompt for 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.
In `@multi-storage-file-system/config.go` around lines 1776 - 1783, The anonymous
branch in the S3 config normalization leaves useCredentialsEnv enabled even
after clearing credentials, so update the same backendConfigS3AsStruct handling
to force useCredentialsEnv false whenever anonymous is true. Make this change in
the logic around anonymous/useCredentialsEnv so setupS3Context and SIGHUP
validation no longer treat credential-env settings as active for anonymous mode.
Comment on lines
+193
to
+215
| func TestWriteConfig_NoneModeAIStoreOmitsAnonymousAndCreds(t *testing.T) { | ||
| ns := newNodeServer("node-test", "/usr/local/bin/msfs") | ||
| dir, configPath := writeConfigOrFatal(t, ns, | ||
| map[string]string{ | ||
| "backendType": "AIStore", | ||
| "bucketName": "ais-bucket", | ||
| "aisEndpoint": "https://ais.example.com", | ||
| }, | ||
| map[string]string{}, | ||
| credentialModeNone, | ||
| ) | ||
| defer os.RemoveAll(dir) | ||
|
|
||
| body := readFileOrFatal(t, configPath) | ||
| if !strings.Contains(body, "backend_type: AIStore") { | ||
| t.Fatalf("expected AIStore backend; got:\n%s", body) | ||
| } | ||
| // anonymous is an S3-only field; for AIStore, no-credentials means an empty | ||
| // token, so neither an anonymous flag nor a token should be emitted. | ||
| if strings.Contains(body, "anonymous:") || strings.Contains(body, "authn_token") { | ||
| t.Fatalf("none mode AIStore config must not emit anonymous/token; got:\n%s", body) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a regression for AIStore token inputs in none mode.
This only tests the empty-attributes case. multi-storage-file-system/csi/pkg/driver/node.go:477-491 still writes authn_token / authn_token_file whenever those volume attributes are present, so authType=none can still send AIStore credentials even though the documented contract says the token is left empty. Please cover the case with aisAuthnToken and aisAuthnTokenFile populated, and update writeConfig to ignore them in none mode.
🤖 Prompt for 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.
In `@multi-storage-file-system/csi/pkg/driver/node_test.go` around lines 193 -
215, Add a regression test in
TestWriteConfig_NoneModeAIStoreOmitsAnonymousAndCreds that populates
aisAuthnToken and aisAuthnTokenFile while using credentialModeNone, and assert
that writeConfig does not emit authn_token or authn_token_file for AIStore. Then
update writeConfig in node.go so the AIStore path skips both token fields when
authType is none, matching the none-mode contract and the existing
anonymous/token omission behavior in the test.
Comment on lines
+523
to
+528
| // It returns ("", "", nil) when per-workload IRSA does not apply — static or | ||
| // no-credentials mode, a non-S3 backend, or no workload token in volume_context | ||
| // — so the caller keeps today's driver-SA behavior. | ||
| func (ns *nodeServer) resolveWorkloadIdentity(configDir string, volCtx map[string]string, mode credentialMode) (string, string, error) { | ||
| if mode == credentialModeStatic { | ||
| // Static and no-credentials mounts never use a projected workload token. | ||
| if mode == credentialModeStatic || mode == credentialModeNone { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strip inherited AWS identity vars in none mode.
credentialModeNone skips workload identity here, but buildEnv still returns os.Environ() unchanged, so driver-pod AWS_ROLE_ARN / AWS_WEB_IDENTITY_TOKEN_FILE can still reach the msfs child. Add an explicit none-mode branch in buildEnv that removes AWS credential/IRSA variables before returning.
🤖 Prompt for 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.
In `@multi-storage-file-system/csi/pkg/driver/node.go` around lines 523 - 528, The
credentialModeNone path still lets inherited driver-pod AWS identity variables
leak into the child process because buildEnv returns os.Environ() unchanged.
Update buildEnv to add an explicit none-mode branch that strips AWS
credential/IRSA-related environment variables before returning, and use the
existing credentialModeNone and buildEnv symbols to locate the change.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Adds an opt-in no-credentials / anonymous connection mode to the MSFS CSI driver and the core S3 backend (NGCDP-9029; follow-up to #84). This is Shane's clarified "empty secret" ask — connect with no creds of any kind (no IRSA, no access keys), e.g. a local AIS cluster or a public S3 bucket.
volumeAttributes.authType: none(aliasanonymous) → no Secret, no IRSA, no AWS env vars injected, per-workload identity bypassed. S3 mounts emitanonymous: true; AIStore mounts are left with an empty token.anonymousconfig field; the parser no longer requiresaccess_key_id/secret_access_keywhen it is set, andsetupS3Contextinstallsaws.AnonymousCredentials{}so requests are unsigned. Rejected for live change via SIGHUP, consistent with the other S3 fields.auto/static/irsabehavior is unchanged.Test plan
go build/go vet/gofmtclean (CSI + MSFS modules)none/anonymousresolution, S3 emitsanonymous: true, AIStore omits token/anonymous,buildEnvinjects nothing, workload-identity no-opauthType: noneTracked by NGCDP-9029.
Summary by CodeRabbit
New Features
noneandanonymousalongside existing credential modes.Bug Fixes
Documentation