From 60b6b5cf64adec380bc195aa87e4f0b12182fe16 Mon Sep 17 00:00:00 2001 From: Saravanan Balasubramanian <33908564+sarabala1979@users.noreply.github.com> Date: Mon, 24 May 2021 07:59:16 -0700 Subject: [PATCH] fix(controller): Empty global output param crashes (#5931) Signed-off-by: Saravanan Balasubramanian --- cmd/argo/commands/cron/get.go | 4 +- cmd/argo/commands/get.go | 4 +- pkg/apis/workflow/v1alpha1/workflow_types.go | 14 +++ .../workflow/v1alpha1/workflow_types_test.go | 18 ++++ workflow/controller/operator.go | 8 +- workflow/controller/operator_test.go | 97 +++++++++++++++++++ 6 files changed, 140 insertions(+), 5 deletions(-) diff --git a/cmd/argo/commands/cron/get.go b/cmd/argo/commands/cron/get.go index 99b478aee546..3b488e23b4ae 100644 --- a/cmd/argo/commands/cron/get.go +++ b/cmd/argo/commands/cron/get.go @@ -106,10 +106,10 @@ func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string { if len(cwf.Spec.WorkflowSpec.Arguments.Parameters) > 0 { out += fmt.Sprintf(fmtStr, "Workflow Parameters:", "") for _, param := range cwf.Spec.WorkflowSpec.Arguments.Parameters { - if param.Value == nil { + if !param.HasValue() { continue } - out += fmt.Sprintf(fmtStr, " "+param.Name+":", *param.Value) + out += fmt.Sprintf(fmtStr, " "+param.Name+":", param.GetValue()) } } return out diff --git a/cmd/argo/commands/get.go b/cmd/argo/commands/get.go index eeb11ef291d5..885b94fe4b38 100644 --- a/cmd/argo/commands/get.go +++ b/cmd/argo/commands/get.go @@ -162,7 +162,9 @@ func printWorkflowHelper(wf *wfv1.Workflow, getArgs getFlags) string { if len(wf.Status.Outputs.Parameters) > 0 { out += fmt.Sprintf(fmtStr, "Output Parameters:", "") for _, param := range wf.Status.Outputs.Parameters { - out += fmt.Sprintf(fmtStr, " "+param.Name+":", *param.Value) + if param.HasValue() { + out += fmt.Sprintf(fmtStr, " "+param.Name+":", param.GetValue()) + } } } if len(wf.Status.Outputs.Artifacts) > 0 { diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go index d4a801d026e6..bff559b8fe09 100644 --- a/pkg/apis/workflow/v1alpha1/workflow_types.go +++ b/pkg/apis/workflow/v1alpha1/workflow_types.go @@ -774,6 +774,20 @@ type ValueFrom struct { Expression string `json:"expression,omitempty" protobuf:"bytes,8,rep,name=expression"` } +func (p *Parameter) HasValue() bool { + return p.Value != nil || p.Default != nil || p.ValueFrom != nil +} + +func (p *Parameter) GetValue() string { + if p.Value != nil { + return p.Value.String() + } + if p.Default != nil { + return p.Default.String() + } + return "" +} + // SuppliedValueFrom is a placeholder for a value to be filled in directly, either through the CLI, API, etc. type SuppliedValueFrom struct{} diff --git a/pkg/apis/workflow/v1alpha1/workflow_types_test.go b/pkg/apis/workflow/v1alpha1/workflow_types_test.go index f7b6d5a83277..369b60f306ba 100644 --- a/pkg/apis/workflow/v1alpha1/workflow_types_test.go +++ b/pkg/apis/workflow/v1alpha1/workflow_types_test.go @@ -890,3 +890,21 @@ func TestHasChild(t *testing.T) { assert.False(t, node.HasChild("c")) assert.False(t, node.HasChild("")) } + +func TestParameterGetValue(t *testing.T) { + empty := Parameter{} + defaultVal := Parameter{Default: AnyStringPtr("Default")} + value := Parameter{Value: AnyStringPtr("Test")} + + valueFrom := Parameter{ValueFrom: &ValueFrom{}} + assert.False(t, empty.HasValue()) + assert.Empty(t, empty.GetValue()) + assert.True(t, defaultVal.HasValue()) + assert.NotEmpty(t, defaultVal.GetValue()) + assert.Equal(t, "Default", defaultVal.GetValue()) + assert.True(t, value.HasValue()) + assert.NotEmpty(t, value.GetValue()) + assert.Equal(t, "Test", value.GetValue()) + assert.True(t, valueFrom.HasValue()) + +} diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index cf73011f49cd..edefaead6a39 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -494,7 +494,9 @@ func (woc *wfOperationCtx) setGlobalParameters(executionParameters wfv1.Argument } if woc.wf.Status.Outputs != nil { for _, param := range woc.wf.Status.Outputs.Parameters { - woc.globalParams["workflow.outputs.parameters."+param.Name] = param.Value.String() + if param.HasValue() { + woc.globalParams["workflow.outputs.parameters."+param.Name] = param.GetValue() + } } } } @@ -2610,7 +2612,9 @@ func (woc *wfOperationCtx) addParamToGlobalScope(param wfv1.Parameter) { return } paramName := fmt.Sprintf("workflow.outputs.parameters.%s", param.GlobalName) - woc.globalParams[paramName] = param.Value.String() + if param.HasValue() { + woc.globalParams[paramName] = param.GetValue() + } wfUpdated := wfutil.AddParamToGlobalScope(woc.wf, woc.log, param) if wfUpdated { woc.updated = true diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go index 0ae0c2d9cb23..b62b3ad457a6 100644 --- a/workflow/controller/operator_test.go +++ b/workflow/controller/operator_test.go @@ -6785,3 +6785,100 @@ func TestMutexWfPendingWithNoPod(t *testing.T) { assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase) assert.Equal(t, wfv1.NodePending, woc.wf.Status.Nodes.FindByDisplayName("hello-world-mpdht").Phase) } + +var wfGlopalArtifactNil = `apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + name: global-outputs-ttsfq + namespace: default +spec: + entrypoint: generate-globals + onExit: consume-globals + templates: + - name: generate-globals + steps: + - - name: generate + template: global-output + - - name: consume-globals + template: consume-globals + - container: + args: + - sleep 1; exit 1 + command: + - sh + - -c + image: alpine:3.7 + name: global-output + outputs: + artifacts: + - globalName: my-global-art + name: hello-art + path: /tmp/hello_world.txt + parameters: + - globalName: my-global-param + name: hello-param + valueFrom: + path: /tmp/hello_world.txt + - name: consume-globals + steps: + - - name: consume-global-param + template: consume-global-param + - arguments: + artifacts: + - from: '{{workflow.outputs.artifacts.my-global-art}}' + name: art + name: consume-global-art + template: consume-global-art + - container: + args: + - echo {{inputs.parameters.param}} + command: + - sh + - -c + image: alpine:3.7 + inputs: + parameters: + - name: param + value: '{{workflow.outputs.parameters.my-global-param}}' + name: consume-global-param + - container: + args: + - cat /art + command: + - sh + - -c + image: alpine:3.7 + inputs: + artifacts: + - name: art + path: /art + name: consume-global-art +` + +func TestWFGlobalArtifactNil(t *testing.T) { + wf := wfv1.MustUnmarshalWorkflow(wfGlopalArtifactNil) + cancel, controller := newController(wf) + defer cancel() + + ctx := context.Background() + woc := newWorkflowOperationCtx(wf, controller) + woc.operate(ctx) + makePodsPhase(ctx, woc, apiv1.PodRunning) + woc.operate(ctx) + makePodsPhase(ctx, woc, apiv1.PodFailed, func(pod *apiv1.Pod) { + pod.Annotations[common.AnnotationKeyOutputs] = string("{\"parameters\":[{\"name\":\"hello-param\",\"valueFrom\":{\"path\":\"/tmp/hello_world.txt\"},\"globalName\":\"my-global-param\"}],\"artifacts\":[{\"name\":\"hello-art\",\"path\":\"/tmp/hello_world.txt\",\"globalName\":\"my-global-art\"}]}") + pod.Status.ContainerStatuses = []apiv1.ContainerStatus{ + { + Name: "main", + State: apiv1.ContainerState{ + Terminated: &apiv1.ContainerStateTerminated{ + ExitCode: 1, + Message: "", + }, + }, + }, + } + }) + woc.operate(ctx) + assert.NotPanics(t, func() { woc.operate(ctx) }) +}