Skip to content

RSDK-10407: Only send raw IP addresses for ICE host candidates. #423

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

dgottlieb
Copy link
Member

@dgottlieb dgottlieb commented Apr 9, 2025

Rather than having clients generate an mdns mapping.

I think this a good change (remove non-default behaviors for things we can't justify) in general. I believe* this will also solve the user problem, but I expect that will need more field testing. Which I think will be easier with an official release.

This patch certainly fixed the symptom where before we'd have multiple candidate pair options:

udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:47022 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:50598 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:51948 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:50677 <-> (remote) udp4 host 192.168.2.4:57604

where each of those hosts (from the client perspective) were actually:

udp4 host 127.0.0.1:47022 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host 192.168.2.1:50598 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host 10.1.8.60:51948 <-> (remote) udp4 host 192.168.2.4:57604
udp4 host 169.254.14.173:50677 <-> (remote) udp4 host 192.168.2.4:57604

But from the "server" perspective, it would get:

udp4 host 192.168.2.4:57604 <-> (remote) udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:47022
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:50598
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:51948
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host aa8f9b3e-ebac-4d2f-89ba-18b1b3b366f5.local:50677

Which would be fine if it resolved that to:

udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 127.0.0.1:47022
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 192.168.2.1:50598
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 10.1.8.60:51948
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 169.254.14.173:50677

But in reality, it's only getting one IP from the mdns resolution and (in the bad case) can be trying:

udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 169.254.14.173:47022
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 169.254.14.173:50598
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 169.254.14.173:51948
udp4 host 192.168.2.4:57604 <-> (remote) udp4 host 169.254.14.173:50677

where the 169 address is (in the bad case) unreachable from the 192 interface.

Rather than having clients generate an mdns mapping.
@dgottlieb dgottlieb requested review from benjirewis and cheukt April 9, 2025 14:50
@viambot viambot added the safe to test Mark as safe to test label Apr 9, 2025
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Apr 9, 2025
@dgottlieb dgottlieb merged commit 387a7b7 into viamrobotics:main Apr 9, 2025
6 checks passed
@dgottlieb dgottlieb deleted the RSDK-10407 branch April 9, 2025 15:16
@benjirewis
Copy link
Member

Apologies for following up so late with questions, but:

it's only getting one IP from the mdns resolution (and in the bad case) can be trying

Does this imply that mDNS resolution is non-determinstic? i.e. we could resolve an mDNS address to multiple, actual IP addresses, and at some point in our or zeroconf's code, we have to select one? And, it can be bad if we select an unreachable one where other possibilities were actually reachable?

where the 169 address is (in the bad case) unreachable from the 192 interface.

Is there significance to IP addresses that begin with 169.254? A quick google search seems to indicate that a 169.254 IP address reported by a client may indicate that that client cannot reach the public internet? Is that what you mean by "unreachable from the 192 interface"?

@dgottlieb
Copy link
Member Author

dgottlieb commented Apr 11, 2025

Does this imply that mDNS resolution is non-determinstic?

Yes, but more generally, all dns resolution where one name maps to more than one IP to map to is non-deterministic. But it's supposed to that all possible IPs to select from will behave the same. There ought to be no "wrong choice".

i.e. we could resolve an mDNS address to multiple, actual IP addresses, and at some point in our or zeroconf's code, we have to select one?

We can experiment maybe with this, but I suspect the bug is more with pion. While we use zeroconf for our own mdns, pion/webrtc has its own code for an mdns server and mdns resolver. And all of these webrtc candidates are managed internally in pion/its libraries' code.

And, it can be bad if we select an unreachable one where other possibilities were actually reachable?

But yes -- that's exactly what I'm claiming here.

Is there significance to IP addresses that begin with 169.254?

Yes, that's another IP range that's considered "link local". I don't really know what link local means. I'm categorizing it as a "local network". That's probably an oversimplification.

A quick google search seems to indicate that a 169.254 IP address reported by a client may indicate that that client cannot reach the public internet?

I think that's going a bit down the wrong path. Having a 192.168. IP address means one is connected to a router. But it doesn't necessarily mean the internet is necessarily working on that router.

Is that what you mean by "unreachable from the 192 interface"?

But yes, I'm treating that 169.254... as just a different interface that can not be routed to from the 192.168... address.

@benjirewis
Copy link
Member

That all makes sense to me; thanks for following up here. So we're just turning off clients doing pion-gathering for mDNS.

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

Successfully merging this pull request may close these issues.

4 participants