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

[lmdb] Ignore unreachable TSIG keys in getTSIGKeys #15004

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miodvallat
Copy link
Contributor

Short description

As reported in #14779, pre-v5 database schema would (incorrectly) allow for multiple TSIG keys to be created with the same name and algorithm. Once the database gets converted to v5 schema, those redundant keys can no longer be used, yet would appear in pdnsutil list-tsig-key output.

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
Copy link

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 12631856897

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.

Details

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9579 unchanged lines in 172 files lost coverage.
  • Overall coverage increased (+4.6%) to 64.844%

Files with Coverage Reduction New Missed Lines %
pdns/dnsdistdist/dnsdist-session-cache.cc 1 62.86%
pdns/auth-catalogzone.hh 1 66.67%
pdns/recursordist/nod.hh 1 92.59%
modules/lmdbbackend/lmdbbackend.hh 1 90.0%
ext/lmdb-safe/lmdb-typed.cc 1 89.66%
pdns/ws-auth.cc 1 80.9%
pdns/test-dnsrecords_cc.cc 2 95.97%
pdns/epollmplexer.cc 2 85.0%
pdns/recursordist/negcache.hh 2 88.24%
pdns/dnssecinfra.cc 2 76.54%
Totals Coverage Status
Change from base Build 12589349850: 4.6%
Covered Lines: 126159
Relevant Lines: 163777

💛 - Coveralls

@miodvallat miodvallat force-pushed the old_lmdb_schema_leftovers branch from f2d98aa to bf60067 Compare January 6, 2025 11:17
// results of this method.
// In order to prevent this, we first only gather the list of key names, and
// in a second step, query for them using a similar logic as getTSIGKey().
// Unfortunately, there does not seem to be a way to know if the database had
Copy link
Member

Choose a reason for hiding this comment

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

while I hope we never have an incident like this again, this makes me wonder if we should have a way to know. Also, we could try to add a cleanup for this to the v6 migration once we get to it.

Copy link
Member

Choose a reason for hiding this comment

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

.. in fact I think we could add a cleanup command to pdnsutil backend-cmd lmdb .. today if we wanted to

modules/lmdbbackend/lmdbbackend.cc Outdated Show resolved Hide resolved
modules/lmdbbackend/lmdbbackend.cc Outdated Show resolved Hide resolved
Pre-v5 database schema would (incorrectly) allow for multiple TSIG
keys to be created with the same name and algorithm. Once the database
gets converted to v5 schema, those redundant keys can no longer be used,
yet would appear in pdnsutil list-tsig-key output.

Change the list logic to only report reachable keys.

Fixes 14779
@miodvallat miodvallat force-pushed the old_lmdb_schema_leftovers branch from bf60067 to deff288 Compare January 6, 2025 11:41
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.

5 participants