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

Remove credentials provider settings #45

Merged
merged 2 commits into from
Mar 18, 2021
Merged

Remove credentials provider settings #45

merged 2 commits into from
Mar 18, 2021

Conversation

rocket357
Copy link

Remove credentials provider settings to allow the AWS go SDK to pull credentials from Instance Profile/Metadata.

This is similar to PR #15, but involves just commenting out the credentials provider section so the AWS go SDK can do it's thing.

Resolves the issues I am having with wal-g hanging on an instance with no .aws/credentials or similar.

Remove credentials provider settings to allow the AWS go SDK to pull credentials from Instance Profile/Metadata.
@andersosthus
Copy link

This issue hit us as well. So either this change, or a flag to disable this behaviour

@x4m
Copy link

x4m commented Mar 18, 2021

This definitely makes sense. AWS creds were used improperly from the beginning. And one day we need to turn to conventional use.

But won't this break something to existing users? AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY will go on working as expected?

Also, I don't see any value in commenting code instead of deleting it.

Thanks!

@andersosthus
Copy link

andersosthus commented Mar 18, 2021

The way the Go AWS SDK (and all the other AWS SDKs work) resolve credentials is as follows (here's the actual source https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html):

  1. Environment variables.
  2. Shared credentials file.
  3. If your application uses an ECS task definition or RunTask API operation, IAM role for tasks.
  4. If your application is running on an Amazon EC2 instance, IAM role for Amazon EC2.

So ENV variables (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY) will always take precedence. AWS_REGION will control the region and AWS_PROFILE will select a profile from your .aws credentails. So this shouldn't break anything.

@rocket357
Copy link
Author

rocket357 commented Mar 18, 2021

Also, I don't see any value in commenting code instead of deleting it.

Totally fair. I'll update the PR.

Also, to add to what @andersosthus has already said, it may be that prior versions of the aws go sdk didn't work as the other aws sdks did, so the code that was forked from the original storages may have been required at some point in the past. That's speculation on my part, though. Currently the aws go sdk, if given a blank credential provider configuration, will search the four places listed above automatically with no need for additional configuration. So it shouldn't break anything =)

Per wal-g#45 (comment), remove the code rather than comment it out.
@x4m x4m merged commit eaed9bc into wal-g:master Mar 18, 2021
@x4m
Copy link

x4m commented Mar 18, 2021

Thanks!
Let's bump this into WAL-G and see if something will get broken.

@slesarev-hub
Copy link

Thanks!
Let's bump this into WAL-G and see if something will get broken.

Have you tried it? When I updated main WAL-G with this pr, TestConfigure from unittes failed.

mialinx added a commit to mialinx/wal-g that referenced this pull request Dec 19, 2021
After merging wal-g/storages#45 we are unable to
configure multiple s3 storages as it needed in copy commands.

Suggested fix wal-g#1050 works, but it's
dirty. Call to the `FolderFromConfig` pollutes the environment,
making impposible to any configuration function
(like `ConfigureCompressor`) after it.

Solution:
* Wal-g config file should be the first priorty configuration source.
* Set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY only if they are not
empty, otherwise fallback to the enviroment (and other storage
dependent) configuration defaults
* Do not overwrite the environment in `FolderFromConfig`
usernamedt pushed a commit to usernamedt/wal-g that referenced this pull request Feb 10, 2022
After merging wal-g/storages#45 we are unable to
configure multiple s3 storages as it needed in copy commands.

Suggested fix wal-g#1050 works, but it's
dirty. Call to the `FolderFromConfig` pollutes the environment,
making impposible to any configuration function
(like `ConfigureCompressor`) after it.

Solution:
* Wal-g config file should be the first priorty configuration source.
* Set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY only if they are not
empty, otherwise fallback to the enviroment (and other storage
dependent) configuration defaults
* Do not overwrite the environment in `FolderFromConfig`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants