[TEP-0091] update taskrun and pipelinerun condition based on VerificationResult #6757
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ import ( | |
| resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" | ||
| remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" | ||
| "github.com/tektoncd/pipeline/pkg/trustedresources" | ||
| "github.com/tektoncd/pipeline/pkg/trustedresources/verifier" | ||
| "github.com/tektoncd/pipeline/pkg/workspace" | ||
| "github.com/tektoncd/pipeline/test" | ||
| "github.com/tektoncd/pipeline/test/diff" | ||
|
|
@@ -4958,27 +4959,47 @@ spec: | |
| status: | ||
| podName: the-pod | ||
| `, resolverName)) | ||
|
|
||
| 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 was a problem hiding this comment. There are duplicate messages, will clean up the error handling when the feature is done, added an item #6356 |
||
| } | ||
| passCondition := &apis.Condition{ | ||
| Type: trustedresources.ConditionTrustedResourcesVerified, | ||
| Status: corev1.ConditionTrue, | ||
| } | ||
| failNoKeysCondition := &apis.Condition{ | ||
| Type: trustedresources.ConditionTrustedResourcesVerified, | ||
| Status: corev1.ConditionFalse, | ||
| Message: fmt.Sprintf("failed to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys), | ||
| } | ||
| testCases := []struct { | ||
| name string | ||
| task []*v1beta1.Task | ||
| noMatchPolicy string | ||
| verificationPolicies []*v1alpha1.VerificationPolicy | ||
| wantTrustedResourcesCondition *apis.Condition | ||
| }{{ | ||
| name: "ignore no match policy", | ||
| noMatchPolicy: config.IgnoreNoMatchPolicy, | ||
| verificationPolicies: noMatchPolicy, | ||
| wantTrustedResourcesCondition: nil, | ||
| }, { | ||
| name: "warn no match policy", | ||
| noMatchPolicy: config.WarnNoMatchPolicy, | ||
| verificationPolicies: noMatchPolicy, | ||
| wantTrustedResourcesCondition: failNoMatchCondition, | ||
| }, { | ||
| name: "pass enforce policy", | ||
| noMatchPolicy: config.FailNoMatchPolicy, | ||
| verificationPolicies: vps, | ||
| wantTrustedResourcesCondition: passCondition, | ||
| }, { | ||
| name: "only fail warn policy", | ||
| noMatchPolicy: config.FailNoMatchPolicy, | ||
| verificationPolicies: warnPolicy, | ||
| wantTrustedResourcesCondition: failNoKeysCondition, | ||
| }, | ||
| } | ||
| for _, tc := range testCases { | ||
|
|
@@ -5018,6 +5039,10 @@ status: | |
| if condition != nil && condition.Reason != v1beta1.TaskRunReasonRunning.String() { | ||
| t.Errorf("Expected reason %q but was %s", v1beta1.TaskRunReasonRunning.String(), condition.Reason) | ||
| } | ||
| gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) | ||
| if d := cmp.Diff(tc.wantTrustedResourcesCondition, gotVerificationCondition, ignoreLastTransitionTime); d != "" { | ||
| t.Error(diff.PrintWantGot(d)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -5122,6 +5147,10 @@ status: | |
| if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != podconvert.ReasonResourceVerificationFailed { | ||
| t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", podconvert.ReasonResourceVerificationFailed, tr.Status.Conditions) | ||
| } | ||
| gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) | ||
| if gotVerificationCondition == nil || gotVerificationCondition.Status != corev1.ConditionFalse { | ||
| t.Errorf("Expected to have false condition, but had %v", gotVerificationCondition) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same
switch-caseseems to be repeated. Can this be wrapped into a function to avoid repitition?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try out, I think we could wrap it into a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PipelineRuns returns only seem to be happening for the case of
verificationError. In one case, you returncontroller.New...and in other case you returnnil, controller.New....What if this function returned
error, conditionToSet? You could catch that and handle it in both cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ is how you would use it well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this should work, just pushed the code.