Skip to content
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

dnsdist: properly set protocol when logging #13716

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

chbruyand
Copy link
Member

@chbruyand chbruyand commented Jan 15, 2024

Short description

This PR properly sets protocol when logging via dnstap or protobuf when using DNS over QUIC and DNS over HTTP/3.

For protobuf, we add a httpVersion field so we can distinguish between DoH and DoHTTP/3 (PowerDNS/dnsmessage#8).
We should eventually do something similar for dnstap if there is a possibility to do so (dnstap/dnstap.pb#20).

Fix #13672 and #13673

Todo:

  • add regression tests
  • should I include the formatting commit ?

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@coveralls
Copy link

coveralls commented Jan 15, 2024

Pull Request Test Coverage Report for Build 7568813501

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 57.698%

Totals Coverage Status
Change from base Build 7545714324: 0.002%
Covered Lines: 107810
Relevant Lines: 155503

💛 - Coveralls

@rgacogne rgacogne self-requested a review January 16, 2024 07:34
@rgacogne rgacogne added this to the dnsdist-1.9 milestone Jan 16, 2024
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

pdns/dnsdist-protobuf.cc Show resolved Hide resolved
@rgacogne
Copy link
Member

* [ ]  should I include the formatting commit ?

If you can find the motivation to fix the clang-tidy warnings resulting from the reformat then yes, please :)

pdns/dnsdist-protobuf.hh Fixed Show resolved Hide resolved
@chbruyand chbruyand force-pushed the dnsdist-protocol-logging branch from 8595da5 to 1baa9a8 Compare January 17, 2024 09:38
@chbruyand chbruyand force-pushed the dnsdist-protocol-logging branch from 0609e65 to 655fe34 Compare January 17, 2024 15:47
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for dealing with the massive pain of delinting these files! I think we need to discuss one clang-tidy rule with the team, so I'll bring it up on the internal chat.

pdns/dnsdist-lua-actions.cc Outdated Show resolved Hide resolved
pdns/dnsdist-lua-actions.cc Outdated Show resolved Hide resolved
pdns/dnsdist-protobuf.hh Fixed Show resolved Hide resolved
pdns/dnsdist-protobuf.hh Outdated Show resolved Hide resolved
pdns/dnsname.hh Outdated Show resolved Hide resolved
@chbruyand
Copy link
Member Author

Thanks a lot for dealing with the massive pain of delinting these files! I think we need to discuss one clang-tidy rule with the team, so I'll bring it up on the internal chat.

I reverted the const member related changes and the dnsname.{cc,hh} files.

Sorry I've gone a bit wild with clang-tidy.

@chbruyand
Copy link
Member Author

chbruyand commented Jan 18, 2024

There is still one clang-tidy warning left:

pdns/dnsdist-lua-actions.cc:945 narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to signed type 'int' is implementation-defined (bugprone-narrowing-conversions)

This should be fixed with the modification made in:
#13723

The above fix might have some collateral damages, so I purpose to ignore that warning for the scope of this PR.

@chbruyand chbruyand marked this pull request as ready for review January 18, 2024 13:08
@rgacogne rgacogne merged commit 0d2aa6c into PowerDNS:master Jan 18, 2024
74 of 75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dnsdist: No responses when DNStap is enabled with DoH3 and DoQ
3 participants