feat(aws-autoscaling): add flag and aspect to require imdsv2 #16052

jericht · 2021-08-13T23:05:32Z

Partially fixes: #5137
Related PR: #16051

Note: I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details.

Changes

Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups

Testing

Added unit tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-08-13T23:05:34Z

jericht · 2021-09-09T22:39:17Z

@comcalvi I just rebased this and it is ready for review. Will you be able to take a look at it?

comcalvi · 2021-09-10T16:58:08Z

@jericht Thanks for the PR! I'll definitely take a look at this.

comcalvi

Overall, looks great! Thank you for your patience in this review. I have some comments below; in addition, we should add a property to autoscaling groups to enable / disable imdsv1.

comcalvi · 2021-09-16T19:35:47Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+   * Whether warning annotations from this Aspect should be suppressed or not.
+   * @default false


Should be a line with only an * on it between these two. Also, @default <value> clauses should be structured @default - <value>.

comcalvi · 2021-09-17T17:03:40Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+    /* istanbul ignore next */
+    if (node === undefined || !(node instanceof AutoScalingGroup)) {
+      return;
+    }


The change in this comment will make the return line covered. In addition, the node === undefined check is unnecessary, as node is of type IConstruct and so can never be undefined.

Sounds good, updated

comcalvi · 2021-09-17T18:21:41Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+function implementsIResolvable(obj: any): boolean {
+  return 'resolve' in obj && typeof(obj.resolve) === 'function' &&
+         'creationStack' in obj && Array.isArray(obj.creationStack) &&
+         'toString' in obj && typeof(obj.toString) === 'function';
+}


We have a function that does this in core which also checks for null and undefined 🙂.

Oh nice, thanks! Updated

comcalvi · 2021-09-17T18:22:23Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+    }
+
+    const launchConfig = node.node.tryFindChild('LaunchConfig') as CfnLaunchConfiguration;
+    if (launchConfig.metadataOptions !== undefined && implementsIResolvable(launchConfig.metadataOptions)) {


the launchConfig.metadataOptions !== undefined can be removed; see comment above.

comcalvi · 2021-09-17T18:43:32Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+   * Adds a warning annotation to a node, unless `suppressWarnings` is true.
+   * @param node The scope to add the warning to.


Should be a line with only an * on it between these two.

comcalvi · 2021-09-17T22:38:03Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+      // WHEN
+      aspect.visit(asg);
+
+      // THEN
+      expect(asg.node.metadataEntry).toContainEqual({
+        data: expect.stringContaining('CfnLaunchConfiguration.MetadataOptions field is a CDK token.'),
+        type: 'aws:cdk:warning',
+        trace: undefined,
+      });
+      expectCDK(stack).notTo(haveResourceLike('AWS::AutoScaling::LaunchConfiguration', {
+        MetadataOptions: {
+          HttpTokens: 'required',
+        },
+      }));


aspect.visit() should not be called explicitly here. Instead, use the aspects API (and swap the expect() calls to ensure synthesize() is properly called, this will properly call the visit() function from within synthesize()):

Suggested change

// WHEN

aspect.visit(asg);

// THEN

expect(asg.node.metadataEntry).toContainEqual({

data: expect.stringContaining('CfnLaunchConfiguration.MetadataOptions field is a CDK token.'),

type: 'aws:cdk:warning',

trace: undefined,

});

expectCDK(stack).notTo(haveResourceLike('AWS::AutoScaling::LaunchConfiguration', {

MetadataOptions: {

HttpTokens: 'required',

},

}));

// WHEN

cdk.Aspects.of(stack).add(aspect);

// THEN

expectCDK(stack).notTo(haveResourceLike('AWS::AutoScaling::LaunchConfiguration', {

MetadataOptions: {

HttpTokens: 'required',

},

}));

expect(asg.node.metadataEntry).toContainEqual({

data: expect.stringContaining('CfnLaunchConfiguration.MetadataOptions field is a CDK token.'),

type: 'aws:cdk:warning',

trace: undefined,

});

Sounds good, updated

comcalvi · 2021-09-17T23:14:43Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+      const aspect = new AutoScalingGroupImdsAspect({ enableImdsV1 });
+
+      // WHEN
+      aspect.visit(asg);


Similarly to above:

Suggested change

aspect.visit(asg);

cdk.Aspects.of(stack).add(aspect);

Pull request has been modified.

jericht · 2021-09-28T19:20:49Z

@comcalvi I also wanted to get an opinion on my comments about the duplicated code between this PR and the ec2 PR. Do you think what we have right now is fine or should we reorganize these files?

comcalvi

We're almost there! It would be helpful if you could elaborate on the "suppress warnings" functionality; I don't fully understand what that's doing. We should definitely reduce duplication between this PR and the ec2 PR.

comcalvi · 2021-09-30T19:45:36Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/index.ts

@@ -0,0 +1 @@
+export * from './imds-aspect';


nitpick: can we add a newline to the end of this file?

Sure, added

comcalvi · 2021-09-30T19:46:34Z

packages/@aws-cdk/aws-autoscaling/package.json

@@ -73,12 +73,14 @@
  "license": "Apache-2.0",
  "devDependencies": {
    "@types/jest": "^26.0.24",
+    "@types/sinon": "^9.0.11",


We prefer to use jest over sinon. Can you remove sinon from the dependencies?

Sure, removed

comcalvi · 2021-09-30T19:47:57Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+    aspect.visit(construct);
+
+    // THEN
+    expect(stub.calledOnce).toBeTruthy();


in keeping with above comments, this should be expect(stub).toHaveBeenCalled()

comcalvi · 2021-09-30T19:49:06Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+      suppressWarnings: true,
+    });
+    const errmsg = 'ERROR';
+    const stub = sinon.stub(aspect, 'visit').callsFake((node) => {


Can you make this a jest stub and it a more descriptive name? Something like visitStub.

comcalvi · 2021-09-30T19:51:19Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+  /**
+   * Adds a warning annotation to a node, unless `suppressWarnings` is true.
+   *
+   * @param node The scope to add the warning to.
+   * @param message The warning message.
+   */
+  protected warn(node: cdk.IConstruct, message: string) {
+    if (this.suppressWarnings !== true) {
+      cdk.Annotations.of(node).addWarning(`${ImdsAspect.name} failed on node ${node.node.id}: ${message}`);
+    }
+  }
+}


Could you explain the need for the warning suppressing functionality here? I don't entirely understand why we need this.

We actually don't need this here now that I think about it, thanks for calling this out! Removed.

For context, this is a leftover from my original implementation, which included the changes in #16051. In that PR, I've added the option to suppress warnings in the event that the aspects for LaunchTemplates and Instances are used together. This is because the Instance aspect works by creating a LaunchTemplate for the instance, and it emits a warning if an instance is already associated with a LaunchTemplate (to avoid silently modifying a LaunchTemplate created by a user). If used together, warnings from the Instance aspect would be misleading since the LaunchTemplate aspect would handle those cases.

comcalvi · 2021-09-30T19:52:22Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+    const construct = new cdk.Construct(stack, 'Construct');
+
+    // WHEN
+    aspect.visit(construct);


This should be cdk.Aspects.of(stack).add(aspect);.

comcalvi · 2021-09-30T19:56:59Z

packages/@aws-cdk/aws-autoscaling/test/aspects/imds-aspect.test.ts

+    vpc = new ec2.Vpc(stack, 'Vpc');
+  });
+
+  test('suppresses warnings', () => {


Can you explain what this test verifies? Why do we need the capability to suppress warnings (and which warnings do we need to suppress)? I don't understand what this is checking.

I've removed this test since we took out the ability to suppress warnings.

packages/@aws-cdk/aws-autoscaling/README.md

Pull request has been modified.

rix0rrr · 2021-10-12T08:38:40Z

packages/@aws-cdk/aws-autoscaling/README.md

+
+```ts
+const aspect = new autoscaling.AutoScalingGroupImdsAspect({
+  enableImdsV1: false,


It's a bit confusing to me that this field is called enableImdsV1 but the other one is called disableImdsv1.

Feels like they should be the same?

Good catch, updated them both to be requireImdsv2 as requested in another comment

rix0rrr · 2021-10-12T08:39:17Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+ */
+interface ImdsAspectProps {
+  /**
+   * Whether IMDSv1 should be enabled or not.


Isn't it the default to have it enabled? So does it ever make sense to set this to anything other than false ?

Good point, removed this variable and renamed the class to reflect its behavior

rix0rrr · 2021-10-12T08:39:58Z

packages/@aws-cdk/aws-autoscaling/lib/aspects/imds-aspect.ts

+/**
+ * Base class for IMDS configuration Aspect.
+ */
+abstract class ImdsAspect implements cdk.IAspect {


Why does this base class need to exist? There's only one subclass?

It doesn't, this was leftover from a previous implementation where there were multiple subclasses. I've merged this code into a single class.

rix0rrr · 2021-10-12T08:40:51Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+   *
+   * @default - false
+   */
+  readonly disableImdsv1?: boolean;


I'd like to avoid negatives in booleans. Isn't something like requireImdsv2 much more descriptive?

I agree, updated

Pull request has been modified.

mergify · 2021-10-19T08:15:08Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-10-19T09:06:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: a26b1d6
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-10-19T09:07:30Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Partially fixes: #5137 Related PR: #16052 **Note:** This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here: - Should we keep it as is (2 PRs) so these Aspects are cleanly separated? or, - Does it make sense to either combine them in some way (e.g. a new package `@aws-cdk/aspects`) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package). ### Changes Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates. ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Partially fixes: aws#5137 Related PR: aws#16051 **Note:** I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details. ### Changes Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Partially fixes: aws#5137 Related PR: aws#16052 **Note:** This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here: - Should we keep it as is (2 PRs) so these Aspects are cleanly separated? or, - Does it make sense to either combine them in some way (e.g. a new package `@aws-cdk/aspects`) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package). ### Changes Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates. ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

jericht mentioned this pull request Aug 13, 2021

feat(ec2): add aspect to require imdsv2 #16051

Merged

jericht force-pushed the jericht/autoscaling_imds_aspect branch from 1689715 to ff3f13d Compare August 13, 2021 23:25

peterwoodworth assigned comcalvi Aug 19, 2021

peterwoodworth added effort/small Small work item – less than a day of effort @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling p2 labels Aug 19, 2021

jericht force-pushed the jericht/autoscaling_imds_aspect branch from ff3f13d to 8d01baa Compare September 9, 2021 19:40

comcalvi previously requested changes Sep 17, 2021

View reviewed changes

jericht force-pushed the jericht/autoscaling_imds_aspect branch from 8d01baa to 6160686 Compare September 27, 2021 23:38

jericht requested a review from comcalvi September 28, 2021 01:34

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

comcalvi previously requested changes Sep 30, 2021

View reviewed changes

jericht requested a review from comcalvi October 5, 2021 01:10

jericht force-pushed the jericht/autoscaling_imds_aspect branch 2 times, most recently from 8cb15e3 to 8b3906a Compare October 5, 2021 01:13

rix0rrr previously requested changes Oct 12, 2021

View reviewed changes

jericht requested a review from rix0rrr October 13, 2021 00:35

jericht added 5 commits October 13, 2021 20:45

feat(aws-autoscaling): add aspect to enable/disable imdsv1

f12fe19

use aspects api in unit test and use isResolvableObject in core

fd32844

add option to disable imdsv1

ca7cc7b

remove suppressWarnings option

b2ed9d7

remove base class and rename variable to requireImdsv2

336da33

jericht force-pushed the jericht/autoscaling_imds_aspect branch from 8e54242 to 336da33 Compare October 13, 2021 23:16

rix0rrr changed the title ~~feat(aws-autoscaling): add aspect to enable/disable imdsv1~~ feat(aws-autoscaling): add flag and aspect to require imdsv2 Oct 19, 2021

rix0rrr approved these changes Oct 19, 2021

View reviewed changes

Merge branch 'master' into jericht/autoscaling_imds_aspect

a26b1d6

mergify bot merged commit ef7e20d into aws:master Oct 19, 2021

Miradorn mentioned this pull request Oct 19, 2021

Support setting EC2 instance metadata to require token (IMDSv2) #5137

Closed

erka mentioned this pull request Oct 22, 2021

feat(autoscaling): AutoScale LaunchConfiguration MetadataOptions to configure IMDS V2 #14640

Closed

iamgabeortiz mentioned this pull request Jan 24, 2023

aws_cdk.aws_ec2 NatInstanceProvider: missing require_imdsv2 #23803

Open

2 tasks

		* Whether warning annotations from this Aspect should be suppressed or not.
		* @default false

		* Adds a warning annotation to a node, unless `suppressWarnings` is true.
		* @param node The scope to add the warning to.

feat(aws-autoscaling): add flag and aspect to require imdsv2 #16052

feat(aws-autoscaling): add flag and aspect to require imdsv2 #16052

Conversation

jericht commented Aug 13, 2021 • edited Loading

Changes

Testing

gitpod-io bot commented Aug 13, 2021

jericht commented Sep 9, 2021

comcalvi commented Sep 10, 2021

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comcalvi Sep 17, 2021 • 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

comcalvi Sep 17, 2021 • 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

comcalvi Sep 17, 2021 • 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

jericht commented Sep 28, 2021

comcalvi left a comment • 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

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

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

mergify bot commented Oct 19, 2021

aws-cdk-automation commented Oct 19, 2021

AWS CodeBuild CI Report

mergify bot commented Oct 19, 2021

jericht commented Aug 13, 2021 •

edited

Loading

comcalvi Sep 17, 2021 •

edited

Loading

comcalvi Sep 17, 2021 •

edited

Loading

comcalvi Sep 17, 2021 •

edited

Loading

comcalvi left a comment •

edited

Loading