Skip to content

Commit

Permalink
fix: allow wf templates without parameter values (Fixes #6044) (#7124)
Browse files Browse the repository at this point in the history 
* fix: allow wf templates without parameter values

Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
  • Loading branch information
@kennytrytek
kennytrytek authored Nov 9, 2021
1 parent 225a5a3 commit 17fb9d8
Show file tree
Hide file tree
Showing 9 changed files with 130 additions and 54 deletions.
6 changes: 3 additions & 3 deletions server/clusterworkflowtemplate/cluster_workflow_template_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
53 changes: 41 additions & 12 deletions server/clusterworkflowtemplate/cluster_workflow_template_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion server/workflow/workflow_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
21 changes: 17 additions & 4 deletions server/workflow/workflow_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,7 @@ const workflowtmpl = `
"arguments": {
"parameters": [
{
"name": "message",
"value": "hello world"
"name": "message"
}
]
},
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions server/workflowtemplate/workflow_template_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
12 changes: 12 additions & 0 deletions server/workflowtemplate/workflow_template_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions workflow/util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down
44 changes: 27 additions & 17 deletions workflow/validate/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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))
}
Expand All @@ -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))
}
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -1266,7 +1276,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
}
Expand Down
Loading

0 comments on commit 17fb9d8

Please sign in to comment.