From c5a0b0efd51d043b4af965fc8023c7a2a8a74b97 Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 16:39:02 -0700 Subject: [PATCH 01/12] fix: Fix validation for ValueFrom in AnalysisTemplate Signed-off-by: khhirani --- pkg/apis/rollouts/validation/validation_references.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 2ce52b9ffe..dc7c238bd3 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -138,6 +138,7 @@ func ValidateAnalysisTemplateWithType(rollout *v1alpha1.Rollout, template *v1alp } if templateType != BackgroundAnalysis { + templateSpec = *templateSpec.DeepCopy() setArgValuePlaceHolder(templateSpec.Args) resolvedMetrics, err := analysisutil.ResolveMetrics(templateSpec.Metrics, templateSpec.Args) if err != nil { From 57c7cffde695bb5dc9fe7c524de7812e5a92639b Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 19:39:17 -0700 Subject: [PATCH 02/12] Only resolve Analysis metrics in validation_references.go Signed-off-by: khhirani --- analysis/analysis.go | 13 +--- .../validation/validation_references.go | 27 ++++++- utils/analysis/factory.go | 26 +------ utils/analysis/factory_test.go | 70 +++++++++---------- 4 files changed, 62 insertions(+), 74 deletions(-) diff --git a/analysis/analysis.go b/analysis/analysis.go index ea0944ce68..265406ef60 100644 --- a/analysis/analysis.go +++ b/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) diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index dc7c238bd3..923e73cd91 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -2,7 +2,6 @@ package validation import ( "fmt" - analysisutil "github.com/argoproj/argo-rollouts/utils/analysis" "github.com/argoproj/argo-rollouts/utils/conditions" serviceutil "github.com/argoproj/argo-rollouts/utils/service" @@ -138,9 +137,8 @@ func ValidateAnalysisTemplateWithType(rollout *v1alpha1.Rollout, template *v1alp } if templateType != BackgroundAnalysis { - templateSpec = *templateSpec.DeepCopy() 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)) @@ -294,3 +292,26 @@ 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 +} + diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index 84e9dd29a0..1eb2fb5bcf 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -3,9 +3,8 @@ package analysis import ( "encoding/json" "fmt" - "strconv" - templateutil "github.com/argoproj/argo-rollouts/utils/template" + "strconv" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" appsv1 "k8s.io/api/apps/v1" @@ -96,7 +95,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 +116,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 { diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index d1d3bd5507..0b592703ba 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -426,38 +426,38 @@ func TestResolveMetricArgsWithQuotes(t *testing.T) { 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()) -} +//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()) +//} From 9f6869a45caca8ce692dcf026efec4945105f266 Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 20:09:28 -0700 Subject: [PATCH 03/12] Modify tests Signed-off-by: khhirani --- analysis/analysis_test.go | 80 ++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 35 deletions(-) diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index 3fd3bcd448..8b025c2207 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -1087,41 +1087,51 @@ 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) -} +//func TestSecretContentReferenceValueFromError(t *testing.T) { +// secret := &corev1.Secret{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "web-metric-secret", +// Namespace: metav1.NamespaceDefault, +// }, +// Data: map[string][]byte{ +// "apikey": []byte("12345"), +// }, +// } +// f := newFixture(t) +// defer f.Close() +// c, _, _ := f.newController(noResyncPeriodFunc) +// f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) +// 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) { From bf514796e92c76b8f7205c4619ba8764248b7f1c Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 20:17:29 -0700 Subject: [PATCH 04/12] Make lint Signed-off-by: khhirani --- analysis/analysis_test.go | 94 ++++++++++--------- .../validation/validation_references.go | 2 +- utils/analysis/factory.go | 3 +- 3 files changed, 52 insertions(+), 47 deletions(-) diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index 8b025c2207..641ede2144 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -1087,51 +1087,54 @@ 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) { -// secret := &corev1.Secret{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "web-metric-secret", -// Namespace: metav1.NamespaceDefault, -// }, -// Data: map[string][]byte{ -// "apikey": []byte("12345"), -// }, -// } -// f := newFixture(t) -// defer f.Close() -// c, _, _ := f.newController(noResyncPeriodFunc) -// f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) -// 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) -//} +func TestSecretContentReferenceValueFromError(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "web-metric-secret", + Namespace: metav1.NamespaceDefault, + }, + Data: map[string][]byte{ + "apikey": []byte("12345"), + }, + } + f := newFixture(t) + defer f.Close() + c, _, _ := f.newController(noResyncPeriodFunc) + f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) + argName := "apikey" + argVal := "value" + run := &v1alpha1.AnalysisRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: metav1.NamespaceDefault, + }, + 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) { @@ -1148,6 +1151,7 @@ func TestSecretContentReferenceSuccess(t *testing.T) { defer f.Close() c, _, _ := f.newController(noResyncPeriodFunc) f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) + argName := "apikey" run := &v1alpha1.AnalysisRun{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 923e73cd91..60cb8ff48e 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -2,6 +2,7 @@ package validation import ( "fmt" + analysisutil "github.com/argoproj/argo-rollouts/utils/analysis" "github.com/argoproj/argo-rollouts/utils/conditions" serviceutil "github.com/argoproj/argo-rollouts/utils/service" @@ -314,4 +315,3 @@ func validateAnalysisMetrics(metrics []v1alpha1.Metric, args []v1alpha1.Argument } return metrics, nil } - diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index 1eb2fb5bcf..6b5e9ec448 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -3,9 +3,10 @@ package analysis import ( "encoding/json" "fmt" - templateutil "github.com/argoproj/argo-rollouts/utils/template" "strconv" + templateutil "github.com/argoproj/argo-rollouts/utils/template" + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" appsv1 "k8s.io/api/apps/v1" "k8s.io/kubernetes/pkg/fieldpath" From 446ffb2756cf44000aaa932b2d8d67ed85092aa1 Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 20:20:27 -0700 Subject: [PATCH 05/12] Move tests Signed-off-by: khhirani --- .../validation/validation_references_test.go | 36 +++++++++++++++++++ utils/analysis/factory_test.go | 36 ------------------- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go index 38c3a09ef8..570ce7c765 100644 --- a/pkg/apis/rollouts/validation/validation_references_test.go +++ b/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()) +} diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index 0b592703ba..02ca4ecace 100644 --- a/utils/analysis/factory_test.go +++ b/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()) -//} From 2e13c8af9fef2ef57eb09fa5a3033dadb9e57a83 Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 20:33:23 -0700 Subject: [PATCH 06/12] Create test Signed-off-by: khhirani --- analysis/analysis_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index 641ede2144..a370011f9f 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -1367,6 +1367,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) { From 465c8b1a1c1353bd60b55e0c70f5adb75a964066 Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 21:17:12 -0700 Subject: [PATCH 07/12] Remove extraneous test Signed-off-by: khhirani --- analysis/analysis_test.go | 50 --------------------------------------- 1 file changed, 50 deletions(-) diff --git a/analysis/analysis_test.go b/analysis/analysis_test.go index a370011f9f..c901984e35 100644 --- a/analysis/analysis_test.go +++ b/analysis/analysis_test.go @@ -1087,55 +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) { - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "web-metric-secret", - Namespace: metav1.NamespaceDefault, - }, - Data: map[string][]byte{ - "apikey": []byte("12345"), - }, - } - f := newFixture(t) - defer f.Close() - c, _, _ := f.newController(noResyncPeriodFunc) - f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) - argName := "apikey" - argVal := "value" - run := &v1alpha1.AnalysisRun{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: metav1.NamespaceDefault, - }, - 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) @@ -1151,7 +1102,6 @@ func TestSecretContentReferenceSuccess(t *testing.T) { defer f.Close() c, _, _ := f.newController(noResyncPeriodFunc) f.kubeclient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.TODO(), secret, metav1.CreateOptions{}) - argName := "apikey" run := &v1alpha1.AnalysisRun{ ObjectMeta: metav1.ObjectMeta{ From 8e308b9de045eb494a423782c31ea208caa9feda Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 22:57:14 -0700 Subject: [PATCH 08/12] fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani --- .../validation/validation_references.go | 28 +++++++++++++++---- rollout/controller.go | 4 +++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 2ce52b9ffe..8c2c6339d6 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -2,6 +2,8 @@ package validation import ( "fmt" + appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" analysisutil "github.com/argoproj/argo-rollouts/utils/analysis" "github.com/argoproj/argo-rollouts/utils/conditions" @@ -36,6 +38,7 @@ type AnalysisTemplatesWithType struct { TemplateType AnalysisTemplateType // CanaryStepIndex only used for InlineAnalysis CanaryStepIndex int + Args []v1alpha1.AnalysisRunArgument } type ServiceType string @@ -103,14 +106,9 @@ func ValidateAnalysisTemplatesWithType(rollout *v1alpha1.Rollout, templates Anal return allErrs } - flattenedTemplate, err := analysisutil.FlattenTemplates(templates.AnalysisTemplates, templates.ClusterAnalysisTemplates) templateNames := GetAnalysisTemplateNames(templates) value := fmt.Sprintf("templateNames: %s", templateNames) - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath, value, err.Error())) - return allErrs - } - err = analysisutil.ResolveArgs(flattenedTemplate.Spec.Args) + _, err := analysisutil.NewAnalysisRunFromTemplates(templates.AnalysisTemplates, templates.ClusterAnalysisTemplates, buildAnalysisArgs(templates.Args, rollout), "", "", "") if err != nil { allErrs = append(allErrs, field.Invalid(fldPath, value, err.Error())) return allErrs @@ -293,3 +291,21 @@ func GetAnalysisTemplateWithTypeFieldPath(templateType AnalysisTemplateType, can } return fldPath } + +func buildAnalysisArgs(args []v1alpha1.AnalysisRunArgument, r *v1alpha1.Rollout) []v1alpha1.Argument { + stableRSDummy := appsv1.ReplicaSet{ + ObjectMeta: v1.ObjectMeta{ + Labels: map[string]string{ + v1alpha1.DefaultRolloutUniqueLabelKey: "dummy-stable-hash", + }, + }, + } + newRSDummy := appsv1.ReplicaSet{ + ObjectMeta: v1.ObjectMeta{ + Labels: map[string]string{ + v1alpha1.DefaultRolloutUniqueLabelKey: "dummy-new-hash", + }, + }, + } + return analysisutil.BuildArgumentsForRolloutAnalysisRun(args, &stableRSDummy, &newRSDummy, r) +} diff --git a/rollout/controller.go b/rollout/controller.go index 7de5a36e5b..937620fe86 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -636,6 +636,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT if err != nil { return nil, err } + templates.Args = blueGreen.PrePromotionAnalysis.Args analysisTemplates = append(analysisTemplates, *templates) } @@ -645,6 +646,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT if err != nil { return nil, err } + templates.Args = blueGreen.PostPromotionAnalysis.Args analysisTemplates = append(analysisTemplates, *templates) } } else if c.rollout.Spec.Strategy.Canary != nil { @@ -652,6 +654,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT for i, step := range canary.Steps { if step.Analysis != nil { templates, err := c.getReferencedAnalysisTemplates(c.rollout, step.Analysis, validation.InlineAnalysis, i) + templates.Args = step.Analysis.Args if err != nil { return nil, err } @@ -663,6 +666,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT if err != nil { return nil, err } + templates.Args = canary.Analysis.Args analysisTemplates = append(analysisTemplates, *templates) } } From 5a2cc9b95d5b0aeb81bc98756aa121953dc0699a Mon Sep 17 00:00:00 2001 From: khhirani Date: Fri, 21 May 2021 23:00:41 -0700 Subject: [PATCH 09/12] make lint Signed-off-by: khhirani --- pkg/apis/rollouts/validation/validation_references.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index 8c2c6339d6..669ca4e0dc 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -2,6 +2,7 @@ package validation import ( "fmt" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,7 +39,7 @@ type AnalysisTemplatesWithType struct { TemplateType AnalysisTemplateType // CanaryStepIndex only used for InlineAnalysis CanaryStepIndex int - Args []v1alpha1.AnalysisRunArgument + Args []v1alpha1.AnalysisRunArgument } type ServiceType string From f63126fe2b37e13258457f6b02e700a14257102d Mon Sep 17 00:00:00 2001 From: khhirani Date: Mon, 24 May 2021 13:59:20 -0700 Subject: [PATCH 10/12] Create e2e test Signed-off-by: khhirani --- examples/rollout-secret.yaml | 20 +++---- .../validation/validation_references_test.go | 18 ++++-- test/e2e/analysis_test.go | 21 +++++++ test/e2e/functional/rollout-secret.yaml | 56 +++++++++++++++++++ 4 files changed, 99 insertions(+), 16 deletions(-) create mode 100644 test/e2e/functional/rollout-secret.yaml diff --git a/examples/rollout-secret.yaml b/examples/rollout-secret.yaml index 87d59ceb24..f6b3094289 100644 --- a/examples/rollout-secret.yaml +++ b/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,11 +36,10 @@ 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 @@ -48,19 +47,16 @@ 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}" diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go index 570ce7c765..c969691e3e 100644 --- a/pkg/apis/rollouts/validation/validation_references_test.go +++ b/pkg/apis/rollouts/validation/validation_references_test.go @@ -528,8 +528,18 @@ func TestValidateAnalysisMetrics(t *testing.T) { 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()) + 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()) + + }) } diff --git a/test/e2e/analysis_test.go b/test/e2e/analysis_test.go index 12655c3652..42e3e2135e 100644 --- a/test/e2e/analysis_test.go +++ b/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")) +} diff --git a/test/e2e/functional/rollout-secret.yaml b/test/e2e/functional/rollout-secret.yaml new file mode 100644 index 0000000000..42db8000f2 --- /dev/null +++ b/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}" From c336bd3f253deabb2bfcdaadc2e1cc9eec2cfe40 Mon Sep 17 00:00:00 2001 From: khhirani Date: Mon, 24 May 2021 15:27:18 -0700 Subject: [PATCH 11/12] Modify tests Signed-off-by: khhirani --- .../validation/validation_references_test.go | 92 +++++++++---------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go index c969691e3e..baedefbd10 100644 --- a/pkg/apis/rollouts/validation/validation_references_test.go +++ b/pkg/apis/rollouts/validation/validation_references_test.go @@ -74,7 +74,7 @@ spec: host: canary weight: 0` -func getAnalysisTemplateWithType() AnalysisTemplatesWithType { +func getAnalysisTemplatesWithType() AnalysisTemplatesWithType { count := intstr.FromInt(1) return AnalysisTemplatesWithType{ AnalysisTemplates: []*v1alpha1.AnalysisTemplate{{ @@ -160,7 +160,7 @@ func getServiceWithType() ServiceWithType { func TestValidateRolloutReferencedResources(t *testing.T) { refResources := ReferencedResources{ - AnalysisTemplatesWithType: []AnalysisTemplatesWithType{getAnalysisTemplateWithType()}, + AnalysisTemplatesWithType: []AnalysisTemplatesWithType{getAnalysisTemplatesWithType()}, Ingresses: []v1beta1.Ingress{getIngress()}, ServiceWithType: []ServiceWithType{getServiceWithType()}, VirtualServices: nil, @@ -169,18 +169,46 @@ func TestValidateRolloutReferencedResources(t *testing.T) { assert.Empty(t, allErrs) } +func TestValidateAnalysisTemplatesWithType(t *testing.T) { + rollout := getRollout() + templates := getAnalysisTemplatesWithType() + + t.Run("failure - invalid argument", func(t *testing.T) { + templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "invalid"}) + allErrs := ValidateAnalysisTemplatesWithType(rollout, templates) + assert.Len(t, allErrs, 1) + msg := fmt.Sprintf("spec.strategy.canary.steps[0].analysis.templates: Invalid value: \"templateNames: [analysis-template-name cluster-analysis-template-name]\": args.invalid was not resolved") + assert.Equal(t, msg, allErrs[0].Error()) + }) + + t.Run("success", func(t *testing.T) { + templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "valid"}) + templates.Args = []v1alpha1.AnalysisRunArgument{{Name: "valid", Value: "true"}} + allErrs := ValidateAnalysisTemplatesWithType(rollout, templates) + assert.Empty(t, allErrs) + }) + + t.Run("failure - duplicate metrics", func(t *testing.T) { + templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")}) + templates.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")} + allErrs := ValidateAnalysisTemplatesWithType(rollout, templates) + assert.Empty(t, allErrs) + }) + +} + func TestValidateAnalysisTemplateWithType(t *testing.T) { t.Run("validate analysisTemplate - success", func(t *testing.T) { rollout := getRollout() - template := getAnalysisTemplateWithType() - allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) + templates := getAnalysisTemplatesWithType() + allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex)) assert.Empty(t, allErrs) }) t.Run("validate inline clusterAnalysisTemplate - failure", func(t *testing.T) { rollout := getRollout() count := intstr.FromInt(0) - template := getAnalysisTemplateWithType() + template := getAnalysisTemplatesWithType() template.ClusterAnalysisTemplates[0].Spec.Metrics[0].Count = &count allErrs := ValidateAnalysisTemplateWithType(rollout, nil, template.ClusterAnalysisTemplates[0], template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) assert.Len(t, allErrs, 1) @@ -191,7 +219,7 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) { t.Run("validate inline analysisTemplate argument - success", func(t *testing.T) { rollout := getRollout() - template := getAnalysisTemplateWithType() + template := getAnalysisTemplatesWithType() template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ { Name: "service-name", @@ -204,7 +232,7 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) { t.Run("validate background analysisTemplate - failure", func(t *testing.T) { rollout := getRollout() - template := getAnalysisTemplateWithType() + template := getAnalysisTemplatesWithType() template.TemplateType = BackgroundAnalysis template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ { @@ -235,23 +263,23 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) { t.Run("validate background analysisTemplate - success", func(t *testing.T) { rollout := getRollout() - template := getAnalysisTemplateWithType() - template.TemplateType = BackgroundAnalysis - allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) + templates := getAnalysisTemplatesWithType() + templates.TemplateType = BackgroundAnalysis + allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex)) assert.Empty(t, allErrs) // default value should be fine defaultValue := "value-name" - template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ + templates.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ { Name: "service-name", Value: &defaultValue, }, } - allErrs = ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) + allErrs = ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex)) assert.Empty(t, allErrs) - template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ + templates.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{ { Name: "service-name", Value: pointer.StringPtr("service-name"), @@ -266,7 +294,7 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) { }, }, } - allErrs = ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) + allErrs = ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex)) assert.Empty(t, allErrs) }) @@ -274,42 +302,14 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) { t.Run("validate background analysisTemplate - success", func(t *testing.T) { rollout := getRollout() count := intstr.FromInt(0) - template := getAnalysisTemplateWithType() - template.TemplateType = BackgroundAnalysis - template.AnalysisTemplates[0].Spec.Metrics[0].Count = &count - allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) + templates := getAnalysisTemplatesWithType() + templates.TemplateType = BackgroundAnalysis + templates.AnalysisTemplates[0].Spec.Metrics[0].Count = &count + allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex)) assert.Empty(t, allErrs) }) } -func TestValidateAnalysisTemplateWithTypeFlattenMetricsAndResolveArgs(t *testing.T) { - rollout := getRollout() - template := getAnalysisTemplateWithType() - - t.Run("failure - invalid argument", func(t *testing.T) { - template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "invalid"}) - allErrs := ValidateAnalysisTemplatesWithType(rollout, template) - assert.Len(t, allErrs, 1) - msg := fmt.Sprintf("spec.strategy.canary.steps[0].analysis.templates: Invalid value: \"templateNames: [analysis-template-name cluster-analysis-template-name]\": args.invalid was not resolved") - assert.Equal(t, msg, allErrs[0].Error()) - }) - - t.Run("success", func(t *testing.T) { - template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")}) - template.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")} - allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) - assert.Empty(t, allErrs) - }) - - t.Run("failure - duplicate metrics", func(t *testing.T) { - template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")}) - template.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")} - allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex)) - assert.Empty(t, allErrs) - }) - -} - func TestValidateIngress(t *testing.T) { t.Run("validate ingress - success", func(t *testing.T) { ingress := getIngress() From c45d05ab336bfd5132a8178b053373381876d6c0 Mon Sep 17 00:00:00 2001 From: khhirani Date: Mon, 24 May 2021 15:45:08 -0700 Subject: [PATCH 12/12] Fix tests Signed-off-by: khhirani --- .../rollouts/validation/validation_references_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go index baedefbd10..734db40ba4 100644 --- a/pkg/apis/rollouts/validation/validation_references_test.go +++ b/pkg/apis/rollouts/validation/validation_references_test.go @@ -170,10 +170,9 @@ func TestValidateRolloutReferencedResources(t *testing.T) { } func TestValidateAnalysisTemplatesWithType(t *testing.T) { - rollout := getRollout() - templates := getAnalysisTemplatesWithType() - t.Run("failure - invalid argument", func(t *testing.T) { + rollout := getRollout() + templates := getAnalysisTemplatesWithType() templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "invalid"}) allErrs := ValidateAnalysisTemplatesWithType(rollout, templates) assert.Len(t, allErrs, 1) @@ -182,6 +181,8 @@ func TestValidateAnalysisTemplatesWithType(t *testing.T) { }) t.Run("success", func(t *testing.T) { + rollout := getRollout() + templates := getAnalysisTemplatesWithType() templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "valid"}) templates.Args = []v1alpha1.AnalysisRunArgument{{Name: "valid", Value: "true"}} allErrs := ValidateAnalysisTemplatesWithType(rollout, templates) @@ -189,6 +190,8 @@ func TestValidateAnalysisTemplatesWithType(t *testing.T) { }) t.Run("failure - duplicate metrics", func(t *testing.T) { + rollout := getRollout() + templates := getAnalysisTemplatesWithType() templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")}) templates.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")} allErrs := ValidateAnalysisTemplatesWithType(rollout, templates)