Skip to content

auth: include serial (that is, SOA record) while sending a notify#16886

Draft
omoerbeek wants to merge 3 commits intoPowerDNS:masterfrom
omoerbeek:auth-notify-with-serial
Draft

auth: include serial (that is, SOA record) while sending a notify#16886
omoerbeek wants to merge 3 commits intoPowerDNS:masterfrom
omoerbeek:auth-notify-with-serial

Conversation

@omoerbeek
Copy link
Member

Short description

This fixes #16129. I'm setting it to Draft status as I:

  1. I'm not at home in the auth code
  2. A test is missing. I went looking to extend an existing test, but failed to find an existing test testing notify. This might be related to point 1.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • read and accepted the Developer Certificate of Origin document, including the AI Policy, and added a "Signed-off-by" to my commits
  • 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)

Untested other than looking at a packet dump

Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>
@omoerbeek omoerbeek changed the title auth: include serial (taht is, SOA recoerd) whil e auth: include serial (that is, SOA record) while sending a notify Feb 16, 2026
Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>
@omoerbeek omoerbeek force-pushed the auth-notify-with-serial branch from fe85284 to 56b9ed7 Compare February 16, 2026 10:58
@coveralls
Copy link

Pull Request Test Coverage Report for Build 22059975061

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • 72 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.01%) to 71.585%

Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.89%
pdns/dnsdistdist/dnsdist-tcp.cc 1 74.68%
pdns/recursordist/aggressive_nsec.cc 2 66.02%
modules/gpgsqlbackend/spgsql.cc 3 68.18%
pdns/axfr-retriever.cc 3 67.47%
pdns/recursordist/recpacketcache.hh 3 91.14%
pdns/recursordist/rec-tcpout.cc 3 82.31%
pdns/dnsdistdist/dnsdist-carbon.cc 4 59.56%
pdns/recursordist/syncres.cc 5 81.58%
pdns/recursordist/test-syncres_cc1.cc 5 79.9%
Totals Coverage Status
Change from base Build 22059357599: -0.01%
Covered Lines: 129592
Relevant Lines: 166000

💛 - Coveralls

@mind04
Copy link
Contributor

mind04 commented Feb 23, 2026

This feels expensive. This will add at least 2 backend lookups per notified host. With three dual stack namservers this wil add 12 backend queries per notify action.
It also it seems to me that the getSOAUncached() call is redundant and the cached version is sufficient. This will improve the numbers somewhat but I still recommend proper performance testing before this is merged. Especially since there is 0 benefit for powerdns itself.

@omoerbeek
Copy link
Member Author

This feels expensive. This will add at least 2 backend lookups per notified host. With three dual stack namservers this wil add 12 backend queries per notify action. It also it seems to me that the getSOAUncached() call is redundant and the cached version is sufficient. This will improve the numbers somewhat but I still recommend proper performance testing before this is merged. Especially since there is 0 benefit for powerdns itself.

Valid point. Looking at the surrounding code, it does not look easy to reduce the number of SOA lookups. I don't think I'll have an opportunity to do measurements any time of soon, so I'll leave it to auth people to either close this or pick it up.

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.

no-serial in notify via API call

3 participants