Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: detect Comcast et al DNS and auto-skip DNS tests #17884

Closed
bradfitz opened this issue Nov 10, 2016 · 5 comments
Closed

net/http: detect Comcast et al DNS and auto-skip DNS tests #17884

bradfitz opened this issue Nov 10, 2016 · 5 comments
Labels
FrozenDueToAge help wanted Suggested Issues that may be good for new contributors looking for work to do. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@bradfitz
Copy link
Contributor

Follow-up to #17670 ...

Some net and net/http tests verify that Go does the right thing upon getting a DNS NXDOMAIN (no DNS name found) error.

But Comcast at al have DNS servers which return fake results pointing you to search engines instead.

Maybe we can detect those DNS servers or DNS results and make those tests auto-skip (t.Skip)

/cc @odeke-em

@bradfitz bradfitz added help wanted Suggested Issues that may be good for new contributors looking for work to do. Testing An issue that has been verified to require only test changes, not just a test failure. labels Nov 10, 2016
@bradfitz bradfitz added this to the Unplanned milestone Nov 10, 2016
@odeke-em
Copy link
Member

Aha interesting, AT&T does this for me too http://dnserrorassist.att.net/search/?q=http%3A//dns-should-not-resolve.golang/&r=&t=0.

Thanks for the ping, I am interested.

@siennathesane
Copy link

You could do something like this:

// all fake DNS tests
func TestNonResolvingDNS(t *testing.T) {
	tr := &http.Transport{}
	c := &http.Client{Transport: tr, CheckRedirect: func(next *http.Response, prev []*http.Request) bool {
		if len(prev) > 1 {
			return false
		} else {
			return true
		}
	},
	}
	req, _ := http.NewRequest("GET", "unresolving.dns", nil)
	resp, _ := c.Do(req)
	if !c.CheckRedirect(req, resp) {
		t.Skip("TestWithRealDNS", TestSomethingWithRealDNS1)
		t.Skip("TestWithRealDNS2", TestSomethingWithRealDNS2)
	}
	
}

That would allow you basic segregation for the tests without having to do too much logic. Without a mostly full-featured DNS library, checking for redirections is the easiest method. That being said, an ISC-compliant library in the sub-repo library could be useful to a lot of people.

@vcabbage
Copy link
Member

@bradfitz Making a request for a known nonexistent domain and checking that no addresses were returned should be a reasonably accurate.

For this purpose the error from net.LookupHost can likely be ignored since the DNS tests will fail anyway if there are issues unrelated to NXDOMAIN responses.

I can create a CL if this approach seems reasonable.

var (
	isShadyDNSOnce sync.Once
	isShadyDNS     bool
)

func IsShadyDNS() bool {
	isShadyDNSOnce.Do(func() {
		addrs, _ := net.LookupHost("dns-should-not-resolve.golang")
		isShadyDNS = len(addrs) != 0
	})
	return isShadyDNS
}

For testing, some of the public providers on this list exhibit the bad behavior.

Example querying "Comodo Secure DNS."

# nslookup dns-should-not-resolve.golang 8.26.56.26
Server:		8.26.56.26
Address:	8.26.56.26#53

Non-authoritative answer:
Name:	dns-should-not-resolve.golang
Address: 92.242.144.50

@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 18, 2016 via email

@gopherbot
Copy link

CL https://golang.org/cl/34516 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Feb 1, 2017
@golang golang locked and limited conversation to collaborators Feb 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted Suggested Issues that may be good for new contributors looking for work to do. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

5 participants