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: Fix several bugs in the TCP code path, add unit tests #10108

Merged
merged 35 commits into from Mar 2, 2021

Conversation

rgacogne
Copy link
Member

Short description

This PR fixes several issues in corner cases, introduced during the refactoring for out-of-order support. It also adds a lot of unit tests for that TCP code, which was planned for 1.7.0 but fast-forwarded because of issues reported with 1.6.0-alpha1.

It also adds a new configuration option, setTCPInternalPipeBufferSize, that can be used to set the size of the buffer of the internal pipe used to pass queries to TCP worker threads, thus allowing a smoother handling of incoming TCP connections peaks.

The diff seems huge but most of that (3200 out of 3900 additions) comes from the new unit tests, which actually amount to more lines of code than the TCP code itself and provides a very nice code coverage (which unfortunately does not prevent bugs from still hiding in a unexpected state.. ;-)).

One caveat is that this drops source selection support other than via SO_BINDTODEVICE, because having to pass the source interface name and index down to the actual sendmsg code is very annoying. Perhaps we should at least look into supporting IP_SENDIF in addition to SO_BINDTODEVICE?

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)

@omoerbeek
Copy link
Member

omoerbeek commented Feb 24, 2021

Nice to see more general TCP FastOpen support. I'm a bit worried about the source binding changes as you are relying on specific functions. Both alternatives are not generally available.

@omoerbeek
Copy link
Member

Nice to see more general TCP FastOpen support. I'm a bit worried about the source binding changes as you are relying on specific functions. Both alternatives are not generally available.

Thinking about this a bit more, the old-fashioned way of binding to a source address before connect should still work, right?

pdns/tcpiohandler.hh Outdated Show resolved Hide resolved
pdns/tcpiohandler.hh Outdated Show resolved Hide resolved
@rgacogne
Copy link
Member Author

Thinking about this a bit more, the old-fashioned way of binding to a source address before connect should still work, right?

Yes, that only concerns source interface selection, sorry!

@omoerbeek
Copy link
Member

omoerbeek commented Feb 24, 2021

Something to investigate: I did not have TCP_FASTOPEN_CONNECT, but it seems I did not get the fallback to MSG_FASTOPEN either.

The fallback to MSG_FASTOPEN if the include is not there is enabled, but does not seem tot work as intended for sdig using the write call.

@omoerbeek omoerbeek closed this Feb 24, 2021
@omoerbeek omoerbeek reopened this Feb 24, 2021
@rgacogne
Copy link
Member Author

Something to investigate: I did not have TCP_FASTOPEN_CONNECT, but it seems I did not get the fallback to MSG_FASTOPEN either.

The fallback to MSG_FASTOPEN if the include is not there is enabled, but does not seem tot work as intended for sdig using the write call.

Fixed the detection of TCP_FASTOPEN_CONNECT and the handling of MSG_FASTOPEN in blocking mode (only the non-blocking one was supported). I also added a fastOpen option to sdig.

@omoerbeek
Copy link
Member

omoerbeek commented Feb 24, 2021

Nice, when this is merged, I'll make a PR to change the two write calls in sdig into as single one.

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

FastOpen works now as expected in sdig. I do see two writes in captures, but that is expected. I browsed the rest of the changes, and found nothing surprising.

We need to be careful about the client going away (closes the connection,
for example) while we are sending queued responses.
@rgacogne
Copy link
Member Author

rgacogne commented Mar 2, 2021

Rebased to fix some commits not compiling.

@rgacogne rgacogne merged commit 0b37b97 into PowerDNS:master Mar 2, 2021
@rgacogne rgacogne deleted the ddist-tcp-fixes branch March 2, 2021 12:29
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

2 participants