correct behavior around trust_nx_responses #1556
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1556 +/- ##
=======================================
Coverage 83.39% 83.40%
=======================================
Files 171 171
Lines 17029 17026 -3
=======================================
- Hits 14201 14199 -2
+ Misses 2828 2827 -1 |
peterthejohnston
force-pushed
the
main
branch
from
8e9db8c
to
c148835
Compare
In 1b524af, which moved the `trust_nx_responses` setting from a configuration on the resolver to a configuration at the name server level, the option was also renamed from `distrust_nx_responses`. There were some places in the codebase where the corresponding boolean value was not also flipped, unintentionally changing the semantics. This has led to some confusing behavior. For example, in the `test_trust_nx_responses_fails_servfail` test, where `distrust_nx_responses` was previously set to `false`, `trust_nx_responses` was left as `false` in the change, where it should have been changed to `true`. This change fixes a bug where retry behavior doesn't occur unless `trust_nx_responses` is set to `true` (due to an error response not being considered a `ResolveError`), and reworks the tests to accurately test the current behavior. It also consolidates the `test_distrust_nx_responses` and `test_retry_on_error_response` tests into one test, as they exercised the same functionality.
peterthejohnston
force-pushed
the
main
branch
from
c148835
to
6d7df2f
Compare
|
Thanks for the review! Is there a plan for when the next alpha release of v0.21.0 will be tagged, or when v0.21.0 itself will come out? We'd like to be able to pull in this change whenever it's included in a release. |
| got {:#?}", | ||
| kind, | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_distrust_nx_responses() { |
There was a problem hiding this comment.
Is this test totally invalid now?
There was a problem hiding this comment.
It's basically testing the exact same thing as the test below it, which i added in #1526 — that when trust_nx_responses is set to false, we see the resolver try other name servers after getting an negative response.
So, I made sure that SERVFAIL, which is tested in test_distrust_nx_responses, was covered in the test below, and combined them. Does that make sense?
I can publish a new release whenever. Is there a date you'd like that? |
|
As soon as you're able to publish a new release would be excellent. There's no specific day, but the sooner the better. |
In this change which moved the
trust_nx_responsesconfig from a configuration on the Resolver to a configuration at the name server level, the option was also renamed fromdistrust_nx_responses. There were some places in the codebase where the corresponding boolean value was not also flipped, unintentionally changing the semantics. This has led to some confusing behavior. For example, in this name_server_pool_test, wheredistrust_nx_responseswas previously set tofalse,trust_nx_responseswas left asfalsein the change, where it should have been changed totrue.This PR fixes a bug where retry behavior doesn't occur unless
trust_nx_responsesis set to true (due to an error response not being considered aResolveError), and reworks the tests to accurately test the current behavior. It also consolidates thetest_distrust_nx_responsesandtest_retry_on_error_responsetests into one test, as they exercised the same functionality.