Skip to content

Ntp: Add pool checkbox to timeserver configuration #7760

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Ntp: Add pool checkbox to timeserver configuration #7760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e54d45a
ntpd: make pool option for timeservers configurable
daerSeebaer Aug 11, 2024
7f8a534
keep ntp pool servers as pool type in default config
daerSeebaer Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/etc/config.xml.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@
<enable/>
</rrd>
<ntpd>
<pool_server>0.opnsense.pool.ntp.org 1.opnsense.pool.ntp.org 2.opnsense.pool.ntp.org 3.opnsense.pool.ntp.org</pool_server>
Copy link
Member

Choose a reason for hiding this comment

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

cleaning up the timerserver mess being anchored in system instead of ntpd would be much more appealing, but it requires MVC code to make this work appealing and future-proof.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the whole structure of the data is mind boggling. You want to create a server entry and add all options like also pool_server as a property, but now you have all properties as global arrays that refer to timeservers which isn't even anchored here. This also negatively impacts partial config imports as it is BTW.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the config is all over the place. I tried to "fit in" with the other options like <prefer> and just put the <pool_server> next to them. For this PR that seemed like the best thing to do, since my goal was to introduce the ability for pool configurations, not to refactor the entire thing.

Do you have alternative ideas where and how to store this data? One option that comes to mind would be to start the migration to a more sensible model and introduce a list of timeserver-tags that for now only contains a <pool_server>[true|false]</pool_server>, however the naming conventions for that structure should be agreed upon since they should last into the MVC age.

Copy link
Contributor

Choose a reason for hiding this comment

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

cleaning up the timerserver mess being anchored in system instead of ntpd would be much more appealing, but it requires MVC code to make this work appealing and future-proof.

Since I use the os-chrony plugin as ntpd was unreliable I would prefer "Services - Network time" to become a plugin - so I can uninstall it. Same for Dnsmasq...

<prefer>0.opnsense.pool.ntp.org</prefer>
</ntpd>
<widgets>
Expand Down
9 changes: 5 additions & 4 deletions src/etc/inc/plugins.inc.d/ntpd.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,17 +329,18 @@ function ntpd_configure_do($verbose = false)

$noselect = isset($config['ntpd']['noselect']) ? explode(' ', $config['ntpd']['noselect']) : [];
$prefer = isset($config['ntpd']['prefer']) ? explode(' ', $config['ntpd']['prefer']) : [];
$pool_server = isset($config['ntpd']['pool_server']) ? explode(' ', $config['ntpd']['pool_server']) : [];
$iburst = isset($config['ntpd']['iburst']) ? explode(' ', $config['ntpd']['iburst']) : [];

$ntpcfg .= "\n\n# Upstream Servers\n";
/* foreach through ntp servers and write out to ntpd.conf */
foreach (explode(' ', $config['system']['timeservers']) as $ts) {
/* Determine if Network Time Server is from the NTP Pool or not */
if (preg_match("/\.pool\.ntp\.org$/", $ts)) {
Copy link
Member

Choose a reason for hiding this comment

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

@daerSeebaer looking at this again what pool servers do not conform with this address? what servers are you using? Also noselect poses an issue when adding an arbitrary pool switch as shown by #7841

Copy link
Author

Choose a reason for hiding this comment

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

@daerSeebaer looking at this again what pool servers do not conform with this address? what servers are you using?

The pool under pool.ntp.org is the capital P "NTP Pool". However, anyone that operates two or more ntp servers at different IP addresses can make them reachable under the same host name. Notable examples: time.google.com, time.cloudflare.com and time.apple.com all return multiple IP addresses and could each be used as a single entry with the pool option set. Personally, I also operate multiple time servers that are all reachable under a common host name that I don't want to disclose.

As discussed above, in order to not affect existing setups the current idea is to handle any pool.ntp.org entries and any entries where the pool flag is set as pool in the ntp config.

Also noselect poses an issue when adding an arbitrary pool switch as shown by #7841

I'm fully aware of this and changed the status of this PR to draft when #7841 was opened. I was hoping that the other PR would be merged first and I could then finalize my PR to reflect the noselect behavior in the UI.

$ntpcfg .= "pool {$ts}";
if (in_array($ts, $pool_server)) {
$ntpcfg .= "pool";
} else {
$ntpcfg .= "server {$ts}";
$ntpcfg .= "server";
}
$ntpcfg .= " {$ts}";
if (in_array($ts, $iburst)) {
$ntpcfg .= ' iburst';
}
Expand Down
11 changes: 10 additions & 1 deletion src/www/services_ntpd.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,12 @@

// parse timeservers
$pconfig['timeservers_host'] = array();
$pconfig['timeservers_pool'] = array();
$pconfig['timeservers_noselect'] = array();
$pconfig['timeservers_prefer'] = array();
$pconfig['timeservers_iburst'] = array();
if (!empty($config['system']['timeservers'])) {
$pconfig['timeservers_pool'] = !empty($a_ntpd['pool_server']) ? explode(' ', $a_ntpd['pool_server']) : array();
$pconfig['timeservers_noselect'] = !empty($a_ntpd['noselect']) ? explode(' ', $a_ntpd['noselect']) : array();
$pconfig['timeservers_prefer'] = !empty($a_ntpd['prefer']) ? explode(' ', $a_ntpd['prefer']) : array();
$pconfig['timeservers_iburst'] = !empty($a_ntpd['iburst']) ? explode(' ', $a_ntpd['iburst']) : array();
Expand Down Expand Up @@ -115,13 +117,14 @@

// list types
$config['system']['timeservers'] = trim(implode(' ', $pconfig['timeservers_host']));
$a_ntpd['pool_server'] = !empty($pconfig['timeservers_pool']) ? trim(implode(' ', $pconfig['timeservers_pool'])) : null;
$a_ntpd['noselect'] = !empty($pconfig['timeservers_noselect']) ? trim(implode(' ', $pconfig['timeservers_noselect'])) : null;
$a_ntpd['prefer'] = !empty($pconfig['timeservers_prefer']) ? trim(implode(' ', $pconfig['timeservers_prefer'])) : null;
$a_ntpd['iburst'] = !empty($pconfig['timeservers_iburst']) ? trim(implode(' ', $pconfig['timeservers_iburst'])) : null;
$a_ntpd['interface'] = !empty($pconfig['interface']) ? implode(',', $pconfig['interface']) : null;

// unset empty
foreach (array('noselect', 'prefer', 'iburst', 'interface') as $fieldname) {
foreach (array('pool_server', 'noselect', 'prefer', 'iburst', 'interface') as $fieldname) {
if (empty($a_ntpd[$fieldname])) {
unset($a_ntpd[$fieldname]);
}
Expand Down Expand Up @@ -256,6 +259,7 @@ function removeRow() {
<tr>
<th></th>
<th><?=gettext("Network"); ?></th>
<th><?=gettext("Pool"); ?></th>
<th><?=gettext("Prefer"); ?></th>
<th><?=gettext("Iburst"); ?></th>
<th><?=gettext("Do not use"); ?></th>
Expand All @@ -274,6 +278,9 @@ function removeRow() {
<td>
<input name="timeservers_host[]" type="text" value="<?=$timeserver;?>" />
</td>
<td>
<input name="timeservers_pool[]" class="ts_checkbox" type="checkbox" value="<?=$timeserver;?>" <?= !empty($pconfig['timeservers_pool']) && in_array($timeserver, $pconfig['timeservers_pool']) ? 'checked="checked"' : '' ?>/>
</td>
<td>
<input name="timeservers_prefer[]" class="ts_checkbox" type="checkbox" value="<?=$timeserver;?>" <?= !empty($pconfig['timeservers_prefer']) && in_array($timeserver, $pconfig['timeservers_prefer']) ? 'checked="checked"' : '' ?>/>
</td>
Expand All @@ -299,6 +306,8 @@ function removeRow() {
<?=gettext('For best results three to five servers should be configured here.'); ?>
<?=gettext('When no servers are specified NTP will be completely disabled.'); ?>
<br />
<?= gettext('The "pool" option indicates that the specified hostname represents a server pool instead of a single server.') ?>
<br />
<?= gettext('The "prefer" option indicates that NTP should favor the use of this server more than all others.') ?>
<br />
<?= gettext('The "iburst" option enables faster clock synchronisation on startup at the expense of the peer.') ?>
Expand Down