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

net: InterfaceAddrs is not good for multi-homed IP node #14518

Closed
mikioh opened this issue Feb 26, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@mikioh
Copy link
Contributor

commented Feb 26, 2016

I propose adding the missing Zone field to IPNet structure.

A trouble: The lack of IPv6 addressing scope information makes IPNet less usable. For example, when we have an IPNet and it represents an IPv6 link-local address, it's hard for us to distinguish the address belongs to which link/network interface.

A workaround: Rescan IP routing information by using Interfaces function and Addrs method of Interface structure (and play a guessing game when the address comes from foreign packages because it's possible to have the same IPv6 link-local address on all the equipped links.)

Proposal: From Go 1.6 the network interface and interface address identification feature works correctly for both IPv4 and IPv6 on tier-1 platforms. I think it makes sense to add Zone field to IPNet structure the same as {IP,TCP,UDP}Addr structures like the following:

type IPNet struct {
        IP   IP     // network number
        Mask IPMask // network mask
        Zone string // IPv6 scoped addressing zone
}

make IPNet.String return "fe80::1%en0/64" and ParseCIDR function being able to parse IPv6 address prefix literal including zone identifier such as "fe80::1%en0/64". Also network interface and interface address identification feature comprised of Interfaces function and Addrs method of Interface structure behaves the same as IP address resolution and connection setup features; just handles unicast IPv6 link-local addressing scope information only.

An implementation: https://go-review.googlesource.com/#/c/19946/

@mikioh mikioh added the Proposal label Feb 26, 2016

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

This is a proposal for Go 1.7.

@mikioh mikioh added this to the Go1.7 milestone Mar 9, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Mar 10, 2016

CL https://golang.org/cl/19946 mentions this issue.

@gopherbot gopherbot closed this in 3e9264c Apr 19, 2016

@rsc rsc reopened this Apr 27, 2016

@rsc rsc changed the title proposal: net: add missing Zone field to IPNet proposal: net: add missing Zone field to IPNet? Apr 27, 2016

@rsc rsc changed the title proposal: net: add missing Zone field to IPNet? net: add missing Zone field to IPNet (revert?) Apr 27, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

Hi Mikio,

Can you explain to me why this makes sense? The type is called IPNet and it used to be an IP address plus a network mask. Now it is really an IPNetZone but still called IPNet. This is confusing to me, and adding the field is going to break a TON of composite literals that assumed the thing called IPNet would only ever contain two fields: an IP and a net. That code shouldn't do that, but it will break nonetheless.

Furthermore, the main reason IPNet exists is to provide the Contains method, and of course the Zone has no bearing there. That's also confusing.

It looks like IPNet is being extended only because it was already in use in certain data structures. Maybe that use is the real mistake and this change should be reverted. I know we've already reverted this change once, in 2013 (#4501), but I still don't understand the rationale. Can you help me understand?

Thanks.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2016

Hi Russ,

My understanding of IPNet is that it represents an IP network. That was my intention of adding the type to net package and is the reason of existence.

The type can be used for conveying the information of IP network. For example, APIs surveying network interfaces in net package use the type to represent on-link address prefixes that designate connected networks and addresses on the node. The information is necessary for applications discovering and making some association between on-link neighbors.

Applications using only global-scope addresses never suffer with the lack of Zone, but applications using link-local addresses may suffer. What if the returned list of IPNet from InterfaceAddrs contains a few address prefixes which are the same, such as "fe80::1/64". The applications need some information to distinguish IP links.

Now it is really an IPNetZone but still called IPNet.

Interesting, I've never thought that it would be confusing. My understanding is that IP of IPNet would be the entire address space, a functional address, or address blocks. Mask of IPNet helps to cut certain address blocks, and Zone of IPNet helps to identify the required link. IPNet just represents a network at IP layer not only for network-to-network communication but node-to-network (or node-to-node) communication.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

@pmarks-net, do you have any insights into this? I have basically no experience with link-scoped addresses in IPv6. Thanks.

@pmarks-net

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

I would bet that the vast majority of IPNet users just want to represent a CIDR prefix (e.g. 2607:f8b0::/32). Adding a zone gives you something conceptually different, because an interface name is only relevant to the machine that created it.

Whatever change is made, a primary goal should be to be as unobtrusive as possible to the "normal" users; if people need to start adding nils to their IP geolocation and spam filtering libraries, then it's a lost cause.

My inclination would be to make IPNet and IPNetZone distinct types, so that the type system can highlight the cases where a zone is actually relevant.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

I'm fine to have a new type and make InterfaceAddrs and Addrs method of Interface return the new type instead of IPNet. I'd like to prefer IPPrefix or IPAddrPrefix than IPNetZone, because I have no good answer to "why not TCPAddrZone instead of TCPAddr?"

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

Of course I meant make InterfacePrefixes and Prefixes method of Interface return the new type not to break existing applications.

@gopherbot

This comment has been minimized.

Copy link

commented May 5, 2016

CL https://golang.org/cl/22794 mentions this issue.

gopherbot pushed a commit that referenced this issue May 6, 2016

Revert "net: add support for Zone of IPNet"
Updates #14518.

This reverts commit 3e9264c.

Change-Id: I2531b04efc735b5b51ef675541172f2f5ae747d9
Reviewed-on: https://go-review.googlesource.com/22836
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Run-TryBot: Mikio Hara <mikioh.mikioh@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@mikioh mikioh modified the milestones: Go1.8, Go1.7 May 6, 2016

@mikioh mikioh changed the title net: add missing Zone field to IPNet (revert?) net: add a new type that represents IP address prefix instead of IPNet May 6, 2016

@mikioh mikioh unassigned rsc May 6, 2016

@mikioh mikioh added Documentation and removed Proposal labels Oct 12, 2016

@mikioh mikioh changed the title net: add a new type that represents IP address prefix instead of IPNet net: InterfaceAddrs is not good for multi-homed IP node Oct 12, 2016

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2016

I changed my mind. We can use the combination of Interfaces function and Addrs method of Interface instead of InterfaceAddrs function. Also it returns more useful information than InterfaceAddrs. So just updating the documentation on InterfaceAddrs is enough.

@quentinmit quentinmit added the NeedsFix label Oct 12, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Oct 18, 2016

CL https://golang.org/cl/31371 mentions this issue.

@gopherbot gopherbot closed this in 79c0362 Oct 18, 2016

@golang golang locked and limited conversation to collaborators Oct 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.