Fix root servers query handling - #59

Hi @admitrievsky,

Thanks for the PR!

I wonder though, can there be a different/better name than "DnsNoAnswer". It sounds a bit like there was no response, but there was a response, but it had no matching answers to the query. So maybe "DnsNoMatchingAnswers"?

Hi @michalc,

Thanks for the suggestion. Renamed the exception in both PRs.

Ah @admitrievsky thanks for the change. However, I see the new test is failing in CI - I think it uses whatever resolving server is available, and it's behaving differently to how you expect?

Hey @michalc, I didn’t make any major changes. I think it’s a temporary issue with the external DNS servers. When I run the tests locally, they work fine. Could you please run the CI again?

Done, and same error:

FAILED test.py::TestResolverEndToEnd::test_root_servers_queries - aiodnsresolver.DnsResponseCode: 2

Running nslookup . inside GitHub’s CI environment now returns the following:

 ;; Got SERVFAIL reply from 127.0.0.53
Server:		127.0.0.53
Address:	127.0.0.53#53

** server can't find .: SERVFAIL

while running nslookup . 8.8.8.8 returns:

Server:		8.8.8.8
Address:	8.8.8.8#53

Non-authoritative answer:
*** Can't find .: No answer

I can suggest the following options:

1. Move the tests inside TestResolverIntegration and mock upstream DNS server behaviour
2. change defaults to 8.8.8.8 inside TestResolverEndToEnd
3. drop those tests at all
4. change how the library deals with . domain name.

I'm wondering what you think about that?

I believe that the upstream DNS server should respond with a NOERROR status code and an empty A record set. To simplify the testing process, I suggest that we switch to using the 8.8.8.8 DNS server.

To simplify the testing process, I suggest that we switch to using the 8.8.8.8 DNS server.

I think that sounds good - but just be for the test added in this PR?

Yes, I changed this only for the new test. Now tests are OK.

I had to do a rebase on the main branch, so I've squashed commits too.

Thanks! Merged and released in v0.0.157

Please review the following PR: https://github.com/uktrade/dns-rewrite-proxy/pull/7. However, it is not essential for my current needs, as I am using a fork of the project.

Ah I missed that! Will do

Since there’s no dedicated issues section for this project, I thought I’d bring it up here. What are your thoughts on adding a DoH resolver? Is it feasible within the project’s scope?

Ah I just realised I never enabled issues (/must have disabled them at some point...), and as of this morning I have enabled them. Can you raise a separate issue there?