Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EKS UserScript Bootstrapper is duplicated when using UserScript with Managed AMI #207

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

EKS UserScript Bootstrapper is duplicated when using UserScript with Managed AMI #207

wants to merge 2 commits into from

Conversation

ChrisMcKee
Copy link
Contributor

what

When using the default AMI with a custom userscript AWS EKS will automatically inject the bootstrapper after your user-script.

This means

  • The userscript template will have two bootstrappers which causes the node to fail to register with the cluster
  • The "after" userscript option fails as there's no way to register code after the EKS injected registration.

Before

image

After

image

why

  • Added further information to the variable after_cluster_joining_userdata
  • Split the bootstrapper out into its own file so it can be added only if the config allows it
  • Added precondition to launch-template.tf aws_launch_template
  • Added note to readme

@ChrisMcKee ChrisMcKee requested review from a team as code owners February 11, 2025 18:02
@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Feb 11, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

This pull request introduces multiple documentation and logic improvements across module files. The updates include a clarification note in the README, enhanced formatting in Terraform documentation, refined preconditions in the launch template for custom AMI usage, and adjustments to the userdata scripts for dynamic bootstrap command execution. Additionally, variable descriptions have been revised to clearly specify differences for Linux and Windows environments.

Changes

File(s) Change Summary
README.yaml Added a note clarifying that specifying an AMI ID enables the fourth option for user data, otherwise the system defaults to the standard AMI behavior.
docs/terraform.md Updated multiple input variable descriptions by inserting <br/> tags and rephrasing for clarity (e.g., after_cluster_joining_userdata, ami_release_version, etc.).
launch-template.tf Introduced a new precondition in the lifecycle block to ensure that when using after_cluster_joining_userdata with AL2 or WINDOWS, a custom ami_image_id is provided.
userdata.tf Added a new local variable (userdata_bootstrapper_template_file) for OS-specific template mapping and introduced the bootstrap_script key in userdata_vars.
userdata.tpl, userdata_bootstrap.tpl, userdata_bootstrap_nt.tpl, userdata_nt.tpl Modified bootstrap script execution: replaced hardcoded command invocations with dynamic variable references and added new PowerShell variable declarations.
variables.tf Updated descriptions for before_cluster_joining_userdata and after_cluster_joining_userdata to indicate support for both bash (Linux) and PowerShell (Windows) scripts, and to specify their usage conditions with custom AMIs.

Possibly related PRs

  • Bugfixes #205: Updates related to the documentation and formatting of the after_cluster_joining_userdata variable, aligning it with custom AMI and user data logic changes.
📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4519c49 and cf2209d.

⛔ Files ignored due to path filters (1)
  • README.md is excluded by !README.md
📒 Files selected for processing (9)
  • README.yaml (1 hunks)
  • docs/terraform.md (2 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)
🧰 Additional context used
🪛 LanguageTool
docs/terraform.md

[uncategorized] ~59-~59: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... will not
be associated to the nodes and you will need to provide another securi...

(COMMA_COMPOUND_SENTENCE)

[grammar] ~82-~82: The verb ‘keep’ seems to be in the wrong form here.
Context: ...for unlimited length.
Set to null for keep the existing setting, which defaults to...

(FOR_VB)

[typographical] ~101-~101: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...

(RB_LY_COMMA)

[typographical] ~102-~102: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...

(RB_LY_COMMA)

[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ..._length](#input_random_pet_length) | In order to support "create before destroy" behavio...

(IN_ORDER_TO_PREMIUM)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (11)
userdata_bootstrap.tpl (1)

1-1: LGTM! Bootstrap command is correctly structured.

The bootstrap command is properly formatted with all required parameters and appropriate escaping of variable values.

userdata_bootstrap_nt.tpl (1)

1-5: LGTM! Windows bootstrap script is correctly structured.

The script properly:

  1. Sets up variables for EKS paths
  2. Constructs the bootstrap command with all required parameters
  3. Handles output redirection for comprehensive logging
userdata.tpl (1)

18-18: LGTM! Bootstrap script variable improves flexibility.

The change from hardcoded command to ${bootstrap_script} variable aligns with the PR objectives by allowing conditional inclusion of the bootstrapper, preventing duplication when using custom AMIs.

userdata_nt.tpl (1)

29-29: LGTM! Bootstrap script variable improves flexibility.

The change from hardcoded command to ${bootstrap_script} variable aligns with the PR objectives by allowing conditional inclusion of the bootstrapper, preventing duplication when using custom AMIs.

userdata.tf (2)

52-56: LGTM! Good separation of bootstrapper templates by OS.

The new mapping clearly separates bootstrapper templates by operating system type, improving maintainability and making the code more organized.

78-78: Verify the bootstrap script conditions.

The bootstrap script is only included when:

  1. A custom AMI is specified (ami_image_id)
  2. After-cluster userdata is provided
  3. OS is either AL2 or Windows

This aligns with the PR objective of preventing bootstrapper duplication.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Description: Verify the bootstrap script conditions are working as expected

# Test: Check for any instances where bootstrap script might be included without meeting all conditions
rg -A 5 'bootstrap_script.*=.*file\(' userdata.tpl userdata_nt.tpl

Length of output: 68

I'll now run an additional script to directly inspect the userdata.tf file (around line 78) where the bootstrap script assignment occurs. This will confirm that the conditions for including the bootstrap script are exactly as described.

Please run the script and share the output so we can verify that the conditions in the bootstrap script exactly match our expectations.

🏁 Script executed:

#!/bin/bash
# Show context from userdata.tf around the bootstrap_script assignment
sed -n '70,85p' userdata.tf

Length of output: 1270

Bootstrap Script Conditions Verified

The code in userdata.tf clearly implements the conditions as outlined in the review comment:

  • It requires a non-empty ami_image_id.
  • It requires a non-empty after_cluster_joining_userdata.
  • It checks that the OS is either "AL2" or "WINDOWS".

This confirms that the bootstrap script is only included when all three conditions are met, aligning with the PR objective of preventing bootstrapper duplication.

launch-template.tf (1)

200-203: LGTM! Good validation of after_cluster_joining_userdata usage.

The precondition correctly enforces that after-cluster userdata is only used with custom AMIs on supported OS types (AL2/Windows), preventing bootstrapper duplication.

README.yaml (1)

104-104: LGTM! Clear documentation of AMI behavior.

The added note clearly explains when the fourth userdata option is available and what happens with the default AMI, helping users understand the bootstrapper behavior.

variables.tf (2)

361-361: LGTM! Clear documentation of script types.

The description now correctly indicates that both bash and PowerShell scripts are supported via bootstrap.sh/ps1.

368-372: LGTM! Comprehensive documentation of after-cluster userdata limitations.

The description now clearly documents:

  1. Script type differences (bash for Linux, PowerShell for Windows)
  2. AMI type limitations (AL2 and Windows only)
  3. Custom AMI requirement
  4. Reason for limitations (EKS bootstrapper injection)
docs/terraform.md (1)

54-120: LGTM! Improved documentation formatting.

The HTML line breaks enhance readability while maintaining accurate descriptions that match the variables.tf changes.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~59-~59: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... will not
be associated to the nodes and you will need to provide another securi...

(COMMA_COMPOUND_SENTENCE)

[grammar] ~78-~78: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...

(TOO_ADJECTIVE_TO)

[grammar] ~82-~82: The verb ‘keep’ seems to be in the wrong form here.
Context: ...for unlimited length.
Set to null for keep the existing setting, which defaults to...

(FOR_VB)

[typographical] ~101-~101: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...

(RB_LY_COMMA)

[typographical] ~102-~102: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...

(RB_LY_COMMA)

[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ..._length](#input_random_pet_length) | In order to support "create before destroy" behavio...

(IN_ORDER_TO_PREMIUM)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@mergify mergify bot added the triage Needs triage label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gowiem Gowiem Awaiting requested review from Gowiem Gowiem is a code owner automatically assigned from cloudposse/contributors

@joe-niland joe-niland Awaiting requested review from joe-niland joe-niland is a code owner automatically assigned from cloudposse/contributors

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
triage Needs triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ChrisMcKee