[TEP-0091] add VerificationResult #6663
Conversation
tekton-robot
added
kind/feature
tekton-robot
added
the
size/S
|
/test check-pr-has-kind-label |
|
@Yongxuanzhang: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
pkg/trustedresources/verify.go
Outdated
| // 3) VerificationPass: The verification passes. | ||
| // 4) WarnModeVerificationFail: Only Warn mode verification policies fail. | ||
| VerificationResultType VerificationResultType | ||
| // Message contains the warning message when there is a warning logged. |
There was a problem hiding this comment.
just curious, is this a k8s style "Reason" e.g. "NoMatchingVerificationPolicies" or meant to be human-readable?
There was a problem hiding this comment.
I think this meant to be human-readable, my plan is to update the condition message with this message
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
from
4373dd4
to
a047741
Compare
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
6 times, most recently
from
271f92e
to
9dd7a1e
Compare
|
/assign @lbernick |
|
/assign @chuangw6 |
pkg/trustedresources/verify.go
Outdated
| @@ -41,6 +41,30 @@ const ( | |||
| SignatureAnnotation = "tekton.dev/signature" | |||
| ) | |||
|
|
|||
| const ( | |||
| IgnoreWhenNoMatchPolicy = iota | |||
There was a problem hiding this comment.
Suggest more consistency in naming, e.g.
NoMatchingPoliciesIgnore
NoMatchingPoliciesWarn
NoMatchingPoliciesError
VerificationWarning
VerificationFailed
VerificationPassed
Also I think we still need a mode for no matching policies + failure?
There was a problem hiding this comment.
no matching policies + failure belongs to VerificationError, all previous returned error cases go into this case since we don't need to have different types for them, there will be only 1 case when updating the condition.
There was a problem hiding this comment.
How about this?
VerificationError
VerificationWarn
VerificationSuccess
VerificationSkip
I realized that NoMatchingPoliciesWarn and WarnModeVerificationFail are actually the same case when updating the condition
There was a problem hiding this comment.
My 2c:
what result mode we need here depends on what the tekton controller will be doing with different verification result and what level of details we want to surface to the users. @Yongxuanzhang Can you please add some details to the PR description about that?
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
from
9dd7a1e
to
b7118bf
Compare
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
from
b7118bf
to
f63b7dd
Compare
pkg/trustedresources/verify.go
Outdated
| // 2) VerificationPass: The verification passed. Err is nil in this case. | ||
| // 3) VerificationWarn: A warning is logged. It could be no matching policies and feature flag "trusted-resources-verification-no-match-policy" is warn, | ||
| // or only Warn mode verification policies fail. | ||
| // 4) VerificationError: The verification failed, it could be the signature doesn't match the public key or there are errors during verification. |
There was a problem hiding this comment.
This could also happen if there are no matching policies and "no-match-policy" is set to "fail" right?
There was a problem hiding this comment.
ah yes, I missed that case, will add now
|
[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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
from
f63b7dd
to
66820b4
Compare
The commit adds VerificationResult struct, the new struct has 2 fields, VerificationResultType and Err. VerificationResultType has 4 types: VerificationSkip, VerificationPass, VerificationWarn, VerificationError. VerificationResult will be used in reconciler to update taskrun, pipelinerun conditions. Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang
force-pushed
the
add-verificationresults
branch
from
c19d24a
to
3c7bfb4
Compare
|
/lgtm |
|
/retest a possible flaky test? https://prow.tekton.dev/view/gs/tekton-prow/pr-logs/pull/tektoncd_pipeline/6663/pull-tekton-pipeline-alpha-integration-tests/1658558758315364352 This PR is not related to |
Could you create an issue to track the flake? |
Sure, I'm trying to check the logs and see what could be the issue, but still no idea. It seems that we're experiencing more flaky tests recently? |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@Yongxuanzhang: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
Changes
The commit adds VerificationResult struct, the new struct has 2 fields, VerificationResultType and Err.
VerificationResultType has 4 types: VerificationSkip, VerificationPass,
VerificationWarn, VerificationError.
VerificationResult will be used in reconciler to update taskrun, pipelinerun conditions, conditions' message will be filled with Err from VerificationResult.
This PR is a split from #6654.
/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:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes