Bind to correct interface by specifying zone index#1
Bind to correct interface by specifying zone index#1antoninbas merged 1 commit intoantrea-io:mainfrom
Conversation
xliuxu
force-pushed
the
xliu2/allow-addr-with-zone
branch
from
8cf5475 to
1013127
Compare
antoninbas
left a comment
antoninbas
left a comment
There was a problem hiding this comment.
IMO, a simple alternative would be to add a ListenAddress function that skips address resolution (see comment)
| // If a preferred address is set, use it directly. | ||
| if preferred.IsValid() { | ||
| if preferred.Zone() == "" { | ||
| return preferred.WithZone(zone) | ||
| } | ||
| return preferred | ||
| } |
There was a problem hiding this comment.
this makes the function quite confusing. Why even bother to call match if we are going to do this? Wouldn't the "default" case (return ip.WithZone(zone)) just work in this case?
|
|
||
| // chooseAddr selects an Addr from the interface based on the specified Addr type. | ||
| func chooseAddr(addrs []net.Addr, zone string, addr Addr) (netip.Addr, error) { | ||
| func chooseAddr(addrs []net.Addr, zone string, zoneIndex int, addr Addr) (netip.Addr, error) { |
There was a problem hiding this comment.
I feel like the code is a bit more complicated than it needs to be. Could we do the following:
- assume that
addrdoes not include a zone information (or ignore it?) as whenListenis called, we are already working with a specificnet.Interface - remove
zoneand only keepzoneIndex - when returning the final address (return value of type
netip.Addr), always set the zone tozoneIndex
The current code feels like it's a bit too complex and was tweaked for a specific use case
There was a problem hiding this comment.
The motivation for this change is to adjust the library to support our specific use case while preserving the original behavior.
I am not very sure whether setting the zone to 'index' regardless of other factors could have side effects or not.
There was a problem hiding this comment.
There is a point to be made that the rest of the library is consistently using the interface name as the zone, so you're right that may not be the best approach.
But the current solution is a bit too hacky IMO.
| } | ||
|
|
||
| ip, err := chooseAddr(addrs, ifi.Name, addr) | ||
| ip, err := chooseAddr(addrs, ifi.Name, ifi.Index, addr) |
There was a problem hiding this comment.
honestly, an alternative to this patch would be to just have a new function ListenAddr(address string) that directly calls icmp.ListenPacket("ip6:ipv6-icmp", address)
it seems that in our case (by which I mean your PR antrea-io/antrea#6700) we already know the address and zone, and we don't want to rely on this library to do the address resolution
There was a problem hiding this comment.
This is definitely better. I will update the PR.
There was a problem hiding this comment.
revisited this. The ndp.Conn still depends on net.Interface. Should we retrieve the interface from the address in ListenAddr(address string)?
There was a problem hiding this comment.
After thinking about it more, the big issue IMO comes from the fact that we are trying to use the interface index as zone, when the library uses the interface name consistently. This is unfortunate since we require interface index in our case and interface index may be more portable.
I see 2 options:
- update the library to use zone index everywhere, which is not such a big change
- only use the zone index when calling
icmp.ListenPacket
For option 2, it is a one-line change:
ic, err := icmp.ListenPacket("ip6:ipv6-icmp", ip.WithZone(ifi.Index).String())Unless I am misunderstanding the issue, this should take care of it ^
Signed-off-by: Xu Liu <xu.liu@broadcom.com>
xliuxu
force-pushed
the
xliu2/allow-addr-with-zone
branch
from
1013127 to
4b1e72c
Compare
xliuxu
changed the title
|
Closing and reopening to trigger workflows |
|
Thanks for the review! I will update antrea-io/antrea#6700 |
2 participants
Mirror of upstream change: mdlayher#32