Skip to content

Commit 7ee54ae

Browse files
pritidesaipritidesai
committed
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
1 parent a7a4c5d commit 7ee54ae

File tree

2 files changed

+124
-86
lines changed

2 files changed

+124
-86
lines changed

‎pkg/apis/pipeline/v1alpha1/pipeline_validation.go‎

Lines changed: 62 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func isOutput(outputs []PipelineTaskOutputResource, resource string) bool {
9090

9191
// validateFrom ensures that the `from` values make sense: that they rely on values from Tasks
9292
// that ran previously, and that the PipelineResource is actually an output of the Task it should come from.
93-
func validateFrom(tasks []PipelineTask) error {
93+
func validateFrom(tasks []PipelineTask) *apis.FieldError {
9494
taskOutputs := map[string][]PipelineTaskOutputResource{}
9595
for _, task := range tasks {
9696
var to []PipelineTaskOutputResource
@@ -114,10 +114,12 @@ func validateFrom(tasks []PipelineTask) error {
114114
for _, pt := range rd.From {
115115
outputs, found := taskOutputs[pt]
116116
if !found {
117-
return fmt.Errorf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt)
117+
return apis.ErrInvalidValue(fmt.Sprintf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt),
118+
"spec.tasks.resources.inputs.from")
118119
}
119120
if !isOutput(outputs, rd.Resource) {
120-
return fmt.Errorf("the resource %s from %s must be an output but is an input", rd.Resource, pt)
121+
return apis.ErrInvalidValue(fmt.Sprintf("the resource %s from %s must be an output but is an input", rd.Resource, pt),
122+
"spec.tasks.resources.inputs.from")
121123
}
122124
}
123125
}
@@ -142,45 +144,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
142144
return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces")
143145
}
144146

145-
// Names cannot be duplicated
146-
taskNames := map[string]struct{}{}
147-
for i, t := range ps.Tasks {
148-
if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 {
149-
return &apis.FieldError{
150-
Message: fmt.Sprintf("invalid value %q", t.Name),
151-
Paths: []string{fmt.Sprintf("spec.tasks[%d].name", i)},
152-
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",
153-
}
154-
}
155-
// can't have both taskRef and taskSpec at the same time
156-
if (t.TaskRef != nil && t.TaskRef.Name != "") && t.TaskSpec != nil {
157-
return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i))
158-
}
159-
// Check that one of TaskRef and TaskSpec is present
160-
if (t.TaskRef == nil || (t.TaskRef != nil && t.TaskRef.Name == "")) && t.TaskSpec == nil {
161-
return apis.ErrMissingOneOf(fmt.Sprintf("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i))
162-
}
163-
// Validate TaskSpec if it's present
164-
if t.TaskSpec != nil {
165-
if err := t.TaskSpec.Validate(ctx); err != nil {
166-
return err
167-
}
168-
}
169-
if t.TaskRef != nil && t.TaskRef.Name != "" {
170-
// Task names are appended to the container name, which must exist and
171-
// must be a valid k8s name
172-
if errSlice := validation.IsQualifiedName(t.Name); len(errSlice) != 0 {
173-
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf("spec.tasks[%d].name", i))
174-
}
175-
// TaskRef name must be a valid k8s name
176-
if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 {
177-
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf("spec.tasks[%d].taskRef.name", i))
178-
}
179-
if _, ok := taskNames[t.Name]; ok {
180-
return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].name", i))
181-
}
182-
taskNames[t.Name] = struct{}{}
183-
}
147+
// PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified
148+
if err := validatePipelineTasks(ctx, ps.Tasks); err != nil {
149+
return err
184150
}
185151

186152
// 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 {
191157

192158
// The from values should make sense
193159
if err := validateFrom(ps.Tasks); err != nil {
194-
return apis.ErrInvalidValue(err.Error(), "spec.tasks.resources.inputs.from")
160+
return err
195161
}
196162

197163
// Validate the pipeline task graph
@@ -212,6 +178,59 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
212178
return nil
213179
}
214180

181+
func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError {
182+
// Names cannot be duplicated
183+
taskNames := map[string]struct{}{}
184+
var err *apis.FieldError
185+
for i, t := range tasks {
186+
if err = validatePipelineTaskName(ctx, "spec.tasks", i, t, taskNames); err != nil {
187+
return err
188+
}
189+
}
190+
return nil
191+
}
192+
193+
func validatePipelineTaskName(ctx context.Context, prefix string, i int, t PipelineTask, taskNames map[string]struct{}) *apis.FieldError {
194+
if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 {
195+
return &apis.FieldError{
196+
Message: fmt.Sprintf("invalid value %q", t.Name),
197+
Paths: []string{fmt.Sprintf(prefix+"[%d].name", i)},
198+
Details: "Pipeline Task name must be a valid DNS Label." +
199+
"For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names",
200+
}
201+
}
202+
// can't have both taskRef and taskSpec at the same time
203+
if (t.TaskRef != nil && t.TaskRef.Name != "") && t.TaskSpec != nil {
204+
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
205+
}
206+
// Check that one of TaskRef and TaskSpec is present
207+
if (t.TaskRef == nil || (t.TaskRef != nil && t.TaskRef.Name == "")) && t.TaskSpec == nil {
208+
return apis.ErrMissingOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
209+
}
210+
// Validate TaskSpec if it's present
211+
if t.TaskSpec != nil {
212+
if err := t.TaskSpec.Validate(ctx); err != nil {
213+
return err
214+
}
215+
}
216+
if t.TaskRef != nil && t.TaskRef.Name != "" {
217+
// Task names are appended to the container name, which must exist and
218+
// must be a valid k8s name
219+
if errSlice := validation.IsQualifiedName(t.Name); len(errSlice) != 0 {
220+
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].name", i))
221+
}
222+
// TaskRef name must be a valid k8s name
223+
if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 {
224+
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].taskRef.name", i))
225+
}
226+
if _, ok := taskNames[t.Name]; ok {
227+
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i))
228+
}
229+
taskNames[t.Name] = struct{}{}
230+
}
231+
return nil
232+
}
233+
215234
func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError {
216235
// Workspace names must be non-empty and unique.
217236
wsTable := make(map[string]struct{})

‎pkg/apis/pipeline/v1beta1/pipeline_validation.go‎

Lines changed: 62 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func isOutput(outputs []PipelineTaskOutputResource, resource string) bool {
8989

9090
// validateFrom ensures that the `from` values make sense: that they rely on values from Tasks
9191
// that ran previously, and that the PipelineResource is actually an output of the Task it should come from.
92-
func validateFrom(tasks []PipelineTask) error {
92+
func validateFrom(tasks []PipelineTask) *apis.FieldError {
9393
taskOutputs := map[string][]PipelineTaskOutputResource{}
9494
for _, task := range tasks {
9595
var to []PipelineTaskOutputResource
@@ -113,10 +113,12 @@ func validateFrom(tasks []PipelineTask) error {
113113
for _, pt := range rd.From {
114114
outputs, found := taskOutputs[pt]
115115
if !found {
116-
return fmt.Errorf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt)
116+
return apis.ErrInvalidValue(fmt.Sprintf("expected resource %s to be from task %s, but task %s doesn't exist", rd.Resource, pt, pt),
117+
"spec.tasks.resources.inputs.from")
117118
}
118119
if !isOutput(outputs, rd.Resource) {
119-
return fmt.Errorf("the resource %s from %s must be an output but is an input", rd.Resource, pt)
120+
return apis.ErrInvalidValue(fmt.Sprintf("the resource %s from %s must be an output but is an input", rd.Resource, pt),
121+
"spec.tasks.resources.inputs.from")
120122
}
121123
}
122124
}
@@ -141,45 +143,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
141143
return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces")
142144
}
143145

144-
// Names cannot be duplicated
145-
taskNames := map[string]struct{}{}
146-
for i, t := range ps.Tasks {
147-
if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 {
148-
return &apis.FieldError{
149-
Message: fmt.Sprintf("invalid value %q", t.Name),
150-
Paths: []string{fmt.Sprintf("spec.tasks[%d].name", i)},
151-
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",
152-
}
153-
}
154-
// can't have both taskRef and taskSpec at the same time
155-
if (t.TaskRef != nil && t.TaskRef.Name != "") && t.TaskSpec != nil {
156-
return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i))
157-
}
158-
// Check that one of TaskRef and TaskSpec is present
159-
if (t.TaskRef == nil || (t.TaskRef != nil && t.TaskRef.Name == "")) && t.TaskSpec == nil {
160-
return apis.ErrMissingOneOf(fmt.Sprintf("spec.tasks[%d].taskRef", i), fmt.Sprintf("spec.tasks[%d].taskSpec", i))
161-
}
162-
// Validate TaskSpec if it's present
163-
if t.TaskSpec != nil {
164-
if err := t.TaskSpec.Validate(ctx); err != nil {
165-
return err
166-
}
167-
}
168-
if t.TaskRef != nil && t.TaskRef.Name != "" {
169-
// Task names are appended to the container name, which must exist and
170-
// must be a valid k8s name
171-
if errSlice := validation.IsQualifiedName(t.Name); len(errSlice) != 0 {
172-
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf("spec.tasks[%d].name", i))
173-
}
174-
// TaskRef name must be a valid k8s name
175-
if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 {
176-
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf("spec.tasks[%d].taskRef.name", i))
177-
}
178-
if _, ok := taskNames[t.Name]; ok {
179-
return apis.ErrMultipleOneOf(fmt.Sprintf("spec.tasks[%d].name", i))
180-
}
181-
taskNames[t.Name] = struct{}{}
182-
}
146+
// PipelineTask must have a valid unique label and at least one of taskRef or taskSpec should be specified
147+
if err := validatePipelineTasks(ctx, ps.Tasks); err != nil {
148+
return err
183149
}
184150

185151
// 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 {
190156

191157
// The from values should make sense
192158
if err := validateFrom(ps.Tasks); err != nil {
193-
return apis.ErrInvalidValue(err.Error(), "spec.tasks.resources.inputs.from")
159+
return err
194160
}
195161

196162
// Validate the pipeline task graph
@@ -220,6 +186,59 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
220186
return nil
221187
}
222188

189+
func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError {
190+
// Names cannot be duplicated
191+
taskNames := map[string]struct{}{}
192+
var err *apis.FieldError
193+
for i, t := range tasks {
194+
if err = validatePipelineTaskName(ctx, "spec.tasks", i, t, taskNames); err != nil {
195+
return err
196+
}
197+
}
198+
return nil
199+
}
200+
201+
func validatePipelineTaskName(ctx context.Context, prefix string, i int, t PipelineTask, taskNames map[string]struct{}) *apis.FieldError {
202+
if errs := validation.IsDNS1123Label(t.Name); len(errs) > 0 {
203+
return &apis.FieldError{
204+
Message: fmt.Sprintf("invalid value %q", t.Name),
205+
Paths: []string{fmt.Sprintf(prefix+"[%d].name", i)},
206+
Details: "Pipeline Task name must be a valid DNS Label." +
207+
"For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names",
208+
}
209+
}
210+
// can't have both taskRef and taskSpec at the same time
211+
if (t.TaskRef != nil && t.TaskRef.Name != "") && t.TaskSpec != nil {
212+
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
213+
}
214+
// Check that one of TaskRef and TaskSpec is present
215+
if (t.TaskRef == nil || (t.TaskRef != nil && t.TaskRef.Name == "")) && t.TaskSpec == nil {
216+
return apis.ErrMissingOneOf(fmt.Sprintf(prefix+"[%d].taskRef", i), fmt.Sprintf(prefix+"[%d].taskSpec", i))
217+
}
218+
// Validate TaskSpec if it's present
219+
if t.TaskSpec != nil {
220+
if err := t.TaskSpec.Validate(ctx); err != nil {
221+
return err
222+
}
223+
}
224+
if t.TaskRef != nil && t.TaskRef.Name != "" {
225+
// Task names are appended to the container name, which must exist and
226+
// must be a valid k8s name
227+
if errSlice := validation.IsQualifiedName(t.Name); len(errSlice) != 0 {
228+
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].name", i))
229+
}
230+
// TaskRef name must be a valid k8s name
231+
if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 {
232+
return apis.ErrInvalidValue(strings.Join(errSlice, ","), fmt.Sprintf(prefix+"[%d].taskRef.name", i))
233+
}
234+
if _, ok := taskNames[t.Name]; ok {
235+
return apis.ErrMultipleOneOf(fmt.Sprintf(prefix+"[%d].name", i))
236+
}
237+
taskNames[t.Name] = struct{}{}
238+
}
239+
return nil
240+
}
241+
223242
func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError {
224243
// Workspace names must be non-empty and unique.
225244
wsTable := make(map[string]struct{})

0 commit comments

Comments
 (0)