From a44bb76f1e9f7c2d1680b506d5d388eb388ea49c Mon Sep 17 00:00:00 2001 From: yoyinzyc Date: Tue, 23 Feb 2021 15:21:46 -0800 Subject: [PATCH 1/2] add flag allow-metric-labels to get the input metric label value allowlist. --- .../k8s.io/component-base/metrics/options.go | 36 ++++++++- .../component-base/metrics/options_test.go | 67 ++++++++++++++++ .../src/k8s.io/component-base/metrics/opts.go | 72 ++++++++++++++--- .../component-base/metrics/opts_test.go | 79 +++++++++++++++++++ 4 files changed, 243 insertions(+), 11 deletions(-) create mode 100644 staging/src/k8s.io/component-base/metrics/options_test.go diff --git a/staging/src/k8s.io/component-base/metrics/options.go b/staging/src/k8s.io/component-base/metrics/options.go index ec14b9968fb..91a76ba7e51 100644 --- a/staging/src/k8s.io/component-base/metrics/options.go +++ b/staging/src/k8s.io/component-base/metrics/options.go @@ -18,6 +18,7 @@ package metrics import ( "fmt" + "regexp" "github.com/blang/semver" "github.com/spf13/pflag" @@ -29,6 +30,7 @@ import ( type Options struct { ShowHiddenMetricsForVersion string DisabledMetrics []string + AllowListMapping map[string]string } // NewOptions returns default metrics options @@ -38,12 +40,20 @@ func NewOptions() *Options { // Validate validates metrics flags options. func (o *Options) Validate() []error { + var errs []error err := validateShowHiddenMetricsVersion(parseVersion(version.Get()), o.ShowHiddenMetricsForVersion) if err != nil { - return []error{err} + errs = append(errs, err) } - return nil + if err := validateAllowMetricLabel(o.AllowListMapping); err != nil { + errs = append(errs, err) + } + + if len(errs) == 0 { + return nil + } + return errs } // AddFlags adds flags for exposing component metrics. @@ -63,6 +73,10 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) { "This flag provides an escape hatch for misbehaving metrics. "+ "You must provide the fully qualified metric name in order to disable it. "+ "Disclaimer: disabling metrics is higher in precedence than showing hidden metrics.") + fs.StringToStringVar(&o.AllowListMapping, "allow-metric-labels", o.AllowListMapping, + "The map from metric-label to value allow-list of this label. The key's format is ,. "+ + "The value's format is ,..."+ + "e.g. metric1,label1='v1,v2,v3', metric1,label2='v1,v2,v3' metric2,label1='v1,v2,v3'.") } // Apply applies parameters into global configuration of metrics. @@ -77,6 +91,9 @@ func (o *Options) Apply() { for _, metricName := range o.DisabledMetrics { SetDisabledMetric(metricName) } + if o.AllowListMapping != nil { + SetLabelAllowListFromCLI(o.AllowListMapping) + } } func validateShowHiddenMetricsVersion(currentVersion semver.Version, targetVersionStr string) error { @@ -91,3 +108,18 @@ func validateShowHiddenMetricsVersion(currentVersion semver.Version, targetVersi return nil } + +func validateAllowMetricLabel(allowListMapping map[string]string) error { + if allowListMapping == nil { + return nil + } + metricNameRegex := `[a-zA-Z_:][a-zA-Z0-9_:]*` + labelRegex := `[a-zA-Z_][a-zA-Z0-9_]*` + for k := range allowListMapping { + reg := regexp.MustCompile(metricNameRegex + `,` + labelRegex) + if reg.FindString(k) != k { + return fmt.Errorf("--allow-metric-labels must has a list of kv pair with format `metricName:labelName=labelValue, labelValue,...`") + } + } + return nil +} diff --git a/staging/src/k8s.io/component-base/metrics/options_test.go b/staging/src/k8s.io/component-base/metrics/options_test.go new file mode 100644 index 00000000000..22792ef8699 --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/options_test.go @@ -0,0 +1,67 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package metrics + +import "testing" + +func TestValidateAllowMetricLabel(t *testing.T) { + var tests = []struct { + name string + input map[string]string + expectedError bool + }{ + { + "validated", + map[string]string{ + "metric_name,label_name": "labelValue1,labelValue2", + }, + false, + }, + { + "metric name is not valid", + map[string]string{ + "-metric_name,label_name": "labelValue1,labelValue2", + }, + true, + }, + { + "label name is not valid", + map[string]string{ + "metric_name,:label_name": "labelValue1,labelValue2", + }, + true, + }, + { + "no label name", + map[string]string{ + "metric_name": "labelValue1,labelValue2", + }, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateAllowMetricLabel(tt.input) + if err == nil && tt.expectedError { + t.Error("Got error is nil, wanted error is not nil") + } + if err != nil && !tt.expectedError { + t.Errorf("Got error is %v, wanted no error", err) + } + }) + } +} diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 906050c7f78..395913936c6 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -18,10 +18,17 @@ package metrics import ( "fmt" + "strings" "sync" "time" "github.com/prometheus/client_golang/prometheus" + "k8s.io/apimachinery/pkg/util/sets" +) + +var ( + labelValueAllowLists = map[string]*MetricLabelAllowList{} + allowListLock sync.RWMutex ) // KubeOpts is superset struct for prometheus.Opts. The prometheus Opts structure @@ -31,15 +38,16 @@ import ( // Name must be set to a non-empty string. DeprecatedVersion is defined only // if the metric for which this options applies is, in fact, deprecated. type KubeOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + LabelValueAllowLists *MetricLabelAllowList } // BuildFQName joins the given three name components by "_". Empty name @@ -243,3 +251,49 @@ func (o *SummaryOpts) toPromSummaryOpts() prometheus.SummaryOpts { BufCap: o.BufCap, } } + +type MetricLabelAllowList struct { + labelToAllowList map[string]sets.String +} + +func (allowList *MetricLabelAllowList) ConstrainToAllowedList(labelNameList, labelValueList []string) { + for index, value := range labelValueList { + name := labelNameList[index] + if allowValues, ok := allowList.labelToAllowList[name]; ok { + if !allowValues.Has(value) { + labelValueList[index] = "unexpected" + } + } + } +} + +func (allowList *MetricLabelAllowList) ConstrainLabelMap(labels map[string]string) { + for name, value := range labels { + if allowValues, ok := allowList.labelToAllowList[name]; ok { + if !allowValues.Has(value) { + labels[name] = "unexpected" + } + } + } +} + +func SetLabelAllowListFromCLI(allowListMapping map[string]string) { + allowListLock.Lock() + defer allowListLock.Unlock() + for metricLabelName, labelValues := range allowListMapping { + metricName := strings.Split(metricLabelName, ",")[0] + labelName := strings.Split(metricLabelName, ",")[1] + valueSet := sets.NewString(strings.Split(labelValues, ",")...) + + allowList, ok := labelValueAllowLists[metricName] + if ok { + allowList.labelToAllowList[labelName] = valueSet + } else { + labelToAllowList := make(map[string]sets.String) + labelToAllowList[labelName] = valueSet + labelValueAllowLists[metricName] = &MetricLabelAllowList{ + labelToAllowList, + } + } + } +} diff --git a/staging/src/k8s.io/component-base/metrics/opts_test.go b/staging/src/k8s.io/component-base/metrics/opts_test.go index db4574a0a09..14e20f4bff4 100644 --- a/staging/src/k8s.io/component-base/metrics/opts_test.go +++ b/staging/src/k8s.io/component-base/metrics/opts_test.go @@ -17,9 +17,11 @@ limitations under the License. package metrics import ( + "reflect" "testing" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/sets" ) func TestDefaultStabilityLevel(t *testing.T) { @@ -59,3 +61,80 @@ func TestDefaultStabilityLevel(t *testing.T) { }) } } + +func TestConstrainToAllowedList(t *testing.T) { + allowList := &MetricLabelAllowList{ + labelToAllowList: map[string]sets.String{ + "label_a": sets.NewString("allow_value1", "allow_value2"), + }, + } + labelNameList := []string{"label_a", "label_b"} + var tests = []struct { + name string + inputLabelValueList []string + outputLabelValueList []string + }{ + { + "no unexpected value", + []string{"allow_value1", "label_b_value"}, + []string{"allow_value1", "label_b_value"}, + }, + { + "with unexpected value", + []string{"not_allowed", "label_b_value"}, + []string{"unexpected", "label_b_value"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + allowList.ConstrainToAllowedList(labelNameList, test.inputLabelValueList) + if !reflect.DeepEqual(test.inputLabelValueList, test.outputLabelValueList) { + t.Errorf("Got %v, expected %v", test.inputLabelValueList, test.outputLabelValueList) + } + }) + } +} + +func TestConstrainLabelMap(t *testing.T) { + allowList := &MetricLabelAllowList{ + labelToAllowList: map[string]sets.String{ + "label_a": sets.NewString("allow_value1", "allow_value2"), + }, + } + var tests = []struct { + name string + inputLabelMap map[string]string + outputLabelMap map[string]string + }{ + { + "no unexpected value", + map[string]string{ + "label_a": "allow_value1", + "label_b": "label_b_value", + }, + map[string]string{ + "label_a": "allow_value1", + "label_b": "label_b_value", + }, + }, + { + "with unexpected value", + map[string]string{ + "label_a": "not_allowed", + "label_b": "label_b_value", + }, + map[string]string{ + "label_a": "unexpected", + "label_b": "label_b_value", + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + allowList.ConstrainLabelMap(test.inputLabelMap) + if !reflect.DeepEqual(test.inputLabelMap, test.outputLabelMap) { + t.Errorf("Got %v, expected %v", test.inputLabelMap, test.outputLabelMap) + } + }) + } +} From b81e2d18f9b307b646617a2b2fdc22ca7d86f00d Mon Sep 17 00:00:00 2001 From: yoyinzyc Date: Tue, 23 Feb 2021 15:22:41 -0800 Subject: [PATCH 2/2] enforce metric cardinality check to counter metric. --- .../k8s.io/component-base/metrics/counter.go | 15 +++- .../component-base/metrics/counter_test.go | 77 +++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 34c177fe1ac..addf680c87b 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -112,13 +112,20 @@ type CounterVec struct { func NewCounterVec(opts *CounterOpts, labels []string) *CounterVec { 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 := &CounterVec{ CounterVec: noopCounterVec, CounterOpts: opts, originalLabels: labels, lazyMetric: lazyMetric{}, } - cv.lazyInit(cv, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + cv.lazyInit(cv, fqName) return cv } @@ -158,6 +165,9 @@ func (v *CounterVec) WithLabelValues(lvs ...string) CounterMetric { if !v.IsCreated() { return noop // return no-op counter } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } return v.CounterVec.WithLabelValues(lvs...) } @@ -169,6 +179,9 @@ func (v *CounterVec) With(labels map[string]string) CounterMetric { if !v.IsCreated() { return noop // return no-op counter } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainLabelMap(labels) + } return v.CounterVec.With(labels) } 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 9a0c951fbce..3595a57946d 100644 --- a/staging/src/k8s.io/component-base/metrics/counter_test.go +++ b/staging/src/k8s.io/component-base/metrics/counter_test.go @@ -208,3 +208,80 @@ func TestCounterVec(t *testing.T) { }) } } + +func TestCounterWithLabelValueAllowList(t *testing.T) { + labelAllowValues := map[string]string{ + "namespace_subsystem_metric_test_name,label_a": "allowed", + } + labels := []string{"label_a", "label_b"} + opts := &CounterOpts{ + Namespace: "namespace", + Name: "metric_test_name", + 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 := NewCounterVec(opts, labels) + registry.MustRegister(c) + + for _, lv := range test.labelValues { + c.WithLabelValues(lv...).Inc() + } + 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.GetCounter().GetValue()) + 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) + } + } + }) + } +}