-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: add flag to disable IMDSv1 fallback #4748
Conversation
|
||
// Check if request timed out while waiting for response | ||
if e, ok := requestFailureError.OrigErr().(awserr.Error); ok { | ||
if e.Code() == request.ErrCodeRequestError { |
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.
Question: did we ever get a handle on what was triggering this code path? From when I looked at it, the comment didn't appear accurate, it seemed like anything in the handler stack could have made it here.
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.
That is my concern and maybe a reason that fallback is happening more often than it should. This seems as though it can trigger too easily.
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.
agreed. the original Standard states that this should be for "timeouts". im wondering if it should be checking for ErrCodeResponseTimeout
rather than ErrCodeRequestError
EDIT: this was meant to be a reply to @lucix-aws and @aajtodd thread but github seemed to format it as a separate thread
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.
We talked offline about this and decided to just remove this. This was triggering permanent fallback for likely far too many error response codes. In the event of a timeout error it will still fallback for the current request but will retry imdsv2 for subsequent requests.
aws/ec2metadata/token_provider.go
Outdated
@@ -49,23 +54,21 @@ func (t *tokenProvider) fetchTokenHandler(r *request.Request) { | |||
output, err := t.client.getToken(r.Context(), t.configuredTTL) | |||
|
|||
if err != nil { | |||
// only attempt fallback to insecure data flow if IMDSv1 is enabled | |||
if !t.fallbackEnabled() { | |||
r.Error = awserr.New("EC2MetadataError", "failed to get IMDS token and fallback is disabled", err) |
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.
nit: i think this error message could have specified what version of IMDS failed and what version its falling back to. so like:
failed to get IMDSv2 token and fallback to IMDSv1 is disabled
if requestFailureError, ok := err.(awserr.RequestFailure); ok { | ||
switch requestFailureError.StatusCode() { | ||
case http.StatusForbidden, http.StatusNotFound, http.StatusMethodNotAllowed: | ||
atomic.StoreUint32(&t.disabled, 1) | ||
t.client.Config.Logger.Log(fmt.Sprintf("WARN: failed to get session token, falling back to IMDSv1: %v", requestFailureError)) |
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.
nice
|
||
// Check if request timed out while waiting for response | ||
if e, ok := requestFailureError.OrigErr().(awserr.Error); ok { | ||
if e.Code() == request.ErrCodeRequestError { |
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.
agreed. the original Standard states that this should be for "timeouts". im wondering if it should be checking for ErrCodeResponseTimeout
rather than ErrCodeRequestError
EDIT: this was meant to be a reply to @lucix-aws and @aajtodd thread but github seemed to format it as a separate thread
EC2Metadata
client will return an error when fetching a token from IMDS fails instead of falling back to the insecure data flow of IMDSv1v2 PR: aws/aws-sdk-go-v2#2048