Replace dns "resolver" calls with "dns.Exchange" func#339
Replace dns "resolver" calls with "dns.Exchange" func#339openshift-merge-bot[bot] merged 6 commits intocontainers:mainfrom
Conversation
cfergeau
left a comment
There was a problem hiding this comment.
Haven't looked yet at the 3rd commit, the amount of code needed for Windows is a bit scary :-/
Very happy to see all this code gone in the 2nd commit though ^^
pkg/services/dns/dns.go
Outdated
| resolver := net.Resolver{ | ||
| PreferGo: false, | ||
| // need to create new message struct, as reusing original message struct leading | ||
| // to request errors |
There was a problem hiding this comment.
I was quite confused by this for a long while, imo this is a good opportunity to rework handle and addAnswers .
The reason the copy is needed is because r is a "reply" message, sending it as a "query" message with Exchange won't work as expected. The copy you do will clear the m.Response boolean which is set to true.
Looking at handle, we have:
func (h *dnsHandler) handle(w dns.ResponseWriter, r *dns.Msg, responseMessageSize int) {
m := new(dns.Msg)
m.SetReply(r)
m.RecursionAvailable = true
h.addAnswers(m)
It gets a "query" message in the r parameter, it creates a "reply" message as m, and passes it to addAnswers so that m.Answers gets filled, and then the m "reply" is sent to back.
However, addAnswers does not limit itself to filling the Answers of the "reply" message. It used to reuse the Query field from that message to generate some go DNS lookups, and now with your changes, we need a full dns.Msg instance to forward to the system DNS server as a "query". So we have a "reply" message which we try to turn into a "query" message, which you did through this struct copying.
In my opinion, it would be cleaner to forward the initial request in Exchange, ie the r argument to the handle method, and to keep m strictly as a "reply" message.
I'd also argue that a lot of the confusion comes from the one-letter variables, r, m, ... :-/
I've done some work towards this in https://github.com/cfergeau/gvisor-tap-vsock/tree/replace-dns
This includes a new test which is useful to test this code.
If you agree with the approach, feel free to take this code over and rework it in the way you like, the code in my branch is not final, just a way for me to check this can work :)
There was a problem hiding this comment.
I have add your code in this PR
5579f34 to
bcea6b1
Compare
b7bf0a7 to
f2ae1d9
Compare
|
(note for myself) I was wondering about the 'no ipv6/AAAA support' mention in the commit log, it's a preexisting issue, support for AAAA requests is only in the ipv6 PR 964d113 |
| ) | ||
|
|
||
| func GetDNSHostAndPort() (string, string, error) { | ||
| conf, err := dns.ClientConfigFromFile("/etc/resolv.conf") |
There was a problem hiding this comment.
It might be better to in future specify the location of this file as systems are moving away from using this; eg. systemd-resolvd
There was a problem hiding this comment.
For now, systemd-resolved provides a stub resolv.conf with a DNS resolver listening on 127.0.0.53
|
|
||
| var nameserver netip.AddrPort | ||
| for _, n := range nameservers { | ||
| // return first non ipv6 nameserver |
There was a problem hiding this comment.
same 'issue' (limitation) here? only a single nameserver is handled
gbraad
left a comment
There was a problem hiding this comment.
Overall looks OK, especially as testcases are added.
However, there are limitations in how multiple nameservers are handled; this needs to be a follow-up.
… has multiple records Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This adds a new test which sets up a DNS server using gvisor-tap-vsock code listening on localhost tcp and udp, and then compare its answers with the one from the default go resolver. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This is the beginning of a refactor of addAnswers to make it easier to understand. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
It would be used to get DNS configuration on windows, as it requires to deals with syscall to windows kernel Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
|
I did a few changes in https://github.com/cfergeau/gvisor-tap-vsock/commits/replace-dns/ , but overall it looks good to me! Changes I did:
|
CRC relies on the |
|
@cfergeau @gbraad I add 'hosts' file records handling in evidolob#2 |
Looks like a good way forward! Let's merge this PR which has waited for far too long, and then we'll add |
This highly simplify resolving DNS code. Also, DNS will work only for IPv4 Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com> Co-authored-by: Christophe Fergeau <cfergeau@redhat.com> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
At the moment, the forwarded DNS requests all go over TCP. This commit uses 2 different dns clients so that we can forward UDP requests over UDP instead of TCP. This removes FIXMEs in the code. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
b56d081 to
be5257a
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau, evidolob 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 |
|
win-sshproxy-tests failing is unfortunately expected :-/ |
Due to reverting PR containers#339 thous files is not used Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Due to reverting PR containers#339 thous files is not used Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Since containers#339 was reverted, the filewatcher functionality is no longer used. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
This PR is trying to simplify DNS server implementation, by replacing parsing query message and pass it to different
resolver.*function calls with singledns.Exchange()function.The downside of this, is getting DNS configuration on Windows is quite difficult, as it is required syscall.