added aks node fleet and corrected input var.azs#505
added aks node fleet and corrected input var.azs#505rauneetverma wants to merge 1 commit intomasterfrom
Conversation
rauneetverma
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
WalkthroughThis pull request introduces a new AKS node fleet module for Kubernetes node pool management alongside an update to the existing Kubernetes node pool base module. The AKS node fleet provides a parent module that orchestrates multiple node pool configurations through a nested flavor module, with schema validation and Terraform resource definitions for Azure Kubernetes Service node pools. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@modules/kubernetes_node_pool/aks/0.1/main.tf`:
- Line 58: The zones expression is using compact(...) only for the length check
but then indexing the original azs list, which can return empty/null; change it
to consistently use the compacted list (e.g., compute valid_azs =
compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs) or
an inline compact) and then set zones = length(local.valid_azs) == 0 ? null :
[local.valid_azs[0]] so you always pick the first non-empty AZ (update the
reference to use local.valid_azs or compact(...) in the ternary and keep the
rest of the attribute name unchanged).
In `@modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf`:
- Line 25: The current ternary that returns the constant "windos" when
local.os_type == "Windows" collapses distinct Windows pool names; update the
expression that sets the name (the local.name conditional) to generate a
deterministic, unique Windows-safe name instead — for example, produce a
truncated version of local.name (e.g., first N chars) or truncated+hash (e.g.,
substr(local.name,0,N) + "-" + substr(sha256(local.name),0,M)) when
local.os_type == "Windows" so you preserve uniqueness across pools while keeping
the name length valid.
- Around line 17-18: Guard the for-expressions against missing spec.taints by
replacing direct iteration over var.instance.spec.taints with a safe fallback
(e.g. iterate over try(var.instance.spec.taints, [])); update the taints
assignment (the line using taints = [for taint in var.instance.spec.taints :
...]) and any outputs that reference var.instance.spec.taints to use the same
try(...) fallback, and apply this change in both the aks_node_fleet and
kubernetes_node_pool modules so the for-expressions and outputs won't fail when
spec or taints is omitted.
- Line 24: The provider meta-argument is using a string literal
"azurerm3-105-0"; change it to an unquoted identifier by removing the quotes
(i.e., set provider = azurerm3-105-0) and ensure the corresponding provider
block uses the matching alias azurerm3-105-0 so Terraform can resolve the
referenced provider.
- Around line 2-12: Add a local for the autoscaling flag and make node_count
conditional: extract enable_auto_scaling from local.aks_advanced into a new
local (e.g., local.enable_auto_scaling = lookup(local.aks_advanced,
"enable_auto_scaling", false)) and replace the unconditional node_count with a
computed local (e.g., local.node_count = local.enable_auto_scaling ? null :
lookup(local.aks_advanced, "node_count", 1)); update any references to
node_count in the AKS node pool config to use local.node_count so node_count is
null when autoscaling is enabled and avoids provider errors.
In `@modules/node_fleet/aks_node_fleet/0.1/facets.yaml`:
- Around line 98-105: The disk_size schema currently uses pattern "^[0-9]+G$"
which permits values outside the documented 50–1000G range; update the pattern
for the disk_size property in facets.yaml (the disk_size field) to enforce
numeric values between 50 and 1000 followed by "G" (for example use a regex that
matches 50–99, 100–999, or 1000 such as one that captures [5-9]\d, [1-9]\d{2},
or 1000) so the validation aligns with the x-ui-error-message.
In `@modules/node_fleet/aks_node_fleet/0.1/locals.tf`:
- Around line 10-15: processed_taints is building a numeric-keyed map from
local.taints but the AKS module expects an array; replace usages of
processed_taints with the original array (local.taints) or convert
processed_taints into a proper list—i.e., stop transforming into a map in
locals.tf (remove the for idx, taint : ... map) and set the module input taints
= local.taints in main.tf (or create a list comprehension that yields an array)
so the input matches the variable/schema type: array.
In `@modules/node_fleet/aks_node_fleet/0.1/main.tf`:
- Line 14: The default for disk_size in the lookup call (disk_size =
lookup(each.value, "disk_size", "G")) is invalid and violates the pattern
^[0-9]+G$; change the default to null for an optional parameter or a valid size
string (e.g., "50G") so downstream trim/digits conversion doesn't produce an
empty string — update the lookup default and any callers expecting a non-null
string accordingly, and ensure any logic using trim(disk_size, "G") guards
against null/empty before integer conversion.
🧹 Nitpick comments (1)
modules/node_fleet/aks_node_fleet/0.1/aks/0.1/variables.tf (1)
13-19: Prefernulldefaults over empty strings for numeric fields.
Empty strings can leak into node pool configuration and cause provider validation errors if defaults are used.✅ Suggested defaults
- "instance_type" = "", - "min_node_count" = "", - "max_node_count" = "", - "disk_size" = "", + "instance_type" = null, + "min_node_count" = null, + "max_node_count" = null, + "disk_size" = null,
| tags = local.tags | ||
| spot_max_price = local.priority == "Spot" ? lookup(local.aks_advanced, "spot_max_price", "-1") : local.spot_max_price | ||
| zones = length(compact(var.cluster.azs)) == 0 ? null : [var.cluster.azs[0]] | ||
| zones = length(compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)) == 0 ? null : [var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs[0]] |
There was a problem hiding this comment.
Inconsistent use of compact() may result in selecting an empty zone value.
The condition uses compact(...) to filter out empty strings/nulls when checking length, but the index access [0] is performed on the original (non-compacted) list. If the original list starts with empty or null values (e.g., ["", "eastus"]), the length check passes but azs[0] returns an empty string instead of the first valid zone.
🐛 Proposed fix: use compacted list consistently
- zones = length(compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)) == 0 ? null : [var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs[0]]
+ zones = length(compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)) == 0 ? null : [compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)[0]]Alternatively, for better readability, consider extracting to a local:
locals {
# ... existing locals ...
valid_azs = compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)
}
# Then on line 58:
zones = length(local.valid_azs) == 0 ? null : [local.valid_azs[0]]📝 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.
| zones = length(compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)) == 0 ? null : [var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs[0]] | |
| zones = length(compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)) == 0 ? null : [compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs)[0]] |
🤖 Prompt for AI Agents
In `@modules/kubernetes_node_pool/aks/0.1/main.tf` at line 58, The zones
expression is using compact(...) only for the length check but then indexing the
original azs list, which can return empty/null; change it to consistently use
the compacted list (e.g., compute valid_azs =
compact(var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs) or
an inline compact) and then set zones = length(local.valid_azs) == 0 ? null :
[local.valid_azs[0]] so you always pick the first non-empty AZ (update the
reference to use local.valid_azs or compact(...) in the ternary and keep the
rest of the attribute name unchanged).
| aks_advanced = lookup(lookup(lookup(var.instance, "advanced", {}), "aks", {}), "node_pool", {}) | ||
| aks_upgrade_settings = lookup(local.aks_advanced, "upgrade_settings", {}) | ||
| aks_kubelet_config = lookup(local.aks_advanced, "kubelet_config", {}) | ||
| aks_linux_os_config = lookup(local.aks_advanced, "linux_os_config", {}) | ||
| aks_sysctl_config = lookup(local.aks_linux_os_config, "sysctl_config", {}) | ||
| metadata = lookup(var.instance, "metadata", {}) | ||
| priority = lookup(local.aks_advanced, "priority", "Regular") | ||
| user_defined_tags = lookup(local.aks_advanced, "tags", {}) | ||
| facets_defined_cloud_tags = lookup(var.environment, "cloud_tags", {}) | ||
| tags = merge(local.user_defined_tags, local.facets_defined_cloud_tags) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "main.tf" -path "*/aks_node_fleet/*" -o -name "main.tf" -path "*/aks/0.1/*" | head -20Repository: Facets-cloud/facets-modules
Length of output: 221
🏁 Script executed:
cd modules && find . -name "main.tf" | grep -i aks | head -10Repository: Facets-cloud/facets-modules
Length of output: 197
🏁 Script executed:
git ls-files | grep -E "aks.*main\.tf|main\.tf.*aks" | head -20Repository: Facets-cloud/facets-modules
Length of output: 215
🏁 Script executed:
git ls-files | grep "node_fleet" | head -20Repository: Facets-cloud/facets-modules
Length of output: 707
🌐 Web query:
Terraform azurerm_kubernetes_cluster_node_pool enable_auto_scaling node_count requirements
💡 Result:
- If
enable_auto_scaling = true(AzureRM v3.x) you must also setmin_countandmax_countfor the node pool. - With autoscaling enabled, don’t manage
node_count(typically omit it / set it tonull). Changingnode_countwhile autoscaling is enabled can fail with an error like “cannot changenode_countwhenenable_auto_scalingis set totrue”. [2] - Ensure your effective node count stays within your autoscaler range over time; Azure may temporarily run outside the min/max (for example after restart) and later converge back as autoscaling actions occur. [3]
- Provider version note: in AzureRM v4.x the argument was renamed from
enable_auto_scaling→auto_scaling_enabled. [1]
Example (recommended pattern when autoscaling is on):
resource "azurerm_kubernetes_cluster_node_pool" "user" {
kubernetes_cluster_id = azurerm_kubernetes_cluster.aks.id
name = "user"
vm_size = "Standard_D2s_v3"
enable_auto_scaling = true
min_count = 1
max_count = 5
node_count = null # or omit entirely
}(Pattern shown in practice.) [4]
Sources: [1] [2] [3] [4]
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tfRepository: Facets-cloud/facets-modules
Length of output: 10890
🏁 Script executed:
wc -l modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tfRepository: Facets-cloud/facets-modules
Length of output: 128
Make node_count conditional on autoscaling to prevent provider errors.
When enable_auto_scaling is true, setting node_count causes the Terraform provider to error or drift. The current code (lines 34–37) unconditionally sets node_count regardless of the autoscaling state. Extract enable_auto_scaling to a local variable in the locals block (lines 2–11) and conditionally set node_count to null when autoscaling is enabled.
⚙️ Suggested fix
locals {
aks_advanced = lookup(lookup(lookup(var.instance, "advanced", {}), "aks", {}), "node_pool", {})
aks_upgrade_settings = lookup(local.aks_advanced, "upgrade_settings", {})
aks_kubelet_config = lookup(local.aks_advanced, "kubelet_config", {})
aks_linux_os_config = lookup(local.aks_advanced, "linux_os_config", {})
aks_sysctl_config = lookup(local.aks_linux_os_config, "sysctl_config", {})
metadata = lookup(var.instance, "metadata", {})
priority = lookup(local.aks_advanced, "priority", "Regular")
user_defined_tags = lookup(local.aks_advanced, "tags", {})
facets_defined_cloud_tags = lookup(var.environment, "cloud_tags", {})
tags = merge(local.user_defined_tags, local.facets_defined_cloud_tags)
+ enable_auto_scaling = lookup(local.aks_advanced, "enable_auto_scaling", true)
name = lookup(local.metadata, "name", var.instance_name)
taints = [for taint in var.instance.spec.taints : "${taint.key}=${taint.value}:${taint.effect}"]
spot_max_price = lookup(local.aks_advanced, "spot_max_price", null)
os_type = lookup(local.aks_advanced, "os_type", "Linux")
}
resource "azurerm_kubernetes_cluster_node_pool" "node_pool" {
- enable_auto_scaling = lookup(local.aks_advanced, "enable_auto_scaling", true)
+ enable_auto_scaling = local.enable_auto_scaling
max_count = var.instance.spec.max_node_count
min_count = var.instance.spec.min_node_count
- node_count = lookup(local.aks_advanced, "node_count", var.instance.spec.min_node_count)
+ node_count = local.enable_auto_scaling ? null : lookup(local.aks_advanced, "node_count", var.instance.spec.min_node_count)🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf` around lines 2 - 12,
Add a local for the autoscaling flag and make node_count conditional: extract
enable_auto_scaling from local.aks_advanced into a new local (e.g.,
local.enable_auto_scaling = lookup(local.aks_advanced, "enable_auto_scaling",
false)) and replace the unconditional node_count with a computed local (e.g.,
local.node_count = local.enable_auto_scaling ? null : lookup(local.aks_advanced,
"node_count", 1)); update any references to node_count in the AKS node pool
config to use local.node_count so node_count is null when autoscaling is enabled
and avoids provider errors.
| taints = [for taint in var.instance.spec.taints : "${taint.key}=${taint.value}:${taint.effect}"] | ||
| spot_max_price = lookup(local.aks_advanced, "spot_max_price", null) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of spec.taints in the codebase
rg -n 'spec\.taints' -C 3Repository: Facets-cloud/facets-modules
Length of output: 2927
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf | head -50Repository: Facets-cloud/facets-modules
Length of output: 3459
🏁 Script executed:
# Find variable definitions related to instance.spec
rg -n 'variable.*instance|spec.*taints' -A 5 modules/node_fleet/aks_node_fleet/0.1/aks/0.1/Repository: Facets-cloud/facets-modules
Length of output: 2184
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/variables.tfRepository: Facets-cloud/facets-modules
Length of output: 1188
🏁 Script executed:
# Check how many modules have this spec.taints pattern
find . -name "main.tf" -o -name "output.tf" | xargs rg -l 'spec\.taints' | sortRepository: Facets-cloud/facets-modules
Length of output: 280
Guard against missing spec.taints across all references to avoid plan errors.
The default value in the variable definition only applies if var.instance is completely omitted. If a user provides a partial override without the taints key, the for-expressions will fail. Apply the fix consistently to both main.tf and output.tf in both the aks_node_fleet and kubernetes_node_pool modules.
🛡️ Locations requiring fixes
modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf:17modules/node_fleet/aks_node_fleet/0.1/aks/0.1/output.tf:4modules/kubernetes_node_pool/aks/0.1/main.tf:17modules/kubernetes_node_pool/aks/0.1/output.tf:4
Suggested fix pattern:
- taints = [for taint in var.instance.spec.taints : ...]
+ taints = [for taint in try(var.instance.spec.taints, []) : ...]📝 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.
| taints = [for taint in var.instance.spec.taints : "${taint.key}=${taint.value}:${taint.effect}"] | |
| spot_max_price = lookup(local.aks_advanced, "spot_max_price", null) | |
| taints = [for taint in try(var.instance.spec.taints, []) : "${taint.key}=${taint.value}:${taint.effect}"] | |
| spot_max_price = lookup(local.aks_advanced, "spot_max_price", null) |
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf` around lines 17 - 18,
Guard the for-expressions against missing spec.taints by replacing direct
iteration over var.instance.spec.taints with a safe fallback (e.g. iterate over
try(var.instance.spec.taints, [])); update the taints assignment (the line using
taints = [for taint in var.instance.spec.taints : ...]) and any outputs that
reference var.instance.spec.taints to use the same try(...) fallback, and apply
this change in both the aks_node_fleet and kubernetes_node_pool modules so the
for-expressions and outputs won't fail when spec or taints is omitted.
| } | ||
|
|
||
| resource "azurerm_kubernetes_cluster_node_pool" "node_pool" { | ||
| provider = "azurerm3-105-0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure to locate the file
find . -name "main.tf" -path "*aks*" 2>/dev/null | head -20Repository: Facets-cloud/facets-modules
Length of output: 221
🏁 Script executed:
# Let's also find and read the specific file mentioned in the review
find . -path "*/aks_node_fleet/0.1/aks/0.1/main.tf" 2>/dev/nullRepository: Facets-cloud/facets-modules
Length of output: 126
🏁 Script executed:
# Search for provider declarations and meta-argument usage
rg -n 'provider\s*=' -C3Repository: Facets-cloud/facets-modules
Length of output: 50385
🏁 Script executed:
# Search for terraform provider blocks (provider configuration)
rg -n '^\s*provider\s*"' -C3Repository: Facets-cloud/facets-modules
Length of output: 5334
🏁 Script executed:
# Search for alias declarations
rg -nP 'alias\s*=' -C2Repository: Facets-cloud/facets-modules
Length of output: 5986
🏁 Script executed:
# Read the specific file around line 24
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf | head -50Repository: Facets-cloud/facets-modules
Length of output: 3459
🏁 Script executed:
# Search for provider declarations of azurerm (no "provider" keyword, looking for "provider "azurerm" declarations)
rg 'provider\s+"azurerm' modules/node_fleet/aks_node_fleet/ -A5Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check if there's a variables or provider configuration file in the aks directory
find modules/node_fleet/aks_node_fleet/0.1/aks/0.1 -type f -name "*.tf" | head -10Repository: Facets-cloud/facets-modules
Length of output: 239
🏁 Script executed:
# Search for provider blocks to see if azurerm3-105-0 or azurerm is configured
rg 'provider\s+"azurerm' -A10 modules/node_fleet/aks_node_fleet/0.1/Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check for required_providers blocks that might define these aliases
rg 'required_providers' modules/node_fleet/aks_node_fleet/0.1/ -A20Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Also check parent directory for provider definitions
rg 'provider\s+"azurerm' modules/node_fleet/aks_node_fleet/0.1/main.tf -A10Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check what files exist at the parent level
ls -la modules/node_fleet/aks_node_fleet/0.1/Repository: Facets-cloud/facets-modules
Length of output: 534
🏁 Script executed:
# Check the parent main.tf file for provider configuration
cat -n modules/node_fleet/aks_node_fleet/0.1/main.tfRepository: Facets-cloud/facets-modules
Length of output: 1942
🏁 Script executed:
# Check for any .tf files that might define the provider
cat -n modules/node_fleet/aks_node_fleet/0.1/locals.tfRepository: Facets-cloud/facets-modules
Length of output: 708
🏁 Script executed:
# Search broader in the node_fleet directory for provider definitions
find modules/node_fleet -name "*.tf" -exec grep -l 'provider\s*"azurerm' {} \;Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Also check if there are any terraform files defining providers at module level
find modules/node_fleet/aks_node_fleet -name "versions.tf" -o -name "provider.tf"Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Search for how provider meta-arguments are used in valid contexts in the codebase
rg 'provider\s*=\s*[a-z]+' modules/ | head -20Repository: Facets-cloud/facets-modules
Length of output: 1447
🏁 Script executed:
# Look for provider blocks with alias definitions in the entire codebase
rg -A3 'provider\s+"(aws|azurerm)"' modules/ | grep -A3 'alias'Repository: Facets-cloud/facets-modules
Length of output: 676
🏁 Script executed:
# Check the specific aks/0.1 subdirectory more carefully
cat modules/node_fleet/aks_node_fleet/0.1/aks/0.1/variables.tfRepository: Facets-cloud/facets-modules
Length of output: 873
🏁 Script executed:
# List all files in the aks/0.1 directory
ls -la modules/node_fleet/aks_node_fleet/0.1/aks/0.1/Repository: Facets-cloud/facets-modules
Length of output: 480
🏁 Script executed:
# Verify Terraform provider meta-argument syntax by checking what's valid vs invalid
# Search for any documentation or comments about provider configuration in the codebase
rg -i 'provider.*meta' modules/ --max-count=5Repository: Facets-cloud/facets-modules
Length of output: 675
🏁 Script executed:
# Look for how "azurerm" or similar provider aliases are configured in this codebase
rg 'provider\s+"azurerm' modules/ -C5 | head -40Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check if there's a pattern of how these string-based providers should be corrected
rg 'provider\s*=\s*"' modules/ | head -20Repository: Facets-cloud/facets-modules
Length of output: 2068
🏁 Script executed:
# Let's verify Terraform syntax by looking for actual provider block definitions
# that might define these aliases
find modules -name "versions.tf" -o -name "provider.tf" | head -10Repository: Facets-cloud/facets-modules
Length of output: 933
🏁 Script executed:
# Check if these string-based provider references are documented or commented
rg -B2 'provider\s*=\s*"(aws5|azurerm3-105-0)"' modules/ | head -30Repository: Facets-cloud/facets-modules
Length of output: 2601
🏁 Script executed:
# Search for any provider block definitions with alias that matches what's being used
rg 'provider\s+"(azurerm|aws)"' modules/node_fleet/aks_node_fleet/ -A5Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Broader search for provider configuration blocks
rg 'provider\s+"azurerm"' modules/ -A10 | head -50Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Let's check one of the versions.tf files to see how providers are actually configured
cat modules/dynamodb/default/0.2/terraform-aws-dynamodb-table-2.0.0/versions.tfRepository: Facets-cloud/facets-modules
Length of output: 191
🏁 Script executed:
# Check a versions.tf in the aks module area to see provider config
find modules/node_fleet/aks_node_fleet -name "versions.tf" 2>/dev/nullRepository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Let's verify the exact Terraform syntax rule by checking examples with comments
rg 'provider\s*=' modules/ingress/aws_ecs_alb/0.1/main.tf -B2 -A1 | head -30Repository: Facets-cloud/facets-modules
Length of output: 1333
🏁 Script executed:
# Let's check if there are any provider block configurations in the aks module parent directories
cat modules/node_fleet/aks_node_fleet/0.1/main.tf | grep -A10 'provider'Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Search more broadly for provider block definitions
rg 'provider\s+"azurerm"' modules/ -A10Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check the specific file more carefully for all provider references
grep -n 'provider' modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tfRepository: Facets-cloud/facets-modules
Length of output: 116
🏁 Script executed:
# Let's confirm the Terraform syntax rule by checking what identifiers would be valid
# Search for any provider blocks that might define azurerm or azurerm3 with an alias
rg 'provider\s+"azurerm' modules/ -A10 | head -50Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Check if there's any provider configuration in parent modules or root
find modules/node_fleet -type f -name "*.tf" -exec grep -l 'provider\s*"azurerm' {} \;Repository: Facets-cloud/facets-modules
Length of output: 53
🏁 Script executed:
# Let's look at a working example with proper syntax for comparison
rg 'provider\s*=\s*[a-z]+\.[a-z]+' modules/ | head -10Repository: Facets-cloud/facets-modules
Length of output: 949
Remove the quotes from the provider meta-argument.
The provider meta-argument must reference an unquoted identifier, not a string literal. Change provider = "azurerm3-105-0" to provider = azurerm3-105-0 (or the appropriate provider alias reference if "azurerm3-105-0" is intended as an alias name). Verify that the corresponding provider configuration block is defined with this alias elsewhere in the configuration.
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf` at line 24, The
provider meta-argument is using a string literal "azurerm3-105-0"; change it to
an unquoted identifier by removing the quotes (i.e., set provider =
azurerm3-105-0) and ensure the corresponding provider block uses the matching
alias azurerm3-105-0 so Terraform can resolve the referenced provider.
|
|
||
| resource "azurerm_kubernetes_cluster_node_pool" "node_pool" { | ||
| provider = "azurerm3-105-0" | ||
| name = local.os_type == "Windows" && length(local.name) >= 6 ? "windos" : local.name //replacing the name to windos if os is window to |
There was a problem hiding this comment.
Avoid constant Windows node pool names to prevent collisions.
Hard-coding "windos" collapses distinct Windows pool names into one, which will conflict in multi-pool deployments. Prefer a deterministic truncation (or truncation + hash) that preserves uniqueness.
🪪 Suggested fix
- name = local.os_type == "Windows" && length(local.name) >= 6 ? "windos" : local.name //replacing the name to windos if os is window to
+ name = local.os_type == "Windows" ? substr(local.name, 0, 6) : local.name📝 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.
| name = local.os_type == "Windows" && length(local.name) >= 6 ? "windos" : local.name //replacing the name to windos if os is window to | |
| name = local.os_type == "Windows" ? substr(local.name, 0, 6) : local.name |
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf` at line 25, The
current ternary that returns the constant "windos" when local.os_type ==
"Windows" collapses distinct Windows pool names; update the expression that sets
the name (the local.name conditional) to generate a deterministic, unique
Windows-safe name instead — for example, produce a truncated version of
local.name (e.g., first N chars) or truncated+hash (e.g., substr(local.name,0,N)
+ "-" + substr(sha256(local.name),0,M)) when local.os_type == "Windows" so you
preserve uniqueness across pools while keeping the name length valid.
| disk_size: | ||
| title: Disk Size | ||
| description: Size of the disk in GiB for nodes in this node pool | ||
| type: string | ||
| pattern: ^[0-9]+G$ | ||
| x-ui-placeholder: eg. 50G | ||
| x-ui-error-message: Value doesn't match the format eg. 50G. Minimum | ||
| is 50G and max is 1000G |
There was a problem hiding this comment.
Align disk_size validation with the stated 50–1000G bounds.
Current pattern ^[0-9]+G$ allows values like 1G, which contradicts the error message.
✅ Proposed regex update
- pattern: ^[0-9]+G$
+ pattern: ^((5[0-9]G)|([6-9][0-9]G)|([1-9][0-9][0-9]G)|(1000G))$📝 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.
| disk_size: | |
| title: Disk Size | |
| description: Size of the disk in GiB for nodes in this node pool | |
| type: string | |
| pattern: ^[0-9]+G$ | |
| x-ui-placeholder: eg. 50G | |
| x-ui-error-message: Value doesn't match the format eg. 50G. Minimum | |
| is 50G and max is 1000G | |
| disk_size: | |
| title: Disk Size | |
| description: Size of the disk in GiB for nodes in this node pool | |
| type: string | |
| pattern: ^((5[0-9]G)|([6-9][0-9]G)|([1-9][0-9][0-9]G)|(1000G))$ | |
| x-ui-placeholder: eg. 50G | |
| x-ui-error-message: Value doesn't match the format eg. 50G. Minimum | |
| is 50G and max is 1000G |
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/facets.yaml` around lines 98 - 105, The
disk_size schema currently uses pattern "^[0-9]+G$" which permits values outside
the documented 50–1000G range; update the pattern for the disk_size property in
facets.yaml (the disk_size field) to enforce numeric values between 50 and 1000
followed by "G" (for example use a regex that matches 50–99, 100–999, or 1000
such as one that captures [5-9]\d, [1-9]\d{2}, or 1000) so the validation aligns
with the x-ui-error-message.
| processed_taints = { | ||
| for idx, taint in local.taints : idx => { | ||
| key = taint.key | ||
| value = taint.value | ||
| effect = taint.effect | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure to understand the codebase
git ls-files | head -20Repository: Facets-cloud/facets-modules
Length of output: 774
🏁 Script executed:
# Find the file mentioned in the review
find . -name "locals.tf" -path "*/aks_node_fleet/0.1/*" 2>/dev/nullRepository: Facets-cloud/facets-modules
Length of output: 120
🏁 Script executed:
# Search for where processed_taints is used
rg "processed_taints" -A 3 -B 3Repository: Facets-cloud/facets-modules
Length of output: 1285
🏁 Script executed:
# Look for the downstream schema or usage patterns
rg "taints" -A 3 -B 3 | head -100Repository: Facets-cloud/facets-modules
Length of output: 8056
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/main.tf | head -80Repository: Facets-cloud/facets-modules
Length of output: 1942
🏁 Script executed:
# Get the complete facets.yaml to understand the schema expectations
cat modules/node_fleet/aks_node_fleet/0.1/facets.yamlRepository: Facets-cloud/facets-modules
Length of output: 4606
🏁 Script executed:
# Check the aks/0.1 module's variables.tf to see what it expects for taints
cat modules/node_fleet/aks_node_fleet/0.1/aks/0.1/variables.tfRepository: Facets-cloud/facets-modules
Length of output: 873
🏁 Script executed:
# Check how the taints are used in the aks/0.1 module
cat modules/node_fleet/aks_node_fleet/0.1/aks/0.1/main.tf | grep -A 5 -B 5 "taints"Repository: Facets-cloud/facets-modules
Length of output: 1132
🏁 Script executed:
# Also check what outputs expects (from the earlier grep)
cat modules/node_fleet/aks_node_fleet/0.1/outputs.tfRepository: Facets-cloud/facets-modules
Length of output: 280
🏁 Script executed:
# Let's see the full context of how variables are passed
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/variables.tf | grep -A 20 "taints"Repository: Facets-cloud/facets-modules
Length of output: 617
🏁 Script executed:
# Check if there are any type constraints for taints in the module
grep -r "taints" modules/node_fleet/aks_node_fleet/0.1/aks/0.1/ --include="*.tf" -B 2 -A 2Repository: Facets-cloud/facets-modules
Length of output: 2740
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/locals.tfRepository: Facets-cloud/facets-modules
Length of output: 708
🏁 Script executed:
# Search for all usages of processed_taints
rg "processed_taints" -B 5 -A 5Repository: Facets-cloud/facets-modules
Length of output: 1952
🏁 Script executed:
# Check the structure flow more carefully - see what outputs are expected
cat -n modules/node_fleet/aks_node_fleet/0.1/aks/0.1/output.tfRepository: Facets-cloud/facets-modules
Length of output: 1221
🏁 Script executed:
# Let's create a simple test to understand what gets passed
cat > /tmp/test_taint_flow.txt << 'EOF'
FLOW ANALYSIS:
1. Input in facets.yaml:
- taints: type: array of objects with key, value, effect
2. In locals.tf (line 10-16):
processed_taints = {
for idx, taint in local.taints : idx => { ... }
}
This creates a MAP with string keys "0", "1", "2", etc.
3. In main.tf (line 16):
taints = local.processed_taints
This passes a MAP to the aks module
4. In aks/0.1/variables.tf:
"taints" = []
Expects an ARRAY (default is empty array)
5. In aks/0.1/main.tf:
taints = [for taint in var.instance.spec.taints : "${taint.key}=${taint.value}:${taint.effect}"]
Iterates as if it's an array
6. In aks/0.1/output.tf:
taints = [for taint in var.instance.spec.taints : { ... }]
Also iterates as if it's an array
EOF
cat /tmp/test_taint_flow.txtRepository: Facets-cloud/facets-modules
Length of output: 815
🏁 Script executed:
# Check git history or comments for why processed_taints uses a map
git log --oneline -n 20 -- modules/node_fleet/aks_node_fleet/0.1/locals.tf 2>/dev/null || echo "No git history available"Repository: Facets-cloud/facets-modules
Length of output: 127
🏁 Script executed:
# Check if there are similar patterns in other modules
rg "for idx.*in.*:" modules/ --type tf -B 2 -A 2 | head -40Repository: Facets-cloud/facets-modules
Length of output: 432
Remove unnecessary map transformation for taints.
processed_taints creates a map with numeric keys, but the downstream aks module expects an array. The variable and schema both define taints as type: array. Either pass local.taints directly or convert to a proper array:
💡 Recommended fix
- processed_taints = {
- for idx, taint in local.taints : idx => {
- key = taint.key
- value = taint.value
- effect = taint.effect
- }
+ processed_taints = [
+ for taint in local.taints : {
+ key = taint.key
+ value = taint.value
+ effect = taint.effect
+ }
}Or simply use taints = local.taints in main.tf since it already matches the expected structure.
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/locals.tf` around lines 10 - 15,
processed_taints is building a numeric-keyed map from local.taints but the AKS
module expects an array; replace usages of processed_taints with the original
array (local.taints) or convert processed_taints into a proper list—i.e., stop
transforming into a map in locals.tf (remove the for idx, taint : ... map) and
set the module input taints = local.taints in main.tf (or create a list
comprehension that yields an array) so the input matches the variable/schema
type: array.
| min_node_count = lookup(each.value, "min_node_count", null) | ||
| node_labels = lookup(each.value, "node_labels", null) | ||
| instance_type = lookup(each.value, "instance_type", null) | ||
| disk_size = lookup(each.value, "disk_size", "G") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n modules/node_fleet/aks_node_fleet/0.1/main.tfRepository: Facets-cloud/facets-modules
Length of output: 1942
🏁 Script executed:
# Search for context around disk_size usage in the codebase
rg "disk_size" -B 5 -A 5Repository: Facets-cloud/facets-modules
Length of output: 29315
🌐 Web query:
Terraform azurerm kubernetes cluster node pool disk_size valid values schema
💡 Result:
In the AzureRM provider, the AKS node pool “disk size” field is os_disk_size_gb (for both azurerm_kubernetes_cluster_node_pool and azurerm_kubernetes_cluster.default_node_pool). It’s an integer (GB); 0 means “use the default OS disk size for the selected vm_size”. The underlying AKS/ARM API constraint is min 0, max 2048 (newer API versions) [1]. Older API versions capped this at 1023 [2].
Avoid invalid default disk_size = "G".
The value "G" violates the schema validation pattern (^[0-9]+G$) and will cause runtime failures. When this empty default is used, the downstream code applies trim(disk_size, "G") which produces an empty string, causing integer conversion errors. Use null for optional parameters or a valid size like "50G".
Suggested fix
- disk_size = lookup(each.value, "disk_size", "G")
+ disk_size = lookup(each.value, "disk_size", null)📝 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.
| disk_size = lookup(each.value, "disk_size", "G") | |
| disk_size = lookup(each.value, "disk_size", null) |
🤖 Prompt for AI Agents
In `@modules/node_fleet/aks_node_fleet/0.1/main.tf` at line 14, The default for
disk_size in the lookup call (disk_size = lookup(each.value, "disk_size", "G"))
is invalid and violates the pattern ^[0-9]+G$; change the default to null for an
optional parameter or a valid size string (e.g., "50G") so downstream
trim/digits conversion doesn't produce an empty string — update the lookup
default and any callers expecting a non-null string accordingly, and ensure any
logic using trim(disk_size, "G") guards against null/empty before integer
conversion.
1 participant
Description
This PR creates the module for aks node fleet which helps us to create multiple nodepools using one resource one.
Also have fixed the var.azs reference in kubernetes_node_pool/aks/0.1 using inputs var.inputs.network_details.attributes.legacy_outputs.vpc_details.azs.
Related issues
Type of change
Checklist
developbranchTesting
Reviewer instructions
Summary by CodeRabbit
Release Notes