Allow assuming a role via AWS_PROFILE or configuration files #580
Conversation
|
Sadly I don’t think so without making the test modify configuration files it probably should never touch. |
| Default: &tfbridge.DefaultInfo{ | ||
| EnvVars: []string{"AWS_PROFILE"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
There are quite a few more config vars that can be populated from AWS_* env vars - should we add the rest?
https://www.terraform.io/docs/providers/aws/index.html#environment-variables
There was a problem hiding this comment.
I think we should - I'll follow up on this with a new PR soon.
|
|
||
| _, err = creds.Get() | ||
| if err != nil { | ||
| if _, err := awsbase.GetCredentials(config); err != nil { |
There was a problem hiding this comment.
I'm not sure I follow why this is the right fix.
All of the code here is intended to catch and report failures to authenticate prior to the Terraform Provider Configure call reporting back failures. But the goal is to do the same checks the Terraform Provider will do.
Per https://github.com/hashicorp/aws-sdk-go-base/blob/8fac28668e59a2e5afa6b2d034614c648ddc418e/session.go#L31, it appears the call to creds.Get() will be made there.
Is the problem instead that Profile: stringValue(vars, "profile"), isn't correctly reading the profile here?
There was a problem hiding this comment.
The reason using AWS_PROFILE was failing is because we weren't reading the profile correctly before - whether through a profile configured in ~/.aws/config or ~/.aws/credentials.
I don't think removing the call to Get on the returned Credentials struct is strictly necessary to fix the bug, but I also don't think it buys us anything to do it. GetCredentials itself calls Get here (after constructing a valid credentials chain):
https://github.com/hashicorp/aws-sdk-go-base/blob/master/awsauth.go#L239
It also tests that a role can be assumed, if appropriate:
https://github.com/hashicorp/aws-sdk-go-base/blob/master/awsauth.go#L283
If we hit either of these cases, the call to GetCredentials would have failed before anyway, so Get would not be called. If it worked the first time (in GetCredentials), calling it a second time should not yield different behaviour without configuration changes.
It's worth noting that the aws-sdk-go-base library is newer than the Pulumi AWS provider, and that the checks done by Terraform have changed as a result of using this library.
Finally, the error message we produce in response to any error from configuration complains about AccessKeyID and SecretAccessKey. In future, we should change the error to reflect the actual source of the problem. We could duplicate the entire logic of GetCredentials in the pre-configure callback to give better error messages here, but it like we'd be better off doing string detection on the error values to avoid doing so.
There was a problem hiding this comment.
don't think removing the call to Get on the returned Credentials struct is strictly necessary to fix the bug, but I also don't think it buys us anything to do it.
Got it - makes sense.
Finally, the error message we produce in response to any error from configuration complains about AccessKeyID and SecretAccessKey. In future, we should change the error to reflect the actual source of the problem. We could duplicate the entire logic of GetCredentials in the pre-configure callback to give better error messages here, but it like we'd be better off doing string detection on the error values to avoid doing so.
Totally agreed.
Can we test via creating a first-class provider? |
|
The build failure is due to missing code gen - I'll push a fix for that now. I don't think testing with a first-class provider doesn't help (and it already worked if |
This is the other part of fixing #252.
Ahh - yeah - makes sense. |
This PR enhances the work done by @mi5guided in #579 to allow profiles defined via
AWS_PROFILEto be used as well as the AWS config files.Thanks, @mi5guided!
Fixes #252.