[TEP-0091] update taskrun and pipelinerun condition based on VerificationResult #6757

Yongxuanzhang · 2023-06-01T22:53:52Z

Changes

This commits updates taskrun and pipelinerun condition based on VerificationResult to add TrustedResourcesVerified condition. The condition will be marked as false if verification policy fails, or no matching policies when feature flag is set to fail. The condition will be set to true if verification passes. No condition is added when verification is skipped.

Last feature PR of #6665

/kind feature

Signed-off-by: Yongxuan Zhang [email protected]

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

TrustedResourcesVerified is added to TaskRun/PipelineRun status if trusted resources is enabled, the condition indicates the result of the verification.

tekton-robot · 2023-06-01T22:53:54Z

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

Yongxuanzhang · 2023-06-01T22:54:03Z

/test all

tekton-robot · 2023-06-01T22:59:44Z

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/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-01T23:02:49Z

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/taskrun/taskrun.go	84.6%	84.7%	0.1

Yongxuanzhang · 2023-06-01T23:09:27Z

pkg/reconciler/taskrun/taskrun_test.go

+	failNoMatchCondition := &apis.Condition{
+		Type:    trustedresources.ConditionTrustedResourcesVerified,
+		Status:  corev1.ConditionFalse,
+		Message: fmt.Sprintf("failed to get matched policies: %s: no matching policies are found for resource: %s against source: %s", trustedresources.ErrNoMatchedPolicies, ts.Name, ""),


There are duplicate messages, will clean up the error handling when the feature is done, added an item #6356

tekton-robot · 2023-06-01T23:12:05Z

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/taskrun/taskrun.go	84.6%	84.7%	0.1

Yongxuanzhang · 2023-06-01T23:12:10Z

pkg/trustedresources/verify.go

+	// ConditionTrustedResourcesVerified specifies that the resources pass trusted resources verification or not.
+	ConditionTrustedResourcesVerified apis.ConditionType = "TrustedResourcesVerified"


this may not be the best place to put the ConditionType. But I don't know if there are any other places better than this

tekton-robot · 2023-06-01T23:15:56Z

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/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-02T19:32: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	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

lbernick

coverage report shows all the trusted resources conditions are covered 👍

/meow

lbernick · 2023-06-05T18:57:17Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	failNoKeysCondition := &apis.Condition{
+		Type:    trustedresources.ConditionTrustedResourcesVerified,
+		Status:  corev1.ConditionFalse,
+		Message: fmt.Sprintf("fails to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys),


Suggested change

Message: fmt.Sprintf("fails to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys),

Message: fmt.Sprintf("failed to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys),

tekton-robot · 2023-06-05T19:04:11Z

@lbernick:

In response to this:

coverage report shows all the trusted resources conditions are covered 👍

/meow

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 · 2023-06-05T19:04:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

tekton-robot · 2023-06-05T19:21:54Z

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	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-05T19:26: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/reconciler/pipelinerun/pipelinerun.go	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

chitrangpatel · 2023-06-05T20:49:01Z

pkg/reconciler/pipelinerun/pipelinerun.go

+					Message: err.Error(),
+				})
+				return nil, controller.NewPermanentError(err)
+			case trustedresources.VerificationSkip:


can this not be the default?

Did you mean something like this?

switch verificationResult.VerificationResultType { case trustedresources.VerificationError: xxx case trustedresources.VerificationWarn: xxx case trustedresources.VerificationPass: xxx default:

yes. Because that condition was not doing anything. However, it is clearer for readability compared to default so I'm ok with leaving this as is too.

chitrangpatel · 2023-06-05T20:50:53Z

pkg/reconciler/pipelinerun/pipelinerun.go

+				Type:   trustedresources.ConditionTrustedResourcesVerified,
+				Status: corev1.ConditionTrue,
+			})
+		}


The same switch-case seems to be repeated. Can this be wrapped into a function to avoid repitition?

There are 3 places using the same switch, 2 on pipelinerun and 1 on taskrun. The 2 pipelinerun returns are different.
Do you have any suggestions? 🤔

let me try out, I think we could wrap it into a function

The PipelineRuns returns only seem to be happening for the case of verificationError. In one case, you return controller.New... and in other case you return nil, controller.New....

What if this function returned error, conditionToSet? You could catch that and handle it in both cases?

Something like?

func Verify(verificationResult <typeOfverificationResult>) (error, apis.Condition){ var condition apis.Condition var err error switch verificationResult.VerificationResultType { case trustedresources.VerificationError: err := fmt.Errorf("PipelineRun %s/%s referred pipeline failed signature verification: %w", pr.Namespace, pr.Name, verificationResult.Err) // add this line after the output of the function // pr.Status.MarkFailed(ReasonResourceVerificationFailed, err.Error()) condition = &apis.Condition{ Type: trustedresources.ConditionTrustedResourcesVerified, Status: corev1.ConditionFalse, Message: err.Error(), } // add this line after the output of the function // return controller.NewPermanentError(err) case trustedresources.VerificationSkip: // do nothing case trustedresources.VerificationWarn: condition = &apis.Condition{ Type: trustedresources.ConditionTrustedResourcesVerified, Status: corev1.ConditionFalse, Message: verificationResult.Err.Error(), } // set the condition when you catch the output of the function case trustedresources.VerificationPass: condition = apis.Condition{ Type: trustedresources.ConditionTrustedResourcesVerified, Status: corev1.ConditionTrue, } } return err, condition } err, cond := Verify(verificationResult) pr.Status.SetCondition(&cond) if err != nil { pr.Status.MarkFailed(ReasonResourceVerificationFailed, err.Error()) return controller.NewPermanentError(err) }

☝️ is how you would use it well.

Does that work?

Yeah this should work, just pushed the code.

chitrangpatel · 2023-06-05T21:18:47Z

/lgtm

tekton-robot · 2023-06-05T21:24:07Z

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	90.4%	90.5%	0.2
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-05T21:27:09Z

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	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

chitrangpatel · 2023-06-05T21:27:51Z

/lgtm

tekton-robot · 2023-06-05T21:28:12Z

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	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-05T21:29:22Z

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	90.4%	90.5%	0.1
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

This commits updates taskrun and pipelinerun condition based on VerificationResult to add TrustedResourcesVerified condition. The condition will be marked as false if verification policy fails, or no matching policies when feature flag is set to fail. The condition will be set to true if verification passes. No condition is added when verification is skipped. Signed-off-by: Yongxuan Zhang [email protected]

chitrangpatel · 2023-06-05T21:56:15Z

/lgtm

tekton-robot · 2023-06-05T21:58:46Z

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	90.4%	90.5%	0.2
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

tekton-robot · 2023-06-05T22:01:49Z

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	90.4%	90.5%	0.2
pkg/reconciler/taskrun/taskrun.go	84.6%	84.7%	0.1

Yongxuanzhang · 2023-06-05T22:30:11Z

/retest
same #6772
log: https://prow.tekton.dev/view/gs/tekton-prow/pr-logs/pull/tektoncd_pipeline/6757/pull-tekton-pipeline-beta-integration-tests/1665839335057920000

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 1, 2023

tekton-robot requested review from imjasonh and jerop June 1, 2023 22:53

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 1, 2023

Yongxuanzhang changed the title ~~update taskrun condition based on VerificationResult~~ [TEP-0091]update taskrun condition based on VerificationResult Jun 1, 2023

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Jun 1, 2023

Yongxuanzhang force-pushed the add-tr-condition branch from ee36af9 to f95fa03 Compare June 1, 2023 23:06

Yongxuanzhang marked this pull request as ready for review June 1, 2023 23:07

tekton-robot requested review from abayer and afrittoli June 1, 2023 23:07

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2023

Yongxuanzhang commented Jun 1, 2023

View reviewed changes

Yongxuanzhang changed the title ~~[TEP-0091]update taskrun condition based on VerificationResult~~ [TEP-0091] update taskrun condition based on VerificationResult Jun 1, 2023

Yongxuanzhang commented Jun 1, 2023

View reviewed changes

Yongxuanzhang force-pushed the add-tr-condition branch from f95fa03 to 1fe170b Compare June 2, 2023 19:24

Yongxuanzhang changed the title ~~[TEP-0091] update taskrun condition based on VerificationResult~~ [TEP-0091] update taskrun and pipelinerun condition based on VerificationResult Jun 2, 2023

Yongxuanzhang mentioned this pull request Jun 2, 2023

[TEP-0091] Add condition to reflect verification failure/success. #6665

Closed

6 tasks

Yongxuanzhang force-pushed the add-tr-condition branch from 1fe170b to 62318c9 Compare June 2, 2023 19:27

lbernick approved these changes Jun 5, 2023

View reviewed changes

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

Yongxuanzhang force-pushed the add-tr-condition branch from 62318c9 to f4e05d7 Compare June 5, 2023 19:16

chitrangpatel reviewed Jun 5, 2023

View reviewed changes

Yongxuanzhang force-pushed the add-tr-condition branch from f4e05d7 to d080cf3 Compare June 5, 2023 21:15

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

Yongxuanzhang force-pushed the add-tr-condition branch from d080cf3 to 4840463 Compare June 5, 2023 21:18

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

Yongxuanzhang force-pushed the add-tr-condition branch from 4840463 to a60e8b0 Compare June 5, 2023 21:21

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

Yongxuanzhang force-pushed the add-tr-condition branch from a60e8b0 to 97f37ca Compare June 5, 2023 21:53

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

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

tekton-robot merged commit ee9a5e3 into tektoncd:main Jun 5, 2023

		// ConditionTrustedResourcesVerified specifies that the resources pass trusted resources verification or not.
		ConditionTrustedResourcesVerified apis.ConditionType = "TrustedResourcesVerified"

	Message: fmt.Sprintf("fails to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys),
	Message: fmt.Sprintf("failed to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys),

[TEP-0091] update taskrun and pipelinerun condition based on VerificationResult #6757

[TEP-0091] update taskrun and pipelinerun condition based on VerificationResult #6757

Conversation

Yongxuanzhang commented Jun 1, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 1, 2023

Yongxuanzhang commented Jun 1, 2023

tekton-robot commented Jun 1, 2023

tekton-robot commented Jun 1, 2023

Yongxuanzhang Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jun 1, 2023

Yongxuanzhang Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jun 1, 2023

tekton-robot commented Jun 2, 2023

lbernick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitrangpatel Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitrangpatel commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

chitrangpatel commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

chitrangpatel commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

tekton-robot commented Jun 5, 2023

Yongxuanzhang commented Jun 5, 2023

Yongxuanzhang commented Jun 1, 2023 •

edited

Loading

Yongxuanzhang Jun 1, 2023 •

edited

Loading

Yongxuanzhang Jun 1, 2023 •

edited

Loading

chitrangpatel Jun 5, 2023 •

edited

Loading