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

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,33 +21,45 @@ func init() {
 	}
 }
 
+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 &variable
+		}
+	}
+	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 _, ok := env["retries"]; !ok && hasRetries(unmarshalledExpression) && 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)
-	}
-
-	// 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 ((hasWorkflowStatus(unmarshalledExpression) && !hasVarInEnv(env, "workflow.status")) ||
-		(hasWorkflowFailures(unmarshalledExpression) && !hasVarInEnv(env, "workflow.failures"))) &&
-		allowUnresolved {
+	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)
 	}
 
@@ -100,56 +112,55 @@ 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 {
-	tokens, err := lexer.Lex(file.NewSource(expression))
-	if err != nil {
+func searchTokens(haystack []lexer.Token, needle []lexer.Token) bool {
+	if len(needle) > len(haystack) {
 		return false
 	}
-	for _, token := range tokens {
-		if token.Kind == lexer.Identifier && token.Value == "retries" {
-			return true
+	if len(needle) == 0 {
+		return true
+	}
+outer:
+	for i := 0; i <= len(haystack)-len(needle); i++ {
+		for j := 0; j < len(needle); j++ {
+			if haystack[i+j].String() != needle[j].String() {
+				continue outer
+			}
 		}
+		return true
 	}
 	return false
 }
 
-// hasWorkflowStatus checks if expression contains `workflow.status`
-func hasWorkflowStatus(expression string) bool {
-	if !strings.Contains(expression, "workflow.status") {
-		return false
-	}
-	// Even if the expression contains `workflow.status`, it could be the case that it represents a string (`"workflow.status"`),
-	// not a variable, so we need to parse it and handle filter the string case.
-	tokens, err := lexer.Lex(file.NewSource(expression))
-	if err != nil {
-		return false
-	}
-	for i := 0; i < len(tokens)-2; i++ {
-		if tokens[i].Value+tokens[i+1].Value+tokens[i+2].Value == "workflow.status" {
-			return true
+func filterEOF(toks []lexer.Token) []lexer.Token {
+	newToks := []lexer.Token{}
+	for _, tok := range toks {
+		if tok.Kind != lexer.EOF {
+			newToks = append(newToks, tok)
 		}
 	}
-	return false
+	return newToks
 }
 
-// hasWorkflowFailures checks if expression contains `workflow.failures`
-func hasWorkflowFailures(expression string) bool {
-	if !strings.Contains(expression, "workflow.failures") {
+// hasVariableInExpression checks if an expression contains a variable.
+// This function is somewhat cursed and I have attempted my best to
+// remove this curse, but it still exists.
+// The strings.Contains is needed because the lexer doesn't do
+// any whitespace processing (workflow .status will be seen as workflow.status)
+func hasVariableInExpression(expression, variable string) bool {
+	if !strings.Contains(expression, variable) {
 		return false
 	}
-	// Even if the expression contains `workflow.failures`, it could be the case that it represents a string (`"workflow.failures"`),
-	// not a variable, so we need to parse it and handle filter the string case.
 	tokens, err := lexer.Lex(file.NewSource(expression))
 	if err != nil {
 		return false
 	}
-	for i := 0; i < len(tokens)-2; i++ {
-		if tokens[i].Value+tokens[i+1].Value+tokens[i+2].Value == "workflow.failures" {
-			return true
-		}
+	variableTokens, err := lexer.Lex(file.NewSource(variable))
+	if err != nil {
+		return false
 	}
-	return false
+	variableTokens = filterEOF(variableTokens)
+
+	return searchTokens(tokens, variableTokens)
 }
 
 // hasVarInEnv checks if a parameter is in env or not

util/template/expression_template_test.go

@@ -3,34 +3,34 @@ package template
 import (
 	"testing"
 
+	"github.com/expr-lang/expr/file"
+	"github.com/expr-lang/expr/parser/lexer"
 	"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("hasWorkflowStatusInExpression", func(t *testing.T) {
-		assert.True(t, hasWorkflowStatus("workflow.status"))
-		assert.True(t, hasWorkflowStatus(`workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"`))
-		assert.False(t, hasWorkflowStatus(`"workflow.status" == "Succeeded" ? "SUCCESSFUL" : "FAILED"`))
-		assert.False(t, hasWorkflowStatus("workflow status"))
-		assert.False(t, hasWorkflowStatus("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"))
 
-	t.Run("hasWorkflowFailuresInExpression", func(t *testing.T) {
-		assert.True(t, hasWorkflowFailures("workflow.failures"))
-		assert.True(t, hasWorkflowFailures(`workflow.failures == "Succeeded" ? "SUCCESSFUL" : "FAILED"`))
-		assert.False(t, hasWorkflowFailures(`"workflow.failures" == "Succeeded" ? "SUCCESSFUL" : "FAILED"`))
-		assert.False(t, hasWorkflowFailures("workflow failures"))
-		assert.False(t, hasWorkflowFailures("workflow .failures"))
-	})
+	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"))
 }