ntp: Add setting to configure options #3852

domgoodwin · 2024-03-25T15:05:55Z

Description of changes:

Adds the ability to configure further options for the NTP servers. We would like to set maxpoll to a lower value so I've added the ability to specify options which get added to the servers too. I've removed the default option iburst and added it to this setting default instead.

Testing done:

Ran unit tests locally

Edit: Sam did the testing with Bottlerocket including migration testing, see comment below.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

sam-berning

Thank you for the contribution! Overall this approach makes sense, left a few comments on implementation details.

Probably the toughest bit will be the migration, but you can just adapt the commit I shared to work for this case, keeping in mind that the migration will be for version 1.20.0 instead of 1.19.2. Feel free to reach out if you run into any problems with this, I'm happy to help!

sam-berning · 2024-03-26T20:44:43Z

packages/chrony/chrony-conf

@@ -2,7 +2,7 @@
 ntp = "v1"
 +++
 {{#each settings.ntp.time-servers}}
-pool {{this}} iburst
+pool {{this}} {{#each settings.ntp.options}}{{this}} {{/each}}


minor, but this will avoid any additional whitespace:

Suggested change

pool {{this}} {{#each settings.ntp.options}}{{this}} {{/each}}

pool {{this}}{{#each settings.ntp.options}} {{this}}{{/each}}

sam-berning · 2024-03-26T20:48:42Z

sources/models/shared-defaults/defaults.toml

@@ -119,6 +119,7 @@ template-path = "/usr/share/templates/hosts"

 [settings.ntp]
 time-servers = ["169.254.169.123", "2.amazon.pool.ntp.org"]
+options = ["iburst"]


this will require a migration so that on downgrade to an older version of bottlerocket this setting will be removed. here's an example commit that should be very similar to the migration needed here: 83b3e6a

You will also need to add the migration to Release.toml, reference the above commit for an example.

sam-berning · 2024-03-26T20:55:13Z

sources/settings-extensions/ntp/src/lib.rs

@@ -8,6 +8,7 @@ use std::convert::Infallible;
 #[model(impl_default = true)]
 pub struct NtpSettingsV1 {
    time_servers: Vec<Url>,
+    options: Vec<str>,


You'll need to use an owned string here:

Suggested change

options: Vec<str>,

options: Vec<String>,

sam-berning · 2024-03-26T20:56:52Z

sources/settings-extensions/ntp/src/lib.rs

+                Url::try_from("minpool").unwrap(),
+                Url::try_from("1").unwrap(),
+                Url::try_from("maxpool").unwrap(),


Suggested change

Url::try_from("minpool").unwrap(),

Url::try_from("1").unwrap(),

Url::try_from("maxpool").unwrap(),

Url::try_from("minpoll").unwrap(),

Url::try_from("1").unwrap(),

Url::try_from("maxpoll").unwrap(),

domgoodwin · 2024-04-01T11:52:56Z

@sam-berning Thanks for the review, I've address them and added the migration - I think how I've done it is right

sam-berning

Hey @domgoodwin, thanks for following up! The migration looks good for the most part. Just have a couple more comments, noticed a few things I missed in my first review.

sam-berning · 2024-04-01T23:11:53Z

packages/chrony/chrony-conf

@@ -2,7 +2,7 @@
 ntp = "v1"
 +++
 {{#each settings.ntp.time-servers}}
-pool {{this}} iburst
+pool {{this}} {{#each settings.ntp.options}}{{this}}{{/each}}


we still need a space within the {{#each}} so that each option is separated by a space, it should look like this:

Suggested change

pool {{this}} {{#each settings.ntp.options}}{{this}}{{/each}}

pool {{this}}{{#each settings.ntp.options}} {{this}}{{/each}}

sam-berning · 2024-04-02T01:12:05Z

sources/models/shared-defaults/defaults.toml

@@ -119,6 +119,7 @@ template-path = "/usr/share/templates/hosts"

 [settings.ntp]
 time-servers = ["169.254.169.123", "2.amazon.pool.ntp.org"]
+options = ["iburst"]


You will also need to add the migration to Release.toml, reference the above commit for an example.

sam-berning · 2024-04-02T01:12:30Z

sources/settings-extensions/ntp/src/lib.rs

                time_servers: None
+                options: None


need commas after each of these items:

Suggested change

time_servers: None

options: None

time_servers: None,

options: None,

sam-berning · 2024-04-02T01:14:11Z

sources/settings-extensions/ntp/src/lib.rs

@@ -85,4 +87,22 @@ mod test {
        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
+
+    fn test_options_ntp() {
+        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;


For testing serde_json::to_string() at the end of this test, you'll need to remove all of the extra whitespace from this string:

Suggested change

let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;

let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

sam-berning · 2024-04-02T01:15:10Z

sources/settings-extensions/ntp/src/lib.rs

@@ -85,4 +87,22 @@ mod test {
        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
+
+    fn test_options_ntp() {


In order for Rust to recognize this as a unit test, you need to provide the #[test] macro here:

Suggested change

fn test_options_ntp() {

#[test]

fn test_options_ntp() {

sam-berning · 2024-04-02T01:16:22Z

sources/settings-extensions/ntp/src/lib.rs

+                Url::try_from("minpoll").unwrap(),
+                Url::try_from("1").unwrap(),
+                Url::try_from("maxpoll").unwrap(),
+                Url::try_from("2").unwrap(),


These should be Strings, not Urls:

Suggested change

Url::try_from("minpoll").unwrap(),

Url::try_from("1").unwrap(),

Url::try_from("maxpoll").unwrap(),

Url::try_from("2").unwrap(),

String::from("minpoll"),

String::from("1"),

String::from("maxpoll"),

String::from("2"),

sam-berning · 2024-04-02T01:17:53Z

sources/api/migration/migrations/v1.20.0/add-ntp-default-options-v0-1-0/Cargo.toml

@@ -0,0 +1,10 @@
+[package]
+name = "add-ntp-default-options"


the package name in Cargo.toml needs to be the same as the directory name or the build won't be able to find the package:

Suggested change

name = "add-ntp-default-options"

name = "add-ntp-default-options-v0-1-0"

domgoodwin · 2024-04-02T09:21:38Z

Comments addressed @sam-berning 🙏 , looks like the test is actually running now and passing - I had to remove the try_from stuff but I think that's fine as it's not a URL 🤦

running 3 tests
test test::test_generate_ntp_settings ... ok
test test::test_options_ntp ... ok
test test::test_serde_ntp ... ok

webern · 2024-04-02T18:26:32Z

Makefile.toml

@@ -24,7 +24,7 @@ TWOLITER_ALLOW_BINARY_INSTALL="true"

 # Allow Twoliter to be installed by building from sourcecode.
 TWOLITER_ALLOW_SOURCE_INSTALL="true"
-
+TARGET_CC="x86_64-unknown-linux-gnu"


What is this doing?

I didn't mean to add this, should be removed with the latest commit

sam-berning · 2024-04-02T20:48:19Z

CI is failing on check-fmt, can you reformat sources/api/migration/migrations/v1.20.0/add-ntp-default-options-v0-1-0/src/main.rs and sources/settings-extensions/ntp/src/lib.rs with rustfmt?

sam-berning

nice, thanks for updating it @domgoodwin. CI is now passing! I built this locally and tested on an EC2 instance and /etc/chrony.conf is not getting generated correctly:

Apr 02 23:14:28 ip-172-31-23-40.us-west-2.compute.internal apiserver[1791]: Configuration file 'chrony-conf' failed to render: Failed to render template file ('/usr/share/templates/chrony-conf'): Failed to render template 'Template { frontmatter: TemplateFrontmatter { required_extensions: {"ntp": Version("v1")} }, body: "{{#each settings.ntp.time-servers}}\npool {{this}}{{#each settings.ntp.options}} {{this}}{{/each}}\n{{/each}}\ndriftfile /var/lib/chrony/drift\nmakestep 1.0 3\ndumponexit\ndumpdir /var/lib/chrony\nuser chrony\nrtcsync\n" }': Error rendering "Unnamed template" line 2, col 14: Variable "settings.ntp.options" not found in strict mode.

Turns out within the each loop, you need to use ../ to access a variable outside of the scope of the loop. I've left a comment with the suggested change, but after that this should work as expected.

packages/chrony/chrony-conf

sam-berning · 2024-04-04T00:44:18Z

Thanks @domgoodwin, this is looking really good. Now I'll just need to test that the migrations are working as expected, I'll update you when I finish with that.

In the meantime, can you squash your commits into one?

domgoodwin · 2024-04-04T08:40:29Z

Thanks @sam-berning, commits squashed into 1 🙇

sam-berning · 2024-04-04T23:21:46Z

Tested the migration, it works as expected:

Started an instance:

bash-5.1# cat /etc/os-release                                                                                      
...                                                                                            
VERSION="1.20.0 (aws-dev)"                                                                                         
...                                                                                               
BUILD_ID=cfcc095e
...
bash-5.1# apiclient get settings.ntp
{
  "settings": {
    "ntp": {
      "options": [
        "iburst"
      ],
      "time-servers": [
        "169.254.169.123",
        "2.amazon.pool.ntp.org"
      ]
    }
  }
}
bash-5.1# cat /etc/chrony.conf 
pool 169.254.169.123 iburst
pool 2.amazon.pool.ntp.org iburst
driftfile /var/lib/chrony/drift
makestep 1.0 3
dumponexit
dumpdir /var/lib/chrony
user chrony
rtcsync
bash-5.1# updog update -i 1.19.3 -r -n                                                                             
Starting update to 1.19.3                                                                                          
Reboot scheduled for Thu 2024-04-04 23:07:29 UTC, use 'shutdown -c' to cancel.                                     
Update applied: aws-dev 1.19.3

after downgrade:

bash-5.1# cat /etc/os-release                                                                                      
...                                                                                                  
VERSION="1.19.3 (aws-dev)"                                                                                         
...
bash-5.1# apiclient get settings.ntp                                                                               
{                                                                                                                  
  "settings": {                                                                                                    
    "ntp": {
      "time-servers": [
        "169.254.169.123",
        "2.amazon.pool.ntp.org"
      ]
    }
  }
}
bash-5.1# ls /etc/chrony.conf                                                                                      
/etc/chrony.conf                                                                                                   
bash-5.1# cat /etc/chrony.conf                                                                                     
pool 169.254.169.123 iburst                                                                                        
pool 2.amazon.pool.ntp.org iburst                                                                                  
driftfile /var/lib/chrony/drift                                                                                    
makestep 1.0 3                                                                                                     
dumponexit                                                                                                         
dumpdir /var/lib/chrony                                                                                            
user chrony                                                                                                        
rtcsync
bash-5.1# updog update -i 1.20.0 -r -n
Starting update to 1.20.0
Reboot scheduled for Thu 2024-04-04 23:10:56 UTC, use 'shutdown -c' to cancel.
Update applied: aws-dev 1.20.0

after upgrade:

bash-5.1# cat /etc/os-release 
...
VERSION="1.20.0 (aws-dev)"
...
bash-5.1# apiclient get settings.ntp
{
  "settings": {
    "ntp": {
      "options": [
        "iburst"
      ],
      "time-servers": [
        "169.254.169.123",
        "2.amazon.pool.ntp.org"
      ]
    }
  }
}
bash-5.1# cat /etc/chrony.conf      
pool 169.254.169.123 iburst
pool 2.amazon.pool.ntp.org iburst
driftfile /var/lib/chrony/drift
makestep 1.0 3
dumponexit
dumpdir /var/lib/chrony
user chrony
rtcsync

webern

This looks correct to me. @yeazelm can you take a look to confirm? Thanks.

webern · 2024-04-04T23:38:09Z

sources/models/shared-defaults/defaults.toml

@@ -119,6 +119,7 @@ template-path = "/usr/share/templates/hosts"

 [settings.ntp]
 time-servers = ["169.254.169.123", "2.amazon.pool.ntp.org"]
+options = ["iburst"]


@yeazelm, this looks correct to me. The options setting is added by the migration and the defaults file will now include "iburst" such that behavior does not change on upgrade.

This looks right to me!

sam-berning · 2024-04-04T23:44:37Z

@domgoodwin this needs a rebase

domgoodwin · 2024-04-05T06:52:10Z

@sam-berning done ✅

sam-berning reviewed Mar 26, 2024

View reviewed changes

sam-berning reviewed Apr 2, 2024

View reviewed changes

webern reviewed Apr 2, 2024

View reviewed changes

sam-berning reviewed Apr 2, 2024

View reviewed changes

packages/chrony/chrony-conf Outdated Show resolved Hide resolved

domgoodwin force-pushed the bottlerocket-ntp branch from 7ce0e3b to cfcc095 Compare April 4, 2024 08:39

sam-berning approved these changes Apr 4, 2024

View reviewed changes

webern approved these changes Apr 4, 2024

View reviewed changes

ntp: Add setting to configure options

862c0e2

domgoodwin force-pushed the bottlerocket-ntp branch from cfcc095 to 862c0e2 Compare April 5, 2024 06:51

yeazelm approved these changes Apr 5, 2024

View reviewed changes

sam-berning merged commit 91b3b2d into bottlerocket-os:develop Apr 5, 2024
35 checks passed

ginglis13 mentioned this pull request May 7, 2024

v1.20.0 🐫 Tracking Issue #3942

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ntp: Add setting to configure options #3852

ntp: Add setting to configure options #3852

domgoodwin commented Mar 25, 2024 •

edited by webern

Loading

sam-berning left a comment

sam-berning Mar 26, 2024

sam-berning Mar 26, 2024

sam-berning Apr 2, 2024

sam-berning Mar 26, 2024

sam-berning Mar 26, 2024

domgoodwin commented Apr 1, 2024 •

edited

Loading

sam-berning left a comment

sam-berning Apr 1, 2024

sam-berning Apr 2, 2024

sam-berning Apr 2, 2024

sam-berning Apr 2, 2024

sam-berning Apr 2, 2024

sam-berning Apr 2, 2024

sam-berning Apr 2, 2024

domgoodwin commented Apr 2, 2024

webern Apr 2, 2024

domgoodwin Apr 2, 2024

sam-berning commented Apr 2, 2024

sam-berning left a comment

sam-berning commented Apr 4, 2024

domgoodwin commented Apr 4, 2024

sam-berning commented Apr 4, 2024

webern left a comment

webern Apr 4, 2024

yeazelm Apr 5, 2024

sam-berning commented Apr 4, 2024

domgoodwin commented Apr 5, 2024

	pool {{this}} {{#each settings.ntp.options}}{{this}} {{/each}}
	pool {{this}}{{#each settings.ntp.options}} {{this}}{{/each}}

	let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;
	let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

	name = "add-ntp-default-options"
	name = "add-ntp-default-options-v0-1-0"

ntp: Add setting to configure options #3852

ntp: Add setting to configure options #3852

Conversation

domgoodwin commented Mar 25, 2024 • edited by webern Loading

sam-berning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgoodwin commented Apr 1, 2024 • edited Loading

sam-berning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgoodwin commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-berning commented Apr 2, 2024

sam-berning left a comment

Choose a reason for hiding this comment

sam-berning commented Apr 4, 2024

domgoodwin commented Apr 4, 2024

sam-berning commented Apr 4, 2024

webern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-berning commented Apr 4, 2024

domgoodwin commented Apr 5, 2024

domgoodwin commented Mar 25, 2024 •

edited by webern

Loading

domgoodwin commented Apr 1, 2024 •

edited

Loading