fix: verify analysis arguments name with those in the rollout (#1071)

* fix: verify analysis arguments name with those in the rollout - return error if arguments do not match between rollout spec and analysis template Signed-off-by: Hui Kang <[email protected]> * skip check for arg with default value Signed-off-by: Hui Kang <[email protected]> Co-authored-by: Hui Kang <[email protected]>
argoproj · Apr 14, 2021 · 74e1f0f · 74e1f0f
1 parent d9899f8
commit 74e1f0f
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 6 deletions.
diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go
@@ -65,7 +65,7 @@ func ValidateRolloutReferencedResources(rollout *v1alpha1.Rollout, referencedRes
 		allErrs = append(allErrs, ValidateService(service, rollout)...)
 	}
 	for _, template := range referencedResources.AnalysisTemplateWithType {
-		allErrs = append(allErrs, ValidateAnalysisTemplateWithType(template)...)
+		allErrs = append(allErrs, ValidateAnalysisTemplateWithType(rollout, template)...)
 	}
 	for _, ingress := range referencedResources.Ingresses {
 		allErrs = append(allErrs, ValidateIngress(rollout, ingress)...)
@@ -92,7 +92,7 @@ func ValidateService(svc ServiceWithType, rollout *v1alpha1.Rollout) field.Error
 	return allErrs
 }
 
-func ValidateAnalysisTemplateWithType(template AnalysisTemplateWithType) field.ErrorList {
+func ValidateAnalysisTemplateWithType(rollout *v1alpha1.Rollout, template AnalysisTemplateWithType) field.ErrorList {
 	allErrs := field.ErrorList{}
 	fldPath := GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.AnalysisIndex, template.CanaryStepIndex)
 	if fldPath == nil {
@@ -121,6 +121,27 @@ func ValidateAnalysisTemplateWithType(template AnalysisTemplateWithType) field.E
 				}
 			}
 		}
+	} else if template.TemplateType == BackgroundAnalysis && len(templateSpec.Args) > 0 {
+		for _, arg := range templateSpec.Args {
+			if arg.Value != nil || arg.ValueFrom != nil {
+				continue
+			}
+			if rollout.Spec.Strategy.Canary == nil || rollout.Spec.Strategy.Canary.Analysis == nil || rollout.Spec.Strategy.Canary.Analysis.Args == nil {
+				allErrs = append(allErrs, field.Invalid(fldPath, templateName, "missing analysis arguments in rollout spec"))
+				continue
+			}
+
+			foundArg := false
+			for _, rolloutArg := range rollout.Spec.Strategy.Canary.Analysis.Args {
+				if arg.Name == rolloutArg.Name {
+					foundArg = true
+					break
+				}
+			}
+			if !foundArg {
+				allErrs = append(allErrs, field.Invalid(fldPath, templateName, arg.Name))
+			}
+		}
 	}
 	return allErrs
 }

diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go
@@ -157,40 +157,111 @@ func TestValidateRolloutReferencedResources(t *testing.T) {
 
 func TestValidateAnalysisTemplateWithType(t *testing.T) {
 	t.Run("validate analysisTemplate - success", func(t *testing.T) {
+		rollout := getRollout()
 		template := getAnalysisTemplateWithType()
-		allErrs := ValidateAnalysisTemplateWithType(template)
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
 		assert.Empty(t, allErrs)
 	})
 
 	t.Run("validate inline analysisTemplate - failure", func(t *testing.T) {
+		rollout := getRollout()
 		count := intstr.FromInt(0)
 		template := getAnalysisTemplateWithType()
 		template.AnalysisTemplate.Spec.Metrics[0].Count = &count
-		allErrs := ValidateAnalysisTemplateWithType(template)
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
 		assert.Len(t, allErrs, 1)
 		msg := fmt.Sprintf("AnalysisTemplate %s has metric %s which runs indefinitely. Invalid value for count: %s", "analysis-template-name", "metric-name", count.String())
 		expectedError := field.Invalid(GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.AnalysisIndex, template.CanaryStepIndex), template.AnalysisTemplate.Name, msg)
 		assert.Equal(t, expectedError.Error(), allErrs[0].Error())
 	})
 
 	t.Run("validate inline analysisTemplate argument - success", func(t *testing.T) {
+		rollout := getRollout()
 		template := getAnalysisTemplateWithType()
 		template.AnalysisTemplate.Spec.Args = []v1alpha1.Argument{
 			{
 				Name: "service-name",
 			},
 		}
-		allErrs := ValidateAnalysisTemplateWithType(template)
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
+		assert.Empty(t, allErrs)
+	})
+
+	t.Run("validate background analysisTemplate - failure", func(t *testing.T) {
+		rollout := getRollout()
+		template := getAnalysisTemplateWithType()
+		template.TemplateType = BackgroundAnalysis
+		template.AnalysisTemplate.Spec.Args = []v1alpha1.Argument{
+			{
+				Name: "service-name",
+			},
+		}
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
+		assert.NotEmpty(t, allErrs)
+
+		rollout.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
+			RolloutAnalysis: v1alpha1.RolloutAnalysis{
+				Args: []v1alpha1.AnalysisRunArgument{
+					{
+						Name: "a-different-service-name",
+					},
+				},
+			},
+		}
+		allErrs = ValidateAnalysisTemplateWithType(rollout, template)
+		assert.NotEmpty(t, allErrs)
+
+		template.AnalysisTemplate.Spec.Args = append(template.AnalysisTemplate.Spec.Args, v1alpha1.Argument{Name: "second-service-name"})
+		allErrs = ValidateAnalysisTemplateWithType(rollout, template)
+		assert.NotEmpty(t, allErrs)
+	})
+
+	// verify background analysis matches the arguments in rollout spec
+	t.Run("validate background analysisTemplate - success", func(t *testing.T) {
+		rollout := getRollout()
+
+		template := getAnalysisTemplateWithType()
+		template.TemplateType = BackgroundAnalysis
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
+		assert.Empty(t, allErrs)
+
+		// default value should be fine
+		defaultValue := "value-name"
+		template.AnalysisTemplate.Spec.Args = []v1alpha1.Argument{
+			{
+				Name:  "service-name",
+				Value: &defaultValue,
+			},
+		}
+		allErrs = ValidateAnalysisTemplateWithType(rollout, template)
+		assert.Empty(t, allErrs)
+
+		template.AnalysisTemplate.Spec.Args = []v1alpha1.Argument{
+			{
+				Name: "service-name",
+			},
+		}
+		rollout.Spec.Strategy.Canary.Analysis = &v1alpha1.RolloutAnalysisBackground{
+			RolloutAnalysis: v1alpha1.RolloutAnalysis{
+				Args: []v1alpha1.AnalysisRunArgument{
+					{
+						Name: "service-name",
+					},
+				},
+			},
+		}
+		allErrs = ValidateAnalysisTemplateWithType(rollout, template)
 		assert.Empty(t, allErrs)
 	})
 
 	// verify background analysis does not care about a metric that runs indefinitely
 	t.Run("validate background analysisTemplate - success", func(t *testing.T) {
+		rollout := getRollout()
 		count := intstr.FromInt(0)
 		template := getAnalysisTemplateWithType()
 		template.TemplateType = BackgroundAnalysis
 		template.AnalysisTemplate.Spec.Metrics[0].Count = &count
-		allErrs := ValidateAnalysisTemplateWithType(template)
+		allErrs := ValidateAnalysisTemplateWithType(rollout, template)
 		assert.Empty(t, allErrs)
 	})
 }