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

ACP: Extended logic for IP networks #235

Closed
clarfonthey opened this issue Jun 5, 2023 · 12 comments
Closed

ACP: Extended logic for IP networks #235

clarfonthey opened this issue Jun 5, 2023 · 12 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@clarfonthey
Copy link

clarfonthey commented Jun 5, 2023

Proposal

Problem statement

While most networked applications only need to deal with individual IP addresses, it is sometimes desirable to work with entire ranges of IP addresses, especially under IPv6.

Motivating examples or use cases

The primary use case for most workflows will be filtering (allow-listing or deny-listing) ranges of IPs for connections, although more dedicated networking code that routes traffic among several hosts could also benefit from this.

Additionally, configuring IP addresses for certain applications can benefit from logic surrounding IP networks. For example, under IPv6, it's very common for a host to have access to an entire 64-bit address space and to have free reign over which specific addresses they actually listen on. In these cases, ad-hoc connections can be made on a randomized IP address within this space, where a randomly chosen IP is then checked for collision with an existing address rather than iterating over all possible addresses.

Solution sketch

A good minimal proposal is the following:

  • BitAnd and BitOr impls for Ipv4Addr and Ipv6Addr (not for IpAddr, since the operands must be the same address type)
  • Not for Ipv4Addr, Ipv6Addr, and maybe IpAddr (we can implement this for generic IP addresses, but I'm not sure if it'd be useful)
  • leading_zeros, leading_ones, trailing_zeros, and trailing_ones methods for Ipv4Addr, Ipv6Addr, and potentially IpAddr (there isn't the same restriction as for binary operations, although it's likely you'll still need to narrow down the address type anyway)
  • BITS associated constant for Ipv4Addr and Ipv6Addr set to 32 and 128 respectively
  • Step for Ipv4Addr and Ipv6Addr (again not for IpAddr, since ranges of IPs must have the same type on both ends)

Note that this proposal does not currently include separate IP network types, although that could be a future extension. The main benefit here is to provide operations which can only be provided by the standard library since it owns the IP address types, while leaving the usage of those operations to implement dedicated types up to the end user and various crate authors.

Examples

Checking a network mask with BitAnd:

let addr = Ipv6Addr::new(0x2001, 0xdb8, 0x692e, 0x6e2e, 0x672e, 0x672e, 0x792e, 0x752e);
let mask = Ipv6Addr::new(0xffff, 0xffff, 0, 0, 0, 0, 0, 0);
let net = Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 0);
assert_eq!(addr & mask, net);

Creating an address in a network with BitOr:

let rand_token = Ipv6Addr::new(0, 0, 0, 0, 0, 4);
let net = Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 0);
let addr = Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 4);
assert_eq!(net | rand, addr);

Checking validity of a network mask:

let valid = Ipv4Addr::new(255, 255, 255, 192)
let invalid = Ipv4Addr::new(192, 255, 255, 255);
assert_eq!(valid.leading_ones() + valid.trailing_zeros(), Ipv4Addr::BITS);
assert_ne!(invalid.leading_ones() + invalid.trailing_zeros(), Ipv6Addr::BITS);

Getting the first and last IPs in a network:

let net = Ipv4Addr::new(192, 168, 0, 0);
let mask = Ipv4Addr::new(255, 255, 255, 0);
let mut range = net..=(net | !mask);
let first = range.skip(1).next().unwrap();
let last = range.next_back().unwrap();
assert_eq!(first, Ipv4Addr::new(192, 168, 0, 1));
assert_eq!(last, Ipv4Addr::new(192, 168, 0, 255));

Alternatives

All of these operations could be done through the conversions to u32 and u128, and this is indeed what existing crates do. The primary alternative would be to have users rely on these conversions for any of the proposed operations that are not implemented.

Links and related work

Some existing IP network crates:

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@clarfonthey clarfonthey added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 5, 2023
@BurntSushi
Copy link
Member

Seconded.

I am skeptical of the "implement bit arithmetic traits" path here though, and would like to see alternatives such as "semantically meaningful named methods" instead.

@programmerjake
Copy link
Member

imho we'd probably also want a create-mask-from-bit-count method

@pitaj
Copy link

pitaj commented Jun 5, 2023

Agreed, and I'd add some overflow checking as well

  • BitAnd becomes fn mask(self, mask: Self) -> Self
  • BitOr becomes fn add(self, offset: Self) -> Option<Self>
  • Not becomes fn invert(self) -> Self

@clarfonthey
Copy link
Author

clarfonthey commented Jun 5, 2023

I should add, part of the the thought process for implementing bit operations directly instead of putting them under named methods is that:

  1. They're going to be part of implementing other semantically meaningful methods and are already possible via direct conversion, so, it makes sense to just expose them directly.
  2. There really isn't a solid semantic difference to me between !addr and addr.invert(), and the latter feels more ambiguous in terms of what it's doing. Additionally, it solves the semantic ambiguity between "adding" two IPs together meaning addition or bit-or.

That said, the "create mask from bit count" would be a good operation to include here since it doesn't have as direct of a connection to any existing operation, since it involves bit-shifting. I personally would not think that implementing bit-shift operations for IPs is meaningful, but a method that converts a prefix length to a mask would be good in addition to something like leading_ones which would effectively convert a mask to a prefix length.

Also, while it is awkward to use, implementing Step seems to me like the best way to expose "addition" to addresses, since you can effectively perform all kinds of addition using operations like range.step_by. While iterating over a range of IPs doesn't have as many use cases, this would also provide that as well.

@BurntSushi
Copy link
Member

I don't think we are supposed to be debating the API so much here right? That's why I seconded it. I added my other comment to clarify that I was not fully on board with the API as proposed, but that idea and problem space may be worth addressing some way.

I think the next step is to file a tracking issue and then we can start digging into the API design arguments. :)

(My understanding of the ACP process could be wrong.)

@clarfonthey
Copy link
Author

As I understand it, the ACP process is for both nailing down the problem & the solution, although only the problem has to be included in the initial proposal, since the solution is part of the discussion. I could also be wrong, though.

@bluebear94
Copy link

It might be worth adding cidr to the “Links and related work” section.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 15, 2023

We briefly discussed this in a libs-api meeting. We thought the leading_, trailing_* methods could be confusing, since it's not clear it's counting bits. (Does 2.0.0.0 have 3 trailing zeros or 25?) Having an explicit to_bits() -> u32 method might be better: in ip.to_bits().leading_zeros() it's clear we're talking about leading zero bits. And having explicit to_bits and from_bits methods is probably useful in more situations.

No objections to BitAnd, BitOr, Not, Step and BITS. (Note that the trait implementations can unfortunately not be added as unstable, so those will directly need an FCP.)

@clarfonthey
Copy link
Author

Although I linked this there, figure I might as well make a comment including the PRs I created now that the ACP is merged:

@WiSaGaN
Copy link

WiSaGaN commented Jul 18, 2023

Is the BitAnd meaningful when the second address is not a mask? Same also for other traits like "Not". What exactly does it mean to "Not" an ordinary IP address? Shouldn't we implement a type "Mask" and only allow operations on those?

@clarfonthey
Copy link
Author

I mean, what does it mean to "not" an integer that just represents the count of something? These types have multiple meanings and it's not super worth it to separate them out for these purposes since it just makes the type logic weird. For example, we'd want to have a FromStr implementation for Mask which is identical to IpvNAddr, which just feels weird.

@WiSaGaN
Copy link

WiSaGaN commented Jul 19, 2023

I wouldn't want a FromStr directly for Mask. Instead I want to parse an ip address and then validate whether it is a valid mask by using "TryFromor just a method fromMask`. Mask instead can receive an integer from 0 to 32 as constructor.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 22, 2023
Add BITS, from_bits, to_bits to IP addresses

ACP: rust-lang/libs-team#235
Tracking issue: rust-lang#113744
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jul 23, 2023
Add BITS, from_bits, to_bits to IP addresses

ACP: rust-lang/libs-team#235
Tracking issue: #113744
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 16, 2023
impl Step for IP addresses

ACP: rust-lang/libs-team#235

Note: since this is insta-stable, it requires an FCP.

Separating out from the bit operations PR since it feels logically disjoint, and so their FCPs can be separate.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 17, 2023
impl Step for IP addresses

ACP: rust-lang/libs-team#235

Note: since this is insta-stable, it requires an FCP.

Separating out from the bit operations PR since it feels logically disjoint, and so their FCPs can be separate.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 15, 2023
impl Not, Bit{And,Or}{,Assign} for IP addresses

ACP: rust-lang/libs-team#235

Note: since these are insta-stable, these require an FCP.

Implements, where `N` is either `4` or `6`:

```rust
impl Not for IpvNAddr
impl Not for &IpvNAddr

impl BitAnd<IpvNAddr> for IpvNAddr
impl BitAnd<&IpvNAddr> for IpvNAddr
impl BitAnd<IpvNAddr> for &IpvNAddr
impl BitAnd<&IpvNAddr> for &IpvNAddr

impl BitAndAssign<IpvNAddr> for IpvNAddr
impl BitAndAssign<&IpvNAddr> for IpvNAddr

impl BitOr<IpvNAddr> for IpvNAddr
impl BitOr<&IpvNAddr> for IpvNAddr
impl BitOr<IpvNAddr> for &IpvNAddr
impl BitOr<&IpvNAddr> for &IpvNAddr

impl BitOrAssign<IpvNAddr> for IpvNAddr
impl BitOrAssign<&IpvNAddr> for IpvNAddr
```
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 17, 2023
impl Not, Bit{And,Or}{,Assign} for IP addresses

ACP: rust-lang/libs-team#235

Note: since these are insta-stable, these require an FCP.

Implements, where `N` is either `4` or `6`:

```rust
impl Not for IpvNAddr
impl Not for &IpvNAddr

impl BitAnd<IpvNAddr> for IpvNAddr
impl BitAnd<&IpvNAddr> for IpvNAddr
impl BitAnd<IpvNAddr> for &IpvNAddr
impl BitAnd<&IpvNAddr> for &IpvNAddr

impl BitAndAssign<IpvNAddr> for IpvNAddr
impl BitAndAssign<&IpvNAddr> for IpvNAddr

impl BitOr<IpvNAddr> for IpvNAddr
impl BitOr<&IpvNAddr> for IpvNAddr
impl BitOr<IpvNAddr> for &IpvNAddr
impl BitOr<&IpvNAddr> for &IpvNAddr

impl BitOrAssign<IpvNAddr> for IpvNAddr
impl BitOrAssign<&IpvNAddr> for IpvNAddr
```
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Oct 17, 2023
Add BITS, from_bits, to_bits to IP addresses

ACP: rust-lang/libs-team#235
Tracking issue: #113744
@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 23, 2023
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Add BITS, from_bits, to_bits to IP addresses

ACP: rust-lang/libs-team#235
Tracking issue: #113744
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add BITS, from_bits, to_bits to IP addresses

ACP: rust-lang/libs-team#235
Tracking issue: #113744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

8 participants