-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(aws-ec2): signal, download and execute helpers for UserData #6029
Conversation
User Data objects currently only supports adding commands by providing the full command as a string. This commit hopes to address this by adding the following functionality: * On Exit Commands - Both bash and powershell have the concepts of trap functions which can be used to force a function to run when a an exception is run. Using this we are able to set up a script block that will always run at the end of the script. * add Signal Command - Using the above on Exit commands we are able to make it so the User data will send a signal to a specific resource (eg. Instance/Auto scaling group) with the results of the last command. * Download and Execute a file from S3 - This writes a function into the user data which can be used for repeated calls to download and execute a file from s3 with a list of arguments. This was tested by launching instances which pulled a script from an S3 asset then signaling on completion. Fixes aws#623
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…arguments to the customer.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
It has been almost a week since this pull request was created and there haven't been any comments on it yet. Is there anything that needs to be updated on this or any questions about it that I can answer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I hope we can simplify it somewhat (and make it more generic), but this is great!
/** | ||
* Adds a command to download a file from S3 | ||
*/ | ||
public abstract addDownloadAndExecuteS3FileCommand(params: S3DownloadAndExecuteOptions): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function looks like it serves a very specific use case, that I'm not sure is common enough to warrant inclusion in the out-of-the-box experience.
I would already feel better if we could split this into separate download and execute commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have split these into separate commands now.
/** | ||
* Name of the bucket to download from | ||
*/ | ||
readonly bucketName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this should be an IBucket
, and also that the instance role should get bucket.grantRead()
called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can swap this to an IBucket.
However I disagree that the User Data should be responsible for granting read permissions for the bucket since the User Data can be created separately from the instance and then passed in later and the UserData has no knowledge of what it is being used for.
} | ||
|
||
public addDownloadAndExecuteS3FileCommand( params: S3DownloadAndExecuteOptions ): void { | ||
if (!this.functionsAdded.has('download_and_execute_s3_file')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put this in a bash function? If it's about UserData length, feels like the code can be written a lot more concisely if it wasn't in a bash function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in considering an option to materialize a function for reuse? Say the user is downloading multiple scripts, and would like to cut down on the amount of boilerplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The final download command is 3 lines, one of which is logging. If UserData scripts executed with -x
(which I'm nearly sure they do) that line is unnecessary, so it goes down to 2 lines. That's not a lot of boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default user data is not run under -x in fact echoing $-
only gives us the flags hB
on an AL2 instance
"echo \"Downloading file ${s3Path} to ${path}\";\n" + | ||
"mkdir -p $(dirname ${path}) ;\n" + | ||
"aws s3 cp ${s3Path} ${path};\n" + | ||
"if [ $? -ne 0 ]; then exit 1;fi;\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script should be running under set -e
, so this test is not necessary. (Verify that, make it true if it isn't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default the scripts are not being run under set -e
as such the new execute file command is running set -e
"mkdir -p $(dirname ${path}) ;\n" + | ||
"aws s3 cp ${s3Path} ${path};\n" + | ||
"if [ $? -ne 0 ]; then exit 1;fi;\n" + | ||
"chmod +x ${path};\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like execute should be its own function.
} | ||
|
||
public addDownloadAndExecuteS3FileCommand( params: S3DownloadAndExecuteOptions ): void { | ||
if (!this.functionsAdded.has('download_and_execute_s3_file')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments here as about the bash function.
@@ -101,7 +260,19 @@ class CustomUserData extends UserData { | |||
this.lines.push(...commands); | |||
} | |||
|
|||
public addOnExitCommands(...commands: string[]): void { | |||
this.onExitLines.push(...commands); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't really pretend to support onExitLines
if they're going to do something completely different. Better throw here as well. Please replace all the throws with more descriptive error messages (informing users about the likely mistake they made in THEIR code and what they should do to fix it), such as CustomUserData does not support FooCommand, use UserData.forLinux() or UserData.forWindows() instead
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Changed commands to no longer create bash/powershell functions. Added additional tests for custom userdata
Pull request has been modified.
A user data could be configured to run a script found in an asset through the following: | ||
```ts | ||
const asset = new Asset(this, 'Asset', {path: path.join(__dirname, 'configure.sh')}); | ||
const instance = new ec2.Instance(this, 'Instance', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the indentation and other whitespace here to at least be consistent.
Indentation of 2 spaces please, closing curly is not indented.
/** | ||
* Options when downloading files from S3 | ||
*/ | ||
export interface S3DownloadAndExecuteOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer used.
|
||
public addExecuteFileCommand( params: ExecuteFileOptions): void { | ||
this.addCommands( | ||
`set -e`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The download also needs to execute under set -e
. An easier way to set this is to change the shebang.
Can you change the default shebang to #!/bin/bash -e
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially even #!/bin/bash -ex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we want to actually to modify the shebang like this since I would consider the following:
- -e being a potentially breaking change for users. Anyone who was already adding -e manually would be fine but everyone else could run into unepected failures.
- -x being a potential security risk since unless customers explicitly disabled it we could be printing secrets in the userdata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a fair concern, but by that logic we can never improve any situation. At least -e
should be the default for 99% of users; people who want/need something else because they do custom error handling should be able to deviate from the default if they want to.
I will take this change myself since I also predict it will be a lot of integ test work.
/** | ||
* Adds commands to download a file from S3 | ||
*/ | ||
public abstract addS3DownloadCommand(params: S3DownloadOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not completely obvious what this returns, so that might be worth a @returns
annotation.
@@ -73,9 +73,11 @@ | |||
"dependencies": { | |||
"@aws-cdk/aws-cloudwatch": "1.23.0", | |||
"@aws-cdk/aws-iam": "1.23.0", | |||
"@aws-cdk/aws-s3": "1.23.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma
Removed unused properties.
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I have a question, what would you recommend to use if someone needs to simply copy a script into an ec2 instance you are creating on cdk? In our case, my cdk creates the VPN instance and I need to upload the usercreation script to this instance.... |
If I'm understanding your question correctly... we've used an S3 Asset ( https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3-assets.Asset.html ), and the The S3 Asset is created when you deploy your stack, and is uploaded automatically to your CDK staging bucket. Then, the UserData will download the asset object from S3 to your instance during initialization; note that the return value of So, essentially, something like:
|
@ddneilson Thank you very much, I'll try this tomorrow!!! |
User Data objects currently only supports adding commands by providing the full command as a string. This commit hopes to address this by adding the following functionality:
This was tested by launching instances which pulled a script from an S3 asset then signaling on completion. One example test app was:
The asset referenced in this test was the following script:
Fixes #623
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Commit Message
feat(aws-ec2): signal, download and execute helpers on UserData
User Data objects currently only supports adding commands by providing the full command as a string. This commit hopes to address this by adding the following functionality: