feat: validate NTP responses, trace if valid (NR-138875) (#1684)

* feat: validate NTP responses, trace if valid (NR-138875)

* test: build valid NTP responses

* fix: actually report validation error in debug log

* test: add cases with invalid responses

* style: linter

pkg/metrics/ntp.go

@@ -84,12 +84,23 @@ func (p *Ntp) Offset() (time.Duration, error) {
 	var ntpQueryErr error
 	for _, host := range p.pool {
 		response, err := p.ntpQuery(host, ntp.QueryOptions{Timeout: p.timeout})
-		if err == nil {
-			offsets = append(offsets, response.ClockOffset)
-		} else {
+		if err != nil {
 			ntpQueryErr = multierr.Append(ntpQueryErr, err)
 			syslog.WithError(err).WithField("ntp_host", host).WithField("timeout", p.timeout).Debug("error getting ntp offset")
+
+			continue
+		}
+
+		err = response.Validate()
+		if err != nil {
+			ntpQueryErr = multierr.Append(ntpQueryErr, err)
+			syslog.WithError(err).WithField("ntp_host", host).WithField("timeout", p.timeout).Debug("error validating ntp response, skipping")
+
+			continue
 		}
+
+		syslog.WithField("ntp_host", host).WithField("response", response).Trace("valid ntp response retrieved")
+		offsets = append(offsets, response.ClockOffset)
 	}
 
 	if len(offsets) == 0 {

pkg/metrics/ntp_test.go

@@ -108,7 +108,7 @@ func TestOffset_Cache(t *testing.T) {
 			name:              "first request should not use cached value",
 			now:               nowMock("2022-09-28 16:02:45"),
 			pool:              []string{"one", "two"},
-			ntpResponses:      []ntpResp{{&ntp.Response{ClockOffset: 50 * time.Millisecond}, nil}, {&ntp.Response{ClockOffset: 10 * time.Millisecond}, nil}},
+			ntpResponses:      []ntpResp{{buildValidNtpResponse(50 * time.Millisecond), nil}, {buildValidNtpResponse(10 * time.Millisecond), nil}},
 			expectedOffset:    30 * time.Millisecond,
 			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 16:02:45"), // same as now
 		},
@@ -127,7 +127,7 @@ func TestOffset_Cache(t *testing.T) {
 			updatedAt:         mustParse("2006-01-02 15:04:05", "2022-09-28 15:42:45"),
 			pool:              []string{"one", "two"},
 			offset:            20 * time.Millisecond,
-			ntpResponses:      []ntpResp{{&ntp.Response{ClockOffset: 50 * time.Millisecond}, nil}, {&ntp.Response{ClockOffset: 10 * time.Millisecond}, nil}},
+			ntpResponses:      []ntpResp{{buildValidNtpResponse(50 * time.Millisecond), nil}, {buildValidNtpResponse(10 * time.Millisecond), nil}},
 			expectedOffset:    30 * time.Millisecond,
 			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 16:02:45"), // same as now
 		},
@@ -153,15 +153,15 @@ func TestOffset_OffsetAverage(t *testing.T) {
 	ntpMonitor := NewNtp([]string{"one", "two", "three"}, timeout, interval)
 	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{
 		{
-			resp: &ntp.Response{ClockOffset: 110 * time.Millisecond},
+			resp: buildValidNtpResponse(110 * time.Millisecond),
 			err:  nil,
 		},
 		{
-			resp: &ntp.Response{ClockOffset: 20 * time.Millisecond},
+			resp: buildValidNtpResponse(20 * time.Millisecond),
 			err:  nil,
 		},
 		{
-			resp: &ntp.Response{ClockOffset: 23 * time.Millisecond},
+			resp: buildValidNtpResponse(23 * time.Millisecond),
 			err:  nil,
 		},
 	}...)
@@ -177,14 +177,14 @@ func TestOffset_AnyHostErrorShouldNotReturnError(t *testing.T) {
 	ntpMonitor := NewNtp([]string{"one", "two", "three"}, timeout, interval)
 	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{
 		{
-			resp: &ntp.Response{ClockOffset: 50 * time.Millisecond},
+			resp: buildValidNtpResponse(50 * time.Millisecond),
 			err:  nil,
 		},
 		{
 			err: errors.New("this is an error"),
 		},
 		{
-			resp: &ntp.Response{ClockOffset: 40 * time.Millisecond},
+			resp: buildValidNtpResponse(40 * time.Millisecond),
 			err:  nil,
 		},
 	}...)
@@ -215,6 +215,58 @@ func TestOffset_AllHostErrorShouldReturnError(t *testing.T) {
 	assert.ErrorAs(t, err, &ErrGettingNtpOffset)
 }
 
+func TestOffset_InvalidNtpResponse(t *testing.T) {
+	t.Parallel()
+
+	timeout := uint(5000)
+	interval := uint(15)
+	ntpMonitor := NewNtp([]string{"one", "two", "three"}, timeout, interval)
+	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{
+		{
+			resp: buildInvalidNtpResponse(),
+			err:  nil,
+		},
+		{
+			resp: buildInvalidNtpResponse(),
+			err:  nil,
+		},
+		{
+			resp: buildInvalidNtpResponse(),
+			err:  nil,
+		},
+	}...)
+	ntpMonitor.now = nowMock("2022-09-28 16:02:45")
+	offset, err := ntpMonitor.Offset()
+	assert.Equal(t, time.Duration(0), offset)
+	assert.ErrorAs(t, err, &ErrGettingNtpOffset)
+}
+
+func TestOffset_AnyHostInvalidShouldNotReturnError(t *testing.T) {
+	t.Parallel()
+
+	timeout := uint(5000)
+	interval := uint(15)
+	ntpMonitor := NewNtp([]string{"one", "two", "three"}, timeout, interval)
+	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{
+		{
+			resp: buildValidNtpResponse(50 * time.Millisecond),
+			err:  nil,
+		},
+		{
+			resp: buildInvalidNtpResponse(),
+			err:  nil,
+		},
+		{
+			resp: buildValidNtpResponse(40 * time.Millisecond),
+			err:  nil,
+		},
+	}...)
+	ntpMonitor.now = nowMock("2022-09-28 16:02:45")
+	offset, err := ntpMonitor.Offset()
+	assert.Equal(t, time.Millisecond*45, offset)
+	assert.Equal(t, nil, err)
+}
+
 // nolint:unparam
 func nowMock(dateTime string) func() time.Time {
 	return func() time.Time {
@@ -243,3 +295,25 @@ func mustParse(layout string, value string) time.Time {
 
 	return t
 }
+
+func buildValidNtpResponse(offset time.Duration) *ntp.Response {
+	// A response that should pass the ntp.Validate() check without errors
+	return &ntp.Response{ //nolint:exhaustruct
+		// Stratum should be not 0 and lower than 16
+		Stratum: 1,
+		// Leap should not be ntp.LeapNotInSync
+		Leap: ntp.LeapNoWarning,
+
+		ClockOffset: offset,
+	}
+}
+
+func buildInvalidNtpResponse() *ntp.Response {
+	// A response that should fail the ntp.Validate() check
+	return &ntp.Response{ //nolint:exhaustruct
+		// Stratum should be not 0 and lower than 16
+		Stratum: 0,
+		// Leap should not be ntp.LeapNotInSync
+		Leap: ntp.LeapNotInSync,
+	}
+}