Replace dns "resolver" calls with "dns.Exchange" func #339

evidolob · 2024-03-27T09:48:30Z

This PR is trying to simplify DNS server implementation, by replacing parsing query message and pass it to different resolver.* function calls with single dns.Exchange() function.

The downside of this, is getting DNS configuration on Windows is quite difficult, as it is required syscall.

cfergeau

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

cfergeau · 2024-04-18T16:04:38Z

pkg/services/dns/dns.go

-		resolver := net.Resolver{
-			PreferGo: false,
+		// need to create new message struct, as reusing original message struct leading
+		// to request errors


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 :)

I have add your code in this PR

pkg/services/dns/dns.go

pkg/services/dns/dns_config_windows.go

cfergeau · 2024-08-27T16:50:53Z

(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

pkg/services/dns/dns_config_unix.go

gbraad · 2024-08-28T05:27:13Z

pkg/services/dns/dns_config_unix.go

+)
+
+func GetDNSHostAndPort() (string, string, error) {
+	conf, err := dns.ClientConfigFromFile("/etc/resolv.conf")


It might be better to in future specify the location of this file as systems are moving away from using this; eg. systemd-resolvd

For now, systemd-resolved provides a stub resolv.conf with a DNS resolver listening on 127.0.0.53

gbraad · 2024-08-28T05:28:45Z

pkg/services/dns/dns_config_windows.go

+
+	var nameserver netip.AddrPort
+	for _, n := range nameservers {
+		// return first non ipv6 nameserver


same 'issue' (limitation) here? only a single nameserver is handled

gbraad

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 <[email protected]> Signed-off-by: Christophe Fergeau <[email protected]>

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 <[email protected]>

This is the beginning of a refactor of addAnswers to make it easier to understand. Signed-off-by: Christophe Fergeau <[email protected]>

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 <[email protected]> Signed-off-by: Christophe Fergeau <[email protected]>

cfergeau · 2024-08-28T13:22:16Z

I did a few changes in https://github.com/cfergeau/gvisor-tap-vsock/commits/replace-dns/ , but overall it looks good to me!
There is a possibly significant change with this approach though. Before, if I had 1.2.3.4 some.host.example.com in /etc/hosts on my host machine, then the guest would resolve some.host.example.com to 1.2.3.4.
After these changes, /etc/hosts will no longer be considered. I tested this on macOS.
I don't know if it's going to be problematic or not :-/

Changes I did:

rebased on top of main
merged readAndCreateClient in newDNSHandler as it imo made things more complicated rather than easier to read
made getDNSHostAndPort private
split the test case addition and the addLocalAnswers addition as this simplifies the 'main' commit (the one adding h.dnsClient.Exchange())
fixed the FIXME in handleTCP/handleUDP

cfergeau · 2024-08-29T15:45:07Z

Before, if I had 1.2.3.4 some.host.example.com in /etc/hosts on my host machine, then the guest would resolve some.host.example.com to 1.2.3.4.
After these changes, /etc/hosts will no longer be considered. I tested this on macOS.
I don't know if it's going to be problematic or not :-/

CRC relies on the crc.testing and apps-crc.testing entries in the host /etc/hosts file, so I think this is going to cause a problem :-/ Any idea how/if we could address this?

evidolob · 2024-09-12T07:00:18Z

@cfergeau @gbraad I add 'hosts' file records handling in evidolob#2
Pls, give a look on it

cfergeau · 2024-09-18T08:28:19Z

@cfergeau @gbraad I add 'hosts' file records handling in evidolob#2 Pls, give a look on it

Looks like a good way forward! Let's merge this PR which has waited for far too long, and then we'll add /etc/hosts support as a follow-up.

This highly simplify resolving DNS code. Also, DNS will work only for IPv4 Signed-off-by: Yevhen Vydolob <[email protected]> Co-authored-by: Christophe Fergeau <[email protected]> Signed-off-by: Christophe Fergeau <[email protected]>

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 <[email protected]>

cfergeau · 2024-09-18T08:40:54Z

/lgtm

cfergeau · 2024-09-18T08:41:14Z

/approve
/hold
(waiting for the tests to run)

openshift-ci · 2024-09-18T08:41:20Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cfergeau · 2024-09-18T09:57:59Z

win-sshproxy-tests failing is unfortunately expected :-/
/unhold

evidolob requested a review from cfergeau March 27, 2024 09:48

evidolob force-pushed the replace-dns branch from 8281934 to e6b54f7 Compare March 27, 2024 09:53

cfergeau reviewed Apr 18, 2024

View reviewed changes

cfergeau reviewed Apr 19, 2024

View reviewed changes

pkg/services/dns/dns.go Outdated Show resolved Hide resolved

cfergeau reviewed Apr 19, 2024

View reviewed changes

pkg/services/dns/dns.go Outdated Show resolved Hide resolved

cfergeau reviewed Apr 22, 2024

View reviewed changes

evidolob force-pushed the replace-dns branch 2 times, most recently from 5579f34 to bcea6b1 Compare August 5, 2024 08:16

evidolob requested a review from cfergeau August 5, 2024 08:16

evidolob force-pushed the replace-dns branch 2 times, most recently from b7bf0a7 to f2ae1d9 Compare August 5, 2024 12:00

gbraad reviewed Aug 28, 2024

View reviewed changes

pkg/services/dns/dns_config_unix.go Show resolved Hide resolved

gbraad reviewed Aug 28, 2024

View reviewed changes

gbraad assigned cfergeau Aug 28, 2024

evidolob and others added 4 commits August 28, 2024 14:41

Add "IdentitiesOnly" SSH option to fix running tests when known_hosts…

0cec834

… has multiple records Signed-off-by: Yevhen Vydolob <[email protected]> Signed-off-by: Christophe Fergeau <[email protected]>

Add DNS server test

0752179

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 <[email protected]>

Add 'addLocalAnswers' helper

4cdab42

This is the beginning of a refactor of addAnswers to make it easier to understand. Signed-off-by: Christophe Fergeau <[email protected]>

Add dependency on "github.com/qdm12/dns/v2"

f1d6847

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 <[email protected]> Signed-off-by: Christophe Fergeau <[email protected]>

evidolob force-pushed the replace-dns branch from f2ae1d9 to b56d081 Compare August 30, 2024 11:34

evidolob and others added 2 commits September 18, 2024 10:32

dns: Fix UDP and TCP request handling

be5257a

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 <[email protected]>

cfergeau force-pushed the replace-dns branch from b56d081 to be5257a Compare September 18, 2024 08:39

openshift-ci bot added the lgtm label Sep 18, 2024

openshift-ci bot added the do-not-merge/hold label Sep 18, 2024

openshift-ci bot added the approved label Sep 18, 2024

openshift-ci bot removed the do-not-merge/hold label Sep 18, 2024

openshift-merge-bot bot merged commit 3c875de into containers:main Sep 18, 2024
20 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dns "resolver" calls with "dns.Exchange" func #339

Replace dns "resolver" calls with "dns.Exchange" func #339

evidolob commented Mar 27, 2024

cfergeau left a comment

cfergeau Apr 18, 2024

evidolob Aug 5, 2024

cfergeau commented Aug 27, 2024

gbraad Aug 28, 2024

cfergeau Aug 28, 2024

gbraad Aug 28, 2024

gbraad left a comment

cfergeau commented Aug 28, 2024

cfergeau commented Aug 29, 2024 •

edited

Loading

evidolob commented Sep 12, 2024

cfergeau commented Sep 18, 2024

cfergeau commented Sep 18, 2024

cfergeau commented Sep 18, 2024

openshift-ci bot commented Sep 18, 2024

cfergeau commented Sep 18, 2024

Replace dns "resolver" calls with "dns.Exchange" func #339

Replace dns "resolver" calls with "dns.Exchange" func #339

Conversation

evidolob commented Mar 27, 2024

cfergeau left a comment

Choose a reason for hiding this comment

cfergeau Apr 18, 2024

Choose a reason for hiding this comment

evidolob Aug 5, 2024

Choose a reason for hiding this comment

cfergeau commented Aug 27, 2024

gbraad Aug 28, 2024

Choose a reason for hiding this comment

cfergeau Aug 28, 2024

Choose a reason for hiding this comment

gbraad Aug 28, 2024

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

cfergeau commented Aug 28, 2024

cfergeau commented Aug 29, 2024 • edited Loading

evidolob commented Sep 12, 2024

cfergeau commented Sep 18, 2024

cfergeau commented Sep 18, 2024

cfergeau commented Sep 18, 2024

openshift-ci bot commented Sep 18, 2024

cfergeau commented Sep 18, 2024

cfergeau commented Aug 29, 2024 •

edited

Loading