ntp: changing the default config from server to pool broke the dhcp hook

Bug #1656801 reported by Philip Roche on 2017-01-16
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
ntp (Debian)
Fix Released
Unknown
ntp (Ubuntu)
Medium
Unassigned
Xenial
Medium
Unassigned

Bug Description

In 1:4.2.8p3+dfsg-1, the default config was changed to
"Use pool instead of server". This needs a corresponding
update to /etc/dhcp/dhclient-exit-hooks.d/ntp, since the
DHCP specified servers now get added to the default pool
config instead of replacing them.

This affects Xenial only as the Yakkety build includes the upstream fix (1:4.2.8p7+dfsg-1).

Original Debian busg https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=809344
& https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=806676

[Test Case]

philroche: This surfaced for me initially while testing on GCE. On GCE NTP servers are provided via DHCP so the easiest test case is to launch an instance on GCE without our workaround configured.

One such image is "daily-ubuntu-ntpdebug-1604-xenial-v20170331" in project "ubuntu-os-cloud-devel"

To start an instance of this image:

`gcloud compute instances create daily-ubuntu-ntpdebug-1604-xenial-v20170331 --image-project ubuntu-os-cloud-devel --image "daily-ubuntu-ntpdebug-1604-xenial-v20170331"`

Then run `ntpq -p` which should, once ntp is patched, return only one entry 'metadata.google' and should not return any of the ubuntu NTP pools.

[Regression Potential]

As noted in comments #1 and #2, this SRU might surface an issue if the user is receiving a broken set of NTP servers.

And as noted in comment #4, some people might be hackishly using the distinction between spaces and tabs in the config file to trick our current hook. Debian has gotten rid of that distinction, so that hack will stop working on distro-upgrade anyway. The fix is *probably* going to help more people who unintentionally mixed the two up rather than the few that are relying on that bug. But that bit can be easily dropped from the SRU if ya like.

Robie Basak (racb) wrote :

Thank you for identifying this. I agree it makes sense to fix it in Xenial. Though I wonder about how the change might adversely impact existing users.

For example, if a DHCP user is receiving a broken set of NTP servers that doesn't work, currently his NTP won't be broken because the default set will be used

But with this fixed, the default set will stop being used, and his NTP will appear to break. Though the root cause is that his DHCP or network was misconfigured, it would be the SRU that causes the problem to emerge.

In principle I think this is OK, but we should point it out in "Regression Potential", and I appreciate more opinions.

Changed in ntp (Ubuntu Xenial):
status: New → Triaged
Changed in ntp (Ubuntu):
status: New → Fix Released
Philip Roche (philroche) wrote :

The issue if a broken set of NTP servers is received and having no fallback is the case in Yakkety now too though and previously in Xenial prior to the server/pool changeover in 1:4.2.8p3+dfsg-1.

I agree that ideally there would be a fallback if the received NTP servers were broken but this bug itself is a regression from functionality prior to 1:4.2.8p3+dfsg-1.

Philip Roche (philroche) wrote :

Please find attached patch for this bug. This is the same fix as upstream (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=806676 and https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=809344)

The patch adds "pool" to the "server" and "peer" list as well as handling tabs and spaces in the ntp.conf better.

Changed in ntp (Ubuntu):
importance: Undecided → Medium
Changed in ntp (Ubuntu Xenial):
importance: Undecided → Medium
Paul Gear (paulgear) wrote :

One comment on @philroche's patch: at present, there is no supported way to allow DHCP to augment (rather than replace) NTP servers in /etc/ntp.conf. The different handling of tabs & spaces is a known-useful workaround for those of us who wish to allow DHCP to specify additional servers, without removing those specified in /etc/ntp.conf. The patch would eliminate this option, and would allow only DHCP-specified servers OR /etc/ntp.conf servers, not both. Thus, my opinion is -1 for that patch as-is, and +1 for the upstream Debian version.

Changed in ntp (Debian):
status: Unknown → Fix Released
Paul Gear (paulgear) wrote :

Correction to my comment above: the patch at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=809344 is preferable to the patch as proposed; thanks @philroche.

Paul Gear (paulgear) wrote :

Given that Debian has already applied the patch which eliminates the distinction between tabs & spaces, there doesn't seem a lot of point in keeping a different patch in place. Best to apply the patch as is, and deal with the issue of augmenting the list separately, if necessary.

Michael Terry (mterry) on 2017-03-29
description: updated
Michael Terry (mterry) wrote :

I've uploaded the patch in comment #3 to xenial. I've also added some SRU bits to the description -- if either Robie or Philip could maybe add some test case steps, that would be swell. Thanks!

Philip Roche (philroche) wrote :

Hi mterry.

RE: test case steps.

This surfaced for me initially while testing on GCE. On GCE NTP servers are provided via DHCP so the easiest test case is to launch an instance on GCE without our workaround configured.

One such image is "daily-ubuntu-ntpdebug-1604-xenial-v20170331" in project "ubuntu-os-cloud-devel"

To start an instance of this image:

`gcloud compute instances create daily-ubuntu-ntpdebug-1604-xenial-v20170331 --image-project ubuntu-os-cloud-devel --image "daily-ubuntu-ntpdebug-1604-xenial-v20170331"`

Then run `ntpq -p` which should, once ntp is patched, return only one entry 'metadata.google' and should not return any of the ubuntu NTP pools.

Philip Roche (philroche) on 2017-04-03
description: updated

Hello Philip, or anyone else affected,

Accepted ntp into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/ntp/1:4.2.8p4+dfsg-3ubuntu5.4 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in ntp (Ubuntu Xenial):
status: Triaged → Fix Committed
tags: added: verification-needed
Philip Roche (philroche) wrote :

I have verified that ntp 1:4.2.8p4+dfsg-3ubuntu5.4 in xenial-proposed passes the test case outlined in the description above.

* Launch GCE instance
* Enabled proposed
* Upgrade ntp
* Reboot
* Confirm `ntpq -p` returns only one entry

tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ntp - 1:4.2.8p4+dfsg-3ubuntu5.4

---------------
ntp (1:4.2.8p4+dfsg-3ubuntu5.4) xenial; urgency=medium

  * Fix ntp.dhcp to also check for pool and better handle spaces and tabs.
    (LP: #1656801)

 -- Phil Roche <email address hidden> Thu, 19 Jan 2017 11:06:04 +0000

Changed in ntp (Ubuntu Xenial):
status: Fix Committed → Fix Released

The verification of the Stable Release Update for ntp has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

tags: added: id-58e22b4afdee5f97b3bfbfd7
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.