Comment 1:

Actually noticed that the result is not being returned because of the for loop issue. I
did some bad analysis that made me conclude otherwise.
Also a better test would be to search net.LookupMX("transport-testing.nist.gov").

Comment 2:

I made a proof of concept fix: https://golang.org/cl/122200043/
It uses the same principle to take CNAME into account as OpenDNS (see
https://github.com/opendns/dynamicipupdate/blob/master/windows/src/DnsQuery.cpp).

Comment 3:

Regarding the OpenDNS approach, I'm not quite sure whether the check for
   if (cursor->Flags.S.Section != DnsSectionAnswer) {
and checking:
   if (DnsNameCompare(cursor->pName, name)) {
and handling:
   } else if (cursor->wType == DNS_TYPE_CNAME) {
       name = cursor->Data.CNAME.pNameHost;
Also can a DNS server (DnsQuery) return the results in arbitrary order, e.g. query for
MX d1 returns [d1 CNAME d2, d3 MX mail, d2 CNAME d3]? Or alternatively return irrelevant
in the query?

Comment 4:

Regarding the OpenDNS approach, I'm not quite sure whether the check for
   if (cursor->Flags.S.Section != DnsSectionAnswer) {
and checking:
   if (DnsNameCompare(cursor->pName, name)) {
and handling:
   } else if (cursor->wType == DNS_TYPE_CNAME) {
       name = cursor->Data.CNAME.pNameHost;
is sufficient/necessary.
I guess it depends whether a DNS server (DnsQuery) can return the results in arbitrary
order, e.g. query for MX d1 returns [d1 CNAME d2, d3 MX mail, d2 CNAME d3]. Or
alternatively return irrelevant values in the query, e.g. query for MX d1 returns [d2 MX
mail].

Comment 5:

egonelbre,
Thank you for the issue. Fix is good, but I think we should fix our tests first - tests
let us down - tests didn't complain about our problem. I think we should test more
hosts, the ones that fail. I also think we should use "nslookup -querytype=ns
www.golang.com" command to fetch results during the test - this way we can actually
verify that all our results are correct. Perhaps we should create new windows specific
tests to keep non-windows tests simple (I don't see how non-windows code could be
incorrect here). Would you like to do all that as part of your fix?
Alex

Labels changed: added repo-main, os-windows, release-go1.4.

Owner changed to @alexbrainman.

Status changed to Accepted.

Comment 6:

I'm not sure whether the inclusion of "nslookup" would be a good approach, it seems that
net tests try to get rid of third-party dependencies. Also if tested, then it should be
tested for all platforms, although it's less likely it's incorrect on other platforms.
Also, adding a call to nslookup would slow the tests down.
Currently I simply replaced the LookupNS("gmail.com") test with
LookupNS("www.golang.com"). I believe it should be sufficient, but if not, I can add
more comprehensive tests.

Comment 7:

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

Comment 8:

I am happy to hear other test suggestions (not nslookup). But I think we need better
tests then what we have now. None of current tests fail - so why are we changing code
then? You're also changing many lookup functions, why you only changing LookupNS test
only? Please, add tests for all others that fail now but will get PASSed once your
package changes are applied.
Like I said earlier, I would just use nslookup to test everything properly. You can try
to use nslookup if available and works. Do not fail the test, if you don't find nslookup
or if it fails. I am not sure about using nslookup on non-windows platforms. Surely it
won't work on plan9.
None of these tests run by the builder - builder only runs "go test -short". So new
tests won't slow anyone down, but just people who care about this code.
Also please don't remove old tests (like gmail.com), maybe create a little table of
hostnames you would like to test, and test them in a loop. I would leave current tests
alone, and write all new tests with nslookup to run on windows only in a new test file.
If it is too much for you, I will do it myself. I need proper tests before I would agree
we are making correct changes.
Alex

Comment 9:

Understood, I'll make a separate CL for the windows tests using nslookup.
Currently my main question would be, which hostnames to use. Would ["mail.golang.com",
"gmail.com"] be sufficient? Or should there also be a third server that as more than 1
CNAME redirect in it? Currently I wasn't able to find one.
Instead of nslookup, it could use things in dnsmsg.go to do the queries in pure Go. Of
course that would be much larger change and probably out of my competence.

Comment 10:

https://golang.org/cl/121450043
I wasn't able to figure out what to query to break LookupAddr and LookupSRV.

Comment 11:

I applied your new tests from https://golang.org/cl/121450043 to the current
tip, and they PASS (even without any net package changes). Why?
Alex

Comment 12:

I could get technical, but the main problem was me being stupid and not properly testing
things, sorry about that. PTAL

Comment 13:

Yes that FAILS now. But unfortunately, I think my nslookup output is different then
yours:
gmail.com       MX preference = 5, mail exchanger = gmail-smtp-in.l.google.com
gmail.com       MX preference = 30, mail exchanger = alt3.gmail-smtp-in.l.google.com
gmail.com       MX preference = 10, mail exchanger = alt1.gmail-smtp-in.l.google.com
gmail.com       MX preference = 40, mail exchanger = alt4.gmail-smtp-in.l.google.com
gmail.com       MX preference = 20, mail exchanger = alt2.gmail-smtp-in.l.google.com
I am using windows 7 here. Maybe my nslookup idea is not so good after all ...
Alex

Comment 14:

I'm getting the same result; the gmail.com version doesn't use CNAME-s so it should work
with the current version. It simply cross-validates that both get exactly the same
result as nslookup.
The results for both nslookup and the windows API calls should be the same for a given
computer, most of the time. Of course network changes and things can go down, so there
will be false negatives some times.

Comment 15:

I tried nslookup on my windows xp, and it looks the same as #13. Are you sure you use
windows nslookup (not unix). What windows version you're running?
Alex

Comment 16:

I get this:
--- FAIL: TestLookupMX (1.19s)
        lookup_windows_test.go:45: different results mail.golang.com:   exp:[]
got:[{"Host":"aspmx.l.google.com.","Pref":1},{"Host":"alt1.aspmx.l.google.com.",
"Pref":2},{"Host":"alt2.aspmx.l.google.com.","Pref":2},{"Host":"alt3.aspmx.l.goo
gle.com.","Pref":2}]
        lookup_windows_test.go:45: different results gmail.com: exp:[]  got:[{"H
ost":"gmail-smtp-in.l.google.com.","Pref":5},{"Host":"alt1.gmail-smtp-in.l.googl
e.com.","Pref":10},{"Host":"alt2.gmail-smtp-in.l.google.com.","Pref":20},{"Host"
:"alt3.gmail-smtp-in.l.google.com.","Pref":30},{"Host":"alt4.gmail-smtp-in.l.goo
gle.com.","Pref":40}]
=== RUN TestLookupNS
--- FAIL: TestLookupNS (1.50s)
        lookup_windows_test.go:69: different results "mail.golang.com": exp:[{"H
ost":"ns1.google.com"},{"Host":"ns2.google.com"},{"Host":"ns3.google.com"},{"Hos
t":"ns4.google.com"}]   got:[0xc0820036b0 0xc082003650 0xc082003670 0xc082003690
]
        lookup_windows_test.go:69: different results "gmail.com":       exp:[{"H
ost":"ns1.google.com"},{"Host":"ns2.google.com"},{"Host":"ns3.google.com"},{"Hos
t":"ns4.google.com"}]   got:[0xc0820034d0 0xc082003590 0xc0820035b0 0xc082003390
]
I think your regex does not match my nslookup output.
Alex

Comment 17:

I'm using Windows 7, but I noticed that I have dig installed and it contains nslookup.
C:\Windows>which nslookup
C:\Programs\dig\bin\nslookup.EXE
c:\Programs\Go\src\pkg\net>nslookup -querytype=mx gmail.com
Server:         192.168.1.254
Address:        192.168.1.254#53
Non-authoritative answer:
gmail.com       mail exchanger = 30 alt3.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 40 alt4.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 10 alt1.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 20 alt2.gmail-smtp-in.l.google.com.
gmail.com       mail exchanger = 5 gmail-smtp-in.l.google.com.
I guess using nslookup path could be hardcoded to c:\Windows\system32\nslookup.exe, and
fail the tests if it's not there?

Comment 18:

Yes, mine lives in c:\Windows\system32\nslookup.exe. And I don't have which. Perhaps you
have cygwin installed or something. See, if you can remove C:\Programs\dig\bin from your
PATH and try again. Do you have C:\Programs\dig\bin before c:\Windows\system32 in your
PATH?
Alex

Comment 19:

It seems the MX and TXT syntax are different, but only MX records needed fixing. It
should now pass with both nslookup-s.

Comment 20:

Yes, tests work now. Can you, please, combine your both CLs into one. That is what we
always do, when find new bug. Write a new test (reproducing this bug) that FAILs, fix
the test by changing package source accordingly, then submit all changes in one CL. I
will try and do proper review of your CL tomorrow or Monday. Thank you.
Alex

Comment 21:

This issue was closed by revision a18a360.

Status changed to Fixed.