Skip to content
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

Support ECS TaskRole in S3 storage driver #2973

Conversation

abulford
Copy link
Contributor

@abulford abulford commented Aug 5, 2019

Instead of constructing the list of credential providers manually, if we use the default list we can take advantage of the AWS SDK checking the environment and returning either the EC2RoleProvider or the generic HTTP credentials provider, configured to use the ECS credentials endpoint.

Also, use the defaults.Config() function instead of aws.NewConfig(), as this results in an initialised HTTP client which prevents a fatal error when retrieving credentials from the ECS credentials endpoint.

Fixes #2960

Signed-off-by: Andrew Bulford andrew.bulford@redmatter.com

@caervs caervs self-requested a review October 9, 2019 23:57
Copy link
Contributor

@caervs caervs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM 🐳

@btisdall
Copy link

btisdall commented Nov 6, 2019

Hi @abulford and thanks for the fix which would be a big help to me. The build is failing on the execution script/validate/vendor, let me know if you're too busy to fix and I'll gladly get it done.

@btisdall
Copy link

Ok @abulford looks like the failing vendor modules check is nothing to do with your change but related to some kind of inconsistency in go mod vendor behaviour, possibly golang/go#28102. Would you be able to help @caervs ?

@abulford
Copy link
Contributor Author

@btisdall sorry for missing your comments before - yes, I seem to remember when I submitted this I looked around and saw this test was failing elsewhere as well so assumed it was a known issue and hopefully wouldn't affect the review. I would have investigated further but was rushing something out at the time. Thanks for looking into this!

@Datise
Copy link

Datise commented Nov 25, 2019

Is there anything I can do to move this forward? We have a project that's suffering from this exact problem and it's pretty much the last piece.

@diranged
Copy link

diranged commented Feb 6, 2020

How is this still waiting? This is a critical patch... lets get this in!

@stevvooe
Copy link
Collaborator

stevvooe commented Feb 7, 2020

LGTM

It looks like the build is failing on the vendor changes.

Are there any tests?

@abulford
Copy link
Contributor Author

I've come back to a project that was relying on this and could do with it being merged so I can revert to the official release rather than my fork. Is there anything I can do to move this along? Or is this just awaiting the review from @manishtomar?

@ribbybibby
Copy link

Would love to see this merged and released. We're using our own generic HTTP credentials provider throughout our environment and the registry is the only outlier we have.

@manishtomar
Copy link
Contributor

Could you rebase this with latest master to fix the CI?

Instead of constructing the list of credential providers manually, if we
use the default list we can take advantage of the AWS SDK checking the
environment and returning either the EC2RoleProvider or the generic HTTP
credentials provider, configured to use the ECS credentials endpoint.

Also, use the `defaults.Config()` function instead of `aws.NewConfig()`,
as this results in an initialised HTTP client which prevents a fatal
error when retrieving credentials from the ECS credentials endpoint.

Fixes distribution#2960

Signed-off-by: Andrew Bulford <andrew.bulford@redmatter.com>
@abulford abulford force-pushed the support-ecs-instance-profile-in-s3-driver branch from 920076b to 9690d84 Compare July 1, 2020 07:44
@abulford
Copy link
Contributor Author

abulford commented Jul 1, 2020

@manishtomar rebase done, CI is passing now! :)

@rsrchboy
Copy link

So, what's remaining to merge this?

@manishtomar
Copy link
Contributor

This needs to be tested in AWS before merging. I was hoping to do that but have been busy.

@zfLQ2qx2
Copy link

@manishtomar Tested this works in Fargate and locally with env variables.

Copy link
Contributor

@manishtomar manishtomar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this.

@manishtomar manishtomar merged commit 2800ab0 into distribution:master Aug 26, 2020
@abulford
Copy link
Contributor Author

Thanks @manishtomar!

arkodg added a commit that referenced this pull request Oct 29, 2020
Reopen PR #2973 (Support ECS TaskRole in S3 storage driver).
@jhmartin
Copy link

jhmartin commented Dec 1, 2020

Any chance of getting a new release that encompasses this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 fails to work when running under AWS ECS Fargate