[TEP-0091] add condition for trusted resources #6654

Yongxuanzhang · 2023-05-12T17:10:26Z

Changes

This commit adds the ConditionTrustedResourcesVerified for trusted resources into pipelinerun/taskrun status to show if the resources passes verification. Failing to verify will add a false condition to the taskrun/pipelinerun status and passing the verification will add a true status condition.

Though this is a big size PR, it contains several changes which can be grouped into:

Add VerificationResult to indicate different cases when the Verification doesn't return error (Files: verify.go)
Update GetTask and GetPipeline and all the functions in call stack to return VerificationResult back to reconcile (Files: pipelineref.go, pipelinerunresolution.go, pipelinespec.go, taskref.go, taskspec.go)
Update TrustedResources condition based on VerificationResult (Files: taskrun.go, pipelinerun.go)
Unit tests to cover those cases. (all test files)

Those changes cannot be split into separate PRs.

/kind feature

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

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

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.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add ConditionTrustedResourcesVerified condition to pipelinerun/taskrun status indicate whether trusted resources fails verification or not. Functions including GetTask, GetTaskData, GetPipeline, GetPipelineData are updated to return VerificationResult

Yongxuanzhang · 2023-05-12T17:10:45Z

/kind feature

tekton-robot · 2023-05-12T17:16:35Z

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/pipelinerun/pipelinerun.go	89.2%	88.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	94.8%	-0.8
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-12T17:18:08Z

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/pipelinerun/pipelinerun.go	89.2%	88.8%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	94.8%	-0.8
test/trustedresources.go	10.8%	10.6%	-0.1

Yongxuanzhang · 2023-05-12T17:18:21Z

/assign @wlynch

tekton-robot · 2023-05-12T17:41:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from wlynch after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

Yongxuanzhang · 2023-05-12T17:44:37Z

pkg/trustedresources/verify.go

 // VerifyTask verifies the signature and public key against task.
 // Skip the verification when no policies are found and trusted-resources-verification-no-match-policy is set to ignore or warn
 // Return an error when no policies are found and trusted-resources-verification-no-match-policy is set to fail,
 // or the resource fails to pass matched enforce verification policy
 // refSource contains the source information of the task.
-func VerifyTask(ctx context.Context, taskObj *v1beta1.Task, k8s kubernetes.Interface, refSource *v1beta1.RefSource, verificationpolicies []*v1alpha1.VerificationPolicy) error {
+func VerifyTask(ctx context.Context, taskObj *v1beta1.Task, k8s kubernetes.Interface, refSource *v1beta1.RefSource, verificationpolicies []*v1alpha1.VerificationPolicy) (*VerificationResult, error) {


VerifyTask and VerifyPipeline share lots of common code, I think we may merge them into 1 function in a follow-up pr

tekton-robot · 2023-05-12T17:46:33Z

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/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	94.8%	-0.8
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-12T17:49:09Z

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/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	94.8%	-0.8
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-12T18:24:07Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-12T18:26:11Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.7%	0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

lbernick · 2023-05-15T19:00:32Z

@Yongxuanzhang Thank you for taking the time to write a detailed/helpful commit message! It might be a bit tricky, but I do think it's possible to split this into multiple PRs. For example:

you could introduce a PR where a VerificationResult is just an empty struct
you could introduce the VerificationResult struct by itself and not use it
you could have GetPipeline/GetTask return a verification result and not use it in the pipelinerun reconciler yet
it also seems like there are some unrelated changes in test/trustedresources.go?

Can you play around with this a bit and see what PR splits are possible?

Yongxuanzhang · 2023-05-15T19:12:24Z

@Yongxuanzhang Thank you for taking the time to write a detailed/helpful commit message! It might be a bit tricky, but I do think it's possible to split this into multiple PRs. For example:

you could introduce a PR where a VerificationResult is just an empty struct

you could introduce the VerificationResult struct by itself and not use it

you could have GetPipeline/GetTask return a verification result and not use it in the pipelinerun reconciler yet

it also seems like there are some unrelated changes in test/trustedresources.go?

Can you play around with this a bit and see what PR splits are possible?

Thanks for the suggestions! Let me try to split it as you suggested!
The changes in test/trustedresources.go is used for unit testing across different pkgs

Yongxuanzhang · 2023-05-15T20:24:57Z

/hold
I will try to break it down into smaller prs

[TEP-0091] add VerificationResult #6663

tekton-robot · 2023-05-16T16:40:28Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-16T16:41:14Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-16T19:14:23Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-16T19:15:34Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

This commit adds the ConditionTrustedResourcesVerified for trusted resources into pipelinerun/taskrun status to show if the resources passes verification. Failing to verify will add a false condition to the taskrun/pipelinerun status and passing the verification will add a true status condition. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

tekton-robot · 2023-05-16T19:50:56Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-16T19:52:45Z

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/pipelinerun/pipelinerun.go	89.2%	89.4%	0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.8%	91.3%	-2.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	96.7%	0.1
pkg/reconciler/taskrun/resources/taskref.go	90.0%	88.2%	-1.8
pkg/reconciler/taskrun/taskrun.go	85.0%	85.2%	0.2
pkg/trustedresources/verify.go	95.6%	95.9%	0.3
test/trustedresources.go	10.8%	10.6%	-0.1

tekton-robot · 2023-05-19T17:58:08Z

@Yongxuanzhang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 12, 2023

tekton-robot requested review from dibyom and pritidesai May 12, 2023 17:10

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 12, 2023

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 12, 2023

tekton-robot assigned wlynch May 12, 2023

Yongxuanzhang force-pushed the update-conditions branch from 242703a to 14c3d4d Compare May 12, 2023 17:41

Yongxuanzhang commented May 12, 2023

View reviewed changes

Yongxuanzhang force-pushed the update-conditions branch from 14c3d4d to 726da1b Compare May 12, 2023 18:18

Yongxuanzhang mentioned this pull request May 15, 2023

[TEP-0091] add VerificationResult #6663

Merged

7 tasks

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

pritidesai mentioned this pull request May 16, 2023

[TEP-0091] add condition for trusted resources #6431

Closed

7 tasks

Yongxuanzhang force-pushed the update-conditions branch from d92601a to a17a0cb Compare May 16, 2023 19:45

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2023

Yongxuanzhang closed this Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0091] add condition for trusted resources #6654

[TEP-0091] add condition for trusted resources #6654

Yongxuanzhang commented May 12, 2023 •

edited

Loading

Yongxuanzhang commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

Yongxuanzhang commented May 12, 2023

tekton-robot commented May 12, 2023

Yongxuanzhang May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

lbernick commented May 15, 2023

Yongxuanzhang commented May 15, 2023

Yongxuanzhang commented May 15, 2023 •

edited

Loading

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 19, 2023

[TEP-0091] add condition for trusted resources #6654

[TEP-0091] add condition for trusted resources #6654

Conversation

Yongxuanzhang commented May 12, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

Yongxuanzhang commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

Yongxuanzhang commented May 12, 2023

tekton-robot commented May 12, 2023

Yongxuanzhang May 12, 2023

Choose a reason for hiding this comment

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

lbernick commented May 15, 2023

Yongxuanzhang commented May 15, 2023

Yongxuanzhang commented May 15, 2023 • edited Loading

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 19, 2023

Yongxuanzhang commented May 12, 2023 •

edited

Loading

Yongxuanzhang commented May 15, 2023 •

edited

Loading