Adds support for handling interface changes to mdns behaviour. #1830

dvc94ch · 2020-11-09T18:57:00Z

Currently the mdns behaviour will panic if no adapter is available. To test on linux use the following commands:

sudo ifconfig wlan0 down
cargo run --example mdns-passive-discovery
sudo ifconfig wlan0 up

~~Depends on paritytech/ip-watch#1~~

cc @Demi-Marie @rklaehn

dvc94ch · 2020-11-11T21:10:07Z

Updated ip-watch to work on windows too and tested that it handles changing networks.

protocols/mdns/src/service.rs

dvc94ch · 2020-11-17T20:56:49Z

Just noticed an issue, it returns routes on linux but interfaces on windows. Looks like @Demi-Marie intended to query routes? Unless there is a reason for that, I'll change the linux implementation of if-watch to query ip's. This means that the prefix is 32 on linux and the network prefix on windows.

romanb

Just noticed an issue, it returns routes on linux but interfaces on windows. Looks like @Demi-Marie intended to query routes? Unless there is a reason for that, I'll change the linux implementation of if-watch to query ip's. This means that the prefix is 32 on linux and the network prefix on windows.

Since I'm neither familiar with the details of ip-watch nor if-watch, could you elaborate on the observed difference and the changes you made? I'm generally just assuming that these libraries "do the right thing" for the purpose of network interface monitoring as desired for MDNS.

protocols/mdns/src/service.rs

romanb · 2020-11-19T09:51:01Z

protocols/mdns/src/service.rs

+                        }
+                        if let IpAddr::V4(addr) = inet.addr() {
+                            log::trace!("leaving multicast on iface {}", addr);
+                            if let Err(err) = socket.leave_multicast_v4(&multicast, &addr) {


As mentioned in an earlier comment, I think it would be good to get clarity on whether doing this on an interface that is down makes any sense. If this is just a no-op in this situation, there is no reason to do it and if it is an error it causes unnecessary noise in the logs. If you're sure that doing this on an interface that is down is useful and only errors when there is an error other than the interface being down, I'm fine with it.

Well it doesn't cause an error on linux or windows. I'll try to investigate more deeply especially on linux, but we are supporting any platform that rust compiles to I guess.

I'll try to investigate more deeply especially on linux, [..]

Just curious, did you come to a conclusion on the matter?

romanb · 2020-11-19T09:58:50Z

@mxinden If you could also take a quick look at these changes, since you've worked on this code before, it would be much appreciated.

dvc94ch · 2020-11-19T11:06:26Z

They do the right thing now. I switched the rtnetlink code to use RTM_NEWADDR, RTM_DELADDR, RTM_GETADDR instead of RTM_NEWROUTE, RTM_DELROUTE, RTM_GETROUTE. The result is now the same as running ip address on linux.

https://man7.org/linux/man-pages/man7/rtnetlink.7.html

romanb · 2020-11-19T15:12:08Z

They do the right thing now. I switched the rtnetlink code to use RTM_NEWADDR, RTM_DELADDR, RTM_GETADDR instead of RTM_NEWROUTE, RTM_DELROUTE, RTM_GETROUTE. The result is now the same as running ip address on linux.

Thanks for the link. There are some things about the implementation of if-watch that confuse me a bit:

It is sending an RTM_GETADDR request with an rtmsg structure, which according to the man pages is only applicable to RTM_NEWROUTE, RTM_DELROUTE and RTM_GETROUTE types. The expected payload for RTM_GETADDR et al on the other hand according to the manpage is an ifaddrmsg structure. I guess the rtmsg is simply ignored when sent with an RTM_GETADDR request?
I only see send_getaddr() being called once when initialising the watcher. How will the watcher ever receive further updates? The previous use of RTM_GETROUTE may have relied upon the RTM_F_NOTIFY flag, but that does not seem applicable to RTM_GETADDR because it is a flag for the rtmsg structure.
Would RTM_GETLINK not be the most appropriate to list network interfaces? Of course, it may have the same problem as RTM_GETADDR in that it needs to be sent periodically, as far as I currently understand.

This is the first time I'm looking at rtnetlink, so I may be missing the obvious.

dvc94ch · 2020-11-19T15:18:54Z

It only needs to send_getaddr at the beginning to get the current interfaces. We subscribe to notifications by setting the nl_groups to RTMGRP_IPV4_IFADDR and RTMGRP_IPV6_IFADDR. I'm not sure about the rtmsg as I didn't find any documentation on it. The behaviour I'm observing is that it ignores the fields, but from looking at examples written in C, it needs to send at least the family as a payload. As the first element is a family of size u8, I'm not sure if the rest is just zeroes as common practice in C or if it uses a different struct.

RTM_GETLINK doesn't do what we want, it'll return mac addresses.

EDIT: you're right, the correct thing to do is send an ifaddrmsg, updated master with a fix.

mxinden

The direct changes in this pull request look good to me. Thanks for making libp2p-mdns more robust. On the side I am very much in favor of getting rid of the async-std/tokio macros.

I want to highlight once more that this switches libp2p-mdns from previously using either tokio or async-std to async-io wrapping an std udp socket. In case there is anyone objecting to this change, please raise your concerns here.

I still have to take a deeper look at the introduced if-watch dependency.

dvc94ch · 2020-11-20T13:33:30Z

Some maintainance stuff that might be good to do before merging is replacing the old net2 crate with socket2 as used in the tcp transport and remove the wasm-time dependency in favor of async-io, as mdns doesn't run in browsers anyway.

smol-rs/async-io#41

dvc94ch · 2020-11-30T14:13:25Z

Well if-watch was written for libp2p, in particular for mdns and tcp/quic transports, so it is the first crate. I can add an interval based solution for macos until someone adds proper support. The windows implementation isn't that great either, someone more into windows could probably improve that too.

romanb · 2020-11-30T14:30:36Z

The diff itself looks good to me, but as @mxinden said, even if we don't have dedicated tests or CI checks for it at the moment, in principle rust-libp2p certainly has the ambition to support MacOS, hence including a library that is known to lack support is not really an option. Whenever we get tickets here that rust-libp2p does not compile for some platform, especially when it is as common as MacOS, we necessarily need to look into it. Without access to the right system for testing (I don't have or use MacOS either) we still need to make a best-effort and deal with issues in collaboration with users that do use these platforms as they come up. So we need an approach here (via if-watch or otherwise) that is at least intended to work on MacOS as well, even if it may turn out to have issues that need to be resolved afterwards.

dvc94ch · 2020-11-30T14:52:50Z

Fixed in 0.1.6, now has a fallback that polls in 10s intervals. Also has ci that tests on linux/windows/macos and builds on android/ios

romanb · 2020-11-30T15:52:58Z

@dvc94ch You don't need to keep this branch mergeable, e.g. by merging master and resolving conflicts. We typically resolve conflicts and update versions and changelogs just before, respectively after merging the PR, after approval. The only thing that is advisable is to eventually stop rebasing / force-pushing to PR branches, in order not to cause unnecessary problems when we want to make final adjustments to the branch before merging.

mxinden · 2020-12-01T13:11:50Z

Thanks for adding a fallback mode @dvc94ch.

@romanb feel free to go ahead with merging.

romanb · 2020-12-02T15:40:17Z

@mxinden Thanks for your feedback. I intend to merge and probably release a new version of libp2p-mdns tomorrow.

dvc94ch · 2020-12-02T18:59:38Z

any thoughts on the tcp pr so far? I didn't find a good way to spawn the if_task in an executor agnostik way, the tokio and async-std people do not want any solution that requires more than one allocation in total and has been accepted in an rfc process [0], so I'm just spawning a thread.

[0] executor independent spawning async-rs/async-std#912

romanb · 2020-12-03T11:16:43Z

any thoughts on the tcp pr so far?

I didn't get a chance to look at it yet in any detail but intend to do so by next week.

eskimor · 2020-12-07T13:45:02Z

Thanks @dvc94ch ! Looks really awesome, especially that we react to interface changes. One thing that struck me when skimming through the changes: Just joining the multicast group on all interfaces is enough to make sure, messages go out on each interface?! I mean it kinda makes sense, but my research indicated otherwise. Anyhow, I really like that PR!

dvc94ch · 2020-12-07T13:49:00Z

Can you elaborate on what your research indicates is required? It has worked for me on linux and windows using the following test procedure:

start the mdns example on linux and on windows
wait for both to discover each other
change network to mobile access point on both devices
wait for both to discover each other

I don't know about multiple interfaces

eskimor · 2020-12-07T13:59:18Z

I see, different use case! In my case I really had multiple interfaces at the same time and according to some man page (would have to dig that one up again), when binding via multicast address or 0.0.0.0 you start receiving on all interfaces, but outgoing messages will be transmitted to the default interface (which is usually the one with the default gw with the highest priority) - in any case that matched my experience as well so far.

I will retry with your implementation and will report any issues :-)

eskimor · 2020-12-07T14:08:32Z

In any case one can change the outgoing interface, so in my implementation I just iterated over all interfaces, changed the outgoing interface to the current one and sent out the message on each one.

https://github.com/libp2p/rust-libp2p/pull/1750/files#diff-d82ec3ce9fb38291708105fbda3b4caeb5aa4d2d44a59b27c1e5a5679f9b9badR235

eskimor · 2020-12-07T14:10:23Z

Actually memory served me wrong. There is a platform specific call for sending, which allows specifying the interface that should be used.

dvc94ch · 2020-12-07T14:16:38Z

so it doesn't work? can you open a PR for it, I assume the changes should be pretty small now?

eskimor · 2020-12-07T14:22:37Z

Well, considering that you have not tested that particular case, I am pretty sure it won't, but I will test it and will provide a PR if that assumption is correct. The changes should be rather minimal. The only issue is that it depends on platform specific functionality, so this complicates matters. MacOS might be fine, but I have no idea about Windows.

dvc94ch · 2020-12-07T14:29:27Z

My devices only have a wifi adapter. I can help test that it doesn't break anything. Feel free to cc me on the PR.

eskimor · 2020-12-07T14:29:49Z

Thanks @dvc94ch !

dvc94ch force-pushed the mdns-ip-watch branch 2 times, most recently from 62ce8d4 to 014ea9e Compare November 9, 2020 19:57

dvc94ch force-pushed the mdns-ip-watch branch 3 times, most recently from 28adc9a to e015f6d Compare November 13, 2020 23:56

dvc94ch mentioned this pull request Nov 17, 2020

Tcp #1845

Closed

dvc94ch commented Nov 17, 2020

View reviewed changes

protocols/mdns/src/service.rs Show resolved Hide resolved

protocols/mdns/src/service.rs Outdated Show resolved Hide resolved

rkuhn reviewed Nov 17, 2020

View reviewed changes

protocols/mdns/src/service.rs Show resolved Hide resolved

dvc94ch force-pushed the mdns-ip-watch branch from e015f6d to b7f33f6 Compare November 17, 2020 15:35

romanb reviewed Nov 17, 2020

View reviewed changes

protocols/mdns/src/service.rs Outdated Show resolved Hide resolved

protocols/mdns/src/service.rs Outdated Show resolved Hide resolved

protocols/mdns/src/service.rs Outdated Show resolved Hide resolved

dvc94ch force-pushed the mdns-ip-watch branch from 2938a06 to 36c997c Compare November 18, 2020 15:19

dvc94ch requested a review from romanb November 18, 2020 20:18

romanb reviewed Nov 19, 2020

View reviewed changes

romanb linked an issue Nov 19, 2020 that may be closed by this pull request

mdns service operates on only one interface #1518

Closed

mxinden reviewed Nov 20, 2020

View reviewed changes

dvc94ch added 8 commits November 28, 2020 21:09

mdns: handle address changes.

a03cb96

Update examples.

383a872

Use async-io.

f348140

Fix tokio-chat.

19cdb86

Address review comments.

32f106d

Update if-watch.

084319c

Poll interfaces correctly.

1988a63

Use socket2 and remove wasm-time.

6c074a9

Update if-watch.

86a954c

romanb approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into mdns-ip-watch

1c5a283

romanb and others added 3 commits December 3, 2020 12:42

Merge branch 'master' into mdns-ip-watch

7d38786

Update versions and changelogs.

3d8753d

Further changelog updates.

c2c23b2

romanb merged commit 505a17d into libp2p:master Dec 3, 2020

romanb mentioned this pull request Dec 7, 2020

WIP: Join multicast group on all interfaces #1750

Closed

This was referenced Dec 8, 2020

*: Update to libp2p v0.32.0 paritytech/substrate#7696

Merged

Replace get_if_addrs with a pure Rust library #1489

Closed

eskimor mentioned this pull request Dec 27, 2020

MDNS: Support multiple simultaneous interfaces #1905

Closed

mxinden mentioned this pull request Dec 27, 2020

Node panicking paritytech/substrate#7794

Closed

romanb mentioned this pull request Jan 26, 2021

The MdnsService fails to start if no network is available #789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for handling interface changes to mdns behaviour. #1830

Adds support for handling interface changes to mdns behaviour. #1830

dvc94ch commented Nov 9, 2020 •

edited

Loading

dvc94ch commented Nov 11, 2020

dvc94ch commented Nov 17, 2020 •

edited

Loading

romanb left a comment

romanb Nov 19, 2020 •

edited

Loading

dvc94ch Nov 19, 2020

romanb Nov 30, 2020

romanb commented Nov 19, 2020

dvc94ch commented Nov 19, 2020

romanb commented Nov 19, 2020

dvc94ch commented Nov 19, 2020 •

edited

Loading

mxinden left a comment

dvc94ch commented Nov 20, 2020 •

edited

Loading

dvc94ch commented Nov 30, 2020

romanb commented Nov 30, 2020

dvc94ch commented Nov 30, 2020 •

edited

Loading

romanb commented Nov 30, 2020 •

edited

Loading

mxinden commented Dec 1, 2020

romanb commented Dec 2, 2020

dvc94ch commented Dec 2, 2020

romanb commented Dec 3, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020 •

edited

Loading

eskimor commented Dec 7, 2020

eskimor commented Dec 7, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020

eskimor commented Dec 7, 2020

Adds support for handling interface changes to mdns behaviour. #1830

Adds support for handling interface changes to mdns behaviour. #1830

Conversation

dvc94ch commented Nov 9, 2020 • edited Loading

dvc94ch commented Nov 11, 2020

dvc94ch commented Nov 17, 2020 • edited Loading

romanb left a comment

Choose a reason for hiding this comment

romanb Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

dvc94ch Nov 19, 2020

Choose a reason for hiding this comment

romanb Nov 30, 2020

Choose a reason for hiding this comment

romanb commented Nov 19, 2020

dvc94ch commented Nov 19, 2020

romanb commented Nov 19, 2020

dvc94ch commented Nov 19, 2020 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

dvc94ch commented Nov 20, 2020 • edited Loading

dvc94ch commented Nov 30, 2020

romanb commented Nov 30, 2020

dvc94ch commented Nov 30, 2020 • edited Loading

romanb commented Nov 30, 2020 • edited Loading

mxinden commented Dec 1, 2020

romanb commented Dec 2, 2020

dvc94ch commented Dec 2, 2020

romanb commented Dec 3, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020 • edited Loading

eskimor commented Dec 7, 2020

eskimor commented Dec 7, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Dec 7, 2020

eskimor commented Dec 7, 2020

dvc94ch commented Nov 9, 2020 •

edited

Loading

dvc94ch commented Nov 17, 2020 •

edited

Loading

romanb Nov 19, 2020 •

edited

Loading

dvc94ch commented Nov 19, 2020 •

edited

Loading

dvc94ch commented Nov 20, 2020 •

edited

Loading

dvc94ch commented Nov 30, 2020 •

edited

Loading

romanb commented Nov 30, 2020 •

edited

Loading

dvc94ch commented Dec 7, 2020 •

edited

Loading