rec: dedup records #14617

omoerbeek · 2024-09-03T11:31:03Z

Short description

This deduplicaties records in two places:

Records received from authoritative servers (disabled later and replaces by specific dedup call)
Records sent out to clients (only on non-packet cache hits) (disabled later).

When testing this, I encountered a few cases where auths sent duplicate records.

dig @ns4.dnsv5.com getui.com txt 
dig @ord.geons.ftw.jiveip.net api.geoip.jive.com txt
dig @dnsd10.chatango.com chatango.com
dig @ns.sotoon53.com yektanet.com txt
dig @ns.viettelidc.com.vn ns9.viettelidc.com.vn
dig @ns1.cdnetdns.net flypgs.com txt

Nothing actually breaks if we don't dedup afaik. So it is questionable of we want this part.

But on the client side, this fixes #14120 and maybe other cases I do not know.

The big question is if we want this. Dedupping is fundamentally not cheap, although I tried to optimize the dedup code.

Originally I played with the idea to change the data structure building the reply vector to avoid duplicates, but that requires changes in many places. Still the idea is not completely off the table.

On the receiving side the dedup internals could also be weaved into the sanitize code, at the cost of increasing complexity. That would avoid the separate dedup() call.

This will remain a draft until I have some some speed measurements and pondered the alternative approaches some more. This PR is mainly to share thoughts.

The test changes are needed as a few of them use duplicate records.

Update: I removed the general dedup calls and I'm now only using dedup where it makes sense to solve a specific case of potential duplciate records. The general purpose (quite performant) dedup code remains and is used, in one case replacing existing ad-hoc dedup code.

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)

coveralls · 2024-09-03T12:07:26Z

Pull Request Test Coverage Report for Build 12705938523

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

75 of 75 (100.0%) changed or added relevant lines in 8 files are covered.
41 unchanged lines in 12 files lost coverage.
Overall coverage decreased (-0.009%) to 64.835%

Files with Coverage Reduction	New Missed Lines	%
modules/gpgsqlbackend/gpgsqlbackend.cc	1	88.62%
pdns/misc.cc	1	64.74%
pdns/validate.cc	1	68.24%
pdns/backends/gsql/gsqlbackend.hh	1	97.71%
pdns/pollmplexer.cc	1	83.66%
pdns/packethandler.cc	3	72.48%
pdns/iputils.cc	3	55.91%
pdns/dnsrecords.cc	3	80.2%
pdns/recursordist/rec-main.cc	3	62.4%
pdns/recursordist/pdns_recursor.cc	4	72.42%

Totals
Change from base Build 12690664929:	-0.009%
Covered Lines:	126273
Relevant Lines:	163958

💛 - Coveralls

rgacogne

The logic looks good to me. I guess we could try a few more heuristics before actually serializing the content, like checking if we have several records with the same (qtype, qname), but it might not be worth it. Having real-world numbers would indeed be useful.

pdns/dnsparser.hh

pdns/shuffle.hh

omoerbeek · 2024-10-25T06:50:29Z

Rebased to fix conflict.

omoerbeek · 2024-10-25T07:12:41Z

Some observations:
quad1 does dedup in some cases (only when the dups are adjacent?)
quad8 does not dedup
OpenDNS does dedup

Software tested in default config
unbound does not do dedup
bind does dedup
knot-resolver does dedup

omoerbeek · 2024-10-25T08:50:34Z

Speedtest results:

'2 DedupRecords (generate only)' 0.10 seconds: 1230169.1 runs/s, 0.81 us/run
'2 DedupRecords' 0.10 seconds: 504897.9 runs/s, 1.98 us/run
'2 DedupRecords (with dup)' 0.10 seconds: 628577.4 runs/s, 1.59 us/run
'256 DedupRecords (generate only)' 0.10 seconds: 9248.3 runs/s, 108.13 us/run
'256 DedupRecords' 0.10 seconds: 3867.5 runs/s, 258.57 us/run
'256 DedupRecords (with dup)' 0.10 seconds: 3823.4 runs/s, 261.55 us/run
'4096 DedupRecords (generate only)' 0.11 seconds: 561.7 runs/s, 1780.35 us/run
'4096 DedupRecords' 0.10 seconds: 241.7 runs/s, 4137.49 us/run
'4096 DedupRecords (with dup)' 0.10 seconds: 239.3 runs/s, 4178.26 us/run

The measured slowdown is about 2.5 and is uniform over the various test case sizes.

So the dedupping takes time as expected, but for the already pretty extreme case of 256, records, its absolute value is not a lot compared to the expected network latency. For the 4096 case we spent time that comes closer to the expected network latency.

paddg · 2024-10-25T08:56:19Z

Can we rule out that deduping is an attack vector?

omoerbeek · 2024-10-25T09:05:21Z

Can we rule out that deduping is an attack vector?

Not completely, in the extreme case spending even a few CPU ms on a single auth result is quite a lot.

paddg · 2024-10-25T10:00:48Z

Not completely, in the extreme case spending even a few CPU ms on a single auth result is quite a lot.

Are you considering an on/off switch for it?

omoerbeek · 2024-10-25T10:02:15Z

Not completely, in the extreme case spending even a few CPU ms on a single auth result is quite a lot.

Are you considering an on/off switch for it?

Yes, that would be one of the options. Another alternative would be to not do the dedupping on large answers as we already refuse to cache them anyway.

omoerbeek · 2024-11-04T08:23:40Z

The logic looks good to me. I guess we could try a few more heuristics before actually serializing the content, like checking if we have several records with the same (qtype, qname), but it might not be worth it. Having real-world numbers would indeed be useful.

I played a bit with a pre-scan on qtype and name only, but saw no speedup

…ows for an unordered_set as well.

rgacogne

The general logic and the places were it is applied look good to me.

rgacogne · 2024-12-24T09:03:08Z

pdns/recursordist/pdns_recursor.cc

@@ -1527,6 +1512,9 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi
      }

      if (!ret.empty()) {
+#ifdef notyet


This might warrant a comment, at the very least :)

pdns/shuffle.cc

rgacogne · 2024-12-24T09:12:28Z

pdns/speedtest.cc

+    std::vector<DNSRecord> vec;
+    vec.reserve(d_howmany);
+    std::string name("some.name.in.some.domain");
+    auto count = d_howmany;
+    if (d_withdup) {
+      count--;
+    }
+    for (size_t i = 0; i < count; i++) {
+      auto content = DNSRecordContent::make(QType::TXT, QClass::IN, "\"a text " + std::to_string(i) + "\"");
+      DNSRecord rec(name, content, QType::TXT);
+      if (i == 0 && d_withdup) {
+        vec.emplace_back(rec);
+      }
+      vec.emplace_back(std::move(rec));
+    }


Would it make sense to do this in the constructor instead, to get the initial steps out of the timed computation?

I initially did not do that as operator()() must be const, and dedup modifies the vector. A middle ground would be to copy the vec constructed in the ct and operate on that one.

Oh, right! The middle ground looks good indeed.

The new version looks good, note that I still have a few comments that have not been addressed as far as I can tell.

Hmm, I could have sworn I did the other ones as well, will fix.

omoerbeek added the rec label Sep 3, 2024

rgacogne reviewed Oct 7, 2024

View reviewed changes

pdns/dnsparser.hh Show resolved Hide resolved

pdns/shuffle.hh Outdated Show resolved Hide resolved

omoerbeek self-assigned this Oct 25, 2024

omoerbeek force-pushed the rec-dedup-recs branch from 863fd47 to 8089aeb Compare October 25, 2024 06:50

omoerbeek force-pushed the rec-dedup-recs branch from 1de6a90 to 5a98b0f Compare October 25, 2024 08:34

omoerbeek force-pushed the rec-dedup-recs branch from 080d63b to 97795db Compare October 25, 2024 09:36

omoerbeek force-pushed the rec-dedup-recs branch 2 times, most recently from 274b82b to e26c334 Compare November 19, 2024 11:47

omoerbeek added this to the rec-5.3.0 milestone Dec 16, 2024

omoerbeek added 9 commits December 16, 2024 11:28

rec: dedup results from auths and results constructed ourselves

31767d5

No need to dedup the dns64 case seperately anymore

71a6d88

Adapt test to not use repeating records

4136f5d

Rework dedup code and add a test for pdsn::dedup

053da7f

Faster dedup, not using zoneRepresentation but wire format, which all…

75258d5

…ows for an unordered_set as well.

Add speedtest for shuffle, plus a speedup in shuffle itself

0b3735a

rename pdns::shuffle to pdns::shufleRecords, as suggested by @rgacogne

15b3406

Refactor serialize/wireFormatContent as suggested by @rgacogne

f3d1ab1

Dedup only in specific places

a5962f6

omoerbeek force-pushed the rec-dedup-recs branch from e26c334 to a5962f6 Compare December 16, 2024 11:13

omoerbeek requested a review from rgacogne December 16, 2024 11:25

omoerbeek marked this pull request as ready for review December 16, 2024 11:25

rgacogne requested changes Dec 24, 2024

View reviewed changes

omoerbeek requested a review from rgacogne January 7, 2025 09:29

omoerbeek force-pushed the rec-dedup-recs branch from 9046cd0 to bcf9b97 Compare January 10, 2025 08:23

Separate speedtest setup, process review comments

0941cf9

omoerbeek force-pushed the rec-dedup-recs branch from bcf9b97 to 0941cf9 Compare January 10, 2025 08:29

rgacogne approved these changes Jan 10, 2025

View reviewed changes

omoerbeek merged commit 1ec2bb1 into PowerDNS:master Jan 10, 2025
79 checks passed

omoerbeek deleted the rec-dedup-recs branch January 10, 2025 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rec: dedup records #14617

rec: dedup records #14617

omoerbeek commented Sep 3, 2024 •

edited

Loading

coveralls commented Sep 3, 2024 •

edited

Loading

rgacogne left a comment

omoerbeek commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

paddg commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

paddg commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

omoerbeek commented Nov 4, 2024

rgacogne left a comment

rgacogne Dec 24, 2024

rgacogne Dec 24, 2024

omoerbeek Jan 7, 2025 •

edited

Loading

rgacogne Jan 7, 2025

rgacogne Jan 9, 2025

omoerbeek Jan 10, 2025

rec: dedup records #14617

rec: dedup records #14617

Conversation

omoerbeek commented Sep 3, 2024 • edited Loading

Short description

Checklist

coveralls commented Sep 3, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12705938523

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

rgacogne left a comment

Choose a reason for hiding this comment

omoerbeek commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

paddg commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

paddg commented Oct 25, 2024

omoerbeek commented Oct 25, 2024

omoerbeek commented Nov 4, 2024

rgacogne left a comment

Choose a reason for hiding this comment

rgacogne Dec 24, 2024

Choose a reason for hiding this comment

rgacogne Dec 24, 2024

Choose a reason for hiding this comment

omoerbeek Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

rgacogne Jan 7, 2025

Choose a reason for hiding this comment

rgacogne Jan 9, 2025

Choose a reason for hiding this comment

omoerbeek Jan 10, 2025

Choose a reason for hiding this comment

omoerbeek commented Sep 3, 2024 •

edited

Loading

coveralls commented Sep 3, 2024 •

edited

Loading

omoerbeek Jan 7, 2025 •

edited

Loading