dns: fix custom servers support and extend tests #320

Choraden · 2023-07-28T11:12:17Z

No description provided.

Choraden · 2023-07-28T11:22:09Z

It looks like the dns fallback mechanism is broken.

func (r *resolver) dialDNS(ctx context.Context, network, address string) (net.Conn, error) {
	for _, u := range r.servers {
		r.log.Debugf("dial DNS server %s instead of %s", u, address)
		conn, err := r.dialer.DialContext(ctx, network, u.String())
		if err != nil {
			r.log.Errorf("failed to dial DNS server %s: %v", u, err)
			continue
		}
		return conn, nil
	}

	return nil, fmt.Errorf("failed to dial DNS server")
}

Custom dialDNS function always returns the connection to the first dns server, even if the server is wrong on purpose. The dial to UDP succeeds every time.

The error is encountered later when dns query is performed.

2023/07/28 13:16:34 INFO: using DNS servers: [127.0.0.1:34567 1.1.1.1:53]
--- FAIL: TestResolverLookupHostFallback (0.00s)
    dns_test.go:73: LookupHost failed: lookup google.com on 10.255.3.12:53: read udp 127.0.0.1:61671->127.0.0.1:34567: read: connection refused
    dns_test.go:76: LookupHost returned no address

mmatczuk · 2023-07-31T13:51:58Z

bind/flag.go

@@ -39,7 +39,8 @@ func DNSConfig(fs *pflag.FlagSet, cfg *forwarder.DNSConfig) {
 			"If specified multiple times, the first one is used as primary server, the rest are used as fallbacks. "+
 			"The port is optional, if not specified the default port is 53. ")
 	fs.DurationVar(&cfg.Timeout,
-		"dns-timeout", cfg.Timeout, "Timeout for dialing DNS servers.")
+		"dns-timeout", cfg.Timeout, "Timeout for dialing DNS servers. "+
+			"If specified multiple servers, the timeout for each lookup is equal to timeout divided by the number of servers.")


This should be for each server.

mmatczuk · 2023-08-02T13:38:54Z

dial.go

+	resolvers := make([]net.Resolver, 0, len(cfg.DNSServers))
+	for _, s := range cfg.DNSServers {
+		dnsAddress := s.String()
+		resolvers = append(resolvers, net.Resolver{
+			PreferGo: true,
+			Dial: func(ctx context.Context, network, address string) (net.Conn, error) {
+				l.Debugf("dial DNS server %s instead of %s", dnsAddress, address)
+				return nd.DialContext(ctx, network, dnsAddress)
+			},
+		})
+	}


Refactor to function, if no DNS servers specified return nil

mmatczuk · 2023-08-02T13:39:35Z

dial.go

+		lctx, cancel := context.WithTimeout(ctx, d.cfg.ResolveTimeout)
+		ips, err := d.resolvers[i].LookupIP(lctx, network, hostname)
+		cancel()
+		if err != nil {
+			d.log.Debugf("failed to resolve %s on %s: %s", hostname, d.cfg.DNSServers[i], err)
+		} else {
+			return ips, nil
+		}


Refactor to method

mmatczuk · 2023-08-02T13:41:53Z

dial.go

+		return nil, err
+	}
+
+	ips, err := d.LookupIP(ctx, "ip", host)


network is specified in the DNS server entry, this parameter should be removed from LookupIP.

http RoundTripper uses tcp network, when resolver.LookupIP needs ip, ipv4 or ipv6

mmatczuk · 2023-08-02T13:46:39Z

dial.go

+	return nil, fmt.Errorf("failed to dial %s on any IP address", address)
+}
+
+func (d *Dialer) DialFunc() dialFunc { //nolint:revive,golint // Unexported by design.


I agree with the linter. It looks odd and has no purpose really. The Dialer should implement DialContext.

mmatczuk · 2023-08-02T13:49:17Z

dial_test.go

+)
+
+func TestDialerLookupHost(t *testing.T) {
+	if _, ok := os.LookupEnv("CI"); ok {


We should run in in CI. We have tests talking to google.com so a DNS query to CF won't hurt.

dial.go

mmatczuk · 2023-08-02T14:01:35Z

dial.go

+		resolvers = append(resolvers, net.Resolver{
+			PreferGo: true,
+			Dial: func(ctx context.Context, network, address string) (net.Conn, error) {
+				l.Debugf("dial DNS server %s instead of %s", dnsAddress, address)


What if I have 5 dns servers?
Would it dial each DNSServer 5 times?

It would sequentially dial the 5 servers, one after another, unless a lookup succeeds. The net.Dialer dials two times by default.

mmatczuk · 2023-08-02T14:03:45Z

dial_test.go

@@ -16,7 +16,7 @@ import (
 	"github.com/saucelabs/forwarder/log/stdlog"
 )

-func TestResolverLookupHost(t *testing.T) {


I'd like to see a test for context cancelation.

mmatczuk · 2023-08-02T14:16:10Z

I'm still not really convinced that using the resolver hack is better.

For instance https://github.com/rs/dnstrace provides you with more robust client supporting recursive queries (with CNAME support).
On top of that we would get DNS over https support for free.

The full list is here maybe you can find something even better https://pkg.go.dev/github.com/miekg/dns?tab=importedby.

Choraden · 2023-08-02T14:48:09Z

I agree, the resolver hack has its disadvantages, but I would not give up this idea. We can try with the client and compare the results.

Choraden · 2023-08-03T14:38:53Z

We abandoned the custom dialer with resolvers idea in favour of using Golang's net package internals. The internals are hidden from user. To overcome it, we link the private function and manually configure dnsConfig. This way we can manipulate the behaviour of internal Golang's Resolver.

mmatczuk · 2023-08-04T08:39:26Z

dns.go


 	"github.com/saucelabs/forwarder/log"
 )

+// dnsConfig is 1:1 copy of net.dnsConfig struct.


put it to a separate file then.

We are not capable of asserting the hostname is resolved at the right dns server. The tests will be rewritten to e2e suite where we have more powerfull tools to perform them.

mmatczuk · 2023-08-04T10:59:20Z

bind/flag.go

+	DNSConfig(fs, &cfg.DNSConfig)
+}
+
+func HTTPTransportConfig(fs *pflag.FlagSet, cfg *forwarder.HTTPTransportConfig) {
+	DialConfig(fs, &cfg.DialConfig, "http")
+


Function order.

mmatczuk

Those can be squashed.

e2e(dns server setup): increase http dial timeout for proxy Hubert Grochowski 04/08/2023, 09:37
e2e/forwarder(service): add withHTTPDialTimeout method Hubert Grochowski 04/08/2023, 09:35

Overall very well done, work nicely split to commits.

I left some minor comments inline.

LGTM

mmatczuk · 2023-08-08T08:36:45Z

dial.go

+	KeepAlive time.Duration
+}
+
+func DefaultDialConfig() DialConfig {


Could we return a ptr here?

mmatczuk · 2023-08-08T08:37:46Z

dial.go

+	nd  *net.Dialer
+}
+
+func NewDialer(cfg DialConfig) (*Dialer, error) {


Same here pass a ptr, Dialer keeps a copy like it does now.

mmatczuk · 2023-08-08T08:41:19Z

utils/osdns/dnsconf.go

@@ -0,0 +1,51 @@
+// Copyright 2023 Sauce Labs Inc. All rights reserved.


It looks like this file should also have the Go license in it.

It would be also good to point to the file it's based on.

mmatczuk · 2023-08-08T08:41:43Z

utils/osdns/resolvconf.go

@@ -0,0 +1,41 @@
+// Copyright 2023 Sauce Labs Inc. All rights reserved.


Same as in dnsconf.go

mmatczuk · 2023-08-08T08:43:45Z

bind/flag.go

 	fs.VarP(anyflag.NewSliceValue[netip.AddrPort](nil, &cfg.Servers, forwarder.ParseDNSAddress),
 		"dns-server", "n", "<ip>[:<port>]"+
 			"DNS server(s) to use instead of system default. "+
-			"If specified multiple times, the first one is used as primary server, the rest are used as fallbacks. "+
+			"If specified multiple times, the first one is used as primary server, the rest are used as fallbacks, "+


Would be good to add a note on --dns-round-robin

mmatczuk · 2023-08-08T08:48:16Z

bind/flag.go

+			"Only used if DNS servers are specified. ")
+
+	fs.BoolVar(&cfg.RoundRobin, "dns-round-robin", cfg.RoundRobin, "Enable round-robin DNS server selection. "+
+		"Won't have any effect if DNS servers are not specified. ")


This sentence is adds no value. Add something more relevant

If more than one DNS server is specified with the --dns-server flag, passing this flag ....

mmatczuk · 2023-08-08T08:51:14Z

cmd/forwarder/pac/server/server.go

@@ -36,7 +38,14 @@ func (c *command) RunE(cmd *cobra.Command, args []string) error {
 	logger := stdlog.New(c.logConfig)
 	logger.Debugf("configuration\n%s", config)

-	t, err := forwarder.NewHTTPTransport(c.httpTransportConfig, nil)
+	if len(c.dnsConfig.Servers) > 0 {
+		logger.Named("dns").Infof("using DNS servers %v", c.dnsConfig.Servers)


It would be nicer using strings.Join(c.dnsConfig.Servers, ", ",)

mmatczuk · 2023-08-08T08:55:36Z

e2e/setups.go

+		networkName      = "forwarder-e2e_default"
+		httpbinIPAddr    = "192.168.100.10"
+		proxyIPAddr      = "192.168.100.11"
+		dnsIPAddr        = "192.168.100.13"


It would be nicer if DNS came first with lower IPs.

This patch separates the dialer to its custom wrapper. This not only introduces a good structure to the package, but also is nice foundation to #118.

Use net package internal functions to configure custom dns options.

Fixes #318

e2e/forwarder(service): add withHTTPDialTimeout method Default value for HTTPDialTiemout is 10s. Default value for DNSTimeout is 5s. When test queried invalid address on two dns servers it would result in nondeterministic return code, as sometimes the dial timed out and sometimes the query failed.

Forwarder uses go:linkname directive. It may lead to unsafe behavior if the linked code changes. This workflow performs a primitive check by calculating the checksums of linked modules. If the checksums do not match, a user is forced to manually check the changes and update the checksums. The action runs on '.version' file changes.

Choraden · 2023-08-08T10:01:30Z

@mmatczuk I've addressed your comments. Merge when you are ready.

Increase health check start period and remove interval timeout as the test always succeeds.

Choraden · 2023-08-08T10:21:30Z

@mmatczuk I also tweaked sc-2450 setup health check values as the server got frozen in the previous run.

saucelabs deleted a comment from what-the-diff bot Jul 28, 2023

Choraden changed the title ~~dns: add test on fallback mechanism for custom dns servers~~ dns: rework resolver and add tests Jul 31, 2023

mmatczuk reviewed Jul 31, 2023

View reviewed changes

Choraden force-pushed the hg/dns branch 3 times, most recently from 2907a7f to c6e6f66 Compare August 1, 2023 15:47

Choraden mentioned this pull request Aug 2, 2023

add DNS e2e tests #328

Merged

Choraden force-pushed the hg/dns branch from c6e6f66 to 2e111b3 Compare August 2, 2023 12:19

Choraden marked this pull request as ready for review August 2, 2023 12:26

Choraden requested a review from mmatczuk August 2, 2023 12:27

mmatczuk reviewed Aug 2, 2023

View reviewed changes

dial.go Show resolved Hide resolved

mmatczuk reviewed Aug 2, 2023

View reviewed changes

Choraden force-pushed the hg/dns branch 3 times, most recently from 3c6abaf to 6780733 Compare August 3, 2023 14:34

Choraden changed the title ~~dns: rework resolver and add tests~~ dns: fix custom servers support and extend tests Aug 3, 2023

Choraden force-pushed the hg/dns branch from 637b6cf to 9117883 Compare August 4, 2023 08:29

mmatczuk reviewed Aug 4, 2023

View reviewed changes

dns: remove unit tests

263db5c

We are not capable of asserting the hostname is resolved at the right dns server. The tests will be rewritten to e2e suite where we have more powerfull tools to perform them.

mmatczuk reviewed Aug 4, 2023

View reviewed changes

Choraden force-pushed the hg/dns branch 14 times, most recently from 7fa3e8b to a65d8a7 Compare August 7, 2023 12:51

Choraden requested a review from mmatczuk August 7, 2023 14:32

mmatczuk approved these changes Aug 8, 2023

View reviewed changes

dial: separate custom dialer

63c25ad

This patch separates the dialer to its custom wrapper. This not only introduces a good structure to the package, but also is nice foundation to #118.

Choraden force-pushed the hg/dns branch from a65d8a7 to 7057a1d Compare August 8, 2023 09:55

Choraden added 6 commits August 8, 2023 11:56

utils/osdns: allow overridding os dns config

ae3b822

Use net package internal functions to configure custom dns options.

cmd/forwarder/run: allow to configure os dns

1f68f1e

cmd/forwarder/pac: allow to configure os dns

28ca8c3

e2e: add dns fallback setup

e49ebdf

Fixes #318

Choraden force-pushed the hg/dns branch from 7057a1d to b6de797 Compare August 8, 2023 09:57

e2e(sc-2450): tweak health ckeck values

847b15d

Increase health check start period and remove interval timeout as the test always succeeds.

mmatczuk merged commit 667dd88 into main Aug 8, 2023
3 checks passed

mmatczuk deleted the hg/dns branch August 8, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: fix custom servers support and extend tests #320

dns: fix custom servers support and extend tests #320

Choraden commented Jul 28, 2023

Choraden commented Jul 28, 2023 •

edited

Loading

mmatczuk Jul 31, 2023

mmatczuk Aug 2, 2023

mmatczuk Aug 2, 2023 •

edited

Loading

mmatczuk Aug 2, 2023

Choraden Aug 2, 2023

mmatczuk Aug 2, 2023

Choraden Aug 2, 2023

mmatczuk Aug 2, 2023

mmatczuk Aug 2, 2023 •

edited

Loading

Choraden Aug 2, 2023 •

edited

Loading

mmatczuk Aug 2, 2023

Choraden Aug 2, 2023

mmatczuk commented Aug 2, 2023 •

edited

Loading

Choraden commented Aug 2, 2023

Choraden commented Aug 3, 2023

mmatczuk Aug 4, 2023

mmatczuk Aug 4, 2023

mmatczuk left a comment

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

mmatczuk Aug 8, 2023

Choraden commented Aug 8, 2023

Choraden commented Aug 8, 2023

		@@ -0,0 +1,51 @@
		// Copyright 2023 Sauce Labs Inc. All rights reserved.

		@@ -0,0 +1,41 @@
		// Copyright 2023 Sauce Labs Inc. All rights reserved.

dns: fix custom servers support and extend tests #320

dns: fix custom servers support and extend tests #320

Conversation

Choraden commented Jul 28, 2023

Choraden commented Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choraden Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk commented Aug 2, 2023 • edited Loading

Choraden commented Aug 2, 2023

Choraden commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choraden commented Aug 8, 2023

Choraden commented Aug 8, 2023

Choraden commented Jul 28, 2023 •

edited

Loading

mmatczuk Aug 2, 2023 •

edited

Loading

mmatczuk Aug 2, 2023 •

edited

Loading

Choraden Aug 2, 2023 •

edited

Loading

mmatczuk commented Aug 2, 2023 •

edited

Loading