From b8c24629aa4d7c1cbaec2a4b3f89a01a9230ae9b Mon Sep 17 00:00:00 2001 From: Jerome Ju Date: Wed, 29 Mar 2023 11:45:46 -0400 Subject: [PATCH] Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconciler This commit separates v1beta1.TaskObject to Task and ClusterTask to get prepared for v1 storage swap. --- .../resources/pipelinerunresolution.go | 15 +- .../resources/pipelinerunresolution_test.go | 36 ++-- pkg/reconciler/taskrun/resources/taskref.go | 70 +++---- .../taskrun/resources/taskref_test.go | 71 ++++--- pkg/reconciler/taskrun/resources/taskspec.go | 23 ++- .../taskrun/resources/taskspec_test.go | 176 +++++++++++------- pkg/trustedresources/verify.go | 7 +- pkg/trustedresources/verify_test.go | 4 +- 8 files changed, 241 insertions(+), 161 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 7524c980600..8f7c24aacdd 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -692,7 +692,8 @@ func resolveTask( pipelineTask v1beta1.PipelineTask, ) (v1beta1.TaskSpec, string, v1beta1.TaskKind, error) { var ( - t v1beta1.TaskObject + t *v1beta1.Task + ct *v1beta1.ClusterTask err error spec v1beta1.TaskSpec taskName string @@ -707,7 +708,7 @@ func resolveTask( } else { // Following minimum status principle (TEP-0100), no need to propagate the source about PipelineTask up to PipelineRun status. // Instead, the child TaskRun's status will be the place recording the source of individual task. - t, _, err = getTask(ctx, pipelineTask.TaskRef.Name) + t, ct, _, err = getTask(ctx, pipelineTask.TaskRef.Name) switch { case errors.Is(err, remote.ErrRequestInProgress): return v1beta1.TaskSpec{}, "", "", err @@ -719,8 +720,14 @@ func resolveTask( Msg: err.Error(), } default: - spec = t.TaskSpec() - taskName = t.TaskMetadata().Name + if ct == nil && t != nil { + spec = t.TaskSpec() + taskName = t.TaskMetadata().Name + } + if ct != nil && t == nil { + spec = ct.TaskSpec() + taskName = ct.TaskMetadata().Name + } } } kind = pipelineTask.TaskRef.Kind diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index e69a9e5d44d..4fa5d7d0fe2 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -47,8 +47,8 @@ import ( func nopGetRun(string) (v1beta1.RunObject, error) { return nil, errors.New("GetRun should not be called") } -func nopGetTask(context.Context, string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, errors.New("GetTask should not be called") +func nopGetTask(context.Context, string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, errors.New("GetTask should not be called") } func nopGetTaskRun(string) (*v1beta1.TaskRun, error) { return nil, errors.New("GetTaskRun should not be called") @@ -1871,8 +1871,8 @@ func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) { TaskRef: &v1beta1.TaskRef{Name: "task"}, }} - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil } pr := v1beta1.PipelineRun{ @@ -1921,8 +1921,8 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) { }}} // Return an error when the Task is retrieved, as if it didn't exist - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, kerrors.NewNotFound(v1beta1.Resource("task"), name) + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, kerrors.NewNotFound(v1beta1.Resource("task"), name) } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, kerrors.NewNotFound(v1beta1.Resource("taskrun"), name) @@ -1962,8 +1962,8 @@ func TestResolvePipelineRun_VerificationFailed(t *testing.T) { }}, }}} - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, trustedresources.ErrResourceVerificationFailed + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, trustedresources.ErrResourceVerificationFailed } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil } pr := v1beta1.PipelineRun{ @@ -2199,8 +2199,8 @@ func TestResolvePipeline_WhenExpressions(t *testing.T) { WhenExpressions: []v1beta1.WhenExpression{ptwe1}, } - getTask := func(_ context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(_ context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } pr := v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ @@ -2232,8 +2232,8 @@ func TestIsCustomTask(t *testing.T) { Name: "pipelinerun", }, } - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil } getRun := func(name string) (v1beta1.RunObject, error) { return nil, nil } @@ -2999,8 +2999,8 @@ func TestIsMatrixed(t *testing.T) { Name: "pipelinerun", }, } - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil } getRun := func(name string) (v1beta1.RunObject, error) { return &runs[0], nil } @@ -3133,8 +3133,8 @@ func TestResolvePipelineRunTask_WithMatrix(t *testing.T) { }}}, } - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return taskRunsMap[name], nil } getRun := func(name string) (v1beta1.RunObject, error) { return &runs[0], nil } @@ -3237,8 +3237,8 @@ func TestResolvePipelineRunTask_WithMatrixedCustomTask(t *testing.T) { }}}, }} - getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, nil, nil + getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return task, nil, nil, nil } getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil } getRun := func(name string) (v1beta1.RunObject, error) { return runsMap[name], nil } diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index b8f343725c3..ca211fee288 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -62,7 +62,7 @@ func GetTaskFuncFromTaskRun(ctx context.Context, k8s kubernetes.Interface, tekto // if the spec is already in the status, do not try to fetch it again, just use it as source of truth. // Same for the Source field in the Status.Provenance. if taskrun.Status.TaskSpec != nil { - return func(_ context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { + return func(_ context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { var configsource *v1beta1.ConfigSource if taskrun.Status.Provenance != nil { configsource = taskrun.Status.Provenance.ConfigSource @@ -73,7 +73,7 @@ func GetTaskFuncFromTaskRun(ctx context.Context, k8s kubernetes.Interface, tekto Namespace: taskrun.Namespace, }, Spec: *taskrun.Status.TaskSpec, - }, configsource, nil + }, nil, configsource, nil } } return GetVerifiedTaskFunc(ctx, k8s, tekton, requester, taskrun, taskrun.Spec.TaskRef, taskrun.Name, taskrun.Namespace, taskrun.Spec.ServiceAccountName, verificationpolicies) @@ -85,19 +85,23 @@ func GetVerifiedTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton c owner kmeta.OwnerRefable, taskref *v1beta1.TaskRef, trName string, namespace, saName string, verificationpolicies []*v1alpha1.VerificationPolicy) GetTask { get := GetTaskFunc(ctx, k8s, tekton, requester, owner, taskref, trName, namespace, saName) - return func(context.Context, string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - t, s, err := get(ctx, taskref.Name) + return func(context.Context, string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + t, ct, s, err := get(ctx, taskref.Name) if err != nil { - return nil, nil, fmt.Errorf("failed to get task: %w", err) + return nil, nil, nil, fmt.Errorf("failed to get task: %w", err) + } + // TODO: check if we do not need to pass in ClusterTask for verification + if ct != nil { + return nil, ct, s, nil } var source string if s != nil { source = s.URI } if err := trustedresources.VerifyTask(ctx, t, k8s, source, verificationpolicies); err != nil { - return nil, nil, fmt.Errorf("GetVerifiedTaskFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) + return nil, nil, nil, fmt.Errorf("GetVerifiedTaskFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) } - return t, s, nil + return t, nil, s, nil } } @@ -117,14 +121,14 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset case cfg.FeatureFlags.EnableTektonOCIBundles && tr != nil && tr.Bundle != "": // Return an inline function that implements GetTask by calling Resolver.Get with the specified task type and // casting it to a TaskObject. - return func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { + return func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { // If there is a bundle url at all, construct an OCI resolver to fetch the task. kc, err := k8schain.New(ctx, k8s, k8schain.Options{ Namespace: namespace, ServiceAccountName: saName, }) if err != nil { - return nil, nil, fmt.Errorf("failed to get keychain: %w", err) + return nil, nil, nil, fmt.Errorf("failed to get keychain: %w", err) } resolver := oci.NewResolver(tr.Bundle, kc) @@ -133,7 +137,7 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset case tr != nil && tr.Resolver != "" && requester != nil: // Return an inline function that implements GetTask by calling Resolver.Get with the specified task type and // casting it to a TaskObject. - return func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { + return func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { var replacedParams []v1beta1.Param if ownerAsTR, ok := owner.(*v1beta1.TaskRun); ok { stringReplacements, arrayReplacements := paramsFromTaskRun(ctx, ownerAsTR) @@ -165,32 +169,34 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset // resolveTask accepts an impl of remote.Resolver and attempts to // fetch a task with given name. An error is returned if the // remoteresource doesn't work or the returned data isn't a valid -// v1beta1.TaskObject. -func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { +// v1beta1.Task or v1beta1.ClusterTask +func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { // Because the resolver will only return references with the same kind (eg ClusterTask), this will ensure we // don't accidentally return a Task with the same name but different kind. obj, configSource, err := resolver.Get(ctx, strings.TrimSuffix(strings.ToLower(string(kind)), "s"), name) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - taskObj, err := readRuntimeObjectAsTask(ctx, obj) + taskObj, ct, err := readRuntimeObjectAsTask(ctx, obj) if err != nil { - return nil, nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String()) + return nil, nil, nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String()) } - return taskObj, configSource, nil + return taskObj, ct, configSource, nil } // readRuntimeObjectAsTask tries to convert a generic runtime.Object -// into a v1beta1.TaskObject type so that its meta and spec fields -// can be read. v1 object will be converted to v1beta1 and returned. -// An error is returned if the given object is not a -// TaskObject or if there is an error validating or upgrading an +// into a v1beta1.Task or v1beta1.ClusterTask type so that its meta +// and spec fields can be read. v1 object will be converted to v1beta1 +// and returned. An error is returned if the given object is not a +// Task or ClusterTask or if there is an error validating or upgrading an // older TaskObject into its v1beta1 equivalent. // TODO(#5541): convert v1beta1 obj to v1 once we use v1 as the stored version -func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object) (v1beta1.TaskObject, error) { +func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object) (*v1beta1.Task, *v1beta1.ClusterTask, error) { switch obj := obj.(type) { - case v1beta1.TaskObject: - return obj, nil + case *v1beta1.Task: + return obj, nil, nil + case *v1beta1.ClusterTask: + return nil, obj, nil case *v1.Task: t := &v1beta1.Task{ TypeMeta: metav1.TypeMeta{ @@ -199,11 +205,11 @@ func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object) (v1beta1.T }, } if err := t.ConvertFrom(ctx, obj); err != nil { - return nil, err + return nil, nil, err } - return t, nil + return t, nil, nil } - return nil, errors.New("resource is not a task") + return nil, nil, errors.New("resource is not a task") } // LocalTaskRefResolver uses the current cluster to resolve a task reference. @@ -217,24 +223,24 @@ type LocalTaskRefResolver struct { // return an error if it can't find an appropriate Task for any reason. // TODO: if we want to set source for in-cluster task, set it here. // https://github.com/tektoncd/pipeline/issues/5522 -func (l *LocalTaskRefResolver) GetTask(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { +func (l *LocalTaskRefResolver) GetTask(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { if l.Kind == v1beta1.ClusterTaskKind { task, err := l.Tektonclient.TektonV1beta1().ClusterTasks().Get(ctx, name, metav1.GetOptions{}) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - return task, nil, nil + return nil, task, nil, nil } // If we are going to resolve this reference locally, we need a namespace scope. if l.Namespace == "" { - return nil, nil, fmt.Errorf("must specify namespace to resolve reference to task %s", name) + return nil, nil, nil, fmt.Errorf("must specify namespace to resolve reference to task %s", name) } task, err := l.Tektonclient.TektonV1beta1().Tasks(l.Namespace).Get(ctx, name, metav1.GetOptions{}) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - return task, nil, nil + return task, nil, nil, nil } // IsGetTaskErrTransient returns true if an error returned by GetTask is retryable. diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index a65191fc4d4..7fb620ff79d 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -215,21 +215,28 @@ func TestLocalTaskRef(t *testing.T) { Tektonclient: tektonclient, } - task, configSource, err := lc.GetTask(ctx, tc.ref.Name) + task, clusterTask, configSource, err := lc.GetTask(ctx, tc.ref.Name) if tc.wantErr && err == nil { t.Fatal("Expected error but found nil instead") } else if !tc.wantErr && err != nil { t.Fatalf("Received unexpected error ( %#v )", err) } - if d := cmp.Diff(task, tc.expected); tc.expected != nil && d != "" { - t.Error(diff.PrintWantGot(d)) - } - // local cluster tasks have empty source for now. This may be changed in future. if configSource != nil { t.Errorf("expected configsource is nil, but got %v", configSource) } + + if clusterTask != nil { + if d := cmp.Diff(clusterTask, tc.expected); tc.expected != nil && d != "" { + t.Error(diff.PrintWantGot(d)) + } + return + } + + if d := cmp.Diff(task, tc.expected); tc.expected != nil && d != "" { + t.Error(diff.PrintWantGot(d)) + } }) } } @@ -450,11 +457,18 @@ func TestGetTaskFunc(t *testing.T) { } fn := resources.GetTaskFunc(ctx, kubeclient, tektonclient, nil, trForFunc, tc.ref, "", "default", "default") - task, configSource, err := fn(ctx, tc.ref.Name) + task, clusterTask, configSource, err := fn(ctx, tc.ref.Name) if err != nil { t.Fatalf("failed to call taskfn: %s", err.Error()) } + if clusterTask != nil { + if diff := cmp.Diff(clusterTask, tc.expected); tc.expected != nil && diff != "" { + t.Error(diff) + } + return + } + if diff := cmp.Diff(task, tc.expected); tc.expected != nil && diff != "" { t.Error(diff) } @@ -517,7 +531,7 @@ echo hello fn := resources.GetTaskFuncFromTaskRun(ctx, kubeclient, tektonclient, nil, TaskRun, []*v1alpha1.VerificationPolicy{}) - actualTask, actualConfigSource, err := fn(ctx, name) + actualTask, _, actualConfigSource, err := fn(ctx, name) if err != nil { t.Fatalf("failed to call Taskfn: %s", err.Error()) } @@ -569,7 +583,7 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { } fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default") - resolvedTask, resolvedConfigSource, err := fn(ctx, taskRef.Name) + resolvedTask, _, resolvedConfigSource, err := fn(ctx, taskRef.Name) if err != nil { t.Fatalf("failed to call pipelinefn: %s", err.Error()) } @@ -635,7 +649,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { } fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default") - resolvedTask, resolvedConfigSource, err := fn(ctx, taskRef.Name) + resolvedTask, _, resolvedConfigSource, err := fn(ctx, taskRef.Name) if err != nil { t.Fatalf("failed to call pipelinefn: %s", err.Error()) } @@ -677,7 +691,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { } fnNotMatching := resources.GetTaskFunc(ctx, nil, nil, requester, trNotMatching, trNotMatching.Spec.TaskRef, "", "default", "default") - _, _, err = fnNotMatching(ctx, taskRefNotMatching.Name) + _, _, _, err = fnNotMatching(ctx, taskRefNotMatching.Name) if err == nil { t.Fatal("expected error for non-matching params, did not get one") } @@ -702,7 +716,7 @@ func TestGetPipelineFunc_RemoteResolutionInvalidData(t *testing.T) { }, } fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default") - if _, _, err := fn(ctx, taskRef.Name); err == nil { + if _, _, _, err := fn(ctx, taskRef.Name); err == nil { t.Fatalf("expected error due to invalid pipeline data but saw none") } } @@ -820,7 +834,7 @@ func TestGetVerifiedTaskFunc_Success(t *testing.T) { } fn := resources.GetVerifiedTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "default", "default", tc.policies) - resolvedTask, source, err := fn(ctx, taskRef.Name) + resolvedTask, _, source, err := fn(ctx, taskRef.Name) if err != nil { t.Fatalf("Received unexpected error ( %#v )", err) @@ -898,43 +912,43 @@ func TestGetVerifiedTaskFunc_VerifyError(t *testing.T) { name: "unsigned task with fails verification with fail no match policy", requester: requesterUnsigned, verificationNoMatchPolicy: config.FailNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "unsigned task with fails verification with warn no match policy", requester: requesterUnsigned, verificationNoMatchPolicy: config.WarnNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "unsigned task with fails verification with ignore no match policy", requester: requesterUnsigned, verificationNoMatchPolicy: config.IgnoreNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "modified task fails verification with fail no match policy", requester: requesterModified, verificationNoMatchPolicy: config.FailNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "modified task fails verification with warn no match policy", requester: requesterModified, verificationNoMatchPolicy: config.WarnNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "modified task fails verification with ignore no match policy", requester: requesterModified, verificationNoMatchPolicy: config.IgnoreNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, { name: "unmatched task fails with fail no match policy", requester: requesterUnmatched, verificationNoMatchPolicy: config.FailNoMatchPolicy, - expected: nil, + expected: (*v1beta1.Task)(nil), expectedErr: trustedresources.ErrResourceVerificationFailed, }, } @@ -950,19 +964,26 @@ func TestGetVerifiedTaskFunc_VerifyError(t *testing.T) { } fn := resources.GetVerifiedTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "default", "default", vps) - resolvedTask, source, err := fn(ctx, taskRef.Name) + resolvedTask, clusterTask, source, err := fn(ctx, taskRef.Name) if !errors.Is(err, tc.expectedErr) { t.Errorf("GetVerifiedTaskFunc got %v but want %v", err, tc.expectedErr) } - if d := cmp.Diff(tc.expected, resolvedTask); d != "" { - t.Errorf("resolvedTask did not match: %s", diff.PrintWantGot(d)) - } - if source != nil { t.Errorf("source is: %v but want is nil", source) } + + if clusterTask != nil { + if d := cmp.Diff(tc.expected, clusterTask); d != "" { + t.Errorf("resolved ClusterTask did not match: %s", diff.PrintWantGot(d)) + } + return + } + + if d := cmp.Diff(tc.expected, resolvedTask); d != "" { + t.Errorf("resolvedTask did not match: %s", diff.PrintWantGot(d)) + } }) } } @@ -1042,7 +1063,7 @@ func TestGetVerifiedTaskFunc_GetFuncError(t *testing.T) { fn := resources.GetVerifiedTaskFunc(ctx, k8sclient, tektonclient, tc.requester, &tc.taskrun, tc.taskrun.Spec.TaskRef, "", "default", "default", vps) - _, _, err = fn(ctx, tc.taskrun.Spec.TaskRef.Name) + _, _, _, err = fn(ctx, tc.taskrun.Spec.TaskRef.Name) if d := cmp.Diff(err.Error(), tc.expectedErr.Error()); d != "" { t.Fatalf("Expected error %v but found %v instead", tc.expectedErr, err) diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index 3b5653f9ff8..78977132354 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -35,7 +35,7 @@ type ResolvedTask struct { } // GetTask is a function used to retrieve Tasks. -type GetTask func(context.Context, string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) +type GetTask func(context.Context, string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) // GetTaskRun is a function used to retrieve TaskRuns type GetTaskRun func(string) (*v1beta1.TaskRun, error) @@ -50,25 +50,34 @@ func GetTaskData(ctx context.Context, taskRun *v1beta1.TaskRun, getTask GetTask) switch { case taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Name != "": // Get related task for taskrun - t, source, err := getTask(ctx, taskRun.Spec.TaskRef.Name) + t, ct, source, err := getTask(ctx, taskRun.Spec.TaskRef.Name) if err != nil { return nil, nil, fmt.Errorf("error when listing tasks for taskRun %s: %w", taskRun.Name, err) } - taskMeta = t.TaskMetadata() - taskSpec = t.TaskSpec() - configSource = source + if ct == nil { + taskMeta = t.TaskMetadata() + taskSpec = t.TaskSpec() + configSource = source + } else { + taskMeta = ct.TaskMetadata() + taskSpec = ct.TaskSpec() + configSource = source + } case taskRun.Spec.TaskSpec != nil: taskMeta = taskRun.ObjectMeta taskSpec = *taskRun.Spec.TaskSpec // TODO: if we want to set source for embedded taskspec, set it here. // https://github.com/tektoncd/pipeline/issues/5522 case taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Resolver != "": - task, source, err := getTask(ctx, taskRun.Name) + task, ct, source, err := getTask(ctx, taskRun.Name) switch { case err != nil: return nil, nil, err - case task == nil: + case task == nil && ct == nil: return nil, nil, errors.New("resolution of remote resource completed successfully but no task was returned") + case task == nil && ct != nil: + taskMeta = ct.TaskMetadata() + taskSpec = ct.TaskSpec() default: taskMeta = task.TaskMetadata() taskSpec = task.TaskSpec() diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index d038425acac..3c007217571 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -29,45 +29,67 @@ import ( ) func TestGetTaskSpec_Ref(t *testing.T) { - task := &v1beta1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "orchestrate", - }, - Spec: v1beta1.TaskSpec{ - Steps: []v1beta1.Step{{ - Name: "step1", - }}, - }, - } - tr := &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mytaskrun", + testcases := []struct { + name string + task *v1beta1.Task + clusterTask *v1beta1.ClusterTask + }{{ + name: "get taskspec with task", + task: &v1beta1.Task{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orchestrate", + }, + Spec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step1", + }}, + }, }, - Spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{ + }, { + name: "get taskspec with clustertask", + clusterTask: &v1beta1.ClusterTask{ + ObjectMeta: metav1.ObjectMeta{ Name: "orchestrate", }, + Spec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step1", + }}, + }, }, - } + }} - gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return task, sampleConfigSource.DeepCopy(), nil - } - resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt) + for _, tc := range testcases { + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + }, + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{ + Name: "orchestrate", + }, + }, + } - if err != nil { - t.Fatalf("Did not expect error getting task spec but got: %s", err) - } + gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return tc.task, tc.clusterTask, sampleConfigSource.DeepCopy(), nil + } + resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt) - if resolvedObjectMeta.Name != "orchestrate" { - t.Errorf("Expected task name to be `orchestrate` but was %q", resolvedObjectMeta.Name) - } + if err != nil { + t.Fatalf("Did not expect error getting task spec but got: %s", err) + } - if len(taskSpec.Steps) != 1 || taskSpec.Steps[0].Name != "step1" { - t.Errorf("Task Spec not resolved as expected, expected referenced Task spec but got: %v", taskSpec) - } - if d := cmp.Diff(sampleConfigSource, resolvedObjectMeta.ConfigSource); d != "" { - t.Errorf("configsource did not match: %s", diff.PrintWantGot(d)) + if resolvedObjectMeta.Name != "orchestrate" { + t.Errorf("Expected task name to be `orchestrate` but was %q", resolvedObjectMeta.Name) + } + + if len(taskSpec.Steps) != 1 || taskSpec.Steps[0].Name != "step1" { + t.Errorf("Task Spec not resolved as expected, expected referenced Task spec but got: %v", taskSpec) + } + if d := cmp.Diff(sampleConfigSource, resolvedObjectMeta.ConfigSource); d != "" { + t.Errorf("configsource did not match: %s", diff.PrintWantGot(d)) + } } } @@ -84,8 +106,8 @@ func TestGetTaskSpec_Embedded(t *testing.T) { }, }, } - gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, errors.New("shouldn't be called") + gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, errors.New("shouldn't be called") } resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt) @@ -113,8 +135,8 @@ func TestGetTaskSpec_Invalid(t *testing.T) { Name: "mytaskrun", }, } - gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, errors.New("shouldn't be called") + gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, errors.New("shouldn't be called") } _, _, err := resources.GetTaskData(context.Background(), tr, gt) if err == nil { @@ -133,8 +155,8 @@ func TestGetTaskSpec_Error(t *testing.T) { }, }, } - gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, errors.New("something went wrong") + gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, errors.New("something went wrong") } _, _, err := resources.GetTaskData(context.Background(), tr, gt) if err == nil { @@ -143,6 +165,35 @@ func TestGetTaskSpec_Error(t *testing.T) { } func TestGetTaskData_ResolutionSuccess(t *testing.T) { + sourceMeta := metav1.ObjectMeta{ + Name: "task", + } + sourceSpec := v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step1", + Image: "ubuntu", + Script: `echo "hello world!"`, + }}, + } + testcases := []struct { + getTask resources.GetTask + }{ + { + getTask: func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return &v1beta1.Task{ + ObjectMeta: *sourceMeta.DeepCopy(), + Spec: *sourceSpec.DeepCopy(), + }, nil, sampleConfigSource.DeepCopy(), nil + }, + }, { + getTask: func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, &v1beta1.ClusterTask{ + ObjectMeta: *sourceMeta.DeepCopy(), + Spec: *sourceSpec.DeepCopy(), + }, sampleConfigSource.DeepCopy(), nil + }}, + } + tr := &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "mytaskrun", @@ -162,38 +213,23 @@ func TestGetTaskData_ResolutionSuccess(t *testing.T) { }, }, } - sourceMeta := metav1.ObjectMeta{ - Name: "task", - } - sourceSpec := v1beta1.TaskSpec{ - Steps: []v1beta1.Step{{ - Name: "step1", - Image: "ubuntu", - Script: `echo "hello world!"`, - }}, - } - getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return &v1beta1.Task{ - ObjectMeta: *sourceMeta.DeepCopy(), - Spec: *sourceSpec.DeepCopy(), - }, sampleConfigSource.DeepCopy(), nil - } - ctx := context.Background() - resolvedMeta, resolvedSpec, err := resources.GetTaskData(ctx, tr, getTask) - if err != nil { - t.Fatalf("Unexpected error getting mocked data: %v", err) - } - if sourceMeta.Name != resolvedMeta.Name { - t.Errorf("Expected name %q but resolved to %q", sourceMeta.Name, resolvedMeta.Name) - } + for _, tc := range testcases { + resolvedMeta, resolvedSpec, err := resources.GetTaskData(context.Background(), tr, tc.getTask) + if err != nil { + t.Fatalf("Unexpected error getting mocked data: %v", err) + } + if sourceMeta.Name != resolvedMeta.Name { + t.Errorf("Expected name %q but resolved to %q", sourceMeta.Name, resolvedMeta.Name) + } - if d := cmp.Diff(sampleConfigSource, resolvedMeta.ConfigSource); d != "" { - t.Errorf("configsource did not match: %s", diff.PrintWantGot(d)) - } + if d := cmp.Diff(sampleConfigSource, resolvedMeta.ConfigSource); d != "" { + t.Errorf("configsource did not match: %s", diff.PrintWantGot(d)) + } - if d := cmp.Diff(sourceSpec, *resolvedSpec); d != "" { - t.Errorf(diff.PrintWantGot(d)) + if d := cmp.Diff(sourceSpec, *resolvedSpec); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } } } @@ -210,8 +246,8 @@ func TestGetPipelineData_ResolutionError(t *testing.T) { }, }, } - getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, errors.New("something went wrong") + getTask := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, errors.New("something went wrong") } ctx := context.Background() _, _, err := resources.GetTaskData(ctx, tr, getTask) @@ -233,8 +269,8 @@ func TestGetTaskData_ResolvedNilTask(t *testing.T) { }, }, } - getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) { - return nil, nil, nil + getTask := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) { + return nil, nil, nil, nil } ctx := context.Background() _, _, err := resources.GetTaskData(ctx, tr, getTask) diff --git a/pkg/trustedresources/verify.go b/pkg/trustedresources/verify.go index f64b29a9561..7c7ec3aadf2 100644 --- a/pkg/trustedresources/verify.go +++ b/pkg/trustedresources/verify.go @@ -46,8 +46,9 @@ const ( // Return an error when no policies are found and trusted-resources-verification-no-match-policy is set to fail, // or the resource fails to pass matched enforce verification policy // source is from ConfigSource.URI, which will be used to match policy patterns. k8s is used to fetch secret from cluster -func VerifyTask(ctx context.Context, taskObj v1beta1.TaskObject, k8s kubernetes.Interface, source string, verificationpolicies []*v1alpha1.VerificationPolicy) error { - matchedPolicies, err := getMatchedPolicies(taskObj.TaskMetadata().Name, source, verificationpolicies) +func VerifyTask(ctx context.Context, taskObj *v1beta1.Task, k8s kubernetes.Interface, source string, verificationpolicies []*v1alpha1.VerificationPolicy) error { + objectMeta := taskObj.TaskMetadata() + matchedPolicies, err := getMatchedPolicies(objectMeta.Name, source, verificationpolicies) if err != nil { if errors.Is(err, ErrNoMatchedPolicies) { switch config.GetVerificationNoMatchPolicy(ctx) { @@ -62,7 +63,7 @@ func VerifyTask(ctx context.Context, taskObj v1beta1.TaskObject, k8s kubernetes. return fmt.Errorf("failed to get matched policies: %w", err) } - tm, signature, err := prepareObjectMeta(taskObj.TaskMetadata()) + tm, signature, err := prepareObjectMeta(objectMeta) if err != nil { return err } diff --git a/pkg/trustedresources/verify_test.go b/pkg/trustedresources/verify_test.go index 70c2731aae1..93e00017f9e 100644 --- a/pkg/trustedresources/verify_test.go +++ b/pkg/trustedresources/verify_test.go @@ -199,7 +199,7 @@ func TestVerifyTask_Success(t *testing.T) { mismatchedSource := "wrong source" tcs := []struct { name string - task v1beta1.TaskObject + task *v1beta1.Task source string signer signature.SignerVerifier verificationNoMatchPolicy string @@ -276,7 +276,7 @@ func TestVerifyTask_Error(t *testing.T) { mismatchedSource := "wrong source" tcs := []struct { name string - task v1beta1.TaskObject + task *v1beta1.Task source string verificationPolicy []*v1alpha1.VerificationPolicy expectedError error