Fix invalid silence causes incomplete updates (#3898)

This commit fixes a bug where an invalid silence causes incomplete
updates of existing silences. This is fixed moving validation
out of the setSilence method and putting it at the start of the
Set method instead.

Signed-off-by: George Robinson <[email protected]>

silence/silence.go

@@ -518,9 +518,6 @@ func matchesEmpty(m *pb.Matcher) bool {
 }
 
 func validateSilence(s *pb.Silence) error {
-	if s.Id == "" {
-		return errors.New("ID missing")
-	}
 	if len(s.Matchers) == 0 {
 		return errors.New("at least one matcher required")
 	}
@@ -544,9 +541,6 @@ func validateSilence(s *pb.Silence) error {
 	if s.EndsAt.Before(s.StartsAt) {
 		return errors.New("end time must not be before start time")
 	}
-	if s.UpdatedAt.IsZero() {
-		return errors.New("invalid zero update timestamp")
-	}
 	return nil
 }
 
@@ -571,15 +565,9 @@ func (s *Silences) toMeshSilence(sil *pb.Silence) *pb.MeshSilence {
 	}
 }
 
-func (s *Silences) setSilence(sil *pb.Silence, now time.Time, skipValidate bool) error {
+func (s *Silences) setSilence(sil *pb.Silence, now time.Time) error {
 	sil.UpdatedAt = now
 
-	if !skipValidate {
-		if err := validateSilence(sil); err != nil {
-			return fmt.Errorf("silence invalid: %w", err)
-		}
-	}
-
 	msil := s.toMeshSilence(sil)
 	b, err := marshalMeshSilence(msil)
 	if err != nil {
@@ -611,13 +599,21 @@ func (s *Silences) Set(sil *pb.Silence) error {
 	defer s.mtx.Unlock()
 
 	now := s.nowUTC()
+	if sil.StartsAt.IsZero() {
+		sil.StartsAt = now
+	}
+
+	if err := validateSilence(sil); err != nil {
+		return fmt.Errorf("invalid silence: %w", err)
+	}
+
 	prev, ok := s.getSilence(sil.Id)
 	if sil.Id != "" && !ok {
 		return ErrNotFound
 	}
 
 	if ok && canUpdate(prev, sil, now) {
-		return s.setSilence(sil, now, false)
+		return s.setSilence(sil, now)
 	}
 
 	// If we got here it's either a new silence or a replacing one (which would
@@ -647,7 +643,7 @@ func (s *Silences) Set(sil *pb.Silence) error {
 		sil.StartsAt = now
 	}
 
-	return s.setSilence(sil, now, false)
+	return s.setSilence(sil, now)
 }
 
 // canUpdate returns true if silence a can be updated to b without
@@ -705,10 +701,7 @@ func (s *Silences) expire(id string) error {
 		sil.StartsAt = now
 		sil.EndsAt = now
 	}
-
-	// Skip validation of the silence when expiring it. Without this, silences created
-	// with valid UTF-8 matchers cannot be expired when Alertmanager is run in classic mode.
-	return s.setSilence(sil, now, true)
+	return s.setSilence(sil, now)
 }
 
 // QueryParam expresses parameters along which silences are queried.

silence/silence_test.go

@@ -295,7 +295,7 @@ func TestSilencesSetSilence(t *testing.T) {
 	func() {
 		s.mtx.Lock()
 		defer s.mtx.Unlock()
-		require.NoError(t, s.setSilence(sil, nowpb, false))
+		require.NoError(t, s.setSilence(sil, nowpb))
 	}()
 
 	// Ensure broadcast was called.
@@ -468,6 +468,19 @@ func TestSilenceSet(t *testing.T) {
 		},
 	}
 	require.Equal(t, want, s.st, "unexpected state after silence creation")
+
+	// Updating an existing silence with an invalid silence should not expire
+	// the original silence.
+	clock.Add(time.Millisecond)
+	sil8 := cloneSilence(sil7)
+	sil8.EndsAt = time.Time{}
+	require.EqualError(t, s.Set(sil8), "invalid silence: invalid zero end timestamp")
+
+	// sil7 should not be expired because the update failed.
+	clock.Add(time.Millisecond)
+	sil7, err = s.QueryOne(QIDs(sil7.Id))
+	require.NoError(t, err)
+	require.Equal(t, types.SilenceStateActive, getState(sil7, s.nowUTC()))
 }
 
 func TestSilenceLimits(t *testing.T) {
@@ -643,11 +656,15 @@ func TestSilencesSetFail(t *testing.T) {
 		err string
 	}{
 		{
-			s:   &pb.Silence{Id: "some_id"},
+			s: &pb.Silence{
+				Id:       "some_id",
+				Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}},
+				EndsAt:   clock.Now().Add(5 * time.Minute),
+			},
 			err: ErrNotFound.Error(),
 		}, {
 			s:   &pb.Silence{}, // Silence without matcher.
-			err: "silence invalid",
+			err: "invalid silence",
 		},
 	}
 	for _, c := range cases {
@@ -1488,18 +1505,6 @@ func TestValidateSilence(t *testing.T) {
 			},
 			err: "",
 		},
-		{
-			s: &pb.Silence{
-				Id: "",
-				Matchers: []*pb.Matcher{
-					{Name: "a", Pattern: "b"},
-				},
-				StartsAt:  validTimestamp,
-				EndsAt:    validTimestamp,
-				UpdatedAt: validTimestamp,
-			},
-			err: "ID missing",
-		},
 		{
 			s: &pb.Silence{
 				Id:        "some_id",
@@ -1572,18 +1577,6 @@ func TestValidateSilence(t *testing.T) {
 			},
 			err: "invalid zero end timestamp",
 		},
-		{
-			s: &pb.Silence{
-				Id: "some_id",
-				Matchers: []*pb.Matcher{
-					{Name: "a", Pattern: "b"},
-				},
-				StartsAt:  validTimestamp,
-				EndsAt:    validTimestamp,
-				UpdatedAt: zeroTimestamp,
-			},
-			err: "invalid zero update timestamp",
-		},
 	}
 	for _, c := range cases {
 		checkErr(t, c.err, validateSilence(c.s))

test/with_api_v2/acceptance/utf8_test.go

@@ -267,7 +267,7 @@ receivers:
 
 	_, err := am.Client().Silence.PostSilences(silenceParams)
 	require.Error(t, err)
-	require.True(t, strings.Contains(err.Error(), "silence invalid: invalid label matcher"))
+	require.True(t, strings.Contains(err.Error(), "invalid silence: invalid label matcher"))
 }
 
 func TestSendAlertsToUTF8Route(t *testing.T) {