diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index 09c18d954d9..b1a752981f9 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -145,39 +145,25 @@ func NewArrayOrString(value string, values ...string) *ArrayOrString { } } -func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError { +func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) { for _, param := range params { if param.Value.Type == ParamTypeString { - if err := validateStringVariableInTaskParameters(fmt.Sprintf("[%s]", param.Name), param.Value.StringVal, prefix, paramNames, arrayParamNames); err != nil { - return err - } + errs = errs.Also(validateStringVariableInTaskParameters(param.Value.StringVal, prefix, paramNames, arrayParamNames).ViaFieldKey("params", param.Name)) } else { - for _, arrayElement := range param.Value.ArrayVal { - if err := validateArrayVariableInTaskParameters(fmt.Sprintf("[%s]", param.Name), arrayElement, prefix, paramNames, arrayParamNames); err != nil { - return err - } + for idx, arrayElement := range param.Value.ArrayVal { + errs = errs.Also(validateArrayVariableInTaskParameters(arrayElement, prefix, paramNames, arrayParamNames).ViaFieldIndex("value", idx).ViaFieldKey("params", param.Name)) } } } - return nil + return errs } -func validateStringVariableInTaskParameters(name, value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { - if err := substitution.ValidateVariable(name, value, prefix, "task parameter", "pipelinespec.params", stringVars); err != nil { - return err - } - if err := substitution.ValidateVariableProhibited(name, value, prefix, "task parameter", "pipelinespec.params", arrayVars); err != nil { - return err - } - return nil +func validateStringVariableInTaskParameters(value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { + errs := substitution.ValidateVariableP(value, prefix, stringVars) + return errs.Also(substitution.ValidateVariableProhibitedP(value, prefix, arrayVars)) } -func validateArrayVariableInTaskParameters(name, value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { - if err := substitution.ValidateVariable(name, value, prefix, "task parameter", "pipelinespec.params", stringVars); err != nil { - return err - } - if err := substitution.ValidateVariableIsolated(name, value, prefix, "task parameter", "pipelinespec.params", arrayVars); err != nil { - return err - } - return nil +func validateArrayVariableInTaskParameters(value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { + errs := substitution.ValidateVariableP(value, prefix, stringVars) + return errs.Also(substitution.ValidateVariableProhibitedP(value, prefix, arrayVars)) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index fac8294c2c5..9244db87088 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -36,187 +36,38 @@ var _ apis.Validatable = (*Pipeline)(nil) // Validate checks that the Pipeline structure is valid but does not validate // that any references resources exist, that is done at run time. func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { - if err := validate.ObjectMetadata(p.GetObjectMeta()); err != nil { - return err.ViaField("metadata") - } - return p.Spec.Validate(ctx) -} - -// validateDeclaredResources ensures that the specified resources have unique names and -// validates that all the resources referenced by pipeline tasks are declared in the pipeline -func validateDeclaredResources(resources []PipelineDeclaredResource, tasks []PipelineTask, finalTasks []PipelineTask) error { - encountered := sets.NewString() - for _, r := range resources { - if encountered.Has(r.Name) { - return fmt.Errorf("resource with name %q appears more than once", r.Name) - } - encountered.Insert(r.Name) - } - required := []string{} - for _, t := range tasks { - if t.Resources != nil { - for _, input := range t.Resources.Inputs { - required = append(required, input.Resource) - } - for _, output := range t.Resources.Outputs { - required = append(required, output.Resource) - } - } - - for _, condition := range t.Conditions { - for _, cr := range condition.Resources { - required = append(required, cr.Resource) - } - } - } - for _, t := range finalTasks { - if t.Resources != nil { - for _, input := range t.Resources.Inputs { - required = append(required, input.Resource) - } - for _, output := range t.Resources.Outputs { - required = append(required, output.Resource) - } - } - } - - provided := make([]string, 0, len(resources)) - for _, resource := range resources { - provided = append(provided, resource.Name) - } - missing := list.DiffLeft(required, provided) - if len(missing) > 0 { - return fmt.Errorf("pipeline declared resources didn't match usage in Tasks: Didn't provide required values: %s", missing) - } - return nil -} - -func isOutput(outputs []PipelineTaskOutputResource, resource string) bool { - for _, output := range outputs { - if output.Resource == resource { - return true - } - } - return false -} - -// validateFrom ensures that the `from` values make sense: that they rely on values from Tasks -// that ran previously, and that the PipelineResource is actually an output of the Task it should come from. -func validateFrom(tasks []PipelineTask) *apis.FieldError { - taskOutputs := map[string][]PipelineTaskOutputResource{} - for _, task := range tasks { - var to []PipelineTaskOutputResource - if task.Resources != nil { - to = make([]PipelineTaskOutputResource, len(task.Resources.Outputs)) - copy(to, task.Resources.Outputs) - } - taskOutputs[task.Name] = to - } - for _, t := range tasks { - inputResources := []PipelineTaskInputResource{} - if t.Resources != nil { - inputResources = append(inputResources, t.Resources.Inputs...) - } - - for _, c := range t.Conditions { - inputResources = append(inputResources, c.Resources...) - } - - for _, rd := range inputResources { - for _, pt := range rd.From { - outputs, found := taskOutputs[pt] - if !found { - return apis.ErrInvalidValue(fmt.Sprintf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt), - "spec.tasks.resources.inputs.from") - } - if !isOutput(outputs, rd.Resource) { - return apis.ErrInvalidValue(fmt.Sprintf("the resource %s from %s must be an output but is an input", rd.Resource, pt), - "spec.tasks.resources.inputs.from") - } - } - } - } - return nil -} - -// validateGraph ensures the Pipeline's dependency Graph (DAG) make sense: that there is no dependency -// cycle or that they rely on values from Tasks that ran previously, and that the PipelineResource -// is actually an output of the Task it should come from. -func validateGraph(tasks []PipelineTask) error { - if _, err := dag.Build(PipelineTaskList(tasks)); err != nil { - return err - } - return nil + errs := validate.ObjectMetadata(p.GetObjectMeta()).ViaField("metadata") + return errs.Also(p.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) } // Validate checks that taskNames in the Pipeline are valid and that the graph // of Tasks expressed in the Pipeline makes sense. -func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { +func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { if equality.Semantic.DeepEqual(ps, &PipelineSpec{}) { - return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces") + errs = errs.Also(apis.ErrGeneric("expected at least one, got none", "description", "params", "resources", "tasks", "workspaces")) } - // PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified - if err := validatePipelineTasks(ctx, ps.Tasks, ps.Finally); err != nil { - return err - } - + errs = errs.Also(validatePipelineTasks(ctx, ps.Tasks, ps.Finally)) // All declared resources should be used, and the Pipeline shouldn't try to use any resources // that aren't declared - if err := validateDeclaredResources(ps.Resources, ps.Tasks, ps.Finally); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.resources") - } - + errs = errs.Also(validateDeclaredResources(ps.Resources, ps.Tasks, ps.Finally)) // The from values should make sense - if err := validateFrom(ps.Tasks); err != nil { - return err - } - + errs = errs.Also(validateFrom(ps.Tasks)) // Validate the pipeline task graph - if err := validateGraph(ps.Tasks); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.tasks") - } - - if err := validateParamResults(ps.Tasks); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.tasks.params.value") - } - + errs = errs.Also(validateGraph(ps.Tasks)) + errs = errs.Also(validateParamResults(ps.Tasks)) // The parameter variables should be valid - if err := validatePipelineParameterVariables(ps.Tasks, ps.Params); err != nil { - return err - } - - if err := validatePipelineParameterVariables(ps.Finally, ps.Params); err != nil { - return err - } - - if err := validatePipelineContextVariables(ps.Tasks); err != nil { - return err - } - + errs = errs.Also(validatePipelineParameterVariables(ps.Tasks, ps.Params).ViaField("tasks")) + errs = errs.Also(validatePipelineParameterVariables(ps.Finally, ps.Params).ViaField("finally")) + errs = errs.Also(validatePipelineContextVariables(ps.Tasks)) // Validate the pipeline's workspaces. - if err := validatePipelineWorkspaces(ps.Workspaces, ps.Tasks, ps.Finally); err != nil { - return err - } - + errs = errs.Also(validatePipelineWorkspaces(ps.Workspaces, ps.Tasks, ps.Finally)) // Validate the pipeline's results - if err := validatePipelineResults(ps.Results); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.tasks.params.value") - } - - if err := validateTasksAndFinallySection(ps); err != nil { - return err - } - - if err := validateFinalTasks(ps.Finally); err != nil { - return err - } - - if err := validateWhenExpressions(ps.Tasks); err != nil { - return err - } - - return nil + errs = errs.Also(validatePipelineResults(ps.Results)) + errs = errs.Also(validateTasksAndFinallySection(ps)) + errs = errs.Also(validateFinalTasks(ps.Finally)) + errs = errs.Also(validateWhenExpressions(ps.Tasks)) + return errs } // validatePipelineTasks ensures that pipeline tasks has unique label, pipeline tasks has specified one of @@ -224,72 +75,68 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { func validatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { // Names cannot be duplicated taskNames := sets.NewString() - var err *apis.FieldError + var errs *apis.FieldError for i, t := range tasks { - if err = validatePipelineTaskName(ctx, "spec.tasks", i, t, taskNames); err != nil { - return err - } + errs = errs.Also(validatePipelineTask(ctx, t, taskNames).ViaFieldIndex("tasks", i)) } for i, t := range finalTasks { - if err = validatePipelineTaskName(ctx, "spec.finally", i, t, taskNames); err != nil { - return err - } + errs = errs.Also(validatePipelineTask(ctx, t, taskNames).ViaFieldIndex("finally", i)) } - return nil + return errs } -func validatePipelineTaskName(ctx context.Context, prefix string, i int, t PipelineTask, taskNames sets.String) *apis.FieldError { - if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 { +func validatePipelineTaskName(name string) *apis.FieldError { + if err := validation.IsDNS1123Label(name); len(err) > 0 { return &apis.FieldError{ - Message: fmt.Sprintf("invalid value %q", t.Name), - Paths: []string{fmt.Sprintf(prefix+"[%d].name", i)}, + Message: fmt.Sprintf("invalid value %q", name), + Paths: []string{"name"}, Details: "Pipeline Task name must be a valid DNS Label." + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", } } + return nil +} + +func validatePipelineTask(ctx context.Context, t PipelineTask, taskNames sets.String) *apis.FieldError { + errs := validatePipelineTaskName(t.Name) // can't have both taskRef and taskSpec at the same time if (t.TaskRef != nil && t.TaskRef.Name != "") && t.TaskSpec != nil { - return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i)) + errs = errs.Also(apis.ErrMultipleOneOf("taskRef", "taskSpec")) } // Check that one of TaskRef and TaskSpec is present if (t.TaskRef == nil || (t.TaskRef != nil && t.TaskRef.Name == "")) && t.TaskSpec == nil { - return apis.ErrMissingOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i)) + errs = errs.Also(apis.ErrMissingOneOf("taskRef", "taskSpec")) } // Validate TaskSpec if it's present if t.TaskSpec != nil { - if err := t.TaskSpec.Validate(ctx); err != nil { - return err - } + errs = errs.Also(t.TaskSpec.Validate(ctx).ViaField("taskSpec")) } if t.TaskRef != nil && t.TaskRef.Name != "" { - // Task names are appended to the container name, which must exist and - // must be a valid k8s name - if errSlice := validation.IsQualifiedName(t.Name); len(errSlice) != 0 { - return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].name", i)) - } // TaskRef name must be a valid k8s name if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 { - return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].taskRef.name", i)) + errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) } if _, ok := taskNames[t.Name]; ok { - return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i)) + errs = errs.Also(apis.ErrMultipleOneOf("name")) } taskNames[t.Name] = struct{}{} } - return nil + return errs } // validatePipelineWorkspaces validates the specified workspaces, ensuring having unique name without any empty string, // and validates that all the referenced workspaces (by pipeline tasks) are specified in the pipeline -func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { +func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []PipelineTask, finalTasks []PipelineTask) (errs *apis.FieldError) { // Workspace names must be non-empty and unique. wsTable := sets.NewString() for i, ws := range wss { if ws.Name == "" { - return apis.ErrInvalidValue(fmt.Sprintf("workspace %d has empty name", i), "spec.workspaces") + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("workspace %d has empty name", i), + "").ViaFieldIndex("workspaces", i)) } if wsTable.Has(ws.Name) { - return apis.ErrInvalidValue(fmt.Sprintf("workspace with name %q appears more than once", ws.Name), "spec.workspaces") + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("workspace with name %q appears more than once", ws.Name), + "").ViaFieldIndex("workspaces", i)) } wsTable.Insert(ws.Name) } @@ -299,30 +146,30 @@ func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []Pipeli for i, pt := range pts { for j, ws := range pt.Workspaces { if !wsTable.Has(ws.Workspace) { - return apis.ErrInvalidValue( + errs = errs.Also(apis.ErrInvalidValue( fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", pt.Name, ws.Workspace), - fmt.Sprintf("spec.tasks[%d].workspaces[%d]", i, j), - ) + "", + ).ViaFieldIndex("workspaces", j).ViaFieldIndex("tasks", i)) } } } for i, t := range finalTasks { for j, ws := range t.Workspaces { if !wsTable.Has(ws.Workspace) { - return apis.ErrInvalidValue( + errs = errs.Also(apis.ErrInvalidValue( fmt.Sprintf("pipeline task %q expects workspace with name %q but none exists in pipeline spec", t.Name, ws.Workspace), - fmt.Sprintf("spec.finally[%d].workspaces[%d]", i, j), - ) + "", + ).ViaFieldIndex("workspaces", j).ViaFieldIndex("finally", i)) } } } - return nil + return errs } // validatePipelineParameterVariables validates parameters with those specified by each pipeline task, // (1) it validates the type of parameter is either string or array (2) parameter default value matches // with the type of that param (3) ensures that the referenced param variable is defined is part of the param declarations -func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec) *apis.FieldError { +func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec) (errs *apis.FieldError) { parameterNames := sets.NewString() arrayParameterNames := sets.NewString() @@ -335,23 +182,17 @@ func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec } } if !validType { - return apis.ErrInvalidValue(string(p.Type), fmt.Sprintf("spec.params.%s.type", p.Name)) + errs = errs.Also(apis.ErrInvalidValue(string(p.Type), "type").ViaFieldKey("params", p.Name)) } // If a default value is provided, ensure its type matches param's declared type. if (p.Default != nil) && (p.Default.Type != p.Type) { - return &apis.FieldError{ - Message: fmt.Sprintf( - "\"%v\" type does not match default value's type: \"%v\"", p.Type, p.Default.Type), - Paths: []string{ - fmt.Sprintf("spec.params.%s.type", p.Name), - fmt.Sprintf("spec.params.%s.default.type", p.Name), - }, - } + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("\"%v\" type does not match default value's type: \"%v\"", p.Type, p.Default.Type), + "type", "default.type").ViaFieldKey("params", p.Name)) } if parameterNames.Has(p.Name) { - return apis.ErrGeneric("parameter appears more than once", fmt.Sprintf("spec.params.%s", p.Name)) + errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", p.Name)) } // Add parameter name to parameterNames, and to arrayParameterNames if type is array. parameterNames.Insert(p.Name) @@ -360,19 +201,15 @@ func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec } } - return validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames) + return errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames)) } -func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError { - for _, task := range tasks { - if err := validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames); err != nil { - return err - } - if err := task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames); err != nil { - return err - } +func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) { + for idx, task := range tasks { + errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames).ViaIndex(idx)) + errs = errs.Also(task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames).ViaIndex(idx)) } - return nil + return errs } func validatePipelineContextVariables(tasks []PipelineTask) *apis.FieldError { @@ -391,24 +228,20 @@ func validatePipelineContextVariables(tasks []PipelineTask) *apis.FieldError { paramValues = append(paramValues, param.Value.ArrayVal...) } } - if err := validatePipelineContextVariablesInParamValues(paramValues, "context\\.pipelineRun", pipelineRunContextNames); err != nil { - return err - } - return validatePipelineContextVariablesInParamValues(paramValues, "context\\.pipeline", pipelineContextNames) + errs := validatePipelineContextVariablesInParamValues(paramValues, "context\\.pipelineRun", pipelineRunContextNames) + return errs.Also(validatePipelineContextVariablesInParamValues(paramValues, "context\\.pipeline", pipelineContextNames)) } -func validatePipelineContextVariablesInParamValues(paramValues []string, prefix string, contextNames sets.String) *apis.FieldError { +func validatePipelineContextVariablesInParamValues(paramValues []string, prefix string, contextNames sets.String) (errs *apis.FieldError) { for _, paramValue := range paramValues { - if err := substitution.ValidateVariable(fmt.Sprintf("param[%s]", paramValue), paramValue, prefix, "params", "pipelinespec.params", contextNames); err != nil { - return err - } + errs = errs.Also(substitution.ValidateVariableP(paramValue, prefix, contextNames).ViaField("value")) } - return nil + return errs } // validateParamResults ensures that task result variables are properly configured -func validateParamResults(tasks []PipelineTask) error { - for _, task := range tasks { +func validateParamResults(tasks []PipelineTask) (errs *apis.FieldError) { + for idx, task := range tasks { for _, param := range task.Params { expressions, ok := GetVarSubstitutionExpressionsForParam(param) if ok { @@ -416,13 +249,14 @@ func validateParamResults(tasks []PipelineTask) error { expressions = filter(expressions, looksLikeResultRef) resultRefs := NewResultRefs(expressions) if len(expressions) != len(resultRefs) { - return fmt.Errorf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs), + "value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx)) } } } } } - return nil + return errs } func filter(arr []string, cond func(string) bool) []string { @@ -436,47 +270,49 @@ func filter(arr []string, cond func(string) bool) []string { } // validatePipelineResults ensure that pipeline result variables are properly configured -func validatePipelineResults(results []PipelineResult) error { - for _, result := range results { +func validatePipelineResults(results []PipelineResult) (errs *apis.FieldError) { + for idx, result := range results { expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result) if ok { if LooksLikeContainsResultRefs(expressions) { expressions = filter(expressions, looksLikeResultRef) resultRefs := NewResultRefs(expressions) if len(expressions) != len(resultRefs) { - return fmt.Errorf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs), + "value").ViaFieldIndex("results", idx)) } } } } - return nil + + return errs } func validateTasksAndFinallySection(ps *PipelineSpec) *apis.FieldError { if len(ps.Finally) != 0 && len(ps.Tasks) == 0 { - return apis.ErrInvalidValue(fmt.Sprintf("spec.tasks is empty but spec.finally has %d tasks", len(ps.Finally)), "spec.finally") + return apis.ErrInvalidValue(fmt.Sprintf("spec.tasks is empty but spec.finally has %d tasks", len(ps.Finally)), "finally") } return nil } func validateFinalTasks(finalTasks []PipelineTask) *apis.FieldError { - for _, f := range finalTasks { + for idx, f := range finalTasks { if len(f.RunAfter) != 0 { - return apis.ErrInvalidValue(fmt.Sprintf("no runAfter allowed under spec.finally, final task %s has runAfter specified", f.Name), "spec.finally") + return apis.ErrInvalidValue(fmt.Sprintf("no runAfter allowed under spec.finally, final task %s has runAfter specified", f.Name), "").ViaFieldIndex("finally", idx) } if len(f.Conditions) != 0 { - return apis.ErrInvalidValue(fmt.Sprintf("no conditions allowed under spec.finally, final task %s has conditions specified", f.Name), "spec.finally") + return apis.ErrInvalidValue(fmt.Sprintf("no conditions allowed under spec.finally, final task %s has conditions specified", f.Name), "").ViaFieldIndex("finally", idx) } if len(f.WhenExpressions) != 0 { - return apis.ErrInvalidValue(fmt.Sprintf("no when expressions allowed under spec.finally, final task %s has when expressions specified", f.Name), "spec.finally") + return apis.ErrInvalidValue(fmt.Sprintf("no when expressions allowed under spec.finally, final task %s has when expressions specified", f.Name), "").ViaFieldIndex("finally", idx) } } - if err := validateTaskResultReferenceNotUsed(finalTasks); err != nil { + if err := validateTaskResultReferenceNotUsed(finalTasks).ViaField("finally"); err != nil { return err } - if err := validateTasksInputFrom(finalTasks); err != nil { + if err := validateTasksInputFrom(finalTasks).ViaField("finally"); err != nil { return err } @@ -484,13 +320,13 @@ func validateFinalTasks(finalTasks []PipelineTask) *apis.FieldError { } func validateTaskResultReferenceNotUsed(tasks []PipelineTask) *apis.FieldError { - for _, t := range tasks { + for idx, t := range tasks { for _, p := range t.Params { expressions, ok := GetVarSubstitutionExpressionsForParam(p) if ok { if LooksLikeContainsResultRefs(expressions) { return apis.ErrInvalidValue(fmt.Sprintf("no task result allowed under params,"+ - "final task param %s has set task result as its value", p.Name), "spec.finally.task.params") + "final task param %s has set task result as its value", p.Name), "params").ViaIndex(idx) } } } @@ -498,38 +334,140 @@ func validateTaskResultReferenceNotUsed(tasks []PipelineTask) *apis.FieldError { return nil } -func validateTasksInputFrom(tasks []PipelineTask) *apis.FieldError { - for _, t := range tasks { +func validateTasksInputFrom(tasks []PipelineTask) (errs *apis.FieldError) { + for idx, t := range tasks { inputResources := []PipelineTaskInputResource{} if t.Resources != nil { inputResources = append(inputResources, t.Resources.Inputs...) } - for _, rd := range inputResources { + for i, rd := range inputResources { if len(rd.From) != 0 { - return apis.ErrDisallowedFields(fmt.Sprintf("no from allowed under inputs,"+ - " final task %s has from specified", rd.Name), "spec.finally.task.resources.inputs") + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("no from allowed under inputs,"+ + " final task %s has from specified", rd.Name), "").ViaFieldIndex("inputs", i).ViaField("resources").ViaIndex(idx)) } } } - return nil + return errs } -func validateWhenExpressions(tasks []PipelineTask) *apis.FieldError { +func validateWhenExpressions(tasks []PipelineTask) (errs *apis.FieldError) { for i, t := range tasks { - if err := validateOneOfWhenExpressionsOrConditions(i, t); err != nil { - return err + errs = errs.Also(validateOneOfWhenExpressionsOrConditions(t).ViaFieldIndex("tasks", i)) + errs = errs.Also(t.WhenExpressions.validate().ViaFieldIndex("tasks", i)) + } + return errs +} + +func validateOneOfWhenExpressionsOrConditions(t PipelineTask) *apis.FieldError { + if t.WhenExpressions != nil && t.Conditions != nil { + return apis.ErrMultipleOneOf("when", "conditions") + } + return nil +} + +// validateDeclaredResources ensures that the specified resources have unique names and +// validates that all the resources referenced by pipeline tasks are declared in the pipeline +func validateDeclaredResources(resources []PipelineDeclaredResource, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { + encountered := sets.NewString() + for _, r := range resources { + if encountered.Has(r.Name) { + return apis.ErrInvalidValue(fmt.Sprintf("resource with name %q appears more than once", r.Name), "resources") + } + encountered.Insert(r.Name) + } + required := []string{} + for _, t := range tasks { + if t.Resources != nil { + for _, input := range t.Resources.Inputs { + required = append(required, input.Resource) + } + for _, output := range t.Resources.Outputs { + required = append(required, output.Resource) + } } - if err := t.WhenExpressions.validate(); err != nil { - return err + + for _, condition := range t.Conditions { + for _, cr := range condition.Resources { + required = append(required, cr.Resource) + } } } + for _, t := range finalTasks { + if t.Resources != nil { + for _, input := range t.Resources.Inputs { + required = append(required, input.Resource) + } + for _, output := range t.Resources.Outputs { + required = append(required, output.Resource) + } + } + } + + provided := make([]string, 0, len(resources)) + for _, resource := range resources { + provided = append(provided, resource.Name) + } + missing := list.DiffLeft(required, provided) + if len(missing) > 0 { + return apis.ErrInvalidValue(fmt.Sprintf("pipeline declared resources didn't match usage in Tasks: Didn't provide required values: %s", missing), "resources") + } return nil } -func validateOneOfWhenExpressionsOrConditions(i int, t PipelineTask) *apis.FieldError { - prefix := "spec.tasks" - if t.WhenExpressions != nil && t.Conditions != nil { - return apis.ErrMultipleOneOf(fmt.Sprintf(fmt.Sprintf(prefix+"[%d].when", i), fmt.Sprintf(prefix+"[%d].conditions", i))) +func isOutput(outputs []PipelineTaskOutputResource, resource string) bool { + for _, output := range outputs { + if output.Resource == resource { + return true + } + } + return false +} + +// validateFrom ensures that the `from` values make sense: that they rely on values from Tasks +// that ran previously, and that the PipelineResource is actually an output of the Task it should come from. +func validateFrom(tasks []PipelineTask) (errs *apis.FieldError) { + taskOutputs := map[string][]PipelineTaskOutputResource{} + for _, task := range tasks { + var to []PipelineTaskOutputResource + if task.Resources != nil { + to = make([]PipelineTaskOutputResource, len(task.Resources.Outputs)) + copy(to, task.Resources.Outputs) + } + taskOutputs[task.Name] = to + } + for i, t := range tasks { + inputResources := []PipelineTaskInputResource{} + if t.Resources != nil { + inputResources = append(inputResources, t.Resources.Inputs...) + } + + for _, c := range t.Conditions { + inputResources = append(inputResources, c.Resources...) + } + + for j, rd := range inputResources { + for _, pt := range rd.From { + outputs, found := taskOutputs[pt] + if !found { + return apis.ErrInvalidValue(fmt.Sprintf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt), + "from").ViaFieldIndex("inputs", j).ViaField("resources").ViaFieldIndex("tasks", i) + } + if !isOutput(outputs, rd.Resource) { + return apis.ErrInvalidValue(fmt.Sprintf("the resource %s from %s must be an output but is an input", rd.Resource, pt), + "from").ViaFieldIndex("inputs", j).ViaField("resources").ViaFieldIndex("tasks", i) + } + } + } + } + return errs +} + +// validateGraph ensures the Pipeline's dependency Graph (DAG) make sense: that there is no dependency +// cycle or that they rely on values from Tasks that ran previously, and that the PipelineResource +// is actually an output of the Task it should come from. +func validateGraph(tasks []PipelineTask) *apis.FieldError { + if _, err := dag.Build(PipelineTaskList(tasks)); err != nil { + return apis.ErrInvalidValue(err.Error(), "tasks") } return nil } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 30c5c34d1eb..96eb336c205 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -21,9 +21,13 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/selection" + "knative.dev/pkg/apis" ) func TestPipeline_Validate_Success(t *testing.T) { @@ -110,8 +114,9 @@ func TestPipeline_Validate_Success(t *testing.T) { func TestPipeline_Validate_Failure(t *testing.T) { tests := []struct { - name string - p *Pipeline + name string + p *Pipeline + expectedError apis.FieldError }{{ name: "period in name", p: &Pipeline{ @@ -120,16 +125,31 @@ func TestPipeline_Validate_Failure(t *testing.T) { Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}}}, }, }, + expectedError: apis.FieldError{ + Message: `Invalid resource name: special character . must not be present`, + Paths: []string{"metadata.name"}, + }, }, { name: "pipeline name too long", p: &Pipeline{ ObjectMeta: metav1.ObjectMeta{Name: "asdf123456789012345678901234567890123456789012345678901234567890"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}}}, + }, + }, + expectedError: apis.FieldError{ + Message: `Invalid resource name: length must be no more than 63 characters`, + Paths: []string{"metadata.name"}, }, }, { name: "pipeline spec missing", p: &Pipeline{ ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, }, + expectedError: apis.FieldError{ + Message: `expected at least one, got none`, + Paths: []string{"spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -137,14 +157,18 @@ func TestPipeline_Validate_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.Validate() did not return error for invalid pipeline: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("Pipeline.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } func TestPipelineSpec_Validate_Failure(t *testing.T) { tests := []struct { - name string - ps *PipelineSpec + name string + ps *PipelineSpec + expectedError apis.FieldError }{{ name: "invalid pipeline with one pipeline task having taskRef and taskSpec both", ps: &PipelineSpec{ @@ -158,6 +182,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }}, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"tasks[1].taskRef", "tasks[1].taskSpec"}, + }, }, { name: "invalid pipeline with one pipeline task having both conditions and when expressions", ps: &PipelineSpec{ @@ -175,6 +203,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"tasks[0].conditions", "tasks[0].when"}, + }, }, { name: "invalid pipeline with one pipeline task having when expression with invalid operator (not In/NotIn)", ps: &PipelineSpec{ @@ -189,6 +221,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: operator "exists" is not recognized. valid operators: in,notin`, + Paths: []string{"tasks[0].when[0]"}, + }, }, { name: "invalid pipeline with one pipeline task having when expression with invalid values (empty)", ps: &PipelineSpec{ @@ -203,6 +239,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: expecting non-empty values field`, + Paths: []string{"tasks[0].when[0]"}, + }, }, { name: "invalid pipeline with one pipeline task having when expression with invalid operator (missing)", ps: &PipelineSpec{ @@ -216,6 +256,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: operator "" is not recognized. valid operators: in,notin`, + Paths: []string{"tasks[0].when[0]"}, + }, }, { name: "invalid pipeline with one pipeline task having when expression with invalid values (missing)", ps: &PipelineSpec{ @@ -229,6 +273,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: expecting non-empty values field`, + Paths: []string{"tasks[0].when[0]"}, + }, }, { name: "invalid pipeline with one pipeline task having when expression with misconfigured result reference", ps: &PipelineSpec{ @@ -246,6 +294,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`, + Paths: []string{"tasks[1].when[0]"}, + }, }, { name: "invalid pipeline with one pipeline task having blank when expression", ps: &PipelineSpec{ @@ -259,6 +311,10 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { WhenExpressions: []WhenExpression{{}}, }}, }, + expectedError: apis.FieldError{ + Message: `missing field(s)`, + Paths: []string{"tasks[1].when[0]"}, + }, }, { name: "invalid pipeline with pipeline task having reference to resources which does not exist", ps: &PipelineSpec{ @@ -300,9 +356,16 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }}, }}, }, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline declared resources didn't match usage in Tasks: Didn't provide required values: [missing-great-resource missing-wonderful-resource missing-great-resource]`, + Paths: []string{"resources"}, + }, }, { name: "invalid pipeline spec - from referring to a pipeline task which does not exist", ps: &PipelineSpec{ + Resources: []PipelineDeclaredResource{{ + Name: "great-resource", Type: PipelineResourceTypeGit, + }}, Tasks: []PipelineTask{{ Name: "baz", TaskRef: &TaskRef{Name: "baz-task"}, }, { @@ -315,15 +378,8 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { }, }}, }, - }, { - name: "invalid pipeline spec with DAG having cyclic dependency", - ps: &PipelineSpec{ - Tasks: []PipelineTask{{ - Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"bar"}, - }, { - Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"}, - }}, - }, + expectedError: *apis.ErrGeneric(`invalid value: couldn't add link between foo and bar: task foo depends on bar but bar wasn't present in Pipeline`, "tasks").Also( + apis.ErrInvalidValue("expected resource great-resource to be from task bar, but task bar doesn't exist", "tasks[1].resources.inputs[0].from")), }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -331,10 +387,28 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { if err == nil { t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } +func TestPipelineSpec_Validate_Failure_CycleDAG(t *testing.T) { + name := "invalid pipeline spec with DAG having cyclic dependency" + ps := &PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, RunAfter: []string{"bar"}, + }, { + Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"}, + }}, + } + err := ps.Validate(context.Background()) + if err == nil { + t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec: %s", name) + } +} + func TestValidatePipelineTasks_Success(t *testing.T) { tests := []struct { name string @@ -364,13 +438,18 @@ func TestValidatePipelineTasks_Success(t *testing.T) { func TestValidatePipelineTasks_Failure(t *testing.T) { tests := []struct { - name string - tasks []PipelineTask + name string + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "pipeline task missing taskref and taskspec", tasks: []PipelineTask{{ Name: "foo", }}, + expectedError: apis.FieldError{ + Message: `expected exactly one, got neither`, + Paths: []string{"tasks[0].taskRef", "tasks[0].taskSpec"}, + }, }, { name: "pipeline task with both taskref and taskspec", tasks: []PipelineTask{{ @@ -378,30 +457,64 @@ func TestValidatePipelineTasks_Failure(t *testing.T) { TaskRef: &TaskRef{Name: "foo-task"}, TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }}, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"tasks[0].taskRef", "tasks[0].taskSpec"}, + }, }, { name: "pipeline task with invalid taskspec", tasks: []PipelineTask{{ Name: "foo", TaskSpec: &EmbeddedTask{TaskSpec: &TaskSpec{}}, }}, + expectedError: apis.FieldError{ + Message: `missing field(s)`, + Paths: []string{"tasks[0].taskSpec.steps"}, + }, }, { name: "pipeline tasks invalid (duplicate tasks)", tasks: []PipelineTask{ {Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}}, {Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}}, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"tasks[1].name"}, + }, }, { name: "pipeline task with empty task name", tasks: []PipelineTask{{Name: "", TaskRef: &TaskRef{Name: "foo-task"}}}, + expectedError: apis.FieldError{ + Message: `invalid value ""`, + Paths: []string{"tasks[0].name"}, + Details: "Pipeline Task name must be a valid DNS Label." + + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", + }, }, { name: "pipeline task with invalid task name", tasks: []PipelineTask{{Name: "_foo", TaskRef: &TaskRef{Name: "foo-task"}}}, + expectedError: apis.FieldError{ + Message: `invalid value "_foo"`, + Paths: []string{"tasks[0].name"}, + Details: "Pipeline Task name must be a valid DNS Label." + + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", + }, }, { name: "pipeline task with invalid task name (camel case)", tasks: []PipelineTask{{Name: "fooTask", TaskRef: &TaskRef{Name: "foo-task"}}}, + expectedError: apis.FieldError{ + Message: `invalid value "fooTask"`, + Paths: []string{"tasks[0].name"}, + Details: "Pipeline Task name must be a valid DNS Label." + + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", + }, }, { name: "pipeline task with invalid taskref name", tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "_foo-task"}}}, + expectedError: apis.FieldError{ + Message: `invalid value: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + Paths: []string{"tasks[0].name"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -409,6 +522,9 @@ func TestValidatePipelineTasks_Failure(t *testing.T) { if err == nil { t.Error("Pipeline.validatePipelineTasks() did not return error for invalid pipeline tasks:", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -445,8 +561,9 @@ func TestValidateFrom_Success(t *testing.T) { func TestValidateFrom_Failure(t *testing.T) { tests := []struct { - name string - tasks []PipelineTask + name string + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "invalid pipeline task - from in a pipeline with single pipeline task", tasks: []PipelineTask{{ @@ -457,7 +574,10 @@ func TestValidateFrom_Failure(t *testing.T) { Name: "the-resource", Resource: "great-resource", From: []string{"bar"}, }}, }, - }, + }}, + expectedError: apis.FieldError{ + Message: `invalid value: expected resource great-resource to be from task bar, but task bar doesn't exist`, + Paths: []string{"tasks[0].resources.inputs[0].from"}, }, }, { name: "invalid pipeline task - from referencing pipeline task which does not exist", @@ -472,6 +592,10 @@ func TestValidateFrom_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: expected resource great-resource to be from task bar, but task bar doesn't exist`, + Paths: []string{"tasks[1].resources.inputs[0].from"}, + }, }, { name: "invalid pipeline task - pipeline task condition resource does not exist", tasks: []PipelineTask{{ @@ -486,6 +610,10 @@ func TestValidateFrom_Failure(t *testing.T) { }}, }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: the resource missing-resource from foo must be an output but is an input`, + Paths: []string{"tasks[1].resources.inputs[0].from"}, + }, }, { name: "invalid pipeline task - from resource referring to a pipeline task which has no output", tasks: []PipelineTask{{ @@ -505,6 +633,10 @@ func TestValidateFrom_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: the resource wonderful-resource from bar must be an output but is an input`, + Paths: []string{"tasks[1].resources.inputs[0].from"}, + }, }, { name: "invalid pipeline task - from resource referring to input resource of the pipeline task instead of output", tasks: []PipelineTask{{ @@ -527,6 +659,10 @@ func TestValidateFrom_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: the resource some-resource from bar must be an output but is an input`, + Paths: []string{"tasks[1].resources.inputs[0].from"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -534,6 +670,9 @@ func TestValidateFrom_Failure(t *testing.T) { if err == nil { t.Error("Pipeline.validateFrom() did not return error for invalid pipeline task resources: ", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -633,9 +772,10 @@ func TestValidateDeclaredResources_Success(t *testing.T) { func TestValidateDeclaredResources_Failure(t *testing.T) { tests := []struct { - name string - resources []PipelineDeclaredResource - tasks []PipelineTask + name string + resources []PipelineDeclaredResource + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "duplicate resource declaration - resource declarations must be unique", resources: []PipelineDeclaredResource{{ @@ -652,6 +792,10 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: resource with name "duplicate-resource" appears more than once`, + Paths: []string{"resources"}, + }, }, { name: "output resource is missing from resource declarations", resources: []PipelineDeclaredResource{{ @@ -669,6 +813,10 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline declared resources didn't match usage in Tasks: Didn't provide required values: [missing-resource]`, + Paths: []string{"resources"}, + }, }, { name: "input resource is missing from resource declarations", resources: []PipelineDeclaredResource{{ @@ -686,6 +834,10 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline declared resources didn't match usage in Tasks: Didn't provide required values: [missing-resource]`, + Paths: []string{"resources"}, + }, }, { name: "invalid condition only resource -" + " pipeline task condition referring to a resource which is missing from resource declarations", @@ -699,6 +851,10 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { }}, }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline declared resources didn't match usage in Tasks: Didn't provide required values: [missing-resource]`, + Paths: []string{"resources"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -706,6 +862,9 @@ func TestValidateDeclaredResources_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.validateDeclaredResources() did not return error for invalid resource declarations: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -723,12 +882,9 @@ func TestValidateGraph_Success(t *testing.T) { }, { Name: "foo-bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo1", "bar1"}, }} - t.Run(desc, func(t *testing.T) { - err := validateGraph(tasks) - if err != nil { - t.Errorf("Pipeline.validateGraph() returned error for valid DAG of pipeline tasks: %s: %v", desc, err) - } - }) + if err := validateGraph(tasks); err != nil { + t.Errorf("Pipeline.validateGraph() returned error for valid DAG of pipeline tasks: %s: %v", desc, err) + } } func TestValidateGraph_Failure(t *testing.T) { @@ -738,13 +894,10 @@ func TestValidateGraph_Failure(t *testing.T) { }, { Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"}, }} - t.Run(desc, func(t *testing.T) { - err := validateGraph(tasks) - if err == nil { - t.Error("Pipeline.validateGraph() did not return error for invalid DAG of pipeline tasks:", desc) + if err := validateGraph(tasks); err == nil { + t.Error("Pipeline.validateGraph() did not return error for invalid DAG of pipeline tasks:", desc) - } - }) + } } func TestValidateParamResults_Success(t *testing.T) { @@ -766,12 +919,9 @@ func TestValidateParamResults_Success(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.foo) and $(tasks.a-task.results.output)"}, }}, }} - t.Run(desc, func(t *testing.T) { - err := validateParamResults(tasks) - if err != nil { - t.Errorf("Pipeline.validateParamResults() returned error for valid pipeline: %s: %v", desc, err) - } - }) + if err := validateParamResults(tasks); err != nil { + t.Errorf("Pipeline.validateParamResults() returned error for valid pipeline: %s: %v", desc, err) + } } func TestValidateParamResults_Failure(t *testing.T) { @@ -783,12 +933,17 @@ func TestValidateParamResults_Failure(t *testing.T) { Params: []Param{{ Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.a-task.resultTypo.bResult)"}}}, }} - t.Run(desc, func(t *testing.T) { - err := validateParamResults(tasks) - if err == nil { - t.Errorf("Pipeline.validateParamResults() did not return error for invalid pipeline: %s", desc) - } - }) + expectedError := apis.FieldError{ + Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`, + Paths: []string{"tasks[1].params[a-param].value"}, + } + err := validateParamResults(tasks) + if err == nil { + t.Errorf("Pipeline.validateParamResults() did not return error for invalid pipeline: %s", desc) + } + if d := cmp.Diff(expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("Pipeline.validateParamResults() errors diff %s", diff.PrintWantGot(d)) + } } func TestValidatePipelineResults_Success(t *testing.T) { @@ -798,12 +953,9 @@ func TestValidatePipelineResults_Success(t *testing.T) { Description: "this is my pipeline result", Value: "$(tasks.a-task.results.output)", }} - t.Run(desc, func(t *testing.T) { - err := validatePipelineResults(results) - if err != nil { - t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err) - } - }) + if err := validatePipelineResults(results); err != nil { + t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err) + } } func TestValidatePipelineResults_Failure(t *testing.T) { @@ -813,12 +965,17 @@ func TestValidatePipelineResults_Failure(t *testing.T) { Description: "this is my pipeline result", Value: "$(tasks.a-task.results.output.output)", }} - t.Run(desc, func(t *testing.T) { - err := validatePipelineResults(results) - if err == nil { - t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", desc) - } - }) + expectedError := apis.FieldError{ + Message: `invalid value: expected all of the expressions [tasks.a-task.results.output.output] to be result expressions but only [] were`, + Paths: []string{"results[0].value"}, + } + err := validatePipelineResults(results) + if err == nil { + t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", desc) + } + if d := cmp.Diff(expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("Pipeline.validateParamResults() errors diff %s", diff.PrintWantGot(d)) + } } func TestValidatePipelineParameterVariables_Success(t *testing.T) { @@ -913,9 +1070,10 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { func TestValidatePipelineParameterVariables_Failure(t *testing.T) { tests := []struct { - name string - params []ParamSpec - tasks []PipelineTask + name string + params []ParamSpec + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "invalid pipeline task with a parameter which is missing from the param declarations", tasks: []PipelineTask{{ @@ -925,6 +1083,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.does-not-exist)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.does-not-exist)"`, + Paths: []string{"[0].params[a-param]"}, + }, }, { name: "invalid string parameter variables in when expression, missing input param from the param declarations", tasks: []PipelineTask{{ @@ -936,6 +1098,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Values: []string{"foo"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.baz)"`, + Paths: []string{"[0].when[0].input"}, + }, }, { name: "invalid string parameter variables in when expression, missing values param from the param declarations", tasks: []PipelineTask{{ @@ -947,6 +1113,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Values: []string{"$(params.foo-is-baz)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.foo-is-baz)"`, + Paths: []string{"[0].when[0].values"}, + }, }, { name: "invalid string parameter variables in when expression, array reference in input", params: []ParamSpec{{ @@ -961,6 +1131,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Values: []string{"foo"}, }}, }}, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.foo)"`, + Paths: []string{"[0].when[0].input"}, + }, }, { name: "invalid string parameter variables in when expression, array reference in values", params: []ParamSpec{{ @@ -975,6 +1149,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Values: []string{"$(params.foo)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.foo)"`, + Paths: []string{"[0].when[0].values"}, + }, }, { name: "invalid pipeline task with a parameter combined with missing param from the param declarations", params: []ParamSpec{{ @@ -987,6 +1165,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.foo) and $(params.does-not-exist)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.foo) and $(params.does-not-exist)"`, + Paths: []string{"[0].params[a-param]"}, + }, }, { name: "invalid pipeline task with two parameters and one of them missing from the param declarations", params: []ParamSpec{{ @@ -1001,6 +1183,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "b-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.does-not-exist)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.does-not-exist)"`, + Paths: []string{"[0].params[b-param]"}, + }, }, { name: "invalid parameter type", params: []ParamSpec{{ @@ -1010,6 +1196,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: invalidtype`, + Paths: []string{"params[foo].type"}, + }, }, { name: "array parameter mismatching default type", params: []ParamSpec{{ @@ -1019,6 +1209,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `"array" type does not match default value's type: "string"`, + Paths: []string{"params[foo].default.type", "params[foo].type"}, + }, }, { name: "string parameter mismatching default type", params: []ParamSpec{{ @@ -1028,6 +1222,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params[foo].default.type", "params[foo].type"}, + }, }, { name: "array parameter used as string", params: []ParamSpec{{ @@ -1040,6 +1238,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.baz)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params[baz].default.type", "params[baz].type"}, + }, }, { name: "star array parameter used as string", params: []ParamSpec{{ @@ -1052,6 +1254,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(params.baz[*])"}, }}, }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params[baz].default.type", "params[baz].type"}, + }, }, { name: "array parameter string template not isolated", params: []ParamSpec{{ @@ -1064,6 +1270,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz)", "last"}}, }}, }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params[baz].default.type", "params[baz].type"}, + }, }, { name: "star array parameter string template not isolated", params: []ParamSpec{{ @@ -1076,6 +1286,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "a-param", Value: ArrayOrString{Type: ParamTypeArray, ArrayVal: []string{"value: $(params.baz[*])", "last"}}, }}, }}, + expectedError: apis.FieldError{ + Message: `"string" type does not match default value's type: "array"`, + Paths: []string{"params[baz].default.type", "params[baz].type"}, + }, }, { name: "multiple string parameters with the same name", params: []ParamSpec{{ @@ -1087,6 +1301,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, }, { name: "multiple array parameters with the same name", params: []ParamSpec{{ @@ -1098,6 +1316,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, }, { name: "multiple different type parameters with the same name", params: []ParamSpec{{ @@ -1109,6 +1331,10 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Name: "foo", TaskRef: &TaskRef{Name: "foo-task"}, }}, + expectedError: apis.FieldError{ + Message: `parameter appears more than once`, + Paths: []string{"params[baz]"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1116,6 +1342,9 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.validatePipelineParameterVariables() did not return error for invalid pipeline parameters: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -1140,9 +1369,10 @@ func TestValidatePipelineWorkspaces_Success(t *testing.T) { func TestValidatePipelineWorkspaces_Failure(t *testing.T) { tests := []struct { - name string - workspaces []PipelineWorkspaceDeclaration - tasks []PipelineTask + name string + workspaces []PipelineWorkspaceDeclaration + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "workspace bindings relying on a non-existent pipeline workspace cause an error", workspaces: []PipelineWorkspaceDeclaration{{ @@ -1155,6 +1385,10 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) { Workspace: "pipelineWorkspaceName", }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline task "foo" expects workspace with name "pipelineWorkspaceName" but none exists in pipeline spec`, + Paths: []string{"tasks[0].workspaces[0]"}, + }, }, { name: "multiple workspaces sharing the same name are not allowed", workspaces: []PipelineWorkspaceDeclaration{{ @@ -1165,6 +1399,10 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) { tasks: []PipelineTask{{ Name: "foo", TaskRef: &TaskRef{Name: "foo"}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: workspace with name "foo" appears more than once`, + Paths: []string{"workspaces[1]"}, + }, }, { name: "workspace name must not be empty", workspaces: []PipelineWorkspaceDeclaration{{ @@ -1173,6 +1411,10 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) { tasks: []PipelineTask{{ Name: "foo", TaskRef: &TaskRef{Name: "foo"}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: workspace 0 has empty name`, + Paths: []string{"workspaces[0]"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1180,6 +1422,9 @@ func TestValidatePipelineWorkspaces_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.validatePipelineWorkspaces() did not return error for invalid pipeline workspaces: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -1261,8 +1506,9 @@ func TestValidatePipelineWithFinalTasks_Success(t *testing.T) { func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { tests := []struct { - name string - p *Pipeline + name string + p *Pipeline + expectedError apis.FieldError }{{ name: "invalid pipeline without any non-final task (tasks set to nil) but at least one final task", p: &Pipeline{ @@ -1275,6 +1521,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `invalid value: spec.tasks is empty but spec.finally has 1 tasks`, + Paths: []string{"spec.finally"}, + }, }, { name: "invalid pipeline without any non-final task (tasks set to empty list of pipeline task) but at least one final task", p: &Pipeline{ @@ -1287,6 +1537,13 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: *apis.ErrMissingOneOf("spec.tasks[0].taskRef", "spec.tasks[0].taskSpec").Also( + &apis.FieldError{ + Message: `invalid value ""`, + Paths: []string{"spec.tasks[0].name"}, + Details: "Pipeline Task name must be a valid DNS Label." + + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", + }), }, { name: "invalid pipeline with valid non-final tasks but empty finally section", p: &Pipeline{ @@ -1299,6 +1556,13 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { Finally: []PipelineTask{{}}, }, }, + expectedError: *apis.ErrMissingOneOf("spec.finally[0].taskRef", "spec.finally[0].taskSpec").Also( + &apis.FieldError{ + Message: `invalid value ""`, + Paths: []string{"spec.finally[0].name"}, + Details: "Pipeline Task name must be a valid DNS Label." + + "For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names", + }), }, { name: "invalid pipeline with duplicate final tasks", p: &Pipeline{ @@ -1317,6 +1581,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"spec.finally[1].name"}, + }, }, { name: "invalid pipeline with same task name for final and non final task", p: &Pipeline{ @@ -1332,8 +1600,12 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"spec.finally[0].name"}, + }, }, { - name: "final task missing tasfref and taskspec", + name: "final task missing taskref and taskspec", p: &Pipeline{ ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, Spec: PipelineSpec{ @@ -1346,6 +1618,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got neither`, + Paths: []string{"spec.finally[0].taskRef", "spec.finally[0].taskSpec"}, + }, }, { name: "final task with both tasfref and taskspec", p: &Pipeline{ @@ -1362,6 +1638,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"spec.finally[0].taskRef", "spec.finally[0].taskSpec"}, + }, }, { name: "extra parameter called final-param provided to final task which is not specified in the Pipeline", p: &Pipeline{ @@ -1383,6 +1663,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.foo) and $(params.does-not-exist)"`, + Paths: []string{"spec.finally[0].params[final-param]"}, + }, }, { name: "invalid pipeline with invalid final tasks with runAfter and conditions", p: &Pipeline{ @@ -1405,6 +1689,10 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `invalid value: no runAfter allowed under spec.finally, final task final-task-1 has runAfter specified`, + Paths: []string{"spec.finally[0]"}, + }, }, { name: "invalid pipeline - workspace bindings in final task relying on a non-existent pipeline workspace", p: &Pipeline{ @@ -1425,14 +1713,19 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { }}, }, }, + expectedError: apis.FieldError{ + Message: `invalid value: pipeline task "final-task" expects workspace with name "pipeline-shared-workspace" but none exists in pipeline spec`, + Paths: []string{"spec.finally[0].workspaces[0]"}, + }, }, { name: "invalid pipeline with no tasks under tasks section and empty finally section", p: &Pipeline{ ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, Spec: PipelineSpec{ - Finally: []PipelineTask{{}}, + Finally: []PipelineTask{}, }, }, + expectedError: *apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces"), }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1440,6 +1733,9 @@ func TestValidatePipelineWithFinalTasks_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.Validate() did not return error for invalid pipeline with finally: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -1485,18 +1781,24 @@ func TestValidateTasksAndFinallySection_Failure(t *testing.T) { Name: "final-task", TaskRef: &TaskRef{Name: "foo"}, }}, } - t.Run(desc, func(t *testing.T) { - err := validateTasksAndFinallySection(ps) - if err == nil { - t.Errorf("Pipeline.ValidateTasksAndFinallySection() did not return error for invalid pipeline with finally: %s", desc) - } - }) + expectedError := apis.FieldError{ + Message: `invalid value: spec.tasks is empty but spec.finally has 1 tasks`, + Paths: []string{"finally"}, + } + err := validateTasksAndFinallySection(ps) + if err == nil { + t.Errorf("Pipeline.ValidateTasksAndFinallySection() did not return error for invalid pipeline with finally: %s", desc) + } + if d := cmp.Diff(expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("Pipeline.validateParamResults() errors diff %s", diff.PrintWantGot(d)) + } } func TestValidateFinalTasks_Failure(t *testing.T) { tests := []struct { - name string - finalTasks []PipelineTask + name string + finalTasks []PipelineTask + expectedError apis.FieldError }{{ name: "invalid pipeline with final task specifying runAfter", finalTasks: []PipelineTask{{ @@ -1504,6 +1806,10 @@ func TestValidateFinalTasks_Failure(t *testing.T) { TaskRef: &TaskRef{Name: "final-task"}, RunAfter: []string{"non-final-task"}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: no runAfter allowed under spec.finally, final task final-task has runAfter specified`, + Paths: []string{"finally[0]"}, + }, }, { name: "invalid pipeline with final task specifying conditions", finalTasks: []PipelineTask{{ @@ -1513,6 +1819,10 @@ func TestValidateFinalTasks_Failure(t *testing.T) { ConditionRef: "some-condition", }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: no conditions allowed under spec.finally, final task final-task has conditions specified`, + Paths: []string{"finally[0]"}, + }, }, { name: "invalid pipeline with final task output resources referring to other task input", finalTasks: []PipelineTask{{ @@ -1524,6 +1834,10 @@ func TestValidateFinalTasks_Failure(t *testing.T) { }}, }, }}, + expectedError: apis.FieldError{ + Message: `no from allowed under inputs, final task final-input-2 has from specified`, + Paths: []string{"finally[0].resources.inputs[0]"}, + }, }, { name: "invalid pipeline with final tasks having reference to task results", finalTasks: []PipelineTask{{ @@ -1533,6 +1847,10 @@ func TestValidateFinalTasks_Failure(t *testing.T) { Name: "param1", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.a-task.results.output)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: no task result allowed under params,final task param param1 has set task result as its value`, + Paths: []string{"finally[0].params"}, + }, }, { name: "invalid pipeline with final task specifying when expressions", finalTasks: []PipelineTask{{ @@ -1544,6 +1862,10 @@ func TestValidateFinalTasks_Failure(t *testing.T) { Values: []string{"foo", "bar"}, }}, }}, + expectedError: apis.FieldError{ + Message: `invalid value: no when expressions allowed under spec.finally, final task final-task has when expressions specified`, + Paths: []string{"finally[0]"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1551,6 +1873,9 @@ func TestValidateFinalTasks_Failure(t *testing.T) { if err == nil { t.Errorf("Pipeline.ValidateFinalTasks() did not return error for invalid pipeline: %s", tt.name) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } @@ -1616,8 +1941,9 @@ func TestContextValid(t *testing.T) { func TestContextInvalid(t *testing.T) { tests := []struct { - name string - tasks []PipelineTask + name string + tasks []PipelineTask + expectedError apis.FieldError }{{ name: "invalid string context variable for pipeline", tasks: []PipelineTask{{ @@ -1627,6 +1953,10 @@ func TestContextInvalid(t *testing.T) { Name: "a-param", Value: ArrayOrString{StringVal: "$(context.pipeline.missing)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(context.pipeline.missing)"`, + Paths: []string{"value"}, + }, }, { name: "invalid string context variable for pipelineRun", tasks: []PipelineTask{{ @@ -1636,6 +1966,10 @@ func TestContextInvalid(t *testing.T) { Name: "a-param", Value: ArrayOrString{StringVal: "$(context.pipelineRun.missing)"}, }}, }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(context.pipelineRun.missing)"`, + Paths: []string{"value"}, + }, }, { name: "invalid array context variables for pipeline and pipelineRun", tasks: []PipelineTask{{ @@ -1645,12 +1979,18 @@ func TestContextInvalid(t *testing.T) { Name: "a-param", Value: ArrayOrString{ArrayVal: []string{"$(context.pipeline.missing)", "and", "$(context.pipelineRun.missing)"}}, }}, }}, + expectedError: *apis.ErrGeneric(`non-existent variable in "$(context.pipeline.missing)"`, "value").Also( + apis.ErrGeneric(`non-existent variable in "$(context.pipelineRun.missing)"`, "value")), }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := validatePipelineContextVariables(tt.tasks); err == nil { + err := validatePipelineContextVariables(tt.tasks) + if err == nil { t.Errorf("Pipeline.validatePipelineContextVariables() did not return error for invalid pipeline parameters: %s, %s", tt.name, tt.tasks[0].Params) } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } }) } } diff --git a/pkg/apis/pipeline/v1beta1/when_validation.go b/pkg/apis/pipeline/v1beta1/when_validation.go index ff7a49e37df..e9803017503 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation.go +++ b/pkg/apis/pipeline/v1beta1/when_validation.go @@ -33,22 +33,15 @@ var validWhenOperators = []string{ } func (wes WhenExpressions) validate() *apis.FieldError { - if err := wes.validateWhenExpressionsFields(); err != nil { - return err - } - if err := wes.validateTaskResultsVariables(); err != nil { - return err - } - return nil + errs := wes.validateWhenExpressionsFields().ViaField("when") + return errs.Also(wes.validateTaskResultsVariables().ViaField("when")) } -func (wes WhenExpressions) validateWhenExpressionsFields() *apis.FieldError { - for _, we := range wes { - if err := we.validateWhenExpressionFields(); err != nil { - return err - } +func (wes WhenExpressions) validateWhenExpressionsFields() (errs *apis.FieldError) { + for idx, we := range wes { + errs = errs.Also(we.validateWhenExpressionFields().ViaIndex(idx)) } - return nil + return errs } func (we *WhenExpression) validateWhenExpressionFields() *apis.FieldError { @@ -57,16 +50,16 @@ func (we *WhenExpression) validateWhenExpressionFields() *apis.FieldError { } if !sets.NewString(validWhenOperators...).Has(string(we.Operator)) { message := fmt.Sprintf("operator %q is not recognized. valid operators: %s", we.Operator, strings.Join(validWhenOperators, ",")) - return apis.ErrInvalidValue(message, "spec.task.when") + return apis.ErrInvalidValue(message, apis.CurrentField) } if len(we.Values) == 0 { - return apis.ErrInvalidValue("expecting non-empty values field", "spec.task.when") + return apis.ErrInvalidValue("expecting non-empty values field", apis.CurrentField) } return nil } func (wes WhenExpressions) validateTaskResultsVariables() *apis.FieldError { - for _, we := range wes { + for idx, we := range wes { expressions, ok := we.GetVarSubstitutionExpressions() if ok { if LooksLikeContainsResultRefs(expressions) { @@ -74,7 +67,7 @@ func (wes WhenExpressions) validateTaskResultsVariables() *apis.FieldError { resultRefs := NewResultRefs(expressions) if len(expressions) != len(resultRefs) { message := fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs) - return apis.ErrInvalidValue(message, "spec.tasks.when") + return apis.ErrInvalidValue(message, apis.CurrentField).ViaIndex(idx) } } } @@ -82,25 +75,16 @@ func (wes WhenExpressions) validateTaskResultsVariables() *apis.FieldError { return nil } -func (wes WhenExpressions) validatePipelineParametersVariables(prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError { - for _, we := range wes { - if err := validateStringVariable(fmt.Sprintf("input[%s]", we.Input), we.Input, prefix, paramNames, arrayParamNames); err != nil { - return err - } +func (wes WhenExpressions) validatePipelineParametersVariables(prefix string, paramNames sets.String, arrayParamNames sets.String) (errs *apis.FieldError) { + for idx, we := range wes { + errs = errs.Also(validateStringVariable(we.Input, prefix, paramNames, arrayParamNames).ViaField("input").ViaFieldIndex("when", idx)) for _, val := range we.Values { - if err := validateStringVariable(fmt.Sprintf("values[%s]", val), val, prefix, paramNames, arrayParamNames); err != nil { - return err - } + errs = errs.Also(validateStringVariable(val, prefix, paramNames, arrayParamNames).ViaField("values").ViaFieldIndex("when", idx)) } } - return nil + return errs } -func validateStringVariable(name, value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { - if err := substitution.ValidateVariable(name, value, prefix, "task when expression", "pipelinespec.when", stringVars); err != nil { - return err - } - if err := substitution.ValidateVariableProhibited(name, value, prefix, "task when expression", "pipelinespec.when", arrayVars); err != nil { - return err - } - return nil +func validateStringVariable(value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError { + errs := substitution.ValidateVariableP(value, prefix, stringVars) + return errs.Also(substitution.ValidateVariableProhibitedP(value, prefix, arrayVars)) }