Add taskRef remote resolution support #4859

abayer · 2022-05-11T19:42:00Z

Changes

Followup to #4596, needed for #4710.

remote resolution, both in explicitly created TaskRuns and in PipelineRuns' PipelineTasks,
from public git repositories using tektoncd/resolution.

To actually see anything happening a dev also needs to deploy tektoncd/resolution and the gitresolver.

This is still in alpha.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

Allow taskRefs to be resolved directly from public git repos, using the tektoncd/resolution project.

abayer · 2022-05-11T19:42:14Z

/assign @vdemeester
/assign @sbwsg

tekton-robot · 2022-05-11T19:45:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	96.6%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.2%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.3%	91.9%	-0.3
pkg/reconciler/taskrun/controller.go	94.7%	95.0%	0.3
pkg/reconciler/taskrun/resources/taskref.go	75.6%	78.9%	3.4
pkg/reconciler/taskrun/taskrun.go	79.9%	80.1%	0.2

lbernick · 2022-05-11T19:49:40Z

Thanks for doing this @abayer! Just FYI that Scott is currently on leave until mid-August.

/assign

abayer · 2022-05-11T20:41:15Z

/retest

abayer · 2022-05-11T21:37:08Z

/retest

tekton-robot · 2022-05-12T08:25:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lbernick

I think it would make sense to start with one PR for adding remote resolution of tasks for TaskRuns, and then have a follow-up PR for adding remote resolution of Pipeline Tasks.

lbernick · 2022-05-12T13:46:35Z

pkg/apis/pipeline/v1beta1/pipeline_types_test.go

@@ -311,14 +311,14 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) {
 		},
 		expectedError: *apis.ErrDisallowedFields("taskref.bundle"),
 	}, {


Can you please add some test cases for use of resolver and resource with the alpha feature flag set?

Yup, will do.

lbernick · 2022-05-12T13:50:09Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -277,7 +277,19 @@ 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)
+		// Create a dummy TaskRun for remote resolution.


Can you help me understand what's going on here? the way I would anticipate this working is that we'd create a TaskRun as normal, but pass the resolver info along to the TaskRun, and have the TaskRun reconciler validate the resolver and resolve the task.

So this is what I'm most unsure about - we resolve the Task whether or not there's a TaskRun or even if we're creating a TaskRun, I believe for situations like when expressions and conditions. But the code in pkg/remote requires a kmeta.OwnerRefable. It only ever uses that kmeta.OwnerRefable to get its Name and Namespace, but nonetheless. Since we need to pass that kmeta.OwnerRefable in to GetTaskFunc, we don't have the TaskRun, if it exists, at the time we call GetTaskFunc.

So there's two aspects here - first, there's the kmeta.OwnerRefable stuff - that could be refactored to have the resolver code take name and namespace instead, and second, even if we did do that refactor, we'd still need to have the TaskRun name when calling GetTaskFunc, because

pipeline/pkg/remote/resolution/resolver.go

Line 86 in adc127a

func buildRequest(resolverName string, owner kmeta.OwnerRefable, params map[string]string) (*resolutionRequest, error) {

definitely wants the TaskRun name before resolving.

The options I see are the dummy TaskRun approach here, refactoring to take name and namespace instead, which still requires generating the TaskRun name at this location, or reworking the PipelineRun reconciler to not resolve the Task until there's already a created TaskRun, deferring all the remote resolution logic to the TaskRun reconciler. The last option definitely feels like the ideal one for this particular change, but I'm not sure what the implications would be if we don't resolve the Task until we have a TaskRun.

Thanks for the explanation!

I think the middle option (refactoring to take name and namespace instead) is the way to go. I find the dummy TaskRun approach to be confusing, and the last approach sounds like a large refactor that may not be necessary. If you look into buildRequest, it looks like the reason a name is needed is to make sure there are not multiple requests for the same pipeline's pipelineRef, the same task's taskRef, or the same pipeline task's taskRef. Therefore, what's needed here is not actually the name that will be used but some sort of unique identifier. I think a combo of pipelinerun name + pipeline task name should work.

Ok, I'll poke around briefly to try to better understand (for my own edification, if nothing else!) when exactly we end up with a resolved Task but no TaskRun and what would be impacted by not resolving the Task unless/until we resolve the TaskRun, but so far, it does look like a lot of behavior, particularly around resolving task resources, would barf in that case. Still good to know better. =)

Aaaaah, yeah, you're right re the name. That said, GetTaskRunName is just checking to see if there already is a name defined in pr.Status.ChildReferences or pr.Status.TaskRuns for the pipeline task name, and if not, it returns kmeta.ChildName(prName, fmt.Sprintf("-%s", ptName))...which is just prName-ptName unless that would be longer than 63 characters (in which case it trims down prName and/or ptName and adds an md5 hash of the "full" string to guarantee uniqueness) anyway. So might as well just use GetTaskRunName.

Aaah - I just checked deeper, and it turns out the kmeta.OwnerRefable ends up being used to add an owner reference to the generated ResolutionRequest:
https://github.com/tektoncd/resolution/blob/52bc1f17d4b6d8477e73a059de4b5808012a0386/pkg/resource/crd_resource.go#L104

So I think things need to be tweaked a bit more so that the ResolutionRequest has the PipelineRun as its owner reference.

And we should definitely use GetTaskRunName for the remote resource name in buildRequest so that we ensure the resolution request made by the PipelineRun reconciler has the same unique identifier as the one made by the TaskRun reconciler to avoid an unnecessary repeat resolution request being created...yeah, I'd really prefer if we didn't try to resolve the Task from the PipelineRun reconciler at all. Grr.

vdemeester · 2022-05-12T14:19:31Z

I think it would make sense to start with one PR for adding remote resolution of tasks for TaskRuns, and then have a follow-up PR for adding remote resolution of Pipeline Tasks.

The PR is not that huge, I don't really see why we would go through the trouble of spliting it into two.

lbernick · 2022-05-12T14:48:41Z

The PR is not that huge, I don't really see why we would go through the trouble of spliting it into two.

The reason I suggest splitting it is to logically separate resolution of taskrefs for taskruns vs pipeline tasks. There's some discussion around how best to handle resolution for pipeline tasks, and I think having taskref resolution in place for taskruns before adding it for pipeline tasks might lead to a better solution in the end. This is also just my opinion on a subjective judgment call about what is small/modular enough.

abayer · 2022-05-12T15:58:56Z

@lbernick @vdemeester @afrittoli 819b9df modifies the remote resolver to take both a kmeta.OwnerRefable and optional target name and namespace. If the target name and namespace aren't given, the owner's name and namespace are used, while the owner is still used for the resulting ResolutionRequest's owner reference regardless. This way, we don't have to create a dummy TaskRun, we still ensure that only one ResolutionRequest is created for a PipelineTask in both the relevant reconcilers, and the PipelineRun is used as the owner reference there when resolving the Task for a PipelineTask.

tekton-robot · 2022-05-12T15:59:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	96.6%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.2%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.3%	91.9%	-0.3
pkg/reconciler/taskrun/controller.go	94.7%	95.0%	0.3
pkg/reconciler/taskrun/resources/taskref.go	75.6%	78.6%	3.0
pkg/reconciler/taskrun/taskrun.go	79.9%	80.1%	0.2
pkg/remote/resolution/resolver.go	85.7%	86.2%	0.5

tekton-robot · 2022-05-12T16:12:34Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	96.6%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.2%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.3%	91.9%	-0.3
pkg/reconciler/taskrun/controller.go	94.7%	95.0%	0.3
pkg/reconciler/taskrun/resources/taskref.go	75.6%	78.6%	3.0
pkg/reconciler/taskrun/taskrun.go	79.9%	80.1%	0.2
pkg/remote/resolution/resolver.go	85.7%	86.2%	0.5

abayer · 2022-05-12T16:40:01Z

/retest

abayer · 2022-05-12T17:32:32Z

/retest

abayer · 2022-05-12T20:02:31Z

/retest

abayer · 2022-05-12T20:30:32Z

/retest

lbernick · 2022-05-13T12:13:21Z

pkg/reconciler/taskrun/taskrun_test.go

@@ -1238,6 +1238,114 @@ spec:
 	}
 }

+// TestReconcileWithResolver checks that a PipelineRun with a populated Resolver


docstring here I think needs to be updated

lbernick · 2022-05-13T12:19:09Z

pkg/reconciler/taskrun/resources/taskref.go

@@ -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.Namespace, taskrun.Spec.ServiceAccountName)


what's the reason to use "" here instead of taskrun.name?

In practice in this case, it doesn't really matter - if the targetName is "", then owner.Name (in this case, taskrun.name) is used. I'm not a huge fan of the signature for GetTaskFunc, but can't come up with something cleaner at the moment - I'll try to come back and refactor GetTaskFunc in general if and when I get a good idea. =) That said, I'll change this to use taskrun.Name for now, even though it's unnecessary.

lbernick · 2022-05-13T12:19:50Z

@lbernick @vdemeester @afrittoli 819b9df modifies the remote resolver to take both a kmeta.OwnerRefable and optional target name and namespace. If the target name and namespace aren't given, the owner's name and namespace are used, while the owner is still used for the resulting ResolutionRequest's owner reference regardless. This way, we don't have to create a dummy TaskRun, we still ensure that only one ResolutionRequest is created for a PipelineTask in both the relevant reconcilers, and the PipelineRun is used as the owner reference there when resolving the Task for a PipelineTask.

Thanks, I think this approach looks better!

tekton-robot · 2022-05-13T16:03:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	96.6%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.2%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.3%	91.9%	-0.3
pkg/reconciler/taskrun/controller.go	94.7%	95.0%	0.3
pkg/reconciler/taskrun/resources/taskref.go	75.6%	78.6%	3.0
pkg/reconciler/taskrun/taskrun.go	79.9%	80.1%	0.2
pkg/remote/resolution/resolver.go	85.7%	86.2%	0.5

lbernick · 2022-05-13T16:11:13Z

thanks @abayer! Just squash and I'll LGTM

Followup to tektoncd#4596, needed for tektoncd#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 <andrew.bayer@gmail.com>

abayer · 2022-05-13T16:27:39Z

@lbernick Done!

tekton-robot · 2022-05-13T16:30:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go	96.5%	96.6%	0.1
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.2%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.3%	91.9%	-0.3
pkg/reconciler/taskrun/controller.go	94.7%	95.0%	0.3
pkg/reconciler/taskrun/resources/taskref.go	75.6%	78.6%	3.0
pkg/reconciler/taskrun/taskrun.go	79.9%	80.1%	0.2
pkg/remote/resolution/resolver.go	85.7%	86.2%	0.5

lbernick · 2022-05-13T16:46:46Z

/lgtm

abayer · 2022-05-13T17:20:36Z

/retest

abayer · 2022-05-13T18:30:30Z

/retest

abayer · 2022-05-13T20:51:38Z

/retest

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels May 11, 2022

tekton-robot requested review from lbernick and pritidesai May 11, 2022 19:42

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 11, 2022

tekton-robot assigned ghost and vdemeester May 11, 2022

tekton-robot assigned lbernick May 11, 2022

vdemeester approved these changes May 12, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2022

lbernick reviewed May 12, 2022

View reviewed changes

lbernick reviewed May 13, 2022

View reviewed changes

abayer force-pushed the remote-resolving-tasks branch from d02a360 to 7235d9c Compare May 13, 2022 16:00

abayer force-pushed the remote-resolving-tasks branch from 7235d9c to f7d1e04 Compare May 13, 2022 16:27

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2022

tekton-robot merged commit 2cff700 into tektoncd:main May 13, 2022

Add taskRef remote resolution support #4859

Add taskRef remote resolution support #4859

Conversation

abayer commented May 11, 2022

Changes

Submitter Checklist

Release Notes

abayer commented May 11, 2022

tekton-robot commented May 11, 2022

lbernick commented May 11, 2022

abayer commented May 11, 2022

abayer commented May 11, 2022

tekton-robot commented May 12, 2022

lbernick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented May 12, 2022

lbernick commented May 12, 2022

abayer commented May 12, 2022

tekton-robot commented May 12, 2022

tekton-robot commented May 12, 2022

abayer commented May 12, 2022

abayer commented May 12, 2022

abayer commented May 12, 2022

abayer commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbernick commented May 13, 2022

tekton-robot commented May 13, 2022

lbernick commented May 13, 2022

abayer commented May 13, 2022

tekton-robot commented May 13, 2022

lbernick commented May 13, 2022

abayer commented May 13, 2022

abayer commented May 13, 2022

abayer commented May 13, 2022