fix: exit handler variables don't get resolved correctly. Fixes argop…

…roj#10393 (argoproj#10449) Signed-off-by: Jiacheng Xu <xjcmaxwellcjx@gmail.com>
julienduchesne · Feb 21, 2023 · 5c3c3b3 · 5c3c3b3
1 parent 43cb4f6
commit 5c3c3b3
Show file tree

Hide file tree

Showing 5 changed files with 165 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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 { //  <nil> 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
@@ -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
@@ -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)