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

[WIP] Fix assuming role via ".aws/config" #1590

Closed
wants to merge 2 commits into from
Closed

[WIP] Fix assuming role via ".aws/config" #1590

wants to merge 2 commits into from

Conversation

svenwltr
Copy link
Contributor

@svenwltr svenwltr commented Sep 5, 2017

Fixes #1184

This PR allows assuming roles via shared configuration ~/.aws/config. It still supports assuming other roles for different provider configurations afterwards.

I am somehow not fully satisfied with the code yet, so feel free to nitpick everything.

Open Questions

  • Should I try to reduce code duplication due to assuming roles twice?
  • I am not entirely sure, if MFA would work correctly, but AFAIS when using MFA one has to do the aws sts assume-role manually anyway, so it shouldn't be affected by this change.

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 11, 2017
@radeksimko
Copy link
Member

Hey @svenwltr
thanks for the contribution.

It makes sense for Terraform to support assuming roles via ~/.aws/config. That said I'd rather see the INI parsing logic and all the format-specific logic, or generally as much of that code as possible in the upstream SDK, e.g. here https://github.com/aws/aws-sdk-go/tree/master/aws/credentials
The SDK is IMO much better place for it and we can make sure it's aligned with AWS' conventions/expectations and kept up to date as AWS engineers maintain the SDK.

Have you looked into the SDK and considered raising a PR there?

@radeksimko radeksimko added upstream Addresses functionality related to the cloud provider. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 11, 2017
@svenwltr
Copy link
Contributor Author

Hey @radeksimko,

the SDK is already able to do this, as I wrote in the actual issue:

sess, err := session.NewSessionWithOptions(session.Options{
    Config: aws.Config{
        Region: "my-region",
    },
    SharedConfigState: session.SharedConfigEnable,
    Profile:           "my-profile",
})

I did it the manual way, because Terraform already does more manually than necessary, which confused me a bit. I also strongly prefer to use the SDKs ability.

Also I am very fine, if we go with the PR #1608, which solves the same problem in another way.

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 11, 2017
@radeksimko
Copy link
Member

radeksimko commented Oct 11, 2017

Ah, I see - sorry I didn't read the whole conversation there. 🙈

Have you seen this PR? #1608 you did

It will probably need some tests & more thorough review, but it's more in-line with what we're discussing here. What do you think?

@radeksimko radeksimko added waiting-response Maintainers are waiting on response from community or contributor. and removed upstream Addresses functionality related to the cloud provider. labels Oct 11, 2017
@radeksimko
Copy link
Member

I should read more before posting comments, clearly...

Also I am very fine, if we go with the PR #1608, which solves the same problem in another way.

Cool. With that are you happy for me to close this PR?

@svenwltr
Copy link
Contributor Author

Yeah, the issues and PRs got a bit messed up :-)

Ok, it looks like we're on the same line here. Therefore I close this in favor of #1608.

@ghost
Copy link

ghost commented Apr 10, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using terraform with assumed roles defined in shared configuration are ineffective
4 participants