-
Notifications
You must be signed in to change notification settings - Fork 214
[RFC-0010] Add multi-tenant workload identity support for AWS Bucket #1868
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
base: main
Are you sure you want to change the base?
[RFC-0010] Add multi-tenant workload identity support for AWS Bucket #1868
Conversation
d550312
to
3e0213c
Compare
Signed-off-by: cappyzawa <[email protected]>
3e0213c
to
3300705
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.
Looking good overall, thanks!
internal/bucket/minio/minio.go
Outdated
|
||
// Set AWS_REGION environment variable from bucket.Spec.Region for pkg/auth/aws | ||
if opts.STSRegion != "" { | ||
os.Setenv("AWS_REGION", opts.STSRegion) |
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.
os.Setenv("AWS_REGION", opts.STSRegion) |
I see that this is already being correctly configured with auth.WithSTSRegion()
in bucket_controller.go
, why is this here too?
(By the way, we should never do something like this, setting an environment variable in the middle of controller code, as there are reconciliations running in parallel and this would leak to other tenants.)
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.
The reason this is here is because pkg/auth/aws
internally uses config.LoadDefaultConfig()
, which requires the AWS_REGION
environment variable when no region is explicitly provided through other means. Even though we pass the region via auth.WithSTSRegion()
, it seems the AWS SDK still looks for the environment variable in some cases.
I was focused on ensuring existing users with .spec.STS
configurations could upgrade seamlessly, but I completely overlooked the multi-tenant implications 😮💨 You're right that this approach is problematic.
I think the proper fix would be to modify pkg/auth/aws
to prioritize the region passed through auth.WithSTSRegion()
over the environment variable. Would you be open to this approach? I could submit a PR to the pkg repository to fix this at the source rather than working around it here.
This would eliminate the need for os.Setenv()
entirely and make the implementation cleaner and safer for multi-tenant environments.
I’d like to revisit how the current implementation works 🙇
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.
Upon further reflection, I realize the core issue: there was no Controller-Level authentication previously.
When no Secret was specified, authentication was handled at the Object level (per-Bucket) using each Bucket's specific IAM authentication (leveraging the individual Bucket's region and STS configuration).
Introducing Controller-Level Workload Identity at this point creates an overlap with the existing Object-level IAM authentication conditions. This means existing test cases that expected Object-level behavior are now failing due to Controller-Level WI attempting to authenticate without the required AWS_REGION
environment variable.
This makes it difficult to introduce Controller-Level WI without regressions.
I've updated the implementation to defer Controller-Level Workload Identity and focus on:
1. Object-Level Workload Identity (when serviceAccountName is specified)
2. Traditional Object-level AWS credential chain (preserving existing behavior)
This approach maintains backward compatibility while delivering the most requested feature.
Question: What are your thoughts on Controller-Level Workload Identity support? Any ideas on how to introduce it safely without breaking existing users?
docs/spec/v1/buckets.md
Outdated
@@ -273,6 +273,55 @@ data: | |||
secretkey: <BASE64> | |||
``` | |||
|
|||
##### AWS Controller-Level Workload Identity example |
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.
Let's mention specifically https://fluxcd.io/flux/integrations/aws/#for-amazon-simple-storage-service as well like we did for GCP
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 think we should add the link eventually. However, the current documentation at that URL (specifically the "At the object level" section) states that S3 integration with the Bucket
API "does not support configuring authentication through OIDC Federation at the object level" and will be introduced in Flux v2.7.
Since this PR is implementing that functionality, I'll include the link in this PR and update the website documentation afterward to reflect the new capabilities. This way users will have the reference ready once the website is updated.
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.
Fixed via 794228f .
…Bucket Signed-off-by: cappyzawa <[email protected]>
3d49525
to
794228f
Compare
…Bucket Signed-off-by: cappyzawa <[email protected]>
53c6940
to
9fe7486
Compare
…Bucket Signed-off-by: cappyzawa <[email protected]>
9fe7486
to
ef755b2
Compare
Part of: fluxcd/flux2#5022