Network servers should not ignore ServiceError.
#390
Conversation
…returning an appropriate DNS error message. Factor common service invoking and response processing code out to new trait ServiceInvoker and implement the network server specific parts for the UDP and TCP servers. Also simplifies some trait bounds.
src/net/server/dgram.rs
Outdated
| } | ||
| } | ||
| match Message::from_octets(buf) { | ||
| Err(err) => { |
There was a problem hiding this comment.
It seems that in this case no error gets returned to the client
There was a problem hiding this comment.
We don't have a DNS request from which to extract a query ID to copy into the error response. So we could generate a FORMERR response and send it, but with what ID?
There was a problem hiding this comment.
Note: When merged with main there is an additional Ok(msg) if msg.header().qr() => match stanza which also doesn't send a reply, but that DOES know the ID of the request and so should respond.
| } | ||
| } | ||
| trace!("Finished processing service call results for request id {request_id}"); | ||
| dispatcher.dispatch(request, service, ()).await |
There was a problem hiding this comment.
I get the impression that if Message::from_octets fails the client doesn't get a reply
| @@ -0,0 +1,188 @@ | |||
| /// Common service invoking logic for network servers. | |||
There was a problem hiding this comment.
Should the check for QR be moved into this?
There was a problem hiding this comment.
I didn't do that because I was trying to use as few server resources as possible and so immediately reject/discard incoming messages as soon as I can, where possible.
This PR addresses an omission in the
net::server::dgramandnet::server::streammodules whereby they iterate over the stream of service responses accepting onlySome(Ok)results and cease iterating on anything else. To stop onNoneis fine, but to stop onErrwithout taking any action is not.Background: The
Serviceinterface was designed such that services can easily returnErr(ServiceError)with the inteion that this allows them to easily produce certain standard DNS error responses without doing the work of constructing the response message correclty, keeping their error handling code paths simple. TheServiceErrortype has anrcode()method which the network server was supposed to use to construct the appopriate DNS response message, e.g.ServiceError::InternalErrortranslates to rcode SERVFAIL.But since the network servers never handle the service error case other than to ignore it that means in such cases the client wouldn't receive any DNS response message at all! This was not the intention, it was simply overlooked and clearly there also aren't any test cases that exercise this.
This PR makes three improvements:
ServiceErrorand construct and return the appropriate DNS response error message to the client.ServiceInvokerbase trait with default fn impls, with some network server specific methods to be filled in by the network server impls.Messageobject, while the TCP server would defer async task creation till a bit later.This PR does NOT (yet?) address the lack of test cases for this logic.