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

protocols.csv: Add "ip6%" multiaddr (IPv6 with zone ID) #68

Merged
merged 1 commit into from Sep 10, 2018
Merged

protocols.csv: Add "ip6%" multiaddr (IPv6 with zone ID) #68

merged 1 commit into from Sep 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 16, 2018

This is a generalization of the "ip6" multiaddr which accepts an
optional zone ID as specified in [rfc4007].

[rfc4007] https://tools.ietf.org/html/rfc4007


One probably controversial point is that I'm using a non alphanumeric character ('%') in the protocol string. I'll be happy to change it to a 'z' if there is the slightest objection.

See also:

@ghost
Copy link
Author

ghost commented Jun 16, 2018

Another point I'm not sure about: I couldn't figure out any pattern or logic to the protocol code assignment so I just went with IP6 + 1.

@Stebalien
Copy link
Member

Yeah, I'd rather have ip6z or even ip6ll (link local). However, there's an alternative we should consider:

/link/lo0/ip6/.../tcp/1234

This would mean "connect to link X, and from there, connect to IP Y, ...". Thoughts?

/cc @lgierth you generally have opinions on multiaddr stuff.

@ghost
Copy link

ghost commented Jun 18, 2018

Why not make /ip6 accept the zone-id? It's a valid part of an IPv6 address after all. (Just making sure I'm not missing anything while going through my backlog.)

@Stebalien
Copy link
Member

Why not make /ip6 accept the zone-id? It's a valid part of an IPv6 address after all. (Just making sure I'm not missing anything while going through my backlog.)

The argument length is currently fixed so that would break existing addresses.

It's a valid part of an IPv6 address after all.

It's a part of a valid textual IPv6 address but there's no place for it in the binary representation.

@ghost
Copy link

ghost commented Jun 18, 2018

Got it -- that makes sense. It's also in line with /ipcidr being separate.

Name bikeshed: /ipzone? The additional characters don't seem too scary, especially given they're not sent over the network anyway.

@Stebalien
Copy link
Member

Well, I was thinking this could also work with IP4 and potentially mac.

@ghost
Copy link
Author

ghost commented Jun 18, 2018

I had considered adding a separate zoneid "multiaddr" code, but there were a few points I didn't quite like about it:

  1. The zone ID is not a protocol. Not sure that matters in the end...

  2. Where would the zone ID even go? Before or after the ip6 entry? Before seems reasonable since the zone ID represents an interface, which I guess is lower down in the IP stack...

  3. The zoneid multiaddr protocol could only be combined with ip6 and nothing else. @Stebalien You seem to be suggesting adding a more general link "protocol" (that might for instance further restrict the interface to use in some circumstances I guess) but I feel like that would constitute an overreach of multiaddr's role: it would require multiaddr implementations to add some logic to distinguish between the ipv6 zone ID case and other cases and perform different network library/system calls depending on the case, and I'm pretty sure the exact semantics of both cases would end up differing.

  4. Interaction and consistency with other non-ipfs/libp2p-related systems and tools. Many other interfaces (APIs, tools) consider an ipv6 address to be an ipv6 address + an optional zone ID. This includes go's net library and certain unix/posix functions I believe.

I'll admit these arguments are all quite weak, so if there is otherwise a consensus against the proposal as currently implemented, I'll make the necessary changes. As already stated in point 3., the only thing I strongly disagree with is the addition of a more general link "protocol". I would rather have an ipv6-specific zoneid "protocol" (or zone?, or ip6zone to avoid future collisions/confusion?).

@ghost
Copy link
Author

ghost commented Jun 24, 2018

So, what do you think? Would you accept a patch adding an ip6zone protocol?

@Stebalien
Copy link
Member

The zone ID is not a protocol. Not sure that matters in the end...

True, but we don't using dns{4,6,addr} as "protocols" either. A multiaddr is basically just a set of steps describing how to build up a connection. In this case, "zone" would just mean "scope any subsequent IP addresses to the given zone".

Where would the zone ID even go? Before or after the ip6 entry? Before seems reasonable since the zone ID represents an interface, which I guess is lower down in the IP stack...

Yes, before. It'd be /link/lo/ip6/::1/tcp/8080.

it would require multiaddr implementations to add some logic to distinguish between the ipv6 zone ID case and other cases and perform different network library/system calls depending on the case, and I'm pretty sure the exact semantics of both cases would end up differing.

So, I'm pretty sure that the zone ID always corresponds to an interface number (on all platforms). link may not be the correct term (maybe iface?) but I'd like to generalize this as much as possible.

Interaction and consistency with other non-ipfs/libp2p-related systems and tools. Many other interfaces (APIs, tools) consider an ipv6 address to be an ipv6 address + an optional zone ID. This includes go's net library and certain unix/posix functions I believe.

So... this quite often isn't the case. For example, in go, the IP struct doesn't have a place for the Zone. Instead, you have to use the IPAddr struct. This really is a general problem because the binary representation of an IPv6 address doesn't encode the zone.


FYI, an alternative is to just do what BSD does and store the zone ID in second 16bit word of the address. This will never conflict with anything as, in "proper" IPv6, this word must be 0. Actually, this may be the way to go.

I'm not suggesting we display it there or put it there in the string format. Basically, we'd take an address in the form /ip6/fe80::...%0/... and serialize it to binary as fe80:0:.... Old versions may display these addresses as /ip6/fe80:x:/..., but that's not really a problem.

Thoughts?

@Stebalien
Copy link
Member

Basically, I'd like to avoid having two ipv6 protocols.

@ghost
Copy link

ghost commented Jul 1, 2018

You're spot on @Stebalien 👍

@ghost
Copy link
Author

ghost commented Jul 1, 2018 via email

@Stebalien
Copy link
Member

I was aware of this BSD trick, however, I can see three major reasons against using it in multiaddr:

All very good points.

Do you think it would make sense (fit in with multiaddr's design principles) to add some syntactic sugar translating /ip6/<ip6>%<zone> to /zone/<zone>/ip6/<ip6>? I feel like it would make for a better user experience in cases where human input is expected, but is it worth the added complexity? What about the reverse translation?

If we go with the /zone/ prefix, we wouldn't want to allow the % syntax.

(although I'd still call it something like iface as, e.g., lo is an interface, not a zone)

@Stebalien
Copy link
Member

Stebalien commented Jul 2, 2018

Sign-offs:

The proposal here is to add a new protocol, /zone (or /iface or /link) that allows one to specify the IPv6 "zone". This would be used as /zone/eth0/ip6/ff80::3/tcp/1234.

The alternatives were:

  1. Introduce a replacement protocol for IPv6 that supports the %zone suffix. My objection was that this would introduce two IPv6 protocols.
  2. Use the BSD and friends hack of sticking the zone in an unused 16bit word inside the encoded IPv6 address. However, as pointed out by @mwnx,
    1. We'd have to resolve interface names to numbers up-front when converting to a string to the binary representation. IMO, we may want to do this anyways and just make it a part of the string to binary conversion but that's something we can discuss after we've figured this step out.
    2. We'd only be able to support 16bit zone IDs. Given that the BSDs (including MacOS) do it this way, I'm not sure how much of an issue this would be however, it might mean we'd have to introduce a new syntax later anyways to deal with larger zone IDs.
    3. This would only support link-local addresses. IMO, this is the main reason not to do it this way.

Given that this is a decision that'll impact us down the road, you should take a look and think about it.


Questions:

  • Should we even allow strings in the encoded binary format? We usually use the string format in APIs/CLIs. However, we may need to allow this for a libp2p daemon (especially when the daemon may be running in a different network namespace.
  • Should we go with the second alternative for now and then consider adding the /zone prefix later? That does add some duplicate functionality but it would allow us to punt the more complicated choice down the road.

@ghost
Copy link
Author

ghost commented Jul 15, 2018

Well, I finally got to it (one week late, sorry). I've force-pushed on all four projects. I went with the "zone" prefix solution. And yes, I do think it should be called "zone" since that is the vocabulary used in the IETF standard (rfc4007).

@Stebalien
Copy link
Member

And yes, I do think it should be called "zone" since that is the vocabulary used in the IETF standard (rfc4007).

Hm. I see. There's actually a distinction between link-local and interface-local so either interface or link would definitely be wrong.

@Stebalien Stebalien assigned ghost , daviddias and whyrusleeping and unassigned whyrusleeping Jul 18, 2018
@ghost
Copy link
Author

ghost commented Jul 29, 2018

So, uh, any chance of this being merged?

@Stebalien
Copy link
Member

I was hoping to get a signoff from either @lgierth or @diasdavid.

@Stebalien
Copy link
Member

Note: It looks like the multicodec definition still needs to be updated.

@ghost
Copy link
Author

ghost commented Jul 30, 2018 via email

@Stebalien
Copy link
Member

@mwnx need to push 😄.

@ghost
Copy link
Author

ghost commented Jul 30, 2018 via email

@Stebalien
Copy link
Member

This PR also needs to be updated.

@ghost
Copy link
Author

ghost commented Jul 31, 2018 via email

@ghost
Copy link
Author

ghost commented Aug 13, 2018

Bump :)

@Stebalien
Copy link
Member

Still trying to get at least one other PL member to sign off on this.

protocols.csv Outdated
@@ -4,6 +4,7 @@ code, size, name, comment
17, 16, udp,
33, 16, dccp,
41, 128, ip6,
42, V, zone, rfc4007 IPv6 zone
Copy link
Member

@daviddias daviddias Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it more explicit and use ip6zone for the name?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's go with /ip6zone -- these zones are about IPv6 only at the moment, and at the moment I don't see us generalizing this enough to fit other cases (similar to /dns4,6 vs. /dns).

@daviddias
Copy link
Member

Pinging the Lead Maintainer of the multiaddr JS module, @victorbjelkholm and js-libp2p devs, @jacobheun & @vasco-santos to review this proposal.

@ghost
Copy link
Author

ghost commented Aug 26, 2018

Sounds reasonable. I guess I'll wait for further confirmations before updating all the pull requests then?

@jacobheun
Copy link

This looks good to me and the change to ip6zone makes sense.

@vasco-santos
Copy link
Member

Makes sense to me! 👍

Can be prefixed to the "ip6" protocol. See [rfc4007].

[rfc4007] https://tools.ietf.org/html/rfc4007
@ghost
Copy link
Author

ghost commented Aug 28, 2018 via email

@Stebalien Stebalien merged commit 6ba6cbb into multiformats:master Sep 10, 2018
@Stebalien
Copy link
Member

I guess we can still say "faster than the IETF", but that's not saying much.

@Stebalien
Copy link
Member

@mwnx mind making PRs against go-multiaddr now?

@ghost
Copy link
Author

ghost commented Sep 11, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants