feat(analysis): Allow analysis arguments to get valueFrom Rollout sta…

…tus (#1242) (#1629) * use objx to read value from Rollout manifest Signed-off-by: Noam Gal <[email protected]> * handle `[]` annotation correcly in BuildArgumentsForRolloutAnalysisRun Signed-off-by: Noam Gal <[email protected]> * validate valueFrom correctly Signed-off-by: Noam Gal <[email protected]> * use jsonpath instead of objx return err if path is inavlid in runtime (don't check in validation time) Signed-off-by: Noam Gal <[email protected]> * parse path in code, instead of using jsonPath Signed-off-by: Noam Gal <[email protected]> * fixed test Signed-off-by: Noam Gal <[email protected]> * updated documentation Signed-off-by: Noam Gal <[email protected]> * added tests for coverage Signed-off-by: Noam Gal <[email protected]> * fixed lint Signed-off-by: Noam Gal <[email protected]> * added another test case Signed-off-by: Noam Gal <[email protected]> * fixed case when path ends with "]" Signed-off-by: Noam Gal <[email protected]> * removed objx dependency Signed-off-by: Noam Gal <[email protected]>
argoproj · Jan 12, 2022 · 4739bcd · 4739bcd
1 parent 7f65b2b
commit 4739bcd
Show file tree

Hide file tree

Showing 7 changed files with 181 additions and 21 deletions.
diff --git 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

diff --git 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/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
@@ -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
@@ -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
@@ -3,17 +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/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	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) {
+	var err error
 	arguments := []v1alpha1.Argument{}
 	for i := range args {
 		arg := args[i]
@@ -26,21 +29,28 @@ 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 {
+				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)
 				}
 			}
-
 		}
+
 		analysisArg := v1alpha1.Argument{
 			Name:  arg.Name,
 			Value: &value,
 		}
 		arguments = append(arguments, analysisArg)
-
 	}
-	return arguments
+
+	return arguments, err
 }
 
 // PostPromotionLabels returns a map[string]string of common labels for the post promotion analysis
@@ -217,3 +227,42 @@ func ValidateMetric(metric v1alpha1.Metric) error {
 	}
 	return nil
 }
+
+func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) {
+	j, _ := json.Marshal(r)
+	m := interface{}(nil)
+	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)
+			} 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 fmt.Sprintf("%v", m), nil
+}
diff --git 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,16 +112,24 @@ 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)
+	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")})
 	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 +444,81 @@ 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
+		wantErr 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']",
+			wantErr: "invalid path metadata.labels['app'] in rollout",
+		},
+		"should return a status value": {
+			path: "status.pauseConditions[0].reason",
+			want: "test-reason",
+		},
+		"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",
+		},
+		"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) {
+			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)
+			}
+		})
+	}
+}