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

Add support for credential_source = Ec2InstanceMetadata in config #2005

Conversation

jbergknoff-rival
Copy link

Addresses #1901, at least partially.

This PR makes it possible to use a profile configured with credential_source = Ec2InstanceMetadata.

I'm not experienced with Go, so I'd appreciate feedback on how to write a test for this change. As defaults.RemoteCredProvider already has tests of its own, I think it would be ideal to have a test in session/shared_config_test.go which stubs out RemoteCredProvider. How does one do that? Thanks.

@@ -131,6 +135,14 @@ func (cfg *sharedConfig) setAssumeRoleSource(origProfile string, files []sharedC
if cfg.AssumeRole.SourceProfile == origProfile {
assumeRoleSrc = *cfg
assumeRoleSrc.AssumeRole = assumeRoleConfig{}
} else if cfg.AssumeRole.CredentialSource == "Ec2InstanceMetadata" {
Copy link
Author

Choose a reason for hiding this comment

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

I think this would work equally well for EcsContainer, but I don't have a readily-available environment where I could test that.

Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to create this PR.

The usage of CredentialsSource probably should happen within the Session's initialization. This is currently where the other credential providers are setup. https://github.com/aws/aws-sdk-go/blob/master/aws/session/session.go#L438

This is also where the sharedConfig type is used to configure the session.

@jbergknoff-rival
Copy link
Author

Thanks, I'll revisit this when I have a chance.

@jbergknoff-rival jbergknoff-rival force-pushed the jbergknoff/credential-source branch from 49d4c77 to 4d99a9b Compare June 26, 2018 15:41
@jbergknoff-rival
Copy link
Author

@jasdel thanks again for the suggestions! I've refactored the PR to do the credential lookup in the session initialization (not sure how I missed this spot the first time!). This also exposes a cleaner interface for testing the interaction with the EC2 metadata service, so I've added what seem like the appropriate tests.

Please take another look when you have a chance.

@jbergknoff-rival
Copy link
Author

@jasdel please have another look when you have time.

@hoshsadiq
Copy link

@jasdel any updates on this?

@hoshsadiq
Copy link

By the way, @jbergknoff-rival does this support credential_source = Environment?

@jbergknoff-rival
Copy link
Author

@hoshsadiq no, it doesn't add support for Environment. I'll rename the PR to be more specific.

@jbergknoff-rival jbergknoff-rival changed the title Add support for credential_source in config Add support for credential_source = Ec2InstanceMetadata in config Sep 17, 2018
@xibz
Copy link
Contributor

xibz commented Oct 11, 2018

Hello @jbergknoff-rival, thank you for creating this. I've went ahead and used your PR to build upon the other missing values for credential_source: EcsContainer and Environment. Going to close this in favor of #2201. Any feedback is most welcome!

@xibz xibz closed this Oct 11, 2018
@jbergknoff-rival
Copy link
Author

@xibz great news, thanks for running with this!

@diehlaws diehlaws added needs-review This issue or pull request needs review from a core team member. and removed review-needed labels Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants