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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 IPAM Webhook allows empty gateway for IPv6 Address #8525

Closed
wants to merge 1 commit into from
Closed

馃悰 IPAM Webhook allows empty gateway for IPv6 Address #8525

wants to merge 1 commit into from

Conversation

christianang
Copy link
Contributor

@christianang christianang commented Apr 13, 2023 •
edited

What this PR does / why we need it:

IPv6 default routes can be acquired through Router Advertisements, therefore empty gateway should be permitted. This PR continues to require gateway for IPv4. We also added validation to ensure the gateway family matches the address family.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 13, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @christianang. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@christianang christianang mentioned this pull request Apr 13, 2023
Closed
@christianang @tylerschultz
IPAM Webhook allows empty gateway for IPv6 Address
792e566 
Continue to require gateway for IPv4.

IPv6 default routes can be acquired through Router Advertisements,
therefore empty gateway should be permitted.

Signed-off-by: Christian Ang <angc@vmware.com>
Co-authored-by: Tyler Schultz <tschultz@vmware.com>
Co-authored-by: Christian Ang <angc@vmware.com>
@schrej schrej mentioned this pull request Apr 13, 2023
Merged
@christianang
Copy link
Contributor Author

christianang commented Apr 13, 2023 •
edited

Saw @schrej made #8506. Closing this PR.

Edit maybe this was premature. Reopening so we can discuss.

@christianang christianang reopened this Apr 13, 2023
@fabriziopandini
Copy link
Member

/hold

While we discuss which one of the solution to go on

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2023
@schrej schrej mentioned this pull request Apr 17, 2023
Closed
vincepri
vincepri reviewed Apr 18, 2023
View reviewed changes
exp/ipam/internal/webhooks/ipaddress.go

_, err = netip.ParseAddr(ip.Spec.Gateway)
if err != nil {
if addr.Is4() && ip.Spec.Gateway == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reading through this, are we not specifying a mode of operation? Wouldn't it be possible in this way to mix ipv4 and ipv6 settings under the same specs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't anything explicit that specifies the mode of operation on the IPAddress, but as part of this PR we did add validation to ensure address and gateway fields are not mixed, which I think would be sufficient to prevent mixing ipv4 and ipv6 on the same IPAddress.

@schrej
Copy link
Member

schrej commented Apr 26, 2023

/close
we've decided to go with #8506

@k8s-ci-robot
Copy link
Contributor

@schrej: Closed this PR.

In response to this:

/close
we've decided to go with #8506

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@christianang @k8s-ci-robot @fabriziopandini @schrej @vincepri