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

EdZou · 2020-08-07T20:32:11Z

Which problem is this PR solving?

Since currently, the most updated version of Instance Metadata Service is already v2 and the previous implementation follows the IMDSv1 standard. We migrate it to follow IMDSv2 standard in this PR.
For more details, refer to AWS EC2 Instance Metadata Service

Short description of the changes

migrate IMDSv1 EC2 resource detector to IMDSv2
updated corresponding unit test
also updated test part for detect-resources

If you would like to review @dyladan @anuraaga
Really appreciate it!

Additional Question:

When implementing this part, I notice that detect-resources take use of all the detectors components, should I also add Beanstalk and ECS detector to this part of code and make new PR?

…urce

codecov · 2020-08-07T20:38:00Z

Codecov Report

Merging #1408 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
+ Coverage   94.11%   94.24%   +0.13%     
==========================================
  Files         142      144       +2     
  Lines        4280     4346      +66     
  Branches      873      879       +6     
==========================================
+ Hits         4028     4096      +68     
+ Misses        252      250       -2

Impacted Files	Coverage Δ
...rces/src/platform/node/detectors/AwsEc2Detector.ts	`98.03% <100.00%> (+10.53%)`	⬆️
packages/opentelemetry-plugin-https/src/utils.ts
packages/opentelemetry-plugin-https/src/https.ts
...ackages/opentelemetry-exporter-zipkin/src/types.ts	`100.00% <0.00%> (ø)`
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100.00% <0.00%> (ø)`
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`98.46% <0.00%> (ø)`
...ackages/opentelemetry-exporter-zipkin/src/utils.ts	`100.00% <0.00%> (ø)`

dyladan · 2020-08-11T17:03:01Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

      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,


Name and hostname are the same?

Yes, at least in Java implementation they take Name and hostname as the same: https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk_extensions/aws_v1_support/src/main/java/io/opentelemetry/sdk/extensions/trace/aws/resource/Ec2Resource.java#L192

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

dyladan · 2020-08-11T17:04:11Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

   * 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: Record<string, any>): Promise<string> {


Is there a more descriptive type we can use than Record<string, any>? Looks like HttpRequestOptions might be a good fit.

Sounds great! Let me do it!

… into EC2detector

mayurkale22 · 2020-08-11T19:35:35Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

  readonly AWS_INSTANCE_IDENTITY_DOCUMENT_URI =
    'http://169.254.169.254/latest/dynamic/instance-identity/document';
+  readonly AWS_INSTANCE_TOKEN_DOCUMENT_URI =


Looks like unused.

Yes, will delete this part, thank you for pointing out!

mayurkale22 · 2020-08-11T19:35:41Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

  readonly AWS_INSTANCE_IDENTITY_DOCUMENT_URI =
    'http://169.254.169.254/latest/dynamic/instance-identity/document';
+  readonly AWS_INSTANCE_TOKEN_DOCUMENT_URI =
+    'http://169.254.169.254/latest/api/token';
+  readonly AWS_INSTANCE_HOST_DOCUMENT_URI =


mayurkale22 · 2020-08-11T19:38:31Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

@@ -30,8 +30,18 @@ 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 HTTP_HEADER = 'http://';
+  readonly AWS_IDMS_ENDPOINT = '169.254.169.254';


Maybe AWS_IDMS_HOST_ADDRESS?

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

It's IMDS (Instance Metadata Service) ;)

Thank you for pointing out! Will modify this

mayurkale22 · 2020-08-11T19:39:41Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

+    const options = {
+      host: this.AWS_IDMS_ENDPOINT,
+      path: this.AWS_INSTANCE_TOKEN_DOCUMENT_PATH,
+      method: 'PUT',


Why is this PUT instead of GET?

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

mwear

LGTM

… into EC2detector

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

packages/opentelemetry-resources/test/detect-resources.test.ts

packages/opentelemetry-resources/test/detectors/AwsEc2Detector.test.ts

dyladan · 2020-08-13T20:35:23Z

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

   */
-  readonly AWS_INSTANCE_IDENTITY_DOCUMENT_URI =
-    'http://169.254.169.254/latest/dynamic/instance-identity/document';
+  readonly HTTP_HEADER = 'http://';


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?

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!

obecny

lgtm, I hope you can address just last Nit comment prior to merging.

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts

EdZou added 5 commits August 3, 2020 20:59

feat: temporarily added EC2 resource detector

72af3a0

feat: update EC2 Resource Detector to IDMSv2 and modified detect-reso…

1f2b962

…urce

Merge remote-tracking branch 'upstream/master' into EC2detector

a54cd34

chore: fix weird package dependency changing

f198f3e

chore: add final line for packages

8748fcf

EdZou added 2 commits August 10, 2020 15:15

Merge branch 'master' into EC2detector

679f807

Merge branch 'master' into EC2detector

0f4d6c2

EdZou marked this pull request as ready for review August 11, 2020 15:51

EdZou requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners August 11, 2020 15:51

dyladan reviewed Aug 11, 2020

View reviewed changes

EdZou added 3 commits August 11, 2020 12:23

chore: modify descriptive type for _fetchString options

328c56d

Merge remote-tracking branch 'upstream/master' into EC2detector

f24e7d4

Merge branch 'EC2detector' of https://github.com/EdZou/opentelemetry-js…

0f8695d

… into EC2detector

mayurkale22 reviewed Aug 11, 2020

View reviewed changes

chore: remove unused variables

e2230b0

mayurkale22 approved these changes Aug 11, 2020

View reviewed changes

dyladan approved these changes Aug 12, 2020

View reviewed changes

EdZou and others added 2 commits August 12, 2020 11:33

Merge remote-tracking branch 'upstream/master' into EC2detector

db26119

Merge branch 'master' into EC2detector

41e855b

mwear approved these changes Aug 12, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Aug 12, 2020

EdZou added 2 commits August 12, 2020 13:59

chore: modified test name

f46e74c

Merge branch 'EC2detector' of https://github.com/EdZou/opentelemetry-js…

8054ef6

… into EC2detector

obecny reviewed Aug 12, 2020

View reviewed changes

EdZou and others added 3 commits August 12, 2020 17:15

refactor: replace request header with constant variables

e959ee5

Merge branch 'master' into EC2detector

d4d99bc

chore: modify comment

9dafafd

dyladan reviewed Aug 13, 2020

View reviewed changes

refactor: remove HTTP_HEADER constant

64c418c

obecny approved these changes Aug 13, 2020

View reviewed changes

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts Outdated Show resolved Hide resolved

EdZou and others added 3 commits August 13, 2020 16:22

refactor: make time out value constant

06c1f79

Merge branch 'master' into EC2detector

d31a8f5

Merge branch 'master' into EC2detector

1074ed6

EdZou changed the title ~~Feat: Migrate EC2 Plugin Resource Detector from IDMSv1 to IDMSv2~~ Feat: Migrate EC2 Plugin Resource Detector from IMDSv1 to IMDSv2 Aug 17, 2020

EdZou and others added 2 commits August 17, 2020 15:22

chore: revise the naming to IMDSv2

89f4c66

Merge branch 'master' into EC2detector

0dae0ff

obecny merged commit 1f8c365 into open-telemetry:master Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EdZou commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

dyladan Aug 11, 2020

EdZou Aug 11, 2020

anuraaga Aug 12, 2020

dyladan Aug 11, 2020

EdZou Aug 11, 2020

mayurkale22 Aug 11, 2020

EdZou Aug 11, 2020

mayurkale22 Aug 11, 2020

mayurkale22 Aug 11, 2020

EdZou Aug 11, 2020 •

edited

Loading

srprash Aug 17, 2020

EdZou Aug 17, 2020

mayurkale22 Aug 11, 2020

EdZou Aug 11, 2020

mwear left a comment

dyladan Aug 13, 2020

EdZou Aug 13, 2020

obecny left a comment

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

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

Conversation

EdZou commented Aug 7, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

Additional Question:

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdZou Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

EdZou commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

EdZou Aug 11, 2020 •

edited

Loading