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

provider/aws: Refactor AWS Authentication chain #4254

Merged
merged 2 commits into from
Dec 16, 2015
Merged

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Dec 10, 2015

This is a refactoring of how we construct the authentication for the AWS provider. It is a fix for #2693 by explicitly checking the authentication providers in order, instead of through a combination of explicit parameters and a fall back DefaultFunc. Synopsis of the problem is here #2693 (comment)

The code as is works, e.g. I tested the scenario(s) that failed in #2693 as well as working with credentials and env vars locally. I need to update/cleanup the test(s) some, but otherwise, this should be fully functional.

Things remaining:

  • Any doc updates (caveats, no longer prompting, etc)
  • Testing with a mocked out IAM environment (mocking out the AWS Metadata url and providing values)


// We only look in the EC2 metadata API if we can connect
// to the metadata service within a reasonable amount of time
conn, err := net.DialTimeout("tcp", "169.254.169.254:80", 100*time.Millisecond)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll mock this out with httptest server, pulling the metadata url from an ENV var for testing. Nomad does this

@catsby
Copy link
Contributor Author

catsby commented Dec 11, 2015

cc @phinze / @jen20 for review. Let me know if you need help confirming or otherwise want to chat

c := http.Client{
Timeout: 100 * time.Millisecond,
}
_, err := c.Get(metadataURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's verify something about the response being from the actual EC2 Metadata service in case there's some other HTTP server listening at this IP/port. Is there a header we can check for or something?

- update auth checking to check metadata header
- refactor tests to not export os env vars
@phinze
Copy link
Contributor

phinze commented Dec 15, 2015

LGTM

}},
&awsCredentials.EnvProvider{},
&awsCredentials.SharedCredentialsProvider{},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to consider adding an AssumeRoleProvider here? Possibly more suitable as a future enhancement.

@jen20
Copy link
Contributor

jen20 commented Dec 16, 2015

LGTM.

catsby added a commit that referenced this pull request Dec 16, 2015
provider/aws: WIP Refactor AWS Authentication chain
@catsby catsby merged commit 54e4432 into master Dec 16, 2015
@gtmtech
Copy link

gtmtech commented Dec 21, 2015

Did this make it into a release yet? How do I test this? If I understand correctly, AWS_SESSION_TOKEN isnt supported at all until this is fixed?

@catsby
Copy link
Contributor Author

catsby commented Jan 4, 2016

Hey @gtmtech – sorry to say this has not yet been released. We had hoped to release before the holidays but ran into some unexpected blockers and had to delay. We should get one out soon! For now, you'll need to build the master branch to see this in action. Sorry!

@catsby catsby changed the title provider/aws: WIP Refactor AWS Authentication chain provider/aws: Refactor AWS Authentication chain Jan 4, 2016
@jen20 jen20 deleted the b-aws-auth-refactor branch January 4, 2016 21:17
@ghost
Copy link

ghost commented Apr 29, 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 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.

@ghost ghost locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants