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: Make DNSDist XFR aware when transfer is finished #10436

Closed
slowr opened this issue May 21, 2021 · 9 comments · Fixed by #10489
Closed

dnsdist: Make DNSDist XFR aware when transfer is finished #10436

slowr opened this issue May 21, 2021 · 9 comments · Fixed by #10489

Comments

@slowr
Copy link
Contributor

slowr commented May 21, 2021

  • Program: dnsdist
  • Issue type: Feature request

Short description

As of now, DNSDist doesn't know when an XFR transfer is finished or still ongoing. The issue is that it keeps the connection open until there is a TCP timeout from either end. This behavior can end up using all TCP connections as they are unavailable for reusing when a connection is doing an XFR.

Another issue is that all these XFRs timeout and generate tcp_gaveup's which can hide actual TCP issues.

Usecase

Have multiple XFRs without running out of TCP connections waiting for TCP timeouts.

Description

It would be great to follow RFC and parse the responses from the DNS server in order to indicate if an XFR has finished or not.

There are 2 possible scenarios:

  1. The rcode is non-zero which indicates an error
  2. First RR's SOA is repeated which means that we finished the transfer.

When the XFR is finished we can re-purpose the connection back to the pool for re-use from other workers.

An example approach can be:

@@ -461,7 +462,35 @@ IOState TCPConnectionToBackend::handleResponse(std::shared_ptr<TCPConnectionToBa
     TCPResponse response;
     response.d_buffer = std::move(d_responseBuffer);
     response.d_connection = conn;
-    clientConn->handleXFRResponse(clientConn, now, std::move(response));
+    try {
+      MOADNSParser parser(true, reinterpret_cast<const char*>(response.d_buffer.data()), response.d_buffer.size());
+      clientConn->handleXFRResponse(clientConn, now, std::move(response));
+
+      if (parser.d_header.rcode != 0U) {
+        DEBUGLOG("non-zero rcode on XFR");
+        conn->d_usedForXFR = false;
+        d_clientConn->d_isXFR = false;
+        d_state = State::idle;
+        d_clientConn.reset();
+        return IOState::Done;
+      }
+
+      // check if XFR has finished
+      if (parser.d_header.ancount > 0U) {
+        unsigned lastAnswerIdx = parser.d_header.ancount - 1U;
+        uint16_t lastAnswerType = parser.d_answers.at(lastAnswerIdx).first.d_type;
+        if (lastAnswerType == QType::SOA) {
+          DEBUGLOG("finished XFR");
+          conn->d_usedForXFR = false;
+          d_clientConn->d_isXFR = false;
+          d_state = State::idle;
+          d_clientConn.reset();
+          return IOState::Done;
+        }
+      }
+    } catch (MOADNSException &e) {
+      DEBUGLOG("Exception when parsing TCPResponse to DNS: " << e.what());
+    }
     d_state = State::waitingForResponseFromBackend;
     d_currentPos = 0;
     d_responseBuffer.resize(sizeof(uint16_t));

The example does not cover the possibility of IXFR or multiple messages that have a single RR each. The accepted solution should instead parse the serial from first SOA, store it in the connection metadata and then parse the messages until rcode != 0 or last RR is SOA with same serial.

@rgacogne
Copy link
Member

I agree it would be nice, but be aware that we actually had the code to detect the end of a transfer before and it led to several bugs, which is why we removed it. Also note that MOADNSParser is very limited in dnsdist, because we don't want to link the hundred of lines of code needed to parse DNS records in there, so the parsing of SOA records needs to be done the hard way.
The biggest issue is that detecting the end of a IXFR is hard, you need to look at all the records and keep track of the serials you have seen, as the final serial, the one starting the IXFR, can be seen twice after that first record, once for the start of the additions and one for the end of the IXFR. I'm not sure you can assume that the SOA indicating the start of the additions won't be in the last position of the answers of a message, since the RFC states that any ordering or grouping should be handled.

@slowr
Copy link
Contributor Author

slowr commented May 31, 2021

Yes you are right. I didn't think of the IXFR scenario where we receive the same SOA but is not the last record.

How much do you think it will affect performance if we start parsing the SOA packets to handle the IXFR case?

@rgacogne
Copy link
Member

How much do you think it will affect performance if we start parsing the SOA packets to handle the IXFR case?

Provided that we only need to parse IXFR responses, and that even then we only care about the content SOA records, I don't expect any meaningful performance impact. It's more the code to do that parsing that worries me a bit, but I would be happy to provide guidance if anyone is interested in working on that :-)

@slowr
Copy link
Contributor Author

slowr commented Jun 2, 2021

Would love some guidance yes. I started with a small PoC to parse the SOA fields but I wasn't sure how to handle DNSRecordContent to achieve this.

@rgacogne
Copy link
Member

rgacogne commented Jun 3, 2021

I pushed a PoC of how to get the serial from XFR messages here: rgacogne@db2cda1

For the remaining parts, I'd suggest looking at processIXFRRecords() (in pdns/ixfr.cc) and especially pdns/test-ixfr_cc.cc for guidance, but of course feel free to ask for help if needed!

@slowr
Copy link
Contributor Author

slowr commented Jun 4, 2021

Do you thinks something like this makes sense? slowr@078c474

I tried to make the implementation as simple as possible.

@rgacogne
Copy link
Member

rgacogne commented Jun 7, 2021

That looks pretty nice, yes! Two nits:

  • (this one is on me because it was not done properly in my PoC): we should use clientConn instead of d_clientConn
  • the new fields would be better placed below d_currentQueriesCount in the struct, to get better padding (thus saving memory). That's not very important since I believe we will need to move these out of that struct it we want to be able to support out-of-order for XFR queries anyway, but that could come later, in a different pull request.

And finally I think pretty much all the code present in the try block could be moved to a different method for readability, but that can also be done later. Did you test that code? Would you be able to write a few regression tests (look at regression-tests.dnsdist/test_AXFR.py for a start)? If the answer is no that's fine, I can add these later :)

@slowr
Copy link
Contributor Author

slowr commented Jun 8, 2021

I cleaned up the code a bit as you suggested slowr@1db2e02

I took a look on the regression tests but I am not sure how can we test if the connection is finished correctly with the new approach.

@rgacogne
Copy link
Member

rgacogne commented Jun 8, 2021

That looks great, thanks a lot, that's very much appreciated! Would you mind opening a pull request? Don't worry about the tests, I will take care of that, especially since it might require of bit of plumbing in the mockup backend code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants