Validate PipelineTask name as Task names 📛

As reported, a `Pipeline` task name (`Pipelinetask`) can be created that will violate `TaskRun` naming rules, making it fail at runtime. This changes this by validation PipelineTask name the same way we validation `TaskRun` names, to make sure we won't fail for this at runtime. Signed-off-by: Vincent Demeester <[email protected]>
tektoncd · Feb 26, 2020 · b93fd4e · b93fd4e
1 parent de40421
commit b93fd4e
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 2 deletions.
diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go
@@ -138,6 +138,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
 	// 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 step 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))

diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go
@@ -68,6 +68,12 @@ func TestPipeline_Validate(t *testing.T) {
 			tb.PipelineTask("_foo", "foo-task"),
 		)),
 		failureExpected: true,
+	}, {
+		name: "pipeline spec invalid task name 2",
+		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
+			tb.PipelineTask("FooTask", "foo-task"),
+		)),
+		failureExpected: true,
 	}, {
 		name: "pipeline spec invalid taskref name",
 		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
@@ -97,13 +103,13 @@ func TestPipeline_Validate(t *testing.T) {
 	}, {
 		name: "pipeline spec invalid taskspec",
 		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineTask("", "", tb.PipelineTaskSpec(&v1alpha1.TaskSpec{})),
+			tb.PipelineTask("foo-task", "", tb.PipelineTaskSpec(&v1alpha1.TaskSpec{})),
 		)),
 		failureExpected: true,
 	}, {
 		name: "pipeline spec valid taskspec",
 		p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
-			tb.PipelineTask("", "", tb.PipelineTaskSpec(&v1alpha1.TaskSpec{
+			tb.PipelineTask("foo-task", "", tb.PipelineTaskSpec(&v1alpha1.TaskSpec{
 				TaskSpec: v1alpha2.TaskSpec{
 					Steps: []v1alpha1.Step{{Container: corev1.Container{
 						Name:  "foo",

diff --git a/pkg/apis/pipeline/v1alpha2/pipeline_validation.go b/pkg/apis/pipeline/v1alpha2/pipeline_validation.go
@@ -137,6 +137,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
 	// 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 step 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))

diff --git a/pkg/apis/pipeline/v1alpha2/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha2/pipeline_validation_test.go
@@ -191,6 +191,15 @@ func TestPipeline_Validate(t *testing.T) {
 			},
 		},
 		failureExpected: true,
+	}, {
+		name: "pipeline spec invalid task name 2",
+		p: &v1alpha2.Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: v1alpha2.PipelineSpec{
+				Tasks: []v1alpha2.PipelineTask{{Name: "fooTask", TaskRef: &v1alpha2.TaskRef{Name: "foo-task"}}},
+			},
+		},
+		failureExpected: true,
 	}, {
 		name: "pipeline spec invalid taskref name",
 		p: &v1alpha2.Pipeline{