-
Notifications
You must be signed in to change notification settings - Fork 347
pkg/net: add ip arithmetic functions #3815
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
Conversation
|
@takonomura thanks for raising this. We are extremely busy in the run up to KubeCon so please bear with us! |
|
Hi @myitcv, any updates on this PR? Let me know if I can do anything. Thanks! |
|
Hi @myitcv, just wanted to follow up on this PR again. If there are any concerns about the implementation or if you'd prefer a different approach, I'm happy to discuss alternatives. Otherwise, please let me know if there's anything I can do to help move this forward. |
|
Thanks for the reminder @takonomura - I have asked @rogpeppe for thoughts on the API here. |
rogpeppe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that having this functionality available is a good thing. I just wonder if we'd maybe be better off adding something a bit more general: for example, a way to convert IP / CIDR addresses to bits or a number, and vice versa, and then we'd gain more than just this specific functionality. That said, the functions proposed here would still be more convenient to use even if we had something more general, so in general I'm in favour of this change.
One thing: I think the Offset part of the name is probably redundant: how about AddIP and AddIPCIDR as names? Adding addresses themselves together doesn't really make sense, so I don't think there's any potential future ambiguity there.
| } | ||
|
|
||
| // AddIPOffset adds a numerical offset to a given IP address. | ||
| // The address can be provided as a string, byte array, or CIDR subnet notation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit odd that we can provide a CIDR string here. What's the use case for that?
For example, net.AddIPOffset("100.0.0.0/8", 1) returns "100.0.0.1/8" but that's not generally useful, as all the bits after the prefix are conventionally zero and ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case is generating network interface configurations, where IP addresses are written in CIDR notation with a prefix length. Since many configuration formats support this form, allowing AddIPOffset to take a CIDR string makes it easier to produce such values.
This change introduces two functions to adding numerical offsets to IP addresses or CIDR networks. Signed-off-by: takonomura <takonomura@users.noreply.github.com>
0040ed7 to
a140ef0
Compare
takonomura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogpeppe Thank you for the review. I've made the changes to address your comments.
I just wonder if we'd maybe be better off adding something a bit more general: for example, a way to convert IP / CIDR addresses to bits or a number, and vice versa, and then we'd gain more than just this specific functionality.
I agree that having a more general way to convert IP/CIDR addresses to numbers or bits could be broadly useful.
However, converting IP addresses to numbers can be problematic for IPv6, as IPv6 addresses are typically 39 digits long.
As discussed in #3787, this can cause the value to be interpreted as a float during arithmetic operations, which leads to unexpected behavior.
| } | ||
|
|
||
| // AddIPOffset adds a numerical offset to a given IP address. | ||
| // The address can be provided as a string, byte array, or CIDR subnet notation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case is generating network interface configurations, where IP addresses are written in CIDR notation with a prefix length. Since many configuration formats support this form, allowing AddIPOffset to take a CIDR string makes it easier to produce such values.
|
@rogpeppe just a friendly ping! This is ready for re-review when you have a moment. |
To address #3142, this PR introduces two functions for performing arithmetic operations on IP addresses and CIDR networks. These functions may be useful for use cases such as IP address management in CUE.
net.AddIPAddIPadds a numerical offset to a given IP address.This function is useful for automatically generating IP addresses for a large number of hosts, as demonstrated in the example below:
net.AddIPCIDRAddIPCIDRadds a numerical offset to a given CIDR subnet.This function is useful for generating network prefixes based on specific rules, as demonstrated in the example below: