Skip to content

Fix root servers query handling#59

Merged
michalc merged 1 commit intomichalc:mainfrom
admitrievsky:fix-root-servers
Jan 7, 2025
Merged

Fix root servers query handling#59
michalc merged 1 commit intomichalc:mainfrom
admitrievsky:fix-root-servers

Conversation

@admitrievsky
Copy link

@admitrievsky admitrievsky commented Dec 30, 2024

I'm using dns-rewrite-proxy, and for some reason the Clickhouse server is querying the root server's IP (.) and it hangs without receiving a proper response. The fix brings proper handling of such requests.

@admitrievsky admitrievsky changed the title fix root servers query handling Fix root servers query handling Dec 30, 2024
@michalc
Copy link
Owner

michalc commented Jan 6, 2025

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"?

@admitrievsky
Copy link
Author

Hi @michalc,

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

@michalc
Copy link
Owner

michalc commented Jan 7, 2025

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?

@admitrievsky
Copy link
Author

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?

@michalc
Copy link
Owner

michalc commented Jan 7, 2025

Done, and same error:

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

@admitrievsky
Copy link
Author

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?

@admitrievsky
Copy link
Author

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.

@michalc
Copy link
Owner

michalc commented Jan 7, 2025

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?

@admitrievsky
Copy link
Author

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.

@michalc michalc merged commit 6337254 into michalc:main Jan 7, 2025
7 checks passed
@michalc
Copy link
Owner

michalc commented Jan 7, 2025

Thanks! Merged and released in v0.0.157

@admitrievsky
Copy link
Author

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.

@michalc
Copy link
Owner

michalc commented Jan 8, 2025

Ah I missed that! Will do

@admitrievsky admitrievsky deleted the fix-root-servers branch January 8, 2025 07:56
@admitrievsky
Copy link
Author

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?

@michalc
Copy link
Owner

michalc commented Jan 8, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants