Fix broken DNS lookups for responses with long TXT records or multiple TXT records#561
Conversation
|
Note: the tests already relied on public dns of redhat.com, and this test adds google.com, and gmail.com to that set. It might be better to refactor pkg/service/dns to be able to pass in an alternate net.Resolver and remove those live dns dependencies from the tests. But i didn't take that on as it would have made for more changes than actual bug fix, so it deserves it's own commit. Also notiing, I found my way here via using docker on colima, to managing an smtp server doing dkim record lookups. colima, using lima, using gvisor-tap-vsock |
abd5d7c to
bce3c18
Compare
looks like go-mockdns will make that doable |
done.. pushed update. |
|
Thanks for the fixes/explanations! the mockdns changes are nice :) We have control over the Could you use a real name for the /lgtm |
This commit addresses two distinct issues in the same part of the code
1. Long TXT records:
DNS lookups of long TXT records were timing out and logging
"string exceeded 255 bytes in txt".
While (net.Resolver).lookupTXT concatenates multiple underlying strings
into a single string per TXT RR, the (dns.TXT).Txt field (miekg/dns)
requires a slice of strings, each 255 bytes or less, for correct packing.
The fix ensures the value from lookupTXT is split into a slice of
appropriately sized strings before being assigned to (dns.TXT).Txt,
preventing the eventual error from dns.packTxtString
"string exceeded 255 bytes in txt"
2. Multiple TXT records:
Multiple TXT records were being concatenated together into one.
Each string returned by (net.Resolver).lookupTXT represents a separate
resource record. The old code code was adding each of them to the same
resource record as a new character string.
we now correctly create a new resource record for each string
Signed-off-by: John "fess" Fessenden <fess-Z3Zpc29yCg@fess.org>
to be used ins dns_test.go Signed-off-by: John "fess" Fessenden <fess-Z3Zpc29yCg@fess.org>
no longer relying on external dns for the tests Signed-off-by: John "fess" Fessenden <fess-Z3Zpc29yCg@fess.org>
83689a9 to
ef911fa
Compare
updated. |
putting DNS fixtures into public DNS controlled by the project would work. go-mockdns reduces dependency on the network in general, and allows new tests to be added without coordinating the new public fixtures, or coming up with a way to mock in them before getting new public fixtures. ( Which, admittedly wasn't that hard in this case I just used a live example. ) I think it's a toss up, either way is ok. I slightly prefer the mockdns approach for less dependency on the external network in general. LMK how you want to proceed. |
|
/retest |
Not sure why the DCO check complains, I’ll force it to "pass".
The mockdns approach is more flexible, and is done already, let’s go this way, we can always switch back to the other option later if we realize mockdns no longer fits our needs. /lgtm |
There’s actually an explanation,the check is being very strict in its expectations:
I’ve told it it’s fine. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau, fessyfoo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Any feedback is accepted and appreciated.
This addresses two distinct issues in the same part of the code
DNS lookups of long TXT records were timing out and logging "string exceeded 255 bytes in txt".
While (net.Resolver).lookupTXT concatenates multiple underlying strings into a single string per TXT RR, the (dns.TXT).Txt field (miekg/dns) requires a slice of strings, each 255 bytes or less, for correct packing.
The fix ensures the value from lookupTXT is split into a slice of appropriately sized strings before being assigned to (dns.TXT).Txt, preventing the eventual error from dns.packTxtString
Multiple TXT records were being concatenated together into one.
Each string returned by (net.Resolver).lookupTXT represents a separate resource record. The old code code was adding each of them to the same resource record as a new character string.
we now correctly create a new resource record for each string
Fixes #562
Fixes #563