Skip to content

Commit ff65b39

Browse files
khhiranikhhirani
authored
fix: Modify validation to check Analysis args passed through RO spec (argoproj#1215)
* fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]>
1 parent 849f77e commit ff65b39

File tree

3 files changed

+76
-52
lines changed

3 files changed

+76
-52
lines changed

‎pkg/apis/rollouts/validation/validation_references.go

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ package validation
33
import (
44
"fmt"
55

6+
appsv1 "k8s.io/api/apps/v1"
7+
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
8+
69
analysisutil "github.com/argoproj/argo-rollouts/utils/analysis"
710
"github.com/argoproj/argo-rollouts/utils/conditions"
811
serviceutil "github.com/argoproj/argo-rollouts/utils/service"
@@ -36,6 +39,7 @@ type AnalysisTemplatesWithType struct {
3639
TemplateType AnalysisTemplateType
3740
// CanaryStepIndex only used for InlineAnalysis
3841
CanaryStepIndex int
42+
Args []v1alpha1.AnalysisRunArgument
3943
}
4044

4145
type ServiceType string
@@ -103,14 +107,9 @@ func ValidateAnalysisTemplatesWithType(rollout *v1alpha1.Rollout, templates Anal
103107
return allErrs
104108
}
105109

106-
flattenedTemplate, err := analysisutil.FlattenTemplates(templates.AnalysisTemplates, templates.ClusterAnalysisTemplates)
107110
templateNames := GetAnalysisTemplateNames(templates)
108111
value := fmt.Sprintf("templateNames: %s", templateNames)
109-
if err != nil {
110-
allErrs = append(allErrs, field.Invalid(fldPath, value, err.Error()))
111-
return allErrs
112-
}
113-
err = analysisutil.ResolveArgs(flattenedTemplate.Spec.Args)
112+
_, err := analysisutil.NewAnalysisRunFromTemplates(templates.AnalysisTemplates, templates.ClusterAnalysisTemplates, buildAnalysisArgs(templates.Args, rollout), "", "", "")
114113
if err != nil {
115114
allErrs = append(allErrs, field.Invalid(fldPath, value, err.Error()))
116115
return allErrs
@@ -294,6 +293,24 @@ func GetAnalysisTemplateWithTypeFieldPath(templateType AnalysisTemplateType, can
294293
return fldPath
295294
}
296295

296+
func buildAnalysisArgs(args []v1alpha1.AnalysisRunArgument, r *v1alpha1.Rollout) []v1alpha1.Argument {
297+
stableRSDummy := appsv1.ReplicaSet{
298+
ObjectMeta: v1.ObjectMeta{
299+
Labels: map[string]string{
300+
v1alpha1.DefaultRolloutUniqueLabelKey: "dummy-stable-hash",
301+
},
302+
},
303+
}
304+
newRSDummy := appsv1.ReplicaSet{
305+
ObjectMeta: v1.ObjectMeta{
306+
Labels: map[string]string{
307+
v1alpha1.DefaultRolloutUniqueLabelKey: "dummy-new-hash",
308+
},
309+
},
310+
}
311+
return analysisutil.BuildArgumentsForRolloutAnalysisRun(args, &stableRSDummy, &newRSDummy, r)
312+
}
313+
297314
// validateAnalysisMetrics validates the metrics of an Analysis object
298315
func validateAnalysisMetrics(metrics []v1alpha1.Metric, args []v1alpha1.Argument) ([]v1alpha1.Metric, error) {
299316
for i, arg := range args {

‎pkg/apis/rollouts/validation/validation_references_test.go

Lines changed: 49 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ spec:
7474
host: canary
7575
weight: 0`
7676

77-
func getAnalysisTemplateWithType() AnalysisTemplatesWithType {
77+
func getAnalysisTemplatesWithType() AnalysisTemplatesWithType {
7878
count := intstr.FromInt(1)
7979
return AnalysisTemplatesWithType{
8080
AnalysisTemplates: []*v1alpha1.AnalysisTemplate{{
@@ -160,7 +160,7 @@ func getServiceWithType() ServiceWithType {
160160

161161
func TestValidateRolloutReferencedResources(t *testing.T) {
162162
refResources := ReferencedResources{
163-
AnalysisTemplatesWithType: []AnalysisTemplatesWithType{getAnalysisTemplateWithType()},
163+
AnalysisTemplatesWithType: []AnalysisTemplatesWithType{getAnalysisTemplatesWithType()},
164164
Ingresses: []v1beta1.Ingress{getIngress()},
165165
ServiceWithType: []ServiceWithType{getServiceWithType()},
166166
VirtualServices: nil,
@@ -169,18 +169,49 @@ func TestValidateRolloutReferencedResources(t *testing.T) {
169169
assert.Empty(t, allErrs)
170170
}
171171

172+
func TestValidateAnalysisTemplatesWithType(t *testing.T) {
173+
t.Run("failure - invalid argument", func(t *testing.T) {
174+
rollout := getRollout()
175+
templates := getAnalysisTemplatesWithType()
176+
templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "invalid"})
177+
allErrs := ValidateAnalysisTemplatesWithType(rollout, templates)
178+
assert.Len(t, allErrs, 1)
179+
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")
180+
assert.Equal(t, msg, allErrs[0].Error())
181+
})
182+
183+
t.Run("success", func(t *testing.T) {
184+
rollout := getRollout()
185+
templates := getAnalysisTemplatesWithType()
186+
templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "valid"})
187+
templates.Args = []v1alpha1.AnalysisRunArgument{{Name: "valid", Value: "true"}}
188+
allErrs := ValidateAnalysisTemplatesWithType(rollout, templates)
189+
assert.Empty(t, allErrs)
190+
})
191+
192+
t.Run("failure - duplicate metrics", func(t *testing.T) {
193+
rollout := getRollout()
194+
templates := getAnalysisTemplatesWithType()
195+
templates.AnalysisTemplates[0].Spec.Args = append(templates.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")})
196+
templates.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")}
197+
allErrs := ValidateAnalysisTemplatesWithType(rollout, templates)
198+
assert.Empty(t, allErrs)
199+
})
200+
201+
}
202+
172203
func TestValidateAnalysisTemplateWithType(t *testing.T) {
173204
t.Run("validate analysisTemplate - success", func(t *testing.T) {
174205
rollout := getRollout()
175-
template := getAnalysisTemplateWithType()
176-
allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
206+
templates := getAnalysisTemplatesWithType()
207+
allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex))
177208
assert.Empty(t, allErrs)
178209
})
179210

180211
t.Run("validate inline clusterAnalysisTemplate - failure", func(t *testing.T) {
181212
rollout := getRollout()
182213
count := intstr.FromInt(0)
183-
template := getAnalysisTemplateWithType()
214+
template := getAnalysisTemplatesWithType()
184215
template.ClusterAnalysisTemplates[0].Spec.Metrics[0].Count = &count
185216
allErrs := ValidateAnalysisTemplateWithType(rollout, nil, template.ClusterAnalysisTemplates[0], template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
186217
assert.Len(t, allErrs, 1)
@@ -191,7 +222,7 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) {
191222

192223
t.Run("validate inline analysisTemplate argument - success", func(t *testing.T) {
193224
rollout := getRollout()
194-
template := getAnalysisTemplateWithType()
225+
template := getAnalysisTemplatesWithType()
195226
template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
196227
{
197228
Name: "service-name",
@@ -204,7 +235,7 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) {
204235

205236
t.Run("validate background analysisTemplate - failure", func(t *testing.T) {
206237
rollout := getRollout()
207-
template := getAnalysisTemplateWithType()
238+
template := getAnalysisTemplatesWithType()
208239
template.TemplateType = BackgroundAnalysis
209240
template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
210241
{
@@ -235,23 +266,23 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) {
235266
t.Run("validate background analysisTemplate - success", func(t *testing.T) {
236267
rollout := getRollout()
237268

238-
template := getAnalysisTemplateWithType()
239-
template.TemplateType = BackgroundAnalysis
240-
allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
269+
templates := getAnalysisTemplatesWithType()
270+
templates.TemplateType = BackgroundAnalysis
271+
allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex))
241272
assert.Empty(t, allErrs)
242273

243274
// default value should be fine
244275
defaultValue := "value-name"
245-
template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
276+
templates.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
246277
{
247278
Name: "service-name",
248279
Value: &defaultValue,
249280
},
250281
}
251-
allErrs = ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
282+
allErrs = ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex))
252283
assert.Empty(t, allErrs)
253284

254-
template.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
285+
templates.AnalysisTemplates[0].Spec.Args = []v1alpha1.Argument{
255286
{
256287
Name: "service-name",
257288
Value: pointer.StringPtr("service-name"),
@@ -266,50 +297,22 @@ func TestValidateAnalysisTemplateWithType(t *testing.T) {
266297
},
267298
},
268299
}
269-
allErrs = ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
300+
allErrs = ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex))
270301
assert.Empty(t, allErrs)
271302
})
272303

273304
// verify background analysis does not care about a metric that runs indefinitely
274305
t.Run("validate background analysisTemplate - success", func(t *testing.T) {
275306
rollout := getRollout()
276307
count := intstr.FromInt(0)
277-
template := getAnalysisTemplateWithType()
278-
template.TemplateType = BackgroundAnalysis
279-
template.AnalysisTemplates[0].Spec.Metrics[0].Count = &count
280-
allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
308+
templates := getAnalysisTemplatesWithType()
309+
templates.TemplateType = BackgroundAnalysis
310+
templates.AnalysisTemplates[0].Spec.Metrics[0].Count = &count
311+
allErrs := ValidateAnalysisTemplateWithType(rollout, templates.AnalysisTemplates[0], nil, templates.TemplateType, GetAnalysisTemplateWithTypeFieldPath(templates.TemplateType, templates.CanaryStepIndex))
281312
assert.Empty(t, allErrs)
282313
})
283314
}
284315

285-
func TestValidateAnalysisTemplateWithTypeFlattenMetricsAndResolveArgs(t *testing.T) {
286-
rollout := getRollout()
287-
template := getAnalysisTemplateWithType()
288-
289-
t.Run("failure - invalid argument", func(t *testing.T) {
290-
template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "invalid"})
291-
allErrs := ValidateAnalysisTemplatesWithType(rollout, template)
292-
assert.Len(t, allErrs, 1)
293-
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")
294-
assert.Equal(t, msg, allErrs[0].Error())
295-
})
296-
297-
t.Run("success", func(t *testing.T) {
298-
template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")})
299-
template.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")}
300-
allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
301-
assert.Empty(t, allErrs)
302-
})
303-
304-
t.Run("failure - duplicate metrics", func(t *testing.T) {
305-
template.AnalysisTemplates[0].Spec.Args = append(template.AnalysisTemplates[0].Spec.Args, v1alpha1.Argument{Name: "metric1-name", Value: pointer.StringPtr("true")})
306-
template.AnalysisTemplates[0].Spec.Args[0] = v1alpha1.Argument{Name: "valid", Value: pointer.StringPtr("true")}
307-
allErrs := ValidateAnalysisTemplateWithType(rollout, template.AnalysisTemplates[0], nil, template.TemplateType, GetAnalysisTemplateWithTypeFieldPath(template.TemplateType, template.CanaryStepIndex))
308-
assert.Empty(t, allErrs)
309-
})
310-
311-
}
312-
313316
func TestValidateIngress(t *testing.T) {
314317
t.Run("validate ingress - success", func(t *testing.T) {
315318
ingress := getIngress()

‎rollout/controller.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT
636636
if err != nil {
637637
return nil, err
638638
}
639+
templates.Args = blueGreen.PrePromotionAnalysis.Args
639640
analysisTemplates = append(analysisTemplates, *templates)
640641
}
641642

@@ -645,13 +646,15 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT
645646
if err != nil {
646647
return nil, err
647648
}
649+
templates.Args = blueGreen.PostPromotionAnalysis.Args
648650
analysisTemplates = append(analysisTemplates, *templates)
649651
}
650652
} else if c.rollout.Spec.Strategy.Canary != nil {
651653
canary := c.rollout.Spec.Strategy.Canary
652654
for i, step := range canary.Steps {
653655
if step.Analysis != nil {
654656
templates, err := c.getReferencedAnalysisTemplates(c.rollout, step.Analysis, validation.InlineAnalysis, i)
657+
templates.Args = step.Analysis.Args
655658
if err != nil {
656659
return nil, err
657660
}
@@ -663,6 +666,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT
663666
if err != nil {
664667
return nil, err
665668
}
669+
templates.Args = canary.Analysis.Args
666670
analysisTemplates = append(analysisTemplates, *templates)
667671
}
668672
}

0 commit comments

Comments
 (0)