Gateway should be optional about cluster-api-ipam-provider-in-cluster HOT 9 CLOSED

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.

from cluster-api-ipam-provider-in-cluster.

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?

from cluster-api-ipam-provider-in-cluster.

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

from cluster-api-ipam-provider-in-cluster.

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

from cluster-api-ipam-provider-in-cluster.

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

from cluster-api-ipam-provider-in-cluster.

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

from cluster-api-ipam-provider-in-cluster.

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.

from cluster-api-ipam-provider-in-cluster.

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.

from cluster-api-ipam-provider-in-cluster.

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

from cluster-api-ipam-provider-in-cluster.