Skip to content

Bind-style views #15512

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

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft

Bind-style views #15512

wants to merge 30 commits into from

Conversation

miodvallat
Copy link
Contributor

Short description

This is a technology pre(view) of a maturing work in progress to implement Bind-style "views" in PowerDNS. In other words, this is a candidate for 5.0.0-alpha.

This feature is currently limited to the LMDB backend, and will require a database update; existing pdns_server will not be able to use the LMDB database once it has been touched by a server running this PR.

All DNS operations (including DNSSEC) ought to work on views, and there should be no regression on existing, views-unaware, setups.

Please refer to the documentation updates in this PR (especially docs/views.rst) for more information and example setups.

There are still debug output (with VIEWS_DEBUG markers for easy removal), and open questions or would-be issues (with FIXME or TODO markers, how unsurprising). We (@Habbie and I) intend to address these before the beta release. In the meantime this gives you a view (pun intended) of what we have been working on.

Tests and feedback welcome.

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)
  • checked a "full house" set of checkboxes


do {
try {
auto [view, _zone] = splitField(key.getNoStripHeader<string>(), '\x0');

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable view is not used.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 14754995094

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

  • 362 of 617 (58.67%) changed or added relevant lines in 14 files are covered.
  • 4226 unchanged lines in 76 files lost coverage.
  • Overall coverage decreased (-2.3%) to 61.303%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/communicator.hh 0 1 0.0%
pdns/test-ueberbackend_cc.cc 0 1 0.0%
pdns/auth-packetcache.cc 59 61 96.72%
pdns/dnsbackend.cc 4 8 50.0%
pdns/dnssecsigner.cc 0 5 0.0%
pdns/test-packetcache_cc.cc 91 98 92.86%
pdns/dnsbackend.hh 0 16 0.0%
pdns/iputils.hh 15 32 46.88%
pdns/auth-zonecache.cc 52 86 60.47%
pdns/test-auth-zonecache_cc.cc 68 104 65.38%
Files with Coverage Reduction New Missed Lines %
ext/yahttp/yahttp/cookie.hpp 1 2.7%
ext/yahttp/yahttp/reqresp.cpp 1 47.34%
pdns/base64.cc 1 80.6%
pdns/json.hh 2 0.0%
pdns/query-local-address.cc 2 89.36%
pdns/auth-packetcache.hh 3 85.0%
pdns/dnssecinfra.hh 3 65.52%
pdns/ednssubnet.hh 3 85.71%
pdns/nameserver.hh 3 0.0%
pdns/recursordist/test-syncres_cc2.cc 3 88.91%
Totals Coverage Status
Change from base Build 14753779081: -2.3%
Covered Lines: 108871
Relevant Lines: 143365

💛 - Coveralls

@hlindqvist
Copy link
Contributor

Tests and feedback welcome.

Some immediate thoughts after reading docs/views.rst, based on previous experience with views :

  • How are views matched exactly? If a "BIND-style" mindset is to be applied, the ordering of the views is absolutely crucial, and some kind of view index, as well as reordering operations would be needed. If a "view order" is not a concept, how are overlapping views (in terms of matching networks) handled in a predictable manner? Or is there validation to ensure that views cannot overlap (otoh that may cause more problems than it solves)?
  • You'll probably want non IP-based matching options, like TSIG, and enough flexibility to express that "not key2 but key1" should go to one view and "not key1 but key2" to another view. This is particularly important for XFR in a views-based setup.
  • While I suppose it adds another layer of complexity, I do like the "zone variant" concept as it both allows putting different variants in different views as well as the same variant in multiple views.

Any plans on interacting with these mechanisms from dnsdist? (That was my immediate thought with a "PowerDNS-style" mindset)

@miodvallat
Copy link
Contributor Author

* How are views matched exactly? If a "BIND-style" mindset is to be applied, the ordering of the views is absolutely crucial, and some kind of view index, as well as reordering operations would be needed. If a "view order" is not a concept, how are overlapping views (in terms of matching networks) handled in a predictable manner? Or is there validation to ensure that views cannot overlap (otoh that may cause more problems than it solves)?

Views are set up by netmasks (base network address + prefix length), the narrowest netmask wins. There is no ordering concept.

* You'll probably want non IP-based matching options, like TSIG, and enough flexibility to express that "not key2 but key1" should go to one view and "not key1 but key2" to another view. This is particularly important for XFR in a views-based setup.

TSIG-based selection of views is planned, but not in the initial Views release, more likely in 5.1 or 5.2.

Any plans on interacting with these mechanisms from dnsdist? (That was my immediate thought with a "PowerDNS-style" mindset)

I don't know!

@hlindqvist
Copy link
Contributor

* How are views matched exactly? If a "BIND-style" mindset is to be applied, the ordering of the views is absolutely crucial, and some kind of view index, as well as reordering operations would be needed. If a "view order" is not a concept, how are overlapping views (in terms of matching networks) handled in a predictable manner? Or is there validation to ensure that views cannot overlap (otoh that may cause more problems than it solves)?

Views are set up by netmasks (base network address + prefix length), the narrowest netmask wins. There is no ordering concept.

Ok, that sounds sensible.

I think I would just wish for the matching behavior to be explicitly stated in the documentation; since while the approach makes sense it's also different from what someone who is used to BIND views would be used to.

* You'll probably want non IP-based matching options, like TSIG, and enough flexibility to express that "not key2 but key1" should go to one view and "not key1 but key2" to another view. This is particularly important for XFR in a views-based setup.

TSIG-based selection of views is planned, but not in the initial Views release, more likely in 5.1 or 5.2.

Ok. As long as it's an expected (and preferably documented?) limitation that operations other than plain queries, like AXFR, UPDATE, etc have limited compatibility with a views-based setup, I guess it's fine if that aligns with priorities.
(Yes, I expect that you can AXFR, UPDATE and whatever else technically, but it seems near useless based on how you would want to match views for those operations in common real-world scenarios)

That's essentially my reaction based on "All DNS operations (including DNSSEC) ought to work on views", which I may well have read too much into.

Any plans on interacting with these mechanisms from dnsdist? (That was my immediate thought with a "PowerDNS-style" mindset)

I don't know!

Ok. I think it would probably make sense (at some point) from a PowerDNS ecosystem perspective to be have some means of interacting with specific views also from dnsdist. (Maybe by a proxyv2 tag picking a view or something like that?)

@omoerbeek
Copy link
Member

omoerbeek commented Apr 30, 2025

Ok. I think it would probably make sense (at some point) from a PowerDNS ecosystem perspective to be have some means of interacting with specific views also from dnsdist. (Maybe by a proxyv2 tag picking a view or something like that?)

I am pondering to use several backends in dnsdist, connecting to the same auth IP, but with a different source address. Then auth can use the source address of the incoming request to select a view in the already existing way.

newServer({address = authIP, pool="A", source=localIP1}
newServer({address = authIP, pool="B", source=localIP2}

And the use rules to assign traffic to pool A or B. Note that I did not try this yet.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

will write more soon

@hlindqvist
Copy link
Contributor

Ok. I think it would probably make sense (at some point) from a PowerDNS ecosystem perspective to be have some means of interacting with specific views also from dnsdist. (Maybe by a proxyv2 tag picking a view or something like that?)

I am pondering to use several backends in dnsdist, connecting to the same auth IP, but with a different source address. Then auth can use the source address of the incoming request to select a view in the already existing way.

newServer({address = authIP, pool="A", source=localIP1}
newServer({address = authIP, pool="B", source=localIP2}

And the use rules to assign traffic to pool A or B. Note that I did not try this yet.

Yes, that really ought to work as long as you don't actually need the client address.

But if you do need the client address, presumably you are already doing proxyv2 or something like that, and pdns will do its IP matching based on the proxyv2 source address, not the dnsdist source address, which is where I arrived at maybe pdns could also have for example proxyv2 KV-data as another option for matching.

@mind04
Copy link
Contributor

mind04 commented May 2, 2025

May I suggest '.' as view separator so a zone with a view will look something like this:

example.com..variant

The benefit of this notation over the currently used ':' is that it is impossible put a view string with a '..' in it in a dnsname. So older pdns versions without views will automatically reject the new zone with a view format.

[mind04@playground ~]$ pdnsutil show-zone example.com..variant
Error: Found . in wrong position in DNSName: example\.com\.\.variant
[mind04@playgorund ~]$ pdnsutil show-zone example.com:variant
No such zone in the database

@miodvallat
Copy link
Contributor Author

May I suggest '.' as view separator so a zone with a view will look something like this:

Hey, that's pretty smart! I'll rework the code to work this way.

@miodvallat
Copy link
Contributor Author

Updated to use .. as variant prefix and incorported @Habbie's cleanups of last week. Should be in a reviewable state soon.

@miodvallat miodvallat force-pushed the blinds branch 4 times, most recently from 1f03619 to f6221de Compare May 5, 2025 12:03
@miodvallat
Copy link
Contributor Author

...and now the documentation should match the new variant syntax as well.

@miodvallat miodvallat force-pushed the blinds branch 2 times, most recently from 7d5b52c to 8e8368a Compare May 5, 2025 15:03
Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

packetcache changes look good

// - if we pass an address, only consider entries with a netmask matching that address
// - if we don't pass an address, only consider entries with no netmask
if (from != nullptr) {
if (!iter->netmask || !iter->netmask->match(*from)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should continue searching for the longest match. I also still don't rule out that we'll be redoing this code with more knowledge of the configured networks at some point. But for now this will certainly do.

Copy link
Member

Choose a reason for hiding this comment

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

and if I'm getting this right, we currently scale in O(n) for the number of netmask-differentiated answers to a question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes (assuming we have cached a packet for all of these networks). Which makes me think that we should probably advise, in the documentation, views users to increase the packet cache size because of this behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, and note the runtime performance implications on a TODO i guess

ZoneName zonename = apiNameToZoneName(stringFromJson(document, "name"));

if (!backend.getDomainInfo(zonename, domainInfo)) {
throw HttpNotFoundException(); // zone name not found
Copy link
Member

Choose a reason for hiding this comment

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

404 feels like the wrong code for the -zone- not being an existing thing

miodvallat and others added 28 commits May 7, 2025 15:04
Make the zone cache check for a possible view (based on the originating
network address) and make it mutate its incoming ZoneName to fill the variant
part if found and applicable.
Allow updates from the http api.

Add zonecache unit test for views.
Note that this causes somewhat important plumbing changes, getSOA will now
take an optional zone ID, which callers should provide if they know it, in
order to save a possible expensive getDomainInfo call.
Annotate use of UnknownDomainID when it's safe to use.
Use this in order to produce the correct RRSIGs and DNSKEYs.
I can't believe I missed this when introducing variants.

And of course, this causes subtle breakage due to the way the zone cache
fills in the variant; add an ugly workaround for now, before going back
to the drawing board.
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.

6 participants