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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs #26045

Merged
merged 2 commits into from Jun 14, 2023
Merged

daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs #26045

merged 2 commits into from Jun 14, 2023

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Jun 8, 2023 •
edited

  • Revert "ip: Add IPNetToPrefix() helper"
  • daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs

daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs

Following the previously reverted commit, replace the ingress IP
restoration to use netip types.

Towards #24246.

Reported-by: Tobias Klauser tobias@cilium.io
Signed-off-by: Chris Tarazi chris@isovalent.com

@christarazi christarazi requested review from a team as code owners June 8, 2023 16:39
@christarazi christarazi added kind/cleanup This includes no functional changes. area/daemon Impacts operation of the Cilium daemon. release-note/misc This PR makes changes that have no direct user impact. labels Jun 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 8, 2023
@christarazi christarazi mentioned this pull request Jun 8, 2023
Merged
@christarazi christarazi changed the title pr/christarazi/netipx cleanup daemon, maps/ipcache: Replace usage of net.IP* Jun 8, 2023
@christarazi christarazi changed the title daemon, maps/ipcache: Replace usage of net.IP* daemon, maps/ipcache: Replace usage of net.IP* Jun 8, 2023
@christarazi christarazi force-pushed the pr/christarazi/netipx-cleanup branch from f4922cb to 6ad1cac Compare June 8, 2023 16:41
@christarazi christarazi changed the title daemon, maps/ipcache: Replace usage of net.IP* daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs Jun 8, 2023
@christarazi
Copy link
Member Author

christarazi commented Jun 8, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (92.39% similarity)

@christarazi
Copy link
Member Author

cc @tklauser

tklauser
tklauser reviewed Jun 8, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, thanks for fixing this!

I'd suggest to reverse the commit order. Otherwise bisectability is broken on the first commit Revert "ip: Add IPNetToPrefix() helper" because ip.IPNetToPrefix is still used and only replaced in the second commit.

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Jun 8, 2023
Closed
mhofstetter
mhofstetter approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@christarazi christarazi force-pushed the pr/christarazi/netipx-cleanup branch from 6ad1cac to 540dd49 Compare June 9, 2023 22:39
@christarazi
Copy link
Member Author

christarazi commented Jun 9, 2023 •
edited

/test

Edit:

@christarazi
Copy link
Member Author

/test-1.26-net-next

@christarazi christarazi mentioned this pull request Jun 14, 2023
Closed
@christarazi christarazi force-pushed the pr/christarazi/netipx-cleanup branch from 540dd49 to 9ebb3cc Compare June 14, 2023 05:21
@christarazi
Copy link
Member Author

/test

@christarazi
daemon, maps/ipcache: Replace usage of net.IP* for ingress IPs
71ea357 
Following the previously reverted commit, replace the ingress IP
restoration to use netip types.

Towards cilium#24246.

Reported-by: Tobias Klauser <tobias@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Revert "ip: Add IPNetToPrefix() helper"
92e27a6 
This reverts commit f8094af.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/netipx-cleanup branch from 9ebb3cc to 92e27a6 Compare June 14, 2023 17:22
@christarazi
Copy link
Member Author

/test

michi-covalent
michi-covalent approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

reviewed tophat-owned files 🧹

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 14, 2023
@tklauser tklauser merged commit 2db7577 into cilium:main Jun 14, 2023
60 of 61 checks passed
@christarazi christarazi deleted the pr/christarazi/netipx-cleanup branch June 14, 2023 22:35
@joestringer
Copy link
Member

joestringer commented Oct 10, 2023 •
edited 

$ git grep IPNet pkg/ | wc -l
890

We still have a lot of *net.IPNet usage in the tree, I think that 2db7577 was premature to remove the helper.

EDIT: Ah it looks like there's a netipx.FromStdIPNet() equivalent we can use instead from netipx package: https://pkg.go.dev/go4.org/netipx?utm_source=godoc#FromStdIPNet

@tklauser
Copy link
Member

EDIT: Ah it looks like there's a netipx.FromStdIPNet() equivalent we can use instead from netipx package: https://pkg.go.dev/go4.org/netipx?utm_source=godoc#FromStdIPNet

Yep, see #25749 (comment) which I think triggered this PR.

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

@tklauser tklauser tklauser approved these changes

@aspsk aspsk aspsk approved these changes

@mhofstetter mhofstetter mhofstetter approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@christarazi @joestringer @tklauser @aspsk @mhofstetter @michi-covalent