gce: Set node IP Alias range to match NodeCIDRMaskSize

[sl1pm4t]

Prior to this change, GCE Node IP Alias CIDR range was hardcoded to be a /24 which can be a limiting factor for scaling out a cluster when using smaller instance types, and could cause IP pool exhaustion despite many unallocated IPs.

This also means it's possible to bring up a cluster where the actual node CIDR size disagrees with what is configured in cluster.spec.kubeControllerManager.NodeCIDRMaskSize - although I do not know how detrimental that would be.

This change updates the IP Alias range to match the value configured in cluster.spec.kubeControllerManager.NodeCIDRMaskSize.

[k8s-ci-robot]

Hi @sl1pm4t. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[s3than]

Thanks for this @sl1pm4t

[hakman]

/cc @upodroid @justinsb

[hakman]

/ok-to-test

[hakman]

/assign @justinsb

[hakman]

/kind office-hours

[justinsb]

This also means it's possible to bring up a cluster where the actual node CIDR size disagrees with what is configured in cluster.spec.kubeControllerManager.NodeCIDRMaskSize - although I do not know how detrimental that would be.

It shouldn't matter, I think, because when we're using GCE networking (aka IP Alias), we bring up instances and GCE allocates the CIDR which becomes the node's pod cidr.

I think cluster.spec.kubeControllerManager.NodeCIDRMaskSize field is unused in GCE networking mode. We also normally suggest that the fields under kubeControllerManager are more for power-users than for things that we expect users to set directly, the reason being that these fields map to flags, and kubernetes doesn't have the same compatibility guarantees around flags - flags are often deprecated.

What we would normally suggest is creating a top level field. In this case it would be something like "NodeCIDRMaskSize" under GCPNetworkingSpec. Would that make more sense here? (Or any other field name that makes sense to you!)

If not, I'm aware this PR has been sitting for a long time (sorry!), so I think we could just merge as-is and then have spec.gce.nodeCIDRMaskSize take precedence if set when/if we add that field.

[sl1pm4t]

What we would normally suggest is creating a top level field. In this case it would be something like "NodeCIDRMaskSize" under GCPNetworkingSpec. Would that make more sense here? (Or any other field name that makes sense to you!)

That makes sense to me. I wasn't sure which was less intrusive, using an existing field or introducing a new one. I'm happy to add the new one.

If not, I'm aware this PR has been sitting for a long time (sorry!), so I think we could just merge as-is and then have spec.gce.nodeCIDRMaskSize take precedence if set when/if we add that field.

No problem. No rush, so long as this could at least make it into a 1.29 GA release. I'll submit the suggested change ASAP.

[justinsb]

We're coming up to the cutoff point for 1.29. I think we could merge this as-is, though if you're able to add another field that would probably be better in the long term. But the reason I think we could merge this as-is because the current field (in this PR) is currently unused anyway, so I think we could just slot in the new field with higher priority if/when we add it

[justinsb]

We discussed a bit in office hours, and just merging this as-is is probably the best approach; it doesn't block adding the "more correct" field and the current version should be harmless.

[upodroid]

+1

[hakman]

/test all

[justinsb]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment