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

Allow setting notify_allowed in Recursor API for forwarded zones #14121

Merged
merged 3 commits into from
May 28, 2024

Conversation

bakern
Copy link

@bakern bakern commented May 2, 2024

Closes #14116

Short description

Allows setting notify_allowed when adding forwarded zones through the Recursor API. This works with the new YAML settings and the old-style settings. If notify_allowed is not sent or is set to false, the behavior is unchanged. If notify_allowed is set to true the Recursor will allow NOTIFY to clear cache for the given zone.

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)

@omoerbeek omoerbeek self-assigned this May 6, 2024
@omoerbeek omoerbeek added the rec label May 6, 2024
@omoerbeek omoerbeek added this to the rec-5.1.0 milestone May 6, 2024
@omoerbeek
Copy link
Member

omoerbeek commented May 6, 2024

Hello, thanks for the PR. I'm wondering about the old config case. I do not see the code that handles notify_allowed for that. I'll try to test soon.

@omoerbeek
Copy link
Member

omoerbeek commented May 6, 2024

I was misdiagnosing earlier, there is code to handle notify, also for old-style configs.
Added one commit to make sure notify_allowed flag is also represented in zones objects returned.
Before merging, this PR should get regression tests.

@coveralls
Copy link

coveralls commented May 6, 2024

Pull Request Test Coverage Report for Build 9265491441

Details

  • 4 of 7 (57.14%) changed or added relevant lines in 2 files are covered.
  • 63 unchanged lines in 14 files lost coverage.
  • Overall coverage decreased (-0.02%) to 64.595%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/recursordist/ws-recursor.cc 3 6 50.0%
Files with Coverage Reduction New Missed Lines %
pdns/packethandler.cc 1 72.86%
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/backends/gsql/gsqlbackend.hh 1 97.71%
pdns/pollmplexer.cc 1 83.66%
pdns/signingpipe.cc 2 85.48%
pdns/recursordist/test-rec-tcounters_cc.cc 2 95.74%
pdns/dnsdistdist/dnsdist.cc 2 68.19%
ext/json11/json11.cpp 3 62.72%
pdns/dnsdistdist/dnsdist-carbon.cc 3 61.56%
pdns/recursordist/rec-tcp.cc 4 62.3%
Totals Coverage Status
Change from base Build 9255528069: -0.02%
Covered Lines: 124282
Relevant Lines: 161640

💛 - Coveralls

@bakern
Copy link
Author

bakern commented May 6, 2024

Thanks @omoerbeek!

I compiled and tested with your change, that is very helpful. One last thing I was wondering, should we also add the notify_allowed flag to the /zones endpoint when listing all zones, or is that too much detail for that endpoint?

Before merging, this PR should get regression tests.

Please let me know if there is anything I need to do for this.

@omoerbeek
Copy link
Member

Added simple test and rebased.

@omoerbeek omoerbeek merged commit dbf7064 into PowerDNS:master May 28, 2024
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursor: Allow setting notify_allowed when creating zone with API
4 participants