Stabilize the "IP" feature (continued from #76098) #85585

saethlin · 2021-05-22T17:19:09Z

I noticed the work in #76098 seemed abandoned, so I'm reviving it here. I'm not particularly familiar with RFC 4291 but I'm no stranger to reading IETF RFCs.

I'm not totally clear on what still needs to be done. There's a fair bit of discussion in the preceding PR, but some if it seems outdated. Over the next few days I'll try to piece together what still needs to be done, but if anyone (@KodrAus, @CDirkx, @jyn514) knows better than I please point me in the right direction.

Feature tracking issue: rust-lang#27709 - Stabilize the following methods: - `IpAddr::is_global` - `IpAddr::is_documentation` - `Ipv4Addr::is_global` - `Ipv4addr::is_shared` - `Ipv4Addr::is_ietf_protocol_assignment` - `Ipv4addr::is_benchmarking` - `Ipv4Addr::is_reserved` - `Ipv6Addr::is_global` - `Ipv6Addr::is_unique_local` - `Ipv6Addr::is_unicast_link_local_strict` - `Ipv6Addr::is_unicast_link_local` - `Ipv6Addr::is_unicast_site_local` - `Ipv6Addr::is_documentation` - `Ipv6Addr::is_unicast_global` - `Ipv6Addr::multicast_scope` - Stabilize the following enum: `Ipv6MulticastScope` - Document IP helpers stability guarantees (fixes rust-lang#60239)

Add variants for the currently reserved and un-assigned scopes. Since variants may be added in the future as new RFCs get published, we also make this enum #[non_exhaustive]. Ref: rust-lang#66584 (comment)

Thanks again for the help with understanding the attributes! Co-authored-by: Ashley Mannix <kodraus@hey.com>

Remove the looser `is_unicast_link_local` and rename `is_unicast_link_local_strict` as i suggested 😊 See rust-lang#76098 (comment)

See rust-lang#66584 (comment)

Getting kind of extremely out of my comfort zone here and modifying quite a few different files...i hope this is all ok...

I really...i'm terrified i'm breaking something here

Because of the changes made due to the discussion [here](rust-lang#76098 (comment)), the checks for the more relaxed behavior of the old `is_unicast_link_local` method failed when they were tuned to the new, stricter checking behavior. These test cases have been modified to check that a given IPv6 address *is not* unicast link local, using a bitwise or'ing strategy that was already present in other test cases. Further discussion [here](https://rust-lang.zulipchat.com/#narrow/stream/212497-t-compiler.2Fhelp-wanted/topic/fix.20tests.20for.20.2376098).

This brings the behavior more in line with Go, Python, Linux, and C. I know, this should have been a `git revert` but honestly I don't trust myself with that and I just had a big mishap trying to merge upstream Rust anyway so forgive me :crying:

This reverts commit e0f66e43da61d8fe8aadc6bcdc2545de51607332.

Per [RFC 4291](https://tools.ietf.org/html/rfc4291#section-2.5.5.1), IPv6-Compatible IPv4 Addresses are deprecated and may be ignored, so let's ignore them! Also add some tests indicating that IPv6-Mapped addresses are treated specially, but IPv6-Compatible addresses are not.

…ses" This reverts commit 63329f9eadbba6da75a04cbf613f5701b06f71e5. Also, change the behavior of `to_ipv4` to ignore Ipv4-compatible addresses instead of adding another method `to_ipv4_mapped` and never ever using `to_ipv4` instead. Since this method is in stable.

- Use new intra-doc linking - Clarify that the RFC's are IETF RFC's

rust-lang#76098 (comment)

just did a really big rebase and i'm slightly concerned that i somehow tanked this whole branch but i guess we'll see

* Move libstd ip tests to their own module * Delete failing test; mapped addresses are never unspecified

rust-highfive · 2021-05-22T17:19:12Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

jyn514 · 2021-05-22T23:36:23Z

@saethlin this is the latest update I know of: #76098 (comment). I don't know the status other than that.

CDirkx · 2021-05-23T03:24:57Z

I think the best way forward for this PR is to split it up into several smaller pieces:

Document Ipv6MulticastScope (Add documentation for Ipv6MulticastScope #86936)
Remove is_unicast_link_local_strict (Remove Ipv6Addr::is_unicast_link_local_strict #85819)
Remove ``is_unicast_site_local` (Remove Ipv6Addr::is_unicast_site_local #85820)
Possibly introduce Ipv6UnicastScope
Currently is_global doesn't take into account some addresses designated as "not globally reachable" (Rework Ipv6Addr::is_global to check for global reachability rather than global scope #86634)
Decide if IPv6 methods should take IPv4-mapped addresses into account (Change Ipv6Addr::is_loopback to include IPv4-mapped loopback addresses #85655 vs Commit to not supporting IPv4-in-IPv6 addresses #86335)
Decide the behaviour of to_ipv4 and whether to keep to_ipv4_mapped
Possibly introduce the IPv6 equivalent of is_benchmarking, more? Maybe is_ipv4_mapped (Add Ipv6Addr::is_benchmarking #86434 and Add Ipv6Addr::is_ipv4_mapped #86490)

Edit: updated to reflect the current state of things

CDirkx · 2021-05-23T20:42:17Z

I split everything up into several issues:

IPv6 Unicast Interface #85604 IPv6 Unicast Interface, for discussion about the is_unicast_ methods.
IPv4-in-IPv6 Address Support #85609 IPv4-in-IPv6 Address Support, for discussion about which Ipv6Addr methods should also check IPv4-mapped addresses.
IP Utility Methods #85612 IP Utility Methods, for discussion about which utility methods should be stabilized.

CDirkx · 2021-05-23T20:58:10Z

Directly actionable for anyone right now:

~~Add documentation to Ipv6MulticastScope~~ (Add documentation for Ipv6MulticastScope #86936)
Make is_loopback take IPv4-mapped addresses into account (so match both ::1 and ::ffff:127.0.0.1) fixing std::net::IpAddr: is_loopback failing on ipv4-in-ipv6 addresses #69772. See IPv4-in-IPv6 Address Support #85609, this seems the only IPv4-mapped change that is uncontroversial. PR Change Ipv6Addr::is_loopback to include IPv4-mapped loopback addresses #85655
Remove is_unicast_link_local_strict. See IPv6 Unicast Interface #85604, it seems pretty clear that we want to keep the current behaviour of is_unicast_link_local and that is_unicast_link_local_strict is not needed. If someone collects all the evidence for that in one place and opens up a PR I think it would be uncontroversial. PR Remove Ipv6Addr::is_unicast_link_local_strict #85819

I think anything else will require addressing the open problems listed in the issues I opened, which will require gathering evidence for the best approach and reaching a consensus. So I don't see stabilization of the entire ip feature possible in the near future. I think it would be better to instead break it up into smaller features like ipv6_unicast, ip_global etc. to enable stabilization piece by piece.

Mark-Simulacrum · 2021-05-26T02:31:40Z

r? @joshtriplett as T-libs member, not sure what the state here is but I likely don't have the expertise on various IP stuff to say anyway.

bors · 2021-05-26T10:23:03Z

☔ The latest upstream changes (presumably #85711) made this pull request unmergeable. Please resolve the merge conflicts.

crlf0710 · 2021-06-26T03:26:32Z

@saethlin Ping from triage! There's merge conflict now. Would you mind rebasing this pr?

saethlin · 2021-07-02T14:54:55Z

@crlf0710 I appreciate the attention. I'll get to this soon.

JohnTitor · 2021-07-13T23:40:37Z

Triage: Marking as S-blocked as we're splitting this PR into smaller ones and triaging is unnecessary until it's done.

saethlin · 2022-02-22T21:41:59Z

Closing this because I have precious little expertise to offer here. It looks like there are much more capable contributors about, and I have other priorities.

little-dude and others added 24 commits May 22, 2021 12:26

std::net: add Ipv6Multicastscope variants

7b7ca5d

Add variants for the currently reserved and un-assigned scopes. Since variants may be added in the future as new RFCs get published, we also make this enum #[non_exhaustive]. Ref: rust-lang#66584 (comment)

Update feature gates

f33f945

Apply suggestions from code review

86e9b37

Thanks again for the help with understanding the attributes! Co-authored-by: Ashley Mannix <kodraus@hey.com>

is_unicast_link_local_strict --> is_unicast_link_local

f91cb14

Remove the looser `is_unicast_link_local` and rename `is_unicast_link_local_strict` as i suggested 😊 See rust-lang#76098 (comment)

Add checking for ipv4-(mapped | compatible) addresses

259ada1

See rust-lang#66584 (comment)

Add stability attributes to Ipv6MulticastScope

f15098f

remove /// #![feature(ip)] tags from doc tests

c8ca757

Remove some more #![feature(ip)] stuff

392a21b

Getting kind of extremely out of my comfort zone here and modifying quite a few different files...i hope this is all ok...

Remove some code having to do with link-local strict

f5694dc

Change checks for is_unicast_link_local_strict to is_unicast_link_local

8de3caf

I really...i'm terrified i'm breaking something here

Rewrite ipv4 checks to check ipv6 stuff first

f224551

Revert "Fix test cases that checked for relaxed behavior"

179d376

This reverts commit e0f66e43da61d8fe8aadc6bcdc2545de51607332.

Remove IP feature gate

4f77918

Remove duplicate UI tests as per @CDirkx's suggestions

2f1078d

Rewrite using new rustdoc comments!!!!

0348e5e

Update doc comments

e6b76f0

- Use new intra-doc linking - Clarify that the RFC's are IETF RFC's

stop treating ::ffff:0.0.0.0 as unspecified

f469444

rust-lang#76098 (comment)

wow i hope i didn't mess something up

240a913

just did a really big rebase and i'm slightly concerned that i somehow tanked this whole branch but i guess we'll see

Fix tests

6c42f4a

* Move libstd ip tests to their own module * Delete failing test; mapped addresses are never unspecified

rust-highfive assigned Mark-Simulacrum May 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2021

CDirkx mentioned this pull request May 23, 2021

IPv6 Unicast Interface #85604

Open

This was referenced May 23, 2021

IPv4-in-IPv6 Address Support #85609

Open

IP Utility Methods #85612

Open

CDirkx mentioned this pull request May 25, 2021

Change Ipv6Addr::is_loopback to include IPv4-mapped loopback addresses #85655

Closed

rust-highfive assigned joshtriplett and unassigned Mark-Simulacrum May 26, 2021

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021

JohnTitor added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2021

the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 8, 2021

elenaf9 mentioned this pull request Nov 15, 2021

protocols/: Add basic AutoNAT implementation libp2p/rust-libp2p#2262

Merged

saethlin closed this Feb 22, 2022

saethlin deleted the stabilize-ip branch May 16, 2022 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize the "IP" feature (continued from #76098) #85585

Stabilize the "IP" feature (continued from #76098) #85585

saethlin commented May 22, 2021

rust-highfive commented May 22, 2021

jyn514 commented May 22, 2021

CDirkx commented May 23, 2021 •

edited

Loading

CDirkx commented May 23, 2021

CDirkx commented May 23, 2021 •

edited

Loading

Mark-Simulacrum commented May 26, 2021

bors commented May 26, 2021

crlf0710 commented Jun 26, 2021

saethlin commented Jul 2, 2021

JohnTitor commented Jul 13, 2021

saethlin commented Feb 22, 2022

Stabilize the "IP" feature (continued from #76098) #85585

Stabilize the "IP" feature (continued from #76098) #85585

Conversation

saethlin commented May 22, 2021

rust-highfive commented May 22, 2021

jyn514 commented May 22, 2021

CDirkx commented May 23, 2021 • edited Loading

CDirkx commented May 23, 2021

CDirkx commented May 23, 2021 • edited Loading

Mark-Simulacrum commented May 26, 2021

bors commented May 26, 2021

crlf0710 commented Jun 26, 2021

saethlin commented Jul 2, 2021

JohnTitor commented Jul 13, 2021

saethlin commented Feb 22, 2022

CDirkx commented May 23, 2021 •

edited

Loading

CDirkx commented May 23, 2021 •

edited

Loading