feat(ec2/asg): CloudFormation-init support #8788
Conversation
rix0rrr
changed the title
| * | ||
| * You must escape the string appropriately. | ||
| */ | ||
| public static shellCommand(shellCommand: string, options: InitCommandOptions = {}): InitCommand { |
There was a problem hiding this comment.
~ Should we stick with the from* convention for the factories? So fromShellCommand here?
There was a problem hiding this comment.
Yeah, that's a good question. It would be more consistent with the rest.
| /** | ||
| * Configuration to apply to an EC2 instance on startup | ||
| */ | ||
| export abstract class CloudFormationInit { |
There was a problem hiding this comment.
Thoughts on CfnInit (and CfnInitElement, etc.) throughout? Or does that imply auto-generated?
There was a problem hiding this comment.
Somewhat. It definitely implies something that's operating at the template level. I'm in doubt whether this is that.
In any case, I don't hate it. Generally not too happy with our names atm.
| /** | ||
| * Map a user name to a user id | ||
| */ | ||
| public static fromName(userName: string, uid: number, groups?: string[], homeDir?: string): InitUser { |
There was a problem hiding this comment.
Both this and the group are a little awkward as the factory methods, although it's mostly in the naming. I do generally agree with keeping all factory methods though for consistency. Only alternative I can think of is "fromAttributes" or "fromProperties" and then having an InitUserOptions/Attributes/Props struct with the name/uid/groups/etc.
| /** | ||
| * Download from a URL at instance startup time | ||
| */ | ||
| public static fromUrl(targetFileName: string, url: string, options: InitFileOptions = {}): InitFile { |
There was a problem hiding this comment.
How would this be extended to support auth? A new fromUrlWithAuth, or creating a InitFileUrlOptions that extends InitFileOptions and includes the auth params?
There was a problem hiding this comment.
I was thinking just a new factory.
| /** | ||
| * A package to be installed during cfn-init time | ||
| */ | ||
| export class InitPackage extends InitElement { |
There was a problem hiding this comment.
Is this one of those examples where we'd want to provide an extension point for when a new repo type is added? Something like:
public static custom(repositoryName: string, packageName: string, ...versions: string[]): InitPackageI'm thinking of use cases like https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-cognito/lib/user-pool-client.ts#L96-L133; for example, if CloudFormation starts supporting npm.
| instances in the AutoScalingGroup. You can write files to it, run commands, | ||
| start services, etc. See the documentation of | ||
| [AWS::CloudFormation::Init](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-init.html) | ||
| and the documentation of CDK's `aws-ec2` library for more information. |
There was a problem hiding this comment.
A direct link to the appropriate section would be nice :)
| import * as fs from 'fs'; | ||
|
|
||
| /** | ||
| * Defines whether this Init template will is being rendered for Windows or Linux-based systems. |
There was a problem hiding this comment.
| * Defines whether this Init template will is being rendered for Windows or Linux-based systems. | |
| * Defines whether this Init template is being rendered for Windows or Linux-based systems. |
| * creation and updates. The UserData script needs to invoke `cfn-signal` | ||
| * with a success or failure code after it is done setting up the instance. | ||
| */ | ||
| public waitForResourceSignal(timeout: Duration) { |
There was a problem hiding this comment.
Why is this public? Does it make sense to call this outside the context of applyCloudFormationInit?
There was a problem hiding this comment.
I think the use case there would be a customer adding/applying their own user-data that called cfn-signal independent of cfn-init.
There was a problem hiding this comment.
Did we design for this scenario? Not sure its worth exposing this as a sort of addendum feature. Do we want to commit to this API? for example, maybe its better to accept a count argument so that users don'r have to split their timeout calculation and invoke this multiple times.
There was a problem hiding this comment.
TBH, @rix0rrr was the main architect of this side. I could certainly see the argument that this (and applyCloudFormationInit) start out their lives as private, and we expose them publicly after we have some more feedback.
| * - Add commands to the instance UserData to run `cfn-init` and `cfn-signal`. | ||
| * - Update the instance's CreationPolicy to wait for the `cfn-signal` commands. | ||
| */ | ||
| public applyCloudFormationInit(init: CloudFormationInit, options: ApplyCloudFormationInitOptions = {}) { |
There was a problem hiding this comment.
Since this overrides a previously configured init, it kinda feels like it should only happen at instance creation time. Any specific reason its exposed post creation as well? Also, I might be missing something here, but wouldn't calling this twice actually produce a single cfn-signal call, while CloudFormation will be waiting for 2 signals per the creation policy?
Side node: If we do keep this, maybe just
instance.init()?
@nija-at - Definitely agreed. Initially, since we paired on the whole thing and had reviewed each others' code, we were going to declare it as already reviewed; we later both agreed having a third set of eyes -- primarily for the naming and public APIs -- made sense. With that in mind, do you think it's reasonable to look at the API surface area and be able to do an effective review, or is this going to need to be split somehow to get through? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This is a prerequisite of #8788, where it will be used to add CfnInit metadata.
This is a prerequisite of #8788, where it will be used to add CfnInit metadata.
> NOTE: This is a reduced version of #8788, which is the full CloudFormation-init support. This has been reduced down to only support instances (not ASGs), and to only support the InitCommand and InitService init elements, rather than the full set. This is to reduce the PR size and encourage a more thorough review. A follow-up review will add the remainder of the elements and auto-scaling group support. Add CloudFormation-init support. The CloudFormation-init metadata is encapsulated in a CloudFormationInit object, and using it automatically renders the UserData to apply it and send a signal to the appropriate CloudFormation resource and adds the permissions required to use cfn-init, cfn-signal and any S3 files/assets to the instance role. On an Instance, using CloudFormation-init automatically adds a ResourceSignal with a default timeout to the instance.
|
NOTE: At @nija-at 's request, I have halved the size of this PR by breaking out a few supporting changes into separate PRs, and posting a PR with just two Init Elements and only instance (not ASG) support. #9065 is about half the size of this PR and should be a bit more manageable to review. Keeping this PR open as-is for now, as it contains all the comments and history; once #9065 is approved, I'll update this PR with just the second half of the functionality. |
Exposes a mechanism to add metadata to CloudFormation resources by key name. This can be used for internal usage like setting the CDK metadata path, or for other metadata like CfnInit use cases. This is a prerequisite of #8788, where it will be used to add CfnInit metadata. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Exposes a mechanism to add metadata to CloudFormation resources by key name. This can be used for internal usage like setting the CDK metadata path, or for other metadata like CfnInit use cases. This is a prerequisite of aws#8788, where it will be used to add CfnInit metadata. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
> NOTE: This is a reduced version of #8788, which is the full CloudFormation-init support. This has been reduced down to only support instances (not ASGs), and to only support the InitCommand and InitService init elements, rather than the full set. This is to reduce the PR size and encourage a more thorough review. A follow-up review will add the remainder of the elements and auto-scaling group support. Add CloudFormation-init support. The CloudFormation-init metadata is encapsulated in a CloudFormationInit object, and using it automatically renders the UserData to apply it and send a signal to the appropriate CloudFormation resource and adds the permissions required to use cfn-init, cfn-signal and any S3 files/assets to the instance role. On an Instance, using CloudFormation-init automatically adds a ResourceSignal with a default timeout to the instance. Note this currently also includes the same changes as #9063, as this relies on it. #9063 can be independently shipped. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…nd groups Cloudformation Init (cfn-init) support was introduced in #9065 with a minimal set of support for various init element types. This PR is a continuation of that support (based on the original #8788), and adds the remaining init element types: files, packages, sources, users, and groups. With this PR, CloudFormation init support for EC2 instances is complete. A final PR will be submitted to extend this support to auto-scaling groups (again, based on the original work done in #8788).
… groups (#9664) Cloudformation Init (cfn-init) support was introduced in #9065 with a minimal set of support for various init element types. This PR is a continuation of that support (based on the original #8788), and adds the remaining init element types: files, packages, sources, users, and groups. With this PR, CloudFormation init support for EC2 instances is complete. A final PR will be submitted to extend this support to auto-scaling groups (again, based on the original work done in #8788). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
Closing this out in favor of the other PRs that have since been opened. #9674 remains as the last piece of this to be merged. |
Add CloudFormation-init support.
The CloudFormation-init metadata is encapsulated in a
CloudFormationInitobject, and using it automatically renders the UserData to apply it and send a signal to the appropriate CloudFormation resource and adds the permissions required to usecfn-init,cfn-signaland any S3 files/assets to the instance role.On an
Instance, using CloudFormation-init automatically adds aResourceSignalwith a default timeout to the instance.On an
AutoScalingGroup, using CloudFormation-init requires that you also specify aSignalconfiguration to indicate how much of the fleet you want to be up and running before continuing deployment.Also on
AutoScalingGroup, the half-baked understanding of resource signals and update policies has been replaced with a stronger, DRY object modeling.Fixes #777
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license