Cloud NAT module variable required but should be optional

[ghacrow]

Describe the bug
In the net-cloudnat module, the variable subnetworks has a required secondary_ranges input. However, this is an optional input (see terraform docs here) as is only required should you use LIST_OF_SECONDARY_IP_RANGES in the config_source_ranges input of the subnetworks variable.

E.g., if I want to use ALL_IP_RANGES in config_source_ranges, I would not have anything to enter in the secondary_ranges, but this stops me from using the module.

Environment

• Terraform v1.5.5
• v5.16.0 of the Google Terraform Provider

To Reproduce
Try using the cloud NAT module with the above config, e.g.,

subnetworks = [
  {
    self_link = "subnet-name"
    config_source_ranges = ["ALL_IP_RANGES"]
  }
]

Expected behavior
Be able to create a Cloud NAT and Router

Result

Error: Invalid value for input variable

The given value is not suitable for var.subnetworks declared at variables.tf:30,1-23: element 0: attribute "secondary_ranges" is required.

Additional context

I think the solution is just to add optional() around the code e.g.,

variable "subnetworks" {
  description = "Subnetworks to NAT, only used when config_source_subnets equals LIST_OF_SUBNETWORKS."
  type = list(object({
    self_link            = string,
    config_source_ranges = list(string)
    secondary_ranges     = optional(list(string))
  }))
  default = []
}

However, you may wish to make this check a little more complex, having it required if the config_source_ranges is a value such that the secondary_ranges are required

[ghacrow]

One thing to note, is that you could just have secondary_ranges = [] and a blank list. This may be desired and is a work around, however it does differ to the config that you would use just using the terraform resource where there is no need to define this at all. Happy to discuss

[sruffilli]

@ghacrow thanks for the detailed feedback, and for coming up with a potential solution!

However, you may wish to make this check a little more complex [...]

Good point, but I wouldn't add validation that the provider itself is already doing

you could just have secondary_ranges = [] and a blank list.

...or set is to null, which is requivalent to not setting an optional field.

Would you mind sending a PR with your proposed solution?
Thanks!

[ghacrow]

@sruffilli Hey - yeah of course. I will try and find some time later this week / next week to get this together alongside documentation updates.

Good point about using null also, but I think having optional() there makes the most sense so it closely aligns to the provider.