This commit introduced the acknowledgment of these error while processing the req/response in DNS prxoy and send refused if there is an error.

cc @bimmlerd as the author of that commit

Thanks @bimmlerd for the review ! :)

I think I need a bit of context to understand what you're trying to solve. Is there an issue which describes what the problem is?

The notifyOnDNSMsg sends an error if there is a DNS message processing issue. We typically don't ack errors in the ServeDNS method and when we do, we always send "refused," which isn't always appropriate. The goal of adding these error types to notifyOnDNSMsg is to ensure DNS requests/responses are denied or forwarded based on processing errors. (Future updates will use NotifyOnDNSMsgContext to send all parameters for better readability).
There is no active issue for this.

I agree with making the error callback and request/response(each) separated. I looked at the callbacks from DNS proxy and divided them into these categories(there are total 12 callbacks):

• Error on DNS Proxy end
  This can happen at different processing times while serving the DNS req.
  • In case of error before the endpoint is not retrieved: We only update the stat(proxy context) and return.
  • In case of error after endpoint is retrieved: We update the stat and ep proxy stat given the dns message(request) is parsed properly. There is a response callback too where we need to update the dns data as well.
• Allow/Deny a DNS request
  • We update the stat and ep proxy stat given the dns message(request) is parsed properly.
• Response from upstream dns
  • We update the stat and ep proxy stat as well dns data in endpoint.

Does it make sense to separate out the stat/ep proxy state updates i.e. UpdateProxyStats ?
Based on that, there will be 3 callbacks :

• NotifyOnDNSProxyError(): Will be called if there is an error on DNS proxy side to update stat, ep proxy stat. (Calls UpdateProxyStats)
• NotifyOnRequest(): Will be called when we want to forward an allowed DNS request. It will call the UpdateProxyStats first and if there is an error will return a Boolean along with the error if the request can be forwarded or not. (Similar to what is proposed in the PR)
• NotifyOnResponse(): Will be called for response from upstream to update the dns data. This will internally call UpdateProxyStats first and then update the DNS data in endpoint. In case of error, handles the error if the response can be forwarded or not.

There is also recording of the log which happens at the end that needs to be handled for each callback. Let me know your thoughts.

12 callbacks

Yeah it's pretty tangled at the moment😞 There's probably no way around trying to write down the full control flow for each of these and then try to find a clearer pattern which fits.

The other approach would be to reason from first principles - i.e. what could the proxy do itself and for what does it really need to call out to other things.

From a brief look through the code, it seems like the proxy currently wants to report errors to the daemon. I'm unsure whether this is truly necessary, or whether the tracing and metrics stuff could be handled internally to the proxy. If so, the query handling would probably also not need a callback. (Unless we factor out the DNS policy (not toFQDN) part from ServerDNS too - that could also be a separate component being called into by the proxy.)

Response handling, on the other hand, must be forwarded to the policy subsystem (i.e. the core of notifyOnDNSMessage gated by being a response) - that part should certainly stay.

(BTW, let's not make the next iteration use function pointers, but have the DNS proxy struct take an interface which other components can implement. That makes testing and reuse easier too, for the purposes of the SDP)

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.