fix(ec2): userData in launchTemplate is created automatically when machineImege is provided

[tmokmss]

closes #23592

Reading through the discussion in PR #12385, which introduced the original code, I could not find any reason not to create userData when machineImage is provided. They also agreed with the design here, but it seems it accidentally became out of scope at the time.

This change should not be considered as a breaking change because we are just adding empty userData to launchTemplates whose userData is not specified explicitly, and it will not have any effect on the existing behavior.

• Users who already sets a userData explicitly:
  • will not see any change to their synthesized template, as this PR only modifies userData when it is not set explicitly.
• Users who is not using userData:
  • will see a userData is added to their template. But the userData is empty and does nothing. It should not have any effect on the previous behavior.

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

• This PR adds new construct runtime dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[tmokmss]

hmm this change will require many downstream snapshots to be updated 🤔

edit) It turns out that only two modules (autoscaling and gamelift) requires snapshot updates.

[tmokmss]

@rix0rrr @ddneilson Hi, what do you think of this change? I'd like to know if I'm missing anything about why the userData was not created in the original PR.

[comcalvi]

This stack:

export class Ec2UserdataStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const launchTemplate = new ec2.LaunchTemplate(this, 'LaunchTemplate', {
      instanceType: new ec2.InstanceType('t3.small'),
      machineImage: new ec2.AmazonLinuxImage({
        generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
      }),
    });

    // this will cause an error
    launchTemplate.userData!.addCommands("yum -y update")

results in an error.

We can either update the docstring to reflect the current behavior or change the default so that userData is created by default. Why would we want userData to be created by default? I'd prefer we leave the current behavior and just update the docstring

[comcalvi]

Why can't we do

Suggested change

	this.userData = props.userData ?? imageConfig?.userData;
	if (props.userData || imageConfig?.userData) {
	this.userData = props.userData ?? imageConfig?.userData;
	}

and avoid the change to the templates? I understand that those won't require replacement or interruption, but we try to avoid changes to your diff caused only by upgrading. We'd have to put this behind a feature flag.

[tmokmss]

Hi thank you for the review!

I believe they behave in the same way. The original code just assigns undefined to this.userData if both props.userData and imageConfig?.userData are undefined. And no code before this line assigns any value to this.userData either, so this.userData remains unchanged when a developer does not specify userData or machineImage.

[tmokmss]

As for adding a feature flag, I'm not yet really convinced to add it. As far as I researched, it will not be a breaking change even without a feature flag. (Not 100% sure this change will not break anything as it is difficult to guarantee, but we can fix/revert it as soon as someone reports it.)

With that being said, if you CDK team wants to go more carefully, I'm willing to add this change behind a feature flag :) What do you think? @comcalvi

[tmokmss]

The reasons I'd like to add this feature are:

1. It will be more convenient and ergonomic

    // previously
    // more code, duplicated knowledge about the instance OS
    const userData = ec2.UserData.forLinux();
    const launchTemplate = new ec2.LaunchTemplate(this, 'LaunchTemplate', {
      instanceType: new ec2.InstanceType('t3.small'),
      machineImage: new ec2.AmazonLinuxImage({
        generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
      }),
      userData,
    });
    launchTemplate.userData!.addCommands("yum -y update")

    // becomes
    const launchTemplate = new ec2.LaunchTemplate(this, 'LaunchTemplate', {
      instanceType: new ec2.InstanceType('t3.small'),
      machineImage: new ec2.AmazonLinuxImage({
        generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
      }),
    });
    launchTemplate.userData!.addCommands("yum -y update")

2. It will be consistent (and less confusing) with the behavior of the ec2.Instance construct.

// we can do this
    const instance = new ec2.Instance(this, 'Instance', {
      vpc,
      instanceType: new ec2.InstanceType('t3.small'),
      machineImage: new ec2.AmazonLinuxImage({
        generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
      }),
    });
    instance.userData!.addCommands("yum -y update")

[MrArnoldPalmer]

hey @tmokmss, so I agree that making LaunchTemplate behave closer to Instance is worthwhile, but I do think @comcalvi is right that it will have to be behind a feature flag. Even if the behavior of resources for users who aren't specifying any user data is technically the same, changing the behavior of the the asg.userData getter to throw when it previously didn't and otherwise changing properties on construct instances that used to undefined to now not can break some users code.

Reading through that discussion though I see some mention about the concept of "empty" user data not being easily defined. In the autoscaling snapshots we see the addition of

     "UserData": {
      "Fn::Base64": "#!/bin/bash"
     }

Does a Linux ec2.Instance resource without user data defined also receive this same "empty" bash script?

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

[tmokmss]

Hi @MrArnoldPalmer, thank you for your comment! OK, I agree with the possibility of breaking someone's code, so I'll add a feature flag for this.

As for an empty user data, yes, a shebang will appear also in an ec2.Instance template without user data defined.
For example, this code:

aws-cdk/packages/@aws-cdk/aws-globalaccelerator-endpoints/test/integ.globalaccelerator.ts

Lines 29 to 33 in 5a0c627

	instances.push(new ec2.Instance(this, `Instance${i}`, {
	vpc,
	machineImage: new ec2.AmazonLinuxImage(),
	instanceType: new ec2.InstanceType('t3.small'),
	}));

will be synthesized to this:

aws-cdk/packages/@aws-cdk/aws-globalaccelerator-endpoints/test/integ.globalaccelerator.js.snapshot/integ-globalaccelerator.template.json

Lines 578 to 580 in 5a0c627

	"UserData": {
	"Fn::Base64": "#!/bin/bash"
	}

It might be cleaner in terms of CFn just to set undefined instead of a shebang, but apparently no one have complained about this behavior for three years.

[tmokmss]

@comcalvi @MrArnoldPalmer I added a feature flag named @aws-cdk/aws-ec2:launchTemplateDefaultUserData. Hope you can review it 🙏

[comcalvi]

lgtm, leaving dnm so @MrArnoldPalmer can take a look.

[mergify]

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: d29f92c
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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

[tmokmss]

@comcalvi Thank you for the approval! I see CI failed on the merge commit bb4311b, but I don't have access to the CodeBuild log. Could you take a look at it just in case?