From d1019f156862da408d43a6ba954eeaca058f125b Mon Sep 17 00:00:00 2001 From: Daniel Fetti Date: Tue, 3 Oct 2023 01:41:25 +0300 Subject: [PATCH] [LLT-4202] Make lookup return error in case of failure on forward Make the forward error visible instead of just filling the response buffer with an empty response. --- crates/server/src/authority/catalog.rs | 89 +++++++++++++------------- crates/server/src/authority/error.rs | 10 +++ 2 files changed, 56 insertions(+), 43 deletions(-) diff --git a/crates/server/src/authority/catalog.rs b/crates/server/src/authority/catalog.rs index 8b7bd3d764e..ebdcd47e505 100644 --- a/crates/server/src/authority/catalog.rs +++ b/crates/server/src/authority/catalog.rs @@ -139,7 +139,10 @@ impl RequestHandler for Catalog { MessageType::Query => match request.op_code() { OpCode::Query => { debug!("query received: {}", request.id()); - let info = self.lookup(request, response_edns, response_handle).await; + let info = self + .lookup(request, response_edns, response_handle) + .await + .unwrap_or_else(|_| ResponseInfo::serve_failed()); Ok(info) } @@ -346,7 +349,7 @@ impl Catalog { request: &Request, response_edns: Option, response_handle: R, - ) -> ResponseInfo { + ) -> Result { let request_info = request.request_info(); let authority = self.find(request_info.query.name()); @@ -365,20 +368,18 @@ impl Catalog { // if this is empty then the there are no authorities registered that can handle the request let response = MessageResponseBuilder::new(Some(request.raw_query())); - let result = send_response( + send_response( response_edns, response.error_msg(request.header(), ResponseCode::Refused), response_handle, ) - .await; + .await + .map_err(|e| { + debug!("failed to send response: {}", e); + LookupError::Io(e) + })?; - match result { - Err(e) => { - debug!("failed to send response: {}", e); - ResponseInfo::serve_failed() - } - Ok(r) => r, - } + Err(LookupError::ResponseCode(ResponseCode::Refused)) } } @@ -405,7 +406,7 @@ async fn lookup<'a, R: ResponseHandler + Unpin>( request: &Request, response_edns: Option, response_handle: R, -) -> ResponseInfo { +) -> Result { let query = request_info.query; debug!( "request: {} found authority: {}", @@ -421,7 +422,7 @@ async fn lookup<'a, R: ResponseHandler + Unpin>( query, request.edns(), ) - .await; + .await?; let response = MessageResponseBuilder::new(Some(request.raw_query())).build( response_header, @@ -431,15 +432,12 @@ async fn lookup<'a, R: ResponseHandler + Unpin>( sections.additionals.iter(), ); - let result = send_response(response_edns.clone(), response, response_handle.clone()).await; - - match result { - Err(e) => { - debug!("error sending response: {}", e); - ResponseInfo::serve_failed() - } - Ok(i) => i, - } + send_response(response_edns.clone(), response, response_handle.clone()) + .await + .map_err(|e| { + debug!("failed to send response: {}", e); + LookupError::Io(e) + }) } #[allow(unused_variables)] @@ -473,7 +471,7 @@ async fn build_response( request_header: &Header, query: &LowerQuery, edns: Option<&Edns>, -) -> (Header, LookupSections) { +) -> Result<(Header, LookupSections), LookupError> { let lookup_options = lookup_options_for_edns(edns); // log algorithms being requested @@ -501,14 +499,14 @@ async fn build_response( request_id, query, ) - .await + .await? } ZoneType::Forward | ZoneType::Hint => { - send_forwarded_response(future, request_header, &mut response_header).await + send_forwarded_response(future, request_header, &mut response_header).await? } }; - (response_header, sections) + Ok((response_header, sections)) } async fn send_authoritative_response( @@ -518,7 +516,7 @@ async fn send_authoritative_response( lookup_options: LookupOptions, request_id: u16, query: &LowerQuery, -) -> LookupSections { +) -> Result { // In this state we await the records, on success we transition to getting // NS records, which indicate an authoritative response. // @@ -533,12 +531,12 @@ async fn send_authoritative_response( // TODO: there are probably other error cases that should just drop through (FormErr, ServFail) Err(LookupError::ResponseCode(ResponseCode::Refused)) => { response_header.set_response_code(ResponseCode::Refused); - return LookupSections { - answers: Box::new(AuthLookup::default()) as Box, - ns: Box::new(AuthLookup::default()) as Box, - soa: Box::new(AuthLookup::default()) as Box, - additionals: Box::new(AuthLookup::default()) as Box, - }; + return Ok(LookupSections { + answers: Box::::default(), + ns: Box::::default(), + soa: Box::::default(), + additionals: Box::::default(), + }); } Err(e) => { if e.is_nx_domain() { @@ -607,19 +605,19 @@ async fn send_authoritative_response( ), }; - LookupSections { + Ok(LookupSections { answers, ns: ns.unwrap_or_else(|| Box::new(AuthLookup::default()) as Box), soa: soa.unwrap_or_else(|| Box::new(AuthLookup::default()) as Box), additionals, - } + }) } async fn send_forwarded_response( future: impl Future, LookupError>>, request_header: &Header, response_header: &mut Header, -) -> LookupSections { +) -> Result { response_header.set_recursion_available(true); response_header.set_authoritative(false); @@ -634,26 +632,31 @@ async fn send_forwarded_response( request_header.id() ); - Box::new(EmptyLookup) + return Err(LookupError::ResponseCode(ResponseCode::Refused)); } else { match future.await { Err(e) => { + debug!("error resolving: {}", e); + if e.is_io() || e.is_unknown() { + return Err(e); + } + + // If the server responds with NXDomain we want to copy this behavior if e.is_nx_domain() { response_header.set_response_code(ResponseCode::NXDomain); } - debug!("error resolving: {}", e); Box::new(EmptyLookup) } Ok(rsp) => rsp, } }; - LookupSections { + Ok(LookupSections { answers, - ns: Box::new(AuthLookup::default()) as Box, - soa: Box::new(AuthLookup::default()) as Box, - additionals: Box::new(AuthLookup::default()) as Box, - } + ns: Box::::default(), + soa: Box::::default(), + additionals: Box::::default(), + }) } struct LookupSections { diff --git a/crates/server/src/authority/error.rs b/crates/server/src/authority/error.rs index db7b58cb010..c09abf8a278 100644 --- a/crates/server/src/authority/error.rs +++ b/crates/server/src/authority/error.rs @@ -56,6 +56,16 @@ impl LookupError { pub fn is_refused(&self) -> bool { matches!(*self, Self::ResponseCode(ResponseCode::Refused)) } + + /// This is an Io error + pub fn is_io(&self) -> bool { + matches!(*self, Self::Io(_)) + } + + /// This is an unknown error + pub fn is_unknown(&self) -> bool { + matches!(*self, Self::ResponseCode(ResponseCode::Unknown(_))) + } } impl From for LookupError {