resolver/dns: support custom dns authority #2265

elliots · 2018-08-15T03:36:26Z

As described in https://github.com/grpc/grpc/blob/master/doc/naming.md, allows the use of a custom authority (dns server) with the dns resolver on go 1.9+. Gives an error in < go 1.9 if an authority is set.

e.g.

resolver.Target{Endpoint: "mydomain.com", Authority: "8.8.8.8"}

I'm not sure how to go about testing it though. Just check that the authority is passed through but continue as before in the tests?

See: #2173

elliots · 2018-08-15T04:25:57Z

I ran into some issues with different versions of go, and this got pretty ugly pretty quickly.

kriskowal · 2018-08-16T04:53:50Z

Looks like CI fails a vet check, and cursory inspection suggests unrelated issue with generated code mismatching expectations.

menghanl · 2018-08-16T17:21:25Z

The vet failure should be fixed by #2263. Please rebase and try again.

kriskowal · 2018-08-16T18:27:09Z

resolver/dns/go18.go

@@ -1,4 +1,4 @@
-// +build go1.8
+// +build go1.6, !go1.9


Filename becomes a bit of a misnomer. Idea: go18down, go19up.

Looks good, pending vet fix.

elliots · 2018-08-17T01:41:04Z

Rebased on top of #2263 and renamed version specific files as suggested.

Is there anything I can add to the tests for this functionality?

kriskowal · 2018-08-17T16:39:36Z

Hello, @lyuxuan. We are very interested in this feature at Uber. Please let us know what we can do to support its progress to release. Thank you @elliots, I was assigned this issue the day before you submitted your change!

elliots · 2018-08-18T02:26:55Z

Cleaned up the default resolver stuff a little. I think it's looking ok now. Still no tests, but not sure how to do that.

kriskowal · 2018-08-20T18:27:28Z

resolver/dns/go19up.go

+			PreferGo: true,
+			Dial: func(ctx context.Context, network, address string) (net.Conn, error) {
+				dialer := net.Dialer{}
+				return dialer.DialContext(ctx, "udp", authority)


Should we pass network through instead of punching in "udp"? I believe DNS as a protocol at least has the option of falling back to TCP if messages exceed the UDP datagram size limit.

Agree. Just pass network to DialContext, and let the golang net package handle the fallback.

kriskowal · 2018-08-20T19:59:40Z

resolver/dns/dns_resolver.go

@@ -121,6 +127,12 @@ func (b *dnsBuilder) Scheme() string {
 	return "dns"
 }

+type netResolver interface {


Creating a fake or mock for this interface might be the key to building up some coverage. We could also subvert a global function pointer for customAuthorityResolver for the duration of tests.

Alternately we can stand up a UDP socket and fake the DNS protocol to verify that the custom authority catches the traffic.

I'm using github.com/miekg/dns to implement my dns server, would it be ok to bring in that dependency for a test?

Or perhaps something simple like https://github.com/d-podkorytov/one_dns_go/blob/master/one_dns.go

It's preferred not to pull in extra dependency.

I agree with @kriskowal suggestion about having a global function pointer for usage inside customAuthorityResolver. We don't really need to have a round trip of DNS request/response, we just need to verify the resolver try to dial to the authority specified in the target string.

lyuxuan · 2018-08-21T00:33:01Z

resolver/dns/go19up.go

+
+	return &go18upResolver{
+		resolver: &net.Resolver{
+			PreferGo: true,


What's the reason for making PreferGo true?

I think we have to set it to true in order to use a custom dialler
( according to golang/go#19268 (comment) )

lyuxuan · 2018-08-21T00:58:55Z

resolver/dns/go19up.go

+)
+
+func customAuthorityResolver(authority string) (netResolver, error) {
+


nit: delete blank line

lyuxuan · 2018-08-21T01:19:30Z

resolver/dns/go18down.go

@@ -0,0 +1,29 @@
+// +build go1.6, !go1.9


change file name to pre_go19.go

lyuxuan · 2018-08-21T01:21:39Z

resolver/dns/go18up_test.go

-	lookupHost = net.DefaultResolver.LookupHost
-	lookupSRV  = net.DefaultResolver.LookupSRV
-	lookupTXT  = net.DefaultResolver.LookupTXT
+import (


change file name to go18_test.go

lyuxuan · 2018-08-21T01:21:57Z

resolver/dns/go19up.go

@@ -0,0 +1,44 @@
+// +build go1.9


change file name to go19.go

lyuxuan · 2018-08-21T01:50:08Z

resolver/dns/go18up.go

+
+/*
+ *
+ * Copyright 2017 gRPC authors.


lyuxuan · 2018-08-21T01:50:21Z

resolver/dns/go19up.go

+
+/*
+ *
+ * Copyright 2017 gRPC authors.


lyuxuan · 2018-08-21T01:50:38Z

resolver/dns/go18down.go

+
+/*
+ *
+ * Copyright 2017 gRPC authors.


lyuxuan · 2018-08-21T01:50:48Z

resolver/dns/go17down.go

+
+/*
+ *
+ * Copyright 2017 gRPC authors.


lyuxuan · 2018-08-21T01:51:03Z

resolver/dns/go17down.go

@@ -0,0 +1,44 @@
+// +build go1.6, !go1.8


change file name to pre_go18.go

kriskowal · 2018-08-21T02:23:21Z

@lyuxuan do you have recommendations for improving test coverage? Asking for a friend ;)

lyuxuan · 2018-08-24T01:09:35Z

Hi @kriskowal, I think having a test verifying DNS contacting the custom DNS authority when target string containing authority info is enough for this change. You are very welcome to bring up more test cases :). Thanks!

lyuxuan · 2018-08-30T20:07:54Z

@elliots, friendly ping. Let us know if you have any question/problem. Thanks!

elliots · 2018-08-31T22:51:44Z

All the tests in testDNSResolver are now run twice, once with no custom authority (as before) and once with one. I check to see the value has been passed in, then resolve using the test resolver.

elliots · 2018-08-31T23:04:56Z

I'm not sure how to go about the dial test...

lyuxuan · 2018-09-04T21:19:51Z

Thanks for the update, @elliots. For testing, I think it's not necessary to include the dial test right now. And

All the tests in testDNSResolver are now run twice, once with no custom authority (as before) and once with one.

This will run the exactly same logic twice. Can you make a separate test for verifying the right authority address is passed to the custom DNS dialer? Thanks!

elliots · 2018-09-07T22:01:17Z

Added a new test that only checks that the resolver receives the custom authority.

lyuxuan · 2018-09-08T00:57:09Z

Hi @elliots, thanks for the quick update. Appreciate it.

I may not have been very clear about what I meant by "dial test". I was suggesting it's not necessary to have a dns server set up and dial to it in the test. But I expect the Dial here to still be invoked. Instead of assigning the inline function to Dial, we can define a global variable to be same as the inline function, and assign the global variable to Dial. In the test, we set the global variable to be a made up dial function which does not really dial, but just verifies the dial target is the authority specified (plus the port :53 if the authority does not have a port).

lyuxuan · 2018-09-18T01:28:39Z

resolver/dns/go18.go

 )
+
+type go18upResolver struct {


This is actually an issue with our testing script (not using the most recent sdk for appengine test). I've fixed it through #2311.
Also, let's just support custom authority dialer from go1.9 (where type alias is supported, and thus the two context types are regarded as the same). Please change the build tags accordingly.

lyuxuan · 2018-09-19T00:00:02Z

resolver/dns/go19.go

+}
+
+func ensureAuthorityPort(target string) (targetWithPort string, err error) {
+


nit: delete empty line.

lyuxuan · 2018-09-19T00:22:15Z

resolver/dns/dns_resolver_go19_test.go

+		customAuthorityDialler = oldCustomAuthorityDialler
+	}()
+	for _, a := range tests {
+


Line 101-112 are unnecessary. ensureAuthorityPort will be called inside b.Build so we can just check the error returned from b.Build.

Errors from the dialler aren't returned from b.Build

customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) { return func(ctx context.Context, network, address string) (net.Conn, error) { return nil, errors.New("NOPE") } } b := NewBuilder() cc := &testClientConn{target: "foo.bar.com"} r, err := b.Build(resolver.Target{Endpoint: "foo.bar.com", Authority: a.authority}, cc, resolver.BuildOption{}) if r != nil { r.Close() } if err == nil { t.Errorf("Should not be nil") }

Am I missing something, or is that an issue with the builder?

We don't care the error from the customAuthorityDialler, but we do want to check the error from customAuthorityResolver (which is essentially the error returned by ensureAuthorityPort if not nil). I think the error from customAuthorityResolver is returned by b.Build.

Cool, that's what I did. Just checking if it should be returning that error. Removed the todos and recommitted.

lyuxuan · 2018-09-19T20:44:37Z

resolver/dns/dns_resolver_go19_test.go

+			}
+			return func(ctx context.Context, network, address string) (net.Conn, error) {
+				var dialer net.Dialer
+				return dialer.DialContext(ctx, network, authority)


we don't need to dial in the test, just return something like nil, errors.New("no need to dial").

lyuxuan · 2018-09-19T20:46:50Z

resolver/dns/dns_resolver_go19_test.go

+
+		b := NewBuilder()
+		cc := &testClientConn{target: "foo.bar.com"}
+		r, _ := b.Build(resolver.Target{Endpoint: "foo.bar.com", Authority: a.authority}, cc, resolver.BuildOption{})


check the error returned by b.Build, essentially the logic from Line 101-108.

lyuxuan · 2018-09-19T21:35:15Z

resolver/dns/go19.go

+	}
+
+	// target host is ipv4 or host-name
+	return fmt.Sprintf("%s:%s", host, port), nil


It seems ensureAuthorityPort share substantial logic with parseTarget. Could you please merge these two functions into one. So parseTarget takes an extra argument like defaultPort. And here for ensureAuthorityPort, you calls parseTarget with the new arg to be "53", and then call the net.JoinHostPort on the returned host and port.

kriskowal · 2018-09-19T22:13:42Z

@elliots Please let me know if you would like me to take a round of addressing feedback. I can send a commit from a fork if need be.

elliots · 2018-09-20T01:38:57Z

Thanks @kriskowal, I'll try to get the fixes in a bit quicker so that shouldn't be necessary. But if I don't respond for a day, you should feel free to take it and get it across the line :)

lyuxuan · 2018-09-20T22:56:12Z

resolver/dns/dns_resolver_go19_test.go

+	}()
+
+	for _, a := range tests {
+


nit: delete empty line.

lyuxuan · 2018-09-20T23:00:53Z

resolver/dns/dns_resolver_go19_test.go

+	for _, a := range tests {
+
+		customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) {
+


nit: delete empty line

lyuxuan · 2018-09-20T23:00:55Z

resolver/dns/dns_resolver_go19_test.go

+			return func(ctx context.Context, network, address string) (net.Conn, error) {
+				if authority != a.authorityWant {
+					t.Errorf("wrong custom authority passed to resolver. input: %s expected: %s actual: %s", a.authority, a.authorityWant, authority)
+					t.FailNow()


why t.FailNow() is needed?

lyuxuan · 2018-09-20T23:00:58Z

resolver/dns/dns_resolver_go19_test.go

+		if !a.expectError && err != nil {
+			t.Errorf("unexpected error using custom authority %s: %s", a.authority, err)
+		}
+


nit: delete empty line

lyuxuan · 2018-09-20T23:01:01Z

resolver/dns/dns_resolver_go19_test.go

+		}
+
+	}
+


nit: delete empty line

lyuxuan · 2018-09-20T23:24:53Z

resolver/dns/go19.go

+}
+
+var customAuthorityResolver = func(authority string) (netResolver, error) {
+	host, port, err := parseTarget(authority, "53")


define a constant(e.g. defaultDNSSvrPort) for "53", and replace it here and in tests.

lyuxuan · 2018-09-21T01:56:36Z

resolver/dns/dns_resolver_go19_test.go

+
+			return func(ctx context.Context, network, address string) (net.Conn, error) {
+				if authority != a.authorityWant {
+					t.Errorf("wrong custom authority passed to resolver. input: %s expected: %s actual: %s", a.authority, a.authorityWant, authority)


Since the the dialler will be called in a separate goroutine(i.e. the watcher goroutine), the Errorf here may not be called before the main test goroutine exit.

The better solution here is to create a channel of error type. And inside the custom dial function, push an error(or nil for no error) to this channel. And in the main goroutine, wait until an error is pushed to the channel, and check whether it is nil or not.

for _, a := range tests { errChan := make(chan error, 1) customAuthorityDialler = func(...) { ... return func(ctx context.Context, network, address string) (net.Conn, error) { if authority != a.authorityWant { errChan <- fmt.Errorf(...) } else { errChan <- nil } ... } ... // wait until the check in the dial function completes err := <- errChan // verify whether there's an error or not. if err != nil { t.Errorf(...) } }

lyuxuan

Thank you @elliots for your continuous effort and responsiveness! Just some nits that need a quick fix. Otherwise LGTM!

lyuxuan · 2018-09-25T00:30:07Z

resolver/dns/dns_resolver_test.go

+
+func (*testResolver) LookupHost(ctx context.Context, host string) ([]string, error) {
+	return hostLookup(host)
+}


add a blank line between functions.

lyuxuan · 2018-09-25T00:30:38Z

resolver/dns/dns_resolver_test.go

+}
+func (*testResolver) LookupSRV(ctx context.Context, service, proto, name string) (string, []*net.SRV, error) {
+	return srvLookup(service, proto, name)
+}


add a blank line between functions.

lyuxuan · 2018-09-25T00:34:45Z

resolver/dns/pre_go19.go

+
+func (*preGo19Resolver) LookupHost(ctx context.Context, host string) ([]string, error) {
+	return net.LookupHost(host)
+}


add a blank line between functions.

lyuxuan · 2018-09-25T00:34:50Z

resolver/dns/pre_go19.go

+}
+func (*preGo19Resolver) LookupSRV(ctx context.Context, service, proto, name string) (string, []*net.SRV, error) {
+	return net.LookupSRV(service, proto, name)
+}


add a blank line between functions.

lyuxuan · 2018-09-25T00:38:12Z

resolver/dns/dns_resolver_go19_test.go

+		},
+		{
+			"4.3.2.1",
+			"4.3.2.1:53",


It's better to change this line to "4.3.2.1:"+defaultDNSSvrPort,, so in the future if we need to change the default port for DNS server, we don't need to also change the tests.

lyuxuan · 2018-09-25T00:38:30Z

resolver/dns/dns_resolver_go19_test.go

+		},
+		{
+			"::1",
+			"[::1]:53",


same here, s/53/defaultDNSSvrPort

lyuxuan · 2018-09-25T00:38:38Z

resolver/dns/dns_resolver_go19_test.go

+		},
+		{
+			"[::1]",
+			"[::1]:53",


same here, s/53/defaultDNSSvrPort

lyuxuan · 2018-09-25T00:38:43Z

resolver/dns/dns_resolver_go19_test.go

+		},
+		{
+			"dnsserver.com",
+			"dnsserver.com:53",


same here, s/53/defaultDNSSvrPort

wip add custom resolver test

elliots · 2018-09-25T01:45:21Z

@lyuxuan I think much more time was spent reviewing my changes than the changes took, thank you :)

kriskowal · 2018-09-25T17:46:44Z

Yes! 🙏

tywkeene · 2018-09-25T19:12:45Z

Looks like this merge is broken

# google.golang.org/grpc/resolver/dns
../../google.golang.org/grpc/resolver/dns/go19.go:30:18: undefined: netResolver
../../google.golang.org/grpc/resolver/dns/go19.go:42:55: undefined: netResolver
../../google.golang.org/grpc/resolver/dns/go19.go:43:32: too many arguments in call to parseTarget
	have (string, string)
	want (string)

Building in golang:1.10 on docker 17.12.1-ce

lyuxuan · 2018-09-25T19:51:06Z

@tywkeene, can you provide more info about how this merge breaks you?

tywkeene · 2018-09-25T20:04:45Z

@tywkeene, can you provide more info about how this merge breaks you?

It's just a pretty cut and dry typo/build error that I posted above.

Not sure what else I could post that would help. Could you be more specific?

lyuxuan · 2018-09-25T20:18:29Z

I am wondering how you updated your code base and got this failure(details like what's the command you use to update the code base and build, etc. will be helpful). In this merge, it added the netResolver interface and extended parseTarget to take two arguments (in dns_resolver.go). From the error message, it looks like you are getting the new go19.go file, but not getting the updated dns_resolver.go file. I am not sure what's happening here.

tywkeene · 2018-09-25T20:24:59Z

I am wondering how you updated your code base and got this failure(details like what's the command you use to update the code base and build, etc. will be helpful).

RUN go get google.golang.org/grpc

In my Dockerfile from the golang:1.10 image. I'm curious how this made it through the CI

I've resolved this issue by pinning to the previous commit using dep

lyuxuan · 2018-09-25T20:37:44Z

Could you please check whether your code base has the latest dns_resolver.go file (which includes the netResolver definition and updated parseTarget)? Thanks!

nomis52 · 2018-10-10T18:17:00Z

Are there any estimates on when this will be available in a release ? We're eager to have it.

lyuxuan · 2018-10-10T18:40:35Z

Our next major release is Oct 23rd. Does it work for you?

nomis52 · 2018-10-11T15:25:46Z

That works. We can hold until then.

elliots force-pushed the customdnsauthority branch 5 times, most recently from 3ce2cd6 to 211bde6 Compare August 15, 2018 04:25

kriskowal reviewed Aug 16, 2018

View reviewed changes

menghanl requested a review from lyuxuan August 16, 2018 20:01

menghanl assigned lyuxuan Aug 16, 2018

elliots force-pushed the customdnsauthority branch from 211bde6 to e5ff36a Compare August 17, 2018 01:37

elliots force-pushed the customdnsauthority branch from e5ff36a to fa6a7d9 Compare August 18, 2018 02:20

elliots changed the title ~~resolver/dns: support custom dns authority WIP~~ resolver/dns: support custom dns authority Aug 18, 2018

kriskowal reviewed Aug 20, 2018

View reviewed changes

lyuxuan suggested changes Aug 21, 2018

View reviewed changes

elliots force-pushed the customdnsauthority branch from fa6a7d9 to fe90869 Compare August 22, 2018 23:47

elliots force-pushed the customdnsauthority branch from fe90869 to 2242818 Compare August 31, 2018 22:48

elliots force-pushed the customdnsauthority branch from 2242818 to a51d9cd Compare September 7, 2018 22:01

elliots force-pushed the customdnsauthority branch from bc3b693 to 91db3ef Compare September 17, 2018 03:20

lyuxuan suggested changes Sep 19, 2018

View reviewed changes

elliots force-pushed the customdnsauthority branch from 91db3ef to 4237fcd Compare September 20, 2018 01:36

elliots force-pushed the customdnsauthority branch 4 times, most recently from 2ff27eb to 15db8e2 Compare September 21, 2018 01:21

lyuxuan suggested changes Sep 21, 2018

View reviewed changes

elliots force-pushed the customdnsauthority branch from 15db8e2 to 972b6e5 Compare September 22, 2018 05:30

lyuxuan approved these changes Sep 25, 2018

View reviewed changes

resolver/dns: support custom dns authority

5094edd

wip add custom resolver test

elliots force-pushed the customdnsauthority branch from 972b6e5 to 5094edd Compare September 25, 2018 01:42

lyuxuan added this to the 1.16 Release milestone Sep 25, 2018

lyuxuan added the Type: Feature New features or improvements in behavior label Sep 25, 2018

lyuxuan merged commit ebea9b5 into grpc:master Sep 25, 2018

dfawley mentioned this pull request Nov 21, 2018

Custom DNS authority support #2173

Closed

lock bot locked as resolved and limited conversation to collaborators Apr 9, 2019

		)

		func customAuthorityResolver(authority string) (netResolver, error) {

		}

		func ensureAuthorityPort(target string) (targetWithPort string, err error) {

		for _, a := range tests {

		customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) {

resolver/dns: support custom dns authority #2265

resolver/dns: support custom dns authority #2265

Conversation

elliots commented Aug 15, 2018 • edited Loading

elliots commented Aug 15, 2018

kriskowal commented Aug 16, 2018

menghanl commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliots commented Aug 17, 2018 • edited Loading

kriskowal commented Aug 17, 2018

elliots commented Aug 18, 2018

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

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

kriskowal commented Aug 21, 2018

lyuxuan commented Aug 24, 2018

lyuxuan commented Aug 30, 2018

elliots commented Aug 31, 2018 • edited Loading

elliots commented Aug 31, 2018

lyuxuan commented Sep 4, 2018 • edited Loading

elliots commented Sep 7, 2018

lyuxuan commented Sep 8, 2018

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

lyuxuan Sep 20, 2018 • 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

kriskowal commented Sep 19, 2018

elliots commented Sep 20, 2018

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

lyuxuan 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

elliots commented Sep 25, 2018 • edited Loading

kriskowal commented Sep 25, 2018

tywkeene commented Sep 25, 2018 • edited Loading

lyuxuan commented Sep 25, 2018

tywkeene commented Sep 25, 2018

lyuxuan commented Sep 25, 2018 • edited Loading

tywkeene commented Sep 25, 2018

lyuxuan commented Sep 25, 2018

nomis52 commented Oct 10, 2018

lyuxuan commented Oct 10, 2018

nomis52 commented Oct 11, 2018

elliots commented Aug 15, 2018 •

edited

Loading

elliots commented Aug 17, 2018 •

edited

Loading

elliots commented Aug 31, 2018 •

edited

Loading

lyuxuan commented Sep 4, 2018 •

edited

Loading

lyuxuan Sep 20, 2018 •

edited

Loading

elliots commented Sep 25, 2018 •

edited

Loading

tywkeene commented Sep 25, 2018 •

edited

Loading

lyuxuan commented Sep 25, 2018 •

edited

Loading