-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add support in TF file configuration for assuming roles via profiles defined in ~/.aws/config #1608
Add support in TF file configuration for assuming roles via profiles defined in ~/.aws/config #1608
Conversation
After merging this PR, it seems that a rebuild/re-release of https://github.com/hashicorp/terraform and https://github.com/terraform-providers/terraform-provider-terraform will also be required to get this working consistently in all cases. Without this remote state backend and the remote state data source do not have the correct behaviour. |
@Ninir what is needed to progress this PR, is there anything I can do? I'm keen to see this merged rather than forgotten. I'm opening a new issue that I could also attempt a fix for, but I'd like to see progress on merging this before doing so. |
Hi @mikemoate Will try to check this asap: don't know much about this specific part, but with some time I should be able to get on this ;) |
Hi @Ninir it's been two more weeks and no activity on this PR, can this get prioritised please (it's not like other PR's aren't being reviewed and merged)? Is there anyone else that can look at this? |
Hi @mikemoate Sorry for the silence, 'had personal matters to manage 😅 |
@catsby and @radeksimko, I would love to get this PR merged as well. It is annoying that the aws provider doesn't support assume role profiles. What's the status of this PR? |
aaa4d81
to
ae63a1a
Compare
I've rebased the branch against latest master, I'm not sure what else I can do to move this along, but would very much like to see it merged! If you want to build with this support locally, you'll need to build this provider, the terraform-provider-terraform provider and terraform itself (since the last two directly pull in code from the AWS provider, making it somewhat less of a 'plugin'). https://gist.github.com/mikemoate/99776bee1c0b161f5bc2db207c771727 should cover all the needed steps on macOS (and has been tested by myself and another team member) but I'm far from an expert with govendor. |
@catsby and @radeksimko, This would be great to have sooner rather than later. @mikemoate, I appreciate you trying to move this along! |
I'm not sure how best to ensure that new releases of terraform-provider-terraform and terraform itself are made after merging this PR? terraform itself doesn't seem to have vendored a specific version of terraform-provider-aws, so a simple update/refresh of the vendored dependency is needed. terraform-provider-terraform is sadly pinned to [email protected] which is somewhat behind the current... Without these releases, profiles won't work in all places, which will no doubt lead to further end-user frustration. |
@mikemoate I just built a binary for Windows (thanks for that gist!), but unfortunately I'm getting the error, I set the profile in the provider as below, and also tried using the envs Any ideas?
|
Realized that while I built the terraform binary for Windows, I probably need to also build the plugins for Windows. Looking into that now. Pointers welcome... Edit: And I didn't checkout the correct branch when making the terraform binary. 🤦 |
Eef. Not seeing any instructions how to build plugins cross-platforms. No Anyway, I just copied my project to a throwaway linux box where I compiled everything, and the |
@lorengordon I can't offer much guidance for windows I'm afraid, though all of the go and git steps should work ok (you'll just need to replace the unix style file and directory operations). My original gist also neglected to checkout the correct branch :-S but I think had been fixed before you looked at it. |
@mikemoate No worries. Yes, I can setup the repos locally following your gist, no problem. I got stuck on the lack of |
Alright, I cribbed from the terraform Makefile and build.sh script, and I was able to use gox to build cross-platform binaries for Windows. Happy to confirm this patch works fine on Windows also. |
@radeksimko I see your comments on #1590 about this PR needing "some tests & more thorough review". I'd love to get that more thorough review soon :-) I'd be happy to write some tests but would like some guidance on the best approach to take. |
Yeah, I would be interested in how one would add tests to this. Either way, I'd love to see it merged soon please. |
+1 |
I wonder if this patch would get more attention if it were a new PR. Seems like there is a lot of actvity on the repo, just not older PRs. |
@Ninir @radeksimko @catsby This PR is now 2 months old and still hasn't been reviewed by a Terraform maintainer, yet newer PR's have been reviewed and merged. I appreciate this is a significant change to consider, but surely that merits more attention, not less... |
@Ninir @radeksimko @catsby, This PR simplifies this and brings terraform in line with the standard AWS CLI tool's way of doing authorization with roles. It would be really great to get this in. |
After #1590 was closed, I thought this topic would get a bit momentum, but it looks like nobody wants to touch it (or the responsible person is in vacation). Currently we need to patch every release and distribute the binaries in our team because of this, which get very tedious over time. |
@Ninir what is holding up this review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mikemoate & folks!
Sorry for the latency here, We all have been working on different stuff, but things are getting back to normal :)
So I've played a bit with a custom build based on this work, and it runs pretty smoothly! 😄
Just left a few nitpicks here, nothing critical 👍
Just a small question: given we have this configuration (taken from a related issue):
[default]
aws_access_key_id = KEY
aws_secret_access_key = SECRET
region = us-west-2
s3 =
signature_version = s3v4
[profile dev]
role_arn = arn:aws:iam::XXXXXXXX671:role/admin
source_profile = default
region = us-west-2
[profile test]
role_arn = arn:aws:iam::XXXXXXXX824:role/admin
source_profile = default
region = us-west-2
do you think it would be possible to handle the assume role logic when the profile is set as an env var, i.e. using AWS_DEFAULT_PROFILE
& AWS_PROFILE
?
it works nicely for non-assumed roles, but not for assumed ones :)
If you think this is something doable here, let's do it and merge, otherwise this should not be a blocker :)
Thanks a lot to everyone of you for your patience: we are about to cross the finish line :)
aws/config.go
Outdated
// Call Get to check for credential provider. If nothing found, we'll get an | ||
// error, and we can present it nicely to the user | ||
cp, err := creds.Get() | ||
if err != nil { | ||
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NoCredentialProviders" { | ||
return nil, errors.New(`No valid credential sources found for AWS Provider. | ||
// If a profile wasn't specifed then error out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: specified
aws/config.go
Outdated
Please see https://terraform.io/docs/providers/aws/index.html for more information on | ||
providing credentials for the AWS Provider`) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: This else is not needed since the if returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the work @mikemoate and patience folks!
A big thanks to @craiglink for the original work! 🚀 😄
Merging!
🎉 Congrats! |
@Ninir as discussed in my comments above, it's great that a new version of this provider will include this, but we also need to force terraform itself and terraform-provider-terraform to release versions that are compiled against this code, how do we achieve that? https://gist.github.com/mikemoate/99776bee1c0b161f5bc2db207c771727 helps to illustrate the scale of the challenge here, especially for terraform-provider-terraform that has very old dependencies! |
@mikemoate Not sure to get it right :/ Still, you can use a given provider by using Terraform 0.10.X & doing:
Is there a reason you want to build terraform itself? |
@Ninir both terraform and terraform-provider-terraform compile in source direct from this repo (including the source I changed), essentially violating the idea that the provider is a plugin :-S. This seems to be used for remote state amongst other things. Without new releases of both those that compile in this change, then the functionality only works in some cases, which isn't a great user experience... |
if I understand correctly: since the provider split, this repo (AWS provider) is really independent from the hashicorp/Terraform repo (the terraform core) regarding the code. Terraform just uses providers, i.e. binaries. So you should be all ok using the official 0.10.8 Terraform binary for instance and this compiled provider using the command I wrote above. If I don't understand correctly: can you provide more details? 😅 |
@Ninir So what I'm saying is that isn't true, both terraform and terraform-provider-terraform are NOT independent from this repo, and compile in source from it. Check the following links for proof: https://github.com/terraform-providers/terraform-provider-terraform/blob/master/vendor/vendor.json#L2000 So both these repos need to re-vendor their dependencies to include the latest changes from this repo, and then compile against that. |
@mikemoate Ok so first: I read Are you confirming they need to be in-sync, or is it just a supposition? Let me investigate & discuss here what this relates to :) |
@mikemoate @Ninir The Terraform reuses some code from Also Edit Edit3 |
@Ninir Oh I'm definitely confirming they need to be in-sync! :-) We use S3 based remote state and profile role assumption didn't work for this until both these were recompiled locally with update dependencies. As @svenwltr notes terraform-provider-terraform pulls in the terraform repos backend code, which then also brings in the code from this provider (which is why it's a vendored dependency for that provider too). It looks like terraform-provider-terraform is being removed for terraform 0.11.x onward, but I suggest we still fix it for any future releases of 0.10.x. |
Hmm ... it sound like this PR only uses the profile from the Terraform config, but to me the change looks not like it gets the profile source didn't change, but only the underlying creation of the session. Therefore setting the profile via |
@svenwltr indeed, there's a comment about it: #1608 (comment) I'm going to open a new issue to request for env support. It may not be trivial as it seems better way of doing this is to utilize the SDK rather than patching in the current solution. |
what happens if the base account requires mfa and you run |
@Ninir any update on getting new releases of terraform and terraform-provider-terraform that include this? I'm worried that a closed PR is easily forgotten, perhaps we should raise issues to track those releases? |
Hi folks, @trung @svenwltr I'm currently looking at how to refactor the logic to fully use the SDK: still theorical as of now, but will get into a PR very soon. This will include Env variables, conf, profiles,... @drdamour MFA is not supported right now. @mikemoate I opened a PR on the Core part: hashicorp/terraform#16661 |
Here we upgrade the AWS Go SDK to 1.12.27 and AWS provider to include hashicorp/terraform-provider-aws#1608. This includes the capability to use named credentials profiles from the `~/.aws/credentials` file to authenticate to the backend.
Hello, I've been eagerly awaiting the 0.11.2 release to test this but unfortunately I'm still getting nowhere :( My setup is as follows:
I've tried adding a provider block to the plan but this didn't help much:
the output from I've tried a bunch of other things but the only one that made a difference was when I changed the profile name to something other than
this is the
|
We have the following terraform config, which I just tested with 0.11.2 and it works as expected (prior to this release we were building terraform from source):
|
Thanks @mikemoate - I can confirm that once a named profile is explicitly specified in the backend block this all works fine including when using a 'default' profile - it just seems to need to be explicitly specified. After trying a few more combinations, it looks like assuming roles works only works if the profiles are explicitly specified (even if it's just |
@choppedpork yeah #1184 discusses support for the AWS_PROFILE env var and I think there is a PR in progress. |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
This is simply a port of hashicorp/terraform#11734 to the new terraform-provider-aws repo. (the original PR won't be merged since the code it relates to has been moved to this repo).
This should address #1184.
This won't support assuming roles where an MFA device has been specified for the profile (in ~/.aws/config). The original PR has some discussion of this, however, Terraform seems to utilise multiple, simultaneous sessions with AWS which complicates the situation somewhat. (I certainly saw repeated prompts for MFA when testing adding this)
I can't see much in the way of test code in config_test.go so am not immediately sure where and how to test this successfuly.
Original work by @craiglink in hashicorp/terraform#11734.