fix: Exit lifecycle hook should respect expression. Fixes #8742 (#8744)

Signed-off-by: Yuan Tang <[email protected]>
argoproj · May 13, 2022 · 354dee8 · 354dee8
1 parent 91fc3ea
commit 354dee8
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 18 deletions.
diff --git a/test/e2e/hooks_test.go b/test/e2e/hooks_test.go
@@ -52,7 +52,6 @@ spec:
 		}).ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
 		return strings.Contains(status.Name, "hook")
 	}, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
-
 		assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase)
 	})
 }
@@ -71,7 +70,6 @@ spec:
         - - name: step-1
             hooks:
               exit:
-                expression: steps["step-1"].status == "Running"
                 template: http
               success:
                 expression: steps["step-1"].status == "Succeeded"
@@ -85,11 +83,43 @@ spec:
 		WaitForWorkflow(fixtures.ToBeSucceeded).
 		Then().
 		ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) {
-			assert.Equal(t, status.Phase, v1alpha1.WorkflowSucceeded)
+			assert.Equal(t, v1alpha1.WorkflowSucceeded, status.Phase)
 		}).ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
 		return strings.Contains(status.Name, "hook")
 	}, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
+		assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase)
+	})
+}
 
+func (s *HooksSuite) TestExitHookWithExpression() {
+	s.Given().
+		Workflow(`apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: lifecycle-hook-exit-hook-
+spec:
+  entrypoint: main
+  templates:
+    - name: main
+      steps:
+        - - name: step-1
+            hooks:
+              exit:
+                expression: steps["step-1"].status == "Running"
+                template: http
+            template: http
+    - name: http
+      http:
+        url: "http://httpstat.us"
+`).When().
+		SubmitWorkflow().
+		WaitForWorkflow(fixtures.ToBeSucceeded).
+		Then().
+		ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) {
+			assert.Equal(t, v1alpha1.WorkflowRunning, status.Phase)
+		}).ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
+		return strings.Contains(status.Name, "hook")
+	}, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
 		assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase)
 	})
 }

diff --git a/workflow/controller/exit_handler.go b/workflow/controller/exit_handler.go
@@ -6,6 +6,8 @@ import (
 	"fmt"
 
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
+	"github.com/argoproj/argo-workflows/v3/util/expr/argoexpr"
+	"github.com/argoproj/argo-workflows/v3/util/expr/env"
 	"github.com/argoproj/argo-workflows/v3/util/template"
 	"github.com/argoproj/argo-workflows/v3/workflow/common"
 	"github.com/argoproj/argo-workflows/v3/workflow/templateresolution"
@@ -17,27 +19,35 @@ func (woc *wfOperationCtx) runOnExitNode(ctx context.Context, exitHook *wfv1.Lif
 		lastChildNode := getChildNodeIndex(parentNode, woc.wf.Status.Nodes, -1)
 		outputs = lastChildNode.Outputs
 	}
-
 	if exitHook != nil && woc.GetShutdownStrategy().ShouldExecute(true) {
-		woc.log.WithField("lifeCycleHook", exitHook).Infof("Running OnExit handler")
-
-		onExitNodeName := common.GenerateOnExitNodeName(parentNode.Name)
-		resolvedArgs := exitHook.Arguments
+		execute := true
 		var err error
-		if !resolvedArgs.IsEmpty() && outputs != nil {
-			resolvedArgs, err = woc.resolveExitTmplArgument(exitHook.Arguments, prefix, outputs)
+		if exitHook.Expression != "" {
+			execute, err = argoexpr.EvalBool(exitHook.Expression, env.GetFuncMap(template.EnvMap(woc.globalParams)))
 			if err != nil {
 				return true, nil, err
 			}
-
 		}
-		onExitNode, err := woc.executeTemplate(ctx, onExitNodeName, &wfv1.WorkflowStep{Template: exitHook.Template, TemplateRef: exitHook.TemplateRef}, tmplCtx, resolvedArgs, &executeTemplateOpts{
+		if execute {
+			woc.log.WithField("lifeCycleHook", exitHook).Infof("Running OnExit handler")
+
+			onExitNodeName := common.GenerateOnExitNodeName(parentNode.Name)
+			resolvedArgs := exitHook.Arguments
+			if !resolvedArgs.IsEmpty() && outputs != nil {
+				resolvedArgs, err = woc.resolveExitTmplArgument(exitHook.Arguments, prefix, outputs)
+				if err != nil {
+					return true, nil, err
+				}
 
-			boundaryID:     boundaryID,
-			onExitTemplate: true,
-		})
-		woc.addChildNode(parentNode.Name, onExitNodeName)
-		return true, onExitNode, err
+			}
+			onExitNode, err := woc.executeTemplate(ctx, onExitNodeName, &wfv1.WorkflowStep{Template: exitHook.Template, TemplateRef: exitHook.TemplateRef}, tmplCtx, resolvedArgs, &executeTemplateOpts{
+
+				boundaryID:     boundaryID,
+				onExitTemplate: true,
+			})
+			woc.addChildNode(parentNode.Name, onExitNodeName)
+			return true, onExitNode, err
+		}
 	}
 	return false, nil, nil
 }

diff --git a/workflow/controller/hooks.go b/workflow/controller/hooks.go
@@ -13,7 +13,7 @@ import (
 
 func (woc *wfOperationCtx) executeWfLifeCycleHook(ctx context.Context, tmplCtx *templateresolution.Context) error {
 	for hookName, hook := range woc.execWf.Spec.Hooks {
-		//exit hook will be execute in runOnExitNode
+		//exit hook will be executed in runOnExitNode
 		if hookName == wfv1.ExitLifecycleEvent {
 			continue
 		}