New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log workarounds only if verbose logging is enabled #101
Comments
The same applies to the |
Thank you for your response. There doesn't suppose to be so much logging. The excessive logging was mixed in through the contributions from all over the world. A large portion of them are not needed (even if in verbose mode). I hope I can fix it soon but currently I am just too busy. The task mainly consists of:
|
|
I won't guarantee I'll revamp logging sooner than you, but who knows ☺. For now I ended up with this: diff --git a/doh-server/ietf.go b/doh-server/ietf.go
index 497ff27..e0ec98a 100644
--- a/doh-server/ietf.go
+++ b/doh-server/ietf.go
@@ -194,7 +194,9 @@ func (s *Server) generateResponseIETF(ctx context.Context, w http.ResponseWriter
}
if respJSON.Status == dns.RcodeServerFailure {
- log.Printf("received server failure from upstream %s: %v\n", req.currentUpstream, req.response)
+ if s.conf.Verbose {
+ log.Printf("received server failure from upstream %s: %v\n", req.currentUpstream, req.response)
+ }
w.WriteHeader(503)
}
_, err = w.Write(respBytes)
@@ -206,7 +208,9 @@ func (s *Server) generateResponseIETF(ctx context.Context, w http.ResponseWriter
// Workaround a bug causing DNSCrypt-Proxy to expect a response with TransactionID = 0xcafe
func (s *Server) patchDNSCryptProxyReqID(w http.ResponseWriter, r *http.Request, requestBinary []byte) bool {
if strings.Contains(r.UserAgent(), "dnscrypt-proxy") && bytes.Equal(requestBinary, []byte("\xca\xfe\x01\x00\x00\x01\x00\x00\x00\x00\x00\x01\x00\x00\x02\x00\x01\x00\x00\x29\x10\x00\x00\x00\x80\x00\x00\x00")) {
- log.Println("DNSCrypt-Proxy detected. Patching response.")
+ if s.conf.Verbose {
+ log.Println("DNSCrypt-Proxy detected. Patching response.")
+ }
w.Header().Set("Content-Type", "application/dns-message")
w.Header().Set("Vary", "Accept, User-Agent")
now := time.Now().UTC().Format(http.TimeFormat)
@@ -220,7 +224,9 @@ func (s *Server) patchDNSCryptProxyReqID(w http.ResponseWriter, r *http.Request,
// Workaround a bug causing Firefox 61-62 to reject responses with Content-Type = application/dns-message
func (s *Server) patchFirefoxContentType(w http.ResponseWriter, r *http.Request, req *DNSRequest) bool {
if strings.Contains(r.UserAgent(), "Firefox") && strings.Contains(r.Header.Get("Accept"), "application/dns-udpwireformat") && !strings.Contains(r.Header.Get("Accept"), "application/dns-message") {
- log.Println("Firefox 61-62 detected. Patching response.")
+ if s.conf.Verbose {
+ log.Println("Firefox 61-62 detected. Patching response.")
+ }
w.Header().Set("Content-Type", "application/dns-udpwireformat")
w.Header().Set("Vary", "Accept, User-Agent")
req.isTailored = true |
@pfactum it was not introduced in that PR, it was already there as a bare @m13253 each and every log line that is reported here to be noisy was present before my changes, so please do not blame me for them. The only new log line introduced in that PR is |
Personally /me is here not to find who to blame but to improve a code a little bit. Do you think my small correction from above can be applied as it is without major logging revamp? |
I am really sorry. I apologize for my rudeness. It should be my responsibility for not checking the code before merging. |
No worries @m13253, I can contribute a patch for that if you like :) although I would expect some feedback to decide exactly what needs to be logged verbosely or not. Edit: PR created: #107 @pfactum quoting you:
Your reading was insufficient, next time I suggest to read the actual PR you link to. |
Eh? I didn't link that PR. |
I guess it's now fixed? |
Hello.
Currently
dns-over-https
works around somednscrypt-proxy
and old Firefox bugs/features, and each time that happens, a message is printed in the log.If
dns-over-https
is used as a public DNS resolver, that hurts logs a lot:Could you please make that printout depend on
s.conf.Verbose
?Thanks.
The text was updated successfully, but these errors were encountered: