fix(validate): Local parameters should be validated locally. Fixes ar…

…goproj#4326 (argoproj#4358) Signed-off-by: conanoc <[email protected]>
jlegrone · Oct 28, 2020 · e8f8261 · e8f8261
1 parent ddd45b6
commit e8f8261
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 15 deletions.
diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
@@ -222,7 +222,7 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
  if hasWorkflowTemplateRef {
  tmpl = &wfv1.WorkflowStep{TemplateRef: wfTmplRef}
  }
- _, err = ctx.validateTemplateHolder(tmpl, tmplCtx, args, map[string]interface{}{})
+ _, err = ctx.validateTemplateHolder(tmpl, tmplCtx, args)
  if err != nil {
  return nil, err
  }
@@ -231,7 +231,7 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
  // now when validating onExit, {{workflow.status}} is now available as a global
  ctx.globalParams[common.GlobalVarWorkflowStatus] = placeholderGenerator.NextPlaceholder()
  ctx.globalParams[common.GlobalVarWorkflowFailures] = placeholderGenerator.NextPlaceholder()
- _, err = ctx.validateTemplateHolder(&wfv1.WorkflowStep{Template: wf.Spec.OnExit}, tmplCtx, &wf.Spec.Arguments, map[string]interface{}{})
+ _, err = ctx.validateTemplateHolder(&wfv1.WorkflowStep{Template: wf.Spec.OnExit}, tmplCtx, &wf.Spec.Arguments)
  if err != nil {
  return nil, err
  }
@@ -247,7 +247,7 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
 
  // Check if all templates can be resolved.
  for _, template := range wf.Spec.Templates {
- _, err := ctx.validateTemplateHolder(&wfv1.WorkflowStep{Template: template.Name}, tmplCtx, &FakeArguments{}, map[string]interface{}{})
+ _, err := ctx.validateTemplateHolder(&wfv1.WorkflowStep{Template: template.Name}, tmplCtx, &FakeArguments{})
  if err != nil {
  return nil, errors.Errorf(errors.CodeBadRequest, "templates.%s %s", template.Name, err.Error())
  }
@@ -358,12 +358,12 @@ For more information, see: %s
  return out
 }
 
-func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx *templateresolution.Context, args wfv1.ArgumentsProvider, extraScope map[string]interface{}) error {
+func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx *templateresolution.Context, args wfv1.ArgumentsProvider) error {
  if err := validateTemplateType(tmpl); err != nil {
  return err
  }
 
- scope, err := validateInputs(tmpl, extraScope)
+ scope, err := validateInputs(tmpl)
  if err != nil {
  return err
  }
@@ -458,7 +458,7 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
 }
 
 // validateTemplateHolder validates a template holder and returns the validated template.
-func (ctx *templateValidationCtx) validateTemplateHolder(tmplHolder wfv1.TemplateReferenceHolder, tmplCtx *templateresolution.Context, args wfv1.ArgumentsProvider, extraScope map[string]interface{}) (*wfv1.Template, error) {
+func (ctx *templateValidationCtx) validateTemplateHolder(tmplHolder wfv1.TemplateReferenceHolder, tmplCtx *templateresolution.Context, args wfv1.ArgumentsProvider) (*wfv1.Template, error) {
  tmplRef := tmplHolder.GetTemplateRef()
  tmplName := tmplHolder.GetTemplateName()
  if tmplRef != nil {
@@ -512,7 +512,7 @@ func (ctx *templateValidationCtx) validateTemplateHolder(tmplHolder wfv1.Templat
  }
  }
 
- return resolvedTmpl, ctx.validateTemplate(resolvedTmpl, tmplCtx, args, extraScope)
+ return resolvedTmpl, ctx.validateTemplate(resolvedTmpl, tmplCtx, args)
 }
 
 // validateTemplateType validates that only one template type is defined
@@ -537,7 +537,7 @@ func validateTemplateType(tmpl *wfv1.Template) error {
  return nil
 }
 
-func validateInputs(tmpl *wfv1.Template, extraScope map[string]interface{}) (map[string]interface{}, error) {
+func validateInputs(tmpl *wfv1.Template) (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())
@@ -547,9 +547,6 @@ func validateInputs(tmpl *wfv1.Template, extraScope map[string]interface{}) (map
  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
  }
@@ -815,7 +812,7 @@ func (ctx *templateValidationCtx) validateSteps(scope map[string]interface{}, tm
  if err != nil {
  return err
  }
- resolvedTmpl, err := ctx.validateTemplateHolder(&step, tmplCtx, &FakeArguments{}, scope)
+ resolvedTmpl, err := ctx.validateTemplateHolder(&step, tmplCtx, &FakeArguments{})
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.steps[%d].%s %s", tmpl.Name, i, step.Name, err.Error())
  }
@@ -837,7 +834,7 @@ func (ctx *templateValidationCtx) validateSteps(scope map[string]interface{}, tm
  ctx.addOutputsToScope(resolvedTmpl, fmt.Sprintf("steps.%s", step.Name), scope, aggregate, false)
 
  // Validate the template again with actual arguments.
- _, err = ctx.validateTemplateHolder(&step, tmplCtx, &step.Arguments, scope)
+ _, err = ctx.validateTemplateHolder(&step, tmplCtx, &step.Arguments)
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.steps[%d].%s %s", tmpl.Name, i, step.Name, err.Error())
  }
@@ -1216,7 +1213,7 @@ func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmpl
  return errors.Errorf(errors.CodeBadRequest, "templates.%s cannot use 'continueOn' when using 'depends'. Instead use 'dep-task.Failed'/'dep-task.Errored'", tmpl.Name)
  }
 
- resolvedTmpl, err := ctx.validateTemplateHolder(&task, tmplCtx, &FakeArguments{}, map[string]interface{}{})
+ resolvedTmpl, err := ctx.validateTemplateHolder(&task, tmplCtx, &FakeArguments{})
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.tasks.%s %s", tmpl.Name, task.Name, err.Error())
  }
@@ -1286,7 +1283,7 @@ func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmpl
  return err
  }
  // Validate the template again with actual arguments.
- _, err = ctx.validateTemplateHolder(&task, tmplCtx, &task.Arguments, scope)
+ _, err = ctx.validateTemplateHolder(&task, tmplCtx, &task.Arguments)
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.tasks.%s %s", tmpl.Name, task.Name, err.Error())
  }

diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
@@ -170,6 +170,35 @@ spec:
  image: docker/whalesay:{{inputs.parameters.unresolved}}
 `
 
+var unresolvedStepInput = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: hello-world-
+spec:
+ entrypoint: entry-step
+ arguments:
+ parameters: []
+ templates:
+ - steps:
+ - - name: a
+ arguments:
+ parameters:
+ - name: message
+ value: "{{inputs.parameters.message}}"
+ template: whalesay
+ name: entry-step
+ inputs:
+ parameters:
+ - name: message
+ value: hello world
+ - name: whalesay
+ container:
+ image: docker/whalesay
+ command: [cowsay]
+ args: ["{{inputs.parameters.message}}"]
+`
+
 var unresolvedOutput = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow
@@ -197,6 +226,10 @@ func TestUnresolved(t *testing.T) {
  if assert.NotNil(t, err) {
  assert.Contains(t, err.Error(), "failed to resolve")
  }
+ _, err = validate(unresolvedStepInput)
+ if assert.NotNil(t, err) {
+ assert.Contains(t, err.Error(), "failed to resolve")
+ }
  _, err = validate(unresolvedOutput)
  if assert.NotNil(t, err) {
  assert.Contains(t, err.Error(), "failed to resolve")