feat(terraform): add the Azure vnet module - #14099
Conversation
Greptile SummaryThis PR introduces the first Azure Terraform module, provisioning a virtual network with configurable subnets, stable NAT-based egress, and optional Network Watcher flow logs.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the revised validation rejects both omitted and blank Network Watcher names when flow logs are enabled. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Inputs[Module inputs] --> VNet[Azure Virtual Network]
VNet --> Subnets[Purpose-keyed subnets]
Subnets --> AKS[AKS subnet]
Subnets --> PostgreSQL[Delegated PostgreSQL subnet]
Subnets --> PrivateEndpoints[Private endpoint subnet]
Subnets --> AppGateway[Application Gateway subnet]
AKS --> NAT[NAT gateway]
NAT --> PublicIP[Stable public IP]
VNet -. optional .-> FlowLogs[Network Watcher flow logs]
FlowLogs --> Storage[Flow-log storage account]
Reviews (3): Last reviewed commit: "feat(terraform): add the Azure vnet modu..." | Re-trigger Greptile |
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
- In
deployment/terraform/modules/azure/vnet/main.tf, custom subnet maps can create prohibited NAT associations for delegated orapp_gatewaysubnets, risking an invalid deployment; reject these combinations through input validation. - In
deployment/terraform/modules/azure/vnet/variables.tf, enabling flow logs validates onlyflow_log_storage_account_idwhilenetwork_watcher_nameis also required by the flow-log resource, allowing an invalid plan; require both values when flow logs are enabled.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deployment/terraform/modules/azure/vnet/variables.tf">
<violation number="1" location="deployment/terraform/modules/azure/vnet/variables.tf:119">
P2: The enable_flow_logs validation requires flow_log_storage_account_id but not network_watcher_name, even though the flow log resource in main.tf needs both. With default null network_watcher_name, plan succeeds and the module fails only at apply with an unclear azurerm error instead of failing loudly in validation. Since the flow log also needs a Network Watcher, add `var.network_watcher_name != null` to the validation condition so misconfiguration is caught at plan time.</violation>
</file>
<file name="deployment/terraform/modules/azure/vnet/main.tf">
<violation number="1" location="deployment/terraform/modules/azure/vnet/main.tf:5">
P2: When a custom subnet map opts a delegated subnet or `app_gateway` into NAT, this comprehension creates the prohibited subnet association. Reject those combinations with input validation instead of relying on callers to preserve the default `false` values.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
7665c83 to
c98a9e8
Compare
There was a problem hiding this comment.
4 issues found across 5 files
Confidence score: 2/5
deployment/terraform/modules/azure/vnet/main.tfandvariables.tfcan fail validation or planning with azurerm 4.x when flow logs are enabled because the flow-log resource uses an unsupported target argument; update the resource to use an NSG ID andnetwork_security_group_id, or constrain the supported provider version.deployment/terraform/modules/azure/vnet/variables.tfdefaults omittednat_gatewayvalues to true for custom subnets, which can attach NAT to every subnet and break configurations such as Application Gateway subnets; make NAT opt-in or require callers to set it explicitly.deployment/terraform/modules/azure/vnet/variables.tfallows empty or whitespace-onlynetwork_watcher_namevalues through flow-log validation, deferring failure to provider planning or Azure apply; reject blank values during input validation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deployment/terraform/modules/azure/vnet/main.tf">
<violation number="1" location="deployment/terraform/modules/azure/vnet/main.tf:90">
P2: When a caller's lock file selects azurerm 4.0.x, Terraform rejects this resource before planning because that provider schema has no `target_resource_id`. Raise the module's minimum azurerm version to the release that introduced this field.</violation>
</file>
<file name="deployment/terraform/modules/azure/vnet/variables.tf">
<violation number="1" location="deployment/terraform/modules/azure/vnet/variables.tf:34">
P1: When a caller supplies a custom `subnets` map, omitting `nat_gateway` defaults it to true and attaches NAT to every subnet. This makes the setting opt-out instead of opt-in and can cause an Application Gateway subnet deployment to fail; default it to false and explicitly opt in the AKS default.</violation>
<violation number="2" location="deployment/terraform/modules/azure/vnet/variables.tf:118">
P1: When `enable_flow_logs` is true, this module cannot validate or apply with azurerm 4.x because the flow-log resource uses an unsupported target argument. Wire an NSG ID and use `network_security_group_id`, or use a provider resource that supports VNet flow logs.</violation>
<violation number="3" location="deployment/terraform/modules/azure/vnet/variables.tf:129">
P2: Reject empty and whitespace-only `network_watcher_name` values when flow logs are enabled. Otherwise invalid input passes this validation and fails later during provider planning or Azure apply.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
First module of the Azure set, mirroring deployment/terraform/modules/aws/vpc. Azure differs from AWS in three ways that show up in the interface: - Subnets are named resources and delegation is a property of the subnet, so the public/private CIDR lists become a map keyed by purpose. PostgreSQL Flexible Server needs its own delegated subnet, and Application Gateway needs a dedicated one. - The Microsoft.Storage service endpoint on the AKS subnet plays the role the S3 gateway endpoint plays on AWS: it is what lets the storage account restrict access to the cluster. - Flow logs are opt-in rather than on by default. Azure writes them to a storage account and needs a Network Watcher in the region, so enabling them by default would either create a storage account the caller did not ask for or fail on subscriptions without a Network Watcher. Tests plan the module against a mocked provider, so they need no Azure subscription. Run 'terraform test' from the module directory.
c98a9e8 to
0c74f24
Compare
Description
First module of the Azure set, mirroring
deployment/terraform/modules/aws/vpc. Creates a virtual network, its subnets, a NAT gateway for stable egress, and optional flow logs.Azure differs from AWS in three ways that show up in the interface:
Microsoft.Storageservice endpoint on the AKS subnet plays the role the S3 gateway endpoint plays on AWS: it is what lets the storage account restrict access to the cluster.onyxcomposition at the top of this stack wires them up.One bug worth calling out, caught while writing the tests: the AWS modules retain logs for 400 days, but Azure caps flow log retention at 365. Carrying the AWS default across would have failed at apply.
How Has This Been Tested?
terraform testagainst a mockedazurermprovider, so the suite needs no Azure subscription and no credentials:The suite covers the default subnet layout, the postgres delegation, which subnets attach to the NAT gateway, and every input validation. I mutation-checked it: making all subnets attach to the NAT gateway fails
nat_gateway_attaches_only_to_opted_in_subnetsand nothing else.terraform validateand the repo'sods fmt tf/ods lint tf/terraform_validatehooks all pass.Not applied against a live subscription.
Additional Options
Changes from review (greptile, cubic)
enable_flow_logs.subnetsvariable.Tests: 8 → 10.
One suggestion not taken, and why
cubic asked for a validation rejecting a NAT gateway on a delegated or Application Gateway subnet. I documented it instead. Azure does support a NAT gateway on a subnet delegated to Flexible Server; it is Application Gateway subnets that cannot have one, and this module neither creates the gateway nor knows which subnet will hold it — the map keys are arbitrary. A blanket validation would reject legal configurations to catch a case the module cannot actually identify. The defaults already set
nat_gateway = falseon both.Round 2
network_watcher_nameno longer passes. The null check accepted" ", which then failed in the provider.nat_gatewayis now opt-in (defaultfalse). This is the better answer to the round-1 comment I pushed back on: rather than validating against a case the module cannot detect, a caller writing their own subnet map now has to ask for egress instead of remembering to refuse it. The default map opts the AKS subnet in explicitly.>= 4.11.0, < 5.0.Tests: 10 → 12.
One P1 not taken: it is factually wrong
cubic says this module "cannot validate or apply with azurerm 4.x because the flow-log resource uses an unsupported target argument."
It validates. I measured it:
target_resource_idis absent in 4.10.0 and present from 4.11.0, and our lock resolves 4.81.0, whereterraform validateand all 12 tests pass. cubic's suggested alternative — wiring an NSG ID and usingnetwork_security_group_id— would move us onto the deprecated argument and give NSG flow logs instead of VNet flow logs.Its companion P2 was right, though, and is fixed above:
~> 4.0allowed 4.0.x, where the field genuinely does not exist. That is the real bug, and the version bisect is what found the exact floor.