-
Notifications
You must be signed in to change notification settings - Fork 757
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
Ntp: Add pool checkbox to timeserver configuration #7760
base: master
Are you sure you want to change the base?
Conversation
I'm probably getting lazy and this static PHP code needs MVC/API conversion anyway. What's the issue with keeping the current behaviour? I mean:
Requires no migration, or? |
One of my goals was to make it configurable if NTP Pool hostnames should be handled as "pool" or "server". But yes, if we keep the old behavior that would preserve all current configurations across the change. If that is the way forward, I would suggest that for entries matching the pool-rule, the frontend should indicate the backend behavior by showing a checked, greyed out, uneditable checkbox in the "pool" column. And the help text could be expanded to also explain the behavior.
Is that planned for the near future? |
Not for 25.1 anyway. I'm all for small scope patches first. I agree this is an issue, but considering what's been going on lately not the most pressing one for sure. |
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Is there some advantage to not treating pool as pool? (Beyond the note below).
Yes, it's already inconsistent with "Do not use". You cannot use Can do some PR if we agree how to treat
From the experience with adding |
Not really. All NTP Pool domains can reasonably be expected to return multiple, changing IP addresses. I initially wanted to make it configurable to bring configurability close to the level that an administrator handling the config file itself would have, but on second thought there is no real benefit. Together with the drawbacks of a migration I agree that the combined option of handling all I'm going to add that functionality to this PR, but keep it as draft until #7841 is merged. After that I can utilize the new JS functions and finalize the PR. |
Yeah, you'd better wait till someone sanitizes my garbage JS. 😂 (Though, I wrote it so that it would easily handle the additional "pool" checkboxes.) |
6586a65
to
607e32a
Compare
This PR solves #6923 while preserving the current default behavior.
Technical description
I more or less just copied one of the other checkboxes and replaced everything with "pool_server". I also tested it by copying the impacted files into a running OPNSense, and then successfully using the new configuration option and verifying the result by inspecting the generated ntp.conf file.
Reasoning
This change makes it more transparent how the entries in the ntp config are configured, and gives more options to administrators to either set a NTP Pool server as "server" type or use the "pool" keyword for pooled servers besides the NTP Pool.
Impact
I extended the default configuration file to set the pool config for the default timeservers, so new installations will not change their behavior.
For existing installations, since the option is "opt in", the missing pool option will set all confgurations for NTP Pool servers back to a "server" type association. If there is a possibility to migrate existing configurations and keep the existing configuration during installation of this fix (
pool
for all NTP Pool servers,server
for anything else), that would be preferrable but I don't know how to do that. One alternative idea was to make the feature "opt out", setting the pool type unless a timeserver gets explicitely marked as "not pool", but that also would impact existing installations by setting the pool type for manually configured not-NTP Pool servers.