PipelineRun Controller Validation - Output Binding Issue #213

aaron-prindle · 2018-11-01T18:07:08Z

Expected Behavior

Currently there is a bug in the PipelineRun Controller Validation as noted in #192 (comment). This allows for Outputs to not have proper Resource bindings. A test for correctly working correct-output-source-bindings (in addition to the bad-output-source-bindings test we currently have) should catch this issue as currently nothing is added to outMapping here:
https://github.com/knative/build-pipeline/blob/master/pkg/reconciler/v1alpha1/pipelinerun/validate.go#L99

Actual Behavior

Outputs mappings should be checked in a PipelineRun. Currently, as nothing is added to the outMapping, it is not possible to have a verified PipelineRun that has Resource mapping

Steps to Reproduce the Problem

Create a Resource
Create a Task requiring an Output with a ResourceRef
Create a TaskRun referencing the created Resource

aaron-prindle · 2018-11-01T18:07:47Z

I can refactor the PipelineRun/TaskRun validation to share code which should fix this, putting this here as a reminder.

aaron-prindle · 2018-11-01T19:57:20Z

Also should address:
#208 (review)

bobcatfish · 2018-11-20T18:48:33Z

I'm touching this code anyway and I can't help myself so I'll take this on

This change makes it possible to reuse a Pipeline with different Resources without having to change the Pipeline itself. It does this by moving the linking of the Resources a Task requires from the Pipeline into the PipelineRun. The relationship between Resources and the Tasks that are expected to be executed on them is still expressed in the Pipeline (via `providedBy`). ResourceBindings move from Pipeline to PipelineRun and become ResourceDependencies. No longer calling these "bindings" in the API or using the term "source" fixes + the additional docs in using.md fixes tektoncd#139. The most difficult part of this change was updating validatePipelineTaskAndTask - hoping to refactor this in a follow up (which would also address tektoncd#213). This interface is still probably not in its final form and hopefully we can iterate on it! Fixes tektoncd#200

bobcatfish · 2018-11-22T18:04:12Z

Outputs mappings should be checked in a PipelineRun. Currently, as nothing is added to the outMapping, it is not possible to have a verified PipelineRun that has Resource mapping

After looking at the code more closely, it turns out that the code will make sure that there is a resource specified for the outputs, the missing bit is that the type won't be checked.

This change makes it possible to reuse a Pipeline with different Resources without having to change the Pipeline itself. It does this by moving the linking of the Resources a Task requires from the Pipeline into the PipelineRun. The relationship between Resources and the Tasks that are expected to be executed on them is still expressed in the Pipeline (via `providedBy`). ResourceBindings move from Pipeline to PipelineRun and become ResourceDependencies. No longer calling these "bindings" in the API or using the term "source" fixes + the additional docs in using.md fixes #139. The most difficult part of this change was updating validatePipelineTaskAndTask - hoping to refactor this in a follow up (which would also address #213). This interface is still probably not in its final form and hopefully we can iterate on it! Fixes #200

In the TaskRun reconciler, there are several different places where logic exists to retrieve references Tasks and Resources; this library will allow us to retrieve these references up front and use them instead of using Listers everywhere. This is a step toward fixing tektoncd#213 (the goal is to reuse and simplify this logic).

With this change, we can use the resolved TaskRun when validating the TaskRun, so we no longer need access to the reconciler or to do any listing when validating the TaskRun. This is a step along the way to tektoncd#213. Next we can do a similar refactoring to the PipelineRun validation. (The goal is that we can share the validation logic, since it's looking at the same thing just the structure of the input objects is different.) We'll probably also separate the param validation out from the resource validation. The current validation will miss the case where extra resources or parameters that aren't needed are supplied (this was the case already).

In the TaskRun reconciler, there are several different places where logic exists to retrieve references Tasks and Resources; this library will allow us to retrieve these references up front and use them instead of using Listers everywhere. This is a step toward fixing tektoncd#213 (the goal is to reuse and simplify this logic).

With this change, we can use the resolved TaskRun when validating the TaskRun, so we no longer need access to the reconciler or to do any listing when validating the TaskRun. This is a step along the way to tektoncd#213. Next we can do a similar refactoring to the PipelineRun validation. (The goal is that we can share the validation logic, since it's looking at the same thing just the structure of the input objects is different.) We'll probably also separate the param validation out from the resource validation. The current validation will miss the case where extra resources or parameters that aren't needed are supplied (this was the case already).

In the TaskRun reconciler, there are several different places where logic exists to retrieve references Tasks and Resources; this library will allow us to retrieve these references up front and use them instead of using Listers everywhere. This is a step toward fixing #213 (the goal is to reuse and simplify this logic).

With this change, we can use the resolved TaskRun when validating the TaskRun, so we no longer need access to the reconciler or to do any listing when validating the TaskRun. This is a step along the way to #213. Next we can do a similar refactoring to the PipelineRun validation. (The goal is that we can share the validation logic, since it's looking at the same thing just the structure of the input objects is different.) We'll probably also separate the param validation out from the resource validation. The current validation will miss the case where extra resources or parameters that aren't needed are supplied (this was the case already).

This is so that the PipelineRun reconciler can use the same resource resolution. When the PipelineRun resolves resources, there are not yet any TaskRuns to pass along, those get created later. Seems a bit weird that the resolution function still needs to be passed the taskspec + name at all really, but we'll see how this goes! This is part of tektoncd#213

Now when resolving a PipelineRun, we will resolve all of the resource references as well. To do this, we make it so that PipelineRuns and TaskRuns use the same types to bind resources. This has the bonus of allowing users to provide resource paths in pipelineruns if they wanted to. This will allow us to use the same logic to validate resource bindings for both pipelineruns and taskruns (for tektoncd#213).

This way we can test the validation without needing an entire reconciler, and we can also eventually call the same resource validation logic used by TaskRuns (tektoncd#213)

Now the pipelinerun reconciler can call the existing task resource validation and not need to duplciate any of it. Bonus: pipelineruns will now respect tasks that have defaults for their resources! Fixes tektoncd#213