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) {