From f0ff9d97073e133b946526d8d797f4df2658c422 Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Tue, 28 Apr 2020 09:36:34 -0700 Subject: [PATCH] refactor pipeline validation This change is part of the larger effort to add pipeline level finally. This initial refactoring is done so that it simplifies implementing validation for finally section which is similar to tasks section. This refactoring includes: 1) creating a new local function validatePipelineTasks which contians check on PipelineTask name and validates PipelineTask to at least contian one of taskRef or taskSpec. The same function will then be used by finally section as well. 2) Changing return type of validateFrom --- .../pipeline/v1alpha1/pipeline_validation.go | 105 +++++++++++------- .../pipeline/v1beta1/pipeline_validation.go | 105 +++++++++++------- 2 files changed, 124 insertions(+), 86 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index 2ef1ee9ad52..9afe435d72f 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -90,7 +90,7 @@ func isOutput(outputs []PipelineTaskOutputResource, resource string) bool { // 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) error { +func validateFrom(tasks []PipelineTask) *apis.FieldError { taskOutputs := map[string][]PipelineTaskOutputResource{} for _, task := range tasks { var to []PipelineTaskOutputResource @@ -114,10 +114,12 @@ func validateFrom(tasks []PipelineTask) error { for _, pt := range rd.From { outputs, found := taskOutputs[pt] if !found { - return fmt.Errorf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt) + 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 fmt.Errorf("the resource %s from %s must be an output but is an input", rd.Resource, pt) + 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") } } } @@ -142,45 +144,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces") } - // Names cannot be duplicated - taskNames := map[string]struct{}{} - for i, t := range ps.Tasks { - if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 { - return &apis.FieldError{ - Message: fmt.Sprintf("invalid value %q", t.Name), - Paths: []string{fmt.Sprintf("spec.tasks[%d].name", i)}, - 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", - } - } - // 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("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i)) - } - // 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("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i)) - } - // Validate TaskSpec if it's present - if t.TaskSpec != nil { - if err := t.TaskSpec.Validate(ctx); err != nil { - return err - } - } - 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("spec.tasks[%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("spec.tasks[%d].taskRef.name", i)) - } - if _, ok := taskNames[t.Name]; ok { - return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].name", i)) - } - taskNames[t.Name] = struct{}{} - } + // PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified + if err := validatePipelineTasks(ctx, ps.Tasks); err != nil { + return err } // All declared resources should be used, and the Pipeline shouldn't try to use any resources @@ -191,7 +157,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { // The from values should make sense if err := validateFrom(ps.Tasks); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.tasks.resources.inputs.from") + return err } // Validate the pipeline task graph @@ -212,6 +178,59 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return nil } +func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError { + // Names cannot be duplicated + taskNames := map[string]struct{}{} + var err *apis.FieldError + for i, t := range tasks { + if err = validatePipelineTaskName(ctx, "spec.tasks", i, t, taskNames); err != nil { + return err + } + } + return nil +} + +func validatePipelineTaskName(ctx context.Context, prefix string, i int, t PipelineTask, taskNames map[string]struct{}) *apis.FieldError { + if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 { + return &apis.FieldError{ + Message: fmt.Sprintf("invalid value %q", t.Name), + Paths: []string{fmt.Sprintf(prefix+"[%d].name", i)}, + 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", + } + } + // 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)) + } + // 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)) + } + // Validate TaskSpec if it's present + if t.TaskSpec != nil { + if err := t.TaskSpec.Validate(ctx); err != nil { + return err + } + } + 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)) + } + if _, ok := taskNames[t.Name]; ok { + return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i)) + } + taskNames[t.Name] = struct{}{} + } + return nil +} + func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError { // Workspace names must be non-empty and unique. wsTable := make(map[string]struct{}) diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 536b4ee337f..168090eb398 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -89,7 +89,7 @@ func isOutput(outputs []PipelineTaskOutputResource, resource string) bool { // 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) error { +func validateFrom(tasks []PipelineTask) *apis.FieldError { taskOutputs := map[string][]PipelineTaskOutputResource{} for _, task := range tasks { var to []PipelineTaskOutputResource @@ -113,10 +113,12 @@ func validateFrom(tasks []PipelineTask) error { for _, pt := range rd.From { outputs, found := taskOutputs[pt] if !found { - return fmt.Errorf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt) + 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 fmt.Errorf("the resource %s from %s must be an output but is an input", rd.Resource, pt) + 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") } } } @@ -141,45 +143,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces") } - // Names cannot be duplicated - taskNames := map[string]struct{}{} - for i, t := range ps.Tasks { - if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 { - return &apis.FieldError{ - Message: fmt.Sprintf("invalid value %q", t.Name), - Paths: []string{fmt.Sprintf("spec.tasks[%d].name", i)}, - 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", - } - } - // 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("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i)) - } - // 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("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i)) - } - // Validate TaskSpec if it's present - if t.TaskSpec != nil { - if err := t.TaskSpec.Validate(ctx); err != nil { - return err - } - } - 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("spec.tasks[%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("spec.tasks[%d].taskRef.name", i)) - } - if _, ok := taskNames[t.Name]; ok { - return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].name", i)) - } - taskNames[t.Name] = struct{}{} - } + // PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified + if err := validatePipelineTasks(ctx, ps.Tasks); err != nil { + return err } // All declared resources should be used, and the Pipeline shouldn't try to use any resources @@ -190,7 +156,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { // The from values should make sense if err := validateFrom(ps.Tasks); err != nil { - return apis.ErrInvalidValue(err.Error(), "spec.tasks.resources.inputs.from") + return err } // Validate the pipeline task graph @@ -220,6 +186,59 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError { return nil } +func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError { + // Names cannot be duplicated + taskNames := map[string]struct{}{} + var err *apis.FieldError + for i, t := range tasks { + if err = validatePipelineTaskName(ctx, "spec.tasks", i, t, taskNames); err != nil { + return err + } + } + return nil +} + +func validatePipelineTaskName(ctx context.Context, prefix string, i int, t PipelineTask, taskNames map[string]struct{}) *apis.FieldError { + if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 { + return &apis.FieldError{ + Message: fmt.Sprintf("invalid value %q", t.Name), + Paths: []string{fmt.Sprintf(prefix+"[%d].name", i)}, + 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", + } + } + // 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)) + } + // 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)) + } + // Validate TaskSpec if it's present + if t.TaskSpec != nil { + if err := t.TaskSpec.Validate(ctx); err != nil { + return err + } + } + 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)) + } + if _, ok := taskNames[t.Name]; ok { + return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i)) + } + taskNames[t.Name] = struct{}{} + } + return nil +} + func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError { // Workspace names must be non-empty and unique. wsTable := make(map[string]struct{})