Skip to content

Commit

Permalink
fix: Modify validation to check Analysis args passed through RO spec (#…
Browse files Browse the repository at this point in the history 
…1215)

* fix: Modify validation to check Analysis args passed through RO spec

Signed-off-by: khhirani <kareena.hirani@gmail.com>
  • Loading branch information
@khhirani
khhirani authored May 25, 2021
1 parent 849f77e commit ff65b39
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 52 deletions.
29 changes: 23 additions & 6 deletions pkg/apis/rollouts/validation/validation_references.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ 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"
serviceutil "github.com/argoproj/argo-rollouts/utils/service"
Expand Down Expand Up @@ -36,6 +39,7 @@ type AnalysisTemplatesWithType struct {
TemplateType AnalysisTemplateType
// CanaryStepIndex only used for InlineAnalysis
CanaryStepIndex int
Args []v1alpha1.AnalysisRunArgument
}

type ServiceType string
Expand Down Expand Up @@ -103,14 +107,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
Expand Down Expand Up @@ -294,6 +293,24 @@ 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)
}

// validateAnalysisMetrics validates the metrics of an Analysis object
func validateAnalysisMetrics(metrics []v1alpha1.Metric, args []v1alpha1.Argument) ([]v1alpha1.Metric, error) {
for i, arg := range args {
Expand Down
95 changes: 49 additions & 46 deletions pkg/apis/rollouts/validation/validation_references_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ spec:
host: canary
weight: 0`

func getAnalysisTemplateWithType() AnalysisTemplatesWithType {
func getAnalysisTemplatesWithType() AnalysisTemplatesWithType {
count := intstr.FromInt(1)
return AnalysisTemplatesWithType{
AnalysisTemplates: []*v1alpha1.AnalysisTemplate{{
Expand Down Expand Up @@ -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,
Expand All @@ -169,18 +169,49 @@ func TestValidateRolloutReferencedResources(t *testing.T) {
assert.Empty(t, allErrs)
}

func TestValidateAnalysisTemplatesWithType(t *testing.T) {
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)
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) {
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)
assert.Empty(t, allErrs)
})

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)
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)
Expand All @@ -191,7 +222,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",
Expand All @@ -204,7 +235,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{
{
Expand Down Expand Up @@ -235,23 +266,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"),
Expand All @@ -266,50 +297,22 @@ 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)
})

// 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.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()
Expand Down
4 changes: 4 additions & 0 deletions rollout/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -645,13 +646,15 @@ 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 {
canary := c.rollout.Spec.Strategy.Canary
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
}
Expand All @@ -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)
}
}
Expand Down

0 comments on commit ff65b39

Please sign in to comment.