-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
|
||
// 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) |
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.
84dae07
to
459fc71
Compare
c := http.Client{ | ||
Timeout: 100 * time.Millisecond, | ||
} | ||
_, err := c.Get(metadataURL) |
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.
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
827863a
to
5f5459a
Compare
LGTM |
}}, | ||
&awsCredentials.EnvProvider{}, | ||
&awsCredentials.SharedCredentialsProvider{}, | ||
} |
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.
Do we want to consider adding an AssumeRoleProvider
here? Possibly more suitable as a future enhancement.
LGTM. |
provider/aws: WIP Refactor AWS Authentication chain
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? |
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 |
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. |
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: