From 9317360f2ef398de232c217dfdf71219b7a2fa41 Mon Sep 17 00:00:00 2001 From: Isitha Subasinghe Date: Wed, 23 Aug 2023 10:51:23 +1000 Subject: [PATCH] fix: do not process withParams when task/step Skipped. Fixes #10173 (#11570) Signed-off-by: Isitha Subasinghe --- workflow/controller/dag.go | 13 +++++++++++-- workflow/controller/steps.go | 13 +++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/workflow/controller/dag.go b/workflow/controller/dag.go index aba88e1285e9..024c89524b8d 100644 --- a/workflow/controller/dag.go +++ b/workflow/controller/dag.go @@ -758,6 +758,9 @@ func (d *dagContext) findLeafTaskNames(tasks []wfv1.DAGTask) []string { } // expandTask expands a single DAG task containing withItems, withParams, withSequence into multiple parallel tasks +// We want to be lazy with expanding. Unfortunately this is not quite possible as the When field might rely on +// expansion to work with the shouldExecute function. To address this we apply a trick, we try to expand, if we fail, we then +// check shouldExecute, if shouldExecute returns false, we continue on as normal else error out func expandTask(task wfv1.DAGTask) ([]wfv1.DAGTask, error) { var err error var items []wfv1.Item @@ -766,12 +769,18 @@ func expandTask(task wfv1.DAGTask) ([]wfv1.DAGTask, error) { } else if task.WithParam != "" { err = json.Unmarshal([]byte(task.WithParam), &items) if err != nil { - return nil, errors.Errorf(errors.CodeBadRequest, "withParam value could not be parsed as a JSON list: %s: %v", strings.TrimSpace(task.WithParam), err) + mustExec, mustExecErr := shouldExecute(task.When) + if mustExecErr != nil || mustExec { + return nil, errors.Errorf(errors.CodeBadRequest, "withParam value could not be parsed as a JSON list: %s: %v", strings.TrimSpace(task.WithParam), err) + } } } else if task.WithSequence != nil { items, err = expandSequence(task.WithSequence) if err != nil { - return nil, err + mustExec, mustExecErr := shouldExecute(task.When) + if mustExecErr != nil || mustExec { + return nil, err + } } } else { return []wfv1.DAGTask{task}, nil diff --git a/workflow/controller/steps.go b/workflow/controller/steps.go index 378c07a382ff..09ae0173603f 100644 --- a/workflow/controller/steps.go +++ b/workflow/controller/steps.go @@ -498,6 +498,9 @@ func (woc *wfOperationCtx) expandStepGroup(sgNodeName string, stepGroup []wfv1.W } // expandStep expands a step containing withItems or withParams into multiple parallel steps +// We want to be lazy with expanding. Unfortunately this is not quite possible as the When field might rely on +// expansion to work with the shouldExecute function. To address this we apply a trick, we try to expand, if we fail, we then +// check shouldExecute, if shouldExecute returns false, we continue on as normal else error out func (woc *wfOperationCtx) expandStep(step wfv1.WorkflowStep) ([]wfv1.WorkflowStep, error) { var err error expandedStep := make([]wfv1.WorkflowStep, 0) @@ -507,12 +510,18 @@ func (woc *wfOperationCtx) expandStep(step wfv1.WorkflowStep) ([]wfv1.WorkflowSt } else if step.WithParam != "" { err = json.Unmarshal([]byte(step.WithParam), &items) if err != nil { - return nil, errors.Errorf(errors.CodeBadRequest, "withParam value could not be parsed as a JSON list: %s: %v", strings.TrimSpace(step.WithParam), err) + mustExec, mustExecErr := shouldExecute(step.When) + if mustExecErr != nil || mustExec { + return nil, errors.Errorf(errors.CodeBadRequest, "withParam value could not be parsed as a JSON list: %s: %v", strings.TrimSpace(step.WithParam), err) + } } } else if step.WithSequence != nil { items, err = expandSequence(step.WithSequence) if err != nil { - return nil, err + mustExec, mustExecErr := shouldExecute(step.When) + if mustExecErr != nil || mustExec { + return nil, err + } } } else { // this should have been prevented in expandStepGroup()