Repository navigation
Support Cluster API NTP stanza - #12
navaneeth-dev wants to merge 2 commits into
Conversation
Configure Flatcar's native timesyncd while retaining DHCP-provided sources. Preserve native defaults for disabled NTP or empty server lists, and reject unsupported fields and unsafe server values. Signed-off-by: Navaneeth <me@rizexor.com>
3f6f118 to
6af6746
Compare
Signed-off-by: Navaneeth <me@rizexor.com>
elmiko
left a comment
elmiko
left a comment
There was a problem hiding this comment.
generally looking good to me, i have a couple questions and a suggestion for the tests.
| // A hostname label is 1–63 ASCII letters, digits, or hyphens. | ||
| // The first and last characters must be letters or digits. | ||
| var ntpHostnameLabel = regexp.MustCompile( | ||
| `^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$`, |
There was a problem hiding this comment.
is there a reference for the hostname label format?
if there is an RFC or some other doc that we could link to the in the comments, it would help with future triaging in case there is an error related to the regex or hostname.
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| explicit := strings.Replace(input, "ntp:\n", "ntp:\n enabled: true\n", 1) |
There was a problem hiding this comment.
it would be nice to have a short comment here about why the input data needs to be changed.
| "ntp: {enabled: true, servers:}", "ntp: {enabled: true, servers: []}", | ||
| "ntp: {enabled: false, servers: [time.example.net]}", | ||
| } { | ||
| t.Run(stanza, func(t *testing.T) { |
There was a problem hiding this comment.
instead of using the full test stanza here, it's nice to use a descriptive name for each test case to make it easier when viewing test output later.
| {"ntp: {config: {}}", "ntp.config"}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.stanza, func(t *testing.T) { |
There was a problem hiding this comment.
similar here about using the stanza for the description.
| "ntp", "Time-1.example.net.", "xn--bcher-kva.example", | ||
| strings.Repeat("a", 63) + ".net", strings.Repeat("abc.", 63) + "a.", | ||
| } { | ||
| t.Run(server, func(t *testing.T) { |
There was a problem hiding this comment.
here as well. having a readable name makes the debugging easier later.
| `ntp: {}` retain Flatcar defaults without generating files or units. | ||
|
|
||
| This relies on Flatcar's native timesyncd service, does not switch clients on images |
There was a problem hiding this comment.
i'm curious about the mention of flatcar here. does this mean that using this library with other linux versions (eg FedoraCoreOS) is going to be problematic?
Implement Cluster API’s
ntp.enabledandntp.serversusing Flatcar’s native systemd-timesyncd, keeping DHCP-provided NTP sources allowed alongside configured servers.enabled: falseleaves time synchronization untouched after validation; omitted/null/empty server lists retain Flatcar defaults.