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

Change S3 getAWSConfig to make compatible with AWS_PROFILE and ECS #156

Closed
wants to merge 1 commit into from

Conversation

mbarrien
Copy link

We are trying to use go-getter as a library within a program where we want it to be able to download a file from Github in some cases and S3 in some cases. For the S3 case, we want them to be able to authenticate using AWS profiles as defined in ~/.aws/config, using the implied AssumeRole operation when you have a role_arn within the profile. We have found that even when passing AWS_SDK_LOAD_CONFIG=1 and AWS_PROFILE=profile_name in the environment go-getter is unable to access the file. I believe this to be because of the override of credentials as written from #3 3 years ago is incompatible with changes in the AWS Go SDK that allow such overrides.

If Credentials is explicitly passed into the Config, there are code paths that are bypassed that allow reading from the environment. The credential chain passed in was mostly copied and pasted from the credentials chain present in the SDK from 3 years ago, but the implementation has changed since then, including adding support for getting metadata from a different location as used in ECS, and standardizing the ~/.aws/config file. go-getter's hard coded chain misses out on this. However, there is one place where you do add a feature: the AWS_METADATA_URL environment for EC2 metadata.

This implementation replaces the implementation of hard-coding the provider chain with instead overriding the EndpointResolver. We now intentionally use the built in provider chain, but ensure the right resolvers are passed in at appropriate times. AWS_METADATA_URL is now implemented using the endpoint resolver detecting whether it is trying to resolve ec2metadata. This sits side by side with the new S3 endpoint resolver override that still respects the user's hostname. If Credentials are passed in (as in the case where the user has provided static AWS credentials in the URL), these are still supported; the ec2metadata resolver would never be called in such a case.

There were several overrides of config variables that were protected by "if creds != nil", but if you look at the logic, creds can never be nil. Thus, I removed the check and made the overrides of S3ForcePathStyle and DisableSSL unconditional. (Note that I believe the DisableSSL may no longer be necessary because this mainly affected the S3 endpoint resolver implementation.) With our now EndpointResolver handling things, conf.Endpoint is no longer overridden (also since overriding Endpoint would affect both the ec2metadata and s3 endpoint, and no single endpoint can handle both).

A vague comment from #3 suggests this may be intentional preventing outside environment from being able to influence behavior. For a generic library not tied to Terraform or Nomad, we actually don't want to force this behavior upon outside developers; they can enforce any isolation from external environment in their own way (and that enforcement is what's breaking our use case).

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 19, 2019

CLA assistant check
All committers have signed the CLA.

@mbarrien mbarrien closed this Jan 19, 2019
@mbarrien mbarrien deleted the s3-resolver branch January 19, 2019 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants