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: Add a new, optional, YAML-based configuration format #14969

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

rgacogne
Copy link
Member

Short description

This PR adds support for a new, optional, YAML-based configuration format. At this point this is still a work in progress!
If the configuration file passed to dnsdist via the -C command-line switch ends in .yml, it is assumed to be in the new YAML format, and an attempt to load a Lua configuration file with the same name but the .lua will be done before loading the YAML configuration. If the names ends in .lua, there will also be an attempt to find a file with the same name but ending in .yml. Otherwise the existing Lua configuration format is assumed.
The parsing of the YAML configuration is done using Serde, so Rust is a requirement for the new configuration format, but the existing Lua configuration format is still supported so DNSdist can still be built without Rust. This might change in the future.

A fair amount of the new code is generated, based on a (YAML) description of the configuration settings, including the selectors and actions composing DNSdist's rules. Lua bindings for a fair amount of supported selectors and actions are now generated as well. My plan is to generate Lua bindings for most of the settings as well in the future.

The documentation of the new configuration format is not yet done, although a fair amount can already be generated from the definitions of settings, selectors and actions.
I have added several regression tests using the new configuration format, so for now it's possible to look at these to get a fell of the new format.

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 Dec 13, 2024

Pull Request Test Coverage Report for Build 12356327481

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

  • 2792 of 4824 (57.88%) changed or added relevant lines in 31 files are covered.
  • 6673 unchanged lines in 105 files lost coverage.
  • Overall coverage decreased (-4.6%) to 60.184%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/dnsdistdist/dnsdist-lua-ffi.cc 6 7 85.71%
pdns/libssl.cc 0 1 0.0%
pdns/dnsdistdist/dnsdist-dnsparser.cc 15 17 88.24%
pdns/qtype.cc 5 7 71.43%
pdns/dnsdistdist/dnsdist-lua.cc 11 14 78.57%
pdns/dnsdistdist/dnsdist-lua-actions-generated.cc 71 75 94.67%
pdns/dnsdistdist/dnsdist-dynblocks.cc 6 11 54.55%
pdns/dnsdistdist/dnsdist-lua-actions.cc 59 64 92.19%
pdns/dnsdistdist/dnsdist-actions-factories-generated.cc 69 75 92.0%
pdns/dnsdistdist/dnsdist-lua-response-actions-generated.cc 33 39 84.62%
Files with Coverage Reduction New Missed Lines %
pdns/webserver.hh 1 69.16%
pdns/packethandler.cc 1 72.57%
pdns/dnsdistdist/test-dnsdisttcp_cc.cc 1 94.98%
pdns/recursordist/sortlist.hh 1 75.0%
pdns/pollmplexer.cc 1 83.66%
pdns/dnstap.cc 2 68.82%
ext/probds/murmur3.cc 2 88.24%
pdns/sstuff.hh 2 53.78%
pdns/distributor.hh 2 51.86%
pdns/statnode.cc 2 47.55%
Totals Coverage Status
Change from base Build 12316427950: -4.6%
Covered Lines: 116190
Relevant Lines: 160313

💛 - Coveralls

pdns/dnsdistdist/dnsdist-configuration-yaml.cc Dismissed Show dismissed Hide dismissed
pdns/dnsdistdist/dnsdist-configuration-yaml.cc Dismissed Show dismissed Hide dismissed
pdns/dnsdistdist/dnsdist-configuration-yaml.cc Dismissed Show dismissed Hide dismissed
pdns/dnsdistdist/dnsdist-rules-factory.hh Dismissed Show dismissed Hide dismissed
@rgacogne
Copy link
Member Author

@pieterlexis just suggested two ways of supporting Lua functions that I like very much, I'll look into it:

 - name: "Lua Action"
    selector:
      type: "QName"
      qname: "no-backend.doh.tests.powerdns.com."
    action:
      type: "Lua"
      lua: |
        function dohHandler(dq)
          if dq:getHTTPScheme() == 'https' and dq:getHTTPHost() == '%s:%d' and dq:getHTTPPath() == '/' and dq:getHTTPQueryString() == '' then
            local foundct = false
            for key,value in pairs(dq:getHTTPHeaders()) do
              if key == 'content-type' and value == 'application/dns-message' then
                foundct = true
                break
              end
            end
            if foundct then
              dq:setHTTPResponse(200, 'It works!', 'text/plain')
              dq.dh:setQR(true)
              return DNSAction.HeaderModify
            end
          end
          return DNSAction.None
        end
 - name: "Lua Action"
    selector:
      type: "QName"
      qname: "no-backend.doh.tests.powerdns.com."
    action:
      type: "Lua"
      lua-file: "/etc/dnsdist/my-action.lua"

@rgacogne rgacogne force-pushed the ddist-yaml-configuration-harder branch from aab8905 to 5bf9e88 Compare December 23, 2024 16:02
The previous method didn't account for TCP workers for backends.
@rgacogne rgacogne force-pushed the ddist-yaml-configuration-harder branch from 53a5493 to e83c9b1 Compare December 26, 2024 08:56
@rgacogne rgacogne marked this pull request as ready for review December 26, 2024 08:56
@rgacogne
Copy link
Member Author

I cleaned up the commits history so it should be a lot easier to review this PR (which is still a big one, sorry about that).

@rgacogne rgacogne force-pushed the ddist-yaml-configuration-harder branch from fba60a1 to 1d02750 Compare December 27, 2024 13:18
@rgacogne rgacogne force-pushed the ddist-yaml-configuration-harder branch from 970895f to 94bb3ba Compare December 27, 2024 16:31
});
}

static void loadDynamicBlockConfiguration(const dnsdist::rust::settings::DynamicRulesSettingsConfiguration& settings, const ::rust::Vec<dnsdist::rust::settings::DynamicRulesConfiguration>& dynamicRules)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 110 lines.
@rgacogne
Copy link
Member Author

rgacogne commented Jan 7, 2025

@pieterlexis just suggested two ways of supporting Lua functions that I like very much, I'll look into it:

 - name: "Lua Action"
    selector:
      type: "QName"
      qname: "no-backend.doh.tests.powerdns.com."
    action:
      type: "Lua"
      lua: |
        function dohHandler(dq)
          if dq:getHTTPScheme() == 'https' and dq:getHTTPHost() == '%s:%d' and dq:getHTTPPath() == '/' and dq:getHTTPQueryString() == '' then
            local foundct = false
            for key,value in pairs(dq:getHTTPHeaders()) do
              if key == 'content-type' and value == 'application/dns-message' then
                foundct = true
                break
              end
            end
            if foundct then
              dq:setHTTPResponse(200, 'It works!', 'text/plain')
              dq.dh:setQR(true)
              return DNSAction.HeaderModify
            end
          end
          return DNSAction.None
        end
 - name: "Lua Action"
    selector:
      type: "QName"
      qname: "no-backend.doh.tests.powerdns.com."
    action:
      type: "Lua"
      lua-file: "/etc/dnsdist/my-action.lua"

I pushed a commit implementing both the "inline" and the "detached to a file" options. Note that the final naming is slightly different.

@omoerbeek
Copy link
Member

omoerbeek commented Jan 10, 2025

First very early impressions with some random remarks, only looking at a simple config I had laying around, trying to convert it. I did not look at code yet. See comments in yaml below

# setStructuredLogging(true, {timeFormat="ISO8601"}) # no equivalent yaml found

console:
 listen-address: 127.0.0.1:5300
 key: "NtQDMjkGRTIBHJvFsSy/f33C50suAZQRwYWjOznnP+Q="

binds:
  - listen-address: '127.0.0.1:5302'
    protocol: DoH # allowed values absent in docs
    doh:
      paths: ['/dns-query'] # Seems required, opposed to what the docs say
    tls:
      certificates:
      - certificate: fullchain.pem
        key: mini.drijf.net.key
      session-timeout: 30

backends:
  - address: '9.9.9.9'
    protocol: Do53 # allowed values absent in docs
    health-checks:
      mode: lazy # Is this the way? allowed values absent in docs

general:
  verbose: true
  verbose-health-checks: true # I saw no effect

load-balancing-policies:
  default-policy: roundrobin # allowed values absent in docs```

@omoerbeek
Copy link
Member

When I did recursor, I went for underscores in names, instead of hyphens. I was under the impression that most YAML formats use underscores (at least Ansible does)... I wonder if we want to keep this consistent between products. If so, I'm afraid the burden is for you.

@rgacogne
Copy link
Member Author

When I did recursor, I went for underscores in names, instead of hyphens. I was under the impression that most YAML formats use underscores (at least Ansible does)... I wonder if we want to keep this consistent between products. If so, I'm afraid the burden is for you.

I really hate underscores in names. I'll do it if I have to, but that sucks.

@omoerbeek
Copy link
Member

omoerbeek commented Jan 10, 2025

When I did recursor, I went for underscores in names, instead of hyphens. I was under the impression that most YAML formats use underscores (at least Ansible does)... I wonder if we want to keep this consistent between products. If so, I'm afraid the burden is for you.

I really hate underscores in names. I'll do it if I have to, but that sucks.

Oh, I wont insist... there are many existing YAML formats using hyphens

@omoerbeek
Copy link
Member

dnsdist-rust-lib/dnsdist-configuration-yaml-items-generated.cc and dnsdist-rust-lib/rust/src/lib.rs are cleaned by make clean but also in git.

@rgacogne
Copy link
Member Author

When I did recursor, I went for underscores in names, instead of hyphens. I was under the impression that most YAML formats use underscores (at least Ansible does)... I wonder if we want to keep this consistent between products. If so, I'm afraid the burden is for you.

I really hate underscores in names. I'll do it if I have to, but that sucks.

Oh, I wont insist... there are many existing YAML formats using hyphens

No, I think you are right. We need to be consistent across our products. I'm just disappointed because I have never been able to get underscores right and the current naming felt so natural to me, but it is what it is.

@omoerbeek
Copy link
Member

omoerbeek commented Jan 10, 2025

When I did recursor, I went for underscores in names, instead of hyphens. I was under the impression that most YAML formats use underscores (at least Ansible does)... I wonder if we want to keep this consistent between products. If so, I'm afraid the burden is for you.

I really hate underscores in names. I'll do it if I have to, but that sucks.

Oh, I wont insist... there are many existing YAML formats using hyphens

No, I think you are right. We need to be consistent across our products. I'm just disappointed because I have never been able to get underscores right and the current naming felt so natural to me, but it is what it is.

Yeah, the rec ship has sailed...wondering if we migh be able to allow both styles in rec using #[serde(alias = "name")]. Right now it's field name (with underscores of course) == yaml name.

Update: it seems possible with very little effort. Maybe something for the upcoming release cycle.

@rgacogne
Copy link
Member Author

dnsdist-rust-lib/dnsdist-configuration-yaml-items-generated.cc and dnsdist-rust-lib/rust/src/lib.rs are cleaned by make clean but also in git.

I went back and forth quite a bit with these. In the end I think it makes sense to have them in git, and thus to remove them from the clean target. Unless you disagree?

@omoerbeek
Copy link
Member

dnsdist-rust-lib/dnsdist-configuration-yaml-items-generated.cc and dnsdist-rust-lib/rust/src/lib.rs are cleaned by make clean but also in git.

I went back and forth quite a bit with these. In the end I think it makes sense to have them in git, and thus to remove them from the clean target. Unless you disagree?

I'm ok with that. I seem to remember meson has some strict ideas about generating dist tarballs, requiring everything to be in git?

@rgacogne
Copy link
Member Author

dnsdist-rust-lib/dnsdist-configuration-yaml-items-generated.cc and dnsdist-rust-lib/rust/src/lib.rs are cleaned by make clean but also in git.

I went back and forth quite a bit with these. In the end I think it makes sense to have them in git, and thus to remove them from the clean target. Unless you disagree?

I'm ok with that. I seem to remember meson has some strict ideas about generating dist tarballs, requiring everything to be in git?

Yep. I think we have other issues to handle to be able to handle dist tarballs with meson, anyway, but I think this would be a step in the right direction.

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.

3 participants