-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
std::net: Ipv4Addr and Ipv6Addr improvements #60145
Conversation
- quote the RFC - add a link to the RFC - fix markdown
RFC 4291 is a little unclear about what is a unicast link local address. According to section 2.4, the entire fe80::/10 range is reserved for these addresses, but section 2.5.3 defines a stricter format for such addresses. After a discussion[0] is has been decided to add a different method for each definition, so this commit: - renames is_unicast_link_local() into is_unicast_link_local_strict() - relaxed the check in is_unicast_link_local() [0]: rust-lang#27709 (comment)
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
This was my first time writing a macro, so things may be very sub-optimal on that front. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
8cff09b
to
793c653
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I don't think In that sense |
As per @therealbstern's comment[0]: The implementation of Ipv4::is_global is not complete, according to the IANA IPv4 Special-Purpose Address Registry. - It compares the address to 0.0.0.0, but anything in 0.0.0.0/8 should not be considered global. - 0/8 is not global and is currently forbidden because some systems used to treat it as the local network. - The implementation of Ipv4::is_unspecified is correct. 0.0.0.0 is the unspecified address. - It does not examine 100.64.0.0/10, which is "Shared Address Space" and not global. - Ditto 192.0.0.0/24 (IETF Protocol Assignments), except for 192.0.0.9/32 and 192.0.0.10/32, which are carved out as globally reachable. - 198.18.0.0/15 is for "Benchmarking" and should not be globally reachable. - 240.0.0.0/4 is reserved and not currently reachable
Ipv4addr::is_global() previously considered 0/8 was global, but has now been fixed, so these tests needed to be fixed as well.
Ipv6Addr::is_unicast_global() now returns `true` for unicast site local addresses, since they are deprecated.
/// } | ||
/// ``` | ||
pub fn is_shared(&self) -> bool { | ||
self.octets()[0] == 100 && (self.octets()[1] & 0b1100_0000 == 0b0100_0000) |
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.
maybe for readability an is_in_netmask
function would help?
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'm sorry I don't understand what you are suggesting here.
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.
A private function is_in_cidr(&self, baseAddress: Ipv4Addr , bits: u8)
, so you can write self.is_in_cidr(Ipv4Addr:new(100, 64, 0, 0), 10)
. I think that would improve readability and crosschecking with iana allocations because they usually give those blocks in CIDR notation.
Thanks for reviewing @the8472.
That could be a problem indeed, but if we don't provide this method people who need it will just implement their own and will have the same issue. What about adding a warning to the documentation of this method?
I haven't yet looked at what's done in other implementations, but I tend to disagree because:
Edit: @therealbstern @kpp @pusateri @jens1o @ollie27 since you've commented on #27709 you might have an opinion about the two questions @the8472 raised. |
also add test to Ipaddr, making sure that these addresses are not global.
Also add tests to IpAddr to make sure these addresses are not global.
can someone from @rust-lang/libs review this? |
@bors: r+ Thanks so much for all the extensive updates here @little-dude, and sorry for the delay! |
📌 Commit cddb838 has been approved by |
Thank you @alexcrichton ! |
std::net: Ipv4Addr and Ipv6Addr improvements Picking this up again from my previous PR: #56050 Related to: #27709 Fixes: #57558 - add `add Ipv4Addr::is_reserved()` - [X] implementation - [X] tests - add `Ipv6Addr::is_unicast_link_local_strict()` and update `Ipv6Addr::is_unicast_link_local()` documentation - [X] implementation - [X] test - add `Ipv4Addr::is_benchmarking()` - [X] implementation - [X] test - add `Ipv4Addr::is_ietf_protocol_assignment()` - [X] implementation - [X] test - add `Ipv4Addr::is_shared()` - [X] implementation - [x] test - fix `Ipv4Addr:is_global()` - [X] implementation - [x] test - [X] refactor the tests for IP properties. This makes the tests more verbose, but using macros have two advantages: - it will now be easier to add tests for all the new methods - we get clear error messages when a test is failing. For instance: ``` ---- net::ip::tests::ip_properties stdout ---- thread '<unnamed>' panicked at 'assertion failed: !ip!("fec0::").is_global()', src/libstd/net/ip.rs:2036:9 ``` Whereas previously it was something like ``` thread '<unnamed>' panicked at 'assertion failed: `(left == right)` left: `true`, right: `false`', libstd/net/ip.rs:1948:13 ``` ----------------------- # Ongoing discussions: ## Should `Ipv4Addr::is_global()` return `true` or `false` for reserved addresses? Reserved addresses are addresses that are matched by `Ipv4Addr::is_reserved()`. @the8472 [pointed out](#60145 (comment)) that [RFC 4291](https://tools.ietf.org/html/rfc4291#section-2.4) says IPv6 reserved addresses should be considered global: ``` Future specifications may redefine one or more sub-ranges of the Global Unicast space for other purposes, but unless and until that happens, implementations must treat all addresses that do not start with any of the above-listed prefixes as Global Unicast addresses. ``` We could extrapolate that this should also be the case for IPv4. However, it seems that [IANA considers them non global](https://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml) (see [my comment](#60145 (comment))) ### Final decision There seems to be a consensus that reserved addresses have a different meaning for IPv4 and IPv6 ([comment1](#60145 (comment)) [comment2](#60145 (comment)), so we can consider that RFC4291 does not apply to IPv4, and that reserved IPv4 addresses are _not_ global. ## Should `Ipv6Addr::is_unicast_site_local()` exist? @pusateri [noted](#60145 (comment)) that site-local addresses have been deprecated for a while by [RFC 3879](https://tools.ietf.org/html/rfc3879) and new implementations _must not_ support them. However, since this method is stable, removing does not seem possible. This kind of situation is covered by the RFC which stated that existing implementation _may_ continue supporting site-local addresses. ### Final decision Let's keep this method. It is stable already, and the RFC explicitly states that existing implementation may remain. --------- Note: I'll be AFK from April 27th to May 11th. Anyone should feel free to pick this up if the PR hasn't been merged by then. Sorry for dragging that for so long already.
☀️ Test successful - checks-travis, status-appveyor |
This introduces a few new pub fn On a second look, already existing ones like |
It looks like items in an unstable module default to inhering the unstable attribute of the module: https://doc.rust-lang.org/1.35.0/std/net/enum.IpAddr.html#method.is_global |
… but since the |
@SimonSapin I'll make a PR Thursday or Friday if @kumbayo hasn't by then. |
Move stability attribute for items under the `ip` feature The `#[unstable]` attribute for items under the `ip` feature is currently located on the `std::net::ip` module itself. This is unusual, and less readable. This has sidetracked discussion about these items numerous times (rust-lang#60145 (comment), rust-lang#76098 (comment), rust-lang#76098 (comment), rust-lang#75019 (comment), rust-lang#75019 (comment)) and lead to incorrect assumptions about which items are actually stable (rust-lang#60145 (comment), rust-lang#76098 (comment)). This PR moves the attribute from the module to the items themselves.
Move stability attribute for items under the `ip` feature The `#[unstable]` attribute for items under the `ip` feature is currently located on the `std::net::ip` module itself. This is unusual, and less readable. This has sidetracked discussion about these items numerous times (rust-lang#60145 (comment), rust-lang#76098 (comment), rust-lang#76098 (comment), rust-lang#75019 (comment), rust-lang#75019 (comment)) and lead to incorrect assumptions about which items are actually stable (rust-lang#60145 (comment), rust-lang#76098 (comment)). This PR moves the attribute from the module to the items themselves.
…htriplett Remove `Ipv6Addr::is_unicast_site_local` Removes the unstable method `Ipv6Addr::is_unicast_site_local`, see also rust-lang#85604 where I have tried to summarize related discussion so far. Unicast site-local addresses (`fec0::/10`) were deprecated in [IETF RFC rust-lang#3879](https://datatracker.ietf.org/doc/html/rfc3879), see also [RFC rust-lang#4291 Section 2.5.7](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.7). Any new implementation must no longer support the special behaviour of site-local addresses. This is mentioned in the docs of `is_unicast_site_local` and already implemented in `is_unicast_global`, which considers addresses in `fec0::/10` to have global scope, thus overlapping with `is_unicast_site_local`. Given that RFC rust-lang#3879 was published in 2004, long before Rust existed, and it is specified that any new implementation must no longer support the special behaviour of site-local addresses, I don't see how a user would ever have a need for `is_unicast_site_local`. It is also confusing that currently both `is_unicast_site_local` and `is_unicast_global` can be `true` for an address, but an address can actually only have a single scope. The deprecating RFC mentions that Site-Local scope was confusing to work with and that the classification of an address as either Link-Local or Global better matches the mental model of users. There has been earlier discussion of removing `is_unicast_site_local` (rust-lang#60145 (comment)) which decided against it, but that had the incorrect assumption that the method was already stable; it is not. (This confusion arose from the placement of the unstable attribute on the entire module, instead of on individual methods, resolved in rust-lang#85672) r? `@joshtriplett` as reviewer of all the related PRs
…riplett Remove `Ipv6Addr::is_unicast_site_local` Removes the unstable method `Ipv6Addr::is_unicast_site_local`, see also rust-lang#85604 where I have tried to summarize related discussion so far. Unicast site-local addresses (`fec0::/10`) were deprecated in [IETF RFC rust-lang#3879](https://datatracker.ietf.org/doc/html/rfc3879), see also [RFC rust-lang#4291 Section 2.5.7](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.7). Any new implementation must no longer support the special behaviour of site-local addresses. This is mentioned in the docs of `is_unicast_site_local` and already implemented in `is_unicast_global`, which considers addresses in `fec0::/10` to have global scope, thus overlapping with `is_unicast_site_local`. Given that RFC rust-lang#3879 was published in 2004, long before Rust existed, and it is specified that any new implementation must no longer support the special behaviour of site-local addresses, I don't see how a user would ever have a need for `is_unicast_site_local`. It is also confusing that currently both `is_unicast_site_local` and `is_unicast_global` can be `true` for an address, but an address can actually only have a single scope. The deprecating RFC mentions that Site-Local scope was confusing to work with and that the classification of an address as either Link-Local or Global better matches the mental model of users. There has been earlier discussion of removing `is_unicast_site_local` (rust-lang#60145 (comment)) which decided against it, but that had the incorrect assumption that the method was already stable; it is not. (This confusion arose from the placement of the unstable attribute on the entire module, instead of on individual methods, resolved in rust-lang#85672) r? `@joshtriplett` as reviewer of all the related PRs
…ou-se Remove `Ipv4Addr::is_ietf_protocol_assignment` This PR removes the unstable method `Ipv4Addr::is_ietf_protocol_assignment`, as I suggested in rust-lang#85612 (comment). The method was added in rust-lang#60145, as far as I can tell primarily for the implementation of `Ipv4Addr::is_global` (addresses reserved for IETF protocol assignment are not globally reachable unless otherwise specified). The method was added in 2019, but I haven't been able to find any open-source code using this method so far. I'm also having a hard time coming up with a usecase for specifically this method; knowing that an address is reserved for future protocols doesn't allow you to do much with it, especially since now some of those addresses are indeed assigned to a protocol and have their own behaviour (and might even be defined to be globally reachable, so if that is what you care about it is always more accurate to call `!is_global()`, instead of `is_ietf_protocol_assignment()`). Because of these reasons, I propose removing the method (or alternatively make it a private helper for `is_global`) and also not introduce `Ipv6Addr::is_ietf_protocol_assignment` and `IpAddr::is_ietf_protocol_assignment` in the future.
Picking this up again from my previous PR: #56050
Related to: #27709
Fixes: #57558
add Ipv4Addr::is_reserved()
Ipv6Addr::is_unicast_link_local_strict()
and updateIpv6Addr::is_unicast_link_local()
documentationIpv4Addr::is_benchmarking()
Ipv4Addr::is_ietf_protocol_assignment()
Ipv4Addr::is_shared()
Ipv4Addr:is_global()
Whereas previously it was something like
Ongoing discussions:
Should
Ipv4Addr::is_global()
returntrue
orfalse
for reserved addresses?Reserved addresses are addresses that are matched by
Ipv4Addr::is_reserved()
.@the8472 pointed out that RFC 4291 says IPv6 reserved addresses should be considered global:
We could extrapolate that this should also be the case for IPv4. However, it seems that IANA considers them non global (see my comment)
Final decision
There seems to be a consensus that reserved addresses have a different meaning for IPv4 and IPv6 (comment1 comment2, so we can consider that RFC4291 does not apply to IPv4, and that reserved IPv4 addresses are not global.
Should
Ipv6Addr::is_unicast_site_local()
exist?@pusateri noted that site-local addresses have been deprecated for a while by RFC 3879 and new implementations must not support them. However, since this method is stable, removing does not seem possible. This kind of situation is covered by the RFC which stated that existing implementation may continue supporting site-local addresses.
Final decision
Let's keep this method. It is stable already, and the RFC explicitly states that existing implementation may remain.
Note: I'll be AFK from April 27th to May 11th. Anyone should feel free to pick this up if the PR hasn't been merged by then. Sorry for dragging that for so long already.