266 aws native config

[BRMatt]

This is still a bit rough/verbose at the moment. Fairly new to Go so I'm sure there things I've done which aren't idiomatic. Please let me know if I've done something wrong/there is a better way to do this with terraform.

Tests pass locally, but I haven't run a full integration test against AWS to verify. Also, the credentials file is being loaded every time DefaultFunc is called, which probably isn't very efficient. Not sure how much of an issue this will be in practice though?

refs #266

TODO:

• Load credentials from ~/.aws/credentials
• Allow specifying profile via AWS_PROFILE (this is handled by goamz)
• Load config from ~/.aws/config (amazon say this file is meant solely for their CLI tool)
• Run integration tests
• Check if we need to preload the credentials/config file

[radeksimko]

Big 👍

I'm not sure if this should be proposed in a separate PR, but it would affect this feature anyway.
How about supporting AWS_SESSION_TOKEN = one-off time-based credentials generated with 2FA?

http://blogs.aws.amazon.com/security/post/Tx3D6U6WSFGOK2H/A-New-and-Standardized-Way-to-Manage-Credentials-in-the-AWS-SDKs

[sethvargo]

@BRMatt @radeksimko you might also be interested in #851. We switched to the "Standard AWS envvars".

[radeksimko]

And this is related too I reckon: #390

[BRMatt]

@sethvargo thanks for the heads up.

@radeksimko thanks for the link. That might be a bit out of scope for this PR, but I'll have a look to see if anything I add would cause problems for MFA.

[radeksimko]

@radeksimko thanks for the link. That might be a bit out of scope for this PR, but I'll have a look to see if anything I add would cause problems for MFA.

Basically it's just about one extra variable from config, so an example of 2FA-compatible config file would look like this:

[default]
aws_access_key_id = access_key_id_from_config
aws_secret_access_key = secret_access_key_from_config
aws_session_token = session_token_from_config

[BRMatt]

@sethvargo Sorry for taking so long to get around to this, I've cleaned up my implementation and squashed it into one commit. It takes into account the new AWS variables (via goamz), but gives precedence to the older env vars until they're removed. I'd really appreciate any criticisms you have.

@radeksimko I had a quick look at this, but unfortunately it's a bit more involved than just reading the extra config var (e.g. there's currently no variable in the schema to store the MFA token), so this PR intentionally focuses on just the access key id/secret key.

[radeksimko]

@BRMatt It's nice to clean the code, but if it's not in a separate commit, it's hard to understand what the actual proposed change is as it's cluttering the whole diff.

And btw. there are no coding standards defined for this project, at least I was not able to find any, so it's really hard to judge what kind of formatting is better.

[radeksimko]

I'm just looking at this and thinking that modifying terraform that much may not be necessary as there's most of the Amazon-related logic in mitchellh/goamz

[BRMatt]

@radeksimko That's a good point, I should really have left the cleanup as a separate commit. The main changes this PR makes are in aws/provider.go, aws/provider_test.go, and the related website doc

With regards to the formatting tweaks themselves, I just ran go-fmt on the aws package, as most projects seem to prefer it & it's part of the effective go guide. I'm presuming that's what this project uses as other commits reference it.

[BRMatt]

I'm just looking at this and thinking that modifying terraform that much may not be necessary as there's most of the Amazon-related logic in mitchellh/goamz

That's true, and in fact this PR delegates most of the auth loading to goamz. That said there are a couple of differences that make me feel this PR may be necessary:

• We're still dealing with legacy env variables (AWS_ACCESS_KEY/AWS_SECRET_KEY), which should take precedence over the new variables - we don't want users' terraform projects being run with completely different credentials after upgrading to the next minor version of terraform! - but goamz gives precedence to the new style
• We need to work within the schema that terraform defines for AWS credentials, and that requires that access key/secret key are provided as separate values. I'm not sure we can work around this without making the code more brittle/harder to reason about.
• The GetAuth() function goamz/aws provides attempts to load instance credentials, which was causing problems on my local machine as it was attempting to lookup credentials from a service that didn't exist. I'm sure other people will run into this problem, and it's probably best to avoid having terraform "hang" unnecessarily.

That said, I'd love to hear your thoughts, especially if you feel there's a better way of doing this!

[radeksimko]

I generally think that if something is broken or behaving unexpectedly in goamz, it should be fixed there in the first time.

Also, duplicating the auth logic in terraform does not seem useful to me (unless I'm missing something), so I'd even go down the path of removing it from terraform and relying completely on goamz => making terraform work better with that library.

[phinze]

@radeksimko It's also worth noting that there's a larger effort (led by @catsby) to convert terraform entirely over to aws-go. So any goamz-specific pain should be only temporary. 😀

[radeksimko]

@phinze ah, ok, that's good to know as I was just about to send a few PRs regarding 2FA support in config files and generally.

Could you point us to the right library, please?
https://github.com/search?utf8=%E2%9C%93&q=aws-go

[radeksimko]

Ah, right, I'm just being blind... #971 awslabs/aws-sdk-go it is.

[BRMatt]

I generally think that if something is broken or behaving unexpectedly in goamz, it should be fixed there in the first time.

That's a reasonable assertion, however I think in this case the issue isn't that goamz is broken - it's perfectly reasonable for that library to prefer the AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY variations, as that's the variation AWS recommend, and the goamz library has been operating this way for nearly 2 years. Changing this in goamz would likely cause problems for other users of goamz. The issue is that we have to support current terraform users, and not change things unexpectedly.

[mitchellh]

Going to close this in favor of #1049 since we removed goamz. I'm really sorry for not attending to this sooner, we should've told you that we weren't reading this yet since we were planning the switch away from goamz.

[ghost]

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.