diff --git a/test/e2e/expr_lang.go b/test/e2e/expr_lang_test.go similarity index 52% rename from test/e2e/expr_lang.go rename to test/e2e/expr_lang_test.go index d6fb31d60261..ec62e36c1fdf 100644 --- a/test/e2e/expr_lang.go +++ b/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)) } diff --git a/util/template/expression_template.go b/util/template/expression_template.go index 59ce6dd0555b..43f1bb0c3b8e 100644 --- a/util/template/expression_template.go +++ b/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 diff --git a/util/template/expression_template_test.go b/util/template/expression_template_test.go index c5f5027502b9..06a978de0e98 100644 --- a/util/template/expression_template_test.go +++ b/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")) }