Add network_config to node_pool
#984
Conversation
DrFaust92
requested review from
bharathkkb,
Jberlinsky and
a team
as code owners
|
Thanks for the PR! 🚀 |
|
@morgante added example + tested config. |
examples/node_pool/variables.tf
Outdated
| @@ -77,3 +77,11 @@ variable "cluster_autoscaling" { | |||
| } | |||
| description = "Cluster autoscaling configuration. See [more details](https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/projects.locations.clusters#clusterautoscaling)" | |||
| } | |||
|
|
|||
| variable "network_config" { | |||
There was a problem hiding this comment.
Don't make this a variable. Just hard code it. Examples can and should use hardcoding extensively.
There was a problem hiding this comment.
np, it just will make an assumption on users env. will change.
| dynamic "network_config" { | ||
| for_each = lookup(each.value, "network_config", false) ? [each.value] : [] | ||
| content { | ||
| create_pod_range = lookup(network_config.value, "create_pod_range", false) |
There was a problem hiding this comment.
I'm not sure we want to dynamically create the range on demand, since that's not very declarative.
There was a problem hiding this comment.
Cool, ill remove both create_pod_range and cidr vars.
There was a problem hiding this comment.
Hi fellows, why not giving the possibility to customize the CIDR if set? Looks like pod_ipv4_cidr_block can not be set so it is set to a default. How can we set this to some CIDR range of our choosing? Thanks!
Co-authored-by: Morgante Pell <morgante.pell@morgante.net>
|
Thanks for the quick review @morgante! addressed comments. |
autogen/main/README.md
Outdated
| @@ -199,6 +199,8 @@ The node_pools variable takes the following parameters: | |||
| {% endif %} | |||
| | min_count | Minimum number of nodes in the NodePool. Must be >=0 and <= max_count. Should be used when autoscaling is true | 1 | Optional | | |||
| | name | The name of the node pool | | Required | | |||
| | network_config | Configuration for Adding Pod IP address ranges to the node pool. | | Optional | | |||
There was a problem hiding this comment.
Since only pod_range is used now, could we collapse this into being the only variable? So network_config doesn't need to be supplied at all?
There was a problem hiding this comment.
future proofing? what if there will an additional var there?
There was a problem hiding this comment.
YAGNI - we shouldn't add complexity for hypothetical gains.
There was a problem hiding this comment.
Fair enough. changed.
There was a problem hiding this comment.
Please increase the minimum required provider version: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/versions.tf#L24
Also make sure that changes only apply to beta modules.
|
@morgante made changes and tested example again (needed some minor change to foreach logic) and bumped beta provider in template. |
…ls (terraform-google-modules#984) BREAKING CHANGE: Minimum beta provider version increased to v3.79.0.
Closes #983