fix: AnalysisRun args could not be resolved from secret (argoproj#1213)

* fix: Fix ValueFrom resolution and validation in AnalysisTemplate

Author: khhirani

analysis/analysis.go

@@ -41,17 +41,6 @@ func (c *Controller) reconcileAnalysisRun(origRun *v1alpha1.AnalysisRun) *v1alph
 	log := logutil.WithAnalysisRun(origRun)
 	run := origRun.DeepCopy()
 
-	metrics, err := analysisutil.ResolveMetrics(run.Spec.Metrics, run.Spec.Args)
-	if err != nil {
-		message := fmt.Sprintf("unable to resolve metric arguments: %v", err)
-		log.Warn(message)
-		run.Status.Phase = v1alpha1.AnalysisPhaseError
-		run.Status.Message = message
-		c.recordAnalysisRunCompletionEvent(run)
-		return run
-	}
-	run.Spec.Metrics = metrics
-
 	if run.Status.MetricResults == nil {
 		run.Status.MetricResults = make([]v1alpha1.MetricResult, 0)
 		err := analysisutil.ValidateMetrics(run.Spec.Metrics)
@@ -67,7 +56,7 @@ func (c *Controller) reconcileAnalysisRun(origRun *v1alpha1.AnalysisRun) *v1alph
 
 	tasks := generateMetricTasks(run)
 	log.Infof("taking %d measurements", len(tasks))
-	err = c.runMeasurements(run, tasks)
+	err := c.runMeasurements(run, tasks)
 	if err != nil {
 		message := fmt.Sprintf("unable to resolve metric arguments: %v", err)
 		log.Warn(message)

analysis/analysis_test.go

@@ -1087,42 +1087,6 @@ func TestResolveMetricArgsUnableToSubstitute(t *testing.T) {
 	assert.Equal(t, newRun.Status.Message, "unable to resolve metric arguments: failed to resolve {{args.metric-name}}")
 }
 
-func TestSecretContentReferenceValueFromError(t *testing.T) {
-	f := newFixture(t)
-	defer f.Close()
-	c, _, _ := f.newController(noResyncPeriodFunc)
-	argName := "apikey"
-	argVal := "value"
-	run := &v1alpha1.AnalysisRun{
-		Spec: v1alpha1.AnalysisRunSpec{
-			Args: []v1alpha1.Argument{{
-				Name:  argName,
-				Value: &argVal,
-				ValueFrom: &v1alpha1.ValueFrom{
-					SecretKeyRef: &v1alpha1.SecretKeyRef{
-						Name: "web-metric-secret",
-						Key:  "apikey",
-					},
-				}},
-			},
-			Metrics: []v1alpha1.Metric{{
-				Name: "rate",
-				Provider: v1alpha1.MetricProvider{
-					Web: &v1alpha1.WebMetric{
-						Headers: []v1alpha1.WebMetricHeader{{
-							Key:   "apikey",
-							Value: "{{args.apikey}}",
-						}},
-					},
-				},
-			}},
-		},
-	}
-	newRun := c.reconcileAnalysisRun(run)
-	assert.Equal(t, v1alpha1.AnalysisPhaseError, newRun.Status.Phase)
-	assert.Equal(t, fmt.Sprintf("unable to resolve metric arguments: arg '%v' has both Value and ValueFrom fields", argName), newRun.Status.Message)
-}
-
 // TestSecretContentReferenceSuccess verifies that secret arguments are properly resolved
 func TestSecretContentReferenceSuccess(t *testing.T) {
 	f := newFixture(t)
@@ -1353,6 +1317,44 @@ func TestKeyNotInSecret(t *testing.T) {
 	assert.Equal(t, "key 'key-name' does not exist in secret 'secret-name'", err.Error())
 }
 
+func TestSecretResolution(t *testing.T) {
+	f := newFixture(t)
+	secretName, secretKey, secretData := "web-metric-secret", "apikey", "12345"
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: metav1.NamespaceDefault,
+		},
+		Data: map[string][]byte{
+			secretKey: []byte(secretData),
+		},
+	}
+	defer f.Close()
+	c, _, _ := f.newController(noResyncPeriodFunc)
+	f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{})
+
+	args := []v1alpha1.Argument{{
+		Name: "secret",
+		ValueFrom: &v1alpha1.ValueFrom{
+			SecretKeyRef: &v1alpha1.SecretKeyRef{
+				Name: secretName,
+				Key:  secretKey,
+			},
+		},
+	}}
+	tasks := []metricTask{{
+		metric: v1alpha1.Metric{
+			Name:             "metric-name",
+			SuccessCondition: "{{args.secret}}",
+		},
+		incompleteMeasurement: nil,
+	}}
+	metricTaskList, secretList, _ := c.resolveArgs(tasks, args, metav1.NamespaceDefault)
+
+	assert.Equal(t, secretData, metricTaskList[0].metric.SuccessCondition)
+	assert.Contains(t, secretList, secretData)
+}
+
 // TestAssessMetricFailureInconclusiveOrError verifies that assessMetricFailureInconclusiveOrError returns the correct phases and messages
 // for Failed, Inconclusive, and Error metrics respectively
 func TestAssessMetricFailureInconclusiveOrError(t *testing.T) {

examples/rollout-secret.yaml

@@ -1,5 +1,5 @@
 # This example demonstrates a Rollout which starts and finishes analysis at a specific canary step.
-# The AnalysisTemplate references an Secret object, which contains an API, token and passes it to a Web metric provider.
+# The AnalysisTemplate references an Secret object, which contains the URL, and passes it to a Web metric provider.
 #
 # Prerequisites: None
 
@@ -20,7 +20,7 @@ spec:
     spec:
       containers:
       - name: rollouts-demo
-        image: argoproj/rollouts-demo:green
+        image: argoproj/rollouts-demo:blue
         imagePullPolicy: Always
         ports:
         - containerPort: 8080
@@ -36,31 +36,27 @@ spec:
 apiVersion: v1
 kind: Secret
 metadata:
-  name: token-secret
+  name: example-secret
 type: Opaque
 data:
-  # This API Token is fake. Its value decoded is "test".
-  apiToken: dGVzdAo=
+  secretUrl: aHR0cHM6Ly9naXN0LmdpdGh1YnVzZXJjb250ZW50LmNvbS9raGhpcmFuaS8yYWIxMTIzMjQwMjUxOGQ1Mjc3YWYwMzBkZDg5MTZkNy9yYXcvZDI3MmY1NTFmMmQxODA2YTAzOTc0ZGJhZWYxMWRmZDU1MTAyZmVlYS9leGFtcGxlLmpzb24=
 ---
 kind: AnalysisTemplate
 apiVersion: argoproj.io/v1alpha1
 metadata:
   name: analysis-secret
 spec:
   args:
-  - name: api-token
+  - name: secret-url
     valueFrom:
       secretKeyRef:
-        name: token-secret
-        key: apiToken
+        name: example-secret
+        key: secretUrl
   metrics:
   - name: webmetric
     successCondition: result == 'It worked!'
     provider:
       web:
       # placeholders are resolved when an AnalysisRun is created
-        url: "https://gist.githubusercontent.com/khhirani/2ab11232402518d5277af030dd8916d7/raw/d272f551f2d1806a03974dbaef11dfd55102feea/example.json"
-        headers:
-        - key: Test
-          value: "{{ args.api-token }}"
+        url: "{{args.secret-url}}"
         jsonPath: "{$.message}"

pkg/apis/rollouts/validation/validation_references.go

@@ -139,7 +139,7 @@ func ValidateAnalysisTemplateWithType(rollout *v1alpha1.Rollout, template *v1alp
 
 	if templateType != BackgroundAnalysis {
 		setArgValuePlaceHolder(templateSpec.Args)
-		resolvedMetrics, err := analysisutil.ResolveMetrics(templateSpec.Metrics, templateSpec.Args)
+		resolvedMetrics, err := validateAnalysisMetrics(templateSpec.Metrics, templateSpec.Args)
 		if err != nil {
 			msg := fmt.Sprintf("AnalysisTemplate %s: %v", templateName, err)
 			allErrs = append(allErrs, field.Invalid(fldPath, templateName, msg))
@@ -293,3 +293,25 @@ func GetAnalysisTemplateWithTypeFieldPath(templateType AnalysisTemplateType, can
 	}
 	return fldPath
 }
+
+// validateAnalysisMetrics validates the metrics of an Analysis object
+func validateAnalysisMetrics(metrics []v1alpha1.Metric, args []v1alpha1.Argument) ([]v1alpha1.Metric, error) {
+	for i, arg := range args {
+		if arg.ValueFrom != nil {
+			if arg.Value != nil {
+				return nil, fmt.Errorf("arg '%s' has both Value and ValueFrom fields", arg.Name)
+			}
+			argVal := "dummy-value"
+			args[i].Value = &argVal
+		}
+	}
+
+	for i, metric := range metrics {
+		resolvedMetric, err := analysisutil.ResolveMetricArgs(metric, args)
+		if err != nil {
+			return nil, err
+		}
+		metrics[i] = *resolvedMetric
+	}
+	return metrics, nil
+}

pkg/apis/rollouts/validation/validation_references_test.go

@@ -497,3 +497,49 @@ func toUnstructured(t *testing.T, manifest string) *k8sunstructured.Unstructured
 	}
 	return obj
 }
+
+func TestValidateAnalysisMetrics(t *testing.T) {
+	count, failureLimit := "5", "1"
+	args := []v1alpha1.Argument{
+		{
+			Name:  "count",
+			Value: &count,
+		},
+		{
+			Name:  "failure-limit",
+			Value: &failureLimit,
+		},
+		{
+			Name: "secret",
+			ValueFrom: &v1alpha1.ValueFrom{
+				SecretKeyRef: &v1alpha1.SecretKeyRef{
+					Name: "web-metric-secret",
+					Key:  "apikey",
+				},
+			},
+		},
+	}
+
+	countVal := intstr.FromString("{{args.count}}")
+	failureLimitVal := intstr.FromString("{{args.failure-limit}}")
+	metrics := []v1alpha1.Metric{{
+		Name:         "metric-name",
+		Count:        &countVal,
+		FailureLimit: &failureLimitVal,
+	}}
+
+	t.Run("Success", func(t *testing.T) {
+		resolvedMetrics, err := validateAnalysisMetrics(metrics, args)
+		assert.Nil(t, err)
+		assert.Equal(t, count, resolvedMetrics[0].Count.String())
+		assert.Equal(t, failureLimit, resolvedMetrics[0].FailureLimit.String())
+	})
+
+	t.Run("Error: arg has both Value and ValueFrom", func(t *testing.T) {
+		args[2].Value = pointer.StringPtr("secret-value")
+		_, err := validateAnalysisMetrics(metrics, args)
+		assert.NotNil(t, err)
+		assert.Equal(t, "arg 'secret' has both Value and ValueFrom fields", err.Error())
+
+	})
+}

test/e2e/analysis_test.go

@@ -598,3 +598,24 @@ spec:
 		Then().
 		ExpectAnalysisRunCount(1)
 }
+
+func (s *AnalysisSuite) TestAnalysisWithSecret() {
+	(s.Given().
+		RolloutObjects("@functional/rollout-secret.yaml").
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		ExpectAnalysisRunCount(0).
+		When().
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		Then().
+		ExpectAnalysisRunCount(1).
+		When().
+		WaitForInlineAnalysisRunPhase("Successful").
+		PromoteRollout().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		ExpectStableRevision("2"))
+}

test/e2e/functional/rollout-secret.yaml

@@ -0,0 +1,56 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-secret
+spec:
+  replicas: 1
+  revisionHistoryLimit: 2
+  selector:
+    matchLabels:
+      app: rollout-secret
+  template:
+    metadata:
+      labels:
+        app: rollout-secret
+    spec:
+      containers:
+      - name: rollouts-demo
+        image: argoproj/rollouts-demo:blue
+        imagePullPolicy: Always
+        ports:
+        - containerPort: 8080
+  strategy:
+    canary:
+      steps:
+      - setWeight: 25
+      - analysis:
+          templates:
+          - templateName: analysis-secret
+      - pause: {}
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: example-secret
+type: Opaque
+data:
+  secretUrl: aHR0cHM6Ly9naXN0LmdpdGh1YnVzZXJjb250ZW50LmNvbS9raGhpcmFuaS8yYWIxMTIzMjQwMjUxOGQ1Mjc3YWYwMzBkZDg5MTZkNy9yYXcvZDI3MmY1NTFmMmQxODA2YTAzOTc0ZGJhZWYxMWRmZDU1MTAyZmVlYS9leGFtcGxlLmpzb24=
+---
+kind: AnalysisTemplate
+apiVersion: argoproj.io/v1alpha1
+metadata:
+  name: analysis-secret
+spec:
+  args:
+  - name: secret-url
+    valueFrom:
+      secretKeyRef:
+        name: example-secret
+        key: secretUrl
+  metrics:
+  - name: webmetric
+    successCondition: result == 'It worked!'
+    provider:
+      web:
+        url: "{{args.secret-url}}"
+        jsonPath: "{$.message}"

utils/analysis/factory.go

@@ -96,7 +96,7 @@ func StepLabels(index int32, podHash, instanceID string) map[string]string {
 	return labels
 }
 
-// resolveMetricArgs resolves args for single metric in AnalysisRun
+// ResolveMetricArgs resolves args for single metric in AnalysisRun
 // Returns resolved metric
 // Uses ResolveQuotedArgs to handle escaped quotes
 func ResolveMetricArgs(metric v1alpha1.Metric, args []v1alpha1.Argument) (*v1alpha1.Metric, error) {
@@ -117,27 +117,6 @@ func ResolveMetricArgs(metric v1alpha1.Metric, args []v1alpha1.Argument) (*v1alp
 	return &newMetric, nil
 }
 
-func ResolveMetrics(metrics []v1alpha1.Metric, args []v1alpha1.Argument) ([]v1alpha1.Metric, error) {
-	for i, arg := range args {
-		if arg.ValueFrom != nil {
-			if arg.Value != nil {
-				return nil, fmt.Errorf("arg '%s' has both Value and ValueFrom fields", arg.Name)
-			}
-			argVal := "dummy-value"
-			args[i].Value = &argVal
-		}
-	}
-
-	for i, metric := range metrics {
-		resolvedMetric, err := ResolveMetricArgs(metric, args)
-		if err != nil {
-			return nil, err
-		}
-		metrics[i] = *resolvedMetric
-	}
-	return metrics, nil
-}
-
 // ValidateMetrics validates an analysis template spec
 func ValidateMetrics(metrics []v1alpha1.Metric) error {
 	if len(metrics) == 0 {