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: Add support for incoming Proxy Protocol #9616

Merged
merged 13 commits into from Jan 11, 2021

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Oct 15, 2020

Short description

This PR:

  • unifies the way UDP, TCP and DoH deal with incoming data by using a std::vector<uint8_t> everywhere. This makes it possible to pass the size around without any risk of mismatch, allow expanding the buffer at any point, and prevents signedness issues that could arise with signed chars. It uses a new type of vector that does not initialize the memory on a resize, because calling zero-ing a bunch of bytes is expensive when you do it 100k times per second ;
  • implements incoming proxy protocol, making it possible to chain several dnsdist, or simply to pass data to dnsdist from a "smart" client. Incoming data will be forwarded to a downstream server if it supports the proxy protocol as well, although TLV data can of course be inspected and edited from dnsdist.

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)

@rgacogne
Copy link
Member Author

This PR also gets rid of the "keep trailing data" feature, since it does not really make sense to maintain it once we have a way to pass data from a client to the backend.

@rgacogne
Copy link
Member Author

rgacogne commented Nov 2, 2020

Rebased on the squashed version of #9582, and squashed as well.

@rgacogne rgacogne force-pushed the ddist-vectorize branch 2 times, most recently from 46ea0f1 to 5b8c1e7 Compare November 10, 2020 10:44
@rgacogne rgacogne changed the title dnsdist: Add support for incoming Proxy Protocol (WIP) dnsdist: Add support for incoming Proxy Protocol Nov 24, 2020
@rgacogne rgacogne marked this pull request as ready for review November 24, 2020 14:47
@rgacogne
Copy link
Member Author

Rebased to fix a conflict.

@rgacogne
Copy link
Member Author

rgacogne commented Jan 5, 2021

Rebased to fix a conflict.

@rgacogne
Copy link
Member Author

rgacogne commented Jan 6, 2021

Rebased to fix a conflict with the protozero PR.

Reported by cppcheck. Using a proper block function also makes the
code easier to read.
The diff looks huge but that's mostly indentation changes, getting
rid of the changed whitespaces yields a very small diff.
@rgacogne
Copy link
Member Author

Rebased to fix a conflict., plus fixed a few nits reported by cppcheck (try blocks being function's bodies, an uninitialized dnsheader variable in one unit test).

@rgacogne
Copy link
Member Author

@omoerbeek @Habbie I'm quite happy with the dnsdist's parts of this PR, and there are very few changes to code used by the Authoritative server or the Recursor, mostly the templating of DNSPacketWriter to be able to use a vector with a specific allocator (NoInitVector in that code) that does not set the memory to zero when a vector is made bigger via std::vector::resize(). That sounds like a small optimization but does matter in dnsdist because for every UDP query we do resize the vector to the actual number of bytes we received, before resizing it again to a larger capacity for the next query. It takes real CPU time to do that hundred of thousands of times per second and we don't care about that memory being set to zero.
So I'll likely merge this PR once our CI turns green, but please let me know if you would like to skim over the changes first!

@Habbie
Copy link
Member

Habbie commented Jan 11, 2021

Skimmed, please go ahead!

@omoerbeek
Copy link
Member

Skimmed, please go ahead!

Yes. I remember skimming over it earlier (without doing an actual review).

@rgacogne rgacogne merged commit 42bee50 into PowerDNS:master Jan 11, 2021
@rgacogne rgacogne deleted the ddist-vectorize branch January 11, 2021 13:00
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.

None yet

3 participants