fix: AnalysisRun args could not be resolved from secret

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) {

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,39 @@ 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,
+	}}
+
+	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())
+}

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 {

utils/analysis/factory_test.go

@@ -425,39 +425,3 @@ func TestResolveMetricArgsWithQuotes(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, fmt.Sprintf(arg), newMetric.SuccessCondition)
 }
-
-func TestResolveMetrics(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,
-	}}
-
-	resolvedMetrics, err := ResolveMetrics(metrics, args)
-	assert.Nil(t, err)
-	assert.Equal(t, count, resolvedMetrics[0].Count.String())
-	assert.Equal(t, failureLimit, resolvedMetrics[0].FailureLimit.String())
-}