refactor trusted resources verification to move verification to reconcile #6502
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test all |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test all |
|
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.
|
|
/test all |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
7ce383c to
81ba41c
Compare
|
/test all |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
81ba41c to
0f4f4ca
Compare
|
/test all |
|
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.
|
|
The following is the coverage report on the affected files.
|
| TaskName string | ||
| Kind v1beta1.TaskKind | ||
| TaskSpec *v1beta1.TaskSpec | ||
| Task *v1beta1.Task |
There was a problem hiding this comment.
Some of this data seems duplicated - e.g. wouldn't TaskSpec/Name be included in Task?
There was a problem hiding this comment.
In cases like inline task&taskspec already in status, there is no "task" but we have the "taskSpec", so I think we cannot use task here to replace the other fields? Another reason is try to reduce the code changes
There was a problem hiding this comment.
This is a bit confusing though because there are similar pieces of data and it's not clear when to use one over the other. e.g.
- Task.spec vs TaskSpec
- Task.ObjectMeta.Name vs TaskName
- Task.TypeMeta.Kind vs Kind
If these are the same, then we should ideally just use a single type (probably Task). If they're different then we should document the differences and explain when you'd want to use each.
There was a problem hiding this comment.
Emmm, I agree there are similar pieces of data, it looks like some data may be contained in other data and this may get people confused.
But we probably cannot use Task.*** to replace TaskSpec, TaskName and Kind (correct me if I'm wrong), take this code for example:
pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 678 to 704 in 3d6ec63
the TaskSpec is from *taskRun.Status.TaskSpec,
the taskName is from pipelineTask.TaskRef.Name and kind is from pipelineTask.TaskRef.Kind. There is no "Task" in the pipeline taskref case,
I will add doc strings for these fields to clarify. And maybe rename the Task to ReferredTask(since it is not inlined task)?
| vp, err := c.verificationPolicyLister.VerificationPolicies(pr.Namespace).List(labels.Everything()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list VerificationPolicies from namespace %s with error %w", pr.Namespace, err) | ||
| } | ||
| var refSourceURI string | ||
| if pipelineMeta.RefSource != nil { | ||
| refSourceURI = pipelineMeta.RefSource.URI | ||
| } |
There was a problem hiding this comment.
This looks duplicated with the other verify calls. Should we just move this into the verify func if it's always needed?
e747c3a to
1ed3a35
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| @@ -703,7 +700,7 @@ func resolveTask( | |||
| } else { | |||
| spec = pipelineTask.TaskSpec.TaskSpec | |||
| } | |||
| return spec, taskName, kind, err | |||
| return spec, taskName, kind, t.DeepCopy(), refSource, err | |||
There was a problem hiding this comment.
Similar to the ResolvedTask object below, could this be simplified to (*v1beta1.Task, *v1beta1.RefSource, error)?
- task can come from t.TaskSpec
- taskName can come from t.ObjectMeta.Name
- kind can come from t.TypeMeta
There was a problem hiding this comment.
Should this also be a ResolvedTask? 🤔
There was a problem hiding this comment.
Should this also be a ResolvedTask? 🤔
🤔 Yeah that's a great suggestion, I think we could refactor this part of code a bit
There was a problem hiding this comment.
Refactored the code to return a ResolvedTask here
| TaskName string | ||
| Kind v1beta1.TaskKind | ||
| TaskSpec *v1beta1.TaskSpec | ||
| Task *v1beta1.Task |
There was a problem hiding this comment.
This is a bit confusing though because there are similar pieces of data and it's not clear when to use one over the other. e.g.
- Task.spec vs TaskSpec
- Task.ObjectMeta.Name vs TaskName
- Task.TypeMeta.Kind vs Kind
If these are the same, then we should ideally just use a single type (probably Task). If they're different then we should document the differences and explain when you'd want to use each.
| if pipelineMeta != nil { | ||
| pipelineToBeVerified = &v1beta1.Pipeline{ | ||
| ObjectMeta: *pipelineMeta.ObjectMeta, | ||
| Spec: *pipelineSpec, |
There was a problem hiding this comment.
These could panic if they're nil (same with taskToBeVerified below)
There was a problem hiding this comment.
Did you mean pipelineMeta.ObjectMeta and pipelineSpec could be nil? Both of them are from
pipeline/pkg/reconciler/pipelinerun/pipelinespec/pipelinespec.go
Lines 35 to 38 in 54c9b51
and they are initialized so I think they cannot be nil?
There was a problem hiding this comment.
That's not obvious from this func though 🙃
I think you should change this to:
if pipelineMeta != nil && pipelineSpec != nil {
pipelineToBeVerified = &v1beta1.Pipeline{
ObjectMeta: pipelineMeta.ObjectMeta,
Spec: *pipelineSpec,
}
}There was a problem hiding this comment.
Oh ok, I will update
|
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.
|
1d2eadc to
9d1c684
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| TaskName string | ||
| Kind v1beta1.TaskKind | ||
| TaskSpec *v1beta1.TaskSpec | ||
| // PipelineResolvedTask is used to store the Pipeline resolved task for trusted resources verification | ||
| PipelineResolvedTask *v1beta1.Task | ||
| // RefSource is used to store the RefSource of Pipeline task | ||
| RefSource *v1beta1.RefSource |
There was a problem hiding this comment.
Same question as before - how should callers interpret the difference between overlapping fields TaskSpec and PipelineResolvedTask.TaskSpec, TaskName and PipelineResolvedTask.Name, etc.?
| if pipelineMeta != nil { | ||
| pipelineToBeVerified = &v1beta1.Pipeline{ | ||
| ObjectMeta: *pipelineMeta.ObjectMeta, | ||
| Spec: *pipelineSpec, |
There was a problem hiding this comment.
That's not obvious from this func though 🙃
I think you should change this to:
if pipelineMeta != nil && pipelineSpec != nil {
pipelineToBeVerified = &v1beta1.Pipeline{
ObjectMeta: pipelineMeta.ObjectMeta,
Spec: *pipelineSpec,
}
}…cile This commit refactor the trusted resources code, move verification logic from GetVerifiedTaskFunc and GetVerifiedPipelineFunc to reconcile. This will help to set the conditions for taskrun/pipelinerun. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
9d1c684 to
d5daaab
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/hold We will try to refactor the code to not block v1 |
Changes
This commit refactor the trusted resources code, move verification logic from GetVerifiedTaskFunc and GetVerifiedPipelineFunc to reconcile. This will help to set the conditions for taskrun/pipelinerun.
/kind misc
Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
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