merge VerifyTask and VerifyPipeline into VerifyResource #6724

Yongxuanzhang · 2023-05-25T21:38:54Z

Changes

This commit merges VerifyTask and VerifyPipeline into VerifyResource to reduce duplicate code.

/kind cleanup

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

action required: VerifyTask and VerifyPipeline are now merged into 1 function VerifyResource, please update the usages if upgrade to the new release

tekton-robot · 2023-05-25T21:38:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tekton-robot · 2023-05-25T21:46:47Z

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/trustedresources/verify.go	96.3%	94.9%	-1.4

Yongxuanzhang · 2023-05-25T21:53:22Z

/assign @lbernick
/assign @chuangw6
/assign @wlynch
Hi, ptal. Many thanks!

tekton-robot · 2023-05-25T21:56:03Z

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/trustedresources/verify.go	96.3%	95.9%	-0.4

tekton-robot · 2023-05-25T21:57:46Z

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/trustedresources/verify.go	96.3%	95.9%	-0.4

Yongxuanzhang · 2023-05-26T04:45:36Z

/retest
https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6724/pull-tekton-pipeline-alpha-integration-tests/1661852264114950144/build-log.txt
might be some issue with resolving the images:

         "step-show-user-2000" exited with code 1 (image: "docker.io/library/ubuntu@sha256:dfd64a3b4296d8c9b62aa3309984f8620b98d87e47492599ee20739e8eb54fbf"); for logs run: kubectl -n arendelle-s9bkm logs show-non-root-steps-run-jghvr-pod -c step-show-user-2000

lbernick · 2023-05-26T13:28:18Z

pkg/trustedresources/verify.go

+		tm, signature, err := prepareObjectMeta(v.ObjectMeta)
+		if err != nil {
+			return VerificationResult{VerificationResultType: VerificationError, Err: err}


can this block be moved out of the switch statement? I think metav1.Object should have ObjectMeta.

Can you also link the TODO for supporting v1 task/pipeline verification?

I tried but metav1.Object doesn't have ObjectMeta, it is an interface and has lots of methods.

pkg/trustedresources/verify.go

lbernick · 2023-05-26T13:29:48Z

/retest https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6724/pull-tekton-pipeline-alpha-integration-tests/1661852264114950144/build-log.txt might be some issue with resolving the images:
         "step-show-user-2000" exited with code 1 (image: "docker.io/library/ubuntu@sha256:dfd64a3b4296d8c9b62aa3309984f8620b98d87e47492599ee20739e8eb54fbf"); for logs run: kubectl -n arendelle-s9bkm logs show-non-root-steps-run-jghvr-pod -c step-show-user-2000

Can you please open an issue tracking this flake?

tekton-robot · 2023-05-26T15:30:41Z

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/trustedresources/verify.go	96.3%	95.9%	-0.4

tekton-robot · 2023-05-26T15:35:16Z

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/trustedresources/verify.go	96.3%	95.9%	-0.4

Yongxuanzhang · 2023-05-26T15:35:54Z

I was thinking whether we should open an issue for this, since it doesn't seem to be an issue from tekton? I will open one just for record

The root bug isn't with Tekton, but knowing that this test is flaky will help us inform if we want to keep it in our test suite and help others know if a test failure they're seeing has been seen elsewhere.

lbernick · 2023-05-26T16:16:40Z

sorry Yongxuan I accidentally edited your comment instead of quote replying it!

Yongxuanzhang · 2023-05-26T16:33:53Z

sorry Yongxuan I accidentally edited your comment instead of quote replying it!

Oh I see, thanks! I'm opening one now.

JeromeJu · 2023-05-26T18:37:36Z

pkg/trustedresources/verify.go

 // VerificationResult is returned with different types for different cases:
 // 1) Return VerificationResult with VerificationSkip type, when no policies are found and no-match-policy is set to ignore
 // 2) Return VerificationResult with VerificationPass type when verification passed;
 // 3) Return VerificationResult with VerificationWarn type, when no matching policies and feature flag "no-match-policy" is "warn", or only Warn mode verification policies fail. Err field is filled with the warning;
 // 4) Return VerificationResult with VerificationError type when no policies are found and no-match-policy is set to fail, the resource fails to pass matched enforce verification policy, or there are errors during verification. Err is filled with the err.
-// 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) VerificationResult {


Wondering if we should preserve refactored functions like what we did for deprecated fields if that would be better for users? ie. func VerifyTask(..., task, ...) {VerifyResource(..., task, ...)} ?

That's a good point, 🤔 Let me try that

Yongxuanzhang · 2023-05-26T19:16:16Z

/test pull-tekton-pipeline-go-coverage
network issue

tekton-robot · 2023-05-26T19:19: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/trustedresources/verify.go	96.3%	94.0%	-2.3

tekton-robot · 2023-05-26T19:21:49Z

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/trustedresources/verify.go	96.3%	94.0%	-2.3

tekton-robot · 2023-05-26T21:46:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, lbernick

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~~ [lbernick]

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

JeromeJu · 2023-05-29T15:34:56Z

/lgtm

This commit merges VerifyTask and VerifyPipeline into VerifyResource to reduce duplicate code. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

tekton-robot · 2023-05-29T16:31:14Z

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/trustedresources/verify.go	96.3%	94.0%	-2.3

tekton-robot · 2023-05-29T16:33:16Z

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/trustedresources/verify.go	96.3%	94.0%	-2.3

tekton-robot · 2023-05-29T16:34: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/trustedresources/verify.go	96.3%	94.0%	-2.3

JeromeJu · 2023-05-29T17:27:18Z

/lgtm

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2023

tekton-robot requested review from dibyom and lbernick May 25, 2023 21:39

Yongxuanzhang force-pushed the merge-verify branch from 415e770 to e19203d Compare May 25, 2023 21:48

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesnt merit a release note. labels May 25, 2023

Yongxuanzhang marked this pull request as ready for review May 25, 2023 21:50

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2023

tekton-robot requested review from afrittoli and wlynch May 25, 2023 21:50

Yongxuanzhang changed the title ~~refactor trusted resources to merge VerifyTask and VerifyPipeline~~ merge VerifyTask and VerifyPipeline into VerifyResource May 25, 2023

tekton-robot assigned chuangw6, lbernick and wlynch May 25, 2023

lbernick approved these changes May 26, 2023

View reviewed changes

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

Yongxuanzhang force-pushed the merge-verify branch from e19203d to 7432654 Compare May 26, 2023 15:25

Yongxuanzhang mentioned this pull request May 26, 2023

run-steps-as-non-root is flaky #6730

Closed

JeromeJu reviewed May 26, 2023

View reviewed changes

Yongxuanzhang force-pushed the merge-verify branch from 7432654 to 728a4de Compare May 26, 2023 19:03

Yongxuanzhang requested a review from JeromeJu May 26, 2023 19:04

JeromeJu approved these changes May 26, 2023

View reviewed changes

tekton-robot assigned JeromeJu May 29, 2023

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

Yongxuanzhang force-pushed the merge-verify branch from 728a4de to fdb8336 Compare May 29, 2023 16:24

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

refactor trusted resources to merge VerifyTask and VerifyPipeline

ab53cb7

This commit merges VerifyTask and VerifyPipeline into VerifyResource to reduce duplicate code. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Yongxuanzhang force-pushed the merge-verify branch from fdb8336 to ab53cb7 Compare May 29, 2023 16:25

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

tekton-robot merged commit 87094b9 into tektoncd:main May 29, 2023

Yongxuanzhang mentioned this pull request May 26, 2023

[TEP-0091] Support v1 Tasks&Pipelines for trusted resources #6729

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge VerifyTask and VerifyPipeline into VerifyResource #6724

merge VerifyTask and VerifyPipeline into VerifyResource #6724

Yongxuanzhang commented May 25, 2023 •

edited

Loading

tekton-robot commented May 25, 2023

tekton-robot commented May 25, 2023

Yongxuanzhang commented May 25, 2023 •

edited

Loading

tekton-robot commented May 25, 2023

tekton-robot commented May 25, 2023

Yongxuanzhang commented May 26, 2023

lbernick May 26, 2023

Yongxuanzhang May 26, 2023

lbernick commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

Yongxuanzhang commented May 26, 2023 •

edited by lbernick

Loading

lbernick commented May 26, 2023

Yongxuanzhang commented May 26, 2023

JeromeJu May 26, 2023 •

edited

Loading

Yongxuanzhang May 26, 2023

Yongxuanzhang May 26, 2023

Yongxuanzhang commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

JeromeJu commented May 29, 2023

tekton-robot commented May 29, 2023

tekton-robot commented May 29, 2023

tekton-robot commented May 29, 2023

JeromeJu commented May 29, 2023

merge VerifyTask and VerifyPipeline into VerifyResource #6724

merge VerifyTask and VerifyPipeline into VerifyResource #6724

Conversation

Yongxuanzhang commented May 25, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 25, 2023

tekton-robot commented May 25, 2023

Yongxuanzhang commented May 25, 2023 • edited Loading

tekton-robot commented May 25, 2023

tekton-robot commented May 25, 2023

Yongxuanzhang commented May 26, 2023

lbernick May 26, 2023

Choose a reason for hiding this comment

Yongxuanzhang May 26, 2023

Choose a reason for hiding this comment

lbernick commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

Yongxuanzhang commented May 26, 2023 • edited by lbernick Loading

lbernick commented May 26, 2023

Yongxuanzhang commented May 26, 2023

JeromeJu May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Yongxuanzhang May 26, 2023

Choose a reason for hiding this comment

Yongxuanzhang May 26, 2023

Choose a reason for hiding this comment

Yongxuanzhang commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

JeromeJu commented May 29, 2023

tekton-robot commented May 29, 2023

tekton-robot commented May 29, 2023

tekton-robot commented May 29, 2023

JeromeJu commented May 29, 2023

Yongxuanzhang commented May 25, 2023 •

edited

Loading

Yongxuanzhang commented May 25, 2023 •

edited

Loading

Yongxuanzhang commented May 26, 2023 •

edited by lbernick

Loading

JeromeJu May 26, 2023 •

edited

Loading