fix: allow wf templates without parameter values (Fixes #6044) (#7124)

* fix: allow wf templates without parameter values

Signed-off-by: Kenny Trytek <[email protected]>

server/clusterworkflowtemplate/cluster_workflow_template_server.go

@@ -32,7 +32,7 @@ func (cwts *ClusterWorkflowTemplateServer) CreateClusterWorkflowTemplate(ctx con
  cwts.instanceIDService.Label(req.Template)
  creator.Label(ctx, req.Template)
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
- _, err := validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template)
+ _, err := validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template, validate.ValidateOpts{})
  if err != nil {
  return nil, err
  }
@@ -97,7 +97,7 @@ func (cwts *ClusterWorkflowTemplateServer) LintClusterWorkflowTemplate(ctx conte
  wfClient := auth.GetWfClient(ctx)
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
 
- _, err := validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template)
+ _, err := validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template, validate.ValidateOpts{Lint: true})
  if err != nil {
  return nil, err
  }
@@ -116,7 +116,7 @@ func (cwts *ClusterWorkflowTemplateServer) UpdateClusterWorkflowTemplate(ctx con
  wfClient := auth.GetWfClient(ctx)
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
 
- _, err = validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template)
+ _, err = validate.ValidateClusterWorkflowTemplate(nil, cwftmplGetter, req.Template, validate.ValidateOpts{})
  if err != nil {
  return nil, err
  }

server/clusterworkflowtemplate/cluster_workflow_template_server_test.go

@@ -154,18 +154,33 @@ func getClusterWorkflowTemplateServer() (clusterwftmplpkg.ClusterWorkflowTemplat
 
 func TestWorkflowTemplateServer_CreateClusterWorkflowTemplate(t *testing.T) {
  server, ctx := getClusterWorkflowTemplateServer()
- cwftReq := clusterwftmplpkg.ClusterWorkflowTemplateCreateRequest{
- Template: unlabelled.DeepCopy(),
- }
- cwftReq.Template.Name = "foo"
- assert.NotContains(t, cwftReq.Template.Labels, common.LabelKeyControllerInstanceID)
- cwftRsp, err := server.CreateClusterWorkflowTemplate(ctx, &cwftReq)
- if assert.NoError(t, err) {
- assert.NotNil(t, cwftRsp)
- // ensure the label is added
- assert.Contains(t, cwftRsp.Labels, common.LabelKeyControllerInstanceID)
- assert.Contains(t, cwftRsp.Labels, common.LabelKeyCreator)
- }
+ t.Run("Without parameter values", func(t *testing.T) {
+ tmpl := unlabelled.DeepCopy()
+ tmpl.Name = "foo-without-param-values"
+ tmpl.Spec.Arguments.Parameters[0].Value = nil
+ req := clusterwftmplpkg.ClusterWorkflowTemplateCreateRequest{
+ Template: tmpl,
+ }
+ resp, err := server.CreateClusterWorkflowTemplate(ctx, &req)
+ if assert.NoError(t, err) {
+ assert.Equal(t, "message", resp.Spec.Arguments.Parameters[0].Name)
+ assert.Nil(t, resp.Spec.Arguments.Parameters[0].Value)
+ }
+ })
+ t.Run("With parameter values", func(t *testing.T) {
+ cwftReq := clusterwftmplpkg.ClusterWorkflowTemplateCreateRequest{
+ Template: unlabelled.DeepCopy(),
+ }
+ cwftReq.Template.Name = "foo-with-param-values"
+ assert.NotContains(t, cwftReq.Template.Labels, common.LabelKeyControllerInstanceID)
+ cwftRsp, err := server.CreateClusterWorkflowTemplate(ctx, &cwftReq)
+ if assert.NoError(t, err) {
+ assert.NotNil(t, cwftRsp)
+ // ensure the label is added
+ assert.Contains(t, cwftRsp.Labels, common.LabelKeyControllerInstanceID)
+ assert.Contains(t, cwftRsp.Labels, common.LabelKeyCreator)
+ }
+ })
 }
 
 func TestWorkflowTemplateServer_GetClusterWorkflowTemplate(t *testing.T) {
@@ -228,6 +243,20 @@ func TestWorkflowTemplateServer_LintClusterWorkflowTemplate(t *testing.T) {
  assert.Contains(t, resp.Labels, common.LabelKeyCreator)
  }
  })
+
+ t.Run("Without param values", func(t *testing.T) {
+ tmpl := unlabelled.DeepCopy()
+ tmpl.Name = "foo-without-param-values"
+ tmpl.Spec.Arguments.Parameters[0].Value = nil
+ req := clusterwftmplpkg.ClusterWorkflowTemplateLintRequest{
+ Template: tmpl,
+ }
+ resp, err := server.LintClusterWorkflowTemplate(ctx, &req)
+ if assert.NoError(t, err) {
+ assert.Equal(t, "message", resp.Spec.Arguments.Parameters[0].Name)
+ assert.Nil(t, resp.Spec.Arguments.Parameters[0].Value)
+ }
+ })
 }
 
 func TestWorkflowTemplateServer_UpdateClusterWorkflowTemplate(t *testing.T) {

server/workflow/workflow_server.go

@@ -609,7 +609,7 @@ func (s *workflowServer) SubmitWorkflow(ctx context.Context, req *workflowpkg.Wo
  wftmplGetter := templateresolution.WrapWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().WorkflowTemplates(req.Namespace))
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
 
- _, err = validate.ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, validate.ValidateOpts{})
+ _, err = validate.ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, validate.ValidateOpts{Submit: true})
  if err != nil {
  return nil, err
  }

server/workflow/workflow_server_test.go

@@ -447,8 +447,7 @@ const workflowtmpl = `
  "arguments": {
  "parameters": [
  {
- "name": "message",
- "value": "hello world"
+ "name": "message"
  }
  ]
  },
@@ -880,12 +879,26 @@ func TestPodLogs(t *testing.T) {
 
 func TestSubmitWorkflowFromResource(t *testing.T) {
  server, ctx := getWorkflowServer()
- t.Run("SubmitFromWorkflowTemplate", func(t *testing.T) {
- wf, err := server.SubmitWorkflow(ctx, &workflowpkg.WorkflowSubmitRequest{
+ t.Run("SubmitFromWorkflowTemplate fails if missing parameters", func(t *testing.T) {
+ _, err := server.SubmitWorkflow(ctx, &workflowpkg.WorkflowSubmitRequest{
  Namespace: "workflows",
  ResourceKind: "workflowtemplate",
  ResourceName: "workflow-template-whalesay-template",
  })
+ assert.EqualError(t, err, "spec.arguments.message.value is required")
+ })
+ t.Run("SubmitFromWorkflowTemplate", func(t *testing.T) {
+ opts := v1alpha1.SubmitOpts{
+ Parameters: []string{
+ "message=hello",
+ },
+ }
+ wf, err := server.SubmitWorkflow(ctx, &workflowpkg.WorkflowSubmitRequest{
+ Namespace: "workflows",
+ ResourceKind: "workflowtemplate",
+ ResourceName: "workflow-template-whalesay-template",
+ SubmitOptions: &opts,
+ })
  if assert.NoError(t, err) {
  assert.NotNil(t, wf)
  assert.Contains(t, wf.Labels, common.LabelKeyControllerInstanceID)

server/workflowtemplate/workflow_template_server.go

@@ -33,7 +33,7 @@ func (wts *WorkflowTemplateServer) CreateWorkflowTemplate(ctx context.Context, r
  creator.Label(ctx, req.Template)
  wftmplGetter := templateresolution.WrapWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().WorkflowTemplates(req.Namespace))
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
- _, err := validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template)
+ _, err := validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template, validate.ValidateOpts{})
  if err != nil {
  return nil, err
  }
@@ -93,7 +93,7 @@ func (wts *WorkflowTemplateServer) LintWorkflowTemplate(ctx context.Context, req
  creator.Label(ctx, req.Template)
  wftmplGetter := templateresolution.WrapWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().WorkflowTemplates(req.Namespace))
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
- _, err := validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template)
+ _, err := validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template, validate.ValidateOpts{Lint: true})
  if err != nil {
  return nil, err
  }
@@ -111,7 +111,7 @@ func (wts *WorkflowTemplateServer) UpdateWorkflowTemplate(ctx context.Context, r
  wfClient := auth.GetWfClient(ctx)
  wftmplGetter := templateresolution.WrapWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().WorkflowTemplates(req.Namespace))
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClient.ArgoprojV1alpha1().ClusterWorkflowTemplates())
- _, err = validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template)
+ _, err = validate.ValidateWorkflowTemplate(wftmplGetter, cwftmplGetter, req.Template, validate.ValidateOpts{})
  if err != nil {
  return nil, err
  }

server/workflowtemplate/workflow_template_server_test.go

@@ -172,6 +172,18 @@ func getWorkflowTemplateServer() (workflowtemplatepkg.WorkflowTemplateServiceSer
 
 func TestWorkflowTemplateServer_CreateWorkflowTemplate(t *testing.T) {
  server, ctx := getWorkflowTemplateServer()
+ t.Run("Without parameter values", func(t *testing.T) {
+ var wftReq workflowtemplatepkg.WorkflowTemplateCreateRequest
+ v1alpha1.MustUnmarshal(wftStr1, &wftReq)
+ wftReq.Template.Name = "foo-without-param-values"
+ wftReq.Template.Spec.Arguments.Parameters[0].Value = nil
+ wftRsp, err := server.CreateWorkflowTemplate(ctx, &wftReq)
+ if assert.NoError(t, err) {
+ assert.NotNil(t, wftRsp)
+ assert.Equal(t, "message", wftRsp.Spec.Arguments.Parameters[0].Name)
+ assert.Nil(t, wftRsp.Spec.Arguments.Parameters[0].Value)
+ }
+ })
  t.Run("Labelled", func(t *testing.T) {
  var wftReq workflowtemplatepkg.WorkflowTemplateCreateRequest
  v1alpha1.MustUnmarshal(wftStr1, &wftReq)

workflow/util/util.go

@@ -169,7 +169,7 @@ func IsWorkflowCompleted(wf *wfv1.Workflow) bool {
  return false
 }
 
-// SubmitWorkflow validates and submit a single workflow and override some of the fields of the workflow
+// SubmitWorkflow validates and submits a single workflow and overrides some of the fields of the workflow
 func SubmitWorkflow(ctx context.Context, wfIf v1alpha1.WorkflowInterface, wfClientset wfclientset.Interface, namespace string, wf *wfv1.Workflow, opts *wfv1.SubmitOpts) (*wfv1.Workflow, error) {
  err := ApplySubmitOpts(wf, opts)
  if err != nil {
@@ -178,7 +178,7 @@ func SubmitWorkflow(ctx context.Context, wfIf v1alpha1.WorkflowInterface, wfClie
  wftmplGetter := templateresolution.WrapWorkflowTemplateInterface(wfClientset.ArgoprojV1alpha1().WorkflowTemplates(namespace))
  cwftmplGetter := templateresolution.WrapClusterWorkflowTemplateInterface(wfClientset.ArgoprojV1alpha1().ClusterWorkflowTemplates())
 
- _, err = validate.ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, validate.ValidateOpts{})
+ _, err = validate.ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, validate.ValidateOpts{Submit: true})
  if err != nil {
  return nil, err
  }

workflow/validate/validate.go

@@ -33,6 +33,7 @@ type ValidateOpts struct {
  // skip some validations which is permissible during linting but not submission (e.g. missing
  // input parameters to the workflow)
  Lint bool
+
  // ContainerRuntimeExecutor will trigger additional validation checks specific to different
  // types of executors. For example, the inability of kubelet/k8s executors to copy artifacts
  // out of the base image layer. If unspecified, will use docker executor validation
@@ -44,6 +45,10 @@ type ValidateOpts struct {
 
  // WorkflowTemplateValidation indicates that the current context is validating a WorkflowTemplate or ClusterWorkflowTemplate
  WorkflowTemplateValidation bool
+
+ // Submit indicates that the current operation is a workflow submission. This will impose
+ // more stringent requirements (e.g. require input values for all spec arguments)
+ Submit bool
 }
 
 // templateValidationCtx is the context for validating a workflow spec
@@ -149,13 +154,12 @@ func ValidateWorkflow(wftmplGetter templateresolution.WorkflowTemplateNamespaced
  if err != nil {
  return nil, errors.Errorf(errors.CodeBadRequest, "spec.templates%s", err.Error())
  }
- if ctx.Lint {
- // if we are just linting we don't care if spec.arguments.parameters.XXX doesn't have an
- // explicit value. workflows without a default value is a desired use case
- err = validateArgumentsFieldNames("spec.arguments.", wfArgs)
- } else {
- err = validateArguments("spec.arguments.", wfArgs)
- }
+
+ // if we are linting, we don't care if spec.arguments.parameters.XXX doesn't have an
+ // explicit value. Workflow templates without a default value are also a desired use
+ // case, since values will be provided during workflow submission.
+ allowEmptyValues := ctx.Lint || (ctx.WorkflowTemplateValidation && !ctx.Submit)
+ err = validateArguments("spec.arguments.", wfArgs, allowEmptyValues)
  if err != nil {
  return nil, err
  }
@@ -239,7 +243,7 @@ func ValidateWorkflowTemplateRefFields(wfSpec wfv1.WorkflowSpec) error {
 }
 
 // ValidateWorkflowTemplate accepts a workflow template and performs validation against it.
-func ValidateWorkflowTemplate(wftmplGetter templateresolution.WorkflowTemplateNamespacedGetter, cwftmplGetter templateresolution.ClusterWorkflowTemplateGetter, wftmpl *wfv1.WorkflowTemplate) (*wfv1.Conditions, error) {
+func ValidateWorkflowTemplate(wftmplGetter templateresolution.WorkflowTemplateNamespacedGetter, cwftmplGetter templateresolution.ClusterWorkflowTemplateGetter, wftmpl *wfv1.WorkflowTemplate, opts ValidateOpts) (*wfv1.Conditions, error) {
  if len(wftmpl.Name) > maxCharsInObjectName {
  return nil, fmt.Errorf("workflow template name %q must not be more than 63 characters long (currently %d)", wftmpl.Name, len(wftmpl.Name))
  }
@@ -251,11 +255,13 @@ func ValidateWorkflowTemplate(wftmplGetter templateresolution.WorkflowTemplateNa
  },
  Spec: wftmpl.Spec.WorkflowSpec,
  }
- return ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, ValidateOpts{IgnoreEntrypoint: wf.Spec.Entrypoint == "", WorkflowTemplateValidation: true})
+ opts.IgnoreEntrypoint = wf.Spec.Entrypoint == ""
+ opts.WorkflowTemplateValidation = true
+ return ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, opts)
 }
 
 // ValidateClusterWorkflowTemplate accepts a cluster workflow template and performs validation against it.
-func ValidateClusterWorkflowTemplate(wftmplGetter templateresolution.WorkflowTemplateNamespacedGetter, cwftmplGetter templateresolution.ClusterWorkflowTemplateGetter, cwftmpl *wfv1.ClusterWorkflowTemplate) (*wfv1.Conditions, error) {
+func ValidateClusterWorkflowTemplate(wftmplGetter templateresolution.WorkflowTemplateNamespacedGetter, cwftmplGetter templateresolution.ClusterWorkflowTemplateGetter, cwftmpl *wfv1.ClusterWorkflowTemplate, opts ValidateOpts) (*wfv1.Conditions, error) {
  if len(cwftmpl.Name) > maxCharsInObjectName {
  return nil, fmt.Errorf("cluster workflow template name %q must not be more than 63 characters long (currently %d)", cwftmpl.Name, len(cwftmpl.Name))
  }
@@ -267,7 +273,9 @@ func ValidateClusterWorkflowTemplate(wftmplGetter templateresolution.WorkflowTem
  },
  Spec: cwftmpl.Spec.WorkflowSpec,
  }
- return ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, ValidateOpts{IgnoreEntrypoint: wf.Spec.Entrypoint == "", WorkflowTemplateValidation: true})
+ opts.IgnoreEntrypoint = wf.Spec.Entrypoint == ""
+ opts.WorkflowTemplateValidation = true
+ return ValidateWorkflow(wftmplGetter, cwftmplGetter, wf, opts)
 }
 
 // ValidateCronWorkflow validates a CronWorkflow
@@ -680,12 +688,12 @@ func (ctx *templateValidationCtx) validateLeaf(scope map[string]interface{}, tmp
  return nil
 }
 
-func validateArguments(prefix string, arguments wfv1.Arguments) error {
+func validateArguments(prefix string, arguments wfv1.Arguments, allowEmptyValues bool) error {
  err := validateArgumentsFieldNames(prefix, arguments)
  if err != nil {
  return err
  }
- return validateArgumentsValues(prefix, arguments)
+ return validateArgumentsValues(prefix, arguments, allowEmptyValues)
 }
 
 func validateArgumentsFieldNames(prefix string, arguments wfv1.Arguments) error {
@@ -703,10 +711,12 @@ func validateArgumentsFieldNames(prefix string, arguments wfv1.Arguments) error
 }
 
 // validateArgumentsValues ensures that all arguments have parameter values or artifact locations
-func validateArgumentsValues(prefix string, arguments wfv1.Arguments) error {
+func validateArgumentsValues(prefix string, arguments wfv1.Arguments, allowEmptyValues bool) error {
  for _, param := range arguments.Parameters {
  if param.ValueFrom == nil && param.Value == nil {
- return errors.Errorf(errors.CodeBadRequest, "%s%s.value is required", prefix, param.Name)
+ if !allowEmptyValues {
+ return errors.Errorf(errors.CodeBadRequest, "%s%s.value is required", prefix, param.Name)
+ }
  }
  if param.Enum != nil {
  if len(param.Enum) == 0 {
@@ -761,7 +771,7 @@ func (ctx *templateValidationCtx) validateSteps(scope map[string]interface{}, tm
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.steps[%d].%s %s", tmpl.Name, i, step.Name, err.Error())
  }
- err = validateArguments(fmt.Sprintf("templates.%s.steps[%d].%s.arguments.", tmpl.Name, i, step.Name), step.Arguments)
+ err = validateArguments(fmt.Sprintf("templates.%s.steps[%d].%s.arguments.", tmpl.Name, i, step.Name), step.Arguments, false)
  if err != nil {
  return err
  }
@@ -1265,7 +1275,7 @@ func (ctx *templateValidationCtx) validateDAG(scope map[string]interface{}, tmpl
  if err != nil {
  return errors.Errorf(errors.CodeBadRequest, "templates.%s.tasks.%s %s", tmpl.Name, task.Name, err.Error())
  }
- err = validateArguments(fmt.Sprintf("templates.%s.tasks.%s.arguments.", tmpl.Name, task.Name), task.Arguments)
+ err = validateArguments(fmt.Sprintf("templates.%s.tasks.%s.arguments.", tmpl.Name, task.Name), task.Arguments, false)
  if err != nil {
  return err
  }