EKS UserScript Bootstrapper is duplicated when using UserScript with Managed AMI #214
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce conditional logic and validation to ensure that the Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This originally rotted out in #207 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
userdata_bootstrap_nt.tpl (1)
5-5: Add existence check and error handling before invocation
If the bootstrap script file is missing or the path is incorrect, this call will fail with a cryptic error. Consider validating the path and emitting a clear error message:if (-Not (Test-Path $EKSBootstrapScriptFile)) { Write-Error "EKS bootstrap script not found at path: $EKSBootstrapScriptFile" Exit 1 }Also ensure
${bootstrap_extra_args}is quoted or validated to avoid injection issues.launch-template.tf (1)
200-203: Improve error message clarity and format OS name correctly
Currentlytitle(lower(local.ami_os))will render"Al2"/"Windows". For consistency, useupper(local.ami_os)and mention both OS and AMI requirement in the error. For example:error_message = format( "after_cluster_joining_userdata requires a custom AMI (ami_image_id) and only supports AL2 or WINDOWS; current OS: %s", upper(local.ami_os) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by!README.md
📒 Files selected for processing (8)
README.yaml(1 hunks)launch-template.tf(1 hunks)userdata.tf(2 hunks)userdata.tpl(1 hunks)userdata_bootstrap.tpl(1 hunks)userdata_bootstrap_nt.tpl(1 hunks)userdata_nt.tpl(1 hunks)variables.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
README.yaml (1)
104-104: Clear documentation of AMI ID requirement.This note properly clarifies the constraint that
after_cluster_joining_userdatarequires a custom AMI ID. This prevents user confusion and aligns with the validation logic being added elsewhere in the codebase.userdata_nt.tpl (1)
29-29: Bootstrap script injection made conditional.The replacement of hardcoded PowerShell bootstrap invocation with
${bootstrap_script}variable enables conditional injection based on AMI configuration. This approach is consistent with the Linux template changes and supports the fix for bootstrap duplication.userdata_bootstrap.tpl (1)
1-1:❓ Verification inconclusive
Bootstrap script extraction implemented correctly.
The extraction of the EKS bootstrap script into a separate template file enables conditional injection. The parameter structure appears correct for EKS bootstrap requirements.
Please verify that the bootstrap script parameters match the current AWS EKS bootstrap.sh requirements:
🏁 Script executed:
#!/bin/bash # Verify the bootstrap script syntax and parameter order rg -A 10 -B 5 "bootstrap\.sh.*--apiserver-endpoint"Length of output: 232
Adjust bootstrap.sh invocation to match AWS EKS usage
Ensure that
<cluster_name>is passed as the first argument, before any flags.• File: userdata_bootstrap.tpl
Line: 1-/etc/eks/bootstrap.sh --apiserver-endpoint '${cluster_endpoint}' --b64-cluster-ca '${certificate_authority_data}' ${bootstrap_extra_args} '${cluster_name}' +/etc/eks/bootstrap.sh '${cluster_name}' --apiserver-endpoint '${cluster_endpoint}' --b64-cluster-ca '${certificate_authority_data}' ${bootstrap_extra_args}Please verify this ordering against the official AWS EKS bootstrap.sh documentation.
userdata.tpl (1)
18-18: Consistent bootstrap script variable injection.The replacement of hardcoded bootstrap script with
${bootstrap_script}variable maintains consistency with the Windows template approach. This enables conditional injection while preserving the MIME multipart structure required for EKS compatibility.userdata_bootstrap_nt.tpl (1)
1-3: Bootstrap script path variables defined correctly
The variables for the EKS bootstrap script directory, name, and full path are clear and accurate.userdata.tf (1)
52-56: Define bootstrapper template file map
The newuserdata_bootstrapper_template_filemap correctly references OS-specific bootstrap templates.variables.tf (1)
359-364:before_cluster_joining_userdatadescription updated appropriately
The description now clarifies the execution order relative to the bootstrap script.
| variable "after_cluster_joining_userdata" { | ||
| type = list(string) | ||
| description = "Additional `bash` commands to execute on each worker node after joining the EKS cluster (after executing the `bootstrap.sh` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production" | ||
| description = <<-EOT | ||
| Additional (bash for linux, powershell for Windows) commands to execute on each worker node after joining the EKS cluster | ||
| (after executing the `bootstrap` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production" | ||
| This can only be used with AL2 and Windows AMI types with a custom AMI set as EKS will inject a bootstrapper into all other user-scripts | ||
| EOT |
There was a problem hiding this comment.
Fix stray quote in multiline description
There’s an extra " at the end of the second line, causing a syntax error in the heredoc. Remove it so the <<-EOT block is valid.
Suggested diff:
- description = <<-EOT
- Additional (bash for linux, powershell for Windows) commands to execute on each worker node after joining the EKS cluster
- (after executing the `bootstrap` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production"
- This can only be used with AL2 and Windows AMI types with a custom AMI set as EKS will inject a bootstrapper into all other user-scripts
- EOT
+ description = <<-EOT
+ Additional (bash for linux, powershell for Windows) commands to execute on each worker node after joining the EKS cluster
+ (after executing the `bootstrap` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production
+ This can only be used with AL2 and Windows AMI types with a custom AMI set as EKS will inject a bootstrapper into all other user-scripts
+ EOT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "after_cluster_joining_userdata" { | |
| type = list(string) | |
| description = "Additional `bash` commands to execute on each worker node after joining the EKS cluster (after executing the `bootstrap.sh` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production" | |
| description = <<-EOT | |
| Additional (bash for linux, powershell for Windows) commands to execute on each worker node after joining the EKS cluster | |
| (after executing the `bootstrap` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production" | |
| This can only be used with AL2 and Windows AMI types with a custom AMI set as EKS will inject a bootstrapper into all other user-scripts | |
| EOT | |
| variable "after_cluster_joining_userdata" { | |
| type = list(string) | |
| description = <<-EOT | |
| Additional (bash for linux, powershell for Windows) commands to execute on each worker node after joining the EKS cluster | |
| (after executing the `bootstrap` script). For more info, see https://kubedex.com/90-days-of-aws-eks-in-production | |
| This can only be used with AL2 and Windows AMI types with a custom AMI set as EKS will inject a bootstrapper into all other user-scripts | |
| EOT | |
| } |
🤖 Prompt for AI Agents
In variables.tf around lines 366 to 372, there is an extra double quote at the
end of the second line in the multiline description of the variable
"after_cluster_joining_userdata". Remove this stray quote to ensure the heredoc
block is syntactically correct and does not cause errors.
…r regardless causing it to break.
what
When using the default AMI with a custom userscript AWS EKS will automatically inject the bootstrapper after your user-script.
This means
Before
After
why
after_cluster_joining_userdataaws_launch_template