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

Feat: Migrate EC2 Plugin Resource Detector from IMDSv1 to IMDSv2 #1408

Merged
merged 24 commits into from
Aug 18, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
72af3a0
feat: temporarily added EC2 resource detector
EdZou Aug 4, 2020
1f2b962
feat: update EC2 Resource Detector to IDMSv2 and modified detect-reso…
EdZou Aug 7, 2020
a54cd34
Merge remote-tracking branch 'upstream/master' into EC2detector
EdZou Aug 7, 2020
f198f3e
chore: fix weird package dependency changing
EdZou Aug 7, 2020
8748fcf
chore: add final line for packages
EdZou Aug 7, 2020
679f807
Merge branch 'master' into EC2detector
EdZou Aug 10, 2020
0f4d6c2
Merge branch 'master' into EC2detector
EdZou Aug 10, 2020
328c56d
chore: modify descriptive type for _fetchString options
EdZou Aug 11, 2020
f24e7d4
Merge remote-tracking branch 'upstream/master' into EC2detector
EdZou Aug 11, 2020
0f8695d
Merge branch 'EC2detector' of https://github.com/EdZou/opentelemetry-…
EdZou Aug 11, 2020
e2230b0
chore: remove unused variables
EdZou Aug 11, 2020
db26119
Merge remote-tracking branch 'upstream/master' into EC2detector
EdZou Aug 12, 2020
41e855b
Merge branch 'master' into EC2detector
EdZou Aug 12, 2020
f46e74c
chore: modified test name
EdZou Aug 12, 2020
8054ef6
Merge branch 'EC2detector' of https://github.com/EdZou/opentelemetry-…
EdZou Aug 12, 2020
e959ee5
refactor: replace request header with constant variables
EdZou Aug 12, 2020
d4d99bc
Merge branch 'master' into EC2detector
EdZou Aug 13, 2020
9dafafd
chore: modify comment
EdZou Aug 13, 2020
64c418c
refactor: remove HTTP_HEADER constant
EdZou Aug 13, 2020
06c1f79
refactor: make time out value constant
EdZou Aug 13, 2020
d31a8f5
Merge branch 'master' into EC2detector
obecny Aug 13, 2020
1074ed6
Merge branch 'master' into EC2detector
dyladan Aug 17, 2020
89f4c66
chore: revise the naming to IMDSv2
EdZou Aug 17, 2020
0dae0ff
Merge branch 'master' into EC2detector
obecny Aug 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ class AwsEc2Detector implements Detector {
* See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html
* for documentation about the AWS instance identity document endpoint.
*/
readonly AWS_INSTANCE_IDENTITY_DOCUMENT_URI =
'http://169.254.169.254/latest/dynamic/instance-identity/document';
readonly HTTP_HEADER = 'http://';
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not a great name since HTTP Header already has its own meaning. It looks like you're looking for the URL Scheme.

It looks like this is also only used in tests, so why is it defined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh here is a historical problem. Initially we use Url in order to connect with AWS identity document. And current version we use host and path separately.
I will move this constant to test code, thank you for pointing out!

readonly AWS_IDMS_ENDPOINT = '169.254.169.254';
Copy link
Member

Choose a reason for hiding this comment

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

Maybe AWS_IDMS_HOST_ADDRESS?

Copy link
Contributor Author

@EdZou EdZou Aug 11, 2020

Choose a reason for hiding this comment

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

Nice suggestion!
But because we have another path called HOST_DOCUMENT. To avoid confusion, I suppose this kind of naming is also acceptable

Copy link

Choose a reason for hiding this comment

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

It's IMDS (Instance Metadata Service) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing out! Will modify this

readonly AWS_INSTANCE_TOKEN_DOCUMENT_PATH = '/latest/api/token';
readonly AWS_INSTANCE_IDENTITY_DOCUMENT_PATH =
'/latest/dynamic/instance-identity/document';
readonly AWS_INSTANCE_HOST_DOCUMENT_PATH = '/latest/meta-data/hostname';

/**
* Attempts to connect and obtain an AWS instance Identity document. If the
Expand All @@ -44,41 +48,86 @@ class AwsEc2Detector implements Detector {
*/
async detect(config: ResourceDetectionConfigWithLogger): Promise<Resource> {
try {
const token = await this._fetchToken();
const {
accountId,
instanceId,
instanceType,
region,
availabilityZone,
} = await this._awsMetadataAccessor();
} = await this._fetchIdentity(token);
const hostname = await this._fetchHost(token);

return new Resource({
[CLOUD_RESOURCE.PROVIDER]: 'aws',
[CLOUD_RESOURCE.ACCOUNT_ID]: accountId,
[CLOUD_RESOURCE.REGION]: region,
[CLOUD_RESOURCE.ZONE]: availabilityZone,
[HOST_RESOURCE.ID]: instanceId,
[HOST_RESOURCE.TYPE]: instanceType,
[HOST_RESOURCE.NAME]: hostname,
Copy link
Member

Choose a reason for hiding this comment

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

Name and hostname are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason for this is not knowing how to populate this :) I filed an issue to ask about why there are two similar fields

open-telemetry/opentelemetry-specification#787

[HOST_RESOURCE.HOSTNAME]: hostname,
});
} catch (e) {
config.logger.debug(`AwsEc2Detector failed: ${e.message}`);
return Resource.empty();
}
}

private async _fetchToken(): Promise<string> {
const options = {
host: this.AWS_IDMS_ENDPOINT,
path: this.AWS_INSTANCE_TOKEN_DOCUMENT_PATH,
method: 'PUT',
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 PUT instead of GET?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of AWS IDMSv2 standard, we need to do PUT first to obtain TOKEN. You can refer to https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html

timeout: 1000,
headers: {
'X-aws-ec2-metadata-token-ttl-seconds': '60',
obecny marked this conversation as resolved.
Show resolved Hide resolved
},
};
return await this._fetchString(options);
}

private async _fetchIdentity(token: string): Promise<any> {
const options = {
host: this.AWS_IDMS_ENDPOINT,
path: this.AWS_INSTANCE_IDENTITY_DOCUMENT_PATH,
method: 'GET',
timeout: 1000,
headers: {
'X-aws-ec2-metadata-token': token,
obecny marked this conversation as resolved.
Show resolved Hide resolved
},
};
const identity = await this._fetchString(options);
return JSON.parse(identity);
}

private async _fetchHost(token: string): Promise<string> {
const options = {
host: this.AWS_IDMS_ENDPOINT,
path: this.AWS_INSTANCE_HOST_DOCUMENT_PATH,
method: 'GET',
timeout: 1000,
obecny marked this conversation as resolved.
Show resolved Hide resolved
headers: {
'X-aws-ec2-metadata-token': token,
obecny marked this conversation as resolved.
Show resolved Hide resolved
},
};
return await this._fetchString(options);
}

/**
* Establishes an HTTP connection to AWS instance identity document url.
* Establishes an HTTP connection to AWS instance document url.
* If the application is running on an EC2 instance, we should be able
* to get back a valid JSON document. Parses that document and stores
* the identity properties in a local map.
*/
private async _awsMetadataAccessor<T>(): Promise<T> {
private async _fetchString(options: http.RequestOptions): Promise<string> {
return new Promise((resolve, reject) => {
const timeoutId = setTimeout(() => {
req.abort();
reject(new Error('EC2 metadata api request timed out.'));
}, 1000);

const req = http.get(this.AWS_INSTANCE_IDENTITY_DOCUMENT_URI, res => {
const req = http.request(options, res => {
clearTimeout(timeoutId);
const { statusCode } = res;
res.setEncoding('utf8');
Expand All @@ -87,7 +136,7 @@ class AwsEc2Detector implements Detector {
res.on('end', () => {
if (statusCode && statusCode >= 200 && statusCode < 300) {
try {
resolve(JSON.parse(rawData));
resolve(rawData);
} catch (e) {
reject(e);
}
Expand All @@ -102,6 +151,7 @@ class AwsEc2Detector implements Detector {
clearTimeout(timeoutId);
reject(err);
});
req.end();
});
}
}
Expand Down
32 changes: 23 additions & 9 deletions packages/opentelemetry-resources/test/detect-resources.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import * as nock from 'nock';
import * as sinon from 'sinon';
import * as assert from 'assert';
import { URL } from 'url';
import { Resource, detectResources } from '../src';
import { awsEc2Detector } from '../src/platform/node/detectors';
import {
Expand All @@ -43,17 +42,20 @@ const PROJECT_ID_PATH = BASE_PATH + '/project/project-id';
const ZONE_PATH = BASE_PATH + '/instance/zone';
const CLUSTER_NAME_PATH = BASE_PATH + '/instance/attributes/cluster-name';

const { origin: AWS_HOST, pathname: AWS_PATH } = new URL(
awsEc2Detector.AWS_INSTANCE_IDENTITY_DOCUMENT_URI
);
const AWS_HOST = awsEc2Detector.HTTP_HEADER + awsEc2Detector.AWS_IDMS_ENDPOINT;
const AWS_TOKEN_PATH = awsEc2Detector.AWS_INSTANCE_TOKEN_DOCUMENT_PATH;
const AWS_IDENTITY_PATH = awsEc2Detector.AWS_INSTANCE_IDENTITY_DOCUMENT_PATH;
const AWS_HOST_PATH = awsEc2Detector.AWS_INSTANCE_HOST_DOCUMENT_PATH;

const mockedAwsResponse = {
const mockedTokenResponse = 'my-token';
const mockedIdentityResponse = {
instanceId: 'my-instance-id',
instanceType: 'my-instance-type',
accountId: 'my-account-id',
region: 'my-region',
availabilityZone: 'my-zone',
};
const mockedHostResponse = 'my-hostname';

describe('detectResources', async () => {
beforeEach(() => {
Expand Down Expand Up @@ -89,7 +91,9 @@ describe('detectResources', async () => {
.get(INSTANCE_PATH)
.reply(200, {}, HEADERS);
const awsScope = nock(AWS_HOST)
.get(AWS_PATH)
.persist()
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
obecny marked this conversation as resolved.
Show resolved Hide resolved
.replyWithError({ code: 'ENOTFOUND' });
const resource: Resource = await detectResources();
awsScope.done();
Expand Down Expand Up @@ -122,8 +126,16 @@ describe('detectResources', async () => {
code: 'ENOTFOUND',
});
const awsScope = nock(AWS_HOST)
.get(AWS_PATH)
.reply(200, () => mockedAwsResponse);
.persist()
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
obecny marked this conversation as resolved.
Show resolved Hide resolved
.reply(200, () => mockedTokenResponse)
.get(AWS_IDENTITY_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedIdentityResponse)
.get(AWS_HOST_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedHostResponse);
const resource: Resource = await detectResources();
gcpSecondaryScope.done();
gcpScope.done();
Expand All @@ -138,6 +150,8 @@ describe('detectResources', async () => {
assertHostResource(resource, {
id: 'my-instance-id',
hostType: 'my-instance-type',
name: 'my-hostname',
hostName: 'my-hostname',
});
assertServiceResource(resource, {
instanceId: '627cc493',
Expand Down Expand Up @@ -206,7 +220,7 @@ describe('detectResources', async () => {
assert.ok(
callArgsContains(
mockedLoggerMethod,
'AwsEc2Detector failed: Nock: Disallowed net connect for "169.254.169.254:80/latest/dynamic/instance-identity/document"'
'AwsEc2Detector failed: Nock: Disallowed net connect for "169.254.169.254:80/latest/api/token"'
)
);
// Test that the Env Detector successfully found its resource and populated it with the right values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import * as nock from 'nock';
import * as assert from 'assert';
import { URL } from 'url';
import { Resource } from '../../src';
import { awsEc2Detector } from '../../src/platform/node/detectors/AwsEc2Detector';
import {
Expand All @@ -26,36 +25,49 @@ import {
} from '../util/resource-assertions';
import { NoopLogger } from '@opentelemetry/core';

const { origin: AWS_HOST, pathname: AWS_PATH } = new URL(
awsEc2Detector.AWS_INSTANCE_IDENTITY_DOCUMENT_URI
);
const AWS_HOST = awsEc2Detector.HTTP_HEADER + awsEc2Detector.AWS_IDMS_ENDPOINT;
const AWS_TOKEN_PATH = awsEc2Detector.AWS_INSTANCE_TOKEN_DOCUMENT_PATH;
const AWS_IDENTITY_PATH = awsEc2Detector.AWS_INSTANCE_IDENTITY_DOCUMENT_PATH;
const AWS_HOST_PATH = awsEc2Detector.AWS_INSTANCE_HOST_DOCUMENT_PATH;

const mockedAwsResponse = {
const mockedTokenResponse = 'my-token';
const mockedIdentityResponse = {
instanceId: 'my-instance-id',
instanceType: 'my-instance-type',
accountId: 'my-account-id',
region: 'my-region',
availabilityZone: 'my-zone',
};
const mockedHostResponse = 'my-hostname';

describe('awsEc2Detector', () => {
before(() => {
beforeEach(() => {
nock.disableNetConnect();
nock.cleanAll();
});

after(() => {
afterEach(() => {
nock.enableNetConnect();
});

describe('with successful request', () => {
it('should return aws_ec2_instance resource', async () => {
const scope = nock(AWS_HOST)
.get(AWS_PATH)
.reply(200, () => mockedAwsResponse);
.persist()
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedTokenResponse)
.get(AWS_IDENTITY_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedIdentityResponse)
.get(AWS_HOST_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedHostResponse);

const resource: Resource = await awsEc2Detector.detect({
logger: new NoopLogger(),
});

scope.done();

assert.ok(resource);
Expand All @@ -68,18 +80,72 @@ describe('awsEc2Detector', () => {
assertHostResource(resource, {
id: 'my-instance-id',
hostType: 'my-instance-type',
name: 'my-hostname',
hostName: 'my-hostname',
});
});
});

describe('with failing request', () => {
it('should return empty resource', async () => {
const scope = nock(AWS_HOST).get(AWS_PATH).replyWithError({
code: 'ENOTFOUND',
describe('with unsuccessful request', () => {
it('should return empty resource when receiving error response code', async () => {
const scope = nock(AWS_HOST)
.persist()
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedTokenResponse)
.get(AWS_IDENTITY_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(200, () => mockedIdentityResponse)
.get(AWS_HOST_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
Copy link
Member

Choose a reason for hiding this comment

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

please reuse the defined const

.reply(404, () => new Error('NOT FOUND'));

const resource: Resource = await awsEc2Detector.detect({
logger: new NoopLogger(),
});

scope.done();

assert.ok(resource);
assertEmptyResource(resource);
});

it('should return empty resource when timeout', async () => {
const scope = nock(AWS_HOST)
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
obecny marked this conversation as resolved.
Show resolved Hide resolved
.reply(200, () => mockedTokenResponse)
.get(AWS_IDENTITY_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
obecny marked this conversation as resolved.
Show resolved Hide resolved
.reply(200, () => mockedIdentityResponse)
.get(AWS_HOST_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
obecny marked this conversation as resolved.
Show resolved Hide resolved
.delayConnection(2000)
.reply(200, () => mockedHostResponse);

const resource: Resource = await awsEc2Detector.detect({
logger: new NoopLogger(),
});

scope.done();

assert.ok(resource);
assertEmptyResource(resource);
});

it('should return empty resource when replied Error', async () => {
const scope = nock(AWS_HOST)
.put(AWS_TOKEN_PATH)
.matchHeader('X-aws-ec2-metadata-token-ttl-seconds', '60')
obecny marked this conversation as resolved.
Show resolved Hide resolved
.reply(200, () => mockedTokenResponse)
.get(AWS_IDENTITY_PATH)
.matchHeader('X-aws-ec2-metadata-token', mockedTokenResponse)
obecny marked this conversation as resolved.
Show resolved Hide resolved
.replyWithError('NOT FOUND');

const resource: Resource = await awsEc2Detector.detect({
logger: new NoopLogger(),
});

scope.done();

assert.ok(resource);
Expand Down