Repository navigation
aggregation: browse mDNS from port 5353 so peers answer by multicast - #720
Open
mfaferek93 wants to merge 1 commit into
Open
mfaferek93 wants to merge 1 commit into
mfaferek93 wants to merge 1 commit into
Conversation
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.
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.
Comment on lines
+527
to
+529
| if (sock < 0) { | ||
| on_mdns_port = false; | ||
| sock = mdns_socket_open_ipv4(nullptr); |
Comment on lines
+445
to
+446
| if (listener < 0) { | ||
| GTEST_SKIP() << "cannot bind udp/5353 here"; |
This branch has not been deployed
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.