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

rec: on rhel and debian install recursor.yml default file, but only for new installs #13935

Closed

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Mar 18, 2024

So only if no existing recursor.conf and no recursor.yml was found. This allows:

  • new installes to have a default .yml file.
  • existing .conf install to be untouched
  • existing .yml installs to be left untouched

This needs careful review from somebody familiar with packaging do's and don't s.

Short description

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)

…ig file was found

So only if no recursor.conf and no recursor.yml was found. This allows:

- new installes to have a default .yml file.
- existing .conf install to be untouched
- existing .yml installs to be left untouched

It remains to be seen how we can acheive something similar for Debian.
@omoerbeek omoerbeek added the rec label Mar 18, 2024
@omoerbeek omoerbeek added this to the rec-5.1.0 milestone Mar 18, 2024
@coveralls
Copy link

coveralls commented Mar 18, 2024

Pull Request Test Coverage Report for Build 8340713424

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 45 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.007%) to 59.242%

Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/distributor.hh 2 52.17%
pdns/tsigverifier.cc 3 77.22%
pdns/axfr-retriever.cc 3 65.53%
pdns/dnsdistdist/dnsdist.cc 3 68.2%
pdns/iputils.cc 4 47.09%
pdns/misc.cc 4 61.25%
ext/luawrapper/include/LuaContext.hpp 8 13.96%
modules/godbcbackend/sodbc.cc 17 70.8%
Totals Coverage Status
Change from base Build 8327910185: 0.007%
Covered Lines: 113641
Relevant Lines: 158711

💛 - Coveralls

@@ -106,6 +106,9 @@ getent passwd pdns-recursor > /dev/null || \
exit 0

%post
if [ ! -e %{_sysconfdir}/%{name}/recursor.conf -a ! -e %{_sysconfdir}/%{name}/recursor.yml ]; then
cp %{_sysconfdir}/%{name}/recursor.yml-dist %{_sysconfdir}/%{name}/recursor.yml
Copy link
Member Author

Choose a reason for hiding this comment

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

The cp should probably be an install to make sure the perms and owner/group are right.

@omoerbeek omoerbeek changed the title rec: on rhel install recursor.yml default file, but only for new installs rec: on rhel and debian install recursor.yml default file, but only for new installs Mar 19, 2024
@@ -5,6 +5,9 @@ case "$1" in
configure)
addgroup --system pdns
adduser --system --home /var/spool/powerdns --shell /bin/false --ingroup pdns --disabled-password --disabled-login --gecos "PowerDNS" pdns
if [ ! -e /etc/powerdns/recursor.conf -a ! -e /etc/powerdns/recursor.yml ]; then
cp /etc/powerdns/recursor.yml-dist /etc/powerdns/recursor.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. this leaves recursor.yml-dist in /etc/powerdns with possible user confusion
  2. this doesn't register recursor.yml as a conffile with dpkg (but recursor.yml-dist would be!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at https://manpages.debian.org/testing/dpkg-dev/deb-conffiles.5.en.html I think we're SOL with this approach and instead must install recursor.yml (at build time).
But that also means migrating old installs!

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the issues. I'm hesitant to do an automatic conversion of .conf to .yml. Though the code almost exists in the form of rec_contol show-yaml. That one writes the suggested config to stdout.

Need to come up with something clever, but also robust enough to not mess things up.

@omoerbeek
Copy link
Member Author

It is clear that this is not the right approach. I have an idea though which might work, will investigate that soon.

@omoerbeek omoerbeek closed this May 28, 2024
omoerbeek added a commit to omoerbeek/pdns that referenced this pull request May 29, 2024
This should us to work around the packaging issues discussed in PowerDNS#13935.
THe idea is that modify the parsing so that .conf files also *may* contain YAML.

The search for a config file then becomes:

1. Try read recuror.yml if it exists. If valid, done. If it is invalid punt.
2. Try read recursor.conf as YAML. If it is valid, done.
3. If it is invalid, try to read as old-style.

This means that the status of recursor.conf as a config file does not change.

This allows us to install a default YAML config into recursor.conf for new installs.
Of course we leave recursor.conf (and recursor.yml) alone for existing installs.

This is a draft. I will add docs and packaging changes after this is deemed
the way to proceed.
omoerbeek added a commit to omoerbeek/pdns that referenced this pull request May 31, 2024
This should us to work around the packaging issues discussed in PowerDNS#13935.
THe idea is that modify the parsing so that .conf files also *may* contain YAML.

The search for a config file then becomes:

1. Try read recuror.yml if it exists. If valid, done. If it is invalid punt.
2. Try read recursor.conf as YAML. If it is valid, done.
3. If it is invalid, try to read as old-style.

This means that the status of recursor.conf as a config file does not change.

This allows us to install a default YAML config into recursor.conf for new installs.
Of course we leave recursor.conf (and recursor.yml) alone for existing installs.

This is a draft. I will add docs and packaging changes after this is deemed
the way to proceed.
omoerbeek added a commit to omoerbeek/pdns that referenced this pull request Jun 3, 2024
This should us to work around the packaging issues discussed in PowerDNS#13935.
THe idea is that modify the parsing so that .conf files also *may* contain YAML.

The search for a config file then becomes:

1. Try read recuror.yml if it exists. If valid, done. If it is invalid punt.
2. Try read recursor.conf as YAML. If it is valid, done.
3. If it is invalid, try to read as old-style.

This means that the status of recursor.conf as a config file does not change.

This allows us to install a default YAML config into recursor.conf for new installs.
Of course we leave recursor.conf (and recursor.yml) alone for existing installs.

This is a draft. I will add docs and packaging changes after this is deemed
the way to proceed.
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.

None yet

3 participants