Use default AWS credential handling under normal circumstances #218
Conversation
| @@ -213,7 +208,7 @@ func (g *S3Getter) getAWSConfig(region string, url *url.URL, creds *credentials. | |||
| conf.Region = aws.String(region) | |||
| } | |||
|
|
|||
| return conf | |||
| return conf.WithCredentialsChainVerboseErrors(true) | |||
There was a problem hiding this comment.
This is just a useful configuration change to help debugging credentials issues.
Before:
Copying configuration from "s3::https://s3-us-west-2.amazonaws.com/..."...
Error: Failed to download module
Could not download module "root"
(-from-module="s3::https://s3-us-west-2.amazonaws.com/...":1)
source code from
"s3::https://s3-us-west-2.amazonaws.com/...
NoCredentialProviders: no valid providers in chain. Deprecated.
For verbose messaging see aws.Config.CredentialsChainVerboseErrors
After:
Copying configuration from "s3::https://s3-us-west-2.amazonaws.com/..."...
Error: Failed to download module
Could not download module "root"
(-from-module="s3::https://s3-us-west-2.amazonaws.com/...":1)
source code from
"s3::https://s3-us-west-2.amazonaws.com/...":
NoCredentialProviders: no valid providers in chain
caused by: EnvAccessKeyNotFound: failed to find credentials in the
environment.
SharedCredsLoad: failed to load profile, <profile name>.
EC2RoleRequestError: no EC2 instance role found
caused by: RequestError: send request failed
caused by: Get
http://169.254.169.254/latest/meta-data/iam/security-credentials/: dial tcp
169.254.169.254:80: connect: no route to host
There was a problem hiding this comment.
This looks good to me and obsoletes my patch. We will test it under ECS to make sure. Thanks!
Oops, that was Terraform 0.11 that does not have the correct version of the aws-go-sdk to get a creds under ECS, please pardon the confusion.
|
|
|
Hi @jbergknoff-rival I also ran into the issue of not being able to access s3 buckets when it belongs to an originalizational unit. I tested your PR and it does solve the issue for us as well. If I understand it correctly, the next step would be to sign the CLA so that the PR can be merged. I was wondering if you are planning to sign it in the near future. |
|
Oh, thanks for pointing that out, @NinaDenisova . I actually have signed the CLA, and when I visit https://cla.hashicorp.com/hashicorp/go-getter?pullRequest=218, it shows me
and the form at the bottom is grayed out, with my details filled in. Maybe there's a bug on the status check. Either way, I don't think that was the primary blocker on this PR. It just hasn't received any attention from maintainers. |
|
Thank you @jbergknoff-rival for the quick reply! |
|
Hello there, about the CLA; you need to make sure the email you signed the CLA with is the same as the email you have in your commit, otherwise we cannot merge this. |
|
Thanks for the clarification. Now I understand what's going on. At the time I signed the CLA, the email address of the commit was associated with this GitHub account. The company has since become defunct, and I disassociated this GitHub account from that email address (which is also defunct). If I get a chance, I'll try rewriting the commit history on the PR branch. |
jbergknoff
force-pushed
the
jbergknoff-aws-creds
branch
from
c15ca05 to
b464f67
Compare
|
👍 I rewrote the commit to use the appropriate email address and now the CLA check is there. I also rebased from the current master branch. |
|
Awesome ! |
|
How does this relate to / duplicates #261? |
Fixes #191 (and #185, #157, #152, #22)
This PR changes go-getter to default to the AWS SDK's built in credential resolution. This makes it possible for go-getter to download from S3 using an assumed role (e.g. when setting environment variables
AWS_SDK_LOAD_CONFIG=true AWS_PROFILE=..., go-getter will assume that role before attempting any operations). It should also add support for various other edge cases relating to AWS credentials.The one case where go-getter will continue to construct its own credential chain is the one with an override for the instance metadata URL. I'm not familiar with the use case of overriding the instance metadata URL, so I left that code as it was.
I'd like to add a test to protect against regressions in assume-role behavior, but I'd appreciate some guidance there. I see the hardcoded read-only IAM access key/secret. Do those credentials also have the ability to assume some role?