-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: validate taskRef.apiVersion format for custom tasks #9045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: validate taskRef.apiVersion format for custom tasks #9045
Conversation
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
3797773 to
e40bda6
Compare
e40bda6 to
cbab5ca
Compare
aThorp96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggested change, two general questions
Valid formats are "group/version" where group contains at least one dot
Is this strictly true? I know addon api groups conventionally use a dot, but as far as I can tell k8s doesn't pose that as a restriction.
| // isValidAPIVersion validates the format of an apiVersion string. | ||
| // Valid formats are "group/version" where group contains at least one dot. | ||
| // For custom tasks, apiVersion must always be in the "group/version" format. | ||
| func isValidAPIVersion(apiVersion string) bool { |
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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 ?
Add validation to ensure taskRef.apiVersion follows the correct format (group/version) when specified. Previously, invalid apiVersion values like 'invalid-api-version' were accepted, causing PipelineRuns to create unhandled CustomRuns that would timeout. Now Pipeline creation fails immediately with a clear error message when an invalid apiVersion is provided. Fixes tektoncd#9044
cbab5ca to
d56067e
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Changes
add validation to ensure taskRef.apiVersion follows the correct format (group/version) when specified. Previously, invalid apiVersion values like 'invalid-api-version' were accepted, causing PipelineRuns to create unhandled CustomRuns that would timeout. Now Pipeline creation fails immediately with a clear error message when an invalid apiVersion is provided.
Fixes #9044
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes