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

[Ankush Pathak]

[Impact]
* An explanation of the effects of the bug on users and
  justification for backporting the fix to the stable release.

* In addition, it is helpful, but not required, to include an
  explanation of how the upload fixes this bug.

[Workaround]
* If available, steps users can take to avoid the issue while waiting
  for a fix. Emphasize whether the workaround sometimes or always
  works, and any side effects or other caveats that may exist.

[Test Case]
* Detailed instructions how to reproduce the bug

* These should allow someone who is not familiar with the affected
  package to reproduce the bug and verify that the updated package fixes
  the problem.

[Where Problems Could Occur]
* Think about what the upload changes in the software. Imagine the change is
  wrong or breaks something else: how would this show up?

* It is assumed that any SRU candidate patch is well-tested before
  upload and has a low overall risk of regression, but it's important
  to make the effort to think about what ''could'' happen in the
  event of a regression.

* This must '''never''' be "None" or "Low", or entirely an argument as to why
  your upload is low risk.

* This both shows the SRU team that the risks have been considered,
  and provides guidance to testers in regression-testing the SRU.

[Other Info]

* Anything else you think is useful to include
* Anticipate questions from users, SRU, +1 maintenance, security teams and the Technical Board
  and address these questions in advance

[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?