Cloud NAT module variable required but should be optional
Closed this issue · 3 comments
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
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
@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!
@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.