Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPAddress.spec.gateway should be optional for IPv6 and maybe for IPv4 #8536

Closed
schrej opened this issue Apr 17, 2023 · 9 comments · Fixed by #8506
Closed

IPAddress.spec.gateway should be optional for IPv6 and maybe for IPv4 #8536

schrej opened this issue Apr 17, 2023 · 9 comments · Fixed by #8506
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@schrej
Copy link
Member

schrej commented Apr 17, 2023

Due to a sluggish implementation of the validation of IPAddress resources, the .spec.gateway field is currently required.

While this does make sense for IPv4, where you almost always provide a gateway when using DHCP (which the IPAM contract is essentially replacing), it does not for IPv6, which supports other methods like Router Advertising to configure a gateway.

We currently have two implementations, one making it completely optional, the other only for IPv6.
#8506 (entirely optional)
#8525 (ipv6 optional)

I'm in favor of making it optional for ipv6, but keep requiring it for ipv4.

For context: kubernetes-sigs/cluster-api-ipam-provider-in-cluster#70

cc @fabriziopandini @sbueringer

@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 17, 2023
@christianang
Copy link
Contributor

+1 for optional ipv6, but required for ipv4.

@vincepri
Copy link
Member

+1 for ipv6, while it's not unheard of networks without a gateway (from a technical point of view), it's extremely uncommon in Kubernetes environments

@sbueringer
Copy link
Member

Thx for writing up the issue. Also agree with only making it optional for ipv6

@christianang
Copy link
Contributor

Do we think at this point we'd be happy to merge #8525 then?

@schrej
Copy link
Member Author

schrej commented Apr 25, 2023

I ran this by our networking colleagues, and it turns out that we are actually using IPv4 without gateways at the moment (we don't even have network addresses, don't ask me how that works).
To be fair, our setup is probably quite unusual (each cluster node is acting as a router), so this might be a rare use case, and we could definitely work around gateways being required for ipv4.

Sorry for bringing this up so late. I think they told me before, but our networking is such a rabbit hole that I missed that while thinking about it in CAPI context.

I would therefore go with optional for both, ipv4 and ipv6, to allow maximum flexibility.
Since this isn't really an interaction point for users, I think it's a lot more meaningful to do proper validation on provider level, where users will actually see the error when applying configuration. On that level validation can then also be configurable if necessary.

The benefit of validating at this level is that providers can rely on the field to be present.

@sbueringer
Copy link
Member

While this does make sense for IPv4, where you almost always provide a gateway when using DHCP (which the IPAM contract is essentially replacing)

To be fair, you were writing "almost" always :). I guess we found one of those cases now where a gateway is not required. Sounds fine to me to make gateway optional for both.

@christianang
Copy link
Contributor

👍 making gateway optional for both is fine with me as well.

@sbueringer
Copy link
Member

Okay so let's go ahead with: #8506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants