Repository navigation
aggregation: browse mDNS from port 5353 so peers answer by multicast - #720
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression test can silently skip, and the new fallback branch lacks deterministic coverage.
2 open findings
What changed in this PR
Updates mDNS peer discovery to request multicast replies through UDP port 5353, with an ephemeral-port fallback.
Changes:
- Binds browse sockets to port 5353 and improves logging.
- Adds a test that inspects the query’s QU bit.
| File | Description |
|---|---|
src/ros2_medkit_gateway/src/core/aggregation/mdns_discovery.cpp |
Adds port 5353 binding and fallback behavior. |
src/ros2_medkit_gateway/test/test_mdns_discovery.cpp |
Adds multicast-query regression coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bburda
reviewed
Oct 24, 2026
bburda
left a comment
bburda
left a comment
Collaborator
There was a problem hiding this comment.
Could you add to the Troubleshooting part of docs/tutorials/multi-instance.rst that discovery now gets its replies on udp/5353, so a firewall that allows the mdns service is enough?
From an ephemeral port the PTR query sets the QU bit and peers answer by unicast to that port, which a host firewall allowing only the mdns service drops. Bind the browse socket to 5353 (shared with announce and avahi), fall back to an ephemeral port when 5353 is unavailable.
Bound to 224.0.0.251:5353 the browse socket gets only multicast, so unicast to 5353 stays with the announce socket and avahi. Queries are dropped before parsing, and SRV records outside our service are ignored. Tests assert instead of skipping and cover the ephemeral-port fallback.
A firewall that allows the mdns service is enough. Also describe the ephemeral-port fallback and its log line.
mfaferek93
force-pushed
the
fix/mdns-browse-from-5353
branch
from
October 25, 2026 18:28
2eb5a99 to
cb6e7e2
Compare
bburda
approved these changes
Oct 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
browse_loopopened its query socket on an ephemeral port. The vendored mdns library then sets the QU (unicast-response) bit on the PTR query, so every peer answers by unicast to that ephemeral port. A host firewall that allows only themdnsservice (udp/5353), which is the usual setup on an appliance, drops those replies. Discovery never finds a peer, with no error.Fix
Bind the browse socket to 5353. The library clears the QU bit for a socket on 5353, peers answer by multicast to 5353, and the standard
mdnsfirewall rule lets it in. The socket shares 5353 with our announce socket and with avahi throughSO_REUSEADDR/SO_REUSEPORT, whichmdns_socket_open_ipv4already sets. If 5353 cannot be bound, it falls back to an ephemeral port as before and says so in the log.Tests
MdnsDiscovery.browse_query_asks_for_multicast_replies: listens on 5353, starts discovery and checks the PTR query has the QU bit clear. Skips when 5353 or multicast loopback is not available. Fails without the fix, passes with it.test_mdns_discovery(24),test_aggregation_manager(72),test_aggregation_classification(10) pass.On real hardware: a RevPi (Debian bookworm, firewalld public zone
ssh http https mdns, avahi running) and a robot gateway announcing on the same LAN.jetson,driver_board,lidaraspeer:m3pro-yahboom). No extra firewall rule.