From 96276a4816d85ddd06bbe3e3fedcd0f9ad9851be Mon Sep 17 00:00:00 2001 From: kim-codefresh Date: Thu, 23 Feb 2023 20:17:15 +0200 Subject: [PATCH] fix: exit handler variables don't get resolved correctly. Fixes #10393 (#10449) (#245) Signed-off-by: Jiacheng Xu Co-authored-by: Jiacheng Xu --- test/e2e/functional_test.go | 17 ++++++ .../e2e/testdata/workflow-hook-parameter.yaml | 36 +++++++++++ util/template/expression_template.go | 59 +++++++++++++++++++ util/template/expression_template_test.go | 18 ++++++ util/template/replace_test.go | 36 ++++++++++- 5 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 test/e2e/testdata/workflow-hook-parameter.yaml diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index 11918cc9fbbe..96332b9f1724 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -688,6 +688,23 @@ spec: }) } +func (s *FunctionalSuite) TestWorkflowHookParameterTemplates() { + s.Given(). + Workflow("@testdata/workflow-hook-parameter.yaml"). + When(). + SubmitWorkflow(). + WaitForWorkflow(fixtures.ToBeSucceeded). + Then(). + ExpectWorkflow(func(t *testing.T, md *metav1.ObjectMeta, status *wfv1.WorkflowStatus) { + assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase) + }). + ExpectWorkflowNode(wfv1.NodeWithDisplayName("workflow-hook-parameter.onExit"), func(t *testing.T, n *wfv1.NodeStatus, p *apiv1.Pod) { + assert.Equal(t, wfv1.NodeSucceeded, n.Phase) + assert.Equal(t, "Succeeded", n.Inputs.Parameters[0].Value.String()) + assert.Equal(t, "Succeeded", n.Inputs.Parameters[1].Value.String()) + }) +} + func (s *FunctionalSuite) TestParametrizableAds() { s.Given(). Workflow(` diff --git a/test/e2e/testdata/workflow-hook-parameter.yaml b/test/e2e/testdata/workflow-hook-parameter.yaml new file mode 100644 index 000000000000..f747e810bee4 --- /dev/null +++ b/test/e2e/testdata/workflow-hook-parameter.yaml @@ -0,0 +1,36 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + name: workflow-hook-parameter +spec: + entrypoint: run-test + templates: + - name: run-test + container: + name: runner + image: 'argoproj/argosay:v2' + command: ['sh','-c'] + args: + - exit 0 + - name: cowsay + inputs: + parameters: + - name: ternary + - name: status + container: + image: 'argoproj/argosay:v2' + command: ['bash','-c'] + args: + - | + echo "{{inputs.parameters.ternary}}" + echo "{{inputs.parameters.status}}" + [[ "{{inputs.parameters.ternary}}" = "{{inputs.parameters.status}}" ]] + hooks: + exit: + template: cowsay + arguments: + parameters: + - name: ternary + value: '{{= workflow.status == "Succeeded" ? "Succeeded" : "Failed" }}' + - name: status + value: '{{= workflow.status }}' diff --git a/util/template/expression_template.go b/util/template/expression_template.go index e2982c2dc8a2..d2a8aa3f563a 100644 --- a/util/template/expression_template.go +++ b/util/template/expression_template.go @@ -5,10 +5,12 @@ import ( "fmt" "io" "os" + "strings" "github.com/antonmedv/expr" "github.com/antonmedv/expr/file" "github.com/antonmedv/expr/parser/lexer" + "github.com/doublerebel/bellows" ) func init() { @@ -33,6 +35,18 @@ func expressionReplace(w io.Writer, expression string, env map[string]interface{ // See https://github.com/argoproj/argo-workflows/issues/5388 return w.Write([]byte(fmt.Sprintf("{{%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/antonmedv/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 { + return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression))) + } + result, err := expr.Eval(unmarshalledExpression, env) if (err != nil || result == nil) && allowUnresolved { // result is also un-resolved, and any error can be unresolved return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression))) @@ -79,3 +93,48 @@ func hasRetries(expression string) bool { } 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 + } + } + return false +} + +// hasWorkflowFailures checks if expression contains `workflow.failures` +func hasWorkflowFailures(expression string) bool { + if !strings.Contains(expression, "workflow.failures") { + 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 + } + } + return false +} + +// hasVarInEnv checks if a parameter is in env or not +func hasVarInEnv(env map[string]interface{}, parameter string) bool { + flattenEnv := bellows.Flatten(env) + _, ok := flattenEnv[parameter] + return ok +} diff --git a/util/template/expression_template_test.go b/util/template/expression_template_test.go index 9f96a88e220e..c5f5027502b9 100644 --- a/util/template/expression_template_test.go +++ b/util/template/expression_template_test.go @@ -16,3 +16,21 @@ func Test_hasRetries(t *testing.T) { assert.False(t, hasRetries("retriesCustom + 1")) }) } + +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")) + }) + + 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")) + }) +} diff --git a/util/template/replace_test.go b/util/template/replace_test.go index fed05c15d2e0..0ab0c8ae6a60 100644 --- a/util/template/replace_test.go +++ b/util/template/replace_test.go @@ -40,6 +40,22 @@ func Test_Replace(t *testing.T) { assert.NoError(t, err) assert.Equal(t, toJsonString("bar"), r) }) + t.Run("Valid WorkflowStatus", func(t *testing.T) { + replaced, err := Replace(toJsonString(`{{=workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), map[string]string{"workflow.status": "Succeeded"}, false) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`SUCCESSFUL`), replaced) + replaced, err = Replace(toJsonString(`{{=workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), map[string]string{"workflow.status": "Failed"}, false) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`FAILED`), replaced) + }) + t.Run("Valid WorkflowFailures", func(t *testing.T) { + replaced, err := Replace(toJsonString(`{{=workflow.failures == "{\"foo\":\"bar\"}" ? "SUCCESSFUL" : "FAILED"}}`), map[string]string{"workflow.failures": `{"foo":"bar"}`}, false) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`SUCCESSFUL`), replaced) + replaced, err = Replace(toJsonString(`{{=workflow.failures == "{\"foo\":\"bar\"}" ? "SUCCESSFUL" : "FAILED"}}`), map[string]string{"workflow.failures": `{"foo":"barr"}`}, false) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`FAILED`), replaced) + }) t.Run("Unresolved", func(t *testing.T) { t.Run("Allowed", func(t *testing.T) { _, err := Replace(toJsonString("{{=foo}}"), nil, true) @@ -48,12 +64,30 @@ func Test_Replace(t *testing.T) { t.Run("AllowedRetries", func(t *testing.T) { replaced, err := Replace(toJsonString("{{=sprig.int(retries)}}"), nil, true) assert.NoError(t, err) - assert.Equal(t, replaced, toJsonString("{{=sprig.int(retries)}}")) + assert.Equal(t, toJsonString("{{=sprig.int(retries)}}"), replaced) + }) + t.Run("AllowedWorkflowStatus", func(t *testing.T) { + replaced, err := Replace(toJsonString(`{{=workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), nil, true) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`{{=workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), replaced) + }) + t.Run("AllowedWorkflowFailures", func(t *testing.T) { + replaced, err := Replace(toJsonString(`{{=workflow.failures == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), nil, true) + assert.NoError(t, err) + assert.Equal(t, toJsonString(`{{=workflow.failures == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), replaced) }) t.Run("Disallowed", func(t *testing.T) { _, err := Replace(toJsonString("{{=foo}}"), nil, false) assert.EqualError(t, err, "failed to evaluate expression \"foo\"") }) + t.Run("DisallowedWorkflowStatus", func(t *testing.T) { + _, err := Replace(toJsonString(`{{=workflow.status == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), nil, false) + assert.ErrorContains(t, err, "failed to evaluate expression") + }) + t.Run("DisallowedWorkflowFailures", func(t *testing.T) { + _, err := Replace(toJsonString(`{{=workflow.failures == "Succeeded" ? "SUCCESSFUL" : "FAILED"}}`), nil, false) + assert.ErrorContains(t, err, "failed to evaluate expression") + }) }) t.Run("Error", func(t *testing.T) { _, err := Replace(toJsonString("{{=!}}"), nil, false)