Gateway should be optional

[tylerschultz]

As it is written now, CAPV assumes that a Gateway will be present on IPAddress objects, and will fail to reconcile a VSphereVM if it's not present.
The webhook should validate the presence of the Gateway, and reject pools that do not have a valid gateway. Gateways should need to be in range of the provided subnet.
Alternatively, the gateway could be defaulted to the first IP in the subnet range.

[schrej]

I gave this some more thought. If you want to rely on a Gateway being set, this would need to be specified on IPAM contract level, not for individual providers.

The fun part starts once you consider IPv6. (Disclaimer: I'm not too deep into IPv6, so feel free to validate)
Afaik there is a lot of new ways to configure IP addresses. There are better alternatives to DHCP, which makes the IPAM contract unnecessary in some scenarios. But there are also options to configure individual aspects of the network using "Router Advertising", which e.g. allows setting a static IPv6 on the host, but not providing a default gateway, which will be configured automatically using Router Advertising.
Considering that, I'd keep this field optional. Alternatively we could only make it required when using IPv4. I'll ask my networking colleagues about that as well, maybe there is a reason not to have it for IPv4 either.

Of course nobody thought of us poor software engineers who only want to understand the manageable complexity of IPv4 when coming up with those ideas. 😄

I'm not sure what a solution for different requirements of providers would be. Of course it should be documented and be properly reflected in resource status. We could also introduce a toggle that makes it required, which would help folks with a less-advanced networking setup.

[tylerschultz]

Thanks for bringing up these points. We agree that there is valid reason to keep it optional.

We do see that right now, the code will fail reconciliation if an invalid gateway is provided:
https://github.com/telekom/cluster-api-ipam-provider-in-cluster/blob/main/internal/poolutil/pool.go#L59

Perhaps for the time being, we permit an empty gateway, allow successful reconciliation, and defer making any decision about validation?

[christianang]

I noticed that the capi-webhook validates gateway and it fails if gateway is empty. Probably have to update the capi-webhook to allow empty gateway to truly allow IPAddresses to get reconciled with empty gateways: https://github.com/kubernetes-sigs/cluster-api/blob/4f60841913fa99912771c3906ee81dd13d4e9633/exp/ipam/internal/webhooks/ipaddress.go#L126-L134

[schrej]

You are right. Since it has an omitempty json tag, I think we should change it to check if it's even set.

[christianang]

Created a PR to update capi-webhook: kubernetes-sigs/cluster-api#8525.

[schrej]

I had already created kubernetes-sigs/cluster-api#8506, not sure if it makes sense to differentiate between ipv4 and ipv6.

[christianang]

Ah, I should of looked. We can close our PR in favor of yours. I can go either way in terms of allowing or disallowing gateway for ipv4.

[tylerschultz]

I have no strong opinions, and am open to any implementation.

If I were all alone making this choice, I'd require gateway on IPv4 Pools/Addresses, and I'd make gateways on IPv6 Pools/Addresses always optional, per our PR.

My rationale for making IPv4 required: The user would need to configure the Gateway on the cluster yaml if they don't provide it on the Pool. This separation of configuration seems a bit disjointed. If neither the pool nor the cluster yaml specified the Gateway, it becomes hard to detect the misconfiguration, and the user would likely not find out until the deploy failed.

[tylerschultz]

I think this issue has been solved by #118. Please feel free to reopen if that's not correct.