-
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?
Changes from 1 commit
3300705
794228f
1b1fc21
ef755b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -23,13 +23,17 @@ import ( | |||
"fmt" | ||||
"net/http" | ||||
"net/url" | ||||
"os" | ||||
"strings" | ||||
|
||||
"github.com/minio/minio-go/v7" | ||||
"github.com/minio/minio-go/v7/pkg/credentials" | ||||
"github.com/minio/minio-go/v7/pkg/s3utils" | ||||
corev1 "k8s.io/api/core/v1" | ||||
|
||||
"github.com/fluxcd/pkg/auth" | ||||
awsauth "github.com/fluxcd/pkg/auth/aws" | ||||
|
||||
sourcev1 "github.com/fluxcd/source-controller/api/v1" | ||||
) | ||||
|
||||
|
@@ -46,6 +50,7 @@ type options struct { | |||
tlsConfig *tls.Config | ||||
stsTLSConfig *tls.Config | ||||
proxyURL *url.URL | ||||
authOpts []auth.Option | ||||
} | ||||
|
||||
// Option is a function that configures the Minio client. | ||||
|
@@ -86,8 +91,15 @@ func WithSTSTLSConfig(tlsConfig *tls.Config) Option { | |||
} | ||||
} | ||||
|
||||
// WithAuth sets the auth options for workload identity authentication. | ||||
func WithAuth(authOpts ...auth.Option) Option { | ||||
return func(o *options) { | ||||
o.authOpts = authOpts | ||||
} | ||||
} | ||||
|
||||
// NewClient creates a new Minio storage client. | ||||
func NewClient(bucket *sourcev1.Bucket, opts ...Option) (*MinioClient, error) { | ||||
func NewClient(ctx context.Context, bucket *sourcev1.Bucket, opts ...Option) (*MinioClient, error) { | ||||
var o options | ||||
for _, opt := range opts { | ||||
opt(&o) | ||||
|
@@ -105,7 +117,11 @@ func NewClient(bucket *sourcev1.Bucket, opts ...Option) (*MinioClient, error) { | |||
case o.secret != nil: | ||||
minioOpts.Creds = newCredsFromSecret(o.secret) | ||||
case bucketProvider == sourcev1.BucketProviderAmazon: | ||||
minioOpts.Creds = newAWSCreds(bucket, o.proxyURL) | ||||
creds, err := newAWSCreds(ctx, &o) | ||||
if err != nil { | ||||
return nil, err | ||||
} | ||||
minioOpts.Creds = creds | ||||
case bucketProvider == sourcev1.BucketProviderGeneric: | ||||
minioOpts.Creds = newGenericCreds(bucket, &o) | ||||
} | ||||
|
@@ -159,23 +175,35 @@ func newCredsFromSecret(secret *corev1.Secret) *credentials.Credentials { | |||
} | ||||
|
||||
// newAWSCreds creates a new Minio credentials object for `aws` bucket provider. | ||||
func newAWSCreds(bucket *sourcev1.Bucket, proxyURL *url.URL) *credentials.Credentials { | ||||
stsEndpoint := "" | ||||
if sts := bucket.Spec.STS; sts != nil { | ||||
stsEndpoint = sts.Endpoint | ||||
} | ||||
|
||||
creds := credentials.NewIAM(stsEndpoint) | ||||
if proxyURL != nil { | ||||
transport := http.DefaultTransport.(*http.Transport).Clone() | ||||
transport.Proxy = http.ProxyURL(proxyURL) | ||||
client := &http.Client{Transport: transport} | ||||
creds = credentials.New(&credentials.IAM{ | ||||
Client: client, | ||||
Endpoint: stsEndpoint, | ||||
}) | ||||
// | ||||
// This function is only called when Secret authentication is not available. | ||||
// | ||||
// Uses AWS SDK's config.LoadDefaultConfig() which supports: | ||||
// - Workload Identity (IRSA/EKS Pod Identity) | ||||
// - EC2 instance profiles | ||||
// - Environment variables | ||||
// - Shared credentials files | ||||
// - All other AWS SDK authentication methods | ||||
func newAWSCreds(ctx context.Context, o *options) (*credentials.Credentials, error) { | ||||
var opts auth.Options | ||||
opts.Apply(o.authOpts...) | ||||
|
||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I see that this is already being correctly configured with (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 commentThe reason will be displayed to describe this comment to others. Learn more. The reason this is here is because I was focused on ensuring existing users with
I’d like to revisit how the current implementation works 🙇 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
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? |
||||
} | ||||
|
||||
awsCredsProvider := awsauth.NewCredentialsProvider(ctx, o.authOpts...) | ||||
awsCreds, err := awsCredsProvider.Retrieve(ctx) | ||||
if err != nil { | ||||
return nil, fmt.Errorf("AWS authentication failed: %w", err) | ||||
} | ||||
return creds | ||||
|
||||
return credentials.NewStaticV4( | ||||
awsCreds.AccessKeyID, | ||||
awsCreds.SecretAccessKey, | ||||
awsCreds.SessionToken, | ||||
), nil | ||||
} | ||||
|
||||
// newGenericCreds creates a new Minio credentials object for the `generic` bucket 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.
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 .