fix: add a special case for item variable during global expression replacement (cherry-pick #15033 for 3.7) (#15036)

Signed-off-by: Elliot Gunton <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: Elliot Gunton <[email protected]>

Author: Joibel

test/e2e/expr_lang.go renamed to test/e2e/expr_lang_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 	apiv1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -19,7 +20,7 @@ type ExprSuite struct {
 	fixtures.E2ESuite
 }
 
-func (s *ExprSuite) TestRegression12037() {
+func (s *ExprSuite) TestShadowedExprFunctionsAsTaskNames() {
 	s.Given().
 		Workflow(`apiVersion: argoproj.io/v1alpha1
 kind: Workflow
@@ -39,12 +40,11 @@ spec:
 
     - name: foo
       container:
-        image: alpine
+        image: argoproj/argosay:v2
         command:
-          - sh
-          - -c
-          - |
-            echo "foo"
+        - echo
+        args:
+        - foo
 `).When().
 		SubmitWorkflow().
 		WaitForWorkflow(fixtures.ToBeSucceeded).
@@ -64,6 +64,49 @@ spec:
 		})
 }
 
-func TestExprLangSSuite(t *testing.T) {
+func (s *ExprSuite) TestItemInSprigExpr() {
+	s.Given().
+		Workflow(`apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: item-used-in-sprig-
+spec:
+  entrypoint: loop-example
+  templates:
+  - name: print-message
+    container:
+      image: argoproj/argosay:v2
+      args:
+      - '{{inputs.parameters.message}}'
+      command:
+      - echo
+    inputs:
+      parameters:
+      - name: message
+  - name: loop-example
+    steps:
+    - - name: print-message-loop
+        template: print-message
+        withItems:
+        - example
+        arguments:
+          parameters:
+          - name: message
+            value: '{{=sprig.upper(item)}}'
+`).When().
+		SubmitWorkflow().
+		WaitForWorkflow(fixtures.ToBeSucceeded).
+		Then().
+		ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) {
+			assert.Equal(t, v1alpha1.WorkflowSucceeded, status.Phase)
+
+			stepNode := status.Nodes.FindByDisplayName("print-message-loop(0:example)")
+			require.NotNil(t, stepNode)
+			require.NotNil(t, stepNode.Inputs.Parameters[0].Value)
+			assert.Equal(t, "EXAMPLE", string(*stepNode.Inputs.Parameters[0].Value))
+		})
+}
+
+func TestExprLangSuite(t *testing.T) {
 	suite.Run(t, new(ExprSuite))
 }

util/template/expression_template.go

@@ -21,49 +21,45 @@ func init() {
 	}
 }
 
-func anyVarNotInEnv(expression string, variables []string, env map[string]interface{}) bool {
-	for _, variable := range variables {
+var variablesToCheck = []string{
+	"item",
+	"retries",
+	"lastRetry.exitCode",
+	"lastRetry.status",
+	"lastRetry.duration",
+	"lastRetry.message",
+	"workflow.status",
+	"workflow.failures",
+}
+
+func anyVarNotInEnv(expression string, env map[string]interface{}) *string {
+	for _, variable := range variablesToCheck {
 		if hasVariableInExpression(expression, variable) && !hasVarInEnv(env, variable) {
-			return true
+			return &variable
 		}
 	}
-	return false
+	return nil
 }
 
 func expressionReplace(w io.Writer, expression string, env map[string]interface{}, allowUnresolved bool) (int, error) {
 	// The template is JSON-marshaled. This JSON-unmarshals the expression to undo any character escapes.
 	var unmarshalledExpression string
 	err := json.Unmarshal([]byte(fmt.Sprintf(`"%s"`, expression)), &unmarshalledExpression)
 	if err != nil && allowUnresolved {
-		log.WithError(err).Debug("unresolved is allowed ")
+		log.WithError(err).Debug("unresolved is allowed")
 		return fmt.Fprintf(w, "{{%s%s}}", kindExpression, expression)
 	}
 	if err != nil {
 		return 0, fmt.Errorf("failed to unmarshall JSON expression: %w", err)
 	}
 
-	if anyVarNotInEnv(unmarshalledExpression, []string{"retries"}, env) && allowUnresolved {
-		// this is to make sure expressions like `sprig.int(retries)` don't get resolved to 0 when `retries` don't exist in the env
-		// See https://github.com/argoproj/argo-workflows/issues/5388
-		log.WithError(err).Debug("Retries are present and unresolved is allowed")
-		return fmt.Fprintf(w, "{{%s%s}}", kindExpression, expression)
-	}
-
-	lastRetryVariables := []string{"lastRetry.exitCode", "lastRetry.status", "lastRetry.duration", "lastRetry.message"}
-	if anyVarNotInEnv(unmarshalledExpression, lastRetryVariables, env) && allowUnresolved {
-		// This is to make sure expressions which contains `lastRetry.*` don't get resolved to nil
-		// when they don't exist in the env.
-		log.WithError(err).Debug("LastRetry variables are present and unresolved is allowed")
-		return fmt.Fprintf(w, "{{%s%s}}", kindExpression, expression)
-	}
-
-	// This is to make sure expressions which contains `workflow.status` and `work.failures` don't get resolved to nil
-	// when `workflow.status` and `workflow.failures` don't exist in the env.
-	// See https://github.com/argoproj/argo-workflows/issues/10393, https://github.com/expr-lang/expr/issues/330
-	// This issue doesn't happen to other template parameters since `workflow.status` and `workflow.failures` only exist in the env
-	// when the exit handlers complete.
-	if anyVarNotInEnv(unmarshalledExpression, []string{"workflow.status", "workflow.failures"}, env) && allowUnresolved {
-		log.WithError(err).Debug("workflow.status or workflow.failures are present and unresolved is allowed")
+	varNameNotInEnv := anyVarNotInEnv(unmarshalledExpression, env)
+	if varNameNotInEnv != nil && allowUnresolved {
+		// this is to make sure expressions don't get resolved to nil or an empty string when certain variables
+		// don't exist in the env during the "global" replacement.
+		// See https://github.com/argoproj/argo-workflows/issues/5388, https://github.com/argoproj/argo-workflows/issues/15008,
+		// https://github.com/argoproj/argo-workflows/issues/10393, https://github.com/expr-lang/expr/issues/330
+		log.WithField("variable", *varNameNotInEnv).Debug("variable not in env but unresolved is allowed")
 		return fmt.Fprintf(w, "{{%s%s}}", kindExpression, expression)
 	}
 
@@ -116,11 +112,6 @@ func EnvMap(replaceMap map[string]string) map[string]interface{} {
 	return envMap
 }
 
-// hasRetries checks if the variable `retries` exists in the expression template
-func hasRetries(expression string) bool {
-	return hasVariableInExpression(expression, "retries")
-}
-
 func searchTokens(haystack []lexer.Token, needle []lexer.Token) bool {
 	if len(needle) > len(haystack) {
 		return false

util/template/expression_template_test.go

@@ -8,30 +8,29 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func Test_hasRetries(t *testing.T) {
-	t.Run("hasRetiresInExpression", func(t *testing.T) {
-		assert.True(t, hasRetries("retries"))
-		assert.True(t, hasRetries("retries + 1"))
-		assert.True(t, hasRetries("sprig(retries)"))
-		assert.True(t, hasRetries("sprig(retries + 1) * 64"))
-		assert.False(t, hasRetries("foo"))
-		assert.False(t, hasRetries("retriesCustom + 1"))
-	})
+func Test_hasVariableInExpression(t *testing.T) {
+	assert.True(t, hasVariableInExpression("retries", "retries"))
+	assert.True(t, hasVariableInExpression("retries + 1", "retries"))
+	assert.True(t, hasVariableInExpression("sprig(retries)", "retries"))
+	assert.True(t, hasVariableInExpression("sprig(retries + 1) * 64", "retries"))
+	assert.False(t, hasVariableInExpression("foo", "retries"))
+	assert.False(t, hasVariableInExpression("retriesCustom + 1", "retries"))
+	assert.True(t, hasVariableInExpression("item", "item"))
+	assert.False(t, hasVariableInExpression("foo", "item"))
+	assert.True(t, hasVariableInExpression("sprig.upper(item)", "item"))
 }
 
 func Test_hasWorkflowParameters(t *testing.T) {
-	t.Run("hasVariableInExpression", func(t *testing.T) {
-		expression := `workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"`
-		exprToks, _ := lexer.Lex(file.NewSource(expression))
-		variableToks, _ := lexer.Lex(file.NewSource("workflow.status"))
-		variableToks = filterEOF(variableToks)
-		assert.True(t, searchTokens(exprToks, variableToks))
-		assert.True(t, hasVariableInExpression(expression, "workflow.status"))
+	expression := `workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"`
+	exprToks, _ := lexer.Lex(file.NewSource(expression))
+	variableToks, _ := lexer.Lex(file.NewSource("workflow.status"))
+	variableToks = filterEOF(variableToks)
+	assert.True(t, searchTokens(exprToks, variableToks))
+	assert.True(t, hasVariableInExpression(expression, "workflow.status"))
 
-		assert.False(t, hasVariableInExpression(expression, "workflow status"))
-		assert.False(t, hasVariableInExpression(expression, "workflow .status"))
+	assert.False(t, hasVariableInExpression(expression, "workflow status"))
+	assert.False(t, hasVariableInExpression(expression, "workflow .status"))
 
-		expression = `"workflow.status" == "Succeeded" ? "SUCCESSFUL" : "FAILED"`
-		assert.False(t, hasVariableInExpression(expression, "workflow .status"))
-	})
+	expression = `"workflow.status" == "Succeeded" ? "SUCCESSFUL" : "FAILED"`
+	assert.False(t, hasVariableInExpression(expression, "workflow .status"))
 }