-
Notifications
You must be signed in to change notification settings - Fork 93
feat: mdns #260
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
feat: mdns #260
Conversation
@jxs can you help review this? (Took liberty of unassigning Marten to help spread the load.) |
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.
It would probably be useful if you could run a kubo node and inspect the mDNS query and response using wireshark, and add (the relevant parts) to this document.
@p-shahi Thank you, but I had already started my review, so I'm posting it anyway. |
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.
There seems to be some misunderstanding of how mDNS works and what we're trying to achieve.
@DannyS03, have you tried Wiresharking the mDNS traffic originating from an IPFS node, and inspecting the packets? I think this could be really instructive. I also think (as I mentioned in my last review) that we should add some of that Wireshark output here as examples.
| No. | Time | Source IP | Destination IP | Protocol | Length | Info | | ||
|-----|------------|--------------------------|------------------------|----------|--------|---------------------------| | ||
| 1 | 0.000000 | 10.0.0.25 | 224.0.0.251 | MDNS | 75 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 2 | 0.000130 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 95 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 3 | 0.002497 | 10.0.0.25 | 224.0.0.251 | MDNS | 918 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | | ||
| 4 | 0.002507 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 938 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | |
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 doesn't show the multiaddr, does it?
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 we're going to show this, we may as well show the multiaddr result of the query.
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.
Yes, that was my motivation when I asked for including a Wireshark capture.
Maybe this could even be a screenshot of Wireshark? No strong opinion in either direction though.
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.
Let's not include this? Why have wireshark output for mDNS and not for other protocols?
I'm in favor of removing this and the following paragraphs.
Instead we can describe the peer discover section https://github.com/libp2p/specs/blob/master/discovery/mdns.md#peer-discovery in two sentences and point to the spec
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.
@MarcoPolo @marten-seemann please re-review. I removed the wireshark section and instead linked to the specs
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.
Tapping marten out, and I'll review from here on out.
| No. | Time | Source IP | Destination IP | Protocol | Length | Info | | ||
|-----|------------|--------------------------|------------------------|----------|--------|---------------------------| | ||
| 1 | 0.000000 | 10.0.0.25 | 224.0.0.251 | MDNS | 75 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 2 | 0.000130 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 95 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 3 | 0.002497 | 10.0.0.25 | 224.0.0.251 | MDNS | 918 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | | ||
| 4 | 0.002507 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 938 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | |
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 we're going to show this, we may as well show the multiaddr result of the query.
| No. | Time | Source IP | Destination IP | Protocol | Length | Info | | ||
|-----|------------|--------------------------|------------------------|----------|--------|---------------------------| | ||
| 1 | 0.000000 | 10.0.0.25 | 224.0.0.251 | MDNS | 75 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 2 | 0.000130 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 95 | Standard query 0x7d14 PTR _p2p._udp.local, "QM" question | | ||
| 3 | 0.002497 | 10.0.0.25 | 224.0.0.251 | MDNS | 918 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | | ||
| 4 | 0.002507 | fe80::8d9:f02f:717f:2c03 | ff02::fb | MDNS | 938 | Standard query response 0x7d14 PTR vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn._p2p._udp.local SRV 0 0 4001 vmvokx8c0b7rcjtx4e14iyaxw7otg05xyg954y5fnh1a9rderxtxycicgtj3nn.local TXT A 127.0.0.1 AAAA ::1 | |
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.
Let's not include this? Why have wireshark output for mDNS and not for other protocols?
I'm in favor of removing this and the following paragraphs.
Instead we can describe the peer discover section https://github.com/libp2p/specs/blob/master/discovery/mdns.md#peer-discovery in two sentences and point to the spec
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.
@p-shahi could you please give this a once over before merging. ty!
Context
Notes
Latest preview
Please view the latest Fleek preview here. (preview outdated as not against master anymore)