Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 69 additions & 4 deletions pkg/apis/pipeline/v1/pipeline_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
expectedError apis.FieldError
}{{
name: "custom task - taskRef without kind",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example.dev/v0", Kind: "", Name: ""}},
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/v0", Kind: "", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: custom task ref must specify kind`,
Paths: []string{"taskRef.kind"},
Expand All @@ -456,7 +456,7 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
name: "custom task - taskSpec without kind",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.dev/v0",
APIVersion: "example/v0",
Kind: "",
},
}},
Expand All @@ -483,6 +483,39 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
Message: `invalid value: custom task ref must specify apiVersion`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskRef with invalid apiVersion format",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "invalid-api-version", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "invalid-api-version", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskSpec with invalid apiVersion format",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "no-slash-no-dot",
Kind: "some-kind",
},
}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "no-slash-no-dot", must be in the format "group/version"`,
Paths: []string{"taskSpec.apiVersion"},
},
}, {
name: "custom task - taskRef with empty group in apiVersion",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "/v1", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "/v1", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskRef with empty version in apiVersion",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "example/", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -497,6 +530,38 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
}
}

func TestPipelineTask_ValidateCustomTask_ValidAPIVersion(t *testing.T) {
tests := []struct {
name string
task PipelineTask
}{{
name: "custom task - valid apiVersion with group/version",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example.dev/v1", Kind: "Example", Name: "example"}},
}, {
name: "custom task - valid apiVersion with multi-level group",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "custom.tekton.dev/v1beta1", Kind: "Custom", Name: "custom"}},
}, {
name: "custom task - valid apiVersion without dots in group",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/v1", Kind: "Example", Name: "example"}},
}, {
name: "custom task - valid apiVersion in taskSpec",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.io/v2",
Kind: "CustomTask",
},
}},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.task.validateCustomTask()
if err != nil {
t.Errorf("PipelineTask.validateCustomTask() returned unexpected error: %v", err)
}
})
}
}

func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -691,7 +756,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
name: "custom task reference in taskref missing apiversion Kind",
p: PipelineTask{
Name: "invalid-custom-task",
TaskRef: &TaskRef{APIVersion: "example.com"},
TaskRef: &TaskRef{APIVersion: "example.com/v1"},
},
expectedError: apis.FieldError{
Message: `invalid value: custom task ref must specify kind`,
Expand All @@ -701,7 +766,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
name: "custom task reference in taskspec missing kind",
p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.com",
APIVersion: "example.com/v1",
}}},
expectedError: *apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"),
}, {
Expand Down
26 changes: 24 additions & 2 deletions pkg/apis/pipeline/v1/pipeline_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,19 @@ func (pt PipelineTask) validateRefOrSpec(ctx context.Context) (errs *apis.FieldE
return errs
}

// isValidAPIVersion validates the format of an apiVersion string.
// Valid formats are "group/version" where both group and version are non-empty.
// For custom tasks, apiVersion must always be in the "group/version" format.
func isValidAPIVersion(apiVersion string) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also validate that the apiVersion and Kind are registered in the cluster or something? A CustomRun will still be created when the apiVersion and kind dosn't exist in the cluster, however the CustomRun is simply never updated with any status so the pipelinerun times out. E.g. in the below screenshots I modified one of the examples to use a bogus apiVersion which was syntactically correct.

Image Image

I suppose being registered in the cluster is not sufficient since I've seen people mistakenly use "apiVersion": "tekton.dev/v1", "kind": "Task" in a TaskRef. Maybe that level of validation should be somewhere else? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your're right kube doesn't strictly require dots in API group names. I've removed that restriction. The validation now only checks for group/version format with non-empty group and version parts. On validating whether the apiVersion/kind is registered in the cluster would be useful, but is a separate concern I beleive, it would require runtime cluster access during admission, which is a bigger change. This PR focuses on catching syntactically invalid apiVersions at the webhook validation level. We could track the runtime check as a follow-up issue ? what do you think ?

parts := strings.Split(apiVersion, "/")
if len(parts) != 2 {
return false
}
group := parts[0]
version := parts[1]
return group != "" && version != ""
}

// validateCustomTask validates custom task specifications - checking kind and fail if not yet supported features specified
func (pt PipelineTask) validateCustomTask() (errs *apis.FieldError) {
if pt.TaskRef != nil && pt.TaskRef.Kind == "" {
Expand All @@ -344,10 +357,19 @@ func (pt PipelineTask) validateCustomTask() (errs *apis.FieldError) {
if pt.TaskSpec != nil && pt.TaskSpec.Kind == "" {
errs = errs.Also(apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"))
}
if pt.TaskRef != nil && pt.TaskRef.APIVersion == "" {
// Validate apiVersion format for custom tasks
if pt.TaskRef != nil && pt.TaskRef.APIVersion != "" {
if !isValidAPIVersion(pt.TaskRef.APIVersion) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("invalid apiVersion format %q, must be in the format \"group/version\"", pt.TaskRef.APIVersion), "taskRef.apiVersion"))
}
} else if pt.TaskRef != nil {
errs = errs.Also(apis.ErrInvalidValue("custom task ref must specify apiVersion", "taskRef.apiVersion"))
}
if pt.TaskSpec != nil && pt.TaskSpec.APIVersion == "" {
if pt.TaskSpec != nil && pt.TaskSpec.APIVersion != "" {
if !isValidAPIVersion(pt.TaskSpec.APIVersion) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("invalid apiVersion format %q, must be in the format \"group/version\"", pt.TaskSpec.APIVersion), "taskSpec.apiVersion"))
}
} else if pt.TaskSpec != nil {
errs = errs.Also(apis.ErrInvalidValue("custom task spec must specify apiVersion", "taskSpec.apiVersion"))
}
return errs
Expand Down
79 changes: 72 additions & 7 deletions pkg/apis/pipeline/v1beta1/pipeline_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
expectedError apis.FieldError
}{{
name: "custom task - taskRef without kind",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example.dev/v0", Kind: "", Name: ""}},
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/v0", Kind: "", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: custom task ref must specify kind`,
Paths: []string{"taskRef.kind"},
Expand All @@ -456,7 +456,7 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
name: "custom task - taskSpec without kind",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.dev/v0",
APIVersion: "example/v0",
Kind: "",
},
}},
Expand All @@ -483,6 +483,39 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
Message: `invalid value: custom task ref must specify apiVersion`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskRef with invalid apiVersion format",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "invalid-api-version", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "invalid-api-version", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskSpec with invalid apiVersion format",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "no-slash-no-dot",
Kind: "some-kind",
},
}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "no-slash-no-dot", must be in the format "group/version"`,
Paths: []string{"taskSpec.apiVersion"},
},
}, {
name: "custom task - taskRef with empty group in apiVersion",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "/v1", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "/v1", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}, {
name: "custom task - taskRef with empty version in apiVersion",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/", Kind: "some-kind", Name: ""}},
expectedError: apis.FieldError{
Message: `invalid value: invalid apiVersion format "example/", must be in the format "group/version"`,
Paths: []string{"taskRef.apiVersion"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -497,6 +530,38 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
}
}

func TestPipelineTask_ValidateCustomTask_ValidAPIVersion(t *testing.T) {
tests := []struct {
name string
task PipelineTask
}{{
name: "custom task - valid apiVersion with group/version",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example.dev/v1", Kind: "Example", Name: "example"}},
}, {
name: "custom task - valid apiVersion with multi-level group",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "custom.tekton.dev/v1beta1", Kind: "Custom", Name: "custom"}},
}, {
name: "custom task - valid apiVersion without dots in group",
task: PipelineTask{Name: "foo", TaskRef: &TaskRef{APIVersion: "example/v1", Kind: "Example", Name: "example"}},
}, {
name: "custom task - valid apiVersion in taskSpec",
task: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.io/v2",
Kind: "CustomTask",
},
}},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.task.validateCustomTask()
if err != nil {
t.Errorf("PipelineTask.validateCustomTask() returned unexpected error: %v", err)
}
})
}
}

func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -687,7 +752,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
name: "custom task reference in taskref missing apiversion Kind",
p: PipelineTask{
Name: "invalid-custom-task",
TaskRef: &TaskRef{APIVersion: "example.com"},
TaskRef: &TaskRef{APIVersion: "example.com/v1"},
},
expectedError: apis.FieldError{
Message: `invalid value: custom task ref must specify kind`,
Expand All @@ -697,7 +762,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
name: "custom task reference in taskspec missing kind",
p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
TypeMeta: runtime.TypeMeta{
APIVersion: "example.com",
APIVersion: "example.com/v1",
},
}},
expectedError: *apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"),
Expand Down Expand Up @@ -846,7 +911,7 @@ func TestPipelineTaskList_Validate(t *testing.T) {
name: "validate all valid custom task, and regular task",
tasks: PipelineTaskList{{
Name: "valid-custom-task",
TaskRef: &TaskRef{APIVersion: "example.com", Kind: "custom"},
TaskRef: &TaskRef{APIVersion: "example.com/v1", Kind: "custom"},
}, {
Name: "valid-task",
TaskRef: &TaskRef{Name: "task"},
Expand All @@ -856,7 +921,7 @@ func TestPipelineTaskList_Validate(t *testing.T) {
name: "validate list of tasks with valid custom task and invalid regular task",
tasks: PipelineTaskList{{
Name: "valid-custom-task",
TaskRef: &TaskRef{APIVersion: "example.com", Kind: "custom"},
TaskRef: &TaskRef{APIVersion: "example.com/v1", Kind: "custom"},
}, {
Name: "invalid-task-without-name",
TaskRef: &TaskRef{Name: ""},
Expand All @@ -867,7 +932,7 @@ func TestPipelineTaskList_Validate(t *testing.T) {
name: "validate all invalid tasks - custom task and regular task",
tasks: PipelineTaskList{{
Name: "invalid-custom-task",
TaskRef: &TaskRef{APIVersion: "example.com"},
TaskRef: &TaskRef{APIVersion: "example.com/v1"},
}, {
Name: "invalid-task",
TaskRef: &TaskRef{Name: ""},
Expand Down
26 changes: 24 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,19 @@ func (pt PipelineTask) validateRefOrSpec(ctx context.Context) (errs *apis.FieldE
return errs
}

// isValidAPIVersion validates the format of an apiVersion string.
// Valid formats are "group/version" where both group and version are non-empty.
// For custom tasks, apiVersion must always be in the "group/version" format.
func isValidAPIVersion(apiVersion string) bool {
parts := strings.Split(apiVersion, "/")
if len(parts) != 2 {
return false
}
group := parts[0]
version := parts[1]
return group != "" && version != ""
}

// validateCustomTask validates custom task specifications - checking kind and fail if not yet supported features specified
func (pt PipelineTask) validateCustomTask() (errs *apis.FieldError) {
if pt.TaskRef != nil && pt.TaskRef.Kind == "" {
Expand All @@ -349,10 +362,19 @@ func (pt PipelineTask) validateCustomTask() (errs *apis.FieldError) {
if pt.TaskSpec != nil && pt.TaskSpec.Kind == "" {
errs = errs.Also(apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"))
}
if pt.TaskRef != nil && pt.TaskRef.APIVersion == "" {
// Validate apiVersion format for custom tasks
if pt.TaskRef != nil && pt.TaskRef.APIVersion != "" {
if !isValidAPIVersion(pt.TaskRef.APIVersion) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("invalid apiVersion format %q, must be in the format \"group/version\"", pt.TaskRef.APIVersion), "taskRef.apiVersion"))
}
} else if pt.TaskRef != nil {
errs = errs.Also(apis.ErrInvalidValue("custom task ref must specify apiVersion", "taskRef.apiVersion"))
}
if pt.TaskSpec != nil && pt.TaskSpec.APIVersion == "" {
if pt.TaskSpec != nil && pt.TaskSpec.APIVersion != "" {
if !isValidAPIVersion(pt.TaskSpec.APIVersion) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("invalid apiVersion format %q, must be in the format \"group/version\"", pt.TaskSpec.APIVersion), "taskSpec.apiVersion"))
}
} else if pt.TaskSpec != nil {
errs = errs.Also(apis.ErrInvalidValue("custom task spec must specify apiVersion", "taskSpec.apiVersion"))
}
return errs
Expand Down
Loading