Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
May 25, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c5a0b0e
fix: Fix validation for ValueFrom in AnalysisTemplate
khhirani May 21, 2021
08acecd
Merge branch 'master' of https://github.com/argoproj/argo-rollouts in…
khhirani May 22, 2021
57c7cff
Only resolve Analysis metrics in validation_references.go
khhirani May 22, 2021
9f6869a
Modify tests
khhirani May 22, 2021
bf51479
Make lint
khhirani May 22, 2021
446ffb2
Move tests
khhirani May 22, 2021
2e13c8a
Create test
khhirani May 22, 2021
465c8b1
Remove extraneous test
khhirani May 22, 2021
8e308b9
fix: Modify validation to check Analysis args passed through RO spec
khhirani May 22, 2021
5a2cc9b
make lint
khhirani May 22, 2021
f63126f
Create e2e test
khhirani May 24, 2021
8edb7c9
Merge branch 'fix-valuefrom-analysistemplate' of https://github.com/k…
khhirani May 24, 2021
c336bd3
Modify tests
khhirani May 24, 2021
c45d05a
Fix tests
khhirani May 24, 2021
4361809
Merge branch 'master' of https://github.com/argoproj/argo-rollouts in…
khhirani May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions pkg/apis/rollouts/validation/validation_references.go
View file
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems this method can be removed as this is the only place to use it.

_, 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
Open in desktop
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
Open in desktop
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
Copy link
Member

@jessesuen jessesuen May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Are you modifying the AnalysisTemplate which was pulled off the informer store (lister)? Objects received form the store should not be modified. See:

https://github.com/kubernetes/sample-controller/blob/b8d9e8c247129e53962d0dcfc08a4e8b47477318/controller.go#L322-L324

  1. It seems we are clobbering the entire arguments of the template with the Arguments of the Rollout. Does this change support default values (i.e. some arguments values are in the AnalysisTemplate, but others are supplied by the rollout).

Copy link
Contributor Author

@khhirani khhirani May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we are clobbering the entire arguments of the template with the Arguments of the Rollout.

Can you elaborate on this? I thought we did this anyway when we created the AnalysisRun from the templates (i.e. combine the templates’ args with the AnalysisRunArguments passed through the RO spec) in the function NewAnalysisRunFromTemplates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you modifying the AnalysisTemplate which was pulled off the informer store (lister)?

I don't believe so. I'm copying the AnalysisRunArguments from the Rollout Spec into a AnalysisTemplatesWithType object, which is a struct I created for validation. I pass the templates and the args from the RO spec into NewAnalysisRunFromTemplates for validation, which doesn't seem to modify the templates.

Copy link
Member

@jessesuen jessesuen May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this?

Can you confirm that this is the behavior?

kind: AnalysisTemplate
...
args:
- name: aaa
  value: foo     # default value
- name: bbb  # mandatory supplied value
- name: ccc
  value: baz

Args supplied by rollout (note that aaa is omitted)

kind: Rollout
...
args:
- name: bbb
  value: "11111"
- name: ccc
  value: "2222"

The AnalysisRun should be created with

kind: AnalysisRun
args:
- name: aaa
  value: foo
- name: bbb
  value: "11111"
- name: ccc
  value: "2222"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so. I'm copying the AnalysisRunArguments from the Rollout Spec into a AnalysisTemplatesWithType object, which is a struct I created for validation

I took a closer look. You're right. I mistakenly thought it was returning AnalysisTemplates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jessesuen Can I merge this in then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first concern is not valid, but can you answer my second concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jessesuen Yes, I confirm that the behavior in this comment is what happens.

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