Skip to content

Commit

Permalink
refactor pipeline validation
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pritidesai
pritidesai committed Apr 28, 2020
1 parent a7a4c5d commit fd32d41
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 86 deletions.
105 changes: 62 additions & 43 deletions pkg/apis/pipeline/v1alpha1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
}
}
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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.Name, t.TaskRef, t.TaskSpec, taskNames); err != nil {
return err
}
}
return nil
}

func validatePipelineTaskName(ctx context.Context, prefix string, i int, name string, taskRef *TaskRef, taskSpec *TaskSpec, taskNames map[string]struct{}) *apis.FieldError {
if errs := validation.IsDNS1123Label(name); len(errs) > 0 {
return &apis.FieldError{
Message: fmt.Sprintf("invalid value %q", 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 (taskRef != nil && taskRef.Name != "") && 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 (taskRef == nil || (taskRef != nil && taskRef.Name == "")) && taskSpec == nil {
return apis.ErrMissingOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
}
// Validate TaskSpec if it's present
if taskSpec != nil {
if err := taskSpec.Validate(ctx); err != nil {
return err
}
}
if taskRef != nil && taskRef.Name != "" {
// Task names are appended to the container name, which must exist and
// must be a valid k8s name
if errSlice := validation.IsQualifiedName(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(taskRef.Name); len(errSlice) != 0 {
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].taskRef.name", i))
}
if _, ok := taskNames[name]; ok {
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i))
}
taskNames[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{})
Expand Down
105 changes: 62 additions & 43 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
}
}
}
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.Name, t.TaskRef, t.TaskSpec, taskNames); err != nil {
return err
}
}
return nil
}

func validatePipelineTaskName(ctx context.Context, prefix string, i int, name string, taskRef *TaskRef, taskSpec *TaskSpec, taskNames map[string]struct{}) *apis.FieldError {
if errs := validation.IsDNS1123Label(name); len(errs) > 0 {
return &apis.FieldError{
Message: fmt.Sprintf("invalid value %q", 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 (taskRef != nil && taskRef.Name != "") && 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 (taskRef == nil || (taskRef != nil && taskRef.Name == "")) && taskSpec == nil {
return apis.ErrMissingOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
}
// Validate TaskSpec if it's present
if taskSpec != nil {
if err := taskSpec.Validate(ctx); err != nil {
return err
}
}
if taskRef != nil && taskRef.Name != "" {
// Task names are appended to the container name, which must exist and
// must be a valid k8s name
if errSlice := validation.IsQualifiedName(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(taskRef.Name); len(errSlice) != 0 {
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].taskRef.name", i))
}
if _, ok := taskNames[name]; ok {
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i))
}
taskNames[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{})
Expand Down

0 comments on commit fd32d41

Please sign in to comment.