From 94ac36b3e04525431d812319229bcf95758b2c93 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Tue, 25 Jun 2024 13:23:09 +0100 Subject: [PATCH] Fix MaxSilenceSizeBytes limit causes incomplete updates of existing silences (#3897) This commit fixes a bug where the MaxSilenceSizeBytes limit can cause an incomplete update of existing silences, where the old silence can be expired but the new silence cannot be created because it would exceed the maximum size limit. Signed-off-by: George Robinson --- silence/silence.go | 61 +++++++++++++++++++++++------------------ silence/silence_test.go | 29 +++++++++++++++++++- 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/silence/silence.go b/silence/silence.go index 06a27cf6..ee975519 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -550,6 +550,16 @@ func cloneSilence(sil *pb.Silence) *pb.Silence { return &s } +func (s *Silences) checkSizeLimits(msil *pb.MeshSilence) error { + if s.limits.MaxSilenceSizeBytes != nil { + n := msil.Size() + if m := s.limits.MaxSilenceSizeBytes(); m > 0 && n > m { + return fmt.Errorf("silence exceeded maximum size: %d bytes (limit: %d bytes)", n, m) + } + } + return nil +} + func (s *Silences) getSilence(id string) (*pb.Silence, bool) { msil, ok := s.st[id] if !ok { @@ -565,30 +575,15 @@ func (s *Silences) toMeshSilence(sil *pb.Silence) *pb.MeshSilence { } } -func (s *Silences) setSilence(sil *pb.Silence, now time.Time) error { - sil.UpdatedAt = now - - msil := s.toMeshSilence(sil) +func (s *Silences) setSilence(msil *pb.MeshSilence, now time.Time) error { b, err := marshalMeshSilence(msil) if err != nil { return err } - - // Check the limit unless the silence has been expired. This is to avoid - // situations where silences cannot be expired after the limit has been - // reduced. - if s.limits.MaxSilenceSizeBytes != nil { - n := msil.Size() - if m := s.limits.MaxSilenceSizeBytes(); m > 0 && n > m && sil.EndsAt.After(now) { - return fmt.Errorf("silence exceeded maximum size: %d bytes (limit: %d bytes)", n, m) - } - } - if s.st.merge(msil, now) { s.version++ } s.broadcast(b) - return nil } @@ -613,7 +608,12 @@ func (s *Silences) Set(sil *pb.Silence) error { } if ok && canUpdate(prev, sil, now) { - return s.setSilence(sil, now) + sil.UpdatedAt = now + msil := s.toMeshSilence(sil) + if err := s.checkSizeLimits(msil); err != nil { + return err + } + return s.setSilence(msil, now) } // If we got here it's either a new silence or a replacing one (which would @@ -625,14 +625,6 @@ func (s *Silences) Set(sil *pb.Silence) error { } } - if ok && getState(prev, s.nowUTC()) != types.SilenceStateExpired { - // We cannot update the silence, expire the old one to leave a history of - // the silence before modification. - if err := s.expire(prev.Id); err != nil { - return fmt.Errorf("expire previous silence: %w", err) - } - } - uid, err := uuid.NewV4() if err != nil { return fmt.Errorf("generate uuid: %w", err) @@ -642,8 +634,22 @@ func (s *Silences) Set(sil *pb.Silence) error { if sil.StartsAt.Before(now) { sil.StartsAt = now } + sil.UpdatedAt = now - return s.setSilence(sil, now) + msil := s.toMeshSilence(sil) + if err := s.checkSizeLimits(msil); err != nil { + return err + } + + if ok && getState(prev, s.nowUTC()) != types.SilenceStateExpired { + // We cannot update the silence, expire the old one to leave a history of + // the silence before modification. + if err := s.expire(prev.Id); err != nil { + return fmt.Errorf("expire previous silence: %w", err) + } + } + + return s.setSilence(msil, now) } // canUpdate returns true if silence a can be updated to b without @@ -701,7 +707,8 @@ func (s *Silences) expire(id string) error { sil.StartsAt = now sil.EndsAt = now } - return s.setSilence(sil, now) + sil.UpdatedAt = now + return s.setSilence(s.toMeshSilence(sil), now) } // QueryParam expresses parameters along which silences are queried. diff --git a/silence/silence_test.go b/silence/silence_test.go index c9a635f4..6fe536e5 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)) + require.NoError(t, s.setSilence(s.toMeshSilence(sil), nowpb)) }() // Ensure broadcast was called. @@ -575,6 +575,33 @@ func TestSilenceLimits(t *testing.T) { sil6, err = s.QueryOne(QIDs(sil6.Id)) require.NoError(t, err) require.Equal(t, types.SilenceStateActive, getState(sil6, s.nowUTC())) + + // Should not be able to update with a comment that exceeds maximum size. + // Need to increase the maximum number of silences to test this. + s.limits.MaxSilences = func() int { return 2 } + sil8 := cloneSilence(sil6) + sil8.Comment = strings.Repeat("m", 2<<11) + require.EqualError(t, s.Set(sil8), fmt.Sprintf("silence exceeded maximum size: %d bytes (limit: 4096 bytes)", s.toMeshSilence(sil8).Size())) + + // sil6 should not be expired because the update failed. + sil6, err = s.QueryOne(QIDs(sil6.Id)) + require.NoError(t, err) + require.Equal(t, types.SilenceStateActive, getState(sil6, s.nowUTC())) + + // Should not be able to replace with a silence that exceeds maximum size. + // This is different from the previous assertion as unlike when adding or + // updating a comment, changing the matchers for a silence should expire + // the existing silence, unless the silence that is replacing it exceeds + // limits, in which case the operation should fail and the existing silence + // should still be active. + sil9 := cloneSilence(sil8) + sil9.Matchers = []*pb.Matcher{{Name: "n", Pattern: "o"}} + require.EqualError(t, s.Set(sil9), fmt.Sprintf("silence exceeded maximum size: %d bytes (limit: 4096 bytes)", s.toMeshSilence(sil9).Size())) + + // sil6 should not be expired because the update failed. + sil6, err = s.QueryOne(QIDs(sil6.Id)) + require.NoError(t, err) + require.Equal(t, types.SilenceStateActive, getState(sil6, s.nowUTC())) } func TestSilenceNoLimits(t *testing.T) {