-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: containerized ossfs auth enhanced #927
feat: containerized ossfs auth enhanced #927
Conversation
40f2bf3
to
2537cf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use OIDC in an non-ACK cluster? It is Open ID connect anyway.
afd58be
to
62b7f22
Compare
IMO, it is too customized and will bring extra params. if users have no strong demand on RRSA, why not use IBMIAM auth with ossfs mount options directly instead? |
I think we can just replace the clusterId parameter with full PROVIDER_ARN. We can still build this ARN for ACK user at installation. |
62b7f22
to
c71b526
Compare
c71b526
to
f259dde
Compare
f259dde
to
1d1f7e8
Compare
1d1f7e8
to
4af1b0e
Compare
current version:
|
/retest-required |
d817e61
to
77b1b8a
Compare
pkg/mounter/helper.go
Outdated
|
||
// GetOIDCProvider get OIDC provider from env or ACK clusterId for RRSA | ||
func GetOIDCProvider() (provider string, err error) { | ||
provider = os.Getenv("OIDC_PROVIDER") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still would like this one to override the full ARN, and can be called "ALIBABA_CLOUD_OIDC_PROVIDER_ARN" for consistency. Can't a user potentially use a provider from others account?
And should this be a parameter in storage class? Maybe we can support multiple providers for different PV, or even different CSI drivers. Or CSI itself may gain OIDC capability in the future. Put it here can be ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If role_arn or provider_arn does not conform to this fixed format, it means that the user is trying to access the OSS from other cloud vendor through RRSA. But ossfs itself does not have the ability to load external authentication libraries (for example, awscred-lib for IRSA). CSI can accept a full arn but it will still not work in ossfs.
And for multiple providers, I think supporting multiple roles is enough. @mowangdk PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the user may also want to override the account ID part of ARN. Consider that user A may want to access an OSS bucket from account B. User B then adds the ACK cluster in account A to his OIDC provider, then user A will need to specify the provider from account B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sts will return error like "acs::ram::111111:oidc-provider/aaaa must be your AccountID, your AccountID is 2222"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested that I can register ACK cluster in account A to OIDC provider in account B. Then everything else should only relate to account B. Which step (which OpenAPI) would fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please give a detailed test case and the AssumeRoleWithOIDC related logs of ossfs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about ossfs. But I've tried my proposed flow, it works and I got token from AssumeRoleWithOIDC.
- Login account A, create an ACK cluster, enable RRSA, copy the provider URL
- Create a Pod in above ACK cluster, mount this volume, and copy the token in it.
- name: rrsa-oidc-token projected: defaultMode: 0600 sources: - serviceAccountToken: audience: sts.aliyuncs.com expirationSeconds: 3600 path: token
- Login account B, go to https://ram.console.aliyun.com/providers/create/OIDC and register an OIDC provider with URL from step 1, copy the provider ARN
- create a RAM role for OIDC, copy the role ARN
- go to https://next.api.aliyun.com/api/Sts/2015-04-01/AssumeRoleWithOIDC, fill in token from step 2, and ARNs from step 3 and 4, I get RAM sts token.
These steps should naturally apply to ossfs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overvall it is OK.
Although I reserve my opinion about the RoleARN/Name. I think API should be minimal. To use only RoleARN and not RoleName as parameter:
pros:
- reduce the possibility of invalid input. simpler implementation, less surprise
- fewer things for user to understand. Especially for those who use both default and non-default provider.
- closer to our OIDC OpenAPI
cons:
- harder for users who just want to use OSS without AK. They should find and copy the ARN from the web console, instead of just typing in the role name.
- storage class yaml is a little harder to read if one don't know about role ARN.
20ef114
to
2db75ec
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlbeeSo, huww98, mowangdk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
support RRSA & csi-secret-store authType for containerized ossfs.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
additional rbac: serviceaccount/csi-fuse-ossfs get
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: