Skip to content

Commit

Permalink
Replace v1beta1.TaskObject with *v1beta1.Task in TaskRun Reconciler
Browse files Browse the repository at this point in the history 
This commit replaces the v1beta1.TaskObject interface with the *v1beta1.Task by converting ClusterTask to Task for resolving Tasks for TaskRun. The deprecated v1beta1 ClusterTasks are converted to Tasks since the GetTask func and its upstream callers only fetch a task spec and store it in the taskrun status while the kind info is not being used.
  • Loading branch information
@JeromeJu @tekton-robot
JeromeJu authored and tekton-robot committed Apr 3, 2023
1 parent 56cb562 commit 8cb2529
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 41 deletions.
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ func resolveTask(
pipelineTask v1beta1.PipelineTask,
) (v1beta1.TaskSpec, string, v1beta1.TaskKind, error) {
var (
t v1beta1.TaskObject
t *v1beta1.Task
err error
spec v1beta1.TaskSpec
taskName string
Expand Down
18 changes: 9 additions & 9 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ 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) {
func nopGetTask(context.Context, string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, errors.New("GetTask should not be called")
}
func nopGetTaskRun(string) (*v1beta1.TaskRun, error) {
Expand Down Expand Up @@ -1942,7 +1942,7 @@ func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) {
TaskRef: &v1beta1.TaskRef{Name: "task"},
}}

getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil }
Expand Down Expand Up @@ -1992,7 +1992,7 @@ 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) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, kerrors.NewNotFound(v1beta1.Resource("task"), name)
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) {
Expand Down Expand Up @@ -2034,7 +2034,7 @@ func TestResolvePipelineRun_VerificationFailed(t *testing.T) {
}},
}}}

getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, trustedresources.ErrResourceVerificationFailed
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil }
Expand Down Expand Up @@ -2271,7 +2271,7 @@ func TestResolvePipeline_WhenExpressions(t *testing.T) {
WhenExpressions: []v1beta1.WhenExpression{ptwe1},
}

getTask := func(_ context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(_ context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
pr := v1beta1.PipelineRun{
Expand Down Expand Up @@ -2304,7 +2304,7 @@ func TestIsCustomTask(t *testing.T) {
Name: "pipelinerun",
},
}
getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil }
Expand Down Expand Up @@ -3071,7 +3071,7 @@ func TestIsMatrixed(t *testing.T) {
Name: "pipelinerun",
},
}
getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil }
Expand Down Expand Up @@ -3205,7 +3205,7 @@ func TestResolvePipelineRunTask_WithMatrix(t *testing.T) {
}}},
}

getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return taskRunsMap[name], nil }
Expand Down Expand Up @@ -3309,7 +3309,7 @@ func TestResolvePipelineRunTask_WithMatrixedCustomTask(t *testing.T) {
}}},
}}

getTask := func(ctx context.Context, name string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return &trs[0], nil }
Expand Down
48 changes: 34 additions & 14 deletions pkg/reconciler/taskrun/resources/taskref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.ConfigSource, error) {
var configsource *v1beta1.ConfigSource
if taskrun.Status.Provenance != nil {
configsource = taskrun.Status.Provenance.ConfigSource
Expand All @@ -85,7 +85,7 @@ 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) {
return func(context.Context, string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
t, s, err := get(ctx, taskref.Name)
if err != nil {
return nil, nil, fmt.Errorf("failed to get task: %w", err)
Expand Down Expand Up @@ -117,7 +117,7 @@ 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.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,
Expand All @@ -133,7 +133,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.ConfigSource, error) {
var replacedParams v1beta1.Params
if ownerAsTR, ok := owner.(*v1beta1.TaskRun); ok {
stringReplacements, arrayReplacements := paramsFromTaskRun(ctx, ownerAsTR)
Expand Down Expand Up @@ -165,8 +165,8 @@ 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.
func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (*v1beta1.Task, *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)
Expand All @@ -181,16 +181,18 @@ func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kin
}

// readRuntimeObjectAsTask tries to convert a generic runtime.Object
// into a v1beta1.TaskObject type so that its meta and spec fields
// into a *v1beta1.Task 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
// older TaskObject into its v1beta1 equivalent.
// An error is returned if the given object is not a Task nor a 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, error) {
switch obj := obj.(type) {
case v1beta1.TaskObject:
case *v1beta1.Task:
return obj, nil
case *v1beta1.ClusterTask:
return convertClusterTaskToTask(*obj), nil
case *v1.Task:
t := &v1beta1.Task{
TypeMeta: metav1.TypeMeta{
Expand All @@ -217,13 +219,13 @@ 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.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 task, nil, nil
return convertClusterTaskToTask(*task), nil, nil
}

// If we are going to resolve this reference locally, we need a namespace scope.
Expand All @@ -241,3 +243,21 @@ func (l *LocalTaskRefResolver) GetTask(ctx context.Context, name string) (v1beta
func IsGetTaskErrTransient(err error) bool {
return strings.Contains(err.Error(), errEtcdLeaderChange)
}

// convertClusterTaskToTask converts deprecated v1beta1 ClusterTasks to Tasks for
// the rest of reconciling process since GetTask func and its upstream callers only
// fetches the task spec and stores it in the taskrun status while the kind info
// is not being used.
func convertClusterTaskToTask(ct v1beta1.ClusterTask) *v1beta1.Task {
t := &v1beta1.Task{
TypeMeta: metav1.TypeMeta{
Kind: "Task",
APIVersion: "tekton.dev/v1beta1",
},
}

t.Spec = ct.Spec
t.ObjectMeta.Name = ct.ObjectMeta.Name

return t
}
29 changes: 23 additions & 6 deletions pkg/reconciler/taskrun/resources/taskref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,11 @@ func TestLocalTaskRef(t *testing.T) {
Name: "cluster-task",
Kind: "ClusterTask",
},
expected: &v1beta1.ClusterTask{
expected: &v1beta1.Task{
TypeMeta: metav1.TypeMeta{
APIVersion: "tekton.dev/v1beta1",
Kind: "Task",
},
ObjectMeta: metav1.ObjectMeta{
Name: "cluster-task",
},
Expand Down Expand Up @@ -397,11 +401,11 @@ func TestGetTaskFunc(t *testing.T) {
Kind: v1beta1.ClusterTaskKind,
Bundle: u.Host + "/remote-cluster-task",
},
expected: &v1beta1.ClusterTask{
expected: &v1beta1.Task{
ObjectMeta: metav1.ObjectMeta{Name: "simple"},
TypeMeta: metav1.TypeMeta{
APIVersion: "tekton.dev/v1beta1",
Kind: "ClusterTask",
Kind: "Task",
},
},
expectedKind: v1beta1.ClusterTaskKind,
Expand All @@ -422,8 +426,21 @@ func TestGetTaskFunc(t *testing.T) {
Name: "simple",
Kind: v1beta1.ClusterTaskKind,
},
expected: simpleClusterTask,
expectedKind: v1beta1.ClusterTaskKind,
expected: &v1beta1.Task{
ObjectMeta: metav1.ObjectMeta{
Name: "simple",
},
TypeMeta: metav1.TypeMeta{
APIVersion: "tekton.dev/v1beta1",
Kind: "Task",
},
Spec: v1beta1.TaskSpec{
Steps: []v1beta1.Step{{
Image: "something",
}},
},
},
expectedKind: v1beta1.NamespacedTaskKind,
},
}

Expand Down Expand Up @@ -892,7 +909,7 @@ func TestGetVerifiedTaskFunc_VerifyError(t *testing.T) {
name string
requester *test.Requester
verificationNoMatchPolicy string
expected runtime.Object
expected *v1beta1.Task
expectedErr error
}{{
name: "unsigned task with fails verification with fail no match policy",
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/resources/taskspec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.ConfigSource, error)

// GetTaskRun is a function used to retrieve TaskRuns
type GetTaskRun func(string) (*v1beta1.TaskRun, error)
Expand Down
14 changes: 7 additions & 7 deletions pkg/reconciler/taskrun/resources/taskspec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestGetTaskSpec_Ref(t *testing.T) {
},
}

gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return task, sampleConfigSource.DeepCopy(), nil
}
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt)
Expand Down Expand Up @@ -84,7 +84,7 @@ func TestGetTaskSpec_Embedded(t *testing.T) {
},
},
}
gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, errors.New("shouldn't be called")
}
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt)
Expand Down Expand Up @@ -113,7 +113,7 @@ func TestGetTaskSpec_Invalid(t *testing.T) {
Name: "mytaskrun",
},
}
gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, errors.New("shouldn't be called")
}
_, _, err := resources.GetTaskData(context.Background(), tr, gt)
Expand All @@ -133,7 +133,7 @@ func TestGetTaskSpec_Error(t *testing.T) {
},
},
}
gt := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
gt := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, errors.New("something went wrong")
}
_, _, err := resources.GetTaskData(context.Background(), tr, gt)
Expand Down Expand Up @@ -173,7 +173,7 @@ func TestGetTaskData_ResolutionSuccess(t *testing.T) {
}},
}

getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return &v1beta1.Task{
ObjectMeta: *sourceMeta.DeepCopy(),
Spec: *sourceSpec.DeepCopy(),
Expand Down Expand Up @@ -210,7 +210,7 @@ func TestGetPipelineData_ResolutionError(t *testing.T) {
},
},
}
getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, errors.New("something went wrong")
}
ctx := context.Background()
Expand All @@ -233,7 +233,7 @@ func TestGetTaskData_ResolvedNilTask(t *testing.T) {
},
},
}
getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
getTask := func(ctx context.Context, n string) (*v1beta1.Task, *v1beta1.ConfigSource, error) {
return nil, nil, nil
}
ctx := context.Background()
Expand Down
2 changes: 1 addition & 1 deletion pkg/trustedresources/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ 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 {
func VerifyTask(ctx context.Context, taskObj *v1beta1.Task, k8s kubernetes.Interface, source string, verificationpolicies []*v1alpha1.VerificationPolicy) error {
matchedPolicies, err := getMatchedPolicies(taskObj.TaskMetadata().Name, source, verificationpolicies)
if err != nil {
if errors.Is(err, ErrNoMatchedPolicies) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/trustedresources/verify_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8cb2529

Please sign in to comment.