feat(ec2): can define Launch Templates (not use them yet)

[ddneilson]

This provides an initial implementation of a level 2 construct for EC2 Launch Templates. It is a start that is intended to help get the ball rolling on implementation of Launch Template support within the CDK. It is a step towards resolving #6734

Launch Templates have value even without the integrations into Instance and AutoScalingGroup being implemented yet. Thus, the intention with this PR is to solely implement the L2 for LaunchTemplate. Future work in a separate PR would be required to implement its integration into Instance, AutoScalingGroup, and others.

---

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

[gitpod-io]

[ddneilson]

I defaulted with making all of the properties optional, since they all are completely optional in a Launch Template. Oddly, it is valid to create a completely empty Launch Template.

My thinking is that the PR is a good place to have a discussion regarding how opinionated the CDK's L2 should be regarding which properties are optional and which are not.

Four of the CloudFormation resources that can make use of LaunchTemplates are:

1. EC2::EC2Fleet -- Uses an array of them, where each element of the array can override some of the LaunchTemplate values.
2. EC2::SpotFleet -- Similar to EC2::Fleet in that it can use an array of LaunchTemplates, but the values that can be overriden are different.
3. EC2::Instance -- Can use a single launch template, and the properties (ex: machine image) defined by the Launch Template are overriden by defining the equivalent top-level property in the EC2::Instance resource.
4. AutoScaling::AutoScalingGroup -- Can use either a single Launch Template with no overrides. Or a mixed instances policy which uses a single launch template as a base template, but may optionally provide an override launch template for each element of an array of instance types.

[rix0rrr]

This is one of the reasons we ourselves have been getting stuck implementing it in the CDK. It just seems so open-ended and anything can be overridden at any point.

Thanks for this list! This is super helpful.

[ddneilson]

A reasonable argument could be probably made for making this a required property; it would allow us to always provide a default userData and mimic the osType + userData interface that is available on the Instance and AutoScalingGroup L2s.

Of the four uses of launch templates that are identified above, the Instance and AutoScalingGroup could both potentially provide a value for a machineImage outside of a particular launch template, but the later needs to provide the override using a launch template.

I'm not familiar enough with launch template use-cases to make a strong argument one way or the other; not sure if there are any uses that get broken if we make this required.

[rix0rrr]

Let's do it like this and see where it breaks.

I'll 100% give you that the moment we make this required, someone will come by and say "but I use Launch Templates to give all my instances the same networking configuration but then configure the AMI on the Instance class".

[ddneilson]

True enough. Let's leave it as optional.

[ddneilson]

This seems to be the only property of a LaunchTemplate that could tie it to a specific VPC. So, though it might be nice to be able to create one by default if we're not provided one, the lack of a VPC property on the LaunchTemplate prevents that.

The IConnectable interface is really useful, so it seems sensible to me to want to have the L2 LaunchTemplate implement the interface. However, the inability to create a default security group makes the case of not being provided a security group a little weird -- you end up with a Connections object that has no actual security group(s) in it.

[rix0rrr]

I see how it will be useful, but I'm still a little hesitant to create one automatically--especially as I can see use cases for Launch Templates where people don't want those (they will use LTs to set Spot Pricing for all instances but then create SGs for individual instances... or something).

I guess we can't!

[ddneilson]

Hrm. I'm thinking that it might not have been a good idea to default-create a Role for the launch template. Unfortunately, not doing so makes implementing IGrantable impossible.

Reason for not creating it would be a potential use-case for LaunchTemplate within the LaunchTemplate spec of an ASG's MixedInstancesPolicy whereby the user provides a base LaunchTemplate that defines all of the common stuff (Role, UserData, AMI, etc) but then selectively overrides some of the non-Role fields (ex: AMI, CpuOptions, Placement) for certain instance types. If we default-create a Role, always, in the L2 then that sort of use-case ends up with the selective-override templates overriding the instance Role.

On the other hand, a user could simply pass the role from the base LaunchTemplate into the override LaunchTemplates in that case. Sure, the override templates would be defining a Role but it would be the same Role as in the base template -- so, functionally equivalent.

Thoughts? Opinions?

[ddneilson]

On the surface, it seems that it might be possible to use a LazyRole here, and use a Lazy to create the CfnInstanceProfile only if the LazyRole has a statement, policy, or managed policy attached. Or if the LazyRole has been instantiated, I suppose.

That's, perhaps, a little sketchy though.

[rix0rrr]

Yeah let's keep it as simple as we can for now, until we get a better grip on this.

Let's let LaunchTemplates do as little as possible, except just hold on to some common objects/variables.

Any method that would normally interact with those unset members can throw an exception for all I care. Let's see where that takes us. It's easier to add magic in than to take it out later.

[ddneilson]

Similar reasoning to that, above, for not providing a default role... I'm thinking that this should probably be modified to set UserData to undefined if this.userData has no actual commands in it. i.e. Still automatically create this.userData, but have the Lazy generate undefined if that userData would be empty.

Thoughts? Opinions?

[rix0rrr]

Seems finicky. We'd have to introduce the concept of emptiness to a UserData first, as they won't necessarily render as empty, they might render to "", to "<powershell></powershell>" or even more complex for a MIME UserData.

Let's just say you have to specify it to start with.

[rix0rrr]

This is one of the reasons we ourselves have been getting stuck implementing it in the CDK. It just seems so open-ended and anything can be overridden at any point.

Thanks for this list! This is super helpful.

[rix0rrr]

Let's do it like this and see where it breaks.

I'll 100% give you that the moment we make this required, someone will come by and say "but I use Launch Templates to give all my instances the same networking configuration but then configure the AMI on the Instance class".

[rix0rrr]

Wait, is it though?

Shouldn't this be just as optional as all the other fields, and not create anything by default to not mess with the downstream consumers too much?

[rix0rrr]

Tags will have to be implemented in core and in cfn2ts, to integrate with the aspect-based tagging (by doing it at the L1 level).

To start, let's take tagging out of this PR.

[ddneilson]

Took a look at it... Looks to be a pretty monumental undertaking to add tagging support. That'll have to go on the CDK team's backlog.

LaunchTemplate tagging really does not look like it will integrate well with the current tagging abstractions. There is no tag field on the LaunchTemplate (i.e. the template itself is not taggable), but rather a tagSpecification member within the launchTemplateData field that has its own unique format. i.e. It does not implement anything resembling ITaggable.

[ddneilson]

Addition... Perhaps I'm being too clever/hacky, but I figured out a way to get the L2 LaunchTemplate to interact with tagging aspects without having to make any changes to CDK core.

[rix0rrr]

Fairly sure @inheritdoc is not necessary.

[rix0rrr]

I see how it will be useful, but I'm still a little hesitant to create one automatically--especially as I can see use cases for Launch Templates where people don't want those (they will use LTs to set Spot Pricing for all instances but then create SGs for individual instances... or something).

I guess we can't!

[rix0rrr]

If you declare this as props: LaunchTemplateProps = {} it saves a bunch of props?. operators in this method.

[rix0rrr]

!this.userData !== undefined 

I'm not sure the precedences work out correctly here.

But isn't the following just as easy:

this.userData = props.userData ?? props.machineImage?.getImage(this).userData

[rix0rrr]

Seems finicky. We'd have to introduce the concept of emptiness to a UserData first, as they won't necessarily render as empty, they might render to "", to "<powershell></powershell>" or even more complex for a MIME UserData.

Let's just say you have to specify it to start with.

[rix0rrr]

👍

Pull request has been modified.

[rix0rrr]

As good as there! Minor comments, after those are addressed I'm totally ready to ship this.

[rix0rrr]

This feels like it doesn't need to be lazy?

const iamProfileToken = iamProfile?.getAtt('Arn').toString();

?

Or if you MUST have that the type will be string instead of string | undefined (although why would you):

const iamProfileToken = iamProfile ? iamProfile.getAtt('Arn').toString() : Token.asString(undefined);

[ddneilson]

Yeah, I got a little carried away with the Lazy uses :-)

[rix0rrr]

Clever. Doesn't look too horrible. It may be unexpected that the L2 is taggable while the L1 isn't. But I can live with this, and if we have tests on it we can always refactor later.

[rix0rrr]

Maybe phrase this in terms of APIs the end user would see (as opposed to APIs the author would see):

LaunchTemplate can only be used as IConnectable if a securityGroup is provided...

[rix0rrr]

Same here:

LaunchTemplate can only be used as IGrantable if...

[rix0rrr]

Since the role property isn't really implementing a protocol such as IGrantable or IConnectable, I'd be fine surfacing the reality here. Declare this just as

public readonly role?: iam.IRole;

And users can deal with the fact that it might or might not be set.

[rix0rrr]

Because also: think about how Instance would consume this.

It needs to do something like:

if (props.role) { 
  instanceRole = props.role;
} else if (!launchTemplate.role) {     // <--- this should not throw then otherwise I can't inspect it
  instanceRole = new iam.Role(...);
}

to make sure the instance ends up with an instance profile.

[rix0rrr]

I thiiiiink... this might also benefit from just being able to be optional, like role.

[ddneilson]

Yeah, I agree. I'll switch all except for grantPrincipal and connection into just plain properties instead of getters.

[rix0rrr]

And I think userData as well...

Pull request has been modified.

[mergify]

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).

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: f84646c
• 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).