-
Notifications
You must be signed in to change notification settings - Fork 45
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
Spawn one ThreadingUDPServer per Interface #23
Conversation
Why do you only listen to the multicast address? That breaks unicast requests, doesn't it? Or is it impossible to bind to a device if you don't specify an address? |
When I bound the sockets to It's certainly possible to detect the link local address on the interface and spawn yet another socketserver for that. Would link-local be sufficient, or is ULA/Global required? |
|
The changes should be sufficient to address the issue. Do you have more feedback? |
I'm not entirely happy with having to bind to every single address, and don't know if there are use cases of querying a non-link-local address, but I'd be fine with merging. |
Listening on link-local only is supposed to be a protection against attacks from outside the mesh. With the BSD Socket API there is little choice between binding anything or just a single address, so I'm not sure where else this would go. |
Shouldn't a single socket bound to any be sufficient? Just join all the interfaces' multicast groups after binding. Filtering for link-local addresses can be done after receiving a packet just fine if desired (it's what Gluon's respondd does). |
@NeoRaider That's what we do now, but that way, you cannot serve different data on different interfaces. |
@jplitza We talked about this on IRC and it might be possible to bind to device with Honestly, I don't like that approach much, binding should be preferred to filtering, even if it uses twice the amount of sockets. What's the cost here? Another issue is, that the bind to e.g. the vpn interface breaks as soon as fastd is restarted. uradvd solves this by monitoring the netlink events and (I guess) rebinding. |
Is this doing any harm, if you only use one interface in your domain? It seems like the version on https://github.com/freifunk-darmstadt/mesh-announce works fine so far. Can this be merged? |
Known Issue: |
With the latest patch mesh-announce will check every 15 seconds all configured interfaces for changed interface ids, which would indicate that the interface vanished and reappeared. In such a case mesh-announce will now stop the threads listening on the vanished interface and create new threads when the interface reappears. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally got around to have a look at this PR. Thanks for the work, @mweinelt! I had a few minor comments.
In general, maybe we also want to rebind if new addresses appear? That's unlikely, but hey…
) | ||
threading.Thread(target=ucast_server.serve_forever).start() | ||
|
||
return socket.if_nametoindex(iface), [mcast_server, ucast_server] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are multiple link-local addresses on the interface, you only return the thread corresponding to the last address.
.format(iface)) | ||
if_threads[iface] = listen( | ||
iface, args.group, args.port, args.directory, env) | ||
except OSError as ex: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This except
covers quite a lot of code (all of listen()
) when really we are only interested in whether socket.if_nametoindex(iface)
has succeeded. Using something like new_ifidx = socket.if_nametoindex(iface)
, you could also save the duplicate code for shutting down the threads.
) | ||
|
||
group_bin = socket.inet_pton(socket.AF_INET6, group) | ||
mreq = group_bin + struct.pack('@I', socket.if_nametoindex(iface)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you start respondd with an interface that doesn't (yet) exist, this call to socket.if_nametoindex
will raise an uncaught exception. But as we (re)bind to (specified) interfaces appearing later on, this could just as well be caught and return an empty thread list.
Before we continue with this we should discuss, whether one instance should serve multiple domains (#28) or if one instance should be started per domain. |
fc25806
to
4440264
Compare
Is this the solution you realized in https://github.com/freifunk-darmstadt/mesh-announce ? could this be rebased then? |
This is that solution, but it breaks with site-local scoped multicast groups, which are required since freifunk-gluon/gluon@59a4427. |
Spawn one ThreadingUDPServer per Interface and bind them to their device scope.
Looks like this: