Support multiple VLAN networks per tenant space#68
Conversation
- vlan_id is now list(number); each entry creates a harvester_network via for_each so networks can be added/removed independently - Auto-route path (no vyos_endpoint) supports multiple VLANs; VyOS path still requires exactly one VLAN (precondition enforced) - network_namespace_name override lets callers import brownfield namespaces whose names differ from the <project>-net default - vlan_network_names map override lets callers import brownfield harvester_network resources with non-default names - network_name output replaced by network_names map keyed by VLAN ID Closes wso2#67 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe tenant-space module now supports multiple VLANs per tenant by converting Changes
Sequence DiagramsequenceDiagram
participant Input as vlan_id Input
participant Validate as Validation
participant Decision as Control Flow
participant NS as Network Namespace
participant Networks as Harvester Networks
participant VyOS as VyOS Module
Input->>Validate: Pass vlan_id (list or null)
Validate->>Validate: Check non-empty list, VLAN range 1–4094
Validate-->>Decision: Validation result
Decision->>Decision: Determine use_vyos<br/>(single VLAN + auto-route disabled)
alt use_vyos = true
Decision->>Decision: Enforce precondition:<br/>length(vlan_id) == 1
Decision-->>NS: ✓ Proceed
else use_vyos = false
Decision->>Decision: Multi-VLAN or auto-route enabled
Decision-->>NS: vyos_endpoint = null
end
NS->>NS: Create network_namespace<br/>(with optional name override)
NS->>Networks: Create harvester_network per VLAN
loop For each VLAN ID
Networks->>Networks: Set vlan_id from array element
alt local.use_vyos = true
Networks->>Networks: Configure route_cidr/route_gateway
else
Networks->>Networks: Set route_cidr/route_gateway = null<br/>(auto-route mode)
end
end
alt local.use_vyos = true
Networks->>VyOS: Invoke vyos_tenant module<br/>with vlan_id[0]
else
Networks-->>VyOS: (skipped)
end
Networks->>Networks: Export network_names map<br/>(VLAN ID → namespace/name)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (3)
modules/management/tenant-space/main.tf (1)
58-61: Precondition looks correct; consider echoing the VLAN count in the error for faster debugging.Logic is sound:
!local.use_vyos || length(var.vlan_id) == 1cleanly rejects the VyOS + multi-VLAN combination. Minor ergonomic suggestion only — interpolatinglength(var.vlan_id)in the error message helps operators identify the offending configuration quickly.Optional message tweak
- error_message = "VyOS path requires exactly one VLAN ID. Set vyos_endpoint = null for multi-VLAN auto-route configurations." + error_message = "VyOS path requires exactly one VLAN ID (got ${var.vlan_id == null ? 0 : length(var.vlan_id)}). Set vyos_endpoint = null for multi-VLAN auto-route configurations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/management/tenant-space/main.tf` around lines 58 - 61, Update the precondition error message to include the actual VLAN count for easier debugging: in the precondition block that checks "local.use_vyos" and "length(var.vlan_id) == 1", change the error_message to interpolate length(var.vlan_id) (e.g., include the value of length(var.vlan_id) in the string) so the log shows the offending VLAN count when the condition fails.modules/management/tenant-space/variables.tf (2)
112-123: Consider rejecting duplicate VLAN IDs in validation.
harvester_network.tenantusestoset([for id in var.vlan_id : tostring(id)])inmain.tf, which silently dedupes. If a caller accidentally passes[608, 608], Terraform will plan a single resource with no warning. A cheap validation here surfaces the mistake at plan time.Proposed validation tweak
validation { condition = var.vlan_id == null || ( length(var.vlan_id) > 0 && + length(var.vlan_id) == length(toset(var.vlan_id)) && alltrue([for id in var.vlan_id : id >= 1 && id <= 4094]) ) - error_message = "vlan_id must be null or a non-empty list of valid 802.1Q VLAN IDs (1–4094)." + error_message = "vlan_id must be null or a non-empty list of unique 802.1Q VLAN IDs (1–4094)." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/management/tenant-space/variables.tf` around lines 112 - 123, The vlan_id variable validation should also reject duplicate IDs so callers get a plan-time error instead of silent dedupe by harvester_network.tenant; update the validation block for variable "vlan_id" to require that either var.vlan_id is null or (length(var.vlan_id) > 0, all entries are in 1..4094, and length(var.vlan_id) == length(distinct(var.vlan_id)) to ensure uniqueness), and adjust the error_message to mention duplicates; this prevents inputs like [608,608] from being silently collapsed by the toset/tostring logic in main.tf's harvester_network.tenant.
125-135: Add name-format validation to the new override variables.Both
network_namespace_nameand values ofvlan_network_namesbecome Kubernetes/Harvester resource names. Without validation, malformed values fail deep inside the provider with opaque errors. Thenamespacesvariable already includes such validation; the same RFC 1123 DNS label constraints should apply here.Validate
network_namespace_nameto allow null or a string matching the RFC 1123 pattern (lowercase alphanumeric and hyphens, 1–63 characters). Validate all values invlan_network_namesmap against the same constraints.As a forward-looking enhancement (blocked until Terraform 1.9 or later in this repo), consider asserting that every key in
vlan_network_namescorresponds to an entry invar.vlan_idvia apreconditionblock on a resource. This prevents stale or misspelled overrides from silently no-op-ing vialookup(..., default).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/management/tenant-space/variables.tf` around lines 125 - 135, Add RFC1123 name-format validation to the new override variables: for variable "network_namespace_name" add a validation block that allows null or a string matching the RFC1123 DNS label regex (lowercase alphanumeric and hyphen, 1–63 chars), and for variable "vlan_network_names" add a validation that iterates over all map values and ensures each value matches the same RFC1123 regex; reference the variable blocks named network_namespace_name and vlan_network_names and use Terraform's validation.rule with regex(match) and a clear error_message when the constraint fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/management/tenant-space/main.tf`:
- Line 10: The current expression using coalesce for network_namespace
(network_namespace = local.create_net_ns ? coalesce(var.network_namespace_name,
"${var.project_name}-net") : null) will accept an empty string as a valid value;
update the logic to guard against empty strings by either adding RFC 1123 +
non-empty validation on var.network_namespace_name in variables.tf or change the
fallback to an explicit empty-string check (e.g., use var.network_namespace_name
!= "" ? var.network_namespace_name : "${var.project_name}-net") so that an empty
string does not become the namespace, keeping local.create_net_ns and
var.project_name references intact.
---
Nitpick comments:
In `@modules/management/tenant-space/main.tf`:
- Around line 58-61: Update the precondition error message to include the actual
VLAN count for easier debugging: in the precondition block that checks
"local.use_vyos" and "length(var.vlan_id) == 1", change the error_message to
interpolate length(var.vlan_id) (e.g., include the value of length(var.vlan_id)
in the string) so the log shows the offending VLAN count when the condition
fails.
In `@modules/management/tenant-space/variables.tf`:
- Around line 112-123: The vlan_id variable validation should also reject
duplicate IDs so callers get a plan-time error instead of silent dedupe by
harvester_network.tenant; update the validation block for variable "vlan_id" to
require that either var.vlan_id is null or (length(var.vlan_id) > 0, all entries
are in 1..4094, and length(var.vlan_id) == length(distinct(var.vlan_id)) to
ensure uniqueness), and adjust the error_message to mention duplicates; this
prevents inputs like [608,608] from being silently collapsed by the
toset/tostring logic in main.tf's harvester_network.tenant.
- Around line 125-135: Add RFC1123 name-format validation to the new override
variables: for variable "network_namespace_name" add a validation block that
allows null or a string matching the RFC1123 DNS label regex (lowercase
alphanumeric and hyphen, 1–63 chars), and for variable "vlan_network_names" add
a validation that iterates over all map values and ensures each value matches
the same RFC1123 regex; reference the variable blocks named
network_namespace_name and vlan_network_names and use Terraform's
validation.rule with regex(match) and a clear error_message when the constraint
fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10f22417-872b-4bd7-88f2-61a851335b81
📒 Files selected for processing (3)
modules/management/tenant-space/main.tfmodules/management/tenant-space/outputs.tfmodules/management/tenant-space/variables.tf
Guard against an empty string being passed as the network namespace override — coalesce accepts "" as truthy so validation is the right fix, consistent with other string variables in this module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
vlan_idchanged fromnumbertolist(number)— each entry provisions a separateharvester_networkviafor_each, so networks can be added or removed independently without replacing othersvyos_endpoint) now supports multiple VLANs; VyOS path still requires exactly one VLAN (enforced by aprecondition)network_namespace_nameoverride allows brownfield callers to import an existing namespace whose name differs from the<project>-netdefaultvlan_network_namesmap override allows brownfield callers to import existingharvester_networkresources with non-default namesnetwork_nameoutput replaced bynetwork_namesmap keyed by VLAN ID stringTest plan
vlan_id = [1000]andvyos_endpointset — confirms VyOS path still works, single VLAN, manual routevlan_id = [608, 609]and novyos_endpoint— confirms multi-VLAN auto-route, twoharvester_networkresources createdvlan_id = [1000, 1001]andvyos_endpointset — VyOS precondition fires with clear errornetwork_namespace_nameandvlan_network_namesoverrides — no diff after importvlan_id = null— no network resources created,network_namesoutput is empty mapCloses #67
🤖 Generated with Claude Code