-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Stabilize the "IP" feature #76098
Stabilize the "IP" feature #76098
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
r? @KodrAus hi! tagging you because you were pretty active on the old pr 😅 |
Since the FCP on the previous PR didn’t complete I think we’ll need to start a fresh one once this is ready to go. |
Yeah I apologize for the time reviewers lost on this =/ And thank you @caass for picking this up. |
@little-dude without your work I wouldn't have even known where to start -- not all is lost! |
☔ The latest upstream changes (presumably #75979) made this pull request unmergeable. Please resolve the merge conflicts. |
@little-dude Oh that's not at all a comment on you! You've done awesome work bringing this together. |
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.
Thanks for taking the time to put this together @caass!
I've left some suggestions for adding the right #[stable]
attributes we need to actually stabilize this API. It's not visible from the diff, but at the top of ip.rs
there's also this attribute:
#![unstable(
feature = "ip",
reason = "extra functionality has not been \
scrutinized to the level that it should \
be to be stable",
issue = "27709"
)]
We'll need to remove this along with adding the #[stable]
attributes.
This sounds like a good path forward to me! Thanks for digging through and making sense of all this material ❤️ |
Ok, so I went through and added checks for secret hidden IPv4 addresses to all (i think -- i may have missed some) of the IPv6 checks. I also renamed The last thing to do I think is to add test cases for the edge cases of e.g. Any sort of walkthrough on how lines 213-261 work would be very, very much appreciated. |
☔ The latest upstream changes (presumably #76422) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Thanks @caass!
I think the merge conflicts will be coming from this change: a2e077e They've just moved inline to the ip
module.
It looks like we've got a failing UI test in here:
failures:
---- [ui] ui/consts/std/net/ipv6.rs stdout ----
error: test run failed!
status: exit code: 101
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/consts/std/net/ipv6/a"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
thread 'main' panicked at 'assertion failed: IS_LOOPBACK', /checkout/src/test/ui/consts/std/net/ipv6.rs:21:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------
It might be easier to figure out what test this is after a rebase.
library/std/src/net/ip.rs
Outdated
#[rustc_const_unstable(feature = "const_ipv6", issue = "76205")] | ||
#[stable(since = "1.7.0", feature = "ip_17")] | ||
pub const fn is_unspecified(&self) -> bool { | ||
u128::from_be_bytes(self.octets()) == u128::from_be_bytes(Ipv6Addr::UNSPECIFIED.octets()) | ||
if let Some(v4_addr) = self.to_ipv4() { |
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 don't think this is a behavioral change, because an unspecified address is also just 0.0.0.0
.
library/std/src/net/ip.rs
Outdated
#[rustc_const_unstable(feature = "const_ipv6", issue = "76205")] | ||
#[stable(since = "1.7.0", feature = "ip_17")] | ||
pub const fn is_loopback(&self) -> bool { | ||
u128::from_be_bytes(self.octets()) == u128::from_be_bytes(Ipv6Addr::LOCALHOST.octets()) | ||
if let Some(v4_addr) = self.to_ipv4() { |
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.
Documenting that this is a behavioral breaking change for:
::127.0.0.1
/::ffff:127.0.0.1
library/std/src/net/ip.rs
Outdated
#[rustc_const_unstable(feature = "const_ipv6", issue = "76205")] | ||
#[stable(since = "1.7.0", feature = "ip_17")] | ||
pub const fn is_multicast(&self) -> bool { | ||
(self.segments()[0] & 0xff00) == 0xff00 | ||
if let Some(v4_addr) = self.to_ipv4() { |
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.
Documenting that this is a behavioral breaking change for:
::224.254.0.0
/::ffff:224.254.0.0
::236.168.10.65
/::ffff:236.168.10.65
Ping from triage |
@JohnCSimon hi! I have been sitting here hoping someone with more knowledge of macros than I will be able to write/fix the test cases before i keep pushing on this PR, but I suppose eventually I'll have to learn them myself 😅 I'll resolve the merge conflicts, but the tests are broken until I (or someone else 🤞) rewrite them to reflect the behavioral changes mentioned by @KodrAus :) |
@caass I can probably help with that. |
!!! @little-dude i would love that! i'll merge later today and write some comments talking about what exactly needs to be done :) |
Would the platform-independent functional to be moved to core? |
@caass Ping from triage, CI is still red. Would you mind fixing the tests? Thanks. |
Ping from triage: |
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.
…r=joshtriplett Remove `Ipv6Addr::is_unicast_link_local_strict` Removes the unstable method `Ipv6Addr::is_unicast_link_local_strict` and keeps the behaviour of `Ipv6Addr::is_unicast_link_local`, see also rust-lang#85604 where I have tried to summarize related discussion so far. My intent is for `is_unicast_link_local`, `is_unicast_site_local` and `is_unicast_global` to have the semantics of checking if an address has Link-Local, Site-Local or Global scope, see also rust-lang#85696 which changes the behaviour of `is_unicast_global` and renames these methods to `has_unicast_XXX_scope` to reflect this. For checking Link-Local scope we currently have two methods: `is_unicast_link_local` and `is_unicast_link_local_strict`. This is because of what appears to be conflicting definitions in [IETF RFC 4291](https://datatracker.ietf.org/doc/html/rfc4291). From [IETF RFC 4291 section 2.4](https://datatracker.ietf.org/doc/html/rfc4291#section-2.4): "Link-Local unicast" (`FE80::/10`) ```text Address type Binary prefix IPv6 notation Section ------------ ------------- ------------- ------- Unspecified 00...0 (128 bits) ::/128 2.5.2 Loopback 00...1 (128 bits) ::1/128 2.5.3 Multicast 11111111 FF00::/8 2.7 Link-Local unicast 1111111010 FE80::/10 2.5.6 Global Unicast (everything else) ``` From [IETF RFC 4291 section 2.5.6](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.6): "Link-Local IPv6 Unicast Addresses" (`FE80::/64`) ```text | 10 bits | 54 bits | 64 bits | +----------+-------------------------+----------------------------+ |1111111010| 0 | interface ID | +----------+-------------------------+----------------------------+ ``` With `is_unicast_link_local` checking `FE80::/10` and `is_unicast_link_local_strict` checking `FE80::/64`. There is also [IETF RFC 5156 section 2.4](https://datatracker.ietf.org/doc/html/rfc5156#section-2.4) which defines "Link-Scoped Unicast" as `FE80::/10`. It has been pointed out that implementations in other languages and the linux kernel all use `FE80::/10` (rust-lang#76098 (comment), rust-lang#76098 (comment)). Given all of this I believe the correct interpretation to be the following: All addresses in `FE80::/10` are defined as having Link-Local scope, however currently only the block `FE80::/64` has been allocated for "Link-Local IPv6 Unicast Addresses". This might change in the future however; more addresses in `FE80::/10` could be allocated and those will have Link-Local scope. I therefore believe the current behaviour of `is_unicast_link_local` to be correct (if interpreting it to have the semantics of `has_unicast_link_local_scope`) and `is_unicast_link_local_strict` to be unnecessary, confusing and even a potential source of future bugs: Currently there is no real difference in checking `FE80::/10` or `FE80::/64`, since any address in practice will be `FE80::/64`. However if an application uses `is_unicast_link_local_strict` to implement link-local (so non-global) behaviour, it will be incorrect in the future if addresses outside of `FE80::/64` are allocated. r? `@joshtriplett` as reviewer of all the related PRs
Stabilize `{IpAddr, Ipv6Addr}::to_canonical` Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`. Newly stable API: ```rust impl IpAddr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; } impl Ipv6Addr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; // Already stable, this makes it const stable under // `const_ipv6_to_ipv4_mapped` const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr> } ``` These stabilize a subset of the following tracking issues: - rust-lang#27709 - rust-lang#76205 Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward. I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708 cc implementor `@the8472` r? libs-api `@rustbot` label +T-libs-api +needs-fcp
Stabilize `{IpAddr, Ipv6Addr}::to_canonical` Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`. Newly stable API: ```rust impl IpAddr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; } impl Ipv6Addr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; // Already stable, this makes it const stable under // `const_ipv6_to_ipv4_mapped` const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr> } ``` These stabilize a subset of the following tracking issues: - rust-lang#27709 - rust-lang#76205 Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward. I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708 cc implementor ``@the8472`` r? libs-api ``@rustbot`` label +T-libs-api +needs-fcp
Rollup merge of rust-lang#115955 - tgross35:ip-to-canonical, r=dtolnay Stabilize `{IpAddr, Ipv6Addr}::to_canonical` Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`. Newly stable API: ```rust impl IpAddr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; } impl Ipv6Addr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; // Already stable, this makes it const stable under // `const_ipv6_to_ipv4_mapped` const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr> } ``` These stabilize a subset of the following tracking issues: - rust-lang#27709 - rust-lang#76205 Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward. I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708 cc implementor ``@the8472`` r? libs-api ``@rustbot`` label +T-libs-api +needs-fcp
There have been no substantial changes in implementation of various Rust's IpAddr helper methods (is_loopback() etc.) that I can think of. The PR[1] that the "Rust (New)" implementation was based on has been closed in May 2021, I think it's safe to say it's no longer relevant and doesn't help anyone. [1] rust-lang/rust#76098
Latest Status Update: #76098 (comment)
Continuation of #66584, since it seems to be dead. This is my first time contributing and I did my best to read through the rustc-dev-guide, but I'll be honest I kind of skipped around a little bit. If there's anything I need to do that hasn't been done yet in terms of process, please let me know and I'll do it 😄
The old PR seemed to me (could be wrong) to be almost done, but there were two issues that were left unresolved. I left a comment on the PR, but I'll copy/paste it here for convenience:
If I understand correctly, the blockers currently are:
From the RFC:
If we already have capability to check IPv4 addresses, I feel like it's almost trivial to implement these rules for IPv6 as well if we just need to pad it with some extra bytes? Famous last words, I know...
is_unicast_link_local_strict
andis_unicast_link_local
(comment)From the commit that introduced the change:
From section 2.4:
Section section 2.5.3 seems...unrelated? It has to do with the loopback address. Section 2.5.6 however, is where the problem lies:
So the difference in the spec is this:
This does seem like an issue, so I did some more digging to figure out what's going on. I found this draft, which appears to have died. It does however point us in the right direction:
I'm not an expert in reading IETF material -- far from it -- but I am vaguely aware of the passage of time. Most documents linked here that specify /64 are more recent than those specifying /10, unless I've missed some -- which is totally possible. Additionally, there are countless secondhand sources (blogs, stackoverflow, etc) online all talking about how /64 is what's commonly used.Again, not an expert. I posted all this info here so someone more versed than me can evaluate and decide what the best course of action is. Without further input, I'd personally go ahead with just the oneis_unicast_link_local
that validates against /64, since it's a more strict subset of /10, and if someone runs into trouble with running custom link-local addresses outside of /64 space I kind of feel like that is a problem outside the scope of the Rust programming language, and is something they should talk to their network administrator about.Edit: Per further discussion here and here, I've decided to simply remove the
_strict
check to bring Rust's functionality more in line with other similar features in other languages (e.g. Go, C, Python) as well as the Linux kernel.At the moment, this PR is just fast-forwarding the existing work done by @little-dude to be up-to-date with rust-lang/rust. I wanted to get some confirmation that this hasn't been implemented yet in some other way / i'm not duplicating work / this feature is still needed before I get started on the code. Thanks in advance for your time! ❤️