From d4b1de05306b127ffd2e8a0ed69a0fe829aa4cf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 10 Dec 2024 15:48:13 +0100 Subject: [PATCH 1/4] Fix an index out of range when running shorter than multiple measurements duration --- CHANGELOG.md | 8 +++++++- collector.go | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa9a1b18..5340bd3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,17 @@ ## v2.8.0 - TBD +Bug fixes: +- Fixed an index out of range when the clock drift is high when running shorter than the multiple measurement duration + Changes: +- Updated all go dependencies to their latest versions. ## v2.7.1 - 2024-09-27 +Bug fixes: +- Fixed a server error being returned if the optional `high_drift` parameter is not given. + Changes: -- Fix a server error being returned if the optional `high_drift` parameter is not given. - Updated all go dependencies to their latest versions. ## v2.7.0 - 2024-09-10 diff --git a/collector.go b/collector.go index 80e7b25d..f5509ecb 100644 --- a/collector.go +++ b/collector.go @@ -197,7 +197,7 @@ func (c Collector) measure() error { var measurementsLeap []float64 log.Printf("WARN: clock drift is above %.3fs, taking multiple measurements for %.2f seconds", c.NtpHighDrift.Seconds(), c.NtpMeasurementDuration.Seconds()) - for time.Since(begin) < c.NtpMeasurementDuration { + for next := true; next; next = time.Since(begin) < c.NtpMeasurementDuration { nextMeasurement, err := c.getClockOffsetAndStratum() if err != nil { c.serverReachable.WithLabelValues(c.NtpServer).Set(0) From 2492cf480c8a8dadb663f787371b64b872ae68b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 10 Dec 2024 17:13:45 +0100 Subject: [PATCH 2/4] Treat high drift as an absolute value as it also can be negative! --- CHANGELOG.md | 1 + collector.go | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5340bd3c..ab0df9cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ Bug fixes: - Fixed an index out of range when the clock drift is high when running shorter than the multiple measurement duration +- Fixed high drift not considering negative drift Changes: - Updated all go dependencies to their latest versions. diff --git a/collector.go b/collector.go index f5509ecb..7435d691 100644 --- a/collector.go +++ b/collector.go @@ -23,6 +23,7 @@ package main import ( "fmt" "log" + "math" "sort" "time" @@ -183,8 +184,9 @@ func (c Collector) measure() error { return fmt.Errorf("couldn't get NTP measurement: %w", err) } - // if clock drift is unusually high (e.g. >10ms): repeat measurements for 30 seconds and submit median value - if measurement.clockOffset > c.NtpHighDrift.Seconds() { + // if absolute clock drift is unusually high (e.g. >10ms): + // repeat measurements for the configured amount of seconds and submit median value + if math.Abs(measurement.clockOffset) > c.NtpHighDrift.Seconds() { // arrays of measurements used to calculate median var measurementsClockOffset []float64 var measurementsStratum []float64 From bdd4c73d73b1a4fda410869c0122922709c167a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 10 Dec 2024 17:19:13 +0100 Subject: [PATCH 3/4] Treat negative duration/high_drift as an hard error --- main.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 49763ead..2a7d81ef 100644 --- a/main.go +++ b/main.go @@ -67,7 +67,7 @@ func main() { } if ntpProtocolVersion < 2 || ntpProtocolVersion > 4 { - log.Fatalf("invalid NTP protocol version %d; must be 2, 3, or 4", ntpProtocolVersion) + logger.Fatalf("invalid NTP protocol version %d; must be 2, 3, or 4", ntpProtocolVersion) } } @@ -86,8 +86,8 @@ func init() { flag.StringVar(&metricsPath, "web.telemetry-path", "/metrics", "Path under which to expose metrics.") flag.StringVar(&ntpServer, "ntp.server", "", "NTP server to use (required).") flag.IntVar(&ntpProtocolVersion, "ntp.protocol-version", 4, "NTP protocol version to use.") - flag.DurationVar(&ntpMeasurementDuration, "ntp.measurement-duration", 30*time.Second, "Duration of measurements in case of high (>10ms) drift.") - flag.DurationVar(&ntpHighDrift, "ntp.high-drift", 10*time.Millisecond, "High drift threshold.") + flag.DurationVar(&ntpMeasurementDuration, "ntp.measurement-duration", 30*time.Second, "Duration of measurements in case of high drift.") + flag.DurationVar(&ntpHighDrift, "ntp.high-drift", 10*time.Millisecond, "Absolute high drift threshold.") flag.StringVar(&ntpSource, "ntp.source", "cli", "source of information about ntp server (cli / http).") flag.Parse() } @@ -100,6 +100,13 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) { d := ntpMeasurementDuration hd := ntpHighDrift + if d < 0 { + logger.Fatal("ntp.measurement-duration cannot be negative") + } + if hd < 0 { + logger.Fatal("ntp.high-drift cannot be negative") + } + if ntpSource == "http" { query := r.URL.Query() for _, i := range []string{"target", "protocol", "duration"} { @@ -122,6 +129,10 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) { } if t, err := time.ParseDuration(query.Get("duration")); err == nil { + if t < 0 { + http.Error(w, "high_drift cannot be negative", http.StatusBadRequest) + return + } d = t } else { http.Error(w, "while parsing duration: "+err.Error(), http.StatusBadRequest) @@ -130,6 +141,10 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) { if query.Get("high_drift") != "" { if u, err := time.ParseDuration(query.Get("high_drift")); err == nil { + if u < 0 { + http.Error(w, "high_drift cannot be negative", http.StatusBadRequest) + return + } hd = u } else { http.Error(w, "while parsing high_drift: "+err.Error(), http.StatusBadRequest) From e1413822b47f852cea6026a948aefc5e221c946b Mon Sep 17 00:00:00 2001 From: Sandro Date: Wed, 11 Dec 2024 16:05:06 +0100 Subject: [PATCH 4/4] Fix copy paste error Co-authored-by: Stefan Majewsky --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 2a7d81ef..81f0b6bc 100644 --- a/main.go +++ b/main.go @@ -130,7 +130,7 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) { if t, err := time.ParseDuration(query.Get("duration")); err == nil { if t < 0 { - http.Error(w, "high_drift cannot be negative", http.StatusBadRequest) + http.Error(w, "duration cannot be negative", http.StatusBadRequest) return } d = t