From 371f57d1459998b3a9163798ec8292f247d89b73 Mon Sep 17 00:00:00 2001 From: yoyinzyc Date: Wed, 3 Mar 2021 15:06:20 -0800 Subject: [PATCH 1/3] enforce metric cardinality check to gauge metric --- .../component-base/metrics/counter_test.go | 4 +- .../k8s.io/component-base/metrics/gauge.go | 15 +++- .../component-base/metrics/gauge_test.go | 77 +++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter_test.go b/staging/src/k8s.io/component-base/metrics/counter_test.go index 3595a57946d..7b29c294688 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -211,12 +211,12 @@ func TestCounterVec(t *testing.T) { func TestCounterWithLabelValueAllowList(t *testing.T) { labelAllowValues := map[string]string{ - "namespace_subsystem_metric_test_name,label_a": "allowed", + "namespace_subsystem_metric_allowlist_test,label_a": "allowed", } labels := []string{"label_a", "label_b"} opts := &CounterOpts{ Namespace: "namespace", - Name: "metric_test_name", + Name: "metric_allowlist_test", Subsystem: "subsystem", } var tests = []struct { diff --git a/staging/src/k8s.io/component-base/metrics/gauge.go b/staging/src/k8s.io/component-base/metrics/gauge.go index 1ff0dce679a..a8d9c201016 100644 --- a/staging/src/k8s.io/component-base/metrics/gauge.go +++ b/staging/src/k8s.io/component-base/metrics/gauge.go @@ -94,13 +94,20 @@ type GaugeVec struct { func NewGaugeVec(opts *GaugeOpts, labels []string) *GaugeVec { opts.StabilityLevel.setDefaults() + fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[fqName]; ok { + opts.LabelValueAllowLists = allowList + } + allowListLock.RUnlock() + cv := &GaugeVec{ GaugeVec: noopGaugeVec, GaugeOpts: opts, originalLabels: labels, lazyMetric: lazyMetric{}, } - cv.lazyInit(cv, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + cv.lazyInit(cv, fqName) return cv } @@ -139,6 +146,9 @@ func (v *GaugeVec) WithLabelValues(lvs ...string) GaugeMetric { if !v.IsCreated() { return noop // return no-op gauge } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } return v.GaugeVec.WithLabelValues(lvs...) } @@ -150,6 +160,9 @@ func (v *GaugeVec) With(labels map[string]string) GaugeMetric { if !v.IsCreated() { return noop // return no-op gauge } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainLabelMap(labels) + } return v.GaugeVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/gauge_test.go b/staging/src/k8s.io/component-base/metrics/gauge_test.go index 6af9920c65d..ee09d845fab 100644 --- a/staging/src/k8s.io/component-base/metrics/gauge_test.go +++ b/staging/src/k8s.io/component-base/metrics/gauge_test.go @@ -268,3 +268,80 @@ namespace_subsystem_metric_deprecated 1 }) } } + +func TestGaugeWithLabelValueAllowList(t *testing.T) { + labelAllowValues := map[string]string{ + "namespace_subsystem_metric_allowlist_test,label_a": "allowed", + } + labels := []string{"label_a", "label_b"} + opts := &GaugeOpts{ + Namespace: "namespace", + Name: "metric_allowlist_test", + Subsystem: "subsystem", + } + var tests = []struct { + desc string + labelValues [][]string + expectMetricValues map[string]float64 + }{ + { + desc: "Test no unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"allowed", "b2"}}, + expectMetricValues: map[string]float64{ + "allowed b1": 100.0, + "allowed b2": 100.0, + }, + }, + { + desc: "Test unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"not_allowed", "b1"}}, + expectMetricValues: map[string]float64{ + "allowed b1": 100.0, + "unexpected b1": 100.0, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + SetLabelAllowListFromCLI(labelAllowValues) + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + g := NewGaugeVec(opts, labels) + registry.MustRegister(g) + + for _, lv := range test.labelValues { + g.WithLabelValues(lv...).Set(100.0) + } + mfs, err := registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range mfs { + if *mf.Name != BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) { + continue + } + mfMetric := mf.GetMetric() + + for _, m := range mfMetric { + var aValue, bValue string + for _, l := range m.Label { + if *l.Name == "label_a" { + aValue = *l.Value + } + if *l.Name == "label_b" { + bValue = *l.Value + } + } + labelValuePair := aValue + " " + bValue + expectedValue, ok := test.expectMetricValues[labelValuePair] + assert.True(t, ok, "Got unexpected label values, lable_a is %v, label_b is %v", aValue, bValue) + actualValue := m.GetGauge().GetValue() + assert.Equalf(t, expectedValue, actualValue, "Got %v, wanted %v as the gauge while setting label_a to %v and label b to %v", actualValue, expectedValue, aValue, bValue) + } + } + }) + } +} From 2a86d4ca69ffa206bbe211d786117130b5b5c875 Mon Sep 17 00:00:00 2001 From: yoyinzyc Date: Wed, 3 Mar 2021 15:06:54 -0800 Subject: [PATCH 2/3] enforce metric cardinality check to histogram metric --- .../component-base/metrics/histogram.go | 15 +++- .../component-base/metrics/histogram_test.go | 77 +++++++++++++++++++ .../src/k8s.io/component-base/metrics/opts.go | 48 ++++++------ 3 files changed, 116 insertions(+), 24 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index 90f21766e9f..9dd75388b33 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -104,13 +104,20 @@ type HistogramVec struct { func NewHistogramVec(opts *HistogramOpts, labels []string) *HistogramVec { opts.StabilityLevel.setDefaults() + fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[fqName]; ok { + opts.LabelValueAllowLists = allowList + } + allowListLock.RUnlock() + v := &HistogramVec{ HistogramVec: noopHistogramVec, HistogramOpts: opts, originalLabels: labels, lazyMetric: lazyMetric{}, } - v.lazyInit(v, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + v.lazyInit(v, fqName) return v } @@ -145,6 +152,9 @@ func (v *HistogramVec) WithLabelValues(lvs ...string) ObserverMetric { if !v.IsCreated() { return noop } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } return v.HistogramVec.WithLabelValues(lvs...) } @@ -156,6 +166,9 @@ func (v *HistogramVec) With(labels map[string]string) ObserverMetric { if !v.IsCreated() { return noop } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainLabelMap(labels) + } return v.HistogramVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/histogram_test.go b/staging/src/k8s.io/component-base/metrics/histogram_test.go index 762441eebd1..99f5dcf808b 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/histogram_test.go @@ -204,3 +204,80 @@ func TestHistogramVec(t *testing.T) { }) } } + +func TestHistogramWithLabelValueAllowList(t *testing.T) { + labelAllowValues := map[string]string{ + "namespace_subsystem_metric_allowlist_test,label_a": "allowed", + } + labels := []string{"label_a", "label_b"} + opts := &HistogramOpts{ + Namespace: "namespace", + Name: "metric_allowlist_test", + Subsystem: "subsystem", + } + var tests = []struct { + desc string + labelValues [][]string + expectMetricValues map[string]int + }{ + { + desc: "Test no unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"allowed", "b2"}}, + expectMetricValues: map[string]int{ + "allowed b1": 1.0, + "allowed b2": 1.0, + }, + }, + { + desc: "Test unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"not_allowed", "b1"}}, + expectMetricValues: map[string]int{ + "allowed b1": 1.0, + "unexpected b1": 1.0, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + SetLabelAllowListFromCLI(labelAllowValues) + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + c := NewHistogramVec(opts, labels) + registry.MustRegister(c) + + for _, lv := range test.labelValues { + c.WithLabelValues(lv...).Observe(1.0) + } + mfs, err := registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range mfs { + if *mf.Name != BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) { + continue + } + mfMetric := mf.GetMetric() + + for _, m := range mfMetric { + var aValue, bValue string + for _, l := range m.Label { + if *l.Name == "label_a" { + aValue = *l.Value + } + if *l.Name == "label_b" { + bValue = *l.Value + } + } + labelValuePair := aValue + " " + bValue + expectedValue, ok := test.expectMetricValues[labelValuePair] + assert.True(t, ok, "Got unexpected label values, lable_a is %v, label_b is %v", aValue, bValue) + actualValue := int(m.GetHistogram().GetSampleCount()) + assert.Equalf(t, expectedValue, actualValue, "Got %v, wanted %v as the count while setting label_a to %v and label b to %v", actualValue, expectedValue, aValue, bValue) + } + } + }) + } +} diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 395913936c6..04203b74e0a 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -148,16 +148,17 @@ func (o *GaugeOpts) toPromGaugeOpts() prometheus.GaugeOpts { // and can safely be left at their zero value, although it is strongly // encouraged to set a Help string. type HistogramOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - Buckets []float64 - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Buckets []float64 + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + LabelValueAllowLists *MetricLabelAllowList } // Modify help description on the metric description. @@ -194,19 +195,20 @@ func (o *HistogramOpts) toPromHistogramOpts() prometheus.HistogramOpts { // a help string and to explicitly set the Objectives field to the desired value // as the default value will change in the upcoming v0.10 of the library. type SummaryOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - Objectives map[float64]float64 - MaxAge time.Duration - AgeBuckets uint32 - BufCap uint32 - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Objectives map[float64]float64 + MaxAge time.Duration + AgeBuckets uint32 + BufCap uint32 + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + LabelValueAllowLists *MetricLabelAllowList } // Modify help description on the metric description. From a5c4780e96d96a7e71d433752ac8bdef789d43e0 Mon Sep 17 00:00:00 2001 From: yoyinzyc Date: Wed, 3 Mar 2021 15:08:29 -0800 Subject: [PATCH 3/3] enforce metric cardinality check to summary metric --- .../k8s.io/component-base/metrics/summary.go | 15 +++- .../component-base/metrics/summary_test.go | 77 +++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/component-base/metrics/summary.go b/staging/src/k8s.io/component-base/metrics/summary.go index 5d3be8def62..4732b02e308 100644 --- a/staging/src/k8s.io/component-base/metrics/summary.go +++ b/staging/src/k8s.io/component-base/metrics/summary.go @@ -99,12 +99,19 @@ type SummaryVec struct { func NewSummaryVec(opts *SummaryOpts, labels []string) *SummaryVec { opts.StabilityLevel.setDefaults() + fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[fqName]; ok { + opts.LabelValueAllowLists = allowList + } + allowListLock.RUnlock() + v := &SummaryVec{ SummaryOpts: opts, originalLabels: labels, lazyMetric: lazyMetric{}, } - v.lazyInit(v, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + v.lazyInit(v, fqName) return v } @@ -139,6 +146,9 @@ func (v *SummaryVec) WithLabelValues(lvs ...string) ObserverMetric { if !v.IsCreated() { return noop } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } return v.SummaryVec.WithLabelValues(lvs...) } @@ -150,6 +160,9 @@ func (v *SummaryVec) With(labels map[string]string) ObserverMetric { if !v.IsCreated() { return noop } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainLabelMap(labels) + } return v.SummaryVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/summary_test.go b/staging/src/k8s.io/component-base/metrics/summary_test.go index 752ae5d5992..86ab5badefb 100644 --- a/staging/src/k8s.io/component-base/metrics/summary_test.go +++ b/staging/src/k8s.io/component-base/metrics/summary_test.go @@ -198,3 +198,80 @@ func TestSummaryVec(t *testing.T) { }) } } + +func TestSummaryWithLabelValueAllowList(t *testing.T) { + labelAllowValues := map[string]string{ + "namespace_subsystem_metric_allowlist_test,label_a": "allowed", + } + labels := []string{"label_a", "label_b"} + opts := &SummaryOpts{ + Namespace: "namespace", + Name: "metric_allowlist_test", + Subsystem: "subsystem", + } + var tests = []struct { + desc string + labelValues [][]string + expectMetricValues map[string]int + }{ + { + desc: "Test no unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"allowed", "b2"}}, + expectMetricValues: map[string]int{ + "allowed b1": 1, + "allowed b2": 1, + }, + }, + { + desc: "Test unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"not_allowed", "b1"}}, + expectMetricValues: map[string]int{ + "allowed b1": 1, + "unexpected b1": 1, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + SetLabelAllowListFromCLI(labelAllowValues) + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + c := NewSummaryVec(opts, labels) + registry.MustRegister(c) + + for _, lv := range test.labelValues { + c.WithLabelValues(lv...).Observe(1.0) + } + mfs, err := registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range mfs { + if *mf.Name != BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) { + continue + } + mfMetric := mf.GetMetric() + + for _, m := range mfMetric { + var aValue, bValue string + for _, l := range m.Label { + if *l.Name == "label_a" { + aValue = *l.Value + } + if *l.Name == "label_b" { + bValue = *l.Value + } + } + labelValuePair := aValue + " " + bValue + expectedValue, ok := test.expectMetricValues[labelValuePair] + assert.True(t, ok, "Got unexpected label values, lable_a is %v, label_b is %v", aValue, bValue) + actualValue := int(m.GetSummary().GetSampleCount()) + assert.Equalf(t, expectedValue, actualValue, "Got %v, wanted %v as the count while setting label_a to %v and label b to %v", actualValue, expectedValue, aValue, bValue) + } + } + }) + } +}