Add webhook validation for remote Tasks #6942

lbernick · 2023-07-19T12:57:58Z

A prior commit (#6887) added validation for remote Pipelines by issuing dry-run create requests to the kubernetes API server, allowing validating admission webhooks to accept or reject remote pipelines without actually creating them. This commit adds the same logic for remote Tasks, and moves common logic into a shared package.

/kind misc

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

n/a Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Remote tasks are now validated by any validating admission webhooks.

tekton-robot · 2023-07-19T13:03:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	88.4%	89.0%	0.6
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-07-19T13:05:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	88.4%	89.0%	0.6
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-07-19T13:52:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	88.4%	89.0%	0.6
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-07-19T13:52:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	88.4%	89.0%	0.6
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

lbernick · 2023-07-19T14:38:27Z

/retest: #6943

lbernick · 2023-07-20T19:00:13Z

/retest
same flake

Yongxuanzhang

/lgtm
thanks! Learned a lot from reading the pr

afrittoli · 2023-07-24T15:51:31Z

@lbernick Thanks for this. My only doubt about this approach was the extra load it generates on the k8s API server. However, considering that it will only call the API once per TaskRun, it shouldn't be an issue.

QuanZhang-William

Thanks, Lee! Changes look good to me!

pkg/pod/status.go

QuanZhang-William · 2023-07-26T17:17:48Z

pkg/reconciler/taskrun/resources/taskref_test.go

@@ -448,14 +451,6 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) {
 			taskYAMLString,
 		}, "\n"),
 		wantTask: parse.MustParseV1Task(t, taskYAMLString),
-	}, {


Curious why do we remove these tests?

The webhook will now validate that beta features aren't used in v1 tasks unless enable-api-fields is set to beta. This replaces the existing hard-coded check for this specific bit of validation (

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Lines 173 to 177 in 080ea13

// Validation of beta fields must happen before the V1 Task is converted into the storage version of the API.

// TODO(#6592): Decouple API versioning from feature versioning

if err := obj.Spec.ValidateBetaFields(ctx); err != nil {

return nil, nil, fmt.Errorf("invalid Task %s: %w", obj.GetName(), err)

}

). I could mock the tests to return a BadRequestError specifically when beta features are used in v1 crds with enable-api-fields set to "stable", but I think the newly added tests that return BadRequestError are sufficient coverage and less brittle.

lbernick · 2023-07-27T20:45:31Z

@lbernick Thanks for this. My only doubt about this approach was the extra load it generates on the k8s API server. However, considering that it will only call the API once per TaskRun, it shouldn't be an issue.

Thanks @afrittoli! I also think this should be fine-- we already call the webhook to update every taskrun and our k8s clients are rate limited. Are there any changes I can make to this PR that would help alleviate concerns about extra api calls?

tekton-robot · 2023-07-27T20:46:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

tekton-robot · 2023-07-27T20:47:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.3%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.1%	93.2%	4.1
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.2%	85.3%	0.1

vdemeester · 2023-07-28T10:23:58Z

Thanks @afrittoli! I also think this should be fine-- we already call the webhook to update every taskrun and our k8s clients are rate limited. Are there any changes I can make to this PR that would help alleviate concerns about extra api calls?

This call is conditionned though (aka it won't happen for all PipelineRun or TaskRun), but ok.

lbernick · 2023-07-28T16:28:00Z

Thanks @afrittoli! I also think this should be fine-- we already call the webhook to update every taskrun and our k8s clients are rate limited. Are there any changes I can make to this PR that would help alleviate concerns about extra api calls?

This call is conditionned though (aka it won't happen for all PipelineRun or TaskRun), but ok.

My understanding (from #5146) is this function is actually what does all our status updates for TaskRuns, so it'll be called multiple times per TaskRun.

Do you have any suggestions for changes I could make to this PR, or concerns about merging it?

lbernick · 2023-07-28T16:28:39Z

/retest
since #6992 is fixed

vdemeester · 2023-08-02T10:04:48Z

/hold
See #7015

lbernick · 2023-08-03T19:06:57Z

/hold cancel

tekton-robot · 2023-08-03T19:12:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.9%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.5%	93.2%	3.7
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.3%	85.4%	0.1

tekton-robot · 2023-08-03T19:13:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.9%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.5%	93.2%	3.7
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.3%	85.4%	0.1

tekton-robot · 2023-08-03T19:19:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.9%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.5%	93.2%	3.7
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.3%	85.4%	0.1

pkg/reconciler/apiserver/apiserver.go

A prior commit added validation for remote Pipelines by issuing dry-run create requests to the kubernetes API server, allowing validating admission webhooks to accept or reject remote pipelines without actually creating them. This commit adds the same logic for remote Tasks, and moves common logic into a shared package.

tekton-robot · 2023-08-04T13:48:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.8%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.5%	93.2%	3.7
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.3%	85.4%	0.1

tekton-robot · 2023-08-04T13:50:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserver/apiserver.go	Do not exist	96.8%
pkg/reconciler/pipelinerun/resources/pipelineref.go	89.5%	93.2%	3.7
pkg/reconciler/taskrun/resources/taskref.go	91.3%	91.8%	0.5
pkg/reconciler/taskrun/taskrun.go	85.3%	85.4%	0.1

Yongxuanzhang

/lgtm

dibyom · 2023-08-16T18:43:22Z

/approve

tekton-robot · 2023-08-16T18:43:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, Yongxuanzhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dibyom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2023

tekton-robot requested review from dibyom and imjasonh July 19, 2023 12:58

lbernick assigned vdemeester, dibyom and chitrangpatel Jul 19, 2023

lbernick force-pushed the validate-remote-pipelines-2 branch from aad013f to e8ff288 Compare July 19, 2023 13:45

Yongxuanzhang reviewed Jul 21, 2023

View reviewed changes

tekton-robot assigned Yongxuanzhang Jul 21, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2023

QuanZhang-William reviewed Jul 26, 2023

View reviewed changes

lbernick force-pushed the validate-remote-pipelines-2 branch from e8ff288 to 9b9cdeb Compare July 27, 2023 20:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2023

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023

lbernick force-pushed the validate-remote-pipelines-2 branch from 9b9cdeb to 309c650 Compare August 3, 2023 19:05

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2023

lbernick force-pushed the validate-remote-pipelines-2 branch from 309c650 to f39cd22 Compare August 3, 2023 19:06

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2023

vdemeester reviewed Aug 4, 2023

View reviewed changes

pkg/reconciler/apiserver/apiserver.go Outdated Show resolved Hide resolved

lbernick force-pushed the validate-remote-pipelines-2 branch from f39cd22 to 0ad1bc8 Compare August 4, 2023 13:42

lbernick added this to the Pipelines v0.51 milestone Aug 8, 2023

afrittoli self-assigned this Aug 8, 2023

Yongxuanzhang approved these changes Aug 15, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023

tekton-robot merged commit 445734d into tektoncd:main Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webhook validation for remote Tasks #6942

Add webhook validation for remote Tasks #6942

lbernick commented Jul 19, 2023 •

edited

Loading

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

lbernick commented Jul 19, 2023

lbernick commented Jul 20, 2023

Yongxuanzhang left a comment

afrittoli commented Jul 24, 2023

QuanZhang-William left a comment

QuanZhang-William Jul 26, 2023

lbernick Jul 27, 2023

lbernick commented Jul 27, 2023

tekton-robot commented Jul 27, 2023

tekton-robot commented Jul 27, 2023

vdemeester commented Jul 28, 2023

lbernick commented Jul 28, 2023

lbernick commented Jul 28, 2023

vdemeester commented Aug 2, 2023

lbernick commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 4, 2023

tekton-robot commented Aug 4, 2023

Yongxuanzhang left a comment

dibyom commented Aug 16, 2023

tekton-robot commented Aug 16, 2023

	// Validation of beta fields must happen before the V1 Task is converted into the storage version of the API.
	// TODO(#6592): Decouple API versioning from feature versioning
	if err := obj.Spec.ValidateBetaFields(ctx); err != nil {
	return nil, nil, fmt.Errorf("invalid Task %s: %w", obj.GetName(), err)
	}

Add webhook validation for remote Tasks #6942

Add webhook validation for remote Tasks #6942

Conversation

lbernick commented Jul 19, 2023 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

tekton-robot commented Jul 19, 2023

lbernick commented Jul 19, 2023

lbernick commented Jul 20, 2023

Yongxuanzhang left a comment

Choose a reason for hiding this comment

afrittoli commented Jul 24, 2023

QuanZhang-William left a comment

Choose a reason for hiding this comment

QuanZhang-William Jul 26, 2023

Choose a reason for hiding this comment

lbernick Jul 27, 2023

Choose a reason for hiding this comment

lbernick commented Jul 27, 2023

tekton-robot commented Jul 27, 2023

tekton-robot commented Jul 27, 2023

vdemeester commented Jul 28, 2023

lbernick commented Jul 28, 2023

lbernick commented Jul 28, 2023

vdemeester commented Aug 2, 2023

lbernick commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 3, 2023

tekton-robot commented Aug 4, 2023

tekton-robot commented Aug 4, 2023

Yongxuanzhang left a comment

Choose a reason for hiding this comment

dibyom commented Aug 16, 2023

tekton-robot commented Aug 16, 2023

lbernick commented Jul 19, 2023 •

edited

Loading