[TEP-0091] use VerificationResult in verify #6673
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
release-note-none
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
634e66e
to
8d6e034
Compare
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
09d4506
to
aa07dee
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
pkg/trustedresources/verify.go
Outdated
| } | ||
| } | ||
| return fmt.Errorf("failed to get matched policies: %w", err) | ||
| return &VerificationResult{VerificationResultType: VerificationError, Err: fmt.Errorf("failed to get matched policies: %w", err)} |
There was a problem hiding this comment.
is this different from ErrNoMatchedPolicies?
There was a problem hiding this comment.
this err could be ErrNoMatchedPolicies or other errors from getMatchedPolicies, e.g. ErrRegexMatch
There was a problem hiding this comment.
wouldn't it make sense to do the same thing for the warning then? if this error adds more context, that's probably also useful for the warning
There was a problem hiding this comment.
Oh sorry, I see the issue, I should return a wrapped error for warning as well, we do have information wrapped in the err and shouldn't return a new err directly
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
265cf3d
to
8dde0b1
Compare
|
The following is the coverage report on the affected files.
|
pkg/trustedresources/verify.go
Outdated
| } | ||
| } | ||
| return fmt.Errorf("failed to get matched policies: %w", err) | ||
| return &VerificationResult{VerificationResultType: VerificationError, Err: fmt.Errorf("failed to get matched policies: %w", err)} |
There was a problem hiding this comment.
wouldn't it make sense to do the same thing for the warning then? if this error adds more context, that's probably also useful for the warning
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
8dde0b1
to
dcda49f
Compare
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
dcda49f
to
5abf73e
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
|
/assign |
pkg/trustedresources/verify.go
Outdated
| logger.Warnf(warn.Error()) | ||
| return VerificationResult{VerificationResultType: VerificationWarn, Err: warn} | ||
| } | ||
| if pass := doesAnyVerifierPass(resource, signature, verifiers); pass { |
There was a problem hiding this comment.
why create the variables pass and passverification? do they have different purposes?
There was a problem hiding this comment.
oh yes, thanks! That was a mistake 😞
|
[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
use-verificationresults-in-verify
branch
2 times, most recently
from
ce7403e
to
b583ba6
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| if errors.Is(err, ErrNoMatchedPolicies) { | ||
| switch config.GetVerificationNoMatchPolicy(ctx) { | ||
| case config.IgnoreNoMatchPolicy: | ||
| return nil | ||
| return VerificationResult{VerificationResultType: VerificationSkip} | ||
| case config.WarnNoMatchPolicy: | ||
| logger := logging.FromContext(ctx) | ||
| logger.Warnf("failed to get matched policies: %v", err) | ||
| return nil | ||
| warning := fmt.Errorf("failed to get matched policies: %w", err) | ||
| logger.Warnf(warning.Error()) | ||
| return VerificationResult{VerificationResultType: VerificationWarn, Err: warning} | ||
| } | ||
| } | ||
| return fmt.Errorf("failed to get matched policies: %w", err) | ||
| return VerificationResult{VerificationResultType: VerificationError, Err: fmt.Errorf("failed to get matched policies: %w", err)} | ||
| } |
There was a problem hiding this comment.
I feel that having this logic after getting matched policies inside VerifyTask has 2 problems: 1). it makes the steps in function VerifyTask look like too long and hard to understand. 2). duplicated code exists in VerifyPipeline.
2 suggestions:
- Can we nest this logic checking config into
getMatchedPoliciesfunction itself by passing the context to it? We can refactorgetMatchedPoliciesso that it returns 2 things:[]*v1alpha1.VerificationPolicyand*VerificationResult? If returned*VerificationResultis not nil, we just return the result from inside VerifyTask. If nil, proceed to next step. - Also, can we just rename
getMatchedPoliciestomatchPolicies? see https://google.github.io/styleguide/go/decisions#getters. Perhaps also add a doc string to this function though it's not exported b/c it has a bit more logics.
This will help mitigate the 2 problems mentioned above.
There was a problem hiding this comment.
Thanks! This is a reasonable suggestion. I'm planing to merge these 2 functions into 1. Maybe we could leave this non-related change in the refactoring PR?
If think we have docstrings for getMatchedPolicies.
There was a problem hiding this comment.
For the naming, I think the doc you linked is for getter, which is used as a member function to return some data from the struct, I think it is different from what we have here?
There was a problem hiding this comment.
Doing this in another PR SGTM.
The naming standard is for both functions and member methods. "Function and method names should not use a Get or get prefix".
| vr := trustedresources.VerifyPipeline(ctx, obj, k8s, refSource, verificationPolicies) | ||
| if vr.VerificationResultType == trustedresources.VerificationError { | ||
| if vr.Err != nil { | ||
| return nil, fmt.Errorf("remote Pipeline verification failed for object %s: %w", obj.GetName(), vr.Err) | ||
| } | ||
| return nil, fmt.Errorf("remote Pipeline verification failed for object %s", obj.GetName()) |
There was a problem hiding this comment.
We also returned error in WarnNoMatchPolicy case. Just curious: shouldn't we propagate that as well instead of cutting it off here?
There was a problem hiding this comment.
WarnNoMatchPolicy case will get VerificationWarn type of VerificationResult right? So we don't consider that to be an error.
This commits uses the VerificationResult as the return value for
VerifyTask and VerifyPipeline. Previously returned error will be
replaced with a VerificationError type VerificationResult and error is
in Err field. The cases when nil is returned are currently changed to 3
types of VerificationResult:
1) Verification is skipped
2) Verification passed
3) Warning is logged during verification.
Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Yongxuanzhang
force-pushed
the
use-verificationresults-in-verify
branch
from
b583ba6
to
ac7d2ff
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Changes
This commits uses the VerificationResult as the return value for VerifyTask and VerifyPipeline. Previously returned error will be replaced with a VerificationError type VerificationResult and error is in Err field. The cases when nil is returned are currently changed to 3 types of VerificationResult:
1) Verification is skipped
2) Verification passed
3) Warning is logged during verification.
2nd PR of #6665
Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
/kind feature
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