Skip to content

feat: add eks auto mode #249

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

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

feat: add eks auto mode #249

wants to merge 18 commits into from

Conversation

RoseSecurity
Copy link

@RoseSecurity RoseSecurity commented Feb 5, 2025

what

  • Add the ability to use EKS Auto Mode:
  cluster_compute_config = {
    enabled    = true
    node_pools = ["general-purpose"]
  }

why

  • Managing Kubernetes clusters often involves configuring node groups, handling networking, and optimizing costs while maintaining performance and security. With EKS Auto Mode, AWS simplifies all of this. Auto Mode abstracts infrastructure details like node configuration and subnet management.

references

@RoseSecurity RoseSecurity requested review from a team as code owners February 5, 2025 16:34
Copy link

coderabbitai bot commented Feb 5, 2025

📝 Walkthrough

Walkthrough

This pull request introduces a new input parameter, cluster_compute_config, to the Terraform module. The changes include updates to documentation, example configurations, and code modifications across several files to support enhanced compute configuration settings for EKS Auto Mode. New local variables and dynamic blocks are added to conditionally apply these configurations, improving the module's flexibility.

Changes

Files Change Summary
README.md, docs/terraform.md Added new input parameter/variable cluster_compute_config with documentation on its usage, including options for enabling compute capabilities.
examples/complete/{fixtures.us-east-2.tfvars, main.tf, variables.tf} Introduced cluster_compute_config block/variable in examples, providing sample configuration (e.g., enabled, node_pools) for EKS clusters.
main.tf Added local variable auto_mode_enabled and dynamic blocks in the aws_eks_cluster resource to conditionally configure compute settings.
variables.tf New variable cluster_compute_config added with default {} and descriptive notes; existing variable region marked as obsolete.

Possibly related PRs

  • Bugfixes #250: The changes in this PR, which introduce a new input parameter cluster_compute_config for EKS configuration, are related as both involve modifications to the Terraform documentation and parameters, specifically enhancing the configurability of EKS resources.

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 5, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🔭 Outside diff range comments (1)
README.md (1)

73-73: Clear declaration of the new module input in the README.

The new input parameter cluster_compute_config is now documented in the module’s README. The description is detailed and aligned with both the module implementation and the documentation in docs/terraform.md. If you decide to update the variable’s type in variables.tf (as suggested above), consider updating this description accordingly to reflect the specific keys and their types.

Also applies to: 418-418

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 549ebd9 and 3cb909d.

📒 Files selected for processing (7)
  • README.md (1 hunks)
  • docs/terraform.md (1 hunks)
  • examples/complete/fixtures.us-east-2.tfvars (1 hunks)
  • examples/complete/main.tf (1 hunks)
  • examples/complete/variables.tf (1 hunks)
  • main.tf (2 hunks)
  • variables.tf (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (4)
examples/complete/fixtures.us-east-2.tfvars (1)

61-64: LGTM! Configuration aligns with EKS Auto Mode requirements.

The configuration correctly enables EKS Auto Mode with a "general-purpose" node pool.

examples/complete/main.tf (1)

118-118: LGTM! Configuration correctly passed to the module.

The cluster_compute_config variable is properly passed to the EKS cluster module.

main.tf (1)

18-18: LGTM! Local variable correctly handles missing configuration.

The auto_mode_enabled variable safely handles missing configuration and defaults to false.

docs/terraform.md (1)

73-73: Comprehensive documentation for the new input variable.

The added documentation for cluster_compute_config clearly explains its purpose for controlling compute configuration in EKS Auto Mode, including options for enabling compute, selecting node pools, and optionally specifying a node role ARN. Ensure that any future refinement of the variable’s type in variables.tf is mirrored here for consistency.

RoseSecurity and others added 2 commits February 5, 2025 11:39
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3cb909d and f4cbfd4.

📒 Files selected for processing (1)
  • variables.tf (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Branch
variables.tf

[warning] 43-43: Unnecessary whitespace found in the default assignment. Please remove extra spaces for consistency.

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

32-47: Structured variable definition for cluster_compute_config is well implemented.
The new variable now uses an object type with clearly defined optional keys (enabled, node_pools, node_role_arn), which greatly improves validation over using the generic any type. This change aligns with our best practices and the earlier recommendation to constrain input types.

🧰 Tools
🪛 GitHub Actions: Branch

[warning] 43-43: Unnecessary whitespace found in the default assignment. Please remove extra spaces for consistency.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f4cbfd4 and 08e9898.

📒 Files selected for processing (5)
  • README.md (2 hunks)
  • docs/terraform.md (2 hunks)
  • examples/complete/variables.tf (1 hunks)
  • main.tf (4 hunks)
  • variables.tf (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (10)
examples/complete/variables.tf (1)

134-148: Good variable declaration for cluster_compute_config.
The new variable is declared using a specific object type with optional fields (enabled, node_pools, and node_role_arn), and its multi‑line description clearly explains its purpose for EKS Auto Mode. This provides both validation and clarity for users configuring compute capabilities.

main.tf (3)

6-20: Review Local Variables for EKS Auto Mode.
The introduction of the local variables
use_elastic_load_balancing = var.kubernetes_elastic_load_balancing_enabled (line 6)
auto_mode_enabled = try(var.cluster_compute_config.enabled, false) (line 20)
correctly extracts user inputs. Make sure that the use of try() aligns with your expectations for the default behavior when no compute configuration is provided.


143-151: Dynamic block for storage_config.
This block conditionally renders storage configuration based on local.auto_mode_enabled and is implemented in a straightforward manner. It meets the intended design for EKS Auto Mode.


77-86: 🧹 Nitpick (assertive)

Dynamic block for compute_config.
This block conditionally renders the compute configuration based on the presence of settings in var.cluster_compute_config. To improve clarity and more directly reflect the intent, consider using the derived boolean value (local.auto_mode_enabled) in the condition. For example:

-    for_each = length(var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : []
+    for_each = local.auto_mode_enabled ? [var.cluster_compute_config] : []

This change makes it unmistakable that the block is only rendered when compute auto mode is enabled.

variables.tf (2)

32-46: New variable cluster_compute_config is well defined.
The variable now uses an object type with optional validations for a boolean, a list, and a string. The detailed multi‑line description offers clear guidance on its intended use in enabling compute capabilities for EKS Auto Mode.


113-117: New variable kubernetes_elastic_load_balancing_enabled added.
This boolean variable is straightforward with a descriptive explanation of its purpose—controlling the load balancing capability for the EKS Auto Mode cluster.

docs/terraform.md (2)

73-74: Documentation for cluster_compute_config is clear.
The explanation and the object type snippet provided in the docs accurately detail the structure and meaning of the parameter. This helps users correctly structure their configuration inputs for compute capability.


94-95: Documentation for kubernetes_elastic_load_balancing_enabled is concise.
The description succinctly explains its role in controlling load balancing behavior within an EKS Auto Mode cluster.

README.md (2)

418-419: README update for cluster_compute_config is informative.
The added documentation clearly explains the purpose of the configuration—detailing that it enables or disables compute capability and manages node pool settings within an EKS Auto Mode cluster.


439-440: README update for kubernetes_elastic_load_balancing_enabled is clear.
The documentation succinctly covers its function and default behavior, enhancing users’ understanding of how load balancing will be handled under EKS Auto Mode.

@RoseSecurity RoseSecurity marked this pull request as draft February 5, 2025 17:28
@RoseSecurity RoseSecurity marked this pull request as ready for review February 7, 2025 21:22
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 08e9898 and 9b809ee.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • docs/terraform.md (1 hunks)
  • examples/complete/fixtures.us-east-2.tfvars (1 hunks)
  • main.tf (3 hunks)
  • variables.tf (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (8)
main.tf (5)

18-18: LGTM! Well-structured local variable.

The local variable uses the try function correctly to handle cases where the config might not be provided.


67-68: LGTM! Correct handling of bootstrap configuration.

The code correctly sets bootstrap_self_managed_addons to false when auto mode is enabled, as required by EKS Auto Mode.


142-150: LGTM! Proper storage configuration for EKS Auto Mode.

The storage configuration correctly enables block storage when auto mode is enabled.


129-140: Verify elastic load balancing configuration.

The nested dynamic blocks for network config and elastic load balancing both check local.auto_mode_enabled, which might be redundant.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Description: Check AWS documentation for EKS Auto Mode network configuration requirements
curl -s "https://docs.aws.amazon.com/eks/latest/userguide/network-load-balancing.html"

Length of output: 47605


Action: Remove the redundant inner condition in the elastic load balancing block
The AWS documentation doesn’t call for a separate condition inside the elastic load balancing block. Since the outer dynamic block already uses local.auto_mode_enabled to gate whether any network configuration should be emitted, re-checking it within the nested elastic load balancing block is redundant. You can simplify the configuration by removing the inner conditional logic without impacting the intended behavior.


76-84: 🧹 Nitpick (assertive)

Improve the compute config block condition.

The current condition uses length(var.cluster_compute_config) > 0 which is less explicit than using local.auto_mode_enabled.

 dynamic "compute_config" {
-  for_each = length(var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : []
+  for_each = local.auto_mode_enabled ? [var.cluster_compute_config] : []

Likely invalid or redundant comment.

variables.tf (1)

31-45: LGTM! Well-structured variable definition.

The variable definition is excellent with:

  • Proper type constraints using optional fields
  • Clear descriptions for each field
  • Appropriate default values
docs/terraform.md (1)

73-73: LGTM! Documentation is clear and consistent.

The documentation for cluster_compute_config matches the implementation and provides clear information about the variable's purpose and structure.

examples/complete/fixtures.us-east-2.tfvars (1)

61-63: Verify EKS Auto Mode configuration against AWS documentation.

Let's verify that this configuration aligns with AWS's requirements for EKS Auto Mode.

✅ Verification successful

🌐 Web query:

What are the required configuration parameters for enabling EKS Auto Mode?

💡 Result:

To enable Amazon EKS Auto Mode, you must configure several core components and meet specific prerequisites. Here are the essential parameters and requirements:

## Core Configuration Parameters
1. **Enable Auto Mode Features**  
   You must enable three integrated capabilities in a single operation:
   - **Compute**: Node provisioning/autoscaling via Karpenter  
   - **Networking**: Automated load balancer management  
   - **Storage**: Persistent volume management via EBS CSI driver  

   *Implementation examples*:  
   ```bash
   # AWS CLI
   aws eks update-cluster-config \
     --name $CLUSTER_NAME \
     --compute-config enabled=true \
     --kubernetes-network-config '{"elasticLoadBalancing":{"enabled": true}}' \
     --storage-config '{"blockStorage":{"enabled": true}}'
   ```

   ```hcl
   # Terraform
   resource "aws_eks_cluster" "example" {
     name = "eks-auto-mode-demo"
     compute_config {
       enabled = true  # Enables all three components
     }
   }
   ```

2. **Authentication Mode**  
   Requires `API` or `ApiAndConfigMap` authentication mode instead of traditional `ConfigMap`-only access[1][3][7]:
   ```yaml
   authentication_mode: API  # Required for EKS Auto Mode compatibility
   ```

3. **IAM Permissions**  
   Cluster IAM Role must include these managed policies[3][5][7]:
   - `AmazonEKSComputePolicy`
   - `AmazonEKSBlockStoragePolicy`
   - `AmazonEKSLoadBalancingPolicy`
   - `AmazonEKSNetworkingPolicy`
   - `AmazonEKSClusterPolicy`

## Infrastructure Prerequisites
1. **Networking Setup**  
   - VPC subnets must be tagged for ELB discovery[1][7]:
     ```bash
     kubernetes.io/role/internal-elb=1
     kubernetes.io/cluster/<CLUSTER_NAME>=shared
     ```
   - Public/private subnet allocation strategy should be `AUTO`[1]

2. **Add-on Requirements** (for existing clusters)[3][9]
   | Add-on               | Minimum Version       |
   |----------------------|-----------------------|
   | VPC CNI              | v1.19.0-eksbuild.1    |
   | EBS CSI Driver       | v1.37.0-eksbuild.1    |
   | CSI Snapshot Controller | v8.1.0-eksbuild.2  |

## Implementation Notes
- New clusters should use infrastructure-as-code tools like Pulumi[1], Terraform[7], or eksctl[4] for simplified setup  
- Existing clusters require workload migration strategies for load balancers and storage volumes[2][3]  
- Auto Mode manages security configurations including IMDSv2 settings and node rotation every 21 days by default[9]

For production deployments, ensure your VPC meets EKS networking requirements[5] and review automated component management details[8][9] before implementation.

Citations:

- 1: https://www.pulumi.com/blog/aws-eks-auto-mode/
- 2: https://dev.to/playtomic/migrating-your-cluster-to-eks-auto-mode-what-you-need-to-know-before-taking-the-leap-253m
- 3: https://docs.aws.amazon.com/eks/latest/userguide/auto-enable-existing.html
- 4: https://github.com/eksctl-io/eksctl/blob/main/userdocs/src/usage/auto-mode.md
- 5: https://docs.aws.amazon.com/eks/latest/userguide/create-cluster-auto.html
- 6: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_cluster
- 7: https://community.aws/content/2sV2SNSoVeq23OvlyHN2eS6lJfa/amazon-eks-auto-mode-enabled-build-your-super-powered-cluster
- 8: https://docs.aws.amazon.com/eks/latest/userguide/getting-started-automode.html
- 9: https://docs.aws.amazon.com/eks/latest/userguide/automode.html
- 10: https://repost.aws/it/questions/QUN6VSWeChQMWTjmFJ9pL2nw/how-to-provision-gpu-nodes-in-eks-auto-mode-type-cluster

EKS Auto Mode Configuration Review

• The AWS documentation indicates that enabling EKS Auto Mode requires a compute configuration block with at least "enabled = true," which the current snippet provides.
• While the minimal configuration is valid, the PR description and broader changes suggest that additional optional fields (for example, node_pools and node_role_arn) are available and may be useful for demonstrating full feature support.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
main.tf (3)

75-84: Dynamic Compute Config Block – For_Each Condition
The dynamic block for compute_config currently uses the condition length(var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : [] to decide if the block should be rendered. For clarity and to better align with the auto mode logic, consider revising this condition to directly depend on local.auto_mode_enabled (e.g., for_each = local.auto_mode_enabled ? [var.cluster_compute_config] : []). Additionally, ensure that the fallback for node_role_arn via aws_iam_role.default[0].arn is valid in all environments where auto mode is active.


129-140: Nested Dynamic Block Redundancy in Kubernetes Network Config
Within the dynamic block for kubernetes_network_config handling elastic load balancing, the outer block already conditions creation on local.auto_mode_enabled (for_each = local.auto_mode_enabled ? [1] : []). The inner dynamic block for elastic_load_balancing repeats this same condition. Consider simplifying by removing the redundant inner for_each check so that the nested block is unconditionally rendered when the outer block is created.


142-150: Storage Config Block – Setting Block Storage Enabled Flag
The dynamic storage_config block is conditionally created when local.auto_mode_enabled is true, and it sets block_storage { enabled = local.auto_mode_enabled }. Since the block’s existence already implies that auto mode is active, you might simplify this by directly setting enabled = true within the block for clarity.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9b809ee and 5eac273.

⛔ Files ignored due to path filters (1)
  • README.md is excluded by !README.md
📒 Files selected for processing (4)
  • docs/terraform.md (1 hunks)
  • examples/complete/main.tf (1 hunks)
  • main.tf (3 hunks)
  • variables.tf (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • examples/complete/main.tf
  • docs/terraform.md
  • variables.tf
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (2)
main.tf (2)

17-18: Auto Mode Variable Initialization
The new local variable auto_mode_enabled is defined using try(var.cluster_compute_config.enabled, false). This is a good safeguard to default the flag to false when cluster_compute_config is not provided. Ensure that the cluster_compute_config variable itself is defined with appropriate defaults so that its structure is consistent.


61-68: Bootstrap Self-Managed Addons Configuration
The attribute bootstrap_self_managed_addons is now conditionally set to false when auto mode is enabled (local.auto_mode_enabled ? false : var.bootstrap_self_managed_addons_enabled), which aligns with the requirement for EKS Auto Mode. Verify that the variable var.bootstrap_self_managed_addons_enabled is correctly defined and that setting it to false in auto mode meets all customer expectations.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
main.tf (1)

75-84: ⚠️ Potential issue

Review Dynamic Block for Compute Config
The introduced dynamic block for compute_config addresses the new compute configuration for EKS Auto Mode. However, the condition:

for_each = local.auto_mode_enabled && (var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : []

is problematic because using the > operator on var.cluster_compute_config (an object) is likely invalid. Consider simplifying the condition to check only for auto mode activation. For example:

-    for_each = local.auto_mode_enabled && (var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : []
+    for_each = local.auto_mode_enabled ? [var.cluster_compute_config] : []

This change ensures that the compute_config block is rendered solely based on the auto mode flag.

🧹 Nitpick comments (1)
main.tf (1)

142-150: Storage Config Dynamic Block Validation
The added dynamic block for storage_config and its nested block_storage configuration enhances the module's features under auto mode. Please verify that the aws_eks_cluster resource supports a storage_config block with a nested block_storage sub-block. Also, ensure that this new feature is documented appropriately to avoid confusion during module usage.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between cd59e29 and 4b92f0f.

📒 Files selected for processing (1)
  • main.tf (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (3)
main.tf (3)

16-19: Auto Mode Enabled Initialization
The new local variable auto_mode_enabled is defined using try(var.cluster_compute_config.enabled, false), which safely defaults to false when no compute configuration is provided. This approach is clear and ensures that subsequent conditional logic relying on auto mode is robust.


67-69: Conditional Handling of Bootstrap Addons
Setting bootstrap_self_managed_addons to false when auto_mode_enabled is true correctly enforces the requirement for EKS Auto Mode. This conditional override ensures that self-managed addons are disabled as needed when auto mode is active.


113-126: Mutually Exclusive Network Configuration Blocks
The dynamic blocks for kubernetes_network_config handling IPv4 and IPv6 are configured to be mutually exclusive based on local.use_ipv6:

  • The IPv4 block (lines 113–118) is rendered when IPv6 is disabled.
  • The IPv6 block (lines 121–126) is rendered when IPv6 is enabled.

This clear segregation helps maintain both readability and correctness of the network configuration.

@RoseSecurity
Copy link
Author

/terratest

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
main.tf (2)

17-18: Auto Mode Toggle Initialization
The new local variable auto_mode_enabled is defined using try(var.cluster_compute_config.enabled, false), which sets a default of false if the attribute is absent. This approach works, but consider using a more explicit lookup (e.g., lookup(var.cluster_compute_config, "enabled", false)) if the variable is always defined as an object.


75-84: Evaluate the Compute Config Dynamic Block Condition
The dynamic block for compute_config uses the condition:

for_each = local.auto_mode_enabled && length(var.cluster_compute_config) > 0 ? [var.cluster_compute_config] : []

If cluster_compute_config is defined as an object, using length() will count its attributes and may always be greater than 0. Consider whether the length check is necessary or if the condition should simply be based on local.auto_mode_enabled. Also, verify that aws_iam_role.default is the intended fallback for node_role_arn.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4b92f0f and 90498be.

📒 Files selected for processing (1)
  • main.tf (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (4)
main.tf (4)

67-68: Enforce Bootstrap Configuration for Auto Mode
The attribute bootstrap_self_managed_addons is now conditionally set to false when auto mode is enabled, which aligns with the requirement for EKS Auto Mode. Ensure that downstream logic and documentation reflect this change.


113-119: IPv4 Network Configuration Dynamic Block
The dynamic block conditioned on local.use_ipv6 ? [] : compact([var.service_ipv4_cidr]) correctly renders an IPv4 network configuration only when IPv6 is disabled. If var.service_ipv4_cidr can ever be an empty or null value, the use of compact() helps filter it out.


121-127: IPv6 Network Configuration Dynamic Block
The block with for_each = local.use_ipv6 ? [true] : [] explicitly sets ip_family to "ipv6" when IPv6 is enabled. This is a clear and straightforward implementation.


142-150: Storage Configuration Based on Auto Mode
The dynamic block for storage_config enables block_storage conditionally based on local.auto_mode_enabled. The logic is straightforward and aligns with the auto mode configuration intent.

@topikachu
Copy link

When will this pr merged?
We are going to create our new cluster in auto mode.

@RoseSecurity RoseSecurity added enhancement New feature or request minor New features that do not break anything needs-cloudposse Needs Cloud Posse assistance labels Mar 7, 2025
Copy link

mergify bot commented Mar 7, 2025

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@GabisCampana
Copy link

@Nuru

Comment on lines +67 to +68
# Enabling EKS Auto Mode also requires that bootstrap_self_managed_addons is set to false
bootstrap_self_managed_addons = local.auto_mode_enabled ? false : var.bootstrap_self_managed_addons_enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider it a bad idea to silently change input configuration like this.

Because this is a configuration conflict, and may change in the future, my first preference is to allow AWS to throw an error message rather than try to fix it, because then if AWS relaxes this condition, we don't need to make any changes.

An acceptable alternative is to add this check as a precondition and report an error.

Comment on lines +133 to +137
dynamic "elastic_load_balancing" {
for_each = local.auto_mode_enabled ? [1] : []
content {
enabled = local.auto_mode_enabled
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really a requirement that elastic load balancing be enabled for auto mode? If so:

Suggested change
dynamic "elastic_load_balancing" {
for_each = local.auto_mode_enabled ? [1] : []
content {
enabled = local.auto_mode_enabled
}
elastic_load_balancing {
enabled = true
}

If it not a requirement, then it should be an option in the auto mode configuration object.


content {
block_storage {
enabled = local.auto_mode_enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, if required, then just use true to emphasize the fact. If not required, make it a separate input option.

Suggested change
enabled = local.auto_mode_enabled
enabled = true

Comment on lines +61 to +63
cluster_compute_config = {
enabled = false
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but for something this complex and impactful, in addition to the specific changes I called out elsewhere, I want to see a separate test of auto mode in the automated testing. It can run in parallel.

Because of a quirk in Terratest, vars set in a test do not override vars set in a varfile set in the test, so my plan would be:

  • Put a full configuration of the enabled cluster_compute_config in the varfile.
  • Add a var to the example: cluster_auto_mode_enabled which defaults to false.
  • Update the main.tf to honor that var. This would include, if I understand correctly, not creating the eks_node_group when in auto mode (enabled = !var.cluster_auto_mode_enabled).
  • Clone TestExamplesComplete to create TestExamplesAutoMode, adding "cluster_auto_mode_enabled": true to the test vars. Adjust the tests as appropriate to ensure we have a functioning cluster in auto mode.

Configuration block with compute configuration for EKS Auto Mode

enabled: Request to enable or disable the compute capability on your EKS Auto Mode cluster. If the compute capability is enabled, EKS Auto Mode will create and delete EC2 Managed Instances in your Amazon Web Services account.
node_pools: Optional configuration for node pools that defines the compute resources for your EKS Auto Mode cluster. Valid options are general-purpose and system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if left undefined?


enabled: Request to enable or disable the compute capability on your EKS Auto Mode cluster. If the compute capability is enabled, EKS Auto Mode will create and delete EC2 Managed Instances in your Amazon Web Services account.
node_pools: Optional configuration for node pools that defines the compute resources for your EKS Auto Mode cluster. Valid options are general-purpose and system.
node_role_arn: Optional ARN of the IAM Role EKS will assign to EC2 Managed Instances in your EKS Auto Mode cluster.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any gotchas with the default IAM role EKS will use, e.g edge cases? If so, then we should probably have an option to create and use the role we create when we use the old/existing node pool. In fact, that should probably be an option in any case.

@matanyall
Copy link

Hey, any updates on this?

@RoseSecurity
Copy link
Author

@matanyall this is a large feature, so I am going to work on creating tests specifically to accommodate this feature while ensuring nothing else with the module breaks. Sorry for the delays

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor New features that do not break anything needs-cloudposse Needs Cloud Posse assistance triage Needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants