-
Notifications
You must be signed in to change notification settings - Fork 27
x/ref/runtime/internal: use aws imds v1 first and then v2 #174
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
Conversation
| body, err := awsGet(ctx, false, awsIdentityDocURL(), timeout) | ||
| if err != nil { | ||
| return false | ||
| logger.VI(1).Infof("failed to access v1 metadata service: %v", 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.
thanks for adding logs, should help a lot with debugging problems like this in the future :)
| var ( | ||
| onceAWS sync.Once | ||
| onAWS bool | ||
| imdsv2 bool |
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.
Maybe rename this to onImdsv2 or something different than imdvs2 since local functions override the scope and is a little confusing
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.
done!
| func testAWSIDMSVersion(t *testing.T, imdsv2Only bool) { | ||
| ctx := context.Background() | ||
| host, stop := startAWSMetadataServer(t) | ||
| host, stop := startAWSMetadataServer(t, false) |
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.
why is this false? Should imdvs2Only be passed instead?
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.
indeed yes!
|
|
||
| externalURL := host + cloudpaths.AWSPublicIPPath + "/noip" | ||
| noip, err := awsGetAddr(ctx, externalURL, time.Second) | ||
| noip, err := awsGetAddr(ctx, false, externalURL, time.Second) |
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.
Similarly why is this false rather than imdvs2Only?
richhx
left a comment
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.
Generally LGTM with some minor comments. Not sure why build is failing though.
AWS' instance meta data service cannot be accessed from a docker container running in bridge mode. This PR changes the behaviour to use the v1 service first and only if that fails, to use the v2 service. This fallback is required since it's possible to
configure aws instances to allow v2 only.