Sending the question back with the answer #210
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #210 +/- ##
==========================================
+ Coverage 66.22% 68.03% +1.80%
==========================================
Files 1 1
Lines 838 826 -12
==========================================
+ Hits 555 562 +7
+ Misses 206 194 -12
+ Partials 77 70 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@atomirex I am fine with merging this! If you can fix the imports and update the tests the more things we are compatible with the better! Thanks for looking into this. |
|
I will now investigate those test failures. It looks plausibly like the problem motivating further changes on the other fork - because the answer now includes a question some safeguards may be needed in cases of loopback. It's curious the same test locally is fine. |
|
Not being a DNS expert I decided to look into if this is the right thing to do. It turns out the DNS RFC that applies (1035 and following) did not explicitly call out this behaviour (i.e. there isn't a MUST or even SHOULD on the subject), but it was implied, assumed and used in the various examples. The result is apparently most DNS servers do follow this convention, but clients need to handle the case where they don't. So I am going to need to adjust the code here on the client side, including adding tests for both cases (where the response has the Question and where it does not). This should also help smooth over situations where people update code using pion/mdns non atomically as it were. |
atomirex
force-pushed
the
nb-send-question-with-answer
branch
from
0d118e9 to
6f83934
Compare
| @@ -46,4 +46,7 @@ type Config struct { | |||
|
|
|||
| // Interfaces will override the interfaces used for queries and answers. | |||
| Interfaces []net.Interface | |||
|
|
|||
| // Override the default behavior of echoing the query with the answer | |||
| DoNotEchoQueryWithAnswer bool | |||
There was a problem hiding this comment.
Having this in the Config looks ugly to me, however, I couldn't see how to be able to unit test both modes without doing this or a very radical restructuring.
| @@ -885,226 +893,214 @@ func (c *Conn) readLoop(name string, pktConn ipPacketConn, inboundBufferSize int | |||
| } | |||
|
|
|||
| func() { | |||
| header, err := p.Start(b[:n]) | |||
| var msg dnsmessage.Message | |||
| err := msg.Unpack(b[:n]) | |||
There was a problem hiding this comment.
Since Questions are echoed with Answers you have to at least parse the Answer blocks before knowing how to handle the Questions.
atomirex
force-pushed
the
nb-send-question-with-answer
branch
from
df66b75 to
4237681
Compare
For code flow simplicity when handling data that may or may not be structured precisely as expected it's easier to move to unpacking the whole message and handling that as is.
atomirex
force-pushed
the
nb-send-question-with-answer
branch
from
4237681 to
39a9a80
Compare
|
Reopening this for review with some pre-emptive remarks. Firstly, I expected this to be fairly simple, but it proved not to be, and the result is I think this is an awkward halfway house. The underlying problem is there are/were some incorrect assumptions in the existing code:
The way dnsmessage.Parser was used was problematic because it made assumptions about being able to identify how to handle a message while parsing it in order, which is not the case, so it has been replaced by unpacking everything. (My operating assumption here is if that is in any way a hot path then you have other problems). Additionally the testing is becoming difficult. That longer timeout bothers me, and I couldn't establish any reasonable cause for why it is necessary. I verified that it is needed on master too, and it is, so this problem is not introduced by these changes. It would be nice to be able to separate the message response generation logic from any actual network calls for testing, but this would be a more substantial refactoring than I was planning on taking on as a first PR here. This would help eliminate the ugly new Config field, but also enable separating logical execution from problems which may be caused by the local network environment. I have a fork at https://github.com/atomirex/mdns which solves item 3 from the above list, and supports per interface IPs for hosts (I have a use case for this), but in doing so changes the interface. (Curiously that now also has the timeout test problem). It might be worth bringing over more of that code, but it is far less optimised than this has been to date, for example I am not sure this would fit in over here: I am curious for any input as to how to proceed. Thanks! |
Description
This includes the question that provoked the answer in the response containing the answer.
This enables Android clients to resolve hostname.local domains published with this package.
Reference issue
This is necessary for certain mdns clients (the Android dnsresolver, primarily) to pick up the answer to their question. This was established by inspecting what Bonjour was doing on a Mac which allowed it to be recognised. (The Mac will also do things like bundle A and AAAA responses for ipv4 and ipv6, but that didn't prove to be necessary to resolve my problem).
This is more for reference of the bug and change made than a serious request this be merged.