feat(nas): support access point rrsa authentication - #1613
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iltyty The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XXL
08de0b4 to
e9aa17c
Compare
|
/retest |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
| if op == nil || op.AuthConfig == nil { | ||
| return handler(ctx, op) | ||
| } | ||
| if op.AuthConfig.AuthType != rrsaAuthType && (op.AuthConfig.AccessKey == "" || op.AuthConfig.AccessSecret == "") { | ||
| return handler(ctx, op) | ||
| } |
There was a problem hiding this comment.
Can we combine these two if statements? There doesn't seem to be any difference between them
| return | ||
| } | ||
|
|
||
| if op.AuthConfig != nil && op.AuthConfig.AuthType == rrsaAuthType { |
There was a problem hiding this comment.
Can we move these statements into a switch block? Just emit an error if the credential file doesn't exist.
| RoleArn: tea.String(roleArn), | ||
| RoleSessionName: tea.String(session), | ||
| } | ||
| resp, err := i.stsClient.AssumeRoleWithOIDC(req) |
There was a problem hiding this comment.
Will there be a scenario where a node doesn't have AssumeRoleWithOIDCRequests policys?
| RoleSessionName: tea.String(session), | ||
| } | ||
| resp, err := i.stsClient.AssumeRoleWithOIDC(req) | ||
| if err != nil || resp.Body == nil || resp.Body.Credentials == nil { |
There was a problem hiding this comment.
We should at least log the error, so that we can identify why request failed
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
There was a problem hiding this comment.
Automated Code Review
Summary: Adds RRSA (RAM Role for Service Account) authentication support for NAS access points, including OIDC token exchange via STS and mount-proxy integration for credential injection.
Issues Found:
- The
AssumeRoleWithOIDCmethod was added to theSTSInterface— ensure the mock implementation matches the real STS client signature exactly. - The mounter refactoring (
MountOperation→MountRequestfrommounter/utils) is a good consolidation, but it touches multiple mounter implementations (AdaptorMounter,OssCmdMounter,ConnectorMounter). Ensure all mount paths still work correctly. - The
alinas_secret.gointerceptor file shows a significant rewrite (from 56 lines of context visible). This is the security-sensitive path — ensure credential handling doesn't leak secrets in logs. csi-agent/main.gochanges fromWrapNodeServerWithValidatortoWrapNodeServer— verify this doesn't remove important request validation.
Suggestions:
- The STS token exchange is a security-critical path. Add unit tests covering: successful OIDC exchange, expired token handling, invalid role ARN, and network failures.
- Document the end-to-end flow: ServiceAccount → projected token → STS AssumeRoleWithOIDC → NAS access credentials.
Overall: Significant feature enabling fine-grained NAS access control via RRSA. The mounter refactoring is clean. Security-critical paths need thorough test coverage.
mowangdk
left a comment
mowangdk
left a comment
There was a problem hiding this comment.
Automated Code Review
Summary: Adds NAS access point RRSA authentication support via OIDC token exchange, with a background refresh loop that keeps tokens fresh while the mount point is active.
Overall: Significant feature with good test coverage. Some concurrency and error handling concerns.
| } | ||
|
|
||
| func (i *AlinasSecretInterceptor) startTokenRefreshLoop(op *mounterutils.MountRequest) { | ||
| if op == nil || op.Target == "" || op.AuthConfig == nil { |
There was a problem hiding this comment.
The startTokenRefreshLoop goroutine runs forever with a 5-second ticker polling isMountPoint. This is a high polling frequency for a stat syscall. Consider using a longer interval (e.g., 30s-60s) since token refresh only needs to happen every ~5 minutes anyway, and the mount point check just gates the loop lifetime.
| if err = handler(ctx, op); err != nil { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
go i.startTokenRefreshLoop(op) captures op by pointer. If the caller modifies op after Intercept returns, the goroutine will see the mutations. This is likely fine given the current calling pattern, but it's fragile. Consider copying the relevant fields (VolumeID, Target, AuthConfig) into the goroutine's closure.
| return | ||
| } | ||
|
|
||
| cred := alicred.FromCredentialsProvider(provider.GetProviderName(), provider) |
There was a problem hiding this comment.
newSTSClient returns (nil, nil) when regionID is empty. Callers of NewAlinasSecretInterceptor will succeed, but later calls to refreshRRSAToken check i.stsClient == nil and just log an error without returning it clearly. The interceptor will silently fail token refresh. Consider returning an error from NewAlinasSecretInterceptor when regionID is empty but RRSA is expected, or make the nil-client path more explicit.
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Support NAS access point RRSA authentication.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: