From 0fa20c7ba317d8c9a837bcc37d92f3fe79808499 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Mon, 5 Aug 2019 17:26:18 +0900 Subject: [PATCH] Fix validation (#1508) --- workflow/common/util.go | 15 ++++--- workflow/validate/validate.go | 43 ++++++++++-------- workflow/validate/validate_dag_test.go | 14 +++--- workflow/validate/validate_test.go | 60 +++++++++++++++++++++++++- 4 files changed, 99 insertions(+), 33 deletions(-) diff --git a/workflow/common/util.go b/workflow/common/util.go index f89372526d59..37eb8386f254 100644 --- a/workflow/common/util.go +++ b/workflow/common/util.go @@ -244,8 +244,8 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa // 1) check if was supplied as argument. if so use the supplied value from arg // 2) if not, use default value. // 3) if no default value, it is an error - tmpl = tmpl.DeepCopy() - for i, inParam := range tmpl.Inputs.Parameters { + newTmpl := tmpl.DeepCopy() + for i, inParam := range newTmpl.Inputs.Parameters { if inParam.Default != nil { // first set to default value inParam.Value = inParam.Default @@ -259,12 +259,12 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa if inParam.Value == nil { return nil, errors.Errorf(errors.CodeBadRequest, "inputs.parameters.%s was not supplied", inParam.Name) } - tmpl.Inputs.Parameters[i] = inParam + newTmpl.Inputs.Parameters[i] = inParam } // Performs substitutions of input artifacts - newInputArtifacts := make([]wfv1.Artifact, len(tmpl.Inputs.Artifacts)) - for i, inArt := range tmpl.Inputs.Artifacts { + newInputArtifacts := make([]wfv1.Artifact, len(newTmpl.Inputs.Artifacts)) + for i, inArt := range newTmpl.Inputs.Artifacts { // if artifact has hard-wired location, we prefer that if inArt.HasLocation() { newInputArtifacts[i] = inArt @@ -288,9 +288,10 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa newInputArtifacts[i] = inArt } } - tmpl.Inputs.Artifacts = newInputArtifacts + newTmpl.Inputs.Artifacts = newInputArtifacts + + return substituteParams(newTmpl, globalParams, localParams) - return substituteParams(tmpl, globalParams, localParams) } // substituteParams returns a new copy of the template with global, pod, and input parameters substituted diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index 34212a494037..127fd2d2c641 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -79,7 +79,13 @@ func ValidateWorkflow(wf *wfv1.Workflow, opts ValidateOpts) error { ctx.globalParams[common.GlobalVarWorkflowNamespace] = placeholderValue ctx.globalParams[common.GlobalVarWorkflowUID] = placeholderValue for _, param := range ctx.wf.Spec.Arguments.Parameters { - ctx.globalParams["workflow.parameters."+param.Name] = placeholderValue + if param.Name != "" { + if param.Value != nil { + ctx.globalParams["workflow.parameters."+param.Name] = *param.Value + } else { + ctx.globalParams["workflow.parameters."+param.Name] = placeholderValue + } + } } for k := range ctx.wf.ObjectMeta.Annotations { @@ -96,7 +102,7 @@ func ValidateWorkflow(wf *wfv1.Workflow, opts ValidateOpts) error { if entryTmpl == nil { return errors.Errorf(errors.CodeBadRequest, "spec.entrypoint template '%s' undefined", ctx.wf.Spec.Entrypoint) } - err = ctx.validateTemplate(entryTmpl, ctx.wf.Spec.Arguments) + err = ctx.validateTemplate(entryTmpl, ctx.wf.Spec.Arguments, map[string]interface{}{}) if err != nil { return err } @@ -107,7 +113,7 @@ func ValidateWorkflow(wf *wfv1.Workflow, opts ValidateOpts) error { } // now when validating onExit, {{workflow.status}} is now available as a global ctx.globalParams[common.GlobalVarWorkflowStatus] = placeholderValue - err = ctx.validateTemplate(exitTmpl, ctx.wf.Spec.Arguments) + err = ctx.validateTemplate(exitTmpl, ctx.wf.Spec.Arguments, map[string]interface{}{}) if err != nil { return err } @@ -115,7 +121,7 @@ func ValidateWorkflow(wf *wfv1.Workflow, opts ValidateOpts) error { return nil } -func (ctx *wfValidationCtx) validateTemplate(tmpl *wfv1.Template, args wfv1.Arguments) error { +func (ctx *wfValidationCtx) validateTemplate(tmpl *wfv1.Template, args wfv1.Arguments, extraScope map[string]interface{}) error { _, ok := ctx.results[tmpl.Name] if ok { // we already processed this template @@ -125,7 +131,7 @@ func (ctx *wfValidationCtx) validateTemplate(tmpl *wfv1.Template, args wfv1.Argu if err := validateTemplateType(tmpl); err != nil { return err } - scope, err := validateInputs(tmpl) + scope, err := validateInputs(tmpl, extraScope) if err != nil { return err } @@ -147,34 +153,34 @@ func (ctx *wfValidationCtx) validateTemplate(tmpl *wfv1.Template, args wfv1.Argu } } - _, err = common.ProcessArgs(tmpl, args, ctx.globalParams, localParams, true) + newTmpl, err := common.ProcessArgs(tmpl, args, ctx.globalParams, localParams, true) if err != nil { return errors.Errorf(errors.CodeBadRequest, "templates.%s %s", tmpl.Name, err) } for globalVar, val := range ctx.globalParams { scope[globalVar] = val } - switch tmpl.GetType() { + switch newTmpl.GetType() { case wfv1.TemplateTypeSteps: - err = ctx.validateSteps(scope, tmpl) + err = ctx.validateSteps(scope, newTmpl) case wfv1.TemplateTypeDAG: - err = ctx.validateDAG(scope, tmpl) + err = ctx.validateDAG(scope, newTmpl) default: - err = validateLeaf(scope, tmpl) + err = validateLeaf(scope, newTmpl) } if err != nil { return err } - err = validateOutputs(scope, tmpl) + err = validateOutputs(scope, newTmpl) if err != nil { return err } - err = ctx.validateBaseImageOutputs(tmpl) + err = ctx.validateBaseImageOutputs(newTmpl) if err != nil { return err } - if tmpl.ArchiveLocation != nil { - err = validateArtifactLocation("templates.archiveLocation", *tmpl.ArchiveLocation) + if newTmpl.ArchiveLocation != nil { + err = validateArtifactLocation("templates.archiveLocation", *newTmpl.ArchiveLocation) if err != nil { return err } @@ -200,7 +206,7 @@ func validateTemplateType(tmpl *wfv1.Template) error { return nil } -func validateInputs(tmpl *wfv1.Template) (map[string]interface{}, error) { +func validateInputs(tmpl *wfv1.Template, extraScope map[string]interface{}) (map[string]interface{}, error) { err := validateWorkflowFieldNames(tmpl.Inputs.Parameters) if err != nil { return nil, errors.Errorf(errors.CodeBadRequest, "templates.%s.inputs.parameters%s", tmpl.Name, err.Error()) @@ -210,6 +216,9 @@ func validateInputs(tmpl *wfv1.Template) (map[string]interface{}, error) { return nil, errors.Errorf(errors.CodeBadRequest, "templates.%s.inputs.artifacts%s", tmpl.Name, err.Error()) } scope := make(map[string]interface{}) + for name, value := range extraScope { + scope[name] = value + } for _, param := range tmpl.Inputs.Parameters { scope[fmt.Sprintf("inputs.parameters.%s", param.Name)] = true } @@ -429,7 +438,7 @@ func (ctx *wfValidationCtx) validateSteps(scope map[string]interface{}, tmpl *wf if err != nil { return err } - err = ctx.validateTemplate(childTmpl, step.Arguments) + err = ctx.validateTemplate(childTmpl, step.Arguments, scope) if err != nil { return err } @@ -795,7 +804,7 @@ func (ctx *wfValidationCtx) validateDAG(scope map[string]interface{}, tmpl *wfv1 return err } taskTmpl := ctx.wf.GetTemplate(task.Template) - err = ctx.validateTemplate(taskTmpl, task.Arguments) + err = ctx.validateTemplate(taskTmpl, task.Arguments, scope) if err != nil { return err } diff --git a/workflow/validate/validate_dag_test.go b/workflow/validate/validate_dag_test.go index 1fcb66ff9ede..1beb9c60bd32 100644 --- a/workflow/validate/validate_dag_test.go +++ b/workflow/validate/validate_dag_test.go @@ -116,7 +116,7 @@ spec: - name: A template: echo arguments: - parameters: + parameters: - name: message value: val - name: B @@ -154,14 +154,14 @@ spec: - name: A template: echo arguments: - parameters: + parameters: - name: message value: val - name: B dependencies: [A] template: echo arguments: - parameters: + parameters: - name: message value: "{{tasks.A.outputs.parameters.hosts}}" - name: C @@ -199,14 +199,14 @@ spec: - name: A template: echo arguments: - parameters: + parameters: - name: message value: val - name: B dependencies: [A] template: echo arguments: - parameters: + parameters: - name: message value: "{{tasks.A.outputs.parameters.hosts}}" - name: C @@ -246,7 +246,7 @@ spec: command: [echo, generate] outputs: artifacts: - - name: hosts + - name: generated_hosts path: /etc/hosts - name: echo @@ -282,7 +282,7 @@ spec: value: val artifacts: - name: passthrough - from: "{{tasks.A.outputs.artifacts.hosts}}" + from: "{{tasks.A.outputs.artifacts.generated_hosts}}" ` func TestDAGArtifactResolution(t *testing.T) { diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go index 104d471c992a..9079c4639e83 100644 --- a/workflow/validate/validate_test.go +++ b/workflow/validate/validate_test.go @@ -239,7 +239,7 @@ var stepOutputReferences = ` apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: - generateName: hello-world- + generateName: step-output-ref- spec: entrypoint: whalesay templates: @@ -267,11 +267,67 @@ spec: value: "{{steps.one.outparam}}" ` -func TestStepReference(t *testing.T) { +func TestStepOutputReference(t *testing.T) { err := validate(stepOutputReferences) assert.Nil(t, err) } +var stepArtReferences = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: step-art-ref- +spec: + entrypoint: stepref + templates: + - name: generate + container: + image: alpine:3.7 + command: [echo, generate] + outputs: + artifacts: + - name: generated_hosts + path: /etc/hosts + + - name: echo + inputs: + parameters: + - name: message + artifacts: + - name: passthrough + path: /tmp/passthrough + container: + image: alpine:3.7 + command: [echo, "{{inputs.parameters.message}}"] + outputs: + parameters: + - name: hosts + valueFrom: + path: /etc/hosts + artifacts: + - name: someoutput + path: /tmp/passthrough + + - name: stepref + steps: + - - name: one + template: generate + - - name: two + template: echo + arguments: + parameters: + - name: message + value: val + artifacts: + - name: passthrough + from: "{{steps.one.outputs.artifacts.generated_hosts}}" +` + +func TestStepArtReference(t *testing.T) { + err := validate(stepArtReferences) + assert.Nil(t, err) +} + var unsatisfiedParam = ` apiVersion: argoproj.io/v1alpha1 kind: Workflow