From 6f00cc74433718ac76b3e9214258ed4f21a65f97 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 5 May 2026 14:37:43 +0300 Subject: [PATCH 01/27] o/c/c/ntp: configure NTP servers from snapd options Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/handlers.go | 3 + overlord/configstate/configcore/ntp.go | 242 ++++++++++++++++++++ 2 files changed, 245 insertions(+) create mode 100644 overlord/configstate/configcore/ntp.go diff --git a/overlord/configstate/configcore/handlers.go b/overlord/configstate/configcore/handlers.go index da39522a240..37915e8cda1 100644 --- a/overlord/configstate/configcore/handlers.go +++ b/overlord/configstate/configcore/handlers.go @@ -124,6 +124,9 @@ func init() { // system.motd addFSOnlyHandler(validateMotdConfiguration, handleMotdConfiguration, coreOnly) + // system.ntp.{servers,fallback-servers} + addFSOnlyHandler(validateNTPSettings, handleNTPConfiguration, coreOnly) + sysconfig.ApplyFilesystemOnlyDefaultsImpl = filesystemOnlyApply } diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go new file mode 100644 index 00000000000..5b84ba979b2 --- /dev/null +++ b/overlord/configstate/configcore/ntp.go @@ -0,0 +1,242 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2026 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +package configcore + +import ( + "fmt" + "io" + "net" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/coreos/go-systemd/unit" + "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/osutil" + "github.com/snapcore/snapd/overlord/configstate/config" + "github.com/snapcore/snapd/release" + "github.com/snapcore/snapd/sysconfig" + "github.com/snapcore/snapd/systemd" +) + +func init() { + // add supported configuration of this module + supportedConfigurations["core.system.ntp.servers"] = true + supportedConfigurations["core.system.ntp.fallback-servers"] = true + // and register it as a external config + config.RegisterExternalConfig("core", "system.ntp", getNTPFromSystemHelper) +} + +// Too simplistic? +var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`).MatchString +var validIPv4 = net.ParseIP + +var timesyncdToSnapKeyMapping = map[string]string{ + "NTP": "servers", + "FallbackNTP": "fallback-servers", +} + +var snapToTimesyncdKeyMapping = map[string]string{ + "servers": "NTP", + "fallback-servers": "FallbackNTP", +} + +func validateNTPSettings(tr ConfGetter) error { + var ntpCfg map[string]any + if err := tr.Get("core", "system.ntp", &ntpCfg); err != nil && !config.IsNoOption(err) { + return fmt.Errorf("cannot get NTP config: %v", err) + } + + for k, v := range ntpCfg { + if err := validateSingleNTPSetting(k, v); err != nil { + return fmt.Errorf("invalid NTP configuration: %v", err) + } + } + + return nil +} + +func validateSingleNTPSetting(key string, value any) (err error) { + switch key { + case "servers", "fallback-servers": + servers, ok := value.([]any) + if !ok { + return fmt.Errorf("%v is not a list of server names", key) + } + if len(servers) == 0 { + return fmt.Errorf("%v is an empty list", key) + } + + if err := validateNTPServers(servers); err != nil { + return err + } + return nil + + default: + return fmt.Errorf("unsupported configuration option %q", key) + } +} + +func validateNTPServers(servers []any) error { + for _, serverAny := range servers { + server, ok := serverAny.(string) + if !ok { + return fmt.Errorf("%q is not a valid string", serverAny) + } + if err := validateServerName(server); err != nil { + return err + } + } + + return nil +} + +func validateServerName(serverAddress string) error { + if validIPv4(serverAddress) == nil && !validHostname(serverAddress) { + return fmt.Errorf("%q is not a valid server name", serverAddress) + } + return nil +} + +func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyContext) error { + var cfg map[string]any + if err := tr.Get("core", "system.ntp", &cfg); err != nil && !config.IsNoOption(err) { + return fmt.Errorf("cannot get NTP config: %v", err) + } + + rootDir := dirs.GlobalRootDir + if opts != nil { + // runtime system + rootDir = opts.RootDir + } + + // Check that the updated configuration is valid + if err := validateNTPSettings(tr); err != nil { + return err + } + + // Create systemd configuration folder, if not present + systemdConfigFolder := filepath.Join(rootDir, "/etc/systemd/") + if err := os.MkdirAll(systemdConfigFolder, 0755); err != nil { + return err + } + + // Write validated configuration to file + ntpConfigPath := filepath.Join(systemdConfigFolder, "/timesyncd.conf") + if err := osutil.AtomicWriteFile(ntpConfigPath, serializeNTPConfiguration(cfg), 0644, 0); err != nil { + return fmt.Errorf("cannot write NTP configuration: %v", err) + } + + // Restart systemd-timesyncd.service to pick up the updated configuration + sysd := systemd.New(systemd.SystemMode, &sysdLogger{}) + if err := sysd.ReloadOrRestart([]string{"systemd-timesyncd.service"}); err != nil { + return fmt.Errorf("cannot restart timesyncd daemon after configuration change: %v", err) + } + + return nil +} + +func serializeNTPConfiguration(config map[string]any) (result []byte) { + unitOptions := []*unit.UnitOption{} + + for k, v := range config { + unitOption := *unit.NewUnitOption( + "Time", + mapKeySnapToTimesyncd(k), + mapValueSnapToTimesyncd(v), + ) + unitOptions = append(unitOptions, &unitOption) + } + + byteStream, _ := io.ReadAll(unit.Serialize(unitOptions)) + return byteStream +} + +func getNTPFromSystemHelper(key string) (result any, err error) { + return getNTPFromSystem() +} + +func mapValueTimesyncdToSnap(option *unit.UnitOption) (result any) { + switch option.Name { + case "NTP", "FallbackNTP": + return strings.Split(option.Value, " ") + + default: + return "" + } +} + +func mapValueSnapToTimesyncd(option any) string { + switch option := option.(type) { + case []any: + // Snapd will return a []any for lists. We need to check + // that each element is a string manually + var builder []string + for _, server := range option { + if s, ok := server.(string); ok { + builder = append(builder, s) + } + } + + return strings.Join(builder, " ") + + default: + return "" + } +} + +func mapKeyTimesyncdToSnap(timesyncdOption string) string { + return timesyncdToSnapKeyMapping[timesyncdOption] +} + +func mapKeySnapToTimesyncd(snapOption string) string { + return snapToTimesyncdKeyMapping[snapOption] +} + +func getNTPFromSystem() (result map[string]any, err error) { + if release.OnClassic { + return nil, nil + } + + file, err := os.Open("/etc/systemd/timesyncd.conf") + if os.IsNotExist(err) { + return map[string]any{}, nil + } + if err != nil { + return nil, fmt.Errorf("cannot read NTP configuration file /etc/systemd/timesyncd.conf: %v", err) + } + defer file.Close() + + unitOptions, err := unit.Deserialize(file) + if err != nil { + return nil, fmt.Errorf("cannot parse systemd unit in configuration file /etc/systemd/timesyncd.conf: %v", err) + } + + val := map[string]any{} + + for _, option := range unitOptions { + snapOptionName := mapKeyTimesyncdToSnap(option.Name) + snapOptionValue := mapValueTimesyncdToSnap(option) + val[snapOptionName] = snapOptionValue + } + + return val, nil +} From be4e5357d7e2c10695b440325206698bb96b19d4 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 5 May 2026 14:47:01 +0300 Subject: [PATCH 02/27] o/c/c/ntp: configure additional NTP options RootDistance, PollInterval, etc. Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 136 ++++++++++++++++++++++--- 1 file changed, 122 insertions(+), 14 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 5b84ba979b2..24a601d49a7 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -20,12 +20,15 @@ package configcore import ( + "encoding/json" "fmt" "io" "net" "os" + "os/exec" "path/filepath" "regexp" + "strconv" "strings" "github.com/coreos/go-systemd/unit" @@ -41,6 +44,11 @@ func init() { // add supported configuration of this module supportedConfigurations["core.system.ntp.servers"] = true supportedConfigurations["core.system.ntp.fallback-servers"] = true + supportedConfigurations["core.system.ntp.root-distance-max-sec"] = true + supportedConfigurations["core.system.ntp.poll-interval-min-sec"] = true + supportedConfigurations["core.system.ntp.poll-interval-max-sec"] = true + supportedConfigurations["core.system.ntp.connection-retry-sec"] = true + supportedConfigurations["core.system.ntp.save-interval-sec"] = true // and register it as a external config config.RegisterExternalConfig("core", "system.ntp", getNTPFromSystemHelper) } @@ -50,13 +58,23 @@ var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0 var validIPv4 = net.ParseIP var timesyncdToSnapKeyMapping = map[string]string{ - "NTP": "servers", - "FallbackNTP": "fallback-servers", + "NTP": "servers", + "FallbackNTP": "fallback-servers", + "RootDistanceMaxSec": "root-distance-max-sec", + "PollIntervalMinSec": "poll-interval-min-sec", + "PollIntervalMaxSec": "poll-interval-max-sec", + "ConnectionRetrySec": "connection-retry-sec", + "SaveIntervalSec": "save-interval-sec", } var snapToTimesyncdKeyMapping = map[string]string{ - "servers": "NTP", - "fallback-servers": "FallbackNTP", + "servers": "NTP", + "fallback-servers": "FallbackNTP", + "root-distance-max-sec": "RootDistanceMaxSec", + "poll-interval-min-sec": "PollIntervalMinSec", + "poll-interval-max-sec": "PollIntervalMaxSec", + "connection-retry-sec": "ConnectionRetrySec", + "save-interval-sec": "SaveIntervalSec", } func validateNTPSettings(tr ConfGetter) error { @@ -71,6 +89,30 @@ func validateNTPSettings(tr ConfGetter) error { } } + // Validate that poll-interval-min-sec > poll-interval-min-sec + // Use the systemd defaults if they have not been overwritten by the user + pollIntervalMinSecString := "32s" + pollIntervalMaxSecString := "2048s" + customPollInterval := false + // Validation for user submitted values has already been done + if minSec, exists := ntpCfg["poll-interval-min-sec"]; exists { + pollIntervalMinSecString, _ = mapSnapOptionValueToUnitOptionValue(minSec) + customPollInterval = true + } + if maxSec, exists := ntpCfg["poll-interval-max-sec"]; exists { + pollIntervalMaxSecString, _ = mapSnapOptionValueToUnitOptionValue(maxSec) + customPollInterval = true + } + + if customPollInterval { + pollIntervalMinUSec, _ := convertSystemdTimespanToUs(pollIntervalMinSecString) + pollIntervalMaxUSec, _ := convertSystemdTimespanToUs(pollIntervalMaxSecString) + + if pollIntervalMinUSec > pollIntervalMaxUSec { + return fmt.Errorf("invalid NTP configuration: poll-interval-min-sec (%q) cannot be greater than poll-interval-max-sec (%q)", pollIntervalMinSecString, pollIntervalMaxSecString) + } + } + return nil } @@ -90,6 +132,26 @@ func validateSingleNTPSetting(key string, value any) (err error) { } return nil + case "root-distance-max-sec", "poll-interval-min-sec", "poll-interval-max-sec", "connection-retry-sec", "save-interval-sec": + span, err := mapSnapOptionValueToUnitOptionValue(value) + if err != nil { + return fmt.Errorf("%v: %v", key, err) + } + + timespanUs, err := validateSystemdTimeSpanFormat(span) + if err != nil { + return fmt.Errorf("%v: %v", key, err) + } + + if key == "poll-interval-min-sec" && timespanUs < 16000000 { + return fmt.Errorf("poll-interval-min-sec: cannot be smaller than 16s.") + } + if key == "connection-retry-sec" && timespanUs < 1000000 { + return fmt.Errorf("connection-retry-sec: cannot be smaller than 1s.") + } + + return nil + default: return fmt.Errorf("unsupported configuration option %q", key) } @@ -116,6 +178,41 @@ func validateServerName(serverAddress string) error { return nil } +func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { + // The most reliable way to parse the timespans appears to be having + // systemd-analyze do it + // We also use this to compare min and max values as input validation + cmd := exec.Command("systemd-analyze", "timespan", span) + + output, err := cmd.CombinedOutput() + if err != nil { + return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) + } + + // Look for the line containing "μs:" or "us:" and capture the following digits + re := regexp.MustCompile(`(?:μs|us):\s*(\d+)`) + matches := re.FindStringSubmatch(string(output)) + + // We did not capture the two parts of the line + if len(matches) < 2 { + return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) + } + + // Parse the captured string digits into an int64 + us, err := strconv.ParseInt(matches[1], 10, 64) + if err != nil { + return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) + } + + return us, nil +} + +// The "parse" function is used for converting the value to a microsecond value +// It implicitly checks its format, so we re-use it for validation +func validateSystemdTimeSpanFormat(span string) (timeSpanUs int64, err error) { + return convertSystemdTimespanToUs(span) +} + func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyContext) error { var cfg map[string]any if err := tr.Get("core", "system.ntp", &cfg); err != nil && !config.IsNoOption(err) { @@ -158,10 +255,12 @@ func serializeNTPConfiguration(config map[string]any) (result []byte) { unitOptions := []*unit.UnitOption{} for k, v := range config { + unitOptionKey := mapSnapOptionNameToUnitOptionName(k) + unitOptionValue, _ := mapSnapOptionValueToUnitOptionValue(v) unitOption := *unit.NewUnitOption( "Time", - mapKeySnapToTimesyncd(k), - mapValueSnapToTimesyncd(v), + unitOptionKey, + unitOptionValue, ) unitOptions = append(unitOptions, &unitOption) } @@ -174,17 +273,20 @@ func getNTPFromSystemHelper(key string) (result any, err error) { return getNTPFromSystem() } -func mapValueTimesyncdToSnap(option *unit.UnitOption) (result any) { +func mapUnitOptionValueToSnapOptionValue(option *unit.UnitOption) (result any) { switch option.Name { case "NTP", "FallbackNTP": return strings.Split(option.Value, " ") + case "RootDistanceMaxSec", "PollIntervalMinSec", "PollIntervalMaxSec", "ConnectionRetrySec", "SaveIntervalSec": + return option.Value + default: return "" } } -func mapValueSnapToTimesyncd(option any) string { +func mapSnapOptionValueToUnitOptionValue(option any) (string, error) { switch option := option.(type) { case []any: // Snapd will return a []any for lists. We need to check @@ -196,18 +298,24 @@ func mapValueSnapToTimesyncd(option any) string { } } - return strings.Join(builder, " ") + return strings.Join(builder, " "), nil + + case string: + return option, nil + + case json.Number: + return option.String() + "s", nil default: - return "" + return "", fmt.Errorf("invalid option type: %T", option) } } -func mapKeyTimesyncdToSnap(timesyncdOption string) string { +func mapUnitOptionNametoSnapOptionName(timesyncdOption string) string { return timesyncdToSnapKeyMapping[timesyncdOption] } -func mapKeySnapToTimesyncd(snapOption string) string { +func mapSnapOptionNameToUnitOptionName(snapOption string) string { return snapToTimesyncdKeyMapping[snapOption] } @@ -233,8 +341,8 @@ func getNTPFromSystem() (result map[string]any, err error) { val := map[string]any{} for _, option := range unitOptions { - snapOptionName := mapKeyTimesyncdToSnap(option.Name) - snapOptionValue := mapValueTimesyncdToSnap(option) + snapOptionName := mapUnitOptionNametoSnapOptionName(option.Name) + snapOptionValue := mapUnitOptionValueToSnapOptionValue(option) val[snapOptionName] = snapOptionValue } From d4bc54c80ee5ce7a69274d88baa46091561bd11b Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Fri, 8 May 2026 17:31:19 +0300 Subject: [PATCH 03/27] o/c/c/ntp: skip writing the configuration if it has not changed, naming improvements, bugfixes Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 126 ++++++++++++++++++------- 1 file changed, 94 insertions(+), 32 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 24a601d49a7..8481a40e7bc 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -57,6 +57,9 @@ func init() { var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`).MatchString var validIPv4 = net.ParseIP +// Match the line containing "μs:" or "us:" and capture the following digits +var timespanUsRegexp = regexp.MustCompile(`(?:μs|us):\s*(\d+)`) + var timesyncdToSnapKeyMapping = map[string]string{ "NTP": "servers", "FallbackNTP": "fallback-servers", @@ -89,18 +92,18 @@ func validateNTPSettings(tr ConfGetter) error { } } - // Validate that poll-interval-min-sec > poll-interval-min-sec + // Validate that poll-interval-min-sec < poll-interval-max-sec // Use the systemd defaults if they have not been overwritten by the user pollIntervalMinSecString := "32s" pollIntervalMaxSecString := "2048s" customPollInterval := false // Validation for user submitted values has already been done if minSec, exists := ntpCfg["poll-interval-min-sec"]; exists { - pollIntervalMinSecString, _ = mapSnapOptionValueToUnitOptionValue(minSec) + pollIntervalMinSecString, _ = mapOptionValueSnapToTimesyncd(minSec) customPollInterval = true } if maxSec, exists := ntpCfg["poll-interval-max-sec"]; exists { - pollIntervalMaxSecString, _ = mapSnapOptionValueToUnitOptionValue(maxSec) + pollIntervalMaxSecString, _ = mapOptionValueSnapToTimesyncd(maxSec) customPollInterval = true } @@ -127,13 +130,10 @@ func validateSingleNTPSetting(key string, value any) (err error) { return fmt.Errorf("%v is an empty list", key) } - if err := validateNTPServers(servers); err != nil { - return err - } - return nil + return validateNTPServers(servers) case "root-distance-max-sec", "poll-interval-min-sec", "poll-interval-max-sec", "connection-retry-sec", "save-interval-sec": - span, err := mapSnapOptionValueToUnitOptionValue(value) + span, err := mapOptionValueSnapToTimesyncd(value) if err != nil { return fmt.Errorf("%v: %v", key, err) } @@ -144,10 +144,10 @@ func validateSingleNTPSetting(key string, value any) (err error) { } if key == "poll-interval-min-sec" && timespanUs < 16000000 { - return fmt.Errorf("poll-interval-min-sec: cannot be smaller than 16s.") + return fmt.Errorf("poll-interval-min-sec: cannot be smaller than 16s") } if key == "connection-retry-sec" && timespanUs < 1000000 { - return fmt.Errorf("connection-retry-sec: cannot be smaller than 1s.") + return fmt.Errorf("connection-retry-sec: cannot be smaller than 1s") } return nil @@ -190,10 +190,9 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { } // Look for the line containing "μs:" or "us:" and capture the following digits - re := regexp.MustCompile(`(?:μs|us):\s*(\d+)`) - matches := re.FindStringSubmatch(string(output)) + matches := timespanUsRegexp.FindStringSubmatch(string(output)) - // We did not capture the two parts of the line + // We did not capture the two parts of the line ("us:" and the digits) if len(matches) < 2 { return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) } @@ -207,22 +206,69 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { return us, nil } -// The "parse" function is used for converting the value to a microsecond value -// It implicitly checks its format, so we re-use it for validation +// validateSystemdTimeSpanFormat validates and converts a systemd timespan string to microseconds. +// It reuses the parsing logic from convertSystemdTimespanToUs for validation. func validateSystemdTimeSpanFormat(span string) (timeSpanUs int64, err error) { return convertSystemdTimespanToUs(span) } +// NTPConfigurationDeepEqual compares two NTP configurations and returns true if they are equal, +// false otherwise. +// The standard reflect.DeepEqual cannot be used to compare the configurations as the values +// of fields "servers" and "fallback-servers" parsed by unit.Deserialize (i.e. the oldConfig) +// are of type []string, while the ones coming from snapd are of type []any. +// reflect.DeepEqual considers them as different, but we want to consider them as equal as +// long as their content is the same. +func NTPConfigurationDeepEqual(oldConfig, newConfig map[string]any) bool { + // Check if both maps have the same number of keys + // Automatically handles empty and nil maps + if len(oldConfig) != len(newConfig) { + return false + } + + for key, oldVal := range oldConfig { + newVal, exists := newConfig[key] + if !exists { + return false + } + + // Use the mapping function to convert all values to their string representation and + // compare those + // Takes case of converting and comparing []any (config read from snapd) and []string + // (config read from the config file) + oldValString, err := mapOptionValueSnapToTimesyncd(oldVal) + if err != nil { + return false + } + newValString, err := mapOptionValueSnapToTimesyncd(newVal) + if err != nil { + return false + } + if oldValString != newValString { + return false + } + } + + return true +} + func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyContext) error { var cfg map[string]any - if err := tr.Get("core", "system.ntp", &cfg); err != nil && !config.IsNoOption(err) { + err := tr.Get("core", "system.ntp", &cfg) + if config.IsNoOption(err) { + return nil + } + if err != nil { return fmt.Errorf("cannot get NTP config: %v", err) } - rootDir := dirs.GlobalRootDir - if opts != nil { - // runtime system - rootDir = opts.RootDir + oldConfig, err := getNTPFromSystem() + if err != nil { + return err + } + if NTPConfigurationDeepEqual(oldConfig, cfg) { + // If the configuration has not changed, do nothing. + return nil } // Check that the updated configuration is valid @@ -230,6 +276,12 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte return err } + rootDir := dirs.GlobalRootDir + if opts != nil { + // runtime system + rootDir = opts.RootDir + } + // Create systemd configuration folder, if not present systemdConfigFolder := filepath.Join(rootDir, "/etc/systemd/") if err := os.MkdirAll(systemdConfigFolder, 0755); err != nil { @@ -255,8 +307,8 @@ func serializeNTPConfiguration(config map[string]any) (result []byte) { unitOptions := []*unit.UnitOption{} for k, v := range config { - unitOptionKey := mapSnapOptionNameToUnitOptionName(k) - unitOptionValue, _ := mapSnapOptionValueToUnitOptionValue(v) + unitOptionKey := mapOptionNameSnapToTimesyncd(k) + unitOptionValue, _ := mapOptionValueSnapToTimesyncd(v) unitOption := *unit.NewUnitOption( "Time", unitOptionKey, @@ -273,7 +325,7 @@ func getNTPFromSystemHelper(key string) (result any, err error) { return getNTPFromSystem() } -func mapUnitOptionValueToSnapOptionValue(option *unit.UnitOption) (result any) { +func mapOptionValueTimesyncdToSnap(option *unit.UnitOption) (result any) { switch option.Name { case "NTP", "FallbackNTP": return strings.Split(option.Value, " ") @@ -286,24 +338,33 @@ func mapUnitOptionValueToSnapOptionValue(option *unit.UnitOption) (result any) { } } -func mapSnapOptionValueToUnitOptionValue(option any) (string, error) { +func mapOptionValueSnapToTimesyncd(option any) (string, error) { switch option := option.(type) { + case []string: + // Matches fields "servers" and "fallback-servers" parsed from the current + // configuration file + return strings.Join(option, " "), nil + case []any: // Snapd will return a []any for lists. We need to check // that each element is a string manually var builder []string for _, server := range option { - if s, ok := server.(string); ok { - builder = append(builder, s) + s, ok := server.(string) + if !ok { + return "", fmt.Errorf("list contains non-string element: %T", server) } + builder = append(builder, s) } return strings.Join(builder, " "), nil case string: + // Matches timespan fields that are set as a string (e.g. "poll-interval-min-sec": "32s") return option, nil case json.Number: + // Matches timespan fields that are set without a unit (e.g. "poll-interval-min-sec": 32) return option.String() + "s", nil default: @@ -311,11 +372,11 @@ func mapSnapOptionValueToUnitOptionValue(option any) (string, error) { } } -func mapUnitOptionNametoSnapOptionName(timesyncdOption string) string { +func mapOptionNameTimesyncdToSnap(timesyncdOption string) string { return timesyncdToSnapKeyMapping[timesyncdOption] } -func mapSnapOptionNameToUnitOptionName(snapOption string) string { +func mapOptionNameSnapToTimesyncd(snapOption string) string { return snapToTimesyncdKeyMapping[snapOption] } @@ -324,8 +385,10 @@ func getNTPFromSystem() (result map[string]any, err error) { return nil, nil } - file, err := os.Open("/etc/systemd/timesyncd.conf") + file, err := os.Open(filepath.Join(dirs.GlobalRootDir, "/etc/systemd/timesyncd.conf")) if os.IsNotExist(err) { + // A missing file is not an error, it just means that there is no custom configuration and + // the system is using the defaults return map[string]any{}, nil } if err != nil { @@ -339,10 +402,9 @@ func getNTPFromSystem() (result map[string]any, err error) { } val := map[string]any{} - for _, option := range unitOptions { - snapOptionName := mapUnitOptionNametoSnapOptionName(option.Name) - snapOptionValue := mapUnitOptionValueToSnapOptionValue(option) + snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) + snapOptionValue := mapOptionValueTimesyncdToSnap(option) val[snapOptionName] = snapOptionValue } From 067e83c4adc6cade1af6d1c8633cb1c32c02b182 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 12 May 2026 11:00:53 +0300 Subject: [PATCH 04/27] o/c/c/ntp_test: added tests for getting and setting core.system.ntp configuration Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp_test.go | 431 ++++++++++++++++++++ 1 file changed, 431 insertions(+) create mode 100644 overlord/configstate/configcore/ntp_test.go diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go new file mode 100644 index 00000000000..eda475f071e --- /dev/null +++ b/overlord/configstate/configcore/ntp_test.go @@ -0,0 +1,431 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +package configcore_test + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + + . "gopkg.in/check.v1" + + "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/overlord/configstate/config" + "github.com/snapcore/snapd/overlord/configstate/configcore" + "github.com/snapcore/snapd/release" + "github.com/snapcore/snapd/systemd" +) + +type ntpSuite struct { + configcoreSuite + timesyncdConfigFile string + sysdLog [][]string +} + +var ( + _ = Suite(&ntpSuite{}) + startingFileContent = []string{ + "[Time]", + "RootDistanceMaxSec=5s", + "NTP=ntp.ubuntu.com", + } + validConfigurationExample = map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + "192.168.15.1", + "ntp.ubuntu.com", + }, + }, + } +) + +func (s *ntpSuite) SetUpTest(c *C) { + s.configcoreSuite.SetUpTest(c) + + c.Assert(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "etc/systemd"), 0755), IsNil) + + // Create the config file and write a predictable configuration + systemdConfigFolder := filepath.Join(dirs.GlobalRootDir, "etc/systemd") + err := os.MkdirAll(systemdConfigFolder, 0755) + c.Assert(err, IsNil) + s.timesyncdConfigFile = filepath.Join(systemdConfigFolder, "timesyncd.conf") + err = os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + c.Assert(err, IsNil) + + // We are on Core + restore := release.MockOnClassic(false) + s.AddCleanup(restore) +} + +func (s *ntpSuite) verifyConfigfileContent(c *C, expectedContent []string, comment string) { + // The serialization order of the individual options is not predictable. Read the file, sort + // the lines and compare them against the (already sorted) expected content. + content, _ := os.ReadFile(s.timesyncdConfigFile) + lines := strings.Split(strings.TrimSpace(string(content)), "\n") + slices.Sort(lines) + // Reverse so the output looks "normal" with the "[Time]" line at the top + slices.Reverse(lines) + + if len(expectedContent) != 0 { + c.Check([]string(lines), DeepEquals, expectedContent, Commentf("%v", comment)) + } else { + c.Check([]string(lines), DeepEquals, startingFileContent, Commentf("%v", comment)) + } + // Reset configuration file to the default value for the next test + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644), IsNil) +} + +// Test setting various configurations, with multiple valid and invalid configurations +func (s *ntpSuite) TestNTPSetValidateValues(c *C) { + var getConfigurationTests = []struct { + newConfig map[string]any + expectedFileContent []string + expectServiceRestart bool + expectedError string + }{ + // 0: Valid configuration with all keys, including json.Number, different units + // and missing units + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + "192.168.15.1", + "ntp.ubuntu.com", + }, + "fallback-servers": []any{ + "192.168.1.100", + "pool.ntp.org", + }, + "root-distance-max-sec": json.Number(fmt.Sprint(5)), + "poll-interval-min-sec": "30s", + "poll-interval-max-sec": "40m", + "connection-retry-sec": "5s", + "save-interval-sec": "20", + }, + }, + // Keep sorted in reverse order. + // unit.Serialize does not serialize options in a predictable order, so + // the test reads the file and sorts its lines. The reverse order makes + // this look like a normal unit file + expectedFileContent: []string{ + "[Time]", + "SaveIntervalSec=20", + "RootDistanceMaxSec=5s", + "PollIntervalMinSec=30s", + "PollIntervalMaxSec=40m", + "NTP=192.168.15.1 ntp.ubuntu.com", + "FallbackNTP=192.168.1.100 pool.ntp.org", + "ConnectionRetrySec=5s", + }, + expectServiceRestart: true, + expectedError: "", + }, + // 1: Valid empty configuration + { + newConfig: map[string]any{ + "system.ntp": map[string]any{}, + }, + expectedFileContent: []string{""}, + expectServiceRestart: true, + expectedError: "", + }, + // 2: Valid configuration identical to current one + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + }, + }, + expectServiceRestart: false, + expectedFileContent: startingFileContent, + }, + // 3: Invalid format for server list + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": "192.168.15.1", + }, + }, + expectedError: "invalid NTP configuration: servers is not a list of server names", + }, + // 4: Empty server list + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{}, + }, + }, + expectedError: "invalid NTP configuration: servers is an empty list", + }, + // 5: Wrong value type in server list + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + 42, + }, + }, + }, + expectedError: "invalid NTP configuration: '\\*' is not a valid string", + }, + // 6: Malformed IP address in server list + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + "192.168.1....", + }, + }, + }, + expectedError: "invalid NTP configuration: \"192.168.1....\" is not a valid server name", + }, + // 7: Malformed domain name in server list + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "servers": []any{ + "canonical......com", + }, + }, + }, + expectedError: "invalid NTP configuration: \"canonical......com\" is not a valid server name", + }, + // 8: Invalid configuration with wrong systemd timespans + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "root-distance-max-sec": "not-a-timespan", + }, + }, + expectedError: "invalid NTP configuration: root-distance-max-sec: \"not-a-timespan\" is not a valid systemd.time timespan", + }, + // 9: poll-interval-min-sec greater than poll-interval-max-sec + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "poll-interval-min-sec": "30m", + "poll-interval-max-sec": "1m", + }, + }, + expectedError: "invalid NTP configuration: poll-interval-min-sec \\(\"30m\"\\) cannot be greater than poll-interval-max-sec \\(\"1m\"\\)", + }, + // 10: poll-interval-min-sec lower than minimum 16s + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "poll-interval-min-sec": "5s", + }, + }, + expectedError: "invalid NTP configuration: poll-interval-min-sec: cannot be smaller than 16s", + }, + // 11: connection-retry-sec lower than minimum 1s + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "connection-retry-sec": "100ms", + }, + }, + expectedError: "invalid NTP configuration: connection-retry-sec: cannot be smaller than 1s", + }, + // 12: unsupported configuration option + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "new-option": "new-value", + }, + }, + expectedError: "invalid NTP configuration: unsupported configuration option \"new-option\"", + }, + // 13: invalid option type for a timespan option + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "poll-interval-min-sec": 2.0, + }, + }, + expectedError: "invalid NTP configuration: poll-interval-min-sec: invalid option type: float64", + }, + } + + for i, test := range getConfigurationTests { + // Apply the config + conf := configcore.PlainCoreConfig(test.newConfig) + err := configcore.FilesystemOnlyRun(core24Dev, conf) + + // Verify correct error is returned + if test.expectedError == "" { + c.Check(err, IsNil, Commentf("config validation: %v", i)) + } else { + c.Check(err, ErrorMatches, test.expectedError, Commentf("config validation: %v", i)) + } + + // The configuration file content is either the new configuration if it was valid, or + // the original configuration if the new one was invalid + s.verifyConfigfileContent(c, test.expectedFileContent, fmt.Sprintf("config validation: %v", i)) + + // Check that the timesyncd service was restarted only if necessary, i.e. the new + // configuration was valid and different from the original one + if test.expectServiceRestart { + c.Check(s.systemctlArgs, DeepEquals, [][]string{ + {"reload-or-restart", "systemd-timesyncd.service"}, + }, Commentf("config validation: %v", i)) + s.systemctlArgs = nil + } else { + c.Check(s.systemctlArgs, IsNil, Commentf("config validation: %v", i)) + } + } +} + +// Test that setting a valid configuration fails when the systemd folder cannot be accessed due to +// missing write permissions +func (s *ntpSuite) TestNTPSetValidConfigurationMissingFolderPermissions(c *C) { + systemdConfigFolder := filepath.Join(dirs.GlobalRootDir, "etc/systemd") + c.Assert(os.Chmod(systemdConfigFolder, 0111), IsNil) + defer os.Chmod(systemdConfigFolder, 0755) + + conf := configcore.PlainCoreConfig(validConfigurationExample) + + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Assert(err, ErrorMatches, "cannot write NTP configuration: open .*/etc/systemd/timesyncd.conf.*: permission denied") + s.verifyConfigfileContent(c, startingFileContent, "") +} + +func (s *ntpSuite) TestNTPSetRestartDaemonError(c *C) { + r := systemd.MockSystemctl(func(cmd ...string) ([]byte, error) { + s.sysdLog = append(s.sysdLog, cmd) + return nil, fmt.Errorf("boom") + }) + defer r() + + conf := configcore.PlainCoreConfig(validConfigurationExample) + + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Check(err, ErrorMatches, "cannot restart timesyncd daemon after configuration change: boom") +} + +func (s *ntpSuite) TestNTPGetMissingConfigFile(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // Remove config file + os.Remove(s.timesyncdConfigFile) + + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, IsNil) + c.Check(ntpConfig, DeepEquals, map[string]any{}) +} + +func (s *ntpSuite) TestNTPGetErrorOpeningFile(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // Change file permissions to inhibit reading it + os.Chmod(s.timesyncdConfigFile, 0000) + defer os.Chmod(s.timesyncdConfigFile, 0644) + + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot read NTP configuration file /etc/systemd/timesyncd.conf: open .*/etc/systemd/timesyncd.conf: permission denied") +} + +func (s *ntpSuite) TestNTPGetInvalidSystemdUnit(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // Write corrupted unit + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[invalid-unit"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot parse systemd unit in configuration file /etc/systemd/timesyncd.conf: unable to find end of section") +} + +func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { + // Test identical configurations are equal + config1 := map[string]any{ + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + } + config2 := map[string]any{ + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config2), Equals, true) + + // Test different server lists are not equal + config3 := map[string]any{ + "servers": []any{ + "192.168.1.2", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config3), Equals, false) + + // Test different timespan values are not equal + config4 := map[string]any{ + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "10s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config4), Equals, false) + + // Test empty configurations are equal + config5 := map[string]any{} + c.Check(configcore.NTPConfigurationDeepEqual(config5, config5), Equals, true) + + // Test configuration with additional fields + config6 := map[string]any{ + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + "poll-interval-min-sec": "16s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config6), Equals, false) + + // Test server list as []string result of parsing timesyncd.conf (oldConfig), instead of + // []any coming from tr.Get call (newConfig). The slices should be considered equal even + // if their type is different, as long as their content is the same. + config7 := map[string]any{ + "servers": []string{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-max-sec": "5s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config7), Equals, true) + + // Test identical "nil" configurations are equal + var config8 map[string]any + c.Check(configcore.NTPConfigurationDeepEqual(config8, config8), Equals, true) + + // Test "nil" configuration is "equal" to empty configuration + c.Check(configcore.NTPConfigurationDeepEqual(config8, config5), Equals, true) + + // Test "nil" and empty configurations are different from any configuration + c.Check(configcore.NTPConfigurationDeepEqual(config8, config1), Equals, false) + c.Check(configcore.NTPConfigurationDeepEqual(config5, config1), Equals, false) +} From ad1be515351d3144ec43cd19fea5bad3e04ae2b8 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 12 May 2026 15:59:57 +0300 Subject: [PATCH 05/27] o/c/c/ntp: support unsetting system.ntp key by deleting the config file Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 23 +++++++++--- overlord/configstate/configcore/ntp_test.go | 39 +++++++++++++++++---- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 8481a40e7bc..ac23e27d58f 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -42,6 +42,7 @@ import ( func init() { // add supported configuration of this module + supportedConfigurations["core.system.ntp"] = true supportedConfigurations["core.system.ntp.servers"] = true supportedConfigurations["core.system.ntp.fallback-servers"] = true supportedConfigurations["core.system.ntp.root-distance-max-sec"] = true @@ -288,10 +289,19 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte return err } - // Write validated configuration to file + // Configuration file path ntpConfigPath := filepath.Join(systemdConfigFolder, "/timesyncd.conf") - if err := osutil.AtomicWriteFile(ntpConfigPath, serializeNTPConfiguration(cfg), 0644, 0); err != nil { - return fmt.Errorf("cannot write NTP configuration: %v", err) + + if len(cfg) == 0 { + // If the config is empty, we want to reset to defaults, which is achieved by deleting the configuration file + if err := os.Remove(ntpConfigPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("cannot reset NTP configuration to defaults: %v", err) + } + } else { + // Otherwise, we overwrite the file with the new configuration + if err := osutil.AtomicWriteFile(ntpConfigPath, serializeNTPConfiguration(cfg), 0644, 0); err != nil { + return fmt.Errorf("cannot write NTP configuration: %v", err) + } } // Restart systemd-timesyncd.service to pick up the updated configuration @@ -389,7 +399,7 @@ func getNTPFromSystem() (result map[string]any, err error) { if os.IsNotExist(err) { // A missing file is not an error, it just means that there is no custom configuration and // the system is using the defaults - return map[string]any{}, nil + return nil, nil } if err != nil { return nil, fmt.Errorf("cannot read NTP configuration file /etc/systemd/timesyncd.conf: %v", err) @@ -401,6 +411,11 @@ func getNTPFromSystem() (result map[string]any, err error) { return nil, fmt.Errorf("cannot parse systemd unit in configuration file /etc/systemd/timesyncd.conf: %v", err) } + // Do not return an empty config "system.ntp {}" if there is no custom configuration in the file + if len(unitOptions) == 0 { + return nil, nil + } + val := map[string]any{} for _, option := range unitOptions { snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index eda475f071e..ea4b9ca18e7 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -60,6 +60,11 @@ func (s *ntpSuite) SetUpTest(c *C) { s.AddCleanup(restore) } +func (s *ntpSuite) writeExampleConfigFile(c *C) { + err := os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + c.Assert(err, IsNil) +} + func (s *ntpSuite) verifyConfigfileContent(c *C, expectedContent []string, comment string) { // The serialization order of the individual options is not predictable. Read the file, sort // the lines and compare them against the (already sorted) expected content. @@ -75,13 +80,14 @@ func (s *ntpSuite) verifyConfigfileContent(c *C, expectedContent []string, comme c.Check([]string(lines), DeepEquals, startingFileContent, Commentf("%v", comment)) } // Reset configuration file to the default value for the next test - c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644), IsNil) + s.writeExampleConfigFile(c) } // Test setting various configurations, with multiple valid and invalid configurations func (s *ntpSuite) TestNTPSetValidateValues(c *C) { var getConfigurationTests = []struct { newConfig map[string]any + expectedFileDeleted bool expectedFileContent []string expectServiceRestart bool expectedError string @@ -128,7 +134,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { newConfig: map[string]any{ "system.ntp": map[string]any{}, }, - expectedFileContent: []string{""}, + expectedFileDeleted: true, expectServiceRestart: true, expectedError: "", }, @@ -267,7 +273,14 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { // The configuration file content is either the new configuration if it was valid, or // the original configuration if the new one was invalid - s.verifyConfigfileContent(c, test.expectedFileContent, fmt.Sprintf("config validation: %v", i)) + // If the configuration is empty, the file should be deleted + if test.expectedFileDeleted { + _, err = os.Lstat(s.timesyncdConfigFile) + c.Check(os.IsNotExist(err), Equals, true, Commentf("config validation: %v", i)) + } else { + s.verifyConfigfileContent(c, test.expectedFileContent, fmt.Sprintf("config validation: %v", i)) + } + s.writeExampleConfigFile(c) // Check that the timesyncd service was restarted only if necessary, i.e. the new // configuration was valid and different from the original one @@ -275,10 +288,10 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { c.Check(s.systemctlArgs, DeepEquals, [][]string{ {"reload-or-restart", "systemd-timesyncd.service"}, }, Commentf("config validation: %v", i)) - s.systemctlArgs = nil } else { c.Check(s.systemctlArgs, IsNil, Commentf("config validation: %v", i)) } + s.systemctlArgs = nil } } @@ -320,8 +333,8 @@ func (s *ntpSuite) TestNTPGetMissingConfigFile(c *C) { var ntpConfig map[string]any err := tr.Get("core", "system.ntp", &ntpConfig) - c.Assert(err, IsNil) - c.Check(ntpConfig, DeepEquals, map[string]any{}) + c.Assert(err, ErrorMatches, "snap \"core\" has no \"system.ntp\" configuration option") + c.Check(ntpConfig, DeepEquals, map[string]any(nil)) } func (s *ntpSuite) TestNTPGetErrorOpeningFile(c *C) { @@ -353,6 +366,20 @@ func (s *ntpSuite) TestNTPGetInvalidSystemdUnit(c *C) { c.Assert(err, ErrorMatches, "cannot parse systemd unit in configuration file /etc/systemd/timesyncd.conf: unable to find end of section") } +func (s *ntpSuite) TestNTPGetEmptySystemdUnit(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // Write unit with no options + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "snap \"core\" has no \"system.ntp\" configuration option") +} + func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { // Test identical configurations are equal config1 := map[string]any{ From 86af750a4f344bdc6f96ace2cdc8ebd377faadf9 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 12 May 2026 17:48:50 +0300 Subject: [PATCH 06/27] o/c/c/ntp: removed validation in handle function, ignore unsupported keys in config file, expand test coverage Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 26 +++-- overlord/configstate/configcore/ntp_test.go | 109 ++++++++++++++++++++ 2 files changed, 125 insertions(+), 10 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index ac23e27d58f..52dbb51eefa 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -272,11 +272,6 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte return nil } - // Check that the updated configuration is valid - if err := validateNTPSettings(tr); err != nil { - return err - } - rootDir := dirs.GlobalRootDir if opts != nil { // runtime system @@ -284,13 +279,13 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte } // Create systemd configuration folder, if not present - systemdConfigFolder := filepath.Join(rootDir, "/etc/systemd/") + systemdConfigFolder := filepath.Join(rootDir, "etc", "systemd") if err := os.MkdirAll(systemdConfigFolder, 0755); err != nil { return err } // Configuration file path - ntpConfigPath := filepath.Join(systemdConfigFolder, "/timesyncd.conf") + ntpConfigPath := filepath.Join(systemdConfigFolder, "timesyncd.conf") if len(cfg) == 0 { // If the config is empty, we want to reset to defaults, which is achieved by deleting the configuration file @@ -383,11 +378,17 @@ func mapOptionValueSnapToTimesyncd(option any) (string, error) { } func mapOptionNameTimesyncdToSnap(timesyncdOption string) string { - return timesyncdToSnapKeyMapping[timesyncdOption] + if v, ok := timesyncdToSnapKeyMapping[timesyncdOption]; ok { + return v + } + return "" } func mapOptionNameSnapToTimesyncd(snapOption string) string { - return snapToTimesyncdKeyMapping[snapOption] + if v, ok := snapToTimesyncdKeyMapping[snapOption]; ok { + return v + } + return "" } func getNTPFromSystem() (result map[string]any, err error) { @@ -395,7 +396,7 @@ func getNTPFromSystem() (result map[string]any, err error) { return nil, nil } - file, err := os.Open(filepath.Join(dirs.GlobalRootDir, "/etc/systemd/timesyncd.conf")) + file, err := os.Open(filepath.Join(dirs.GlobalRootDir, "etc", "systemd", "timesyncd.conf")) if os.IsNotExist(err) { // A missing file is not an error, it just means that there is no custom configuration and // the system is using the defaults @@ -420,6 +421,11 @@ func getNTPFromSystem() (result map[string]any, err error) { for _, option := range unitOptions { snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) snapOptionValue := mapOptionValueTimesyncdToSnap(option) + + if snapOptionName == "" { + // Do not error out on unsupported options inserted in the config file manually, just ignore them + continue + } val[snapOptionName] = snapOptionValue } diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index ea4b9ca18e7..53310b46577 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -17,6 +17,7 @@ import ( "github.com/snapcore/snapd/overlord/configstate/configcore" "github.com/snapcore/snapd/release" "github.com/snapcore/snapd/systemd" + "github.com/snapcore/snapd/testutil" ) type ntpSuite struct { @@ -40,6 +41,10 @@ var ( }, }, } + configurationExampleFileContent = []string{ + "[Time]", + "NTP=192.168.15.1 ntp.ubuntu.com", + } ) func (s *ntpSuite) SetUpTest(c *C) { @@ -295,6 +300,61 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { } } +func (s *ntpSuite) TestNTPSetSystemdAnalyzeError(c *C) { + // Systemd-analyze returns the wrong format for the timespan value + // It will return non-zero exit code if the analysis fails, so we need to mock it here to check + // that we handle the format well and return the correct error message + sysdAnalyzeCmdInvalidResponse := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: xxxx'") + defer sysdAnalyzeCmdInvalidResponse.Restore() + + // Apply the config + invalidReponseConfig := configcore.PlainCoreConfig(map[string]any{ + "system.ntp": map[string]any{ + "connection-retry-sec": "xxxx", + }}) + err := configcore.FilesystemOnlyRun(core24Dev, invalidReponseConfig) + + // Verify correct error is returned + c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-sec: \"xxxx\" is not a valid systemd.time timespan") + s.verifyConfigfileContent(c, startingFileContent, "") + c.Check(s.systemctlArgs, IsNil) + + // Return value that it too big for Int64 (9223372036854775807 + 1) + sysdAnalyzeCmdBigNumber := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: 9223372036854775808'") + defer sysdAnalyzeCmdBigNumber.Restore() + + // Apply the config + invalidNumberConfig := configcore.PlainCoreConfig(map[string]any{ + "system.ntp": map[string]any{ + "connection-retry-sec": "9223372036854s", + }}) + err = configcore.FilesystemOnlyRun(core24Dev, invalidNumberConfig) + + // Verify correct error is returned + c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-sec: \"9223372036854s\" is not a valid systemd.time timespan") + s.verifyConfigfileContent(c, startingFileContent, "") + c.Check(s.systemctlArgs, IsNil) +} + +// Test that setting a valid configuration fails when the systemd folder cannot be accessed due to +// missing write permissions +func (s *ntpSuite) TestNTPSetCannotCreateSystemdFolder(c *C) { + etcFolder := filepath.Join(dirs.GlobalRootDir, "etc") + systemdConfigFolder := filepath.Join(etcFolder, "systemd") + systemdConfigFolderAlternateName := filepath.Join(etcFolder, "systemd.bak") + // Instead of removing it, we rename it to avoid issues for other files + os.Rename(systemdConfigFolder, systemdConfigFolderAlternateName) + // Remove write permission for /etc so that /etc/systemd cannot be recreated + c.Assert(os.Chmod(etcFolder, 0111), IsNil) + defer os.Chmod(etcFolder, 0755) + defer os.Rename(systemdConfigFolderAlternateName, systemdConfigFolder) + + conf := configcore.PlainCoreConfig(validConfigurationExample) + + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Assert(err, ErrorMatches, "mkdir /tmp/check-.*/etc/systemd: permission denied") +} + // Test that setting a valid configuration fails when the systemd folder cannot be accessed due to // missing write permissions func (s *ntpSuite) TestNTPSetValidConfigurationMissingFolderPermissions(c *C) { @@ -309,6 +369,19 @@ func (s *ntpSuite) TestNTPSetValidConfigurationMissingFolderPermissions(c *C) { s.verifyConfigfileContent(c, startingFileContent, "") } +func (s *ntpSuite) TestNTPSetErrorReadingDiskConfiguration(c *C) { + // Remove config file + os.Remove(s.timesyncdConfigFile) + + conf := configcore.PlainCoreConfig(validConfigurationExample) + + // The config file not being present is not an error and the configuration is + // set successfully + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Assert(err, IsNil) + s.verifyConfigfileContent(c, configurationExampleFileContent, "") +} + func (s *ntpSuite) TestNTPSetRestartDaemonError(c *C) { r := systemd.MockSystemctl(func(cmd ...string) ([]byte, error) { s.sysdLog = append(s.sysdLog, cmd) @@ -380,6 +453,23 @@ func (s *ntpSuite) TestNTPGetEmptySystemdUnit(c *C) { c.Assert(err, ErrorMatches, "snap \"core\" has no \"system.ntp\" configuration option") } +func (s *ntpSuite) TestNTPGetUnsupportedOption(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // The unsupported option is skipped and does not cause an error, but the rest of the configuration is still read correctly + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nUnsupportedOption=1\nSaveIntervalSec=10s"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, IsNil) + c.Assert(ntpConfig, DeepEquals, map[string]any{ + "save-interval-sec": "10s", + }) +} + func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { // Test identical configurations are equal config1 := map[string]any{ @@ -455,4 +545,23 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { // Test "nil" and empty configurations are different from any configuration c.Check(configcore.NTPConfigurationDeepEqual(config8, config1), Equals, false) c.Check(configcore.NTPConfigurationDeepEqual(config5, config1), Equals, false) + + // Test configuration with removed fields + config9 := map[string]any{ + "poll-interval-min-sec": "16s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config9), Equals, false) + + // Test configuration with wrong types for values + // Check both as first and second argument to hit both branches of the function + config10 := map[string]any{ + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + 12.5, + }, + "root-distance-max-sec": "5s", + } + c.Check(configcore.NTPConfigurationDeepEqual(config1, config10), Equals, false) + c.Check(configcore.NTPConfigurationDeepEqual(config10, config1), Equals, false) } From 6a7840e6337b04aa1d848a91ae97bb74d5049127 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Wed, 13 May 2026 11:04:02 +0300 Subject: [PATCH 07/27] o/c/c/ntp_test: fixed import errors in unit tests Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 53310b46577..73905392420 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -7,7 +7,7 @@ import ( "fmt" "os" "path/filepath" - "slices" + "sort" "strings" . "gopkg.in/check.v1" @@ -75,9 +75,8 @@ func (s *ntpSuite) verifyConfigfileContent(c *C, expectedContent []string, comme // the lines and compare them against the (already sorted) expected content. content, _ := os.ReadFile(s.timesyncdConfigFile) lines := strings.Split(strings.TrimSpace(string(content)), "\n") - slices.Sort(lines) - // Reverse so the output looks "normal" with the "[Time]" line at the top - slices.Reverse(lines) + // Sort in reverse order so the output looks "normal" with the "[Time]" line at the top + sort.Sort(sort.Reverse(sort.StringSlice(lines))) if len(expectedContent) != 0 { c.Check([]string(lines), DeepEquals, expectedContent, Commentf("%v", comment)) From 0f4b81f6c98c0b70f1cc2f77a7d6fca04ea797f5 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Wed, 13 May 2026 17:27:37 +0300 Subject: [PATCH 08/27] o/c/c/ntp: improved handling of empty options in configuration file Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 24 +++++++++++---------- overlord/configstate/configcore/ntp_test.go | 24 ++++++++++++++++++++- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 52dbb51eefa..87400888c30 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -127,9 +127,6 @@ func validateSingleNTPSetting(key string, value any) (err error) { if !ok { return fmt.Errorf("%v is not a list of server names", key) } - if len(servers) == 0 { - return fmt.Errorf("%v is an empty list", key) - } return validateNTPServers(servers) @@ -333,10 +330,14 @@ func getNTPFromSystemHelper(key string) (result any, err error) { func mapOptionValueTimesyncdToSnap(option *unit.UnitOption) (result any) { switch option.Name { case "NTP", "FallbackNTP": - return strings.Split(option.Value, " ") + trimmedString := strings.TrimSpace(option.Value) + if len(trimmedString) == 0 { + return []string{} + } + return strings.Split(trimmedString, " ") case "RootDistanceMaxSec", "PollIntervalMinSec", "PollIntervalMaxSec", "ConnectionRetrySec", "SaveIntervalSec": - return option.Value + return strings.TrimSpace(option.Value) default: return "" @@ -412,22 +413,23 @@ func getNTPFromSystem() (result map[string]any, err error) { return nil, fmt.Errorf("cannot parse systemd unit in configuration file /etc/systemd/timesyncd.conf: %v", err) } - // Do not return an empty config "system.ntp {}" if there is no custom configuration in the file - if len(unitOptions) == 0 { - return nil, nil - } - val := map[string]any{} for _, option := range unitOptions { snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) snapOptionValue := mapOptionValueTimesyncdToSnap(option) if snapOptionName == "" { - // Do not error out on unsupported options inserted in the config file manually, just ignore them + // If the option name is empty, it means the option is not supported, so we skip it continue } val[snapOptionName] = snapOptionValue } + // Do not return an empty config "system.ntp {}" if there is no custom configuration in the file + // We check here instead of checking the length of unitOptions, since we skip unsupported options + if len(val) == 0 { + return nil, nil + } + return val, nil } diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 73905392420..81cf5aa2fed 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -171,7 +171,11 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "servers": []any{}, }, }, - expectedError: "invalid NTP configuration: servers is an empty list", + expectServiceRestart: true, + expectedFileContent: []string{ + "[Time]", + "NTP=", + }, }, // 5: Wrong value type in server list { @@ -469,6 +473,24 @@ func (s *ntpSuite) TestNTPGetUnsupportedOption(c *C) { }) } +func (s *ntpSuite) TestNTPGetEmptyServerList(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // The empty server list is read as an empty list and not an as an error + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nNTP= \nSaveIntervalSec=10s"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, IsNil) + c.Assert(ntpConfig, DeepEquals, map[string]any{ + "save-interval-sec": "10s", + "servers": []any{}, + }) +} + func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { // Test identical configurations are equal config1 := map[string]any{ From f6314877f1cbf032ff488a856c90ca5046dfa2bd Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Thu, 14 May 2026 14:36:48 +0300 Subject: [PATCH 09/27] o/c/c/ntp_test: improve unit test coverage Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp_test.go | 37 +++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 81cf5aa2fed..43513ac4922 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -372,7 +372,36 @@ func (s *ntpSuite) TestNTPSetValidConfigurationMissingFolderPermissions(c *C) { s.verifyConfigfileContent(c, startingFileContent, "") } +func (s *ntpSuite) TestNTPSetErrorRemoveFileEmptyConfiguration(c *C) { + // Change /etc/systemd permissions to inhibit file deletion when the configuration is empty + systemdConfigFolder := filepath.Join(dirs.GlobalRootDir, "etc/systemd") + c.Assert(os.Chmod(systemdConfigFolder, 0111), IsNil) + defer os.Chmod(systemdConfigFolder, 0755) + + conf := configcore.PlainCoreConfig(map[string]any{ + "system.ntp": map[string]any{}, + }) + + // The config file not being readable triggers an error and the configuration + // is not updated + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Assert(err, ErrorMatches, "cannot reset NTP configuration to defaults: remove .*/etc/systemd/timesyncd.conf: permission denied") +} + func (s *ntpSuite) TestNTPSetErrorReadingDiskConfiguration(c *C) { + // Change file permissions to inhibit reading it + os.Chmod(s.timesyncdConfigFile, 0000) + defer os.Chmod(s.timesyncdConfigFile, 0644) + + conf := configcore.PlainCoreConfig(validConfigurationExample) + + // The config file not being readable triggers an error and the configuration + // is not updated + err := configcore.FilesystemOnlyRun(core24Dev, conf) + c.Assert(err, ErrorMatches, "cannot read NTP configuration file /etc/systemd/timesyncd.conf: open .*/etc/systemd/timesyncd.conf: permission denied") +} + +func (s *ntpSuite) TestNTPSetMissingConfigFile(c *C) { // Remove config file os.Remove(s.timesyncdConfigFile) @@ -567,9 +596,13 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { c.Check(configcore.NTPConfigurationDeepEqual(config8, config1), Equals, false) c.Check(configcore.NTPConfigurationDeepEqual(config5, config1), Equals, false) - // Test configuration with removed fields + // Test configuration with differently named field (max to min) config9 := map[string]any{ - "poll-interval-min-sec": "16s", + "servers": []any{ + "192.168.1.1", + "ntp.ubuntu.com", + }, + "root-distance-min-sec": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config9), Equals, false) From c48427fb99405c34a0a70aafff540c58e2818494 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:31:52 +0300 Subject: [PATCH 10/27] o/c/c/ntp: do not restart systemd-timesyncd during image build Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 30 ++++++++++++++------------ 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 87400888c30..ade459dddcc 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -260,19 +260,19 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte return fmt.Errorf("cannot get NTP config: %v", err) } - oldConfig, err := getNTPFromSystem() - if err != nil { - return err - } - if NTPConfigurationDeepEqual(oldConfig, cfg) { - // If the configuration has not changed, do nothing. - return nil - } - rootDir := dirs.GlobalRootDir if opts != nil { - // runtime system + // filesystem-only context (e.g. image build) rootDir = opts.RootDir + } else { + oldConfig, err := getNTPFromSystem() + if err != nil { + return err + } + if NTPConfigurationDeepEqual(oldConfig, cfg) { + // If the configuration has not changed, do nothing. + return nil + } } // Create systemd configuration folder, if not present @@ -296,10 +296,12 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte } } - // Restart systemd-timesyncd.service to pick up the updated configuration - sysd := systemd.New(systemd.SystemMode, &sysdLogger{}) - if err := sysd.ReloadOrRestart([]string{"systemd-timesyncd.service"}); err != nil { - return fmt.Errorf("cannot restart timesyncd daemon after configuration change: %v", err) + // Restart systemd-timesyncd.service to pick up the updated configuration (runtime only). + if opts == nil { + sysd := systemd.New(systemd.SystemMode, &sysdLogger{}) + if err := sysd.ReloadOrRestart([]string{"systemd-timesyncd.service"}); err != nil { + return fmt.Errorf("cannot restart timesyncd daemon after configuration change: %v", err) + } } return nil From e602fbb89822fc9b42e6b7449729accef25e3d2a Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:37:09 +0300 Subject: [PATCH 11/27] o/c/c/ntp: make NTPConfigurationDeepEqual unexported Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/export_test.go | 15 ++++++++------- overlord/configstate/configcore/ntp.go | 6 +++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/overlord/configstate/configcore/export_test.go b/overlord/configstate/configcore/export_test.go index bd112770045..9e3a9e4b350 100644 --- a/overlord/configstate/configcore/export_test.go +++ b/overlord/configstate/configcore/export_test.go @@ -37,13 +37,14 @@ import ( ) var ( - UpdatePiConfig = updatePiConfig - SwitchHandlePowerKey = switchHandlePowerKey - SwitchDisableService = switchDisableService - UpdateKeyValueStream = updateKeyValueStream - AddFSOnlyHandler = addFSOnlyHandler - FilesystemOnlyApply = filesystemOnlyApply - UpdateHomedirsConfig = updateHomedirsConfig + UpdatePiConfig = updatePiConfig + SwitchHandlePowerKey = switchHandlePowerKey + SwitchDisableService = switchDisableService + UpdateKeyValueStream = updateKeyValueStream + AddFSOnlyHandler = addFSOnlyHandler + FilesystemOnlyApply = filesystemOnlyApply + UpdateHomedirsConfig = updateHomedirsConfig + NTPConfigurationDeepEqual = ntpConfigurationDeepEqual DoExperimentalApparmorPromptingDaemonRestart = doExperimentalApparmorPromptingDaemonRestart ) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index ade459dddcc..39d29457385 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -210,14 +210,14 @@ func validateSystemdTimeSpanFormat(span string) (timeSpanUs int64, err error) { return convertSystemdTimespanToUs(span) } -// NTPConfigurationDeepEqual compares two NTP configurations and returns true if they are equal, +// ntpConfigurationDeepEqual compares two NTP configurations and returns true if they are equal, // false otherwise. // The standard reflect.DeepEqual cannot be used to compare the configurations as the values // of fields "servers" and "fallback-servers" parsed by unit.Deserialize (i.e. the oldConfig) // are of type []string, while the ones coming from snapd are of type []any. // reflect.DeepEqual considers them as different, but we want to consider them as equal as // long as their content is the same. -func NTPConfigurationDeepEqual(oldConfig, newConfig map[string]any) bool { +func ntpConfigurationDeepEqual(oldConfig, newConfig map[string]any) bool { // Check if both maps have the same number of keys // Automatically handles empty and nil maps if len(oldConfig) != len(newConfig) { @@ -269,7 +269,7 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte if err != nil { return err } - if NTPConfigurationDeepEqual(oldConfig, cfg) { + if ntpConfigurationDeepEqual(oldConfig, cfg) { // If the configuration has not changed, do nothing. return nil } From 86adba65b33faf1991c14f406a74ab3b55f3dc98 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:43:34 +0300 Subject: [PATCH 12/27] o/c/c/ntp: replace strings.Split with strings.Fields to improve server list parsing Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 39d29457385..7ade0cb7ca6 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -336,7 +336,7 @@ func mapOptionValueTimesyncdToSnap(option *unit.UnitOption) (result any) { if len(trimmedString) == 0 { return []string{} } - return strings.Split(trimmedString, " ") + return strings.Fields(trimmedString) case "RootDistanceMaxSec", "PollIntervalMinSec", "PollIntervalMaxSec", "ConnectionRetrySec", "SaveIntervalSec": return strings.TrimSpace(option.Value) From 542da8df5fe6af5fd6267314269a6357fdbb3603 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:48:50 +0300 Subject: [PATCH 13/27] o/c/c/ntp_test: make tests more robust by checking more errors on file operations Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 43513ac4922..39a74ac4e33 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -73,7 +73,8 @@ func (s *ntpSuite) writeExampleConfigFile(c *C) { func (s *ntpSuite) verifyConfigfileContent(c *C, expectedContent []string, comment string) { // The serialization order of the individual options is not predictable. Read the file, sort // the lines and compare them against the (already sorted) expected content. - content, _ := os.ReadFile(s.timesyncdConfigFile) + content, err := os.ReadFile(s.timesyncdConfigFile) + c.Assert(err, IsNil) lines := strings.Split(strings.TrimSpace(string(content)), "\n") // Sort in reverse order so the output looks "normal" with the "[Time]" line at the top sort.Sort(sort.Reverse(sort.StringSlice(lines))) @@ -346,11 +347,11 @@ func (s *ntpSuite) TestNTPSetCannotCreateSystemdFolder(c *C) { systemdConfigFolder := filepath.Join(etcFolder, "systemd") systemdConfigFolderAlternateName := filepath.Join(etcFolder, "systemd.bak") // Instead of removing it, we rename it to avoid issues for other files - os.Rename(systemdConfigFolder, systemdConfigFolderAlternateName) + c.Assert(os.Rename(systemdConfigFolder, systemdConfigFolderAlternateName), IsNil) // Remove write permission for /etc so that /etc/systemd cannot be recreated c.Assert(os.Chmod(etcFolder, 0111), IsNil) + defer func() { c.Assert(os.Rename(systemdConfigFolderAlternateName, systemdConfigFolder), IsNil) }() defer os.Chmod(etcFolder, 0755) - defer os.Rename(systemdConfigFolderAlternateName, systemdConfigFolder) conf := configcore.PlainCoreConfig(validConfigurationExample) From b3d660e62f2e161d905fb2bc6e2076909c0d8dbf Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:50:54 +0300 Subject: [PATCH 14/27] o/c/c/ntp: improve string formatting Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 2 +- overlord/configstate/configcore/ntp_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 7ade0cb7ca6..7ea42b84377 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -159,7 +159,7 @@ func validateNTPServers(servers []any) error { for _, serverAny := range servers { server, ok := serverAny.(string) if !ok { - return fmt.Errorf("%q is not a valid string", serverAny) + return fmt.Errorf("server %v is not a valid string", serverAny) } if err := validateServerName(server); err != nil { return err diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 39a74ac4e33..dae9e4c2023 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -187,7 +187,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { }, }, }, - expectedError: "invalid NTP configuration: '\\*' is not a valid string", + expectedError: "invalid NTP configuration: server 42 is not a valid string", }, // 6: Malformed IP address in server list { From 49419379be8284c80f31c90b78146f5a17721619 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 1 Jun 2026 16:51:58 +0300 Subject: [PATCH 15/27] o/c/c/handlers: comment for NTP options changed to system.ntp.* to include all of them Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/handlers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/overlord/configstate/configcore/handlers.go b/overlord/configstate/configcore/handlers.go index 37915e8cda1..d7f18eae040 100644 --- a/overlord/configstate/configcore/handlers.go +++ b/overlord/configstate/configcore/handlers.go @@ -124,7 +124,7 @@ func init() { // system.motd addFSOnlyHandler(validateMotdConfiguration, handleMotdConfiguration, coreOnly) - // system.ntp.{servers,fallback-servers} + // system.ntp.* addFSOnlyHandler(validateNTPSettings, handleNTPConfiguration, coreOnly) sysconfig.ApplyFilesystemOnlyDefaultsImpl = filesystemOnlyApply From 6e6010ef97c9dbb439c7b672b9b75d4f55859178 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Fri, 5 Jun 2026 11:49:40 +0300 Subject: [PATCH 16/27] o/c/c/ntp: rename snap config options to improve clarity Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 56 ++++++++-------- overlord/configstate/configcore/ntp_test.go | 72 ++++++++++----------- 2 files changed, 64 insertions(+), 64 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 7ea42b84377..68b7783f1c3 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -45,11 +45,11 @@ func init() { supportedConfigurations["core.system.ntp"] = true supportedConfigurations["core.system.ntp.servers"] = true supportedConfigurations["core.system.ntp.fallback-servers"] = true - supportedConfigurations["core.system.ntp.root-distance-max-sec"] = true - supportedConfigurations["core.system.ntp.poll-interval-min-sec"] = true - supportedConfigurations["core.system.ntp.poll-interval-max-sec"] = true - supportedConfigurations["core.system.ntp.connection-retry-sec"] = true - supportedConfigurations["core.system.ntp.save-interval-sec"] = true + supportedConfigurations["core.system.ntp.max-root-time-distance"] = true + supportedConfigurations["core.system.ntp.min-poll-interval"] = true + supportedConfigurations["core.system.ntp.max-poll-interval"] = true + supportedConfigurations["core.system.ntp.connection-retry-interval"] = true + supportedConfigurations["core.system.ntp.save-interval"] = true // and register it as a external config config.RegisterExternalConfig("core", "system.ntp", getNTPFromSystemHelper) } @@ -64,21 +64,21 @@ var timespanUsRegexp = regexp.MustCompile(`(?:μs|us):\s*(\d+)`) var timesyncdToSnapKeyMapping = map[string]string{ "NTP": "servers", "FallbackNTP": "fallback-servers", - "RootDistanceMaxSec": "root-distance-max-sec", - "PollIntervalMinSec": "poll-interval-min-sec", - "PollIntervalMaxSec": "poll-interval-max-sec", - "ConnectionRetrySec": "connection-retry-sec", - "SaveIntervalSec": "save-interval-sec", + "RootDistanceMaxSec": "max-root-time-distance", + "PollIntervalMinSec": "min-poll-interval", + "PollIntervalMaxSec": "max-poll-interval", + "ConnectionRetrySec": "connection-retry-interval", + "SaveIntervalSec": "save-interval", } var snapToTimesyncdKeyMapping = map[string]string{ - "servers": "NTP", - "fallback-servers": "FallbackNTP", - "root-distance-max-sec": "RootDistanceMaxSec", - "poll-interval-min-sec": "PollIntervalMinSec", - "poll-interval-max-sec": "PollIntervalMaxSec", - "connection-retry-sec": "ConnectionRetrySec", - "save-interval-sec": "SaveIntervalSec", + "servers": "NTP", + "fallback-servers": "FallbackNTP", + "max-root-time-distance": "RootDistanceMaxSec", + "min-poll-interval": "PollIntervalMinSec", + "max-poll-interval": "PollIntervalMaxSec", + "connection-retry-interval": "ConnectionRetrySec", + "save-interval": "SaveIntervalSec", } func validateNTPSettings(tr ConfGetter) error { @@ -93,17 +93,17 @@ func validateNTPSettings(tr ConfGetter) error { } } - // Validate that poll-interval-min-sec < poll-interval-max-sec + // Validate that min-poll-interval < max-poll-interval // Use the systemd defaults if they have not been overwritten by the user pollIntervalMinSecString := "32s" pollIntervalMaxSecString := "2048s" customPollInterval := false // Validation for user submitted values has already been done - if minSec, exists := ntpCfg["poll-interval-min-sec"]; exists { + if minSec, exists := ntpCfg["min-poll-interval"]; exists { pollIntervalMinSecString, _ = mapOptionValueSnapToTimesyncd(minSec) customPollInterval = true } - if maxSec, exists := ntpCfg["poll-interval-max-sec"]; exists { + if maxSec, exists := ntpCfg["max-poll-interval"]; exists { pollIntervalMaxSecString, _ = mapOptionValueSnapToTimesyncd(maxSec) customPollInterval = true } @@ -113,7 +113,7 @@ func validateNTPSettings(tr ConfGetter) error { pollIntervalMaxUSec, _ := convertSystemdTimespanToUs(pollIntervalMaxSecString) if pollIntervalMinUSec > pollIntervalMaxUSec { - return fmt.Errorf("invalid NTP configuration: poll-interval-min-sec (%q) cannot be greater than poll-interval-max-sec (%q)", pollIntervalMinSecString, pollIntervalMaxSecString) + return fmt.Errorf("invalid NTP configuration: min-poll-interval (%q) cannot be greater than max-poll-interval (%q)", pollIntervalMinSecString, pollIntervalMaxSecString) } } @@ -130,7 +130,7 @@ func validateSingleNTPSetting(key string, value any) (err error) { return validateNTPServers(servers) - case "root-distance-max-sec", "poll-interval-min-sec", "poll-interval-max-sec", "connection-retry-sec", "save-interval-sec": + case "max-root-time-distance", "min-poll-interval", "max-poll-interval", "connection-retry-interval", "save-interval": span, err := mapOptionValueSnapToTimesyncd(value) if err != nil { return fmt.Errorf("%v: %v", key, err) @@ -141,11 +141,11 @@ func validateSingleNTPSetting(key string, value any) (err error) { return fmt.Errorf("%v: %v", key, err) } - if key == "poll-interval-min-sec" && timespanUs < 16000000 { - return fmt.Errorf("poll-interval-min-sec: cannot be smaller than 16s") + if key == "min-poll-interval" && timespanUs < 16000000 { + return fmt.Errorf("min-poll-interval: cannot be smaller than 16s") } - if key == "connection-retry-sec" && timespanUs < 1000000 { - return fmt.Errorf("connection-retry-sec: cannot be smaller than 1s") + if key == "connection-retry-interval" && timespanUs < 1000000 { + return fmt.Errorf("connection-retry-interval: cannot be smaller than 1s") } return nil @@ -368,11 +368,11 @@ func mapOptionValueSnapToTimesyncd(option any) (string, error) { return strings.Join(builder, " "), nil case string: - // Matches timespan fields that are set as a string (e.g. "poll-interval-min-sec": "32s") + // Matches timespan fields that are set as a string (e.g. "min-poll-interval": "32s") return option, nil case json.Number: - // Matches timespan fields that are set without a unit (e.g. "poll-interval-min-sec": 32) + // Matches timespan fields that are set without a unit (e.g. "min-poll-interval": 32) return option.String() + "s", nil default: diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index dae9e4c2023..2e6e9cc81c6 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -110,11 +110,11 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "192.168.1.100", "pool.ntp.org", }, - "root-distance-max-sec": json.Number(fmt.Sprint(5)), - "poll-interval-min-sec": "30s", - "poll-interval-max-sec": "40m", - "connection-retry-sec": "5s", - "save-interval-sec": "20", + "max-root-time-distance": json.Number(fmt.Sprint(5)), + "min-poll-interval": "30s", + "max-poll-interval": "40m", + "connection-retry-interval": "5s", + "save-interval": "20", }, }, // Keep sorted in reverse order. @@ -150,7 +150,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "servers": []any{ "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", }, }, expectServiceRestart: false, @@ -215,38 +215,38 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { { newConfig: map[string]any{ "system.ntp": map[string]any{ - "root-distance-max-sec": "not-a-timespan", + "max-root-time-distance": "not-a-timespan", }, }, - expectedError: "invalid NTP configuration: root-distance-max-sec: \"not-a-timespan\" is not a valid systemd.time timespan", + expectedError: "invalid NTP configuration: max-root-time-distance: \"not-a-timespan\" is not a valid systemd.time timespan", }, - // 9: poll-interval-min-sec greater than poll-interval-max-sec + // 9: min-poll-interval greater than max-poll-interval { newConfig: map[string]any{ "system.ntp": map[string]any{ - "poll-interval-min-sec": "30m", - "poll-interval-max-sec": "1m", + "min-poll-interval": "30m", + "max-poll-interval": "1m", }, }, - expectedError: "invalid NTP configuration: poll-interval-min-sec \\(\"30m\"\\) cannot be greater than poll-interval-max-sec \\(\"1m\"\\)", + expectedError: "invalid NTP configuration: min-poll-interval \\(\"30m\"\\) cannot be greater than max-poll-interval \\(\"1m\"\\)", }, - // 10: poll-interval-min-sec lower than minimum 16s + // 10: min-poll-interval lower than minimum 16s { newConfig: map[string]any{ "system.ntp": map[string]any{ - "poll-interval-min-sec": "5s", + "min-poll-interval": "5s", }, }, - expectedError: "invalid NTP configuration: poll-interval-min-sec: cannot be smaller than 16s", + expectedError: "invalid NTP configuration: min-poll-interval: cannot be smaller than 16s", }, - // 11: connection-retry-sec lower than minimum 1s + // 11: connection-retry-interval lower than minimum 1s { newConfig: map[string]any{ "system.ntp": map[string]any{ - "connection-retry-sec": "100ms", + "connection-retry-interval": "100ms", }, }, - expectedError: "invalid NTP configuration: connection-retry-sec: cannot be smaller than 1s", + expectedError: "invalid NTP configuration: connection-retry-interval: cannot be smaller than 1s", }, // 12: unsupported configuration option { @@ -261,10 +261,10 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { { newConfig: map[string]any{ "system.ntp": map[string]any{ - "poll-interval-min-sec": 2.0, + "min-poll-interval": 2.0, }, }, - expectedError: "invalid NTP configuration: poll-interval-min-sec: invalid option type: float64", + expectedError: "invalid NTP configuration: min-poll-interval: invalid option type: float64", }, } @@ -314,12 +314,12 @@ func (s *ntpSuite) TestNTPSetSystemdAnalyzeError(c *C) { // Apply the config invalidReponseConfig := configcore.PlainCoreConfig(map[string]any{ "system.ntp": map[string]any{ - "connection-retry-sec": "xxxx", + "connection-retry-interval": "xxxx", }}) err := configcore.FilesystemOnlyRun(core24Dev, invalidReponseConfig) // Verify correct error is returned - c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-sec: \"xxxx\" is not a valid systemd.time timespan") + c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-interval: \"xxxx\" is not a valid systemd.time timespan") s.verifyConfigfileContent(c, startingFileContent, "") c.Check(s.systemctlArgs, IsNil) @@ -330,12 +330,12 @@ func (s *ntpSuite) TestNTPSetSystemdAnalyzeError(c *C) { // Apply the config invalidNumberConfig := configcore.PlainCoreConfig(map[string]any{ "system.ntp": map[string]any{ - "connection-retry-sec": "9223372036854s", + "connection-retry-interval": "9223372036854s", }}) err = configcore.FilesystemOnlyRun(core24Dev, invalidNumberConfig) // Verify correct error is returned - c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-sec: \"9223372036854s\" is not a valid systemd.time timespan") + c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-interval: \"9223372036854s\" is not a valid systemd.time timespan") s.verifyConfigfileContent(c, startingFileContent, "") c.Check(s.systemctlArgs, IsNil) } @@ -499,7 +499,7 @@ func (s *ntpSuite) TestNTPGetUnsupportedOption(c *C) { err := tr.Get("core", "system.ntp", &ntpConfig) c.Assert(err, IsNil) c.Assert(ntpConfig, DeepEquals, map[string]any{ - "save-interval-sec": "10s", + "save-interval": "10s", }) } @@ -516,8 +516,8 @@ func (s *ntpSuite) TestNTPGetEmptyServerList(c *C) { err := tr.Get("core", "system.ntp", &ntpConfig) c.Assert(err, IsNil) c.Assert(ntpConfig, DeepEquals, map[string]any{ - "save-interval-sec": "10s", - "servers": []any{}, + "save-interval": "10s", + "servers": []any{}, }) } @@ -528,14 +528,14 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", } config2 := map[string]any{ "servers": []any{ "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config2), Equals, true) @@ -545,7 +545,7 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.2", "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config3), Equals, false) @@ -555,7 +555,7 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-max-sec": "10s", + "max-root-time-distance": "10s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config4), Equals, false) @@ -569,8 +569,8 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", - "poll-interval-min-sec": "16s", + "max-root-time-distance": "5s", + "min-poll-interval": "16s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config6), Equals, false) @@ -582,7 +582,7 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config7), Equals, true) @@ -603,7 +603,7 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "192.168.1.1", "ntp.ubuntu.com", }, - "root-distance-min-sec": "5s", + "root-distance-min": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config9), Equals, false) @@ -615,7 +615,7 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { "ntp.ubuntu.com", 12.5, }, - "root-distance-max-sec": "5s", + "max-root-time-distance": "5s", } c.Check(configcore.NTPConfigurationDeepEqual(config1, config10), Equals, false) c.Check(configcore.NTPConfigurationDeepEqual(config10, config1), Equals, false) From fdf9fcc8a08c065135ea49b5bb465ba752bd8daa Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 8 Jun 2026 17:52:46 +0300 Subject: [PATCH 17/27] o/c/c/ntp: input format for intervals changed from systemd.time strings to go time.Duration strings Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 111 ++++++++----- overlord/configstate/configcore/ntp_test.go | 169 ++++++++++++++------ 2 files changed, 191 insertions(+), 89 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 68b7783f1c3..460c8a26ecb 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -20,7 +20,6 @@ package configcore import ( - "encoding/json" "fmt" "io" "net" @@ -30,6 +29,7 @@ import ( "regexp" "strconv" "strings" + "time" "github.com/coreos/go-systemd/unit" "github.com/snapcore/snapd/dirs" @@ -93,28 +93,27 @@ func validateNTPSettings(tr ConfGetter) error { } } - // Validate that min-poll-interval < max-poll-interval - // Use the systemd defaults if they have not been overwritten by the user - pollIntervalMinSecString := "32s" - pollIntervalMaxSecString := "2048s" - customPollInterval := false - // Validation for user submitted values has already been done - if minSec, exists := ntpCfg["min-poll-interval"]; exists { - pollIntervalMinSecString, _ = mapOptionValueSnapToTimesyncd(minSec) - customPollInterval = true + // Validate that min-poll-interval < max-poll-interval. + // Use the systemd-timesyncd defaults if they have not been overwritten by the user. + // The "*String" variables are used for error messages to show the user the exact + // input they provided in case of an error, instead of a serialized Duration, which might be different + // (e.g. printing "1m0s" when the user input "1m"). + pollIntervalMin := 32 * time.Second + pollIntervalMax := 2048 * time.Second + pollIntervalMinString := "32s" + pollIntervalMaxString := "2048s" + // Validation for user submitted values has already been done, so ParseDuration cannot fail here. + if minVal, exists := ntpCfg["min-poll-interval"]; exists { + pollIntervalMinString = minVal.(string) + pollIntervalMin, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMinString)) } - if maxSec, exists := ntpCfg["max-poll-interval"]; exists { - pollIntervalMaxSecString, _ = mapOptionValueSnapToTimesyncd(maxSec) - customPollInterval = true + if maxVal, exists := ntpCfg["max-poll-interval"]; exists { + pollIntervalMaxString = maxVal.(string) + pollIntervalMax, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMaxString)) } - if customPollInterval { - pollIntervalMinUSec, _ := convertSystemdTimespanToUs(pollIntervalMinSecString) - pollIntervalMaxUSec, _ := convertSystemdTimespanToUs(pollIntervalMaxSecString) - - if pollIntervalMinUSec > pollIntervalMaxUSec { - return fmt.Errorf("invalid NTP configuration: min-poll-interval (%q) cannot be greater than max-poll-interval (%q)", pollIntervalMinSecString, pollIntervalMaxSecString) - } + if pollIntervalMin > pollIntervalMax { + return fmt.Errorf("invalid NTP configuration: min-poll-interval (%q) cannot be greater than max-poll-interval (%q)", pollIntervalMinString, pollIntervalMaxString) } return nil @@ -131,20 +130,28 @@ func validateSingleNTPSetting(key string, value any) (err error) { return validateNTPServers(servers) case "max-root-time-distance", "min-poll-interval", "max-poll-interval", "connection-retry-interval", "save-interval": - span, err := mapOptionValueSnapToTimesyncd(value) - if err != nil { - return fmt.Errorf("%v: %v", key, err) + valueStr, ok := value.(string) + if !ok { + return fmt.Errorf("%v is not a string", key) } - timespanUs, err := validateSystemdTimeSpanFormat(span) + // The value that the user inputs should be parsed as a Go duration string for consistency + // with other configuration options + duration, err := time.ParseDuration(strings.TrimSpace(valueStr)) if err != nil { return fmt.Errorf("%v: %v", key, err) } - if key == "min-poll-interval" && timespanUs < 16000000 { + // systemd's minimum resolution is 1µs; nanosecond values would be silently + // rounded or rejected by timesyncd. + if duration != 0 && duration < time.Microsecond { + return fmt.Errorf("%v: duration %q is below systemd's minimum resolution of 1µs", key, valueStr) + } + + if key == "min-poll-interval" && duration < 16*time.Second { return fmt.Errorf("min-poll-interval: cannot be smaller than 16s") } - if key == "connection-retry-interval" && timespanUs < 1000000 { + if key == "connection-retry-interval" && duration < 1*time.Second { return fmt.Errorf("connection-retry-interval: cannot be smaller than 1s") } @@ -204,10 +211,18 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { return us, nil } -// validateSystemdTimeSpanFormat validates and converts a systemd timespan string to microseconds. -// It reuses the parsing logic from convertSystemdTimespanToUs for validation. -func validateSystemdTimeSpanFormat(span string) (timeSpanUs int64, err error) { - return convertSystemdTimespanToUs(span) +func convertSystemdTimespanToGoDurationString(span string) (string, error) { + us, err := convertSystemdTimespanToUs(span) + if err != nil { + return "", err + } + + const maxDurationUs = int64(time.Duration(1<<63-1) / time.Microsecond) + if us > maxDurationUs { + return "", fmt.Errorf("%q is too large for a Go duration", span) + } + + return (time.Duration(us) * time.Microsecond).String(), nil } // ntpConfigurationDeepEqual compares two NTP configurations and returns true if they are equal, @@ -242,7 +257,18 @@ func ntpConfigurationDeepEqual(oldConfig, newConfig map[string]any) bool { if err != nil { return false } - if oldValString != newValString { + + // For duration fields, compare as time.Duration values so that equivalent + // representations like "40m" (user input) and "40m0s" (normalised from disk) + // are treated as equal. Server list fields cannot be parsed as durations so + // they fall back to plain string comparison. + oldDur, oldErr := time.ParseDuration(oldValString) + newDur, newErr := time.ParseDuration(newValString) + if oldErr == nil && newErr == nil { + if oldDur != newDur { + return false + } + } else if oldValString != newValString { return false } } @@ -329,20 +355,20 @@ func getNTPFromSystemHelper(key string) (result any, err error) { return getNTPFromSystem() } -func mapOptionValueTimesyncdToSnap(option *unit.UnitOption) (result any) { +func mapOptionValueTimesyncdToSnap(option *unit.UnitOption) (result any, err error) { switch option.Name { case "NTP", "FallbackNTP": trimmedString := strings.TrimSpace(option.Value) if len(trimmedString) == 0 { - return []string{} + return []string{}, nil } - return strings.Fields(trimmedString) + return strings.Fields(trimmedString), nil case "RootDistanceMaxSec", "PollIntervalMinSec", "PollIntervalMaxSec", "ConnectionRetrySec", "SaveIntervalSec": - return strings.TrimSpace(option.Value) + return convertSystemdTimespanToGoDurationString(strings.TrimSpace(option.Value)) default: - return "" + return "", nil } } @@ -368,12 +394,8 @@ func mapOptionValueSnapToTimesyncd(option any) (string, error) { return strings.Join(builder, " "), nil case string: - // Matches timespan fields that are set as a string (e.g. "min-poll-interval": "32s") - return option, nil - - case json.Number: - // Matches timespan fields that are set without a unit (e.g. "min-poll-interval": 32) - return option.String() + "s", nil + // Matches timespan fields that are set as a Go duration string (e.g. "min-poll-interval": "32s"). + return strings.ReplaceAll(option, "µs", "us"), nil default: return "", fmt.Errorf("invalid option type: %T", option) @@ -418,12 +440,15 @@ func getNTPFromSystem() (result map[string]any, err error) { val := map[string]any{} for _, option := range unitOptions { snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) - snapOptionValue := mapOptionValueTimesyncdToSnap(option) if snapOptionName == "" { // If the option name is empty, it means the option is not supported, so we skip it continue } + snapOptionValue, err := mapOptionValueTimesyncdToSnap(option) + if err != nil { + return nil, fmt.Errorf("cannot parse NTP option: %s: %v", snapOptionName, err) + } val[snapOptionName] = snapOptionValue } diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 2e6e9cc81c6..48fb7597af0 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -3,7 +3,6 @@ package configcore_test import ( - "encoding/json" "fmt" "os" "path/filepath" @@ -97,8 +96,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { expectServiceRestart bool expectedError string }{ - // 0: Valid configuration with all keys, including json.Number, different units - // and missing units + // 0: Valid configuration with all keys and different Go duration units. { newConfig: map[string]any{ "system.ntp": map[string]any{ @@ -110,11 +108,11 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "192.168.1.100", "pool.ntp.org", }, - "max-root-time-distance": json.Number(fmt.Sprint(5)), + "max-root-time-distance": "5s", "min-poll-interval": "30s", - "max-poll-interval": "40m", + "max-poll-interval": "40m0s", "connection-retry-interval": "5s", - "save-interval": "20", + "save-interval": "20s", }, }, // Keep sorted in reverse order. @@ -123,10 +121,10 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { // this look like a normal unit file expectedFileContent: []string{ "[Time]", - "SaveIntervalSec=20", + "SaveIntervalSec=20s", "RootDistanceMaxSec=5s", "PollIntervalMinSec=30s", - "PollIntervalMaxSec=40m", + "PollIntervalMaxSec=40m0s", "NTP=192.168.15.1 ntp.ubuntu.com", "FallbackNTP=192.168.1.100 pool.ntp.org", "ConnectionRetrySec=5s", @@ -218,7 +216,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "max-root-time-distance": "not-a-timespan", }, }, - expectedError: "invalid NTP configuration: max-root-time-distance: \"not-a-timespan\" is not a valid systemd.time timespan", + expectedError: "invalid NTP configuration: max-root-time-distance: time: invalid duration \"not-a-timespan\"", }, // 9: min-poll-interval greater than max-poll-interval { @@ -264,7 +262,25 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "min-poll-interval": 2.0, }, }, - expectedError: "invalid NTP configuration: min-poll-interval: invalid option type: float64", + expectedError: "invalid NTP configuration: min-poll-interval is not a string", + }, + // 14: missing time unit for a timespan option + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "min-poll-interval": "2", + }, + }, + expectedError: "invalid NTP configuration: min-poll-interval: time: missing unit in duration \"2\"", + }, + // 15: sub-microsecond duration (below systemd's resolution) + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "max-root-time-distance": "500ns", + }, + }, + expectedError: `invalid NTP configuration: max-root-time-distance: duration "500ns" is below systemd's minimum resolution of 1µs`, }, } @@ -304,42 +320,6 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { } } -func (s *ntpSuite) TestNTPSetSystemdAnalyzeError(c *C) { - // Systemd-analyze returns the wrong format for the timespan value - // It will return non-zero exit code if the analysis fails, so we need to mock it here to check - // that we handle the format well and return the correct error message - sysdAnalyzeCmdInvalidResponse := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: xxxx'") - defer sysdAnalyzeCmdInvalidResponse.Restore() - - // Apply the config - invalidReponseConfig := configcore.PlainCoreConfig(map[string]any{ - "system.ntp": map[string]any{ - "connection-retry-interval": "xxxx", - }}) - err := configcore.FilesystemOnlyRun(core24Dev, invalidReponseConfig) - - // Verify correct error is returned - c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-interval: \"xxxx\" is not a valid systemd.time timespan") - s.verifyConfigfileContent(c, startingFileContent, "") - c.Check(s.systemctlArgs, IsNil) - - // Return value that it too big for Int64 (9223372036854775807 + 1) - sysdAnalyzeCmdBigNumber := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: 9223372036854775808'") - defer sysdAnalyzeCmdBigNumber.Restore() - - // Apply the config - invalidNumberConfig := configcore.PlainCoreConfig(map[string]any{ - "system.ntp": map[string]any{ - "connection-retry-interval": "9223372036854s", - }}) - err = configcore.FilesystemOnlyRun(core24Dev, invalidNumberConfig) - - // Verify correct error is returned - c.Check(err, ErrorMatches, "invalid NTP configuration: connection-retry-interval: \"9223372036854s\" is not a valid systemd.time timespan") - s.verifyConfigfileContent(c, startingFileContent, "") - c.Check(s.systemctlArgs, IsNil) -} - // Test that setting a valid configuration fails when the systemd folder cannot be accessed due to // missing write permissions func (s *ntpSuite) TestNTPSetCannotCreateSystemdFolder(c *C) { @@ -503,6 +483,90 @@ func (s *ntpSuite) TestNTPGetUnsupportedOption(c *C) { }) } +func (s *ntpSuite) TestNTPGetSystemdDurationsAsGoDurations(c *C) { + s.state.Lock() + defer s.state.Unlock() + + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nRootDistanceMaxSec=1.5h\nSaveIntervalSec=40min"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, IsNil) + c.Assert(ntpConfig, DeepEquals, map[string]any{ + "max-root-time-distance": "1h30m0s", + "save-interval": "40m0s", + }) +} + +func (s *ntpSuite) TestNTPGetInvalidSystemdDuration(c *C) { + s.state.Lock() + defer s.state.Unlock() + + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nRootDistanceMaxSec=not-a-timespan"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot parse NTP option: max-root-time-distance: \"not-a-timespan\" is not a valid systemd.time timespan") +} + +func (s *ntpSuite) TestNTPGetSystemdAnalyzeNoTimespanLine(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // systemd-analyze exits successfully but its output does not contain a + // "µs:"/"us:" line, so no timespan can be extracted. + sysdAnalyzeCmd := testutil.MockCommand(c, "systemd-analyze", "echo 'no timespan here'") + defer sysdAnalyzeCmd.Restore() + + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nRootDistanceMaxSec=5s"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot parse NTP option: max-root-time-distance: \"5s\" is not a valid systemd.time timespan") +} + +func (s *ntpSuite) TestNTPGetSystemdAnalyzeTimespanOverflowsInt64(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // The captured µs value does not fit into an int64 (math.MaxInt64 + 1), + // so strconv.ParseInt fails. + sysdAnalyzeCmd := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: 9223372036854775808'") + defer sysdAnalyzeCmd.Restore() + + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nRootDistanceMaxSec=5s"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot parse NTP option: max-root-time-distance: \"5s\" is not a valid systemd.time timespan") +} + +func (s *ntpSuite) TestNTPGetSystemdAnalyzeTimespanTooLargeForGoDuration(c *C) { + s.state.Lock() + defer s.state.Unlock() + + // The captured µs value fits into an int64 but exceeds the maximum number of + // microseconds representable by a time.Duration (math.MaxInt64 microseconds). + sysdAnalyzeCmd := testutil.MockCommand(c, "systemd-analyze", "echo 'μs: 9223372036854775807'") + defer sysdAnalyzeCmd.Restore() + + c.Assert(os.WriteFile(s.timesyncdConfigFile, []byte("[Time]\nRootDistanceMaxSec=5s"), 0644), IsNil) + defer os.WriteFile(s.timesyncdConfigFile, []byte(strings.Join(startingFileContent, "\n")), 0644) + tr := config.NewTransaction(s.state) + + var ntpConfig map[string]any + err := tr.Get("core", "system.ntp", &ntpConfig) + c.Assert(err, ErrorMatches, "cannot parse NTP option: max-root-time-distance: \"5s\" is too large for a Go duration") +} + func (s *ntpSuite) TestNTPGetEmptyServerList(c *C) { s.state.Lock() defer s.state.Unlock() @@ -619,4 +683,17 @@ func (s *ntpSuite) TestNTPConfigurationDeepEqual(c *C) { } c.Check(configcore.NTPConfigurationDeepEqual(config1, config10), Equals, false) c.Check(configcore.NTPConfigurationDeepEqual(config10, config1), Equals, false) + + // Test that equivalent duration representations are considered equal. + // The read path normalises disk values (e.g. "40min" → "40m0s"), while the + // user may provide the same duration as "40m". Both should be equal to avoid + // an unnecessary timesyncd restart. + config11 := map[string]any{ + "max-root-time-distance": "40m0s", // normalised form, as read from disk + } + config12 := map[string]any{ + "max-root-time-distance": "40m", // user-supplied shorthand + } + c.Check(configcore.NTPConfigurationDeepEqual(config11, config12), Equals, true) + c.Check(configcore.NTPConfigurationDeepEqual(config12, config11), Equals, true) } From 8d229f494437a5ceccf770011db06a0f7905445c Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 9 Jun 2026 10:54:19 +0300 Subject: [PATCH 18/27] go.mod: added github.com/coreos/go-systemd module Signed-off-by: Lorenzo Medici --- go.mod | 1 + go.sum | 2 ++ 2 files changed, 3 insertions(+) diff --git a/go.mod b/go.mod index 470a1ac2615..b1b7592ca22 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( require ( github.com/cilium/ebpf v0.9.1 + github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf go.etcd.io/bbolt v1.3.9 ) diff --git a/go.sum b/go.sum index f7bc18c089e..8fd5250905f 100644 --- a/go.sum +++ b/go.sum @@ -18,6 +18,8 @@ github.com/chai2010/gettext-go v1.0.3 h1:9liNh8t+u26xl5ddmWLmsOsdNLwkdRTg5AG+JnT github.com/chai2010/gettext-go v1.0.3/go.mod h1:y+wnP2cHYaVj19NZhYKAwEMH2CI1gNHeQQ+5AjwawxA= github.com/cilium/ebpf v0.9.1 h1:64sn2K3UKw8NbP/blsixRpF3nXuyhz/VjRlRzvlBRu4= github.com/cilium/ebpf v0.9.1/go.mod h1:+OhNOIXx/Fnu1IE8bJz2dzOA+VSfyTfdNUVdlQnxUFY= +github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf h1:iW4rZ826su+pqaw19uhpSCzhj44qo35pNgKFGqzDKkU= +github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/frankban/quicktest v1.2.2/go.mod h1:Qh/WofXFeiAFII1aEBu529AtJo6Zg2VHscnEsbBnJ20= From 175a49a9d5f66694a8cb280d2ecc70ef306c7dff Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Fri, 19 Jun 2026 15:40:58 +0300 Subject: [PATCH 19/27] o/c/c/ntp: improved server name input validation * replaced `\-` with `-` in regex for server address validation * removed `validIPv4` variable and directly used `net.ParseIP` as it supports IPv6 * added test with IPv6 server being validated. Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 5 ++--- overlord/configstate/configcore/ntp_test.go | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 460c8a26ecb..11fed77468a 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -55,8 +55,7 @@ func init() { } // Too simplistic? -var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`).MatchString -var validIPv4 = net.ParseIP +var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]))*$`).MatchString // Match the line containing "μs:" or "us:" and capture the following digits var timespanUsRegexp = regexp.MustCompile(`(?:μs|us):\s*(\d+)`) @@ -177,7 +176,7 @@ func validateNTPServers(servers []any) error { } func validateServerName(serverAddress string) error { - if validIPv4(serverAddress) == nil && !validHostname(serverAddress) { + if net.ParseIP(serverAddress) == nil && !validHostname(serverAddress) { return fmt.Errorf("%q is not a valid server name", serverAddress) } return nil diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 48fb7597af0..700cba1ef9f 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -103,6 +103,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "servers": []any{ "192.168.15.1", "ntp.ubuntu.com", + "2610:20:6f15:15::25", // time-f-g.nist.gov }, "fallback-servers": []any{ "192.168.1.100", @@ -125,7 +126,7 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { "RootDistanceMaxSec=5s", "PollIntervalMinSec=30s", "PollIntervalMaxSec=40m0s", - "NTP=192.168.15.1 ntp.ubuntu.com", + "NTP=192.168.15.1 ntp.ubuntu.com 2610:20:6f15:15::25", "FallbackNTP=192.168.1.100 pool.ntp.org", "ConnectionRetrySec=5s", }, From a79fb612cbc55768d7745db37a81adb2e19d12b1 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Fri, 19 Jun 2026 15:45:03 +0300 Subject: [PATCH 20/27] o/c/c/ntp: user-provided strings are rejected if containing leading or trailing whitespace Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 12 ++++++++---- overlord/configstate/configcore/ntp_test.go | 9 +++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 11fed77468a..256318ebd24 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -104,11 +104,11 @@ func validateNTPSettings(tr ConfGetter) error { // Validation for user submitted values has already been done, so ParseDuration cannot fail here. if minVal, exists := ntpCfg["min-poll-interval"]; exists { pollIntervalMinString = minVal.(string) - pollIntervalMin, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMinString)) + pollIntervalMin, _ = time.ParseDuration(pollIntervalMinString) } if maxVal, exists := ntpCfg["max-poll-interval"]; exists { pollIntervalMaxString = maxVal.(string) - pollIntervalMax, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMaxString)) + pollIntervalMax, _ = time.ParseDuration(pollIntervalMaxString) } if pollIntervalMin > pollIntervalMax { @@ -136,7 +136,12 @@ func validateSingleNTPSetting(key string, value any) (err error) { // The value that the user inputs should be parsed as a Go duration string for consistency // with other configuration options - duration, err := time.ParseDuration(strings.TrimSpace(valueStr)) + + if strings.TrimSpace(valueStr) != valueStr { + return fmt.Errorf("%v: contains leading or trailing whitespace", key) + } + + duration, err := time.ParseDuration(valueStr) if err != nil { return fmt.Errorf("%v: %v", key, err) } @@ -185,7 +190,6 @@ func validateServerName(serverAddress string) error { func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { // The most reliable way to parse the timespans appears to be having // systemd-analyze do it - // We also use this to compare min and max values as input validation cmd := exec.Command("systemd-analyze", "timespan", span) output, err := cmd.CombinedOutput() diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 700cba1ef9f..9ad9815a7ce 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -283,6 +283,15 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { }, expectedError: `invalid NTP configuration: max-root-time-distance: duration "500ns" is below systemd's minimum resolution of 1µs`, }, + // 16: string contains leading or trailing whitespace + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "min-poll-interval": " 30s ", + }, + }, + expectedError: "invalid NTP configuration: min-poll-interval: contains leading or trailing whitespace", + }, } for i, test := range getConfigurationTests { From 16e4ecb5a34aa686d229e2b21a8f6035a7a14160 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Fri, 19 Jun 2026 15:45:21 +0300 Subject: [PATCH 21/27] o/c/c/ntp: test cleanup Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index 9ad9815a7ce..a8fcf47ce72 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -22,7 +22,6 @@ import ( type ntpSuite struct { configcoreSuite timesyncdConfigFile string - sysdLog [][]string } var ( @@ -49,8 +48,6 @@ var ( func (s *ntpSuite) SetUpTest(c *C) { s.configcoreSuite.SetUpTest(c) - c.Assert(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "etc/systemd"), 0755), IsNil) - // Create the config file and write a predictable configuration systemdConfigFolder := filepath.Join(dirs.GlobalRootDir, "etc/systemd") err := os.MkdirAll(systemdConfigFolder, 0755) @@ -407,7 +404,7 @@ func (s *ntpSuite) TestNTPSetMissingConfigFile(c *C) { func (s *ntpSuite) TestNTPSetRestartDaemonError(c *C) { r := systemd.MockSystemctl(func(cmd ...string) ([]byte, error) { - s.sysdLog = append(s.sysdLog, cmd) + s.systemctlArgs = append(s.systemctlArgs, cmd) return nil, fmt.Errorf("boom") }) defer r() @@ -416,6 +413,7 @@ func (s *ntpSuite) TestNTPSetRestartDaemonError(c *C) { err := configcore.FilesystemOnlyRun(core24Dev, conf) c.Check(err, ErrorMatches, "cannot restart timesyncd daemon after configuration change: boom") + c.Check(s.systemctlArgs, DeepEquals, [][]string{{"reload-or-restart", "systemd-timesyncd.service"}}) } func (s *ntpSuite) TestNTPGetMissingConfigFile(c *C) { From 1bc2c4f83edb2e5e1c865939528a19e427b40eb6 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Mon, 29 Jun 2026 17:55:36 +0300 Subject: [PATCH 22/27] o/c/c/ntp: reject negative duration values explicitly with clear message Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 4 ++++ overlord/configstate/configcore/ntp_test.go | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 256318ebd24..5bef54d6578 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -146,6 +146,10 @@ func validateSingleNTPSetting(key string, value any) (err error) { return fmt.Errorf("%v: %v", key, err) } + if duration < 0 { + return fmt.Errorf("%v: duration %q cannot be negative", key, valueStr) + } + // systemd's minimum resolution is 1µs; nanosecond values would be silently // rounded or rejected by timesyncd. if duration != 0 && duration < time.Microsecond { diff --git a/overlord/configstate/configcore/ntp_test.go b/overlord/configstate/configcore/ntp_test.go index a8fcf47ce72..ba14a440edd 100644 --- a/overlord/configstate/configcore/ntp_test.go +++ b/overlord/configstate/configcore/ntp_test.go @@ -289,6 +289,24 @@ func (s *ntpSuite) TestNTPSetValidateValues(c *C) { }, expectedError: "invalid NTP configuration: min-poll-interval: contains leading or trailing whitespace", }, + // 17: negative duration value + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "max-root-time-distance": "-5s", + }, + }, + expectedError: `invalid NTP configuration: max-root-time-distance: duration "-5s" cannot be negative`, + }, + // 18: negative duration takes precedence over the min-poll-interval 16s lower bound + { + newConfig: map[string]any{ + "system.ntp": map[string]any{ + "min-poll-interval": "-1h", + }, + }, + expectedError: `invalid NTP configuration: min-poll-interval: duration "-1h" cannot be negative`, + }, } for i, test := range getConfigurationTests { From 552aed52f2484b5ce9df9836a64686220fdc744a Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 30 Jun 2026 16:29:28 +0300 Subject: [PATCH 23/27] o/c/c/ntp: simplify function comparing {min,max}-poll-interval values Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index 5bef54d6578..faaba9815dc 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -97,20 +97,18 @@ func validateNTPSettings(tr ConfGetter) error { // The "*String" variables are used for error messages to show the user the exact // input they provided in case of an error, instead of a serialized Duration, which might be different // (e.g. printing "1m0s" when the user input "1m"). - pollIntervalMin := 32 * time.Second - pollIntervalMax := 2048 * time.Second - pollIntervalMinString := "32s" - pollIntervalMaxString := "2048s" - // Validation for user submitted values has already been done, so ParseDuration cannot fail here. - if minVal, exists := ntpCfg["min-poll-interval"]; exists { - pollIntervalMinString = minVal.(string) - pollIntervalMin, _ = time.ParseDuration(pollIntervalMinString) - } - if maxVal, exists := ntpCfg["max-poll-interval"]; exists { - pollIntervalMaxString = maxVal.(string) - pollIntervalMax, _ = time.ParseDuration(pollIntervalMaxString) + ntpCfgTimeWithDefault := func(cfgName, defString string) (time.Duration, string) { + valString := defString + if v, exists := ntpCfg[cfgName]; exists { + valString = v.(string) + } + retVal, _ := time.ParseDuration(valString) + return retVal, valString } + pollIntervalMin, pollIntervalMinString := ntpCfgTimeWithDefault("min-poll-interval", "32s") + pollIntervalMax, pollIntervalMaxString := ntpCfgTimeWithDefault("max-poll-interval", "2048s") + if pollIntervalMin > pollIntervalMax { return fmt.Errorf("invalid NTP configuration: min-poll-interval (%q) cannot be greater than max-poll-interval (%q)", pollIntervalMinString, pollIntervalMaxString) } From 30b0226427404572d19dd30fb38346a5fe72116a Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Tue, 30 Jun 2026 16:59:39 +0300 Subject: [PATCH 24/27] o/c/c/ntp: added docstrings for getNTPFromSystem and getNTPFromSystemHelper Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index faaba9815dc..b5162ef9d7b 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -112,7 +112,6 @@ func validateNTPSettings(tr ConfGetter) error { if pollIntervalMin > pollIntervalMax { return fmt.Errorf("invalid NTP configuration: min-poll-interval (%q) cannot be greater than max-poll-interval (%q)", pollIntervalMinString, pollIntervalMaxString) } - return nil } @@ -123,7 +122,6 @@ func validateSingleNTPSetting(key string, value any) (err error) { if !ok { return fmt.Errorf("%v is not a list of server names", key) } - return validateNTPServers(servers) case "max-root-time-distance", "min-poll-interval", "max-poll-interval", "connection-retry-interval", "save-interval": @@ -134,7 +132,6 @@ func validateSingleNTPSetting(key string, value any) (err error) { // The value that the user inputs should be parsed as a Go duration string for consistency // with other configuration options - if strings.TrimSpace(valueStr) != valueStr { return fmt.Errorf("%v: contains leading or trailing whitespace", key) } @@ -160,7 +157,6 @@ func validateSingleNTPSetting(key string, value any) (err error) { if key == "connection-retry-interval" && duration < 1*time.Second { return fmt.Errorf("connection-retry-interval: cannot be smaller than 1s") } - return nil default: @@ -178,7 +174,6 @@ func validateNTPServers(servers []any) error { return err } } - return nil } @@ -356,6 +351,10 @@ func serializeNTPConfiguration(config map[string]any) (result []byte) { return byteStream } +// getNTPFromSystemHelper is the callback registered for the "system.ntp" key. +// The key argument is ignored because getNTPFromSystem returns the full configuration +// map and the relevant subkeys are selected by other components before being shown +// to the user func getNTPFromSystemHelper(key string) (result any, err error) { return getNTPFromSystem() } @@ -395,7 +394,6 @@ func mapOptionValueSnapToTimesyncd(option any) (string, error) { } builder = append(builder, s) } - return strings.Join(builder, " "), nil case string: @@ -421,6 +419,12 @@ func mapOptionNameSnapToTimesyncd(snapOption string) string { return "" } +// getNTPFromSystem reads /etc/systemd/timesyncd.conf and returns its contents as a map. +// Keys are translated from timesyncd names (e.g. "NTP", "RootDistanceMaxSec") to snapd-style +// names (e.g. "servers", "max-root-time-distance"). Space-separated server lists become string +// slices, and systemd.time duration values are converted to Go duration strings. +// Returns nil (no error) on classic systems, when the file is absent (default timesyncd +// settings apply), or when no recognised options are present. func getNTPFromSystem() (result map[string]any, err error) { if release.OnClassic { return nil, nil From 49db17effa738645de116e6cb0e5be6c895fd738 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Wed, 1 Jul 2026 15:16:19 +0300 Subject: [PATCH 25/27] tests/core: added spread test for setting timesyncd options through snap configs Signed-off-by: Lorenzo Medici --- tests/core/ntp-config/task.yaml | 133 ++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 tests/core/ntp-config/task.yaml diff --git a/tests/core/ntp-config/task.yaml b/tests/core/ntp-config/task.yaml new file mode 100644 index 00000000000..65fb6d094d9 --- /dev/null +++ b/tests/core/ntp-config/task.yaml @@ -0,0 +1,133 @@ +summary: Check that core.system.ntp.* config options configure systemd-timesyncd + +details: | + core.system.ntp.* configuration options allow configuring NTP time + synchronization behavior on Ubuntu Core. Setting these options writes + the corresponding settings to /etc/systemd/timesyncd.conf and restarts + systemd-timesyncd.service to apply them. + + This test verifies that: + - with no configuration set, "snap get system system.ntp" reports no + configuration option + - setting a full configuration writes the expected values to the + configuration file and restarts systemd-timesyncd.service + - "snap get" reflects the currently configured (and normalized) values + - updating a single option preserves the other, previously configured + options + - re-applying an identical configuration does not trigger an unnecessary + service restart + - invalid configurations are rejected without modifying the on-disk + configuration file or restarting the service + - resetting the configuration to an empty document removes the + configuration file and restores the default (unset) behavior + +environment: + NTP_CONF: /etc/systemd/timesyncd.conf + NTP_CONF_RESTORE: /etc/systemd/timesyncd.conf.orig + NTP_CONF_VALID: /etc/systemd/timesyncd.conf.valid + +prepare: | + if [ -f "$NTP_CONF" ]; then + mv "$NTP_CONF" "$NTP_CONF_RESTORE" + fi + +restore: | + rm -f "$NTP_CONF" + if [ -f "$NTP_CONF_RESTORE" ]; then + mv "$NTP_CONF_RESTORE" "$NTP_CONF" + fi + systemctl reload-or-restart systemd-timesyncd.service + +execute: | + # Gets restart timestamp. This will be compared before and after settings some snap options + # to check if the daemon is restarted or not. If a restart is expected, the values will + # be different. + get_restart_ts() { + systemctl show systemd-timesyncd.service --property=ExecMainStartTimestampMonotonic | cut -d= -f2 + } + + # Verifies that "snap set $@" fails with error message matching the pattern in the + # first argument, that the on-disk configuration file was not modified, and that + # the timesyncd service was not restarted. + # Expects a pattern to match the error against as first argument, then the + # list of arguments for `snap set` + check_setting_invalid_config() { + pattern="$1" + shift + ts_before="$(get_restart_ts)" + not snap set "$@" 2>&1 | MATCH "$pattern" + # Exits with 0 if the files are identical + diff -u "$NTP_CONF_VALID" "$NTP_CONF" + [ "$ts_before" = "$(get_restart_ts)" ] + } + + echo "With no configuration set, snap get reports no configuration option" + not snap get system system.ntp + + echo "Setting a full, valid NTP configuration" + ts_before="$(get_restart_ts)" + snap set -t system system.ntp='{ + "servers": ["192.168.1.1", "ntp.ubuntu.com"], + "fallback-servers": ["pool.ntp.org"], + "max-root-time-distance": "5s", + "min-poll-interval": "32s", + "max-poll-interval": "40m", + "connection-retry-interval": "5s", + "save-interval": "20s" + }' + + echo "The configuration file contains the expected values" + MATCH '^\[Time\]$' < "$NTP_CONF" + MATCH '^NTP=192.168.1.1 ntp.ubuntu.com$' < "$NTP_CONF" + MATCH '^FallbackNTP=pool.ntp.org$' < "$NTP_CONF" + MATCH '^RootDistanceMaxSec=5s$' < "$NTP_CONF" + MATCH '^PollIntervalMinSec=32s$' < "$NTP_CONF" + MATCH '^PollIntervalMaxSec=40m$' < "$NTP_CONF" + MATCH '^ConnectionRetrySec=5s$' < "$NTP_CONF" + MATCH '^SaveIntervalSec=20s$' < "$NTP_CONF" + + echo "systemd-timesyncd.service was restarted to apply the new configuration" + [ "$ts_before" != "$(get_restart_ts)" ] + + echo "snap get reflects the normalized configuration" + snap get -d system system.ntp | MATCH '"max-poll-interval": "40m0s"' + snap get system system.ntp.min-poll-interval | MATCH '^32s$' + snap get system system.ntp.servers | MATCH '192.168.1.1' + snap get system system.ntp.servers | MATCH 'ntp.ubuntu.com' + + echo "Updating a single option preserves the other configured options" + ts_before="$(get_restart_ts)" + snap set system system.ntp.min-poll-interval=1m + MATCH '^PollIntervalMinSec=1m$' < "$NTP_CONF" + MATCH '^NTP=192.168.1.1 ntp.ubuntu.com$' < "$NTP_CONF" + MATCH '^SaveIntervalSec=20s$' < "$NTP_CONF" + [ "$ts_before" != "$(get_restart_ts)" ] + + echo "Re-applying the exact same configuration does not restart the service" + ts_before="$(get_restart_ts)" + snap set system system.ntp.min-poll-interval=1m + [ "$ts_before" = "$(get_restart_ts)" ] + + echo "Save a copy of the current, valid configuration for later comparison" + cp "$NTP_CONF" "$NTP_CONF_VALID" + + echo "Invalid configuration: min-poll-interval greater than max-poll-interval is rejected" + check_setting_invalid_config "cannot be greater than max-poll-interval" \ + system system.ntp.min-poll-interval=1h + + echo "Invalid configuration: malformed server name is rejected" + check_setting_invalid_config "is not a valid server name" \ + -t system system.ntp.servers='["not..a..valid..host"]' + + echo "Invalid configuration: connection-retry-interval below the minimum is rejected" + check_setting_invalid_config "cannot be smaller than 1s" \ + system system.ntp.connection-retry-interval=500ms + + echo "Resetting the configuration to an empty document removes the configuration file" + ts_before="$(get_restart_ts)" + snap set -t system system.ntp='{}' + not test -f "$NTP_CONF" + [ "$ts_before" != "$(get_restart_ts)" ] + + echo "snap get reports no configuration option again after the reset" + not snap get system system.ntp \ No newline at end of file From 01ed5e5283d2ac47b427e82a488782629818a3b4 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Thu, 2 Jul 2026 16:35:49 +0300 Subject: [PATCH 26/27] tests/core/ntp-config: check additional unmodified field after successful set operation Signed-off-by: Lorenzo Medici --- tests/core/ntp-config/task.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/core/ntp-config/task.yaml b/tests/core/ntp-config/task.yaml index 65fb6d094d9..1e9e05aec76 100644 --- a/tests/core/ntp-config/task.yaml +++ b/tests/core/ntp-config/task.yaml @@ -100,6 +100,7 @@ execute: | snap set system system.ntp.min-poll-interval=1m MATCH '^PollIntervalMinSec=1m$' < "$NTP_CONF" MATCH '^NTP=192.168.1.1 ntp.ubuntu.com$' < "$NTP_CONF" + MATCH '^FallbackNTP=pool.ntp.org$' < "$NTP_CONF" MATCH '^SaveIntervalSec=20s$' < "$NTP_CONF" [ "$ts_before" != "$(get_restart_ts)" ] @@ -130,4 +131,4 @@ execute: | [ "$ts_before" != "$(get_restart_ts)" ] echo "snap get reports no configuration option again after the reset" - not snap get system system.ntp \ No newline at end of file + not snap get system system.ntp From 9429cf1146aa5214cc9c01978476c263361b5db2 Mon Sep 17 00:00:00 2001 From: Lorenzo Medici Date: Thu, 2 Jul 2026 16:47:39 +0300 Subject: [PATCH 27/27] o/c/c/ntp: added comments about writing to main configuration file and not using drop-ins for timesyncd configuration Signed-off-by: Lorenzo Medici --- overlord/configstate/configcore/ntp.go | 35 +++++++++++++++----------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/overlord/configstate/configcore/ntp.go b/overlord/configstate/configcore/ntp.go index b5162ef9d7b..b4c8a2746f7 100644 --- a/overlord/configstate/configcore/ntp.go +++ b/overlord/configstate/configcore/ntp.go @@ -129,28 +129,24 @@ func validateSingleNTPSetting(key string, value any) (err error) { if !ok { return fmt.Errorf("%v is not a string", key) } - - // The value that the user inputs should be parsed as a Go duration string for consistency - // with other configuration options if strings.TrimSpace(valueStr) != valueStr { return fmt.Errorf("%v: contains leading or trailing whitespace", key) } + // The value that the user inputs should be parsed as a Go duration string for consistency + // with other configuration options duration, err := time.ParseDuration(valueStr) if err != nil { return fmt.Errorf("%v: %v", key, err) } - if duration < 0 { return fmt.Errorf("%v: duration %q cannot be negative", key, valueStr) } - // systemd's minimum resolution is 1µs; nanosecond values would be silently // rounded or rejected by timesyncd. if duration != 0 && duration < time.Microsecond { return fmt.Errorf("%v: duration %q is below systemd's minimum resolution of 1µs", key, valueStr) } - if key == "min-poll-interval" && duration < 16*time.Second { return fmt.Errorf("min-poll-interval: cannot be smaller than 16s") } @@ -188,7 +184,6 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { // The most reliable way to parse the timespans appears to be having // systemd-analyze do it cmd := exec.Command("systemd-analyze", "timespan", span) - output, err := cmd.CombinedOutput() if err != nil { return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) @@ -196,7 +191,6 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { // Look for the line containing "μs:" or "us:" and capture the following digits matches := timespanUsRegexp.FindStringSubmatch(string(output)) - // We did not capture the two parts of the line ("us:" and the digits) if len(matches) < 2 { return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) @@ -207,7 +201,6 @@ func convertSystemdTimespanToUs(span string) (timeSpanUs int64, err error) { if err != nil { return 0, fmt.Errorf("%q is not a valid systemd.time timespan", span) } - return us, nil } @@ -221,7 +214,6 @@ func convertSystemdTimespanToGoDurationString(span string) (string, error) { if us > maxDurationUs { return "", fmt.Errorf("%q is too large for a Go duration", span) } - return (time.Duration(us) * time.Microsecond).String(), nil } @@ -272,7 +264,6 @@ func ntpConfigurationDeepEqual(oldConfig, newConfig map[string]any) bool { return false } } - return true } @@ -308,6 +299,12 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte } // Configuration file path + // We write the main configuration file directly and not use drop-ins + // to make the reading operation simpler, and to avoid discrepancies + // between `snap get` and `snap set` when other drop-in files are installed + // on the system, though the result might be be imprecise compared to which + // configuration values are read by timesyncd. More explanation is found in the + // docstring for getNTPFromSystem. ntpConfigPath := filepath.Join(systemdConfigFolder, "timesyncd.conf") if len(cfg) == 0 { @@ -329,13 +326,11 @@ func handleNTPConfiguration(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyConte return fmt.Errorf("cannot restart timesyncd daemon after configuration change: %v", err) } } - return nil } func serializeNTPConfiguration(config map[string]any) (result []byte) { unitOptions := []*unit.UnitOption{} - for k, v := range config { unitOptionKey := mapOptionNameSnapToTimesyncd(k) unitOptionValue, _ := mapOptionValueSnapToTimesyncd(v) @@ -347,6 +342,7 @@ func serializeNTPConfiguration(config map[string]any) (result []byte) { unitOptions = append(unitOptions, &unitOption) } + // unit.Serialize should not meet I/O errors as it's reading from memory byteStream, _ := io.ReadAll(unit.Serialize(unitOptions)) return byteStream } @@ -425,6 +421,17 @@ func mapOptionNameSnapToTimesyncd(snapOption string) string { // slices, and systemd.time duration values are converted to Go duration strings. // Returns nil (no error) on classic systems, when the file is absent (default timesyncd // settings apply), or when no recognised options are present. +// The configuration is read from and written to the main file ignoring drop-ins that might be +// present on the system. This should be rare on Ubuntu Core. This is a deliberate choice, that +// simplifies the reading process and avoids merging of the drop-ins in the code. +// The limitation is that the output of this function is not correct if drop-ins are installed +// inside /etc/systemd/timesyncd.conf.d/. +// Compared to the return value of getNTPFromSystem, the values set to timesyncd will have +// the server lists in the drop-ins appended to the one from the main file, and the duration values +// overwritten by the ones in the drop-ins. +// This cannot be avoided by writing the snapd configuration to a drop-in file, as it is always +// possible for the users and applications to install a configuration file that comes later in +// alphabetical order. func getNTPFromSystem() (result map[string]any, err error) { if release.OnClassic { return nil, nil @@ -449,7 +456,6 @@ func getNTPFromSystem() (result map[string]any, err error) { val := map[string]any{} for _, option := range unitOptions { snapOptionName := mapOptionNameTimesyncdToSnap(option.Name) - if snapOptionName == "" { // If the option name is empty, it means the option is not supported, so we skip it continue @@ -466,6 +472,5 @@ func getNTPFromSystem() (result map[string]any, err error) { if len(val) == 0 { return nil, nil } - return val, nil }