From 9fd2b58b8eab17c39bc00a473bdf5bbb5bbba231 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:01 +0200 Subject: [PATCH 01/12] use objx to read value from Rollout manifest Signed-off-by: Noam Gal --- go.mod | 1 + go.sum | 3 ++- utils/analysis/factory.go | 20 ++++++++++++-------- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 4e5eb688da..b72e56e7e8 100644 --- a/go.mod +++ b/go.mod @@ -32,6 +32,7 @@ require ( github.com/soheilhy/cmux v0.1.4 github.com/spaceapegames/go-wavefront v1.8.1 github.com/spf13/cobra v1.1.3 + github.com/stretchr/objx v0.3.0 // indirect github.com/stretchr/testify v1.7.0 github.com/tj/assert v0.0.3 github.com/undefinedlabs/go-mpatch v1.0.6 diff --git a/go.sum b/go.sum index 3784ded5aa..e17eea529b 100644 --- a/go.sum +++ b/go.sum @@ -1093,8 +1093,9 @@ github.com/streadway/amqp v0.0.0-20190827072141-edfb9018d271/go.mod h1:AZpEONHx3 github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= +github.com/stretchr/objx v0.3.0 h1:NGXK3lHquSN08v5vWalVI/L8XU9hdzE/G6xsrze47As= +github.com/stretchr/objx v0.3.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/testify v0.0.0-20161117074351-18a02ba4a312/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index 90b1a7de1f..a3f6cd713d 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -5,11 +5,11 @@ import ( "fmt" "strconv" + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" templateutil "github.com/argoproj/argo-rollouts/utils/template" - "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/stretchr/objx" appsv1 "k8s.io/api/apps/v1" - "k8s.io/kubernetes/pkg/fieldpath" ) // BuildArgumentsForRolloutAnalysisRun builds the arguments for a analysis base created by a rollout @@ -26,20 +26,18 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st case v1alpha1.Stable: value = stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } - } else { - if arg.ValueFrom.FieldRef != nil { - value, _ = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) - } + } else if arg.ValueFrom.FieldRef != nil { + value = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) } - } + analysisArg := v1alpha1.Argument{ Name: arg.Name, Value: &value, } arguments = append(arguments, analysisArg) - } + return arguments } @@ -217,3 +215,9 @@ func ValidateMetric(metric v1alpha1.Metric) error { } return nil } + +func extractValueFromRollout(r *v1alpha1.Rollout, path string) string { + j, _ := json.Marshal(r) + m := objx.MustFromJSON(string(j)) + return m.Get(path).String() +} From f0e3ec28b3fd5bd749491e9fe451538d6bcf71cd Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:01 +0200 Subject: [PATCH 02/12] handle `[]` annotation correcly in BuildArgumentsForRolloutAnalysisRun Signed-off-by: Noam Gal --- utils/analysis/factory.go | 6 ++- utils/analysis/factory_test.go | 79 +++++++++++++++++++++++++++++++--- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index a3f6cd713d..507e9f5ee1 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/objx" appsv1 "k8s.io/api/apps/v1" + "k8s.io/kubernetes/pkg/fieldpath" ) // BuildArgumentsForRolloutAnalysisRun builds the arguments for a analysis base created by a rollout @@ -27,7 +28,10 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st value = stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } } else if arg.ValueFrom.FieldRef != nil { - value = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) + value, _ = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) + if value == "" { + value = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) + } } } diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index 12ec2db117..771f48a152 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -4,22 +4,21 @@ import ( "fmt" "testing" - "k8s.io/apimachinery/pkg/util/intstr" - "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/utils/pointer" ) func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { - new := v1alpha1.Latest stable := v1alpha1.Stable + annotationPath := fmt.Sprintf("metadata.annotations['%s']", annotations.RevisionAnnotation) rolloutAnalysis := &v1alpha1.RolloutAnalysis{ Args: []v1alpha1.AnalysisRunArgument{ { @@ -50,6 +49,18 @@ func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { FieldRef: &v1alpha1.FieldRef{FieldPath: "metadata.labels['env']"}, }, }, + { + Name: annotationPath, + ValueFrom: &v1alpha1.ArgumentValueFrom{ + FieldRef: &v1alpha1.FieldRef{FieldPath: annotationPath}, + }, + }, + { + Name: "status.pauseConditions[0].reason", + ValueFrom: &v1alpha1.ArgumentValueFrom{ + FieldRef: &v1alpha1.FieldRef{FieldPath: "status.pauseConditions[0].reason"}, + }, + }, }, } stableRS := &appsv1.ReplicaSet{ @@ -64,7 +75,6 @@ func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { Labels: map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "123456"}, }, } - ro := &v1alpha1.Rollout{ ObjectMeta: metav1.ObjectMeta{ UID: uuid.NewUUID(), @@ -102,7 +112,13 @@ func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { "env": "test", }}, }, - Status: v1alpha1.RolloutStatus{}, + Status: v1alpha1.RolloutStatus{ + PauseConditions: []v1alpha1.PauseCondition{ + { + Reason: "test-reason", + }, + }, + }, } args := BuildArgumentsForRolloutAnalysisRun(rolloutAnalysis.Args, stableRS, newRS, ro) @@ -111,7 +127,8 @@ func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { assert.Contains(t, args, v1alpha1.Argument{Name: "new-key", Value: pointer.StringPtr("123456")}) assert.Contains(t, args, v1alpha1.Argument{Name: "metadata.labels['app']", Value: pointer.StringPtr("app")}) assert.Contains(t, args, v1alpha1.Argument{Name: "metadata.labels['env']", Value: pointer.StringPtr("test")}) - + assert.Contains(t, args, v1alpha1.Argument{Name: annotationPath, Value: pointer.StringPtr("1")}) + assert.Contains(t, args, v1alpha1.Argument{Name: "status.pauseConditions[0].reason", Value: pointer.StringPtr("test-reason")}) } func TestPrePromotionLabels(t *testing.T) { @@ -426,3 +443,53 @@ func TestResolveMetricArgsWithQuotes(t *testing.T) { assert.NoError(t, err) assert.Equal(t, fmt.Sprintf(arg), newMetric.SuccessCondition) } + +func Test_extractValueFromRollout(t *testing.T) { + ro := &v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Labels: map[string]string{ + "app": "app", + }, + }, + Status: v1alpha1.RolloutStatus{ + PauseConditions: []v1alpha1.PauseCondition{ + { + Reason: "test-reason", + }, + }, + }, + } + tests := map[string]struct { + path string + want string + }{ + "should return a simple metadata value": { + path: "metadata.name", + want: "test", + }, + "should return a label using dot notation": { + path: "metadata.labels.app", + want: "app", + }, + "should fail returning a label using accessor notation": { + path: "metadata.labels['app']", + want: "", + }, + "should return a status value": { + path: "status.pauseConditions[0].reason", + want: "test-reason", + }, + "should return an empty string when path is inavlid": { + path: "some.invalid[2].non.existing.path", + want: "", + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + if got := extractValueFromRollout(ro, tt.path); got != tt.want { + t.Errorf("extractValueFromRollout() = %v, want %v", got, tt.want) + } + }) + } +} From d7a69c2ca9e502db65fb42140f0d47b73081dd80 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:01 +0200 Subject: [PATCH 03/12] validate valueFrom correctly Signed-off-by: Noam Gal --- pkg/apis/rollouts/validation/validation.go | 3 ++- utils/analysis/factory.go | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index 5afb080ddc..da03a1fd95 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "strconv" + "strings" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -278,7 +279,7 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat for _, arg := range analysisRunArgs { if arg.ValueFrom != nil { - if arg.ValueFrom.FieldRef != nil { + if arg.ValueFrom.FieldRef != nil && strings.HasPrefix(arg.ValueFrom.FieldRef.FieldPath, "metadata") { _, err := fieldpath.ExtractFieldPathAsString(rollout, arg.ValueFrom.FieldRef.FieldPath) if err != nil { allErrs = append(allErrs, field.Invalid(stepFldPath.Child("analyses"), analysisRunArgs, InvalidAnalysisArgsMessage)) diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index 507e9f5ee1..d7856695ff 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "strconv" + "strings" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" templateutil "github.com/argoproj/argo-rollouts/utils/template" @@ -28,8 +29,9 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st value = stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } } else if arg.ValueFrom.FieldRef != nil { - value, _ = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) - if value == "" { + if strings.HasPrefix(arg.ValueFrom.FieldRef.FieldPath, "metadata") { + value, _ = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) + } else { value = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) } } From 6c75f7ce93cf32c5d3c4e4938b107c59d2bce68a Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:01 +0200 Subject: [PATCH 04/12] use jsonpath instead of objx return err if path is inavlid in runtime (don't check in validation time) Signed-off-by: Noam Gal --- go.mod | 1 + go.sum | 5 +++ .../validation/validation_references.go | 3 +- rollout/analysis.go | 6 +++- rollout/experiment.go | 6 +++- utils/analysis/factory.go | 35 ++++++++++++++----- utils/analysis/factory_test.go | 29 ++++++++++----- 7 files changed, 65 insertions(+), 20 deletions(-) diff --git a/go.mod b/go.mod index b72e56e7e8..e24228a258 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/argoproj/argo-rollouts go 1.16 require ( + github.com/PaesslerAG/jsonpath v0.1.1 github.com/antonmedv/expr v1.8.9 github.com/argoproj/notifications-engine v0.2.1-0.20210525191332-e8e293898477 github.com/argoproj/pkg v0.9.0 diff --git a/go.sum b/go.sum index e17eea529b..a41327da1a 100644 --- a/go.sum +++ b/go.sum @@ -120,6 +120,11 @@ github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMo github.com/Netflix/go-expect v0.0.0-20180615182759-c93bf25de8e8/go.mod h1:oX5x61PbNXchhh0oikYAH+4Pcfw5LKv21+Jnpr6r6Pc= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= github.com/OpenPeeDeeP/depguard v1.0.1/go.mod h1:xsIw86fROiiwelg+jB2uM9PiKihMMmUx/1V+TNhjQvM= +github.com/PaesslerAG/gval v1.0.0 h1:GEKnRwkWDdf9dOmKcNrar9EA1bz1z9DqPIO1+iLzhd8= +github.com/PaesslerAG/gval v1.0.0/go.mod h1:y/nm5yEyTeX6av0OfKJNp9rBNj2XrGhAf5+v24IBN1I= +github.com/PaesslerAG/jsonpath v0.1.0/go.mod h1:4BzmtoM/PI8fPO4aQGIusjGxGir2BzcV0grWtFzq1Y8= +github.com/PaesslerAG/jsonpath v0.1.1 h1:c1/AToHQMVsduPAa4Vh6xp2U0evy4t8SWp8imEsylIk= +github.com/PaesslerAG/jsonpath v0.1.1/go.mod h1:lVboNxFGal/VwW6d9JzIy56bUsYAP6tH/x80vjnCseY= github.com/PuerkitoBio/purell v1.1.0/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= github.com/PuerkitoBio/purell v1.1.1 h1:WEQqlqaGbrPkxLJWfBwQmfEAE1Z7ONdDLqrN38tNFfI= github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index b02add3d97..1e89764a71 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -382,7 +382,8 @@ func buildAnalysisArgs(args []v1alpha1.AnalysisRunArgument, r *v1alpha1.Rollout) }, }, } - return analysisutil.BuildArgumentsForRolloutAnalysisRun(args, &stableRSDummy, &newRSDummy, r) + res, _ := analysisutil.BuildArgumentsForRolloutAnalysisRun(args, &stableRSDummy, &newRSDummy, r) + return res } // validateAnalysisMetrics validates the metrics of an Analysis object diff --git a/rollout/analysis.go b/rollout/analysis.go index e495b07656..1d9a50fdb5 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -347,7 +347,11 @@ func (c *rolloutContext) reconcileBackgroundAnalysisRun() (*v1alpha1.AnalysisRun } func (c *rolloutContext) createAnalysisRun(rolloutAnalysis *v1alpha1.RolloutAnalysis, infix string, labels map[string]string) (*v1alpha1.AnalysisRun, error) { - args := analysisutil.BuildArgumentsForRolloutAnalysisRun(rolloutAnalysis.Args, c.stableRS, c.newRS, c.rollout) + args, err := analysisutil.BuildArgumentsForRolloutAnalysisRun(rolloutAnalysis.Args, c.stableRS, c.newRS, c.rollout) + if err != nil { + return nil, err + } + podHash := replicasetutil.GetPodTemplateHash(c.newRS) if podHash == "" { return nil, fmt.Errorf("Latest ReplicaSet '%s' has no pod hash in the labels", c.newRS.Name) diff --git a/rollout/experiment.go b/rollout/experiment.go index 807d44ddb7..eddfc64969 100644 --- a/rollout/experiment.go +++ b/rollout/experiment.go @@ -106,7 +106,11 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl } for i := range step.Analyses { analysis := step.Analyses[i] - args := analysisutil.BuildArgumentsForRolloutAnalysisRun(analysis.Args, stableRS, newRS, r) + args, err := analysisutil.BuildArgumentsForRolloutAnalysisRun(analysis.Args, stableRS, newRS, r) + if err != nil { + return nil, err + } + analysisTemplate := v1alpha1.ExperimentAnalysisTemplateRef{ Name: analysis.Name, TemplateName: analysis.TemplateName, diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index d7856695ff..dd06f3c1a4 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -9,13 +9,13 @@ import ( "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" templateutil "github.com/argoproj/argo-rollouts/utils/template" - "github.com/stretchr/objx" + "github.com/PaesslerAG/jsonpath" appsv1 "k8s.io/api/apps/v1" "k8s.io/kubernetes/pkg/fieldpath" ) // BuildArgumentsForRolloutAnalysisRun builds the arguments for a analysis base created by a rollout -func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, stableRS, newRS *appsv1.ReplicaSet, r *v1alpha1.Rollout) []v1alpha1.Argument { +func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, stableRS, newRS *appsv1.ReplicaSet, r *v1alpha1.Rollout) ([]v1alpha1.Argument, error) { arguments := []v1alpha1.Argument{} for i := range args { arg := args[i] @@ -29,10 +29,17 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st value = stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } } else if arg.ValueFrom.FieldRef != nil { + var err error if strings.HasPrefix(arg.ValueFrom.FieldRef.FieldPath, "metadata") { - value, _ = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) + value, err = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) + if err != nil { + return nil, err + } } else { - value = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) + value, err = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) + if err != nil { + return nil, err + } } } } @@ -44,7 +51,7 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st arguments = append(arguments, analysisArg) } - return arguments + return arguments, nil } // PostPromotionLabels returns a map[string]string of common labels for the post promotion analysis @@ -222,8 +229,18 @@ func ValidateMetric(metric v1alpha1.Metric) error { return nil } -func extractValueFromRollout(r *v1alpha1.Rollout, path string) string { - j, _ := json.Marshal(r) - m := objx.MustFromJSON(string(j)) - return m.Get(path).String() +func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) { + j, err := json.Marshal(r) + if err != nil { + return "", err + } + + v := interface{}(nil) + json.Unmarshal(j, &v) + res, err := jsonpath.Get(path, v) + if err != nil { + return "", err + } + + return res.(string), nil } diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index 771f48a152..5251ac8c95 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -121,7 +121,8 @@ func TestBuildArgumentsForRolloutAnalysisRun(t *testing.T) { }, } - args := BuildArgumentsForRolloutAnalysisRun(rolloutAnalysis.Args, stableRS, newRS, ro) + args, err := BuildArgumentsForRolloutAnalysisRun(rolloutAnalysis.Args, stableRS, newRS, ro) + assert.NoError(t, err) assert.Contains(t, args, v1alpha1.Argument{Name: "hard-coded-value-key", Value: pointer.StringPtr("hard-coded-value")}) assert.Contains(t, args, v1alpha1.Argument{Name: "stable-key", Value: pointer.StringPtr("abcdef")}) assert.Contains(t, args, v1alpha1.Argument{Name: "new-key", Value: pointer.StringPtr("123456")}) @@ -461,8 +462,9 @@ func Test_extractValueFromRollout(t *testing.T) { }, } tests := map[string]struct { - path string - want string + path string + want string + wantErr string }{ "should return a simple metadata value": { path: "metadata.name", @@ -473,21 +475,32 @@ func Test_extractValueFromRollout(t *testing.T) { want: "app", }, "should fail returning a label using accessor notation": { - path: "metadata.labels['app']", - want: "", + path: "metadata.labels['app']", + wantErr: "parsing error: metadata.labels['app']\t:1:17 - 1:22 could not parse string: invalid syntax", }, "should return a status value": { path: "status.pauseConditions[0].reason", want: "test-reason", }, "should return an empty string when path is inavlid": { - path: "some.invalid[2].non.existing.path", - want: "", + path: "some.invalid[2].non.existing.path", + wantErr: "unknown parameter some.invalid", }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - if got := extractValueFromRollout(ro, tt.path); got != tt.want { + got, err := extractValueFromRollout(ro, tt.path) + if err != nil { + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + t.Errorf("extractValueFromRollout() error = %v", err) + } + + return + } + + if got != tt.want { t.Errorf("extractValueFromRollout() = %v, want %v", got, tt.want) } }) From e9a05724abd9af79daefb0f11e0a59efcdb922b0 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:02 +0200 Subject: [PATCH 05/12] parse path in code, instead of using jsonPath Signed-off-by: Noam Gal --- go.mod | 1 - go.sum | 5 ----- utils/analysis/factory.go | 46 +++++++++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index e24228a258..b72e56e7e8 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,6 @@ module github.com/argoproj/argo-rollouts go 1.16 require ( - github.com/PaesslerAG/jsonpath v0.1.1 github.com/antonmedv/expr v1.8.9 github.com/argoproj/notifications-engine v0.2.1-0.20210525191332-e8e293898477 github.com/argoproj/pkg v0.9.0 diff --git a/go.sum b/go.sum index a41327da1a..e17eea529b 100644 --- a/go.sum +++ b/go.sum @@ -120,11 +120,6 @@ github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMo github.com/Netflix/go-expect v0.0.0-20180615182759-c93bf25de8e8/go.mod h1:oX5x61PbNXchhh0oikYAH+4Pcfw5LKv21+Jnpr6r6Pc= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= github.com/OpenPeeDeeP/depguard v1.0.1/go.mod h1:xsIw86fROiiwelg+jB2uM9PiKihMMmUx/1V+TNhjQvM= -github.com/PaesslerAG/gval v1.0.0 h1:GEKnRwkWDdf9dOmKcNrar9EA1bz1z9DqPIO1+iLzhd8= -github.com/PaesslerAG/gval v1.0.0/go.mod h1:y/nm5yEyTeX6av0OfKJNp9rBNj2XrGhAf5+v24IBN1I= -github.com/PaesslerAG/jsonpath v0.1.0/go.mod h1:4BzmtoM/PI8fPO4aQGIusjGxGir2BzcV0grWtFzq1Y8= -github.com/PaesslerAG/jsonpath v0.1.1 h1:c1/AToHQMVsduPAa4Vh6xp2U0evy4t8SWp8imEsylIk= -github.com/PaesslerAG/jsonpath v0.1.1/go.mod h1:lVboNxFGal/VwW6d9JzIy56bUsYAP6tH/x80vjnCseY= github.com/PuerkitoBio/purell v1.1.0/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= github.com/PuerkitoBio/purell v1.1.1 h1:WEQqlqaGbrPkxLJWfBwQmfEAE1Z7ONdDLqrN38tNFfI= github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index dd06f3c1a4..af0279b025 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -3,19 +3,20 @@ package analysis import ( "encoding/json" "fmt" + "regexp" "strconv" "strings" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" templateutil "github.com/argoproj/argo-rollouts/utils/template" - "github.com/PaesslerAG/jsonpath" appsv1 "k8s.io/api/apps/v1" "k8s.io/kubernetes/pkg/fieldpath" ) // BuildArgumentsForRolloutAnalysisRun builds the arguments for a analysis base created by a rollout func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, stableRS, newRS *appsv1.ReplicaSet, r *v1alpha1.Rollout) ([]v1alpha1.Argument, error) { + var err error arguments := []v1alpha1.Argument{} for i := range args { arg := args[i] @@ -29,17 +30,15 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st value = stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } } else if arg.ValueFrom.FieldRef != nil { - var err error if strings.HasPrefix(arg.ValueFrom.FieldRef.FieldPath, "metadata") { value, err = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) if err != nil { return nil, err } } else { + // in case of error - return empty value for Validation stage, so it will pass validation + // returned error will only be used in Analysis stage value, err = extractValueFromRollout(r, arg.ValueFrom.FieldRef.FieldPath) - if err != nil { - return nil, err - } } } } @@ -51,7 +50,7 @@ func BuildArgumentsForRolloutAnalysisRun(args []v1alpha1.AnalysisRunArgument, st arguments = append(arguments, analysisArg) } - return arguments, nil + return arguments, err } // PostPromotionLabels returns a map[string]string of common labels for the post promotion analysis @@ -235,12 +234,35 @@ func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) { return "", err } - v := interface{}(nil) - json.Unmarshal(j, &v) - res, err := jsonpath.Get(path, v) - if err != nil { - return "", err + m := interface{}(nil) + json.Unmarshal(j, &m) + sections := regexp.MustCompile("[\\.\\[\\]]+").Split(path, -1) + for _, section := range sections { + if asArray, ok := m.([]interface{}); ok { + if i, err := strconv.Atoi(section); err != nil { + return "", fmt.Errorf("invalid index %s", section) + } else if i >= len(asArray) { + return "", fmt.Errorf("index %d out of range", i) + } else { + m = asArray[i] + } + } else if asMap, ok := m.(map[string]interface{}); ok { + m = asMap[section] + } else { + return "", fmt.Errorf("invalid path %s in rollout", path) + } + } + + if m == nil { + return "", fmt.Errorf("invalid path %s in rollout", path) + } + + var isArray, isMap bool + _, isArray = m.([]interface{}) + _, isMap = m.(map[string]interface{}) + if isArray || isMap { + return "", fmt.Errorf("path %s in rollout must terminate in a primitive value", path) } - return res.(string), nil + return fmt.Sprintf("%v", m), nil } From d21314005f0698f8d3483cc12b479c78c7d93955 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:02 +0200 Subject: [PATCH 06/12] fixed test Signed-off-by: Noam Gal --- utils/analysis/factory_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index 5251ac8c95..a2b6eb5a2e 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -476,7 +476,7 @@ func Test_extractValueFromRollout(t *testing.T) { }, "should fail returning a label using accessor notation": { path: "metadata.labels['app']", - wantErr: "parsing error: metadata.labels['app']\t:1:17 - 1:22 could not parse string: invalid syntax", + wantErr: "invalid path metadata.labels['app'] in rollout", }, "should return a status value": { path: "status.pauseConditions[0].reason", @@ -484,7 +484,7 @@ func Test_extractValueFromRollout(t *testing.T) { }, "should return an empty string when path is inavlid": { path: "some.invalid[2].non.existing.path", - wantErr: "unknown parameter some.invalid", + wantErr: "invalid path some.invalid[2].non.existing.path in rollout", }, } for name, tt := range tests { From 8573e22d929b4cb29e8e22d74327780281e39de4 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:02 +0200 Subject: [PATCH 07/12] updated documentation Signed-off-by: Noam Gal --- docs/features/analysis.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/features/analysis.md b/docs/features/analysis.md index 0c563591ad..980d592c7b 100644 --- a/docs/features/analysis.md +++ b/docs/features/analysis.md @@ -426,8 +426,9 @@ spec: valueFrom: podTemplateHashValue: Latest ``` -Analysis arguments also support valueFrom for reading metadata fields and passing them as arguments to AnalysisTemplate. -An example would be to reference metadata labels like env and region and passing them along to AnalysisTemplate. +Analysis arguments also support valueFrom for reading any Rollout fields and passing them as arguments to AnalysisTemplate. +An example would be to reference metadata labels like env and region and passing them along to AnalysisTemplate, or any field +from the Rollout status ```yaml apiVersion: argoproj.io/v1alpha1 kind: Rollout @@ -457,6 +458,10 @@ spec: valueFrom: fieldRef: fieldPath: metadata.labels['region'] + - name: canary-hash + valueFrom: + fieldRef: + fieldPath: status.canary.weights.canary.podTemplateHash ``` ## BlueGreen Pre Promotion Analysis From 7267c63a039aae7787435554cdde3c2e30b55c38 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:02 +0200 Subject: [PATCH 08/12] added tests for coverage Signed-off-by: Noam Gal --- utils/analysis/factory.go | 8 ++------ utils/analysis/factory_test.go | 14 +++++++++++++- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index af0279b025..169fe38d16 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -229,18 +229,14 @@ func ValidateMetric(metric v1alpha1.Metric) error { } func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) { - j, err := json.Marshal(r) - if err != nil { - return "", err - } - + j, _ := json.Marshal(r) m := interface{}(nil) json.Unmarshal(j, &m) sections := regexp.MustCompile("[\\.\\[\\]]+").Split(path, -1) for _, section := range sections { if asArray, ok := m.([]interface{}); ok { if i, err := strconv.Atoi(section); err != nil { - return "", fmt.Errorf("invalid index %s", section) + return "", fmt.Errorf("invalid index '%s'", section) } else if i >= len(asArray) { return "", fmt.Errorf("index %d out of range", i) } else { diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index a2b6eb5a2e..c808c708ab 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -482,7 +482,19 @@ func Test_extractValueFromRollout(t *testing.T) { path: "status.pauseConditions[0].reason", want: "test-reason", }, - "should return an empty string when path is inavlid": { + "should fail when array indexer is not an int": { + path: "status.pauseConditions[blah].reason", + wantErr: "invalid index 'blah'", + }, + "should fail when array indexer is out of range": { + path: "status.pauseConditions[12].reason", + wantErr: "index 12 out of range", + }, + "should fail when path references an empty field": { + path: "status.pauseConditions[0].startTime", + wantErr: "invalid path status.pauseConditions[0].startTime in rollout", + }, + "should fail when path is inavlid": { path: "some.invalid[2].non.existing.path", wantErr: "invalid path some.invalid[2].non.existing.path in rollout", }, From e56cdb66a55783de404f9ec7bb0cfb245ce46edd Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:03 +0200 Subject: [PATCH 09/12] fixed lint Signed-off-by: Noam Gal --- utils/analysis/factory_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index c808c708ab..ffe933fb52 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -483,11 +483,11 @@ func Test_extractValueFromRollout(t *testing.T) { want: "test-reason", }, "should fail when array indexer is not an int": { - path: "status.pauseConditions[blah].reason", + path: "status.pauseConditions[blah].reason", wantErr: "invalid index 'blah'", }, "should fail when array indexer is out of range": { - path: "status.pauseConditions[12].reason", + path: "status.pauseConditions[12].reason", wantErr: "index 12 out of range", }, "should fail when path references an empty field": { From 63d336976511b13c0b7fd685cd378a65d1f9a1d4 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:03 +0200 Subject: [PATCH 10/12] added another test case Signed-off-by: Noam Gal --- utils/analysis/factory_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/utils/analysis/factory_test.go b/utils/analysis/factory_test.go index ffe933fb52..ceece4ac91 100644 --- a/utils/analysis/factory_test.go +++ b/utils/analysis/factory_test.go @@ -498,6 +498,10 @@ func Test_extractValueFromRollout(t *testing.T) { path: "some.invalid[2].non.existing.path", wantErr: "invalid path some.invalid[2].non.existing.path in rollout", }, + "should fail when path references a non-primitive value": { + path: "status.pauseConditions[0]", + wantErr: "path status.pauseConditions[0] in rollout must terminate in a primitive value", + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { From 1eab4fdf2d7dd46554657d39627a8d12e1e33c71 Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Wed, 17 Nov 2021 17:53:03 +0200 Subject: [PATCH 11/12] fixed case when path ends with "]" Signed-off-by: Noam Gal --- utils/analysis/factory.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/utils/analysis/factory.go b/utils/analysis/factory.go index 169fe38d16..0422daef8d 100644 --- a/utils/analysis/factory.go +++ b/utils/analysis/factory.go @@ -234,6 +234,10 @@ func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) { json.Unmarshal(j, &m) sections := regexp.MustCompile("[\\.\\[\\]]+").Split(path, -1) for _, section := range sections { + if section == "" { + continue // if path ends with a separator char, Split returns an empty last section + } + if asArray, ok := m.([]interface{}); ok { if i, err := strconv.Atoi(section); err != nil { return "", fmt.Errorf("invalid index '%s'", section) From b35fcb5a8281794d78da4f9fe9ea8e5eca43d0ab Mon Sep 17 00:00:00 2001 From: Noam Gal Date: Sun, 21 Nov 2021 14:50:10 +0200 Subject: [PATCH 12/12] removed objx dependency Signed-off-by: Noam Gal --- go.mod | 1 - go.sum | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/go.mod b/go.mod index b72e56e7e8..4e5eb688da 100644 --- a/go.mod +++ b/go.mod @@ -32,7 +32,6 @@ require ( github.com/soheilhy/cmux v0.1.4 github.com/spaceapegames/go-wavefront v1.8.1 github.com/spf13/cobra v1.1.3 - github.com/stretchr/objx v0.3.0 // indirect github.com/stretchr/testify v1.7.0 github.com/tj/assert v0.0.3 github.com/undefinedlabs/go-mpatch v1.0.6 diff --git a/go.sum b/go.sum index e17eea529b..3784ded5aa 100644 --- a/go.sum +++ b/go.sum @@ -1093,9 +1093,8 @@ github.com/streadway/amqp v0.0.0-20190827072141-edfb9018d271/go.mod h1:AZpEONHx3 github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= -github.com/stretchr/objx v0.3.0 h1:NGXK3lHquSN08v5vWalVI/L8XU9hdzE/G6xsrze47As= -github.com/stretchr/objx v0.3.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/testify v0.0.0-20161117074351-18a02ba4a312/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=