Skip to content

Commit

Permalink
Correct behavior around trust_nx_responses
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@peterthejohnston
peterthejohnston committed Sep 22, 2021
1 parent f7465c9 commit 6d7df2f
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 88 deletions.
12 changes: 3 additions & 9 deletions crates/resolver/src/name_server/name_server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,9 @@ impl<C: DnsHandle<Error = ResolveError>, P: ConnectionProvider<Conn = C>> NameSe

match response {
Ok(response) => {
// first we'll evaluate if the message succeeded
// see https://github.com/bluejekyll/trust-dns/issues/606
// TODO: there are probably other return codes from the server we may want to
// retry on. We may also want to evaluate NoError responses that lack records as errors as well
let response = if self.config.trust_nx_responses {
ResolveError::from_response(response, self.config.trust_nx_responses)?
} else {
response
};
// First evaluate if the message succeeded.
let response =
ResolveError::from_response(response, self.config.trust_nx_responses)?;

// TODO: consider making message::take_edns...
let remote_edns = response.edns().cloned();
Expand Down
6 changes: 4 additions & 2 deletions tests/integration-tests/src/mock_client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ pub struct MockClientHandle<O: OnSend, E> {
}

impl<E> MockClientHandle<DefaultOnSend, E> {
/// constructs a new MockClient which returns each Message one after the other
/// constructs a new MockClient which returns each Message one after the other (messages are
/// popped off the back of `messages`, so they are sent in reverse order).
pub fn mock(messages: Vec<Result<DnsResponse, E>>) -> Self {
println!("MockClientHandle::mock message count: {}", messages.len());

Expand All @@ -37,7 +38,8 @@ impl<E> MockClientHandle<DefaultOnSend, E> {
}

impl<O: OnSend, E> MockClientHandle<O, E> {
/// constructs a new MockClient which returns each Message one after the other
/// constructs a new MockClient which returns each Message one after the other (messages are
/// popped off the back of `messages`, so they are sent in reverse order).
pub fn mock_on_send(messages: Vec<Result<DnsResponse, E>>, on_send: O) -> Self {
println!(
"MockClientHandle::mock_on_send message count: {}",
Expand Down
113 changes: 36 additions & 77 deletions tests/integration-tests/tests/name_server_pool_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,97 +262,55 @@ fn test_local_mdns() {
}

#[test]
fn test_trust_nx_responses_fails_servfail() {
fn test_trust_nx_responses_fails() {
use trust_dns_proto::op::ResponseCode;

let options = ResolverOpts::default();
use trust_dns_resolver::error::ResolveErrorKind;

let query = Query::query(Name::from_str("www.example.").unwrap(), RecordType::A);

let mut servfail_message = message(query.clone(), vec![], vec![], vec![]);
servfail_message.set_response_code(ResponseCode::ServFail);
let servfail_message = Ok(servfail_message);

let v4_record = v4_record(query.name().clone(), Ipv4Addr::new(127, 0, 0, 2));
let success_msg = message(query.clone(), vec![v4_record], vec![], vec![]);

let tcp_message = success_msg.clone();
let udp_message = success_msg;
let mut nx_message = message(query.clone(), vec![], vec![], vec![]);
nx_message.set_response_code(ResponseCode::NXDomain);

// fail the first udp request
let udp_nameserver = mock_nameserver_trust_nx(
vec![
Ok(udp_message.into()),
servfail_message.clone().map(Into::into),
],
options,
false,
);
let tcp_nameserver = mock_nameserver_trust_nx(
vec![Err(ResolveError::from("Forced Testing Error"))],
options,
false,
let success_msg = message(
query.clone(),
vec![v4_record(query.name().clone(), Ipv4Addr::new(127, 0, 0, 2))],
vec![],
vec![],
);

let mut pool = mock_nameserver_pool(vec![udp_nameserver], vec![tcp_nameserver], None, options);
// Fail the first UDP request.
let fail_nameserver =
mock_nameserver_trust_nx(vec![Ok(nx_message.into())], ResolverOpts::default(), true);
let succeed_nameserver =
mock_nameserver_trust_nx(vec![Ok(success_msg.into())], ResolverOpts::default(), true);

// lookup on UDP succeeds, any other would fail
let request = message(query.clone(), vec![], vec![], vec![]);
let future = pool.send(request).first_answer();

let response = block_on(future).unwrap();
assert!(response.response_code() == ResponseCode::ServFail);

// fail all udp succeed tcp
let udp_nameserver = mock_nameserver(vec![servfail_message.map(Into::into)], options);
let tcp_nameserver = mock_nameserver(vec![Ok(tcp_message.into())], options);

let mut pool = mock_nameserver_pool(vec![udp_nameserver], vec![tcp_nameserver], None, options);
let mut pool = mock_nameserver_pool(
vec![fail_nameserver, succeed_nameserver],
vec![],
None,
ResolverOpts::default(),
);

// Lookup on UDP should fail, since we trust nx responses.
// (If we retried the query with the second name server, we'd see a successful response.)
let request = message(query, vec![], vec![], vec![]);
let future = pool.send(request).first_answer();

let response = block_on(future).unwrap();
assert!(response.response_code() == ResponseCode::ServFail);
let response = block_on(future).expect_err("lookup request should fail with NXDOMAIN");
match response.kind() {
ResolveErrorKind::NoRecordsFound { response_code, .. }
if *response_code == ResponseCode::NXDomain => {}
kind => panic!(
"got unexpected kind of resolve error; expected `NoRecordsFound` error with NXDOMAIN,
got {:#?}",
kind,
),
}
}

#[test]
fn test_distrust_nx_responses() {
use trust_dns_proto::op::ResponseCode;

let options = ResolverOpts::default();

let query = Query::query(Name::from_str("www.example.").unwrap(), RecordType::A);

let mut servfail_message = message(query.clone(), vec![], vec![], vec![]);
servfail_message.set_response_code(ResponseCode::ServFail);
let servfail_message = Ok(servfail_message);

let v4_record = v4_record(query.name().clone(), Ipv4Addr::new(127, 0, 0, 2));
let success_msg = message(query.clone(), vec![v4_record.clone()], vec![], vec![]);

let tcp_message = success_msg;
//let udp_message = success_msg;

// fail the first udp request
let udp_nameserver =
mock_nameserver_trust_nx(vec![servfail_message.map(Into::into)], options, true);
let tcp_nameserver = mock_nameserver_trust_nx(vec![Ok(tcp_message.into())], options, true);

let mut pool = mock_nameserver_pool(vec![udp_nameserver], vec![tcp_nameserver], None, options);

// lookup on UDP succeeds, any other would fail
let request = message(query, vec![], vec![], vec![]);
let future = pool.send(request).first_answer();

let response = block_on(future).unwrap();
assert_eq!(response.answers()[0], v4_record);
}

#[test]
fn test_retry_on_error_response() {
use trust_dns_proto::op::ResponseCode;

let query = Query::query(Name::from_str("www.example.").unwrap(), RecordType::A);

const RETRYABLE_ERRORS: [ResponseCode; 9] = [
Expand All @@ -366,7 +324,7 @@ fn test_retry_on_error_response() {
ResponseCode::NotAuth,
ResponseCode::NotZone,
];
// Return an error response code, and have the client trust that response.
// Return an error response code, but have the client not trust that response.
let error_nameserver = mock_nameserver_trust_nx(
RETRYABLE_ERRORS
.iter()
Expand All @@ -377,15 +335,16 @@ fn test_retry_on_error_response() {
})
.collect(),
ResolverOpts::default(),
true,
false,
);

// Return a successful response on the fallback request.
let v4_record = v4_record(query.name().clone(), Ipv4Addr::new(127, 0, 0, 2));
let success_message = message(query.clone(), vec![v4_record.clone()], vec![], vec![]);
let fallback_nameserver = mock_nameserver(
let fallback_nameserver = mock_nameserver_trust_nx(
vec![Ok(success_message.into()); RETRYABLE_ERRORS.len()],
ResolverOpts::default(),
false,
);

let mut pool = mock_nameserver_pool(
Expand Down

0 comments on commit 6d7df2f

Please sign in to comment.