fix: Validate metric key names (#4540)

Signed-off-by: Simon Behar <[email protected]>
argoproj · Nov 17, 2020 · a712e53 · a712e53
1 parent c469b05
commit a712e53
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 3 deletions.
diff --git a/workflow/metrics/util.go b/workflow/metrics/util.go
@@ -14,6 +14,7 @@ import (
 
 var (
  invalidMetricNameError = "metric name is invalid: names may only contain alphanumeric characters, '_', or ':'"
+ invalidMetricLabelrror = "metric label '%s' is invalid: keys may only contain alphanumeric characters, '_', or ':'"
  descRegex = regexp.MustCompile(fmt.Sprintf(`Desc{fqName: "%s_%s_(.+?)", help: "(.+?)", constLabels: {`, argoNamespace, workflowsSubsystem))
 )
 
@@ -56,7 +57,11 @@ func ConstructRealTimeGaugeMetric(metricSpec *wfv1.Prometheus, valueFunc func()
 
 func constructOrUpdateCounterMetric(metric prometheus.Metric, metricSpec *wfv1.Prometheus) (prometheus.Metric, error) {
  if metric == nil {
- metric = newCounter(metricSpec.Name, metricSpec.Help, metricSpec.GetMetricLabels())
+ labels := metricSpec.GetMetricLabels()
+ if err := ValidateMetricLabels(labels); err != nil {
+ return nil, err
+ }
+ metric = newCounter(metricSpec.Name, metricSpec.Help, labels)
  }
 
  val, err := strconv.ParseFloat(metricSpec.Counter.Value, 64)
@@ -71,7 +76,11 @@ func constructOrUpdateCounterMetric(metric prometheus.Metric, metricSpec *wfv1.P
 
 func constructOrUpdateGaugeMetric(metric prometheus.Metric, metricSpec *wfv1.Prometheus) (prometheus.Metric, error) {
  if metric == nil {
- metric = newGauge(metricSpec.Name, metricSpec.Help, metricSpec.GetMetricLabels())
+ labels := metricSpec.GetMetricLabels()
+ if err := ValidateMetricLabels(labels); err != nil {
+ return nil, err
+ }
+ metric = newGauge(metricSpec.Name, metricSpec.Help, labels)
  }
 
  val, err := strconv.ParseFloat(metricSpec.Gauge.Value, 64)
@@ -86,7 +95,11 @@ func constructOrUpdateGaugeMetric(metric prometheus.Metric, metricSpec *wfv1.Pro
 
 func constructOrUpdateHistogramMetric(metric prometheus.Metric, metricSpec *wfv1.Prometheus) (prometheus.Metric, error) {
  if metric == nil {
- metric = newHistogram(metricSpec.Name, metricSpec.Help, metricSpec.GetMetricLabels(), metricSpec.Histogram.GetBuckets())
+ labels := metricSpec.GetMetricLabels()
+ if err := ValidateMetricLabels(labels); err != nil {
+ return nil, err
+ }
+ metric = newHistogram(metricSpec.Name, metricSpec.Help, labels, metricSpec.Histogram.GetBuckets())
  }
 
  val, err := strconv.ParseFloat(metricSpec.Histogram.Value, 64)
@@ -179,6 +192,15 @@ func IsValidMetricName(name string) bool {
  return model.IsValidMetricName(model.LabelValue(name))
 }
 
+func ValidateMetricLabels(metrics map[string]string) error {
+ for name := range metrics {
+ if !IsValidMetricName(name) {
+ return fmt.Errorf(invalidMetricLabelrror, name)
+ }
+ }
+ return nil
+}
+
 func mustBeRecoverable(name, help string, metric prometheus.Metric) {
  recoveredName, recoveredHelp := recoverMetricNameAndHelpFromDesc(metric.Desc().String())
  if name != recoveredName {

diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
@@ -449,6 +449,9 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
  if !metrics.IsValidMetricName(metric.Name) {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s metric name '%s' is invalid. Metric names must contain alphanumeric characters, '_', or ':'", tmpl.Name, metric.Name)
  }
+ if err := metrics.ValidateMetricLabels(metric.GetMetricLabels()); err != nil {
+ return err
+ }
  if metric.Help == "" {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s metric '%s' must contain a help string under 'help: ' field", tmpl.Name, metric.Name)
  }

diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
@@ -2318,6 +2318,33 @@ func TestInvalidMetricName(t *testing.T) {
  assert.EqualError(t, err, "templates.whalesay metric name 'invalid.metric.name' is invalid. Metric names must contain alphanumeric characters, '_', or ':'")
 }
 
+var invalidMetricLabelName = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: hello-world-
+spec:
+ entrypoint: whalesay
+ templates:
+ - name: whalesay
+ metrics:
+ prometheus:
+ - name: valid
+ help: "invalid"
+ labels:
+ - key: invalid.key
+ value: hi
+ gauge:
+ value: 1
+ container:
+ image: docker/whalesay:latest
+`
+
+func TestInvalidMetricLabelName(t *testing.T) {
+ _, err := validate(invalidMetricLabelName)
+ assert.EqualError(t, err, "metric label 'invalid.key' is invalid: keys may only contain alphanumeric characters, '_', or ':'")
+}
+
 var invalidMetricHelp = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow