Allow server and pool sources to be overridden through a conf.d or sources.d configuration

[Ankush Pathak]

[Impact]
Currently, the default chrony.conf configures a set of pools. If a user wishes to use only a specific server/server pool or not use the default servers at all, they will need to modify /etc/chrony/chrony.conf. This will possibly lead to a prompt during an Ubuntu release upgrade and during an unattended chrony security upgrade.
There is an effort to move all configuration changes to their respective *.d directories. See: https://bugs.launchpad.net/livecd-rootfs/+bug/1968873. This is also aimed at potentially supporting automated distro upgrades.
CPC test for modified ucf tracked chrony config file by invoking `sudo md5sum --quiet --check /var/lib/ucf/hashfile`. This test fails at the moment due to required changes made at image build-time or during instance boot to chrony time source in /etc/chrony/chrony.conf configuration.

Listing the cases where moving required chrony configuration changes to a *.d config is not possible
1. Azure: Azure needs all default pool entries in chrony.conf disabled. This is currently done by commenting out the pool entries in /etc/chrony/chrony.conf. There doesn't seem to be an alternative way to reset the pool set used by chrony through a configuration in *.d directory.
2. Google: GCP images need to set a single server source entry. This is done indirectly through the ntp cloud-init module configuration. The ntp module replaces the default /etc/chrony/chrony.conf with another file that has required server entry and no pool entries. This cannot be done through an override in *.d directory without touching /etc/chrony/chrony.conf.

[Workaround]
No known workaround

[Test Case]
* Upgrade a GCE Ubuntu instance from any supported release to Noble and the upgrade produces a chrony.conf ucf prompt.
* Run `sudo md5sum --quiet --check /var/lib/ucf/hashfile` on a GCE noble instance.

[Where Problems Could Occur]
A case where debconf configuration that overrides default values prevents package installation scripts from configuring any chrony time sources.
This unlikely to happen without user intention in non-cloud environments.
Some cloud image builds are likely to do this on purpose to be able to configure cloud provider appropriate time sources.
CPC run tests on Ubuntu images for most cloud providers to ensure that chrony is configured with time sources.

----------------------------------------
There is no request at this time to SRU the fix for this bug. Current intention is to land this in devel. The description follows the SRU template, just to better organize the bug summary.
----------------------------------------

[Original Report]

Currently, the default chrony.conf configures a set of pools. Confirmed this on a focal and jammy instance on GCP. If one wishes to use only a specific server/server pool or not use a server at all they will need to modify /etc/chrony/chrony.conf. This will possibly lead to a prompt during an Ubuntu release upgrade and during an unattended chrony security upgrade.
We are trying to move all configuration changes to their respective *.d directories. See: https://bugs.launchpad.net/livecd-rootfs/+bug/1968873
We test for modified chrony config file by invoking `sudo md5sum --quiet --check /var/lib/ucf/hashfile`.

Listing the cases that I know where we are not able to move chrony configuration changes to a *.d config
1. Azure: Azure needs all default pool entries in chrony.conf disabled. This is currently done by commenting out the pool entries in /etc/chrony/chrony.conf. There doesn't seem to be an alternative way to reset the pool set used by chrony through a configuration in *.d directory.
2. Google: GCP images need to set a single server source entry. This is done indirectly through the ntp cloud-init module configuration. The ntp module replaces the default /etc/chrony/chrony.conf with another file that has required server entry and no pool entries. I believe this cannot be done through an override in *.d directory without touching /etc/chrony/chrony.conf.

This request perhaps can be extended to ensure that "negating" a configuration in the default /etc/chrony/chrony.conf should be possible through a configuration in /etc/chrony/*.d directory.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Sergio Durigan Junior (sergiodj)]

Hello Ankush,

I'm going to mark this bug as Triaged and subscribe the Ubuntu Server team, but this is only going to put the bug into our backlog. If this is a priority for you, please let us know (via the internal MM) and we can discuss it accordingly.

As a side note, this would be a good feature to implement first on the Debian package.

Cheers.

[Paride Legovini (paride)]

I thought I commented on this during some past triage rotation, but apparently I didn't.

Implementing "negation" for pool or server entries is going to be tricky (I can think of many corner cases), it's going to make the config painful to read, the patch is unlikely to be trivial and unlikely to be upstreamed.

We should maybe split the server/pool configuration out of the main conf file, put it in a .d fragment _not_ handled by ucf, but rather handled by maintainer scripts or by debconf, with a preseeded, low-priority question. Or maybe there's a way to tell dpkg to always treat a specific conf file in "confold" mode.

As Sergio said, ideally these packaging changes should be implemented in Debian.

[Ankush Pathak (ankushpathak)]

@sergiodj Thanks. Will reach out to the Ubuntu Server team if this needs to be prioritized.

@paride Regarding your point about splitting the server/pool configuration from the main conf file. If I understand correctly, you mean to effectively move the server/pool configuration out of the main conf file and to a file in the *.d/ directory, thus allowing a complete overwrite of the server/pool configuration, not requiring prompts during an upgrade. And that this change should be done at the package level instead of upstream. Did I capture that correctly?

[Lucas Kanashiro (lucaskanashiro)]

Ankush, yes, Paride suggested something like that (but not just that, he gave more details on how this could be implemented in the package). We should work with the Debian maintainers on this.

[Ankush Pathak (ankushpathak)]

I have attempted a draft fix for this issue in the attached debdiff. It introduces a new configuration directive "disallowname" that can make a source name configuration entry ineffective.

Please let me know if my attached changes seem feasible to be adopted, I'd be happy to discuss and iterate them further, and finally work on proposing them to the debian package.
The approach in the attached debdiff might complement the packaging solution described by @paride in https://bugs.launchpad.net/ubuntu/+source/chrony/+bug/2048876/comments/3. This is because, with the proposed debdiff, the source configuration for chrony can be modified without touching any package installed files.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "lp-2048876-disallow-name-conf.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[Robie Basak (racb)]

Thank you for working on this!

Unfortunately I don't think this is the right approach. If we introduce a new configuration directive and users start relying on it, then we get stuck - we can't take it away again without breaking users, including in ways that they won't necessarily be able to work around because users might well use that feature for other purposes. Therefore, I strongly object to adding support for a configuration option that isn't implemented by upstream code.

I suggest following Paride's guidance in comment 3.

[Robie Basak (racb)]

> then we get stuck

To be clear, I mean that we'll get stuck maintaining a delta against upstream forever, causing user confusion because of a difference in configuration syntax between us and non-Ubuntu chrony users, etc.

Unsubscribing ~ubuntu-sponsors as this patch needs adjusting. Please resubscribe ~ubuntu-sponsors as needed.

[Ankush Pathak (ankushpathak)]

Could you please review if this patch is more in line with @paride's suggestion?

The patch moves the pool sources out of chrony.conf and introduces them into `sources.d/default-ntp-pools.sources`.
`default-ntp-pools.sources` is managed by `postinst` through debconf questions. A user can choose not to have `sources.d/default-ntp-pools.sources`. However, it will be installed by default.
If the user has a `default-ntp-pools.sources` that is different than the one included in the package, by default, user's version will be preserved.

Scenarios tested:
1. Upgrade chrony when `sources.d/default-ntp-pools.sources` does not exist. `sources.d/default-ntp-pools.sources` was installed.
2. Upgrade chrony when `sources.d/default-ntp-pools.sources` does not exist and debconf set to not install `sources.d/default-ntp-pools.sources`. `sources.d/default-ntp-pools.sources` was not installed
3. Upgrade chrony when `sources.d/default-ntp-pools.sources` exists and is identical to the one included in the package. `cp` was not invoked
4. Upgrade chrony when `sources.d/default-ntp-pools.sources` exists and is different to the one included in the package. User's version of `sources.d/default-ntp-pools.sources` was preserved.
5. Upgrade chrony when `sources.d/default-ntp-pools.sources` exists, is different to the one included in the package and debconf is set not to preserve user's version. `sources.d/default-ntp-pools.sources` was overwritten with the version in the package.

I haven't marked this as a patch so that ~ubuntu-sponsors are not immediately subscribed in case if the patch requires rework.

Thanks,
Ankush

[Bryce Harrington (bryce)]

Hi Ankush, thanks for reworking the patch, what feedback has Debian provided so far? Can you share the Debian bug or MR where you're discussing this?

It's rather late in the noble cycle to be introducing a change of this scope; you're definitely going to need to put in a FFe. Since FF is nearly upon us, I would suggest treating the FFe as SRU (including providing the filled in SRU template and step-by-step testing requirements).

[Bryce Harrington (bryce)]

Some review comments:

0. I'm ok if you prefer to handle this as debdiff, but the Server team traditionally uses git-ubuntu for MP reviews. If you're familiar with git you'll likely also find it more comfortable, however I understand if due to the time pressure you'd rather do it an already known way. But put git-ubuntu on your todo list to master, if it's not already.

1. As I'm studying more closely I see that the config for the NTP pool servers is part of the package delta that Christian added, so that is going to be less relevant to Debian, and also I see that Debian provides the d/sources.d/ directory that you're using already, so while I did initially think like the others that this should be done in Debian, I'm seeing now it's more relevant to us only. It can't hurt to discuss with Debian but it's highly doubtful they'll take our delta. Also, since they already provide the d/sources.d/ directory I'm less worried about divergence with them.

2. In the postinst, if you reverse the order of the if clauses you can drop the additional filecheck, i.e.:

+ if [ ! -f /etc/chrony/sources.d/default-ntp-pools.sources ];
+ then
+ db_input low chrony/configure_default_pools_in_sourcesd || true
+ db_go
+ db_get chrony/configure_default_pools_in_sourcesd
+ if [ "${RET}" = "true" ];
+ then
+ cp --preserve /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources
+ fi
+ elif ! cmp -s /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources;
+ then
+ db_input low chrony/preserve_user_configured_pools_in_sourcesd || true
+ db_go
+ db_get chrony/preserve_user_configured_pools_in_sourcesd
+ if [ "${RET}" = "false" ];
+ then
+ cp --preserve /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources
+ fi
+ fi

I also think that presents your change a bit clearer since on initial install the file doesn't exist in sources.d and will need copied in place, so that first stanza is what happens first.

3. "The maintainer's version and current version of /etc/chrony/default-ntp-pools.sources are different."

I'm not certain, but this verbage seems a bit off. Can you doublecheck how other packages doing this same operation word the statement? (For example, as this is addressed to an end user, I'm not sure they would understand "current version" means the version currently installed on their system, and might not know what we mean by 'maintainer'.) If other packages are using this same wording style it's fine, but if there's a more conventional phrasing I'd go with something closer to that.

4. You might check whether the postrm should also do `rm -rf /etc/chrony/sources.d/` when purging.

5. Typically something like this should also be accompanied with some documentation regarding the change, such as in README.*, sources.d/README, and/or NEWS.

6. Does chrony-helper require any alterations?

7. There are references to `man chrony.conf(5)` in the service file...

[Andrew Cloke (andrew-cloke)]

Many thanks for that feedback and guidance. In parallel with working on the suggested changes, I wanted to understand the right approach we should take to fixing this bug.

From your comment in #1 "It can't hurt to discuss with Debian but it's highly doubtful they'll take our delta", would it be reasonable to assume that, as Ubuntu has already branched the chrony config associated with NTP pools away from Debian, we should attempt to land this bug fix to the NTP pools config directly into Ubuntu?

Thanks again for your help, Andy.

[Bryce Harrington (bryce)]

It's going to have to land in Ubuntu directly regardless of what's done. The package already has delta so even if this is done in Debian it'd have to get patched in for Ubuntu.

This fix probably needs to be thought of in several discrete chunks. The moving of the config snippet out of chrony.conf to a file in sources.d/ is one component; this one is what I was referring to in that the sources.d/ directory is already provided for in the Debian directory, and our config snippet isn't upstreamable to them, so at least this particular component is not something they'd be interested in.

Another component here is the addition of debconf questions, which came up in discussion with my teammates today. The essence of the prompt desiring to be solved here, as I understand it, is to enable configuration customization to work more seamlessly and in particular to reduce interactive prompting. Adding interactive debconf questions feels not in that spirit, maybe just shifting the prompting from one use case to others or moving it from upgrade time to installation time. If that's true, wouldn't it be better to look for ways to avoid prompts at all, at least for use cases we understand well?

Robie shared one idea, which I hope he won't mind me quoting verbatim:

> I would add a debconf + ucf mechanism to maintain a file in there called /etc/chrony/sources.d/from-debconf. By debconf, this could either be disabled (don't attempt to create the file), Ubuntu's default (make it the same as our defaults), or cloud-specific (use some cloud-specific file). Then ucf would maintain the same file but with different contents depending on debconf configuration, defaulting to Ubuntu's default.
> If a user customises the file using debconf, then great that'll work. If the user customises the file by hand, or deletes it, then ucf would do the right thing.
> Especially as ucf is used already, this shouldn't be burdensome.
> In this case, the debconf would be a multi-select against things like "do nothing", "ubuntu", "gce", "aws region a defaults" or whatever, and there would only need to be a single setting.
> However, we'd need to figure out how that would interact with what cloud-init might be doing already. And separately, if cloud-init does support customisation, then surely clouds should be using that via vendordata rather than doing something custom at image generation time?
> Here's cloud-init's implementation documentation: https://cloudinit.readthedocs.io/en/latest/reference/modules.html#ntp
> Looks like that has chrony support, too, and it expects to overwrite chrony.conf.

Unfortunately, we don't know of an existing package following this ucf + debconf style; most rely on straight ucf only.

Also, it sounds like if cloud-init relies on modifying chrony.conf for it's settings, then your fix for this bug will also break them. So for example cloud-init's ntp module configuration would need to be considered in this.

[Bryce Harrington (bryce)]

Hi Andrew and Ankush, just wanted to check in about status of your plans for this bug?

[Ankush Pathak (ankushpathak)]

Hi Bryce,

Apologies for the delay in responding.

Thank you for your review comments I'll try to address them in my next fix proposal.
With respect to the suggesstion you shared about handling well known cases I'll give it some thought myself , discuss it within my squad, gather feedback and report here as appropriate.
You are right in that cloud-init might also need changes to work in a compatible manner with this bug's fix. We anticipated this and plan to drive any required cloud-init changes with @catred's help.

I plan to resume working on this bug next week.

[Ankush Pathak (ankushpathak)]

Hi Bryce,

Addressing your feedback per point from comment #12
0. I have put an MP for review[0] instead of a debdiff for this iteration. Please let me know if that's the appropriate way to propose changes to server packages. git-ubuntu is definitely on my list to learn now.
1. I concur with this, since the proposed fix is a change to distro specific configuration. However, if there is a strong preference to upstream as much of these changes as possible, it would help me to gather some feedback from you and the server team and satisfy any concerns before attempting to upstream the changes.
2. I have incorporated this recommendation.
3. I have updated the verbiage slightly. It is based on ucf prompt:
```
What do you want to do about modified configuration file sshd_config?
  install the package maintainer's version
  keep the local version currently installed
  show the differences between the versions
  show a side-by-side difference between the versions
  start a new shell to examine the situation
```
4. The other drop-in configuration directory `chrony.conf.d` is not removed in `postrm` so I think we should keep `sources.d` too, otherwise remove both.
5. I have added a note to `sources.d/README`.
6. chrony-helper currently only parses chrony.conf when listing sources. In a way it is incomplete even without the fix for this bug as chrony-helper doesn't cover any sources a user might add under sources.d. chrony-helper is also a bash script parsing configuration. I feel chrony-helper should be deprecated in favor of something better at some point.
7. I did a quick scan of `man chrony.conf(5)` and I don't see any required updates.
8. Without `--debconf-ok` package installation fails with
```
*** WARNING: ucf was run from a maintainer script that uses debconf, but
             the script did not pass --debconf-ok to ucf. The maintainer
             script should be fixed to not stop debconf before calling ucf,
             and pass it this parameter. For now, ucf will revert to using
             old-style, non-debconf prompting. Ugh!

             Please inform the package maintainer about this problem.
Need debconf to interact
dpkg: error processing package chrony (--install):
```
9. Please share your suggestions, I will update the changelog entry accordingly.

RE: Robie's idea of debconf + ucf mechanism
* I discussed this within my squad, and the feeling was that the added complexity to the packaging scripts might not be needed.
* Different clouds require time sources to be setup differently, for ex. GCE relies on cloud-init while Azure requires time source to be set as a hardware device using `refclock`. We would need to handle all such possible scenarios.
* If the cloud providers want time sources to be configured at boot-time, like what GCE does at the moment, this idea won't be helpful.
* Even if we know the required time source configuration at image build time, there's a possibility that the cloud providers request an update to that configuration. Handling this change within CPC image build hooks is trivial, while updating that within...

[Brian Murray (brian-murray)]

Ubuntu 23.10 (Mantic Minotaur) has reached end of life, so this bug will not be fixed for that specific release.

[Andreas Hasenack (ahasenack)]

Thanks for the MP and the detailed explanation, @Ankush. I'll be looking at chrony in the Oracular cycle, and will check that MP now. Of course, this doesn't forbid others from chiming in.