feat(aws-ec2): add support for CloudFormation::Init #792

rix0rrr · 2018-09-27T13:26:08Z

Start of an API to manage UserData in a more principled way, hide the details of resource signaling and support CloudFormation Init.

Soliciting opinions from @moofish32 and @taichi to see if this solves their use cases--I'm not 100% sure I have all the details right.

Proposed to fix #623 and #777.

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

Make a new construct to manager User Data, which should make it easier to apply the features correctly. Fixes #623 and 777.

eladb · 2018-09-27T20:42:46Z

@rix0rrr I merged and aligned this from master and converted indentation to 2 spaces. Enjoy!

eladb · 2018-09-27T20:47:15Z

@mindstorms6 I believe you had some nifty ideas around this, can you take a look?

eladb

README?

eladb · 2018-09-27T20:43:19Z

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

+    this.userData = new ec2.UserData(this, 'UserData', {
+      os: machineImage.os,
+      defaultSignalResource: this.autoScalingGroup
+  });


Indentation

eladb · 2018-09-27T20:43:39Z

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

@@ -290,9 +297,11 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
  /**
   * Add command to the startup script of fleet instances.
   * The command must be in the scripting language supported by the fleet's OS (i.e. Linux/Windows).
+   *
+   * @deprecated Use userdata.addCommands() instead.


What's wrong with this as sugar? Looks pretty useful...

Maybe. I want to guide people towards the object that has convenience methods for common operations, so they don't go templating some complicated strings if there are better solutions available.

eladb · 2018-09-27T20:44:57Z

packages/@aws-cdk/aws-ec2/lib/cloudformation-init.ts

+    const resource = new cdk.Resource(this, 'Resource', {
+      // I know this thing says it lives in the AWS::CloudFormation namespace,
+      // but it really doesn't make sense there.
+      type: 'AWS::CloudFormation::Init',


Something is weird. I believe AWS::CloudFormation::Init is a metadata key, not a resource type:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-init.html

Doh, you're right! Completely misread that.

eladb · 2018-09-27T20:49:04Z

packages/@aws-cdk/aws-ec2/lib/machine-image.ts

+  /**
+   * Whether to stop executing as soon as an error is encountered
+   *
+   * @default false


Wouldn't it make sense that this would be true by default?
Also, we should document what this means on each platform?

Yes, we probably do want that, but then we have to do signaling very differently (which I think is still a good thing!)

Signaling is supposed to be used this way:

some_command cfn-signal -e $? ...

You must always run cfn-signal, but then use it to send the error code of the previous command (typically but not necessarily this will be cfn-init). Of course, this breaks down and becomes complicated if you run multiple commands.

An alternative that I like better is to install a bash error trap:

set -euo pipefail signal_failure() { cfn-signal --success false } trap signal_failure ERR command1 command2 command3 cfn-signal --success true

And this will do the right thing. But it is a lot more ceremony...

Why not trap exit and just send the last status. If they choose not to exit on error, strict (I think). The signal behavior would be the same. This would simplify the need for two signal commands.

Yes, I thought the same later, just hadn't figured out if I can use trap abd $! together in that way

eladb · 2018-09-27T20:50:05Z

packages/@aws-cdk/aws-ec2/lib/machine-image.ts

+    if (options.verbose !== false) { bashFlags.push('x'); }
+    if (options.strict) { bashFlags.push('eu'); }
+
+    const bashCommand = '#!/bin/bash' + (bashFlags.length > 0) ? ` -${bashFlags.join('')}` : '';


Are you sure it's possible to pass in flags like this instead of set -euo pipefail...

Yes:

huijbers ~/W/P/ecs-cdk(ecs-demo)> bash -euo pipefail bash-3.2$ echo $HELLO bash: HELLO: unbound variable huijbers ~/W/P/ecs-cdk(ecs-demo)>

eladb · 2018-09-27T20:50:26Z

packages/@aws-cdk/aws-ec2/lib/machine-image.ts

+  public createUserData(scripts: string[], options: UserDataOptions): string {
+    const bashFlags = [];
+    if (options.verbose !== false) { bashFlags.push('x'); }
+    if (options.strict) { bashFlags.push('eu'); }


By the way, I'd expect strict to also imply pipefail.

Sure. I was on the fence for that one, but I can totally add it.

eladb · 2018-09-27T20:53:06Z

packages/@aws-cdk/aws-ec2/lib/user-data.ts

+    const parts = ['cfn-signal',
+      `--region ${new cdk.AwsRegion()}`,
+      `--stack ${new cdk.AwsStackId()}`,
+      `--resource ${resource.logicalId}`


This should be the EC2 instance that you want to signal

Or AutoScalingGroup.

We have this in quite a few userdata sections

# Ensure our PATH is set correctly (on Amazon Linux, cfn-signal is in /opt/aws/bin) . ~/.bash_profile

This has served us well, but perhaps we want to offer a path override for the location of cfn-signal?

Feels like we should hardcode the path in the script (w/ cdk override)

moofish32

A couple of questions. This does look it would cover my use case. So far all of mine are ASG based not ec2.

moofish32 · 2018-10-02T05:33:07Z

packages/@aws-cdk/aws-ec2/lib/user-data.ts

+    const parts = ['cfn-signal',
+      `--region ${new cdk.AwsRegion()}`,
+      `--stack ${new cdk.AwsStackId()}`,
+      `--resource ${resource.logicalId}`


We have this in quite a few userdata sections

# Ensure our PATH is set correctly (on Amazon Linux, cfn-signal is in /opt/aws/bin) . ~/.bash_profile

This has served us well, but perhaps we want to offer a path override for the location of cfn-signal?

moofish32 · 2018-10-02T05:47:46Z

packages/@aws-cdk/aws-ec2/lib/machine-image.ts

+  /**
+   * Whether to stop executing as soon as an error is encountered
+   *
+   * @default false


Why not trap exit and just send the last status. If they choose not to exit on error, strict (I think). The signal behavior would be the same. This would simplify the need for two signal commands.

eladb · 2018-12-19T13:03:53Z

Closing this PR for now. The branch is still in the repo and we can reopen as needed.

DavidChristiansen · 2019-05-12T15:41:17Z

Can we consider bringing this back into the roadmap?

feat(aws-ec2): add support for CloudFormation::Init

c8e893c

Make a new construct to manager User Data, which should make it easier to apply the features correctly. Fixes #623 and 777.

rix0rrr requested review from RomainMuller, eladb and moofish32 September 27, 2018 13:26

Elad Ben-Israel added 3 commits September 27, 2018 23:39

Prepare for merge

1c5ea38

Merge remote-tracking branch 'origin/master' into huijbers/init-support

5c07740

Update indentation to 2 spaces

92c0ad3

eladb reviewed Sep 27, 2018

View reviewed changes

moofish32 reviewed Oct 2, 2018

View reviewed changes

0xdevalias mentioned this pull request Nov 6, 2018

Add support for AWS::CloudFormation::Init #777

Closed

eladb closed this Dec 19, 2018

rix0rrr deleted the huijbers/init-support branch April 23, 2019 07:11

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-ec2): add support for CloudFormation::Init #792

feat(aws-ec2): add support for CloudFormation::Init #792

rix0rrr commented Sep 27, 2018 •

edited

Loading

eladb commented Sep 27, 2018

eladb commented Sep 27, 2018

eladb left a comment

eladb Sep 27, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018 •

edited

Loading

moofish32 Oct 2, 2018

rix0rrr Oct 2, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018

eladb Sep 27, 2018

rix0rrr Sep 28, 2018

moofish32 Oct 2, 2018

rix0rrr Oct 2, 2018

moofish32 left a comment

moofish32 Oct 2, 2018

moofish32 Oct 2, 2018

eladb commented Dec 19, 2018

DavidChristiansen commented May 12, 2019

feat(aws-ec2): add support for CloudFormation::Init #792

feat(aws-ec2): add support for CloudFormation::Init #792

Conversation

rix0rrr commented Sep 27, 2018 • edited Loading

eladb commented Sep 27, 2018

eladb commented Sep 27, 2018

eladb left a 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

rix0rrr Sep 28, 2018 • 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

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Dec 19, 2018

DavidChristiansen commented May 12, 2019

rix0rrr commented Sep 27, 2018 •

edited

Loading

rix0rrr Sep 28, 2018 •

edited

Loading