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 standard AWS config in the S3 remote backend. #5270

Merged
merged 1 commit into from
May 6, 2016

Conversation

johnrengelman
Copy link
Contributor

Synchronizes the AWS Credential configuration code with the S3 Remote Backend. This allows the S3 remote backend to understand shared credential files and credential profiles.

@johnrengelman
Copy link
Contributor Author

Would be nice for the getCreds function to be shared between the S3 backend and the AWS provider, but I couldn't think of a good place for it to go so that both packages could use it.

@kendawg2
Copy link

+1 for this. Selecting a profile from a shared credentials file would be nice. The code seems to have the hook for it, but not obvious how to pass it. Also, not sure I agree with using the AWS provider config since some people use a common S3 bucket for all states even though they deploy using other accounts. This is exactly the problem I am encountering. I have to set the S3 crews in environment variable and then I can't use a profile in the AWS provider because it seems to look at the reds provided there first, then environment variables, then lastly to the shared credential file. If you pass the credentials on the command line in the remote config it stores them in the state file, which is not good.

@johnrengelman
Copy link
Contributor Author

@kendawg2 it's just the same code...it doesn't share the actual configuration.

@johnrengelman
Copy link
Contributor Author

@kendawg2 the docs were update to reflect how to pass the profile: https://github.com/hashicorp/terraform/pull/5270/files#diff-51fe5c0fa7e6751718babdfccde26120R55

@kendawg2
Copy link

@johnrengelman Got it. Saw the docs. Thanks.

@radeksimko
Copy link
Member

This is looking pretty good, I won't be against if someone reviews & merges this, but I personally want to focus on finishing #5030 or more specifically on aws/aws-sdk-go#543 which is very likely going to have impact on this too.

Also I agree with you that the S3 remote backend logic for getting credentials should be leveraging the existing logic we have in AWS provider. I think the easiest way to achieve it would be to expose aws.getCreds as aws.GetCreds or even better aws.GetCredentials. Then you could import the provider here like:

import "github.com/hashicorp/terraform/builtin/providers/aws"

@kendawg2
Copy link

kendawg2 commented Apr 3, 2016

Any update on this? This actually fixes a problem I am having and it sure would be nice to have merged.

@radeksimko
Copy link
Member

@kendawg2 I just finished refactoring #5030 which also now exposes aws.GetCredentials and aws.GetAccountId as discussed above.

It will need to be peer reviewed by another maintainer & merged, hopefully that will happen soon and then it's mostly up to @johnrengelman to leverage the refactored code here.

@johnrengelman you are also welcomed to have a look 👀 at the mentioned PR to confirm it's solving the problems with duplicity here in this PR.

Finally someone (potentially myself) can review and merge this PR.

@toddrosner
Copy link

It'd be awesome to have this one ship with 0.6.15

@ashmeer7
Copy link

+1 for getting this in 0.6.15!

@johnrengelman
Copy link
Contributor Author

I rebased this on latest master and moved to just sharing the GetCredentials function from the AWS provider since #5030 hasn't been merged yet.
I'd be happy to look at something once that is in, but if it's going to be delayed, I think this should go in regardless.

@radeksimko
Copy link
Member

radeksimko commented May 5, 2016

@johnrengelman
#5030 has been merged and exposes aws.GetCredentials which you can now leverage here.

Would you mind refactoring this PR?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label May 5, 2016
@radeksimko radeksimko self-assigned this May 5, 2016
@johnrengelman
Copy link
Contributor Author

@radeksimko done.

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 6, 2016
@@ -56,3 +56,6 @@ The following configuration options / environment variables are supported:
* `access_key` / `AWS_ACCESS_KEY_ID` - (Optional) AWS access key
* `secret_key` / `AWS_SECRET_ACCESS_KEY` - (Optional) AWS secret key
* `kms_key_id` - (Optional) The ARN of a KMS Key to use for encrypting the state.
* `profile` - (Optional) This is the AWS profile name as set in the shared credentials file.
* `shared_credentials_file` - (Optional) This is the path to the shared credentials file. If this is not set and a profile is specified, ~/.aws/credentials will be used.
* `token` - (Optional) Use this to set an MFA token. It can also be sourced from the `AWS_SECURITY_TOKEN` environment variable.
Copy link
Member

Choose a reason for hiding this comment

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

This is not really an MFA token, it's session token and can be used any time user uses AssumeRole* API - no matter if it's MFA-protected or isn't, but that's really a nitpick, I can fix that in a separate commit.

cristicalin pushed a commit to cristicalin/terraform that referenced this pull request May 24, 2016
@ghost
Copy link

ghost commented Apr 24, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants