From 79f3f6c564bd10657bc7a6488b03acdc0578ceb3 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 15:24:42 -0600 Subject: [PATCH 01/10] Extend LimitsMap to support strings --- pkg/util/validation/limits_map.go | 6 +- pkg/util/validation/limits_map_test.go | 454 ++++++++++++++++++------- 2 files changed, 331 insertions(+), 129 deletions(-) diff --git a/pkg/util/validation/limits_map.go b/pkg/util/validation/limits_map.go index 4d991699e0..1639025ba1 100644 --- a/pkg/util/validation/limits_map.go +++ b/pkg/util/validation/limits_map.go @@ -9,13 +9,13 @@ import ( "gopkg.in/yaml.v3" ) -// LimitsMap is a generic map that can hold either float64 or int as values. -type LimitsMap[T float64 | int] struct { +// LimitsMap is a flag.Value implementation that looks like a generic map, holding float64s, ints, or strings as values. +type LimitsMap[T float64 | int | string] struct { data map[string]T validator func(k string, v T) error } -func NewLimitsMap[T float64 | int](validator func(k string, v T) error) LimitsMap[T] { +func NewLimitsMap[T float64 | int | string](validator func(k string, v T) error) LimitsMap[T] { return LimitsMap[T]{ data: make(map[string]T), validator: validator, diff --git a/pkg/util/validation/limits_map_test.go b/pkg/util/validation/limits_map_test.go index b2a4820772..36fa6cc5c5 100644 --- a/pkg/util/validation/limits_map_test.go +++ b/pkg/util/validation/limits_map_test.go @@ -11,17 +11,45 @@ import ( "gopkg.in/yaml.v3" ) -var fakeValidator = func(_ string, v float64) error { +var fakeFloat64Validator = func(_ string, v float64) error { if v < 0 { return errors.New("value cannot be negative") } return nil } +var fakeIntValidator = func(_ string, v int) error { + if v < 0 { + return errors.New("value cannot be negative") + } + return nil +} + +var fakeStringValidator = func(_ string, v string) error { + if len(v) == 0 { + return errors.New("value cannot be empty") + } + return nil +} + func TestNewLimitsMap(t *testing.T) { - lm := NewLimitsMap(fakeValidator) - lm.data["key1"] = 10 - require.Len(t, lm.data, 1) + t.Run("float64", func(t *testing.T) { + lm := NewLimitsMap(fakeFloat64Validator) + lm.data["key1"] = 10.6 + require.Len(t, lm.data, 1) + }) + + t.Run("int", func(t *testing.T) { + lm := NewLimitsMap(fakeIntValidator) + lm.data["key1"] = 10 + require.Len(t, lm.data, 1) + }) + + t.Run("string", func(t *testing.T) { + lm := NewLimitsMap(fakeStringValidator) + lm.data["key1"] = "test" + require.Len(t, lm.data, 1) + }) } func TestLimitsMap_IsNil(t *testing.T) { @@ -48,164 +76,338 @@ func TestLimitsMap_IsNil(t *testing.T) { } func TestLimitsMap_SetAndString(t *testing.T) { - tc := map[string]struct { - input string - expected map[string]float64 - error string - }{ + t.Run("numeric", func(t *testing.T) { + tc := map[string]struct { + input string + expected map[string]float64 + error string + }{ - "set without error": { - input: `{"key1":10,"key2":20}`, - expected: map[string]float64{"key1": 10, "key2": 20}, - }, - "set with parsing error": { - input: `{"key1": 10, "key2": 20`, - error: "unexpected end of JSON input", - }, - "set with validation error": { - input: `{"key1": -10, "key2": 20}`, - error: "value cannot be negative", - }, - } + "set without error": { + input: `{"key1":10,"key2":20}`, + expected: map[string]float64{"key1": 10, "key2": 20}, + }, + "set with parsing error": { + input: `{"key1": 10, "key2": 20`, + error: "unexpected end of JSON input", + }, + "set with validation error": { + input: `{"key1": -10, "key2": 20}`, + error: "value cannot be negative", + }, + "set with incompatible value type": { + input: `{"key1": "abc", "key2": "def"}`, + error: "json: cannot unmarshal string into Go value of type float64", + }, + } - for name, tt := range tc { - t.Run(name, func(t *testing.T) { - lm := NewLimitsMap(fakeValidator) - err := lm.Set(tt.input) - if tt.error != "" { - require.Error(t, err) - require.Equal(t, tt.error, err.Error()) - } else { - require.NoError(t, err) - require.Equal(t, tt.expected, lm.data) - require.Equal(t, tt.input, lm.String()) - } - }) - } + for name, tt := range tc { + t.Run("numeric/"+name, func(t *testing.T) { + lm := NewLimitsMap(fakeFloat64Validator) + err := lm.Set(tt.input) + if tt.error != "" { + require.Error(t, err) + require.Equal(t, tt.error, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.expected, lm.data) + require.Equal(t, tt.input, lm.String()) + } + }) + } + }) + + t.Run("string", func(t *testing.T) { + tc := map[string]struct { + input string + expected map[string]string + error string + }{ + + "set without error": { + input: `{"key1":"abc","key2":"def"}`, + expected: map[string]string{"key1": "abc", "key2": "def"}, + }, + "set with parsing error": { + input: `{"key1": "abc", "key2": "def`, + error: "unexpected end of JSON input", + }, + "set with validation error": { + input: `{"key1": "", "key2": "def"}`, + error: "value cannot be empty", + }, + "set with incompatible value type": { + input: `{"key1": 10, "key2": 20}`, + error: "json: cannot unmarshal number into Go value of type string", + }, + } + + for name, tt := range tc { + t.Run("string/"+name, func(t *testing.T) { + lm := NewLimitsMap(fakeStringValidator) + err := lm.Set(tt.input) + if tt.error != "" { + require.Error(t, err) + require.Equal(t, tt.error, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.expected, lm.data) + require.Equal(t, tt.input, lm.String()) + } + }) + } + }) } func TestLimitsMap_UnmarshalYAML(t *testing.T) { - tc := []struct { - name string - input string - expected map[string]float64 - error string - }{ - { - name: "unmarshal without error", - input: ` + t.Run("numeric", func(t *testing.T) { + tc := []struct { + name string + input string + expected map[string]float64 + error string + }{ + { + name: "unmarshal without error", + input: ` key1: 10 key2: 20 `, - expected: map[string]float64{"key1": 10, "key2": 20}, - }, - { - name: "unmarshal with validation error", - input: ` + expected: map[string]float64{"key1": 10, "key2": 20}, + }, + { + name: "unmarshal with validation error", + input: ` key1: -10 key2: 20 `, - error: "value cannot be negative", - }, - { - name: "unmarshal with parsing error", - input: ` + error: "value cannot be negative", + }, + { + name: "unmarshal with parsing error", + input: ` key1: 10 key2: 20 - key3: 30 + key3: 30 `, - error: "yaml: line 3: found a tab character that violates indentation", - }, - } + error: "yaml: line 3: found a tab character that violates indentation", + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + lm := NewLimitsMap(fakeFloat64Validator) + err := yaml.Unmarshal([]byte(tt.input), &lm) + if tt.error != "" { + require.Error(t, err) + require.Equal(t, tt.error, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.expected, lm.data) + } + }) + } + }) + + t.Run("string", func(t *testing.T) { + tc := []struct { + name string + input string + expected map[string]string + error string + }{ + { + name: "unmarshal without error", + input: ` +key1: abc +key2: def +`, + expected: map[string]string{"key1": "abc", "key2": "def"}, + }, + { + name: "unmarshal with validation error", + input: ` +key1: abc +key2: "" +`, + error: "value cannot be empty", + }, + { + name: "unmarshal with parsing error", + input: ` +key1: abc +key2: def + key3: ghi +`, + error: "yaml: line 3: found a tab character that violates indentation", + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + lm := NewLimitsMap(fakeStringValidator) + err := yaml.Unmarshal([]byte(tt.input), &lm) + if tt.error != "" { + require.Error(t, err) + require.Equal(t, tt.error, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.expected, lm.data) + } + }) + } + }) - for _, tt := range tc { - t.Run(tt.name, func(t *testing.T) { - lm := NewLimitsMap(fakeValidator) - err := yaml.Unmarshal([]byte(tt.input), &lm) - if tt.error != "" { - require.Error(t, err) - require.Equal(t, tt.error, err.Error()) - } else { - require.NoError(t, err) - require.Equal(t, tt.expected, lm.data) - } - }) - } } func TestLimitsMap_MarshalYAML(t *testing.T) { - lm := NewLimitsMap(fakeValidator) - lm.data["key1"] = 10 - lm.data["key2"] = 20 + t.Run("numeric", func(t *testing.T) { + lm := NewLimitsMap(fakeFloat64Validator) + lm.data["key1"] = 10 + lm.data["key2"] = 20 + + out, err := yaml.Marshal(&lm) + require.NoError(t, err) + require.Equal(t, "key1: 10\nkey2: 20\n", string(out)) + }) + + t.Run("string", func(t *testing.T) { + lm := NewLimitsMap(fakeStringValidator) + lm.data["key1"] = "abc" + lm.data["key2"] = "def" - out, err := yaml.Marshal(&lm) - require.NoError(t, err) - require.Equal(t, "key1: 10\nkey2: 20\n", string(out)) + out, err := yaml.Marshal(&lm) + require.NoError(t, err) + require.Equal(t, "key1: abc\nkey2: def\n", string(out)) + }) } func TestLimitsMap_Equal(t *testing.T) { - tc := map[string]struct { - map1 LimitsMap[float64] - map2 LimitsMap[float64] - expected bool - }{ - "Equal maps with same key-value pairs": { - map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, - map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, - expected: true, - }, - "Different maps with different lengths": { - map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1}}, - map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, - expected: false, - }, - "Different maps with same keys but different values": { - map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1}}, - map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.2}}, - expected: false, - }, - "Equal empty maps": { - map1: LimitsMap[float64]{data: map[string]float64{}}, - map2: LimitsMap[float64]{data: map[string]float64{}}, - expected: true, - }, - } + t.Run("numeric", func(t *testing.T) { + tc := map[string]struct { + map1 LimitsMap[float64] + map2 LimitsMap[float64] + expected bool + }{ + "Equal maps with same key-value pairs": { + map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, + map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, + expected: true, + }, + "Different maps with different lengths": { + map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1}}, + map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.1, "key2": 2.2}}, + expected: false, + }, + "Different maps with same keys but different values": { + map1: LimitsMap[float64]{data: map[string]float64{"key1": 1.1}}, + map2: LimitsMap[float64]{data: map[string]float64{"key1": 1.2}}, + expected: false, + }, + "Equal empty maps": { + map1: LimitsMap[float64]{data: map[string]float64{}}, + map2: LimitsMap[float64]{data: map[string]float64{}}, + expected: true, + }, + } + + for name, tt := range tc { + t.Run(name, func(t *testing.T) { + require.Equal(t, tt.expected, tt.map1.Equal(LimitsMap[float64]{data: tt.map2.data})) + require.Equal(t, tt.expected, cmp.Equal(tt.map1, tt.map2)) + }) + } + }) + + t.Run("string", func(t *testing.T) { + tc := map[string]struct { + map1 LimitsMap[string] + map2 LimitsMap[string] + expected bool + }{ + "Equal maps with same key-value pairs": { + map1: LimitsMap[string]{data: map[string]string{"key1": "abc", "key2": "def"}}, + map2: LimitsMap[string]{data: map[string]string{"key1": "abc", "key2": "def"}}, + expected: true, + }, + "Different maps with different lengths": { + map1: LimitsMap[string]{data: map[string]string{"key1": "abc"}}, + map2: LimitsMap[string]{data: map[string]string{"key1": "abc", "key2": "def"}}, + expected: false, + }, + "Different maps with same keys but different values": { + map1: LimitsMap[string]{data: map[string]string{"key1": "abc"}}, + map2: LimitsMap[string]{data: map[string]string{"key1": "def"}}, + expected: false, + }, + "Equal empty maps": { + map1: LimitsMap[string]{data: map[string]string{}}, + map2: LimitsMap[string]{data: map[string]string{}}, + expected: true, + }, + } + + for name, tt := range tc { + t.Run(name, func(t *testing.T) { + require.Equal(t, tt.expected, tt.map1.Equal(LimitsMap[string]{data: tt.map2.data})) + require.Equal(t, tt.expected, cmp.Equal(tt.map1, tt.map2)) + }) + } + }) - for name, tt := range tc { - t.Run(name, func(t *testing.T) { - require.Equal(t, tt.expected, tt.map1.Equal(LimitsMap[float64]{data: tt.map2.data})) - require.Equal(t, tt.expected, cmp.Equal(tt.map1, tt.map2)) - }) - } } func TestLimitsMap_Clone(t *testing.T) { - // Create an initial LimitsMap with some data. - original := NewLimitsMap[float64](fakeValidator) - original.data["limit1"] = 1.0 - original.data["limit2"] = 2.0 + t.Run("numeric", func(t *testing.T) { + // Create an initial LimitsMap with some data. + original := NewLimitsMap[float64](nil) + original.data["limit1"] = 1.0 + original.data["limit2"] = 2.0 + + // Clone the original LimitsMap. + cloned := original.Clone() + + // Check that the cloned LimitsMap is equal to the original. + require.True(t, original.Equal(cloned), "expected cloned LimitsMap to be different from original") + + // Modify the original LimitsMap and ensure the cloned map is not affected. + original.data["limit1"] = 10.0 + require.False(t, cloned.data["limit1"] == 10.0, "expected cloned LimitsMap to be unaffected by changes to original") + + // Modify the cloned LimitsMap and ensure the original map is not affected. + cloned.data["limit3"] = 3.0 + _, exists := original.data["limit3"] + require.False(t, exists, "expected original LimitsMap to be unaffected by changes to cloned") + }) + + t.Run("string", func(t *testing.T) { + // Create an initial LimitsMap with some data. + original := NewLimitsMap[string](nil) + original.data["limit1"] = "abc" + original.data["limit2"] = "def" - // Clone the original LimitsMap. - cloned := original.Clone() + // Clone the original LimitsMap. + cloned := original.Clone() - // Check that the cloned LimitsMap is equal to the original. - require.True(t, original.Equal(cloned), "expected cloned LimitsMap to be different from original") + // Check that the cloned LimitsMap is equal to the original. + require.True(t, original.Equal(cloned), "expected cloned LimitsMap to be different from original") - // Modify the original LimitsMap and ensure the cloned map is not affected. - original.data["limit1"] = 10.0 - require.False(t, cloned.data["limit1"] == 10.0, "expected cloned LimitsMap to be unaffected by changes to original") + // Modify the original LimitsMap and ensure the cloned map is not affected. + original.data["limit1"] = "zxcv" + require.False(t, cloned.data["limit1"] == "zxcv", "expected cloned LimitsMap to be unaffected by changes to original") - // Modify the cloned LimitsMap and ensure the original map is not affected. - cloned.data["limit3"] = 3.0 - _, exists := original.data["limit3"] - require.False(t, exists, "expected original LimitsMap to be unaffected by changes to cloned") + // Modify the cloned LimitsMap and ensure the original map is not affected. + cloned.data["limit3"] = "test" + _, exists := original.data["limit3"] + require.False(t, exists, "expected original LimitsMap to be unaffected by changes to cloned") + }) } func TestLimitsMap_updateMap(t *testing.T) { initialData := map[string]float64{"a": 1.0, "b": 2.0} updateData := map[string]float64{"a": 3.0, "b": -3.0, "c": 5.0} - limitsMap := LimitsMap[float64]{data: initialData, validator: fakeValidator} + limitsMap := LimitsMap[float64]{data: initialData, validator: fakeFloat64Validator} err := limitsMap.updateMap(updateData) require.Error(t, err) From 9d93f4c68d26e2b4b69e04dd961d9da087824b3f Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 15:30:32 -0600 Subject: [PATCH 02/10] Drive-by fix update bug i noticed --- pkg/util/validation/limits_map.go | 1 + pkg/util/validation/limits_map_test.go | 32 ++++++++++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/pkg/util/validation/limits_map.go b/pkg/util/validation/limits_map.go index 1639025ba1..936b18a72c 100644 --- a/pkg/util/validation/limits_map.go +++ b/pkg/util/validation/limits_map.go @@ -73,6 +73,7 @@ func (m LimitsMap[T]) updateMap(newMap map[string]T) error { } } + clear(m.data) for k, v := range newMap { m.data[k] = v } diff --git a/pkg/util/validation/limits_map_test.go b/pkg/util/validation/limits_map_test.go index 36fa6cc5c5..c45d4fd0cc 100644 --- a/pkg/util/validation/limits_map_test.go +++ b/pkg/util/validation/limits_map_test.go @@ -404,16 +404,30 @@ func TestLimitsMap_Clone(t *testing.T) { } func TestLimitsMap_updateMap(t *testing.T) { - initialData := map[string]float64{"a": 1.0, "b": 2.0} - updateData := map[string]float64{"a": 3.0, "b": -3.0, "c": 5.0} + t.Run("does not apply partial updates", func(t *testing.T) { + initialData := map[string]float64{"a": 1.0, "b": 2.0} + updateData := map[string]float64{"a": 3.0, "b": -3.0, "c": 5.0} - limitsMap := LimitsMap[float64]{data: initialData, validator: fakeFloat64Validator} + limitsMap := LimitsMap[float64]{data: initialData, validator: fakeFloat64Validator} - err := limitsMap.updateMap(updateData) - require.Error(t, err) + err := limitsMap.updateMap(updateData) + require.Error(t, err) - // Verify that no partial updates were applied. - // Because maps in Go are accessed in random order, there's a chance that the validation will fail on the first invalid element of the map thus not asserting partial updates. - expectedData := map[string]float64{"a": 1.0, "b": 2.0} - require.Equal(t, expectedData, limitsMap.data) + // Verify that no partial updates were applied. + // Because maps in Go are accessed in random order, there's a chance that the validation will fail on the first invalid element of the map thus not asserting partial updates. + expectedData := map[string]float64{"a": 1.0, "b": 2.0} + require.Equal(t, expectedData, limitsMap.data) + }) + + t.Run("updates totally replace all values", func(t *testing.T) { + initialData := map[string]float64{"a": 1.0, "b": 2.0} + updateData := map[string]float64{"b": 5.0, "c": 6.0} + limitsMap := LimitsMap[float64]{data: initialData, validator: fakeFloat64Validator} + + err := limitsMap.updateMap(updateData) + require.NoError(t, err) + + expectedData := updateData + require.Equal(t, expectedData, limitsMap.data) + }) } From de427c2eebfaea78b580f7a54a46eebde734aa63 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 15:47:43 -0600 Subject: [PATCH 03/10] Add exported way to fetch the map contents --- pkg/util/validation/limits.go | 6 +++--- pkg/util/validation/limits_map.go | 4 ++++ pkg/util/validation/limits_map_test.go | 14 +++++++------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/util/validation/limits.go b/pkg/util/validation/limits.go index 5751e1b355..2603cb5d4f 100644 --- a/pkg/util/validation/limits.go +++ b/pkg/util/validation/limits.go @@ -890,7 +890,7 @@ func (o *Overrides) RulerTenantShardSize(userID string) int { func (o *Overrides) RulerMaxRulesPerRuleGroup(userID, namespace string) int { u := o.getOverridesForUser(userID) - if namespaceLimit, ok := u.RulerMaxRulesPerRuleGroupByNamespace.data[namespace]; ok { + if namespaceLimit, ok := u.RulerMaxRulesPerRuleGroupByNamespace.Read()[namespace]; ok { return namespaceLimit } @@ -906,7 +906,7 @@ func (o *Overrides) RulerMaxRulesPerRuleGroup(userID, namespace string) int { func (o *Overrides) RulerMaxRuleGroupsPerTenant(userID, namespace string) int { u := o.getOverridesForUser(userID) - if namespaceLimit, ok := u.RulerMaxRuleGroupsPerTenantByNamespace.data[namespace]; ok { + if namespaceLimit, ok := u.RulerMaxRuleGroupsPerTenantByNamespace.Read()[namespace]; ok { return namespaceLimit } @@ -982,7 +982,7 @@ func (o *Overrides) AlertmanagerReceiversBlockPrivateAddresses(user string) bool // 4. default limits func (o *Overrides) getNotificationLimitForUser(user, integration string) float64 { u := o.getOverridesForUser(user) - if n, ok := u.NotificationRateLimitPerIntegration.data[integration]; ok { + if n, ok := u.NotificationRateLimitPerIntegration.Read()[integration]; ok { return n } diff --git a/pkg/util/validation/limits_map.go b/pkg/util/validation/limits_map.go index 936b18a72c..3189bb8918 100644 --- a/pkg/util/validation/limits_map.go +++ b/pkg/util/validation/limits_map.go @@ -45,6 +45,10 @@ func (m LimitsMap[T]) Set(s string) error { return m.updateMap(newMap) } +func (m LimitsMap[T]) Read() map[string]T { + return m.data +} + // Clone returns a copy of the LimitsMap. func (m LimitsMap[T]) Clone() LimitsMap[T] { newMap := make(map[string]T, len(m.data)) diff --git a/pkg/util/validation/limits_map_test.go b/pkg/util/validation/limits_map_test.go index c45d4fd0cc..d9bc2944a0 100644 --- a/pkg/util/validation/limits_map_test.go +++ b/pkg/util/validation/limits_map_test.go @@ -36,19 +36,19 @@ func TestNewLimitsMap(t *testing.T) { t.Run("float64", func(t *testing.T) { lm := NewLimitsMap(fakeFloat64Validator) lm.data["key1"] = 10.6 - require.Len(t, lm.data, 1) + require.Len(t, lm.Read(), 1) }) t.Run("int", func(t *testing.T) { lm := NewLimitsMap(fakeIntValidator) lm.data["key1"] = 10 - require.Len(t, lm.data, 1) + require.Len(t, lm.Read(), 1) }) t.Run("string", func(t *testing.T) { lm := NewLimitsMap(fakeStringValidator) lm.data["key1"] = "test" - require.Len(t, lm.data, 1) + require.Len(t, lm.Read(), 1) }) } @@ -110,7 +110,7 @@ func TestLimitsMap_SetAndString(t *testing.T) { require.Equal(t, tt.error, err.Error()) } else { require.NoError(t, err) - require.Equal(t, tt.expected, lm.data) + require.Equal(t, tt.expected, lm.Read()) require.Equal(t, tt.input, lm.String()) } }) @@ -151,7 +151,7 @@ func TestLimitsMap_SetAndString(t *testing.T) { require.Equal(t, tt.error, err.Error()) } else { require.NoError(t, err) - require.Equal(t, tt.expected, lm.data) + require.Equal(t, tt.expected, lm.Read()) require.Equal(t, tt.input, lm.String()) } }) @@ -416,7 +416,7 @@ func TestLimitsMap_updateMap(t *testing.T) { // Verify that no partial updates were applied. // Because maps in Go are accessed in random order, there's a chance that the validation will fail on the first invalid element of the map thus not asserting partial updates. expectedData := map[string]float64{"a": 1.0, "b": 2.0} - require.Equal(t, expectedData, limitsMap.data) + require.Equal(t, expectedData, limitsMap.Read()) }) t.Run("updates totally replace all values", func(t *testing.T) { @@ -428,6 +428,6 @@ func TestLimitsMap_updateMap(t *testing.T) { require.NoError(t, err) expectedData := updateData - require.Equal(t, expectedData, limitsMap.data) + require.Equal(t, expectedData, limitsMap.Read()) }) } From d248790f09f3fd2d9650d1250e489c91c053b48a Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 15:52:19 -0600 Subject: [PATCH 04/10] Add exported way to initialize with data --- pkg/util/validation/limits_map.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/util/validation/limits_map.go b/pkg/util/validation/limits_map.go index 3189bb8918..324d6ab259 100644 --- a/pkg/util/validation/limits_map.go +++ b/pkg/util/validation/limits_map.go @@ -16,8 +16,12 @@ type LimitsMap[T float64 | int | string] struct { } func NewLimitsMap[T float64 | int | string](validator func(k string, v T) error) LimitsMap[T] { + return NewLimitsMapWithData(make(map[string]T), validator) +} + +func NewLimitsMapWithData[T float64 | int | string](data map[string]T, validator func(k string, v T) error) LimitsMap[T] { return LimitsMap[T]{ - data: make(map[string]T), + data: data, validator: validator, } } From 2815a31236f5dca2b521eae89391d78bae4c66e2 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:04:54 -0600 Subject: [PATCH 05/10] Endpoint params support --- pkg/ruler/notifier.go | 26 +++++++++-- pkg/ruler/notifier_test.go | 92 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/pkg/ruler/notifier.go b/pkg/ruler/notifier.go index 0fd0175b1c..20124627c3 100644 --- a/pkg/ruler/notifier.go +++ b/pkg/ruler/notifier.go @@ -26,11 +26,13 @@ import ( "github.com/prometheus/prometheus/notifier" "github.com/grafana/mimir/pkg/util" + "github.com/grafana/mimir/pkg/util/validation" ) var ( errRulerNotifierStopped = cancellation.NewErrorf("rulerNotifier stopped") errRulerSimultaneousBasicAuthAndOAuth = errors.New("cannot use both Basic Auth and OAuth2 simultaneously") + errEmptyEndpointParamValue = errors.New("endpoint params value cannot be empty") ) type NotifierConfig struct { @@ -49,11 +51,19 @@ func (cfg *NotifierConfig) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.ProxyURL, "ruler.alertmanager-client.proxy-url", "", "Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route requests through. Applies to all requests, including auxiliary traffic, such as OAuth token requests.") } +func validateOAuth2EndpointParam(k string, v string) error { + if v == "" { + return errEmptyEndpointParamValue + } + return nil +} + type OAuth2Config struct { - ClientID string `yaml:"client_id"` - ClientSecret flagext.Secret `yaml:"client_secret"` - TokenURL string `yaml:"token_url"` - Scopes flagext.StringSliceCSV `yaml:"scopes,omitempty"` + ClientID string `yaml:"client_id"` + ClientSecret flagext.Secret `yaml:"client_secret"` + TokenURL string `yaml:"token_url"` + Scopes flagext.StringSliceCSV `yaml:"scopes,omitempty"` + EndpointParams validation.LimitsMap[string] `yaml:"endpoint_params" category:"advanced"` } func (cfg *OAuth2Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { @@ -61,6 +71,10 @@ func (cfg *OAuth2Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) f.Var(&cfg.ClientSecret, prefix+"client_secret", "OAuth2 client secret.") f.StringVar(&cfg.TokenURL, prefix+"token_url", "", "Endpoint used to fetch access token.") f.Var(&cfg.Scopes, prefix+"scopes", "Optional scopes to include with the token request.") + if !cfg.EndpointParams.IsInitialized() { + cfg.EndpointParams = validation.NewLimitsMap(validateOAuth2EndpointParam) + } + f.Var(&cfg.EndpointParams, prefix+"endpoint-params", "Optional additional URL parameters to send to the token URL.") } func (cfg *OAuth2Config) IsEnabled() bool { @@ -224,6 +238,10 @@ func amConfigWithSD(rulerConfig *Config, url *url.URL, sdConfig discovery.Config Scopes: rulerConfig.Notifier.OAuth2.Scopes, } + if rulerConfig.Notifier.OAuth2.EndpointParams.IsInitialized() { + amConfig.HTTPClientConfig.OAuth2.EndpointParams = rulerConfig.Notifier.OAuth2.EndpointParams.Read() + } + if rulerConfig.Notifier.ProxyURL != "" { url, err := url.Parse(rulerConfig.Notifier.ProxyURL) if err != nil { diff --git a/pkg/ruler/notifier_test.go b/pkg/ruler/notifier_test.go index 54c56f46fe..46fa8da4e3 100644 --- a/pkg/ruler/notifier_test.go +++ b/pkg/ruler/notifier_test.go @@ -6,7 +6,10 @@ package ruler import ( + "bytes" "errors" + "flag" + "maps" "net/url" "testing" "time" @@ -17,9 +20,11 @@ import ( "github.com/prometheus/common/model" "github.com/prometheus/prometheus/config" "github.com/prometheus/prometheus/discovery" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/mimir/pkg/util" + "github.com/grafana/mimir/pkg/util/validation" ) func TestBuildNotifierConfig(t *testing.T) { @@ -373,6 +378,51 @@ func TestBuildNotifierConfig(t *testing.T) { }, }, }, + { + name: "with OAuth2 and optional endpoint params", + cfg: &Config{ + AlertmanagerURL: "dnssrv+https://_http._tcp.alertmanager-0.default.svc.cluster.local/alertmanager", + Notifier: NotifierConfig{ + OAuth2: OAuth2Config{ + ClientID: "oauth2-client-id", + ClientSecret: flagext.SecretWithValue("test"), + TokenURL: "https://oauth2-token-endpoint.local/token", + EndpointParams: validation.NewLimitsMapWithData[string]( + map[string]string{ + "param1": "value1", + "param2": "value2", + }, + validateOAuth2EndpointParam, + ), + }, + }, + }, + ncfg: &config.Config{ + AlertingConfig: config.AlertingConfig{ + AlertmanagerConfigs: []*config.AlertmanagerConfig{ + { + HTTPClientConfig: config_util.HTTPClientConfig{ + OAuth2: &config_util.OAuth2{ + ClientID: "oauth2-client-id", + ClientSecret: "test", + TokenURL: "https://oauth2-token-endpoint.local/token", + EndpointParams: map[string]string{"param1": "value1", "param2": "value2"}, + }, + }, + APIVersion: "v2", + Scheme: "https", + PathPrefix: "/alertmanager", + ServiceDiscoveryConfigs: discovery.Configs{ + dnsServiceDiscovery{ + Host: "_http._tcp.alertmanager-0.default.svc.cluster.local", + QType: dns.SRV, + }, + }, + }, + }, + }, + }, + }, { name: "with OAuth2 and proxy_url simultaneously, inheriting proxy", cfg: &Config{ @@ -498,6 +548,48 @@ func TestBuildNotifierConfig(t *testing.T) { } } +func TestOAuth2Config_ValidateEndpointParams(t *testing.T) { + for name, tc := range map[string]struct { + args []string + expected validation.LimitsMap[string] + error string + }{ + "basic test": { + args: []string{"-map-flag", "{\"param1\": \"value1\" }"}, + expected: validation.NewLimitsMapWithData(map[string]string{ + "param1": "value1", + }, validateOAuth2EndpointParam), + }, + + "empty value": { + args: []string{"-map-flag", "{\"param1\": \"\" }"}, + error: "invalid value \"{\\\"param1\\\": \\\"\\\" }\" for flag -map-flag: endpoint params value cannot be empty", + }, + + "parsing error": { + args: []string{"-map-flag", "{\"hello\": ..."}, + error: "invalid value \"{\\\"hello\\\": ...\" for flag -map-flag: invalid character '.' looking for beginning of value", + }, + } { + t.Run(name, func(t *testing.T) { + v := validation.NewLimitsMap(validateOAuth2EndpointParam) + + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.SetOutput(&bytes.Buffer{}) // otherwise errors would go to stderr. + fs.Var(v, "map-flag", "Map flag, you can pass JSON into this") + err := fs.Parse(tc.args) + + if tc.error != "" { + require.NotNil(t, err) + assert.Equal(t, tc.error, err.Error()) + } else { + assert.NoError(t, err) + assert.True(t, maps.Equal(tc.expected.Read(), v.Read())) + } + }) + } +} + func urlMustParse(t *testing.T, raw string) *url.URL { t.Helper() u, err := url.Parse(raw) From c7048cd9130154cc76f47b0f76d2c46ae524fba4 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:06:12 -0600 Subject: [PATCH 06/10] make docs, make reference-help --- cmd/mimir/config-descriptor.json | 8 ++++++++ cmd/mimir/help-all.txt.tmpl | 2 ++ cmd/mimir/help.txt.tmpl | 2 ++ .../mimir/configure/configuration-parameters/index.md | 2 ++ 4 files changed, 14 insertions(+) diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json index 798dad46ed..8c6dc63146 100644 --- a/cmd/mimir/config-descriptor.json +++ b/cmd/mimir/config-descriptor.json @@ -12393,6 +12393,14 @@ "fieldDefaultValue": "", "fieldFlag": "ruler.alertmanager-client.oauth.scopes", "fieldType": "string" + }, + { + "kind": "block", + "name": "endpoint_params", + "required": false, + "desc": "", + "fieldValue": null, + "fieldDefaultValue": null } ], "fieldValue": null, diff --git a/cmd/mimir/help-all.txt.tmpl b/cmd/mimir/help-all.txt.tmpl index cb7cc33979..b9191704fc 100644 --- a/cmd/mimir/help-all.txt.tmpl +++ b/cmd/mimir/help-all.txt.tmpl @@ -2825,6 +2825,8 @@ Usage of ./cmd/mimir/mimir: OAuth2 client ID. Enables the use of OAuth2 for authenticating with Alertmanager. -ruler.alertmanager-client.oauth.client_secret string OAuth2 client secret. + -ruler.alertmanager-client.oauth.endpoint-params value + Optional additional URL parameters to send to the token URL. (default {}) -ruler.alertmanager-client.oauth.scopes comma-separated-list-of-strings Optional scopes to include with the token request. -ruler.alertmanager-client.oauth.token_url string diff --git a/cmd/mimir/help.txt.tmpl b/cmd/mimir/help.txt.tmpl index f0f3b3eba4..60c0d4ff05 100644 --- a/cmd/mimir/help.txt.tmpl +++ b/cmd/mimir/help.txt.tmpl @@ -715,6 +715,8 @@ Usage of ./cmd/mimir/mimir: OAuth2 client ID. Enables the use of OAuth2 for authenticating with Alertmanager. -ruler.alertmanager-client.oauth.client_secret string OAuth2 client secret. + -ruler.alertmanager-client.oauth.endpoint-params value + Optional additional URL parameters to send to the token URL. (default {}) -ruler.alertmanager-client.oauth.scopes comma-separated-list-of-strings Optional scopes to include with the token request. -ruler.alertmanager-client.oauth.token_url string diff --git a/docs/sources/mimir/configure/configuration-parameters/index.md b/docs/sources/mimir/configure/configuration-parameters/index.md index 8a06067d19..3fca5a830f 100644 --- a/docs/sources/mimir/configure/configuration-parameters/index.md +++ b/docs/sources/mimir/configure/configuration-parameters/index.md @@ -1969,6 +1969,8 @@ alertmanager_client: # CLI flag: -ruler.alertmanager-client.oauth.scopes [scopes: | default = ""] + endpoint_params: + # (advanced) Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route # requests through. Applies to all requests, including auxiliary traffic, such # as OAuth token requests. From 3e449c9469141cb98ced3913a1ebe0d2a735a268 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:16:04 -0600 Subject: [PATCH 07/10] linter --- pkg/ruler/notifier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ruler/notifier.go b/pkg/ruler/notifier.go index 20124627c3..46fd5e9b7d 100644 --- a/pkg/ruler/notifier.go +++ b/pkg/ruler/notifier.go @@ -51,7 +51,7 @@ func (cfg *NotifierConfig) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.ProxyURL, "ruler.alertmanager-client.proxy-url", "", "Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route requests through. Applies to all requests, including auxiliary traffic, such as OAuth token requests.") } -func validateOAuth2EndpointParam(k string, v string) error { +func validateOAuth2EndpointParam(_ string, v string) error { if v == "" { return errEmptyEndpointParamValue } From e25750c37e0972ee603af4f9f1dae2bc8c6f90d7 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:29:09 -0600 Subject: [PATCH 08/10] fix doc generator --- tools/doc-generator/parse/parser.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/doc-generator/parse/parser.go b/tools/doc-generator/parse/parser.go index 5be316fe8c..8a49efb7fc 100644 --- a/tools/doc-generator/parse/parser.go +++ b/tools/doc-generator/parse/parser.go @@ -353,6 +353,8 @@ func getFieldCustomType(t reflect.Type) (string, bool) { return "map of string to float64", true case reflect.TypeOf(validation.LimitsMap[int]{}).String(): return "map of string to int", true + case reflect.TypeOf(validation.LimitsMap[string]{}).String(): + return "map of string to string", true case reflect.TypeOf(&url.URL{}).String(): return "url", true case reflect.TypeOf(time.Duration(0)).String(): @@ -439,6 +441,8 @@ func getCustomFieldType(t reflect.Type) (string, bool) { return "map of string to float64", true case reflect.TypeOf(validation.LimitsMap[int]{}).String(): return "map of string to int", true + case reflect.TypeOf(validation.LimitsMap[string]{}).String(): + return "map of string to string", true case reflect.TypeOf(&url.URL{}).String(): return "url", true case reflect.TypeOf(time.Duration(0)).String(): From d827a087cdec6feb5e749e147d963dd508266137 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:30:36 -0600 Subject: [PATCH 09/10] regen docs --- cmd/mimir/config-descriptor.json | 9 ++++++--- .../mimir/configure/configuration-parameters/index.md | 4 +++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json index 8c6dc63146..83b704e251 100644 --- a/cmd/mimir/config-descriptor.json +++ b/cmd/mimir/config-descriptor.json @@ -12395,12 +12395,15 @@ "fieldType": "string" }, { - "kind": "block", + "kind": "field", "name": "endpoint_params", "required": false, - "desc": "", + "desc": "Optional additional URL parameters to send to the token URL.", "fieldValue": null, - "fieldDefaultValue": null + "fieldDefaultValue": {}, + "fieldFlag": "ruler.alertmanager-client.oauth.endpoint-params", + "fieldType": "map of string to string", + "fieldCategory": "advanced" } ], "fieldValue": null, diff --git a/docs/sources/mimir/configure/configuration-parameters/index.md b/docs/sources/mimir/configure/configuration-parameters/index.md index 3fca5a830f..12224de8eb 100644 --- a/docs/sources/mimir/configure/configuration-parameters/index.md +++ b/docs/sources/mimir/configure/configuration-parameters/index.md @@ -1969,7 +1969,9 @@ alertmanager_client: # CLI flag: -ruler.alertmanager-client.oauth.scopes [scopes: | default = ""] - endpoint_params: + # (advanced) Optional additional URL parameters to send to the token URL. + # CLI flag: -ruler.alertmanager-client.oauth.endpoint-params + [endpoint_params: | default = {}] # (advanced) Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route # requests through. Applies to all requests, including auxiliary traffic, such From 2729df9b17c329ea71dee42612e8c397f0f31ae3 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Tue, 26 Nov 2024 16:37:21 -0600 Subject: [PATCH 10/10] Extend changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c65b0e6cb5..e7cef4b7a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,7 +69,7 @@ * [ENHANCEMENT] Distributor: Initialize ha_tracker cache before ha_tracker and distributor reach running state and begin serving writes. #9826 #9976 * [ENHANCEMENT] Ingester: `-ingest-storage.kafka.max-buffered-bytes` to limit the memory for buffered records when using concurrent fetching. #9892 * [ENHANCEMENT] Querier: improve performance and memory consumption of queries that select many series. #9914 -* [ENHANCEMENT] Ruler: Support OAuth2 and proxies in Alertmanager client #9945 +* [ENHANCEMENT] Ruler: Support OAuth2 and proxies in Alertmanager client #9945 #10030 * [BUGFIX] Fix issue where functions such as `rate()` over native histograms could return incorrect values if a float stale marker was present in the selected range. #9508 * [BUGFIX] Fix issue where negation of native histograms (eg. `-some_native_histogram_series`) did nothing. #9508 * [BUGFIX] Fix issue where `metric might not be a counter, name does not end in _total/_sum/_count/_bucket` annotation would be emitted even if `rate` or `increase` did not have enough samples to compute a result. #9508