Skip to content

Reopen PR #2973 (Support ECS TaskRole in S3 storage driver). #3245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 29, 2020
Merged

Reopen PR #2973 (Support ECS TaskRole in S3 storage driver). #3245

merged 1 commit into from
Oct 29, 2020

Conversation

ob1dev
Copy link
Contributor

@ob1dev ob1dev commented Sep 3, 2020

This PR reopens #2973 and fixes still hardcoded list of credential providers:

  • StaticProvider
  • EnvVarProvider
  • SharedCredentialProvider
  • EC2RoleProvider

Instead it relies on SDK defaults when creating a new AWS Config and AWS Session. And only overrides them with StaticProvider if accesskey and secretkey are provided via config.yml:

storage:
  s3:
    accesskey: ABCDEFG123HY4LWA5TOG
    secretkey: AB1C+D2EF/Ge3cA/vQbUfMVpuDImUZsxkpCQDUy0
    region: us-east-1
    ...

As the result, it uses AssumeRoleWithWebIdentity credential provider (in session.resolveCredentials) that resolves assigned IAM role to K8s Service Accounts via OIDC token. Please see here for more details.

This changed have been tested in the following scenario to grant access to AWS S3 Bucket:

  • IAM Role for Service Accounts that're assigned to Pods on EKS cluster
  • IAM Role for EC2 instances
  • Environment Variables: AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY on a local machine

Upstreamed from Docker Hub

@ob1dev
Fix hardcoded credential provides.
34f1322 
Signed-off-by: olegburov <oleg.burov@outlook.com>
@ob1dev
Copy link
Contributor Author

ob1dev commented Sep 3, 2020

@abulford, could you please double check if my changes still work fine for you on ECS TaskRole?

@abulford
Copy link
Contributor

abulford commented Sep 3, 2020

@olegburov oh dear, did my change break something...? Sorry 😞 .

I'm afraid it's been a while since I did anything with this and I've since moved on to a completely different project, so it'll take me some time to get myself set up to build and test your change, but I'll try to give it a test within the next week or two.

@ob1dev
Copy link
Contributor Author

ob1dev commented Sep 5, 2020

@abulford don't worry. It didn't work with previous code either. The trick part is this condition, which was added in PR aws/aws-sdk-go@f23263f to support AssumeRoleWithWebIdentity.

@ob1dev ob1dev mentioned this pull request Sep 6, 2020
Closed
@ssabo
Copy link

ssabo commented Sep 16, 2020

Coming over from #3243, I have verified in my environment that this patch is working for passively assuming roles in other accounts by adding a config file to /root/.aws/config and setting -e AWS_SDK_LOAD_CONFIG=1 in the environment. I was able to assume a role via a config profile as well as select different profiles with environment variables. I also verified this still worked with the instance's IAM role as it did before.

@ingshtrom ingshtrom mentioned this pull request Oct 16, 2020
Closed
arkodg
arkodg approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg arkodg merged commit f5cdc24 into distribution:master Oct 29, 2020
@jrbeilke jrbeilke mentioned this pull request Nov 6, 2020
Closed
@hsoleimani hsoleimani mentioned this pull request Feb 25, 2021
Closed
@davidspek davidspek mentioned this pull request May 16, 2023
Closed
@milosgajdos milosgajdos mentioned this pull request Aug 13, 2023
Closed
@DavidMakin DavidMakin mentioned this pull request Sep 13, 2024
Open
@brucehoff brucehoff mentioned this pull request Oct 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@arkodg arkodg arkodg approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ob1dev @abulford @ssabo @arkodg