Conceptual bug: Wrong resource names used

[dlorenc]

This is a conceptual bug in the way resource names are handled in Tekton today. I'm working on a fix, but wanted to discuss the actual issue here first.

PipelineResources right now can be referred to by two different names:

1. The PipelineResourceSpec.Params.Name field. This is not a top-level field in PipelineResources, but (almost?) all of the existing PipelineResource types take a parameter called "Name" that is then set on the object itself after unmarshalling.
   This name can be thought of as a way to refer to the external object the PipelineResource refers to.
2. The TaskRun.Inputs|Outputs.Resources.Name field. This is a field on the ResourceBinding. This can be thought of as an internal "name" the TaskRun will use to refer to the resource.

Right now, we pass the external 'Name' field to the resource containers via the 'TEKTON_RESOURCE_NAME' env var. This is only used in a few locations right now:

• substitutions
• results ( to indicate which resource the result corresponds to )

We should be using the 'internal' name field here - the external names are not always set and can change/not be unique. Internal names are unique within a Task/TaskRun.

[afrittoli]

I'm trying to understand but I'm not sure I understood the problem fully.
So here is my understanding of what the problem is.

The struct that holds all PipelineResource types has a field called "Name". That field is not a PipelineResource parameter. For instance for the git resource, the struct includes name:

pipeline/pkg/apis/resource/v1alpha1/git/git_resource.go

Lines 37 to 53 in 4d62948

	type Resource struct {
	Name string `json:"name"`
	Type resource.PipelineResourceType `json:"type"`
	URL string `json:"url"`
	// Git revision (branch, tag, commit SHA) to clone, and optionally the refspec to fetch from.
	//See https://git-scm.com/docs/gitrevisions#_specifying_revisions for more information.
	Revision string `json:"revision"`
	Refspec string `json:"refspec"`
	Submodules bool `json:"submodules"`
	Depth uint `json:"depth"`
	SSLVerify bool `json:"sslVerify"`
	HTTPProxy string `json:"httpProxy"`
	HTTPSProxy string `json:"httpsProxy"`
	NOProxy string `json:"noProxy"`
	GitImage string `json:"-"`
	}

Name is not parsed from parameters:

pipeline/pkg/apis/resource/v1alpha1/git/git_resource.go

Lines 68 to 87 in 4d62948

	for _, param := range r.Spec.Params {
	switch {
	case strings.EqualFold(param.Name, "URL"):
	gitResource.URL = param.Value
	case strings.EqualFold(param.Name, "Revision"):
	gitResource.Revision = param.Value
	case strings.EqualFold(param.Name, "Refspec"):
	gitResource.Refspec = param.Value
	case strings.EqualFold(param.Name, "Submodules"):
	gitResource.Submodules = toBool(param.Value, true)
	case strings.EqualFold(param.Name, "Depth"):
	gitResource.Depth = toUint(param.Value, 1)
	case strings.EqualFold(param.Name, "SSLVerify"):
	gitResource.SSLVerify = toBool(param.Value, true)
	case strings.EqualFold(param.Name, "HTTPProxy"):
	gitResource.HTTPProxy = param.Value
	case strings.EqualFold(param.Name, "HTTPSProxy"):
	gitResource.HTTPSProxy = param.Value
	case strings.EqualFold(param.Name, "NOProxy"):
	gitResource.NOProxy = param.Value

Name is taken instead from the PipelineResource CRD "Name" field. I think this is what you refer to as the "external" name.

A Task defines its own resource names, and a TaskRun binds the "external" name of a resource to the task resource. The code in the TaskRun controller should use the bound resource name only for fetching the resource spec, and the task resource name otherwise.

The git pipeline resource uses the TEKTON_RESOURCE_NAME to report results.
The image resource a CLI flag instead to report results.

In both cases, a v1beta1.PipelineResourceResult is created, which holds a v1beta1.PipelineResourceRef. The resource ref points to the resource name and kind.
Finally, the PipelineResourceRef is added to the list of results in the TaskRun.Status:

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 618 to 619 in 232dea7

	taskRun.Status.ResourcesResult = append(taskRun.Status.ResourcesResult, pipelineResourceResults...)
	}

I think your point is that the results in the TaskRun status should refer to the task resource and not to the bound resource, because that is not necessarily unique.

It makes sense to me. I think fixing that would imply changing the v1beta1.PipelineResourceResult to store a task resource name instead of a Ref to a PipelineResource. The pipeline resource would need to have knowledge of the task resource name, so that it could pass it down to the container and ultimately write it in the termination message for the TaskRun controller to pick up.

[afrittoli]

/kind bug

[dlorenc]

I think your point is that the results in the TaskRun status should refer to the task resource and not to the bound resource, because that is not necessarily unique.

It makes sense to me. I think fixing that would imply changing the v1beta1.PipelineResourceResult to store a task resource name instead of a Ref to a PipelineResource. The pipeline resource would need to have knowledge of the task resource name, so that it could pass it down to the container and ultimately write it in the termination message for the TaskRun controller to pick up.

Yes, that's exactly correct, and a much more succinct way of putting it. I left this as a Ref, but since it's no longer a Ref you're probably right that we should change this to just be a Name.

[dlorenc]

I'll also add that it's not only because they are not necessarily unique - the resources referred to by the resourceRefs may not actually exist at all! They could have been defined inline in the TaskRun/PipelineRun. Our handling here gets a little funky...

[dlorenc]

I just added a second commit that stores the name in a ResourcesResult.ResourceName field, in addition to the resourceRef.

This is an API addition, although it really should be a break since we should delete the resourceRef (it doesn't make semantic sense).

Any thoughts on how to stage this? My suggestion is to add the new field, then remove the old one the next time we change the API version.