Skip to content

Conversation

@cosnicolaou
Copy link
Contributor

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.

body, err := awsGet(ctx, false, awsIdentityDocURL(), timeout)
if err != nil {
return false
logger.VI(1).Infof("failed to access v1 metadata service: %v", err)
Copy link
Member

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
Copy link
Member

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

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Member

@richhx richhx left a 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.

@cosnicolaou cosnicolaou merged commit cb85103 into master Nov 20, 2020
@cosnicolaou cosnicolaou deleted the cos-v1-aws-metadata branch October 27, 2021 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants