From 58dc6f8d3358c12dffb8eb287c800ba15b6cde96 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Tue, 25 Jun 2024 12:38:33 +0100 Subject: [PATCH] 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 --- silence/silence.go | 31 ++++++---------- silence/silence_test.go | 47 ++++++++++-------------- test/with_api_v2/acceptance/utf8_test.go | 2 +- 3 files changed, 33 insertions(+), 47 deletions(-) diff --git a/silence/silence.go b/silence/silence.go index 37eadc60..06a27cf6 100644 --- a/silence/silence.go +++ b/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. diff --git a/silence/silence_test.go b/silence/silence_test.go index b77eca83..c9a635f4 100644 --- a/silence/silence_test.go +++ b/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)) diff --git a/test/with_api_v2/acceptance/utf8_test.go b/test/with_api_v2/acceptance/utf8_test.go index 6c12b2f7..c0c1dce2 100644 --- a/test/with_api_v2/acceptance/utf8_test.go +++ b/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) {