From f7d1e040126a936aa77a0323f389a897201797d2 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Wed, 11 May 2022 15:39:30 -0400 Subject: [PATCH] Add taskRef remote resolution support Followup to #4596, needed for #4710. Enables remote resolution of `Task`s, both in explicitly created `TaskRun`s and in `PipelineRun`s' `PipelineTask`s, from public git repositories using tektoncd/resolution. Also modifies `resolution.Resolver` to take both owner and optional name/namespace. This is needed because we don't necessarily have a `TaskRun` yet when calling `GetTaskFunc` in the `PipelineRun` reconciler, but still need to ensure that we only make one remote resolution request for a `PipelineTask` via the `PipelineRun` and `TaskRun` reconcilers. Therefore, we must have the same deterministically-generated resolution request name in both places, using the predictable eventual `TaskRun` name and namespace. We also want to make sure that the created `ResolutionRequest` has the appropriate owner reference, hence still passing a `kmeta.OwnerRefable` to `NewResolver` as well as the name and namespace. This is still in alpha. Signed-off-by: Andrew Bayer --- docs/taskruns.md | 4 +- pkg/apis/pipeline/v1beta1/pipeline_types.go | 21 ++-- .../pipeline/v1beta1/pipeline_types_test.go | 44 +++++-- pkg/apis/pipeline/v1beta1/taskrun_types.go | 3 + pkg/reconciler/pipelinerun/pipelinerun.go | 16 ++- .../pipelinerun/pipelinerun_test.go | 78 ++++++++++++ .../pipelinerun/resources/pipelineref.go | 2 +- .../resources/pipelinerunresolution.go | 23 ++-- pkg/reconciler/taskrun/controller.go | 27 +++-- pkg/reconciler/taskrun/resources/taskref.go | 100 +++++++++++----- .../taskrun/resources/taskref_test.go | 86 +++++++++++++- pkg/reconciler/taskrun/resources/taskspec.go | 14 +++ .../taskrun/resources/taskspec_test.go | 107 +++++++++++++++++ pkg/reconciler/taskrun/taskrun.go | 39 +++--- pkg/reconciler/taskrun/taskrun_test.go | 112 +++++++++++++++++- pkg/remote/resolution/resolver.go | 32 +++-- pkg/remote/resolution/resolver_test.go | 59 +++++++-- 17 files changed, 653 insertions(+), 114 deletions(-) diff --git a/docs/taskruns.md b/docs/taskruns.md index c22f131b610..ba60b2995e2 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -167,8 +167,6 @@ cli *(coming soon)*. **([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#alpha-features))** -**Warning: This feature is still in very early stage of development and is not yet functional. Do not use it.** - A `taskRef` field may specify a Task in a remote location such as git. Support for specific types of remote will depend on the Resolvers your cluster's operator has installed. The below example demonstrates @@ -179,7 +177,7 @@ spec: taskRef: resolver: git resource: - - name: repo + - name: url value: https://github.com/tektoncd/catalog.git - name: commit value: abc123 diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go index 5e177b698f8..420fbef3a54 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types.go @@ -277,6 +277,7 @@ func (pt PipelineTask) validateBundle() (errs *apis.FieldError) { // validateTask validates a pipeline task or a final task for taskRef and taskSpec func (pt PipelineTask) validateTask(ctx context.Context) (errs *apis.FieldError) { + cfg := config.FromContextOrDefaults(ctx) // Validate TaskSpec if it's present if pt.TaskSpec != nil { errs = errs.Also(pt.TaskSpec.Validate(ctx).ViaField("taskSpec")) @@ -287,21 +288,21 @@ func (pt PipelineTask) validateTask(ctx context.Context) (errs *apis.FieldError) if errSlice := validation.IsQualifiedName(pt.TaskRef.Name); len(errSlice) != 0 { errs = errs.Also(apis.ErrInvalidValue(strings.Join(errSlice, ","), "name")) } - } else { + } else if pt.TaskRef.Resolver == "" { errs = errs.Also(apis.ErrInvalidValue("taskRef must specify name", "taskRef.name")) } // fail if bundle is present when EnableTektonOCIBundles feature flag is off (as it won't be allowed nor used) - if pt.TaskRef.Bundle != "" { + if !cfg.FeatureFlags.EnableTektonOCIBundles && pt.TaskRef.Bundle != "" { errs = errs.Also(apis.ErrDisallowedFields("taskref.bundle")) } - // fail if resolver or resource are present regardless - // of enabled api fields because remote resolution is - // not implemented yet for PipelineTasks. - if pt.TaskRef.Resolver != "" { - errs = errs.Also(apis.ErrDisallowedFields("taskref.resolver")) - } - if len(pt.TaskRef.Resource) > 0 { - errs = errs.Also(apis.ErrDisallowedFields("taskref.resource")) + if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { + // fail if resolver or resource are present when enable-api-fields is false. + if pt.TaskRef.Resolver != "" { + errs = errs.Also(apis.ErrDisallowedFields("taskref.resolver")) + } + if len(pt.TaskRef.Resource) > 0 { + errs = errs.Also(apis.ErrDisallowedFields("taskref.resource")) + } } } return errs diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index b245e49f804..711b1f75bb9 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -243,8 +243,10 @@ func TestPipelineTask_ValidateBundle_Failure(t *testing.T) { func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tests := []struct { - name string - tasks PipelineTask + name string + tasks PipelineTask + enableAPIFields bool + enableBundles bool }{{ name: "pipeline task - valid taskRef name", tasks: PipelineTask{ @@ -257,10 +259,40 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { Name: "foo", TaskSpec: &EmbeddedTask{TaskSpec: getTaskSpec()}, }, + }, { + name: "pipeline task - use of resolver with the feature flag set", + tasks: PipelineTask{ + TaskRef: &TaskRef{Name: "boo", ResolverRef: ResolverRef{Resolver: "bar"}}, + }, + enableAPIFields: true, + }, { + name: "pipeline task - use of resource with the feature flag set", + tasks: PipelineTask{ + TaskRef: &TaskRef{Name: "boo", ResolverRef: ResolverRef{Resource: []ResolverParam{{}}}}, + }, + enableAPIFields: true, + }, { + name: "pipeline task - use of bundle with the feature flag set", + tasks: PipelineTask{ + Name: "foo", + TaskRef: &TaskRef{Name: "bar", Bundle: "docker.io/foo"}, + }, + enableBundles: true, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.tasks.validateTask(context.Background()) + ctx := context.Background() + cfg := &config.Config{ + FeatureFlags: &config.FeatureFlags{}, + } + if tt.enableAPIFields { + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + } + if tt.enableBundles { + cfg.FeatureFlags.EnableTektonOCIBundles = true + } + ctx = config.ToContext(ctx, cfg) + err := tt.tasks.validateTask(ctx) if err != nil { t.Errorf("PipelineTask.validateTask() returned error for valid pipeline task: %v", err) } @@ -311,16 +343,14 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) { }, expectedError: *apis.ErrDisallowedFields("taskref.bundle"), }, { - name: "pipeline task - use of resolver", + name: "pipeline task - use of resolver without the feature flag set", task: PipelineTask{ - Name: "foo", TaskRef: &TaskRef{Name: "boo", ResolverRef: ResolverRef{Resolver: "bar"}}, }, expectedError: *apis.ErrDisallowedFields("taskref.resolver"), }, { - name: "pipeline task - use of resource", + name: "pipeline task - use of resource without the feature flag set", task: PipelineTask{ - Name: "foo", TaskRef: &TaskRef{Name: "boo", ResolverRef: ResolverRef{Resource: []ResolverParam{{}}}}, }, expectedError: *apis.ErrDisallowedFields("taskref.resource"), diff --git a/pkg/apis/pipeline/v1beta1/taskrun_types.go b/pkg/apis/pipeline/v1beta1/taskrun_types.go index 1b7937f46e8..c6cea9e7199 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_types.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_types.go @@ -140,6 +140,9 @@ const ( TaskRunReasonCancelled TaskRunReason = "TaskRunCancelled" // TaskRunReasonTimedOut is the reason set when the Taskrun has timed out TaskRunReasonTimedOut TaskRunReason = "TaskRunTimeout" + // TaskRunReasonResolvingTaskRef indicates that the TaskRun is waiting for + // its taskRef to be asynchronously resolved. + TaskRunReasonResolvingTaskRef = "ResolvingTaskRef" ) func (t TaskRunReason) String() string { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 62d4356c57f..0059a099647 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -277,7 +277,10 @@ func (c *Reconciler) resolvePipelineState( pst := resources.PipelineRunState{} // Resolve each task individually because they each could have a different reference context (remote or local). for _, task := range tasks { - fn, err := tresources.GetTaskFunc(ctx, c.KubeClientSet, c.PipelineClientSet, task.TaskRef, pr.Namespace, pr.Spec.ServiceAccountName) + // We need the TaskRun name to ensure that we don't perform an additional remote resolution request for a PipelineTask + // in the TaskRun reconciler. + trName := resources.GetTaskRunName(pr.Status.TaskRuns, pr.Status.ChildReferences, task.Name, pr.Name) + fn, err := tresources.GetTaskFunc(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, pr, task.TaskRef, trName, pr.Namespace, pr.Spec.ServiceAccountName) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it pr.Status.MarkFailed(ReasonCouldntGetTask, "Pipeline %s/%s can't be Run; task %s could not be fetched: %s", @@ -303,6 +306,9 @@ func (c *Reconciler) resolvePipelineState( if tresources.IsGetTaskErrTransient(err) { return nil, err } + if errors.Is(err, remote.ErrorRequestInProgress) { + return nil, err + } switch err := err.(type) { case *resources.TaskNotFoundError: pr.Status.MarkFailed(ReasonCouldntGetTask, @@ -467,8 +473,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get tasks = append(tasks, pipelineSpec.Finally...) } pipelineRunState, err := c.resolvePipelineState(ctx, tasks, pipelineMeta, pr, providedResources) - if err != nil { + switch { + case errors.Is(err, remote.ErrorRequestInProgress): + message := fmt.Sprintf("PipelineRun %s/%s awaiting remote resource", pr.Namespace, pr.Name) + pr.Status.MarkRunning(v1beta1.TaskRunReasonResolvingTaskRef, message) + return nil + case err != nil: return err + default: } // Build PipelineRunFacts with a list of resolved pipeline tasks, diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 1ce7f34c393..b8aaf16f67c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -7178,6 +7178,84 @@ spec: checkPipelineRunConditionStatusAndReason(t, updatedPipelineRun, corev1.ConditionUnknown, v1beta1.PipelineRunReasonRunning.String()) } +// TestReconcileWithTaskResolver checks that a PipelineRun with a populated Resolver +// field for a Task creates a ResolutionRequest object for that Resolver's type, and +// that when the request is successfully resolved the PipelineRun begins running. +func TestReconcileWithTaskResolver(t *testing.T) { + resolverName := "foobar" + pr := parse.MustParsePipelineRun(t, ` +metadata: + name: pr + namespace: default +spec: + pipelineSpec: + tasks: + - name: some-task + taskRef: + resolver: foobar + serviceAccountName: default +`) + + cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} + + d := test.Data{ + ConfigMaps: cms, + PipelineRuns: []*v1beta1.PipelineRun{pr}, + ServiceAccounts: []*corev1.ServiceAccount{{ + ObjectMeta: metav1.ObjectMeta{Name: pr.Spec.ServiceAccountName, Namespace: "foo"}, + }}, + } + + prt := newPipelineRunTest(d, t) + defer prt.Cancel() + + wantEvents := []string(nil) + pipelinerun, _ := prt.reconcileRun(pr.Namespace, pr.Name, wantEvents, false) + checkPipelineRunConditionStatusAndReason(t, pipelinerun, corev1.ConditionUnknown, v1beta1.TaskRunReasonResolvingTaskRef) + + client := prt.TestAssets.Clients.ResolutionRequests.ResolutionV1alpha1().ResolutionRequests("default") + resolutionrequests, err := client.List(prt.TestAssets.Ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("unexpected error listing resource requests: %v", err) + } + numResolutionRequests := len(resolutionrequests.Items) + if numResolutionRequests != 1 { + t.Fatalf("expected exactly 1 resource request but found %d", numResolutionRequests) + } + + resreq := &resolutionrequests.Items[0] + resolutionRequestType := resreq.ObjectMeta.Labels["resolution.tekton.dev/type"] + if resolutionRequestType != resolverName { + t.Fatalf("expected resource request type %q but saw %q", resolutionRequestType, resolverName) + } + + taskBytes := []byte(` +kind: Task +apiVersion: tekton.dev/v1beta1 +metadata: + name: foo +spec: + steps: + - name: step1 + image: ubuntu + script: | + echo "hello world!" +`) + + resreq.Status.ResolutionRequestStatusFields.Data = base64.StdEncoding.Strict().EncodeToString(taskBytes) + resreq.Status.MarkSucceeded() + resreq, err = client.UpdateStatus(prt.TestAssets.Ctx, resreq, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("unexpected error updating resource request with resolved pipeline data: %v", err) + } + + // Check that the resolved pipeline was recognized by the + // PipelineRun reconciler and that the PipelineRun has now + // started executing. + updatedPipelineRun, _ := prt.reconcileRun("default", "pr", nil, false) + checkPipelineRunConditionStatusAndReason(t, updatedPipelineRun, corev1.ConditionUnknown, v1beta1.PipelineRunReasonRunning.String()) +} + func getTaskRunWithTaskSpec(tr, pr, p, t string, labels, annotations map[string]string) *v1beta1.TaskRun { om := taskRunObjectMeta(tr, "foo", pr, p, t, false) for k, v := range labels { diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index 72fbcf9b817..9b70a8f05c0 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -76,7 +76,7 @@ func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clien for _, p := range pr.Resource { params[p.Name] = p.Value } - resolver := resolution.NewResolver(requester, pipelineRun, string(pr.Resolver), params) + resolver := resolution.NewResolver(requester, pipelineRun, string(pr.Resolver), "", "", params) return resolvePipeline(ctx, resolver, name) }, nil default: diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 8d165b53f71..617af19bb7b 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -18,19 +18,20 @@ package resources import ( "context" + "errors" "fmt" "strconv" - "knative.dev/pkg/kmeta" - "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/list" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" - "k8s.io/apimachinery/pkg/api/errors" + "github.com/tektoncd/pipeline/pkg/remote" + kerrors "k8s.io/apimachinery/pkg/api/errors" "knative.dev/pkg/apis" + "knative.dev/pkg/kmeta" ) const ( @@ -482,7 +483,7 @@ func ResolvePipelineRunTask( if rprt.IsCustomTask() { rprt.RunName = getRunName(pipelineRun.Status.Runs, pipelineRun.Status.ChildReferences, task.Name, pipelineRun.Name) run, err := getRun(rprt.RunName) - if err != nil && !errors.IsNotFound(err) { + if err != nil && !kerrors.IsNotFound(err) { return nil, fmt.Errorf("error retrieving Run %s: %w", rprt.RunName, err) } rprt.Run = run @@ -500,7 +501,7 @@ func ResolvePipelineRunTask( taskRun, err := getTaskRun(rprt.TaskRunName) if err != nil { - if !errors.IsNotFound(err) { + if !kerrors.IsNotFound(err) { return nil, fmt.Errorf("error retrieving TaskRun %s: %w", rprt.TaskRunName, err) } } @@ -515,14 +516,18 @@ func ResolvePipelineRunTask( taskName = task.TaskRef.Name } else { t, err = getTask(ctx, task.TaskRef.Name) - if err != nil { + switch { + case errors.Is(err, remote.ErrorRequestInProgress): + return nil, err + case err != nil: return nil, &TaskNotFoundError{ Name: task.TaskRef.Name, Msg: err.Error(), } + default: + spec = t.TaskSpec() + taskName = t.TaskMetadata().Name } - spec = t.TaskSpec() - taskName = t.TaskMetadata().Name } kind = task.TaskRef.Kind } else { @@ -623,7 +628,7 @@ func resolveConditionChecks(pt *v1beta1.PipelineTask, taskRunStatus map[string]* // TODO(#3133): Also handle Custom Task Runs (getRun here) cctr, err := getTaskRun(conditionCheckName) if err != nil { - if !errors.IsNotFound(err) { + if !kerrors.IsNotFound(err) { return nil, fmt.Errorf("error retrieving ConditionCheck %s for taskRun name %s : %w", conditionCheckName, taskRunName, err) } } diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go index 2e4e1006a01..f00ae3a8722 100644 --- a/pkg/reconciler/taskrun/controller.go +++ b/pkg/reconciler/taskrun/controller.go @@ -30,6 +30,9 @@ import ( cloudeventclient "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" "github.com/tektoncd/pipeline/pkg/taskrunmetrics" + resolutionclient "github.com/tektoncd/resolution/pkg/client/injection/client" + resolutioninformer "github.com/tektoncd/resolution/pkg/client/injection/informers/resolution/v1alpha1/resolutionrequest" + resolution "github.com/tektoncd/resolution/pkg/resource" "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/tools/cache" kubeclient "knative.dev/pkg/client/injection/kube/client" @@ -50,6 +53,7 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex podInformer := filteredpodinformer.Get(ctx, v1beta1.ManagedByLabelKey) resourceInformer := resourceinformer.Get(ctx) limitrangeInformer := limitrangeinformer.Get(ctx) + resolutionInformer := resolutioninformer.Get(ctx) configStore := config.NewStore(logger.Named("config-store"), taskrunmetrics.MetricsOnStore(logger)) configStore.WatchConfigs(cmw) @@ -59,17 +63,18 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock) func(contex } c := &Reconciler{ - KubeClientSet: kubeclientset, - PipelineClientSet: pipelineclientset, - Images: opts.Images, - Clock: clock, - taskRunLister: taskRunInformer.Lister(), - resourceLister: resourceInformer.Lister(), - limitrangeLister: limitrangeInformer.Lister(), - cloudEventClient: cloudeventclient.Get(ctx), - metrics: taskrunmetrics.Get(ctx), - entrypointCache: entrypointCache, - pvcHandler: volumeclaim.NewPVCHandler(kubeclientset, logger), + KubeClientSet: kubeclientset, + PipelineClientSet: pipelineclientset, + Images: opts.Images, + Clock: clock, + taskRunLister: taskRunInformer.Lister(), + resourceLister: resourceInformer.Lister(), + limitrangeLister: limitrangeInformer.Lister(), + cloudEventClient: cloudeventclient.Get(ctx), + metrics: taskrunmetrics.Get(ctx), + entrypointCache: entrypointCache, + pvcHandler: volumeclaim.NewPVCHandler(kubeclientset, logger), + resolutionRequester: resolution.NewCRDRequester(resolutionclient.Get(ctx), resolutionInformer.Lister()), } impl := taskrunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options { return controller.Options{ diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 614da79bb19..a6f8d6af987 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -18,6 +18,7 @@ package resources import ( "context" + "errors" "fmt" "strings" @@ -26,9 +27,14 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" + "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remote/oci" + "github.com/tektoncd/pipeline/pkg/remote/resolution" + remoteresource "github.com/tektoncd/resolution/pkg/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes" + "knative.dev/pkg/kmeta" ) // This error is defined in etcd at @@ -50,7 +56,7 @@ func GetTaskKind(taskrun *v1beta1.TaskRun) v1beta1.TaskKind { // also requires a kubeclient, tektonclient, namespace, and service account in case it needs to find that task in // cluster or authorize against an external repositroy. It will figure out whether it needs to look in the cluster or in // a remote image to fetch the reference. It will also return the "kind" of the task being referenced. -func GetTaskFuncFromTaskRun(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, taskrun *v1beta1.TaskRun) (GetTask, error) { +func GetTaskFuncFromTaskRun(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, taskrun *v1beta1.TaskRun) (GetTask, error) { // if the spec is already in the status, do not try to fetch it again, just use it as source of truth if taskrun.Status.TaskSpec != nil { return func(_ context.Context, name string) (v1beta1.TaskObject, error) { @@ -63,14 +69,15 @@ func GetTaskFuncFromTaskRun(ctx context.Context, k8s kubernetes.Interface, tekto }, nil }, nil } - return GetTaskFunc(ctx, k8s, tekton, taskrun.Spec.TaskRef, taskrun.Namespace, taskrun.Spec.ServiceAccountName) + return GetTaskFunc(ctx, k8s, tekton, requester, taskrun, taskrun.Spec.TaskRef, taskrun.Name, taskrun.Namespace, taskrun.Spec.ServiceAccountName) } // GetTaskFunc is a factory function that will use the given TaskRef as context to return a valid GetTask function. It // also requires a kubeclient, tektonclient, namespace, and service account in case it needs to find that task in // cluster or authorize against an external repositroy. It will figure out whether it needs to look in the cluster or in // a remote image to fetch the reference. It will also return the "kind" of the task being referenced. -func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, tr *v1beta1.TaskRef, namespace, saName string) (GetTask, error) { +func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, + owner kmeta.OwnerRefable, tr *v1beta1.TaskRef, trName string, namespace, saName string) (GetTask, error) { cfg := config.FromContextOrDefaults(ctx) kind := v1alpha1.NamespacedTaskKind if tr != nil && tr.Kind != "" { @@ -92,37 +99,21 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset } resolver := oci.NewResolver(tr.Bundle, kc) - // 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, err := resolver.Get(ctx, strings.TrimSuffix(strings.ToLower(string(kind)), "s"), name) - if err != nil { - return nil, err - } - - // If the resolved object is already a v1beta1.{Cluster}Task, it should be returnable as a - // v1beta1.TaskObject. - if ti, ok := obj.(v1beta1.TaskObject); ok { - ti.SetDefaults(ctx) - return ti, nil - } - - // If this object is not already a v1beta1 object, figure out what type it is actually and try to coerce it - // into a v1beta1.TaskInterface compatible object. - switch tt := obj.(type) { - case *v1alpha1.Task: - betaTask := &v1beta1.Task{} - err := tt.ConvertTo(ctx, betaTask) - betaTask.SetDefaults(ctx) - return betaTask, err - case *v1alpha1.ClusterTask: - betaTask := &v1beta1.ClusterTask{} - err := tt.ConvertTo(ctx, betaTask) - betaTask.SetDefaults(ctx) - return betaTask, err + return resolveTask(ctx, resolver, name, kind) + }, nil + case cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields && 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, error) { + params := map[string]string{} + for _, p := range tr.Resource { + params[p.Name] = p.Value } + resolver := resolution.NewResolver(requester, owner, string(tr.Resolver), trName, namespace, params) - return nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String()) + return resolveTask(ctx, resolver, name, kind) }, nil + default: // Even if there is no task ref, we should try to return a local resolver. local := &LocalTaskRefResolver{ @@ -134,6 +125,53 @@ 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) (v1beta1.TaskObject, 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, err := resolver.Get(ctx, strings.TrimSuffix(strings.ToLower(string(kind)), "s"), name) + if err != nil { + return nil, err + } + taskObj, err := readRuntimeObjectAsTask(ctx, obj) + if err != nil { + return nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String()) + } + return taskObj, nil +} + +// readRuntimeObjectAsTask tries to convert a generic runtime.Object +// into a v1beta1.TaskObject type so that its meta and spec fields +// can be read. 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. +func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object) (v1beta1.TaskObject, error) { + if task, ok := obj.(v1beta1.TaskObject); ok { + task.SetDefaults(ctx) + return task, nil + } + + // If this object is not already a v1beta1 object, figure out what type it is actually and try to coerce it + // into a v1beta1.TaskInterface compatible object. + switch tt := obj.(type) { + case *v1alpha1.Task: + betaTask := &v1beta1.Task{} + err := tt.ConvertTo(ctx, betaTask) + betaTask.SetDefaults(ctx) + return betaTask, err + case *v1alpha1.ClusterTask: + betaTask := &v1beta1.ClusterTask{} + err := tt.ConvertTo(ctx, betaTask) + betaTask.SetDefaults(ctx) + return betaTask, err + } + + return nil, errors.New("resource is not a task") +} + // LocalTaskRefResolver uses the current cluster to resolve a task reference. type LocalTaskRefResolver struct { Namespace string diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 861204243cb..124da526ebe 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -20,6 +20,7 @@ import ( "context" "net/http/httptest" "net/url" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -31,6 +32,7 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" + "github.com/tektoncd/pipeline/test/parse" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -416,7 +418,13 @@ func TestGetTaskFunc(t *testing.T) { t.Fatalf("failed to upload test image: %s", err.Error()) } - fn, err := resources.GetTaskFunc(ctx, kubeclient, tektonclient, tc.ref, "default", "default") + trForFunc := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{Name: "some-tr"}, + Spec: v1beta1.TaskRunSpec{ + TaskRef: tc.ref, + }, + } + fn, err := resources.GetTaskFunc(ctx, kubeclient, tektonclient, nil, trForFunc, tc.ref, "", "default", "default") if err != nil { t.Fatalf("failed to get task fn: %s", err.Error()) } @@ -477,7 +485,7 @@ echo hello Spec: TaskSpec, } - fn, err := resources.GetTaskFuncFromTaskRun(ctx, kubeclient, tektonclient, TaskRun) + fn, err := resources.GetTaskFuncFromTaskRun(ctx, kubeclient, tektonclient, nil, TaskRun) if err != nil { t.Fatalf("failed to get Task fn: %s", err.Error()) } @@ -490,3 +498,77 @@ echo hello t.Error(diff) } } + +func TestGetTaskFunc_RemoteResolution(t *testing.T) { + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + ctx = config.ToContext(ctx, cfg) + task := parse.MustParseTask(t, taskYAMLString) + taskRef := &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}} + taskYAML := strings.Join([]string{ + "kind: Task", + "apiVersion: tekton.dev/v1beta1", + taskYAMLString, + }, "\n") + resolved := test.NewResolvedResource([]byte(taskYAML), nil, nil) + requester := test.NewRequester(resolved, nil) + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default"}, + Spec: v1beta1.TaskRunSpec{ + TaskRef: taskRef, + ServiceAccountName: "default", + }, + } + fn, err := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default") + if err != nil { + t.Fatalf("failed to get task fn: %s", err.Error()) + } + + resolvedTask, err := fn(ctx, taskRef.Name) + if err != nil { + t.Fatalf("failed to call pipelinefn: %s", err.Error()) + } + + if d := cmp.Diff(task, resolvedTask); d != "" { + t.Error(d) + } +} + +func TestGetPipelineFunc_RemoteResolutionInvalidData(t *testing.T) { + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + ctx = config.ToContext(ctx, cfg) + taskRef := &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}} + resolvesTo := []byte("INVALID YAML") + resource := test.NewResolvedResource(resolvesTo, nil, nil) + requester := test.NewRequester(resource, nil) + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default"}, + Spec: v1beta1.TaskRunSpec{ + TaskRef: taskRef, + ServiceAccountName: "default", + }, + } + fn, err := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default") + if err != nil { + t.Fatalf("failed to get pipeline fn: %s", err.Error()) + } + if _, err := fn(ctx, taskRef.Name); err == nil { + t.Fatalf("expected error due to invalid pipeline data but saw none") + } +} + +// This is missing the kind and apiVersion because those are added by +// the MustParse helpers from the test package. +var taskYAMLString = ` +metadata: + name: foo +spec: + steps: + - name: step1 + image: ubuntu + script: | + echo "hello world!" +` diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index 4a4d6422f9f..85cb76bb422 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -18,8 +18,10 @@ package resources import ( "context" + "errors" "fmt" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -39,6 +41,7 @@ type GetClusterTask func(name string) (v1beta1.TaskObject, error) func GetTaskData(ctx context.Context, taskRun *v1beta1.TaskRun, getTask GetTask) (*metav1.ObjectMeta, *v1beta1.TaskSpec, error) { taskMeta := metav1.ObjectMeta{} taskSpec := v1beta1.TaskSpec{} + cfg := config.FromContextOrDefaults(ctx) switch { case taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Name != "": // Get related task for taskrun @@ -52,6 +55,17 @@ func GetTaskData(ctx context.Context, taskRun *v1beta1.TaskRun, getTask GetTask) case taskRun.Spec.TaskSpec != nil: taskMeta = taskRun.ObjectMeta taskSpec = *taskRun.Spec.TaskSpec + case cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields && taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Resolver != "": + task, err := getTask(ctx, taskRun.Name) + switch { + case err != nil: + return nil, nil, err + case task == nil: + return nil, nil, errors.New("resolution of remote resource completed successfully but no task was returned") + default: + taskMeta = task.TaskMetadata() + taskSpec = task.TaskSpec() + } default: return nil, nil, fmt.Errorf("taskRun %s not providing TaskRef or TaskSpec", taskRun.Name) } diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index cc9d1d1057a..6c64b6d8078 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -21,7 +21,10 @@ import ( "errors" "testing" + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/test/diff" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -127,3 +130,107 @@ func TestGetTaskSpec_Error(t *testing.T) { t.Fatalf("Expected error when unable to find referenced Task but got none") } } + +func TestGetTaskData_ResolutionSuccess(t *testing.T) { + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + }, + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "foo", + Resource: []v1beta1.ResolverParam{{ + Name: "bar", + Value: "baz", + }}, + }, + }, + }, + } + 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, error) { + return &v1beta1.Task{ + ObjectMeta: *sourceMeta.DeepCopy(), + Spec: *sourceSpec.DeepCopy(), + }, nil + } + // Enable alpha fields for remote resolution + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + ctx = config.ToContext(ctx, cfg) + resolvedMeta, resolvedSpec, err := 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) + } + if d := cmp.Diff(sourceSpec, *resolvedSpec); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } +} + +func TestGetPipelineData_ResolutionError(t *testing.T) { + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + }, + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "git", + }, + }, + }, + } + getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, error) { + return nil, errors.New("something went wrong") + } + // Enable alpha fields for remote resolution + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + ctx = config.ToContext(ctx, cfg) + _, _, err := GetTaskData(ctx, tr, getTask) + if err == nil { + t.Fatalf("Expected error when unable to find referenced Task but got none") + } +} + +func TestGetTaskData_ResolvedNilTask(t *testing.T) { + tr := &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + }, + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{ + ResolverRef: v1beta1.ResolverRef{ + Resolver: "git", + }, + }, + }, + } + getTask := func(ctx context.Context, n string) (v1beta1.TaskObject, error) { + return nil, nil + } + // Enable alpha fields for remote resolution + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + ctx = config.ToContext(ctx, cfg) + _, _, err := GetTaskData(ctx, tr, getTask) + if err == nil { + t.Fatalf("Expected error when unable to find referenced Task but got none") + } +} diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 119959f8ec5..00a5519c2c8 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -23,8 +23,6 @@ import ( "reflect" "strings" - "go.uber.org/zap" - "github.com/hashicorp/go-multierror" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" @@ -44,9 +42,12 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" + "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/taskrunmetrics" _ "github.com/tektoncd/pipeline/pkg/taskrunmetrics/fake" // Make sure the taskrunmetrics are setup "github.com/tektoncd/pipeline/pkg/workspace" + resolution "github.com/tektoncd/resolution/pkg/resource" + "go.uber.org/zap" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -70,13 +71,14 @@ type Reconciler struct { Clock clock.PassiveClock // listers index properties about resources - taskRunLister listers.TaskRunLister - resourceLister resourcelisters.PipelineResourceLister - limitrangeLister corev1Listers.LimitRangeLister - cloudEventClient cloudevent.CEClient - entrypointCache podconvert.EntrypointCache - metrics *taskrunmetrics.Recorder - pvcHandler volumeclaim.PvcHandler + taskRunLister listers.TaskRunLister + resourceLister resourcelisters.PipelineResourceLister + limitrangeLister corev1Listers.LimitRangeLister + cloudEventClient cloudevent.CEClient + entrypointCache podconvert.EntrypointCache + metrics *taskrunmetrics.Recorder + pvcHandler volumeclaim.PvcHandler + resolutionRequester resolution.Requester } // Check that our Reconciler implements taskrunreconciler.Interface @@ -282,7 +284,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 logger := logging.FromContext(ctx) tr.SetDefaults(ctx) - getTaskfunc, err := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, tr) + getTaskfunc, err := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, tr) if err != nil { logger.Errorf("Failed to fetch task reference %s: %v", tr.Spec.TaskRef.Name, err) tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) @@ -290,18 +292,23 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 } taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskfunc) - if err != nil { + switch { + case errors.Is(err, remote.ErrorRequestInProgress): + message := fmt.Sprintf("TaskRun %s/%s awaiting remote resource", tr.Namespace, tr.Name) + tr.Status.MarkResourceOngoing(v1beta1.TaskRunReasonResolvingTaskRef, message) + return nil, nil, err + case err != nil: logger.Errorf("Failed to determine Task spec to use for taskrun %s: %v", tr.Name, err) if resources.IsGetTaskErrTransient(err) { return nil, nil, err } tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) return nil, nil, controller.NewPermanentError(err) - } - - // Store the fetched TaskSpec on the TaskRun for auditing - if err := storeTaskSpecAndMergeMeta(tr, taskSpec, taskMeta); err != nil { - logger.Errorf("Failed to store TaskSpec on TaskRun.Statusfor taskrun %s: %v", tr.Name, err) + default: + // Store the fetched TaskSpec on the TaskRun for auditing + if err := storeTaskSpecAndMergeMeta(tr, taskSpec, taskMeta); err != nil { + logger.Errorf("Failed to store TaskSpec on TaskRun.Statusfor taskrun %s: %v", tr.Name, err) + } } inputs := []v1beta1.TaskResourceBinding{} diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 0138deb56b6..b6ed0f2add7 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -18,6 +18,7 @@ package taskrun import ( "context" + "encoding/base64" "errors" "fmt" "net/http/httptest" @@ -29,8 +30,6 @@ import ( "testing" "time" - "knative.dev/pkg/ptr" - "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/go-containerregistry/pkg/registry" @@ -66,6 +65,7 @@ import ( "knative.dev/pkg/controller" "knative.dev/pkg/kmeta" "knative.dev/pkg/logging" + "knative.dev/pkg/ptr" pkgreconciler "knative.dev/pkg/reconciler" "knative.dev/pkg/system" @@ -1238,6 +1238,114 @@ spec: } } +// TestReconcileWithResolver checks that a TaskRun with a populated Resolver +// field creates a ResolutionRequest object for that Resolver's type, and +// that when the request is successfully resolved the TaskRun begins running. +func TestReconcileWithResolver(t *testing.T) { + resolverName := "foobar" + tr := parse.MustParseTaskRun(t, ` +metadata: + name: tr + namespace: default +spec: + taskRef: + resolver: foobar + serviceAccountName: default +`) + + cms := []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{Namespace: system.Namespace(), Name: config.GetFeatureFlagsConfigName()}, + Data: map[string]string{ + "enable-api-fields": config.AlphaAPIFields, + }, + }} + + d := test.Data{ + ConfigMaps: cms, + TaskRuns: []*v1beta1.TaskRun{tr}, + ServiceAccounts: []*corev1.ServiceAccount{{ + ObjectMeta: metav1.ObjectMeta{Name: tr.Spec.ServiceAccountName, Namespace: "foo"}, + }}, + } + + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + saName := "default" + if _, err := clients.Kube.CoreV1().ServiceAccounts(tr.Namespace).Create(testAssets.Ctx, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: saName, + Namespace: tr.Namespace, + }, + }, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + if err := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tr)); err == nil { + t.Error("Wanted a resource request in progress error, but got nil.") + } else if controller.IsPermanentError(err) { + t.Errorf("expected no error. Got error %v", err) + } + + client := testAssets.Clients.ResolutionRequests.ResolutionV1alpha1().ResolutionRequests("default") + resolutionrequests, err := client.List(testAssets.Ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("unexpected error listing resource requests: %v", err) + } + numResolutionRequests := len(resolutionrequests.Items) + if numResolutionRequests != 1 { + t.Fatalf("expected exactly 1 resource request but found %d", numResolutionRequests) + } + + resreq := &resolutionrequests.Items[0] + resolutionRequestType := resreq.ObjectMeta.Labels["resolution.tekton.dev/type"] + if resolutionRequestType != resolverName { + t.Fatalf("expected resource request type %q but saw %q", resolutionRequestType, resolverName) + } + + // Mock a successful resolution + var taskBytes = []byte(` + kind: Task + apiVersion: tekton.dev/v1beta1 + metadata: + name: foo + spec: + steps: + - name: step1 + image: ubuntu + script: | + echo "hello world!" + `) + resreq.Status.ResolutionRequestStatusFields.Data = base64.StdEncoding.Strict().EncodeToString(taskBytes) + resreq.Status.MarkSucceeded() + resreq, err = client.UpdateStatus(testAssets.Ctx, resreq, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("unexpected error updating resource request with resolved task data: %v", err) + } + + // Check that the resolved task was recognized by the + // TaskRun reconciler and that the TaskRun has now + // started executing. + if err := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tr)); err != nil { + if ok, _ := controller.IsRequeueKey(err); !ok { + t.Errorf("expected no error. Got error %v", err) + } + } + + updatedTR, err := clients.Pipeline.TektonV1beta1().TaskRuns(tr.Namespace).Get(testAssets.Ctx, tr.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated taskrun: %v", err) + } + condition := updatedTR.Status.GetCondition(apis.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionUnknown { + t.Errorf("Expected fresh TaskRun to have in progress status, but had %v", condition) + } + if condition != nil && condition.Reason != v1beta1.TaskRunReasonRunning.String() { + t.Errorf("Expected reason %q but was %s", v1beta1.TaskRunReasonRunning.String(), condition.Reason) + } +} + func TestReconcile_SetsStartTime(t *testing.T) { taskRun := parse.MustParseTaskRun(t, ` metadata: diff --git a/pkg/remote/resolution/resolver.go b/pkg/remote/resolution/resolver.go index 5b3ae2289f4..d65528e2c0a 100644 --- a/pkg/remote/resolution/resolver.go +++ b/pkg/remote/resolution/resolver.go @@ -31,29 +31,33 @@ import ( // is used to make async requests for resources like pipelines from // remote places like git repos. type Resolver struct { - requester remoteresource.Requester - owner kmeta.OwnerRefable - resolverName string - params map[string]string + requester remoteresource.Requester + owner kmeta.OwnerRefable + resolverName string + params map[string]string + targetName string + targetNamespace string } var _ remote.Resolver = &Resolver{} // NewResolver returns an implementation of remote.Resolver capable // of performing asynchronous remote resolution. -func NewResolver(requester remoteresource.Requester, owner kmeta.OwnerRefable, resolverName string, params map[string]string) remote.Resolver { +func NewResolver(requester remoteresource.Requester, owner kmeta.OwnerRefable, resolverName string, targetName string, targetNamespace string, params map[string]string) remote.Resolver { return &Resolver{ - requester: requester, - owner: owner, - resolverName: resolverName, - params: params, + requester: requester, + owner: owner, + resolverName: resolverName, + params: params, + targetName: targetName, + targetNamespace: targetNamespace, } } // Get implements remote.Resolver. func (resolver *Resolver) Get(ctx context.Context, _, _ string) (runtime.Object, error) { resolverName := remoteresource.ResolverName(resolver.resolverName) - req, err := buildRequest(resolver.resolverName, resolver.owner, resolver.params) + req, err := buildRequest(resolver.resolverName, resolver.owner, resolver.targetName, resolver.targetNamespace, resolver.params) if err != nil { return nil, fmt.Errorf("error building request for remote resource: %w", err) } @@ -83,9 +87,11 @@ func (resolver *Resolver) List(_ context.Context) ([]remote.ResolvedObject, erro return nil, nil } -func buildRequest(resolverName string, owner kmeta.OwnerRefable, params map[string]string) (*resolutionRequest, error) { - name := owner.GetObjectMeta().GetName() - namespace := owner.GetObjectMeta().GetNamespace() +func buildRequest(resolverName string, owner kmeta.OwnerRefable, name string, namespace string, params map[string]string) (*resolutionRequest, error) { + if name == "" { + name = owner.GetObjectMeta().GetName() + namespace = owner.GetObjectMeta().GetNamespace() + } if namespace == "" { namespace = "default" } diff --git a/pkg/remote/resolution/resolver_test.go b/pkg/remote/resolution/resolver_test.go index 6822a79c250..aae66067d5b 100644 --- a/pkg/remote/resolution/resolver_test.go +++ b/pkg/remote/resolution/resolver_test.go @@ -11,20 +11,22 @@ See the License for the specific language governing permissions and limitations under the License. */ -package resolution_test +package resolution import ( "context" "errors" "testing" + "github.com/google/go-cmp/cmp" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/remote" - "github.com/tektoncd/pipeline/pkg/remote/resolution" "github.com/tektoncd/pipeline/test" + "github.com/tektoncd/pipeline/test/diff" resolutioncommon "github.com/tektoncd/resolution/pkg/common" remoteresource "github.com/tektoncd/resolution/pkg/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/kmeta" ) var pipelineBytes = []byte(` @@ -66,10 +68,11 @@ func TestGet_Successful(t *testing.T) { SubmitErr: nil, ResolvedResource: resolved, } - resolver := resolution.NewResolver(requester, owner, "git", nil) + resolver := NewResolver(requester, owner, "git", "", "", nil) if _, err := resolver.Get(ctx, "foo", "bar"); err != nil { t.Fatalf("unexpected error: %v", err) } + } } @@ -93,7 +96,7 @@ func TestGet_Errors(t *testing.T) { resolvedResource: nil, }, { submitErr: nil, - expectedGetErr: resolution.ErrorRequestedResourceIsNil, + expectedGetErr: ErrorRequestedResourceIsNil, resolvedResource: nil, }, { submitErr: genericError, @@ -101,11 +104,11 @@ func TestGet_Errors(t *testing.T) { resolvedResource: nil, }, { submitErr: nil, - expectedGetErr: &resolution.ErrorInvalidRuntimeObject{}, + expectedGetErr: &ErrorInvalidRuntimeObject{}, resolvedResource: notARuntimeObject, }, { submitErr: nil, - expectedGetErr: &resolution.ErrorAccessingData{}, + expectedGetErr: &ErrorAccessingData{}, resolvedResource: invalidDataResource, }} { ctx := context.Background() @@ -119,7 +122,7 @@ func TestGet_Errors(t *testing.T) { SubmitErr: tc.submitErr, ResolvedResource: tc.resolvedResource, } - resolver := resolution.NewResolver(requester, owner, "git", nil) + resolver := NewResolver(requester, owner, "git", "", "", nil) obj, err := resolver.Get(ctx, "foo", "bar") if obj != nil { t.Errorf("received unexpected resolved resource") @@ -129,3 +132,45 @@ func TestGet_Errors(t *testing.T) { } } } + +func TestBuildRequest(t *testing.T) { + for _, tc := range []struct { + name string + targetName string + targetNamespace string + }{{ + name: "just owner", + }, { + name: "with target name and namespace", + targetName: "some-object", + targetNamespace: "some-ns", + }} { + t.Run(tc.name, func(t *testing.T) { + owner := &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + } + + req, err := buildRequest("git", owner, tc.targetName, tc.targetNamespace, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if d := cmp.Diff(*kmeta.NewControllerRef(owner), req.OwnerRef()); d != "" { + t.Errorf("expected matching owner ref but got %s", diff.PrintWantGot(d)) + } + reqNameBase := owner.Namespace + "/" + owner.Name + if tc.targetName != "" { + reqNameBase = tc.targetNamespace + "/" + tc.targetName + } + expectedReqName, err := remoteresource.GenerateDeterministicName("git", reqNameBase, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if expectedReqName != req.Name() { + t.Errorf("expected request name %s, but was %s", expectedReqName, req.Name()) + } + }) + } +}