feat(repository): role assumption for S3 storage #4182
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4182 +/- ##
==========================================
+ Coverage 75.86% 76.03% +0.17%
==========================================
Files 470 503 +33
Lines 37301 38709 +1408
==========================================
+ Hits 28299 29434 +1135
- Misses 7071 7317 +246
- Partials 1931 1958 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
repo/blob/s3/s3_assume_role.go
Outdated
| awscreds "github.com/aws/aws-sdk-go/aws/credentials" | ||
| "github.com/aws/aws-sdk-go/aws/credentials/stscreds" | ||
| "github.com/aws/aws-sdk-go/aws/session" | ||
| "github.com/aws/aws-sdk-go/service/sts" |
There was a problem hiding this comment.
@ashmrtn Is there a way to avoid taking a dependency on the AWS SDK (v1)? There was a prior cleanup effort to remove that dependency.
There was a problem hiding this comment.
to clarify, are you asking to remove just the v1 AWS SDK (as in the v2 one would be ok) or are you asking to remove all AWS SDKs (i.e. use minio's role assumption logic)?
It seems like either should be possible, though minio doesn't currently support adding tags to role assumption so that would need an upstream PR to add support for it
I'll also need to see how this is currently bootstrapping the auth for our existing use-case since the current iteration of the code just calls session.NewSession which sources credentials from environment variables
There was a problem hiding this comment.
to clarify, are you asking to remove just the v1 AWS SDK (as in the v2 one would be ok) or are you asking to remove all AWS SDKs (i.e. use minio's role assumption logic)?
Ideally use Minio's.
Why are tags needed when assuming the role? (for my understanding).
There was a problem hiding this comment.
mkay, let me poke at minio a bit (this is good learning for me too), beyond tags, it looks like it should hopefully work out of the box?
I believe tags were used to help with some metrics collection stuff. I was thinking that a good way to approach this would be to get assume role support merged and then add support for tags in a separate PR
There was a problem hiding this comment.
I made some progress getting minio to work. Mostly it was just adjusting the parameters and calling minio's function. I don't have a role setup that I can assume right now, but I have gotten it to give me an error for one I don't have permissions to assume. I'm out for a few days but can circle back to this again when I get back
There was a problem hiding this comment.
hmmm alright, I'm fairly sure this will work, though I haven't setup a role that I can actually try assuming with this
Another open question for me is whether the location for the region for the bucket can be the same as the assume role region. I lack deep knowledge of AWS, but it seems like there's the potential for them to be different which means there may need to be separate config options for them
Mostly testing if it works in CI or not since there's no test output
3753853 to
e3df62f
Compare
CI test env doesn't have bucket populated.
27e2568 to
7a5fdae
Compare
|
@julio-lopez, did you have anything else that needed addressed in this PR? |
|
@Rohit-BM18 FYI |
| // Used for assume role authentication. | ||
| RoleARN string `json:"roleARN"` | ||
| SessionName string `json:"sessionName"` | ||
| RoleDuration time.Duration `json:"duration"` |
There was a problem hiding this comment.
While this marshals correctly, it does not "roundtrip" (i.e., unmarshal back).
We can merge this, and iterate quickly to change the type to jsonencoding.Duration
repo/blob/s3/s3_assume_role.go
Outdated
| awscreds "github.com/aws/aws-sdk-go/aws/credentials" | ||
| "github.com/aws/aws-sdk-go/aws/credentials/stscreds" | ||
| "github.com/aws/aws-sdk-go/aws/session" | ||
| "github.com/aws/aws-sdk-go/service/sts" |
Adds support for AWS assume role credentials in the blob layer. This is only available if one uses the SDK, it doesn't wire up additional CLI options to access it. It does pull in a small amount of AWS-specific code