fix: Enforce metric naming validation (argoproj#2819)

docs/metrics.md

@@ -92,6 +92,8 @@ When defining a `histogram`, `buckets` must also be provided (see below).
 
 [Argo variables](variables.md) can be included anywhere in the metric spec, such as in `labels`, `name`, `help`, `when`, etc.
 
+Metric names can only contain alphanumeric characters, `_`, and `:`.
+
 ### Metric Spec
 
  In Argo you can define a metric on the `Workflow` level or on the `Template` level. Here is an example of a `Workflow`
@@ -208,4 +210,4 @@ To define a realtime metric simply add `realtime: true` to a gauge metric with a
  gauge:
  realtime: true
  value: "{{duration}}"
-```
+```

workflow/controller/operator.go

@@ -2434,6 +2434,15 @@ func (woc *wfOperationCtx) computeMetrics(metricList []*wfv1.Prometheus, localSc
  continue
  }
 
+ if !validate.MetricNameRegex.MatchString(metricTmpl.Name) {
+ woc.reportMetricEmissionError(fmt.Sprintf("metric name '%s' is invalid. Metric names must contain alphanumeric characters, '_', or ':'", metricTmpl.Name))
+ continue
+ }
+ if metricTmpl.Help == "" {
+ woc.reportMetricEmissionError(fmt.Sprintf("metric '%s' must contain a help string under 'help: ' field", metricTmpl.Name))
+ continue
+ }
+
  // Substitute parameters in non-value fields of the template to support variables in places such as labels,
  // name, and help. We do not substitute value fields here (i.e. gauge, histogram, counter) here because they
  // might be realtime ({{workflow.duration}} will not be substituted the same way if it's realtime or if it isn't).

workflow/validate/validate.go

@@ -302,6 +302,7 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
  if err != nil {
  return err
  }
+
  localParams := make(map[string]string)
  if tmpl.IsPodType() {
  localParams[common.LocalVarPodName] = placeholderGenerator.NextPlaceholder()
@@ -362,6 +363,16 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
  return err
  }
  }
+ if newTmpl.Metrics != nil {
+ for _, metric := range newTmpl.Metrics.Prometheus {
+ if !MetricNameRegex.MatchString(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 metric.Help == "" {
+ return errors.Errorf(errors.CodeBadRequest, "templates.%s metric '%s' must contain a help string under 'help: ' field", tmpl.Name, metric.Name)
+ }
+ }
+ }
  return nil
 }
 
@@ -1167,6 +1178,8 @@ var (
  // paramRegex matches a parameter. e.g. {{inputs.parameters.blah}}
  paramRegex = regexp.MustCompile(`{{[-a-zA-Z0-9]+(\.[-a-zA-Z0-9_]+)*}}`)
  paramOrArtifactNameRegex = regexp.MustCompile(`^[-a-zA-Z0-9_]+[-a-zA-Z0-9_]*$`)
+ workflowFieldNameRegex = regexp.MustCompile("^" + workflowFieldNameFmt + "$")
+ MetricNameRegex = regexp.MustCompile(`^[a-zA-Z_:][a-zA-Z_:0-9]*$`)
 )
 
 func isParameter(p string) bool {
@@ -1187,15 +1200,13 @@ const (
  workflowFieldMaxLength int = 128
 )
 
-var workflowFieldNameRegexp = regexp.MustCompile("^" + workflowFieldNameFmt + "$")
-
 // isValidWorkflowFieldName : workflow field name must consist of alpha-numeric characters or '-', and must start with an alpha-numeric character
 func isValidWorkflowFieldName(name string) []string {
  var errs []string
  if len(name) > workflowFieldMaxLength {
  errs = append(errs, apivalidation.MaxLenError(workflowFieldMaxLength))
  }
- if !workflowFieldNameRegexp.MatchString(name) {
+ if !workflowFieldNameRegex.MatchString(name) {
  msg := workflowFieldNameErrMsg + " (e.g. My-name1-2, 123-NAME)"
  errs = append(errs, msg)
  }

workflow/validate/validate_test.go

@@ -2292,3 +2292,50 @@ func TestStepWithItemParam(t *testing.T) {
  _, err := validate(stepWithItemParam)
  assert.NoError(t, err)
 }
+
+var invalidMetricName = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: hello-world-
+spec:
+ entrypoint: whalesay
+ templates:
+ - name: whalesay
+ metrics:
+ prometheus:
+ - name: invalid.metric.name
+ help: "invalid"
+ gauge:
+ value: 1
+ container:
+ image: docker/whalesay:latest
+`
+
+func TestInvalidMetricName(t *testing.T) {
+ _, err := validate(invalidMetricName)
+ assert.EqualError(t, err, "templates.whalesay metric name 'invalid.metric.name' is invalid. Metric names must contain alphanumeric characters, '_', or ':'")
+}
+
+var invalidMetricHelp = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: hello-world-
+spec:
+ entrypoint: whalesay
+ templates:
+ - name: whalesay
+ metrics:
+ prometheus:
+ - name: metric_name
+ gauge:
+ value: 1
+ container:
+ image: docker/whalesay:latest
+`
+
+func TestInvalidMetricHelp(t *testing.T) {
+ _, err := validate(invalidMetricHelp)
+ assert.EqualError(t, err, "templates.whalesay metric 'metric_name' must contain a help string under 'help: ' field")
+}