From 0e9066bacb6fb087a3a3b43492877c94420d03c0 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Tue, 24 Sep 2019 17:13:54 -0400 Subject: [PATCH] Resolve all PipelineResources first before continuing As part of #1184 I need to call `GetSetup` on all PipelineResources early on in PipelineRun execution. Since PipelineRuns declare all their resource up front, I wanted to be able to resolve all of them at once, then call `GetSetup` on all of them. Also, as Pipelines got more complex (we added Conditions) it turned out we were retrieving the resources in a few different places. Also in #1324 @pritidesai is making it so that these Resources can be provided by spec. By resolving all of this up front at once, we can simplify the logic later on. And you can see in this commit that we are able to reduce the responsibilities of ResolvePipelineRun a bit too! --- pkg/reconciler/pipelinerun/pipelinerun.go | 24 +- .../pipelinerun/pipelinerun_test.go | 4 +- .../resources/pipelinerunresolution.go | 164 ++++------- .../resources/pipelinerunresolution_test.go | 255 +++++++----------- .../taskrun/resources/input_resources.go | 21 -- .../resources/taskresourceresolution.go | 22 ++ .../resources/taskresourceresolution_test.go | 31 +++ 7 files changed, 218 insertions(+), 303 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 31fe2ee17ed..e93b87dea05 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -242,8 +242,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er }) return nil } - providedResources, err := resources.GetResourcesFromBindings(p, pr) - if err != nil { + if err := resources.ValidateResourceBindings(p, pr); err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it pr.Status.SetCondition(&apis.Condition{ Type: apis.ConditionSucceeded, @@ -254,6 +253,18 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er }) return nil } + providedResources, err := resources.GetResourcesFromBindings(pr, c.resourceLister.PipelineResources(pr.Namespace).Get) + if err != nil { + // This Run has failed, so we need to mark it as failed and stop reconciling it + pr.Status.SetCondition(&apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: ReasonCouldntGetResource, + Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s", + fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err), + }) + return nil + } // Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type. // Weird substitution issues can occur if this is not validated (ApplyParameters() does not verify type). @@ -284,7 +295,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er func(name string) (v1alpha1.TaskInterface, error) { return c.clusterTaskLister.Get(name) }, - c.resourceLister.PipelineResources(pr.Namespace).Get, func(name string) (*v1alpha1.Condition, error) { return c.conditionLister.Conditions(pr.Namespace).Get(name) }, @@ -301,14 +311,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er Message: fmt.Sprintf("Pipeline %s can't be Run; it contains Tasks that don't exist: %s", fmt.Sprintf("%s/%s", p.Namespace, p.Name), err), }) - case *resources.ResourceNotFoundError: - pr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionFalse, - Reason: ReasonCouldntGetResource, - Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s", - fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err), - }) case *resources.ConditionNotFoundError: pr.Status.SetCondition(&apis.Condition{ Type: apis.ConditionSucceeded, diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index ef7645d75e3..c6f7bfa43f7 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -225,8 +225,8 @@ func TestReconcile(t *testing.T) { ) // ignore IgnoreUnexported ignore both after and before steps fields - if d := cmp.Diff(actual, expectedTaskRun, cmpopts.SortSlices(func(x, y v1alpha1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" { - t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, d) + if d := cmp.Diff(expectedTaskRun, actual, cmpopts.SortSlices(func(x, y v1alpha1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff (-want, +got): %s", expectedTaskRun, d) } // test taskrun is able to recreate correct pipeline-pvc-name if expectedTaskRun.GetPipelineRunPVCName() != "test-pipeline-run-success-pvc" { diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index eddfebb9a00..a181fe3eee2 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -24,7 +24,6 @@ import ( "golang.org/x/xerrors" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" @@ -97,7 +96,7 @@ func (t ResolvedPipelineRunTask) IsSuccessful() bool { return false } -// IsFailed returns true only if the taskrun itself has failed +// IsFailure returns true only if the taskrun itself has failed func (t ResolvedPipelineRunTask) IsFailure() bool { if t.TaskRun == nil { return false @@ -170,12 +169,24 @@ func (state PipelineRunState) SuccessfulPipelineTaskNames() []string { // GetTaskRun is a function that will retrieve the TaskRun name. type GetTaskRun func(name string) (*v1alpha1.TaskRun, error) -// GetResourcesFromBindings will validate that all PipelineResources declared in Pipeline p are bound in PipelineRun pr -// and if so, will return a map from the declared name of the PipelineResource (which is how the PipelineResource will -// be referred to in the PipelineRun) to the ResourceRef. -func GetResourcesFromBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) (map[string]v1alpha1.PipelineResourceBinding, error) { - resources := map[string]v1alpha1.PipelineResourceBinding{} +// GetResourcesFromBindings will retreive all Resources bound in PipelineRun pr and return a map +// from the declared name of the PipelineResource (which is how the PipelineResource will +// be referred to in the PipelineRun) to the PipelineResource, obtained via getResource. +func GetResourcesFromBindings(pr *v1alpha1.PipelineRun, getResource resources.GetResource) (map[string]*v1alpha1.PipelineResource, error) { + resources := map[string]*v1alpha1.PipelineResource{} + for _, resource := range pr.Spec.Resources { + r, err := getResource(resource.ResourceRef.Name) + if err != nil { + return resources, xerrors.Errorf("Error following resource reference for %s: %w", resource.Name, err) + } + resources[resource.Name] = r + } + return resources, nil +} + +// ValidateResourceBindings validate that the PipelineResources declared in Pipeline p are bound in PipelineRun. +func ValidateResourceBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) error { required := make([]string, 0, len(p.Spec.Resources)) for _, resource := range p.Spec.Resources { required = append(required, resource.Name) @@ -184,44 +195,10 @@ func GetResourcesFromBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) (m for _, resource := range pr.Spec.Resources { provided = append(provided, resource.Name) } - err := list.IsSame(required, provided) - if err != nil { - return resources, xerrors.Errorf("PipelineRun bound resources didn't match Pipeline: %w", err) + if err := list.IsSame(required, provided); err != nil { + return xerrors.Errorf("PipelineRun bound resources didn't match Pipeline: %w", err) } - - for _, resource := range pr.Spec.Resources { - resources[resource.Name] = resource - } - return resources, nil -} - -func getPipelineRunTaskResources(pt v1alpha1.PipelineTask, providedResources map[string]v1alpha1.PipelineResourceBinding) ([]v1alpha1.TaskResourceBinding, []v1alpha1.TaskResourceBinding, error) { - inputs, outputs := []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{} - if pt.Resources != nil { - for _, taskInput := range pt.Resources.Inputs { - resource, ok := providedResources[taskInput.Resource] - if !ok { - return inputs, outputs, xerrors.Errorf("pipelineTask tried to use input resource %s not present in declared resources", taskInput.Resource) - } - inputs = append(inputs, v1alpha1.TaskResourceBinding{ - Name: taskInput.Name, - ResourceRef: resource.ResourceRef, - ResourceSpec: resource.ResourceSpec, - }) - } - for _, taskOutput := range pt.Resources.Outputs { - resource, ok := providedResources[taskOutput.Resource] - if !ok { - return outputs, outputs, xerrors.Errorf("pipelineTask tried to use output resource %s not present in declared resources", taskOutput.Resource) - } - outputs = append(outputs, v1alpha1.TaskResourceBinding{ - Name: taskOutput.Name, - ResourceRef: resource.ResourceRef, - ResourceSpec: resource.ResourceSpec, - }) - } - } - return inputs, outputs, nil + return nil } // TaskNotFoundError indicates that the resolution failed because a referenced Task couldn't be retrieved @@ -234,15 +211,6 @@ func (e *TaskNotFoundError) Error() string { return fmt.Sprintf("Couldn't retrieve Task %q: %s", e.Name, e.Msg) } -// ResourceNotFoundError indicates that the resolution failed because a referenced PipelineResource couldn't be retrieved -type ResourceNotFoundError struct { - Msg string -} - -func (e *ResourceNotFoundError) Error() string { - return fmt.Sprintf("Couldn't retrieve PipelineResource: %s", e.Msg) -} - type ConditionNotFoundError struct { Name string Msg string @@ -255,17 +223,15 @@ func (e *ConditionNotFoundError) Error() string { // ResolvePipelineRun retrieves all Tasks instances which are reference by tasks, getting // instances from getTask. If it is unable to retrieve an instance of a referenced Task, it // will return an error, otherwise it returns a list of all of the Tasks retrieved. -// It will retrieve the Resources needed for the TaskRun as well using getResource and the mapping -// of providedResources. +// It will retrieve the Resources needed for the TaskRun using the mapping of providedResources. func ResolvePipelineRun( pipelineRun v1alpha1.PipelineRun, getTask resources.GetTask, getTaskRun resources.GetTaskRun, getClusterTask resources.GetClusterTask, - getResource resources.GetResource, getCondition GetCondition, tasks []v1alpha1.PipelineTask, - providedResources map[string]v1alpha1.PipelineResourceBinding, + providedResources map[string]*v1alpha1.PipelineResource, ) (PipelineRunState, error) { state := []*ResolvedPipelineRunTask{} @@ -292,17 +258,10 @@ func ResolvePipelineRun( } } - // Get all the resources that this task will be using, if any - inputs, outputs, err := getPipelineRunTaskResources(pt, providedResources) - if err != nil { - return nil, xerrors.Errorf("unexpected error which should have been caught by Pipeline webhook: %w", err) - } - spec := t.TaskSpec() - rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, pt.TaskRef.Kind, inputs, outputs, getResource) - + rtr, err := ResolvePipelineTaskResources(pt, &spec, t.TaskMetadata().Name, pt.TaskRef.Kind, providedResources) if err != nil { - return nil, &ResourceNotFoundError{Msg: err.Error()} + return nil, xerrors.Errorf("couldn't match referenced resources with declared resources: %w", err) } rprt.ResolvedTaskResources = rtr @@ -319,7 +278,7 @@ func ResolvePipelineRun( // Get all conditions that this pipelineTask will be using, if any if len(pt.Conditions) > 0 { - rcc, err := resolveConditionChecks(&pt, pipelineRun.Status.TaskRuns, rprt.TaskRunName, getTaskRun, getCondition, getResource, providedResources) + rcc, err := resolveConditionChecks(&pt, pipelineRun.Status.TaskRuns, rprt.TaskRunName, getTaskRun, getCondition, providedResources) if err != nil { return nil, err } @@ -365,7 +324,6 @@ func GetPipelineConditionStatus(pr *v1alpha1.PipelineRun, state PipelineRunState // 2. Any one TaskRun has failed - >Failed. This should change with #1020 and #1023 // 3. All tasks are done or are skipped (i.e. condition check failed).-> Success // 4. A Task or Condition is running right now or there are things left to run -> Running - if pr.IsTimedOut() { return &apis.Condition{ Type: apis.ConditionSucceeded, @@ -494,10 +452,7 @@ func ValidateFrom(state PipelineRunState) error { return nil } -func resolveConditionChecks(pt *v1alpha1.PipelineTask, - taskRunStatus map[string]*v1alpha1.PipelineRunTaskRunStatus, - taskRunName string, getTaskRun resources.GetTaskRun, getCondition GetCondition, - getResource resources.GetResource, providedResources map[string]v1alpha1.PipelineResourceBinding) ([]*ResolvedConditionCheck, error) { +func resolveConditionChecks(pt *v1alpha1.PipelineTask, taskRunStatus map[string]*v1alpha1.PipelineRunTaskRunStatus, taskRunName string, getTaskRun resources.GetTaskRun, getCondition GetCondition, providedResources map[string]*v1alpha1.PipelineResource) ([]*ResolvedConditionCheck, error) { rccs := []*ResolvedConditionCheck{} for _, ptc := range pt.Conditions { cName := ptc.ConditionRef @@ -515,20 +470,21 @@ func resolveConditionChecks(pt *v1alpha1.PipelineTask, return nil, xerrors.Errorf("error retrieving ConditionCheck %s for taskRun name %s : %w", conditionCheckName, taskRunName, err) } } + conditionResources := map[string]*v1alpha1.PipelineResource{} + for _, declared := range ptc.Resources { + r, ok := providedResources[declared.Resource] + if !ok { + return nil, xerrors.Errorf("resources %s missing for condition %s in pipeline task %s", declared.Resource, cName, pt.Name) + } + conditionResources[declared.Name] = r + } rcc := ResolvedConditionCheck{ Condition: c, ConditionCheckName: conditionCheckName, ConditionCheck: v1alpha1.NewConditionCheck(cctr), PipelineTaskCondition: &ptc, - } - - if len(ptc.Resources) > 0 { - r, err := resolveConditionResources(ptc.Resources, getResource, providedResources) - if err != nil { - return nil, xerrors.Errorf("cloud not resolve resources for condition %s in pipeline task %s: %w", cName, pt.Name, err) - } - rcc.ResolvedResources = r + ResolvedResources: conditionResources, } rccs = append(rccs, &rcc) @@ -536,33 +492,31 @@ func resolveConditionChecks(pt *v1alpha1.PipelineTask, return rccs, nil } -func resolveConditionResources(prc []v1alpha1.PipelineConditionResource, - getResource resources.GetResource, - providedResources map[string]v1alpha1.PipelineResourceBinding, -) (map[string]*v1alpha1.PipelineResource, error) { - rr := make(map[string]*v1alpha1.PipelineResource) - for _, r := range prc { - // First get a ref to actual resource name from its bound name - resourceBinding, ok := providedResources[r.Resource] - if !ok { - return nil, xerrors.Errorf("resource %s not present in declared resources", r.Resource) - } - // Next, fetch the actual resource definition - if resourceBinding.ResourceRef.Name != "" { - gotResource, err := getResource(resourceBinding.ResourceRef.Name) - if err != nil { - return nil, xerrors.Errorf("could not retrieve resource %s: %w", r.Name, err) +// ResolvePipelineTaskResources matches PipelineResources referenced by pt inputs and outputs with the +// providedResources and returns an instance of ResolvedTaskResources. +func ResolvePipelineTaskResources(pt v1alpha1.PipelineTask, ts *v1alpha1.TaskSpec, taskName string, kind v1alpha1.TaskKind, providedResources map[string]*v1alpha1.PipelineResource) (*resources.ResolvedTaskResources, error) { + rtr := resources.ResolvedTaskResources{ + TaskName: taskName, + TaskSpec: ts, + Kind: kind, + Inputs: map[string]*v1alpha1.PipelineResource{}, + Outputs: map[string]*v1alpha1.PipelineResource{}, + } + if pt.Resources != nil { + for _, taskInput := range pt.Resources.Inputs { + resource, ok := providedResources[taskInput.Resource] + if !ok { + return nil, xerrors.Errorf("pipelineTask tried to use input resource %s not present in declared resources", taskInput.Resource) } - // Finally add it to the resolved resources map - rr[r.Name] = gotResource - } else if resourceBinding.ResourceSpec != nil && resourceBinding.ResourceSpec.Type != "" { - rr[r.Name] = &v1alpha1.PipelineResource{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceBinding.Name, - }, - Spec: *resourceBinding.ResourceSpec, + rtr.Inputs[taskInput.Name] = resource + } + for _, taskOutput := range pt.Resources.Outputs { + resource, ok := providedResources[taskOutput.Resource] + if !ok { + return nil, xerrors.Errorf("pipelineTask tried to use output resource %s not present in declared resources", taskOutput.Resource) } + rtr.Outputs[taskOutput.Name] = resource } } - return rr, nil + return &rtr, nil } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 0bdfb11e24c..8532cddd434 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "fmt" "strings" "testing" @@ -1031,52 +1032,36 @@ func TestGetPipelineConditionStatus(t *testing.T) { } func TestGetResourcesFromBindings(t *testing.T) { - p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("git-resource", "git"), - )) pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), )) - m, err := GetResourcesFromBindings(p, pr) + r := tb.PipelineResource("sweet-resource", "namespace") + getResource := func(name string) (*v1alpha1.PipelineResource, error) { + if name != "sweet-resource" { + return nil, fmt.Errorf("Request for unexpected resource %s", name) + } + return r, nil + } + m, err := GetResourcesFromBindings(pr, getResource) if err != nil { t.Fatalf("didn't expect error getting resources from bindings but got: %v", err) } - expectedResources := map[string]v1alpha1.PipelineResourceBinding{ - "git-resource": { - Name: "git-resource", - ResourceRef: v1alpha1.PipelineResourceRef{Name: "sweet-resource"}, - }, - } + expectedResources := map[string]*v1alpha1.PipelineResource{"git-resource": r} if d := cmp.Diff(expectedResources, m); d != "" { t.Fatalf("Expected resources didn't match actual -want, +got: %v", d) } } -func TestGetResourcesFromBindings_Missing(t *testing.T) { - p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("git-resource", "git"), - tb.PipelineDeclaredResource("image-resource", "image"), - )) +func TestGetResourcesFromBindings_ErrorGettingResource(t *testing.T) { pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), )) - _, err := GetResourcesFromBindings(p, pr) - if err == nil { - t.Fatalf("Expected error indicating `image-resource` was missing but got no error") + getResource := func(name string) (*v1alpha1.PipelineResource, error) { + return nil, xerrors.Errorf("IT HAS ALL GONE WRONG") } -} - -func TestGetResourcesFromBindings_Extra(t *testing.T) { - p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("git-resource", "git"), - )) - pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", - tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), - tb.PipelineRunResourceBinding("image-resource", tb.PipelineResourceBindingRef("sweet-resource2")), - )) - _, err := GetResourcesFromBindings(p, pr) + _, err := GetResourcesFromBindings(pr, getResource) if err == nil { - t.Fatalf("Expected error indicating `image-resource` was extra but got no error") + t.Fatalf("Expected error indicating resource couldnt be retrieved but got no error") } } @@ -1095,14 +1080,6 @@ func TestResolvePipelineRun(t *testing.T) { tb.PipelineTaskOutputResource("output1", "git-resource"), ), )) - providedResources := map[string]v1alpha1.PipelineResourceBinding{ - "git-resource": { - Name: "someresource", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "someresource", - }, - }, - } r := &v1alpha1.PipelineResource{ ObjectMeta: metav1.ObjectMeta{ @@ -1112,6 +1089,7 @@ func TestResolvePipelineRun(t *testing.T) { Type: v1alpha1.PipelineResourceTypeGit, }, } + providedResources := map[string]*v1alpha1.PipelineResource{"git-resource": r} pr := v1alpha1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinerun", @@ -1122,10 +1100,9 @@ func TestResolvePipelineRun(t *testing.T) { getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return nil, nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, nil } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { return r, nil } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, nil } - pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, p.Spec.Tasks, providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, p.Spec.Tasks, providedResources) if err != nil { t.Fatalf("Error getting tasks for fake pipeline %s: %s", p.ObjectMeta.Name, err) } @@ -1167,8 +1144,8 @@ func TestResolvePipelineRun(t *testing.T) { }, }} - if d := cmp.Diff(pipelineState, expectedState, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { - t.Errorf("Expected to get current pipeline state %v, but actual differed: %s", expectedState, d) + if d := cmp.Diff(expectedState, pipelineState, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { + t.Errorf("Expected to get current pipeline state %v, but actual differed (-want, +got): %s", expectedState, d) } } @@ -1183,21 +1160,18 @@ func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) { Name: "mytask3", TaskRef: v1alpha1.TaskRef{Name: "task"}, }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return &trs[0], nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return clustertask, nil } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("should not get called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, nil } pr := v1alpha1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinerun", }, } - pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, pts, providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, pts, providedResources) if err != nil { t.Fatalf("Did not expect error when resolving PipelineRun without Resources: %v", err) } @@ -1223,7 +1197,7 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) { Name: "mytask1", TaskRef: v1alpha1.TaskRef{Name: "task"}, }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} // Return an error when the Task is retrieved, as if it didn't exist getTask := func(name string) (v1alpha1.TaskInterface, error) { @@ -1236,9 +1210,6 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) { getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return nil, errors.NewNotFound(v1alpha1.Resource("taskrun"), name) } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("should not get called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, nil } @@ -1247,7 +1218,7 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) { Name: "pipelinerun", }, } - _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, pts, providedResources) + _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, pts, providedResources) switch err := err.(type) { case nil: t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.Name) @@ -1277,14 +1248,11 @@ func TestResolvePipelineRun_ResourceBindingsDontExist(t *testing.T) { ), )), }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return &trs[0], nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return clustertask, nil } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("shouldnt be called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, nil } @@ -1296,7 +1264,7 @@ func TestResolvePipelineRun_ResourceBindingsDontExist(t *testing.T) { Name: "pipelinerun", }, } - _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, tt.p.Spec.Tasks, providedResources) + _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, tt.p.Spec.Tasks, providedResources) if err == nil { t.Fatalf("Expected error when bindings are in incorrect state for Pipeline %s but got none", p.Name) } @@ -1304,61 +1272,6 @@ func TestResolvePipelineRun_ResourceBindingsDontExist(t *testing.T) { } } -func TestResolvePipelineRun_ResourcesDontExist(t *testing.T) { - tests := []struct { - name string - p *v1alpha1.Pipeline - }{{ - name: "input doesnt exist", - p: tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( - tb.PipelineTask("mytask1", "task", - tb.PipelineTaskInputResource("input1", "git-resource"), - ), - )), - }, { - name: "output doesnt exist", - p: tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( - tb.PipelineTask("mytask1", "task", - tb.PipelineTaskOutputResource("input1", "git-resource"), - ), - )), - }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{ - "git-resource": { - Name: "doesnt-exist", - }, - } - - getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } - getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return &trs[0], nil } - getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return clustertask, nil } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, errors.NewNotFound(v1alpha1.Resource("pipelineresource"), name) - } - getCondition := func(name string) (*v1alpha1.Condition, error) { - return nil, nil - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pr := v1alpha1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinerun", - }, - } - _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, tt.p.Spec.Tasks, providedResources) - switch err := err.(type) { - case nil: - t.Fatalf("Expected error getting non-existent Resources for Pipeline %s but got none", p.Name) - case *ResourceNotFoundError: - // expected error - default: - t.Fatalf("Expected specific error type returned by func for non-existent Resource for Pipeline %s but got %s", p.Name, err) - } - }) - } -} - func TestValidateFrom(t *testing.T) { r := tb.PipelineResource("holygrail", namespace, tb.PipelineResourceSpec(v1alpha1.PipelineResourceTypeImage)) state := []*ResolvedPipelineRunTask{{ @@ -1543,14 +1456,6 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) { tb.PipelineTaskInputResource("input1", "git-resource"), ), )) - providedResources := map[string]v1alpha1.PipelineResourceBinding{ - "git-resource": { - Name: "someresource", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "someresource", - }, - }, - } r := &v1alpha1.PipelineResource{ ObjectMeta: metav1.ObjectMeta{ @@ -1560,6 +1465,7 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) { Type: v1alpha1.PipelineResourceTypeGit, }, } + providedResources := map[string]*v1alpha1.PipelineResource{"git-resource": r} taskrunStatus := map[string]*v1alpha1.PipelineRunTaskRunStatus{} taskrunStatus["pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj"] = &v1alpha1.PipelineRunTaskRunStatus{ PipelineTaskName: "mytask-with-a-really-long-name-to-trigger-truncation", @@ -1580,9 +1486,8 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) { getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return nil, nil } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { return r, nil } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, nil } - pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, p.Spec.Tasks, providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, p.Spec.Tasks, providedResources) if err != nil { t.Fatalf("Error getting tasks for fake pipeline %s: %s", p.ObjectMeta.Name, err) } @@ -1624,13 +1529,10 @@ func TestResolveConditionChecks(t *testing.T) { TaskRef: v1alpha1.TaskRef{Name: "task"}, Conditions: []v1alpha1.PipelineTaskCondition{ptc}, }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, xerrors.New("should not get called") } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("should not get called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return &condition, nil } pr := v1alpha1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ @@ -1658,6 +1560,7 @@ func TestResolveConditionChecks(t *testing.T) { Condition: &condition, ConditionCheck: v1alpha1.NewConditionCheck(cc), PipelineTaskCondition: &ptc, + ResolvedResources: providedResources, }}, }, { @@ -1674,19 +1577,20 @@ func TestResolveConditionChecks(t *testing.T) { ConditionCheckName: "pipelinerun-mytask1-mssqb-always-true-78c5n", Condition: &condition, PipelineTaskCondition: &ptc, + ResolvedResources: providedResources, }}, }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - pipelineState, err := ResolvePipelineRun(pr, getTask, tc.getTaskRun, getClusterTask, getResource, getCondition, pts, providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, tc.getTaskRun, getClusterTask, getCondition, pts, providedResources) if err != nil { t.Fatalf("Did not expect error when resolving PipelineRun without Conditions: %v", err) } - if d := cmp.Diff(pipelineState[0].ResolvedConditionChecks, tc.expectedConditionCheck, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { - t.Fatalf("ConditionChecks did not resolve as expected for case %s : %s", tc.name, d) + if d := cmp.Diff(tc.expectedConditionCheck, pipelineState[0].ResolvedConditionChecks, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { + t.Fatalf("ConditionChecks did not resolve as expected for case %s (-want, +got) : %s", tc.name, d) } }) } @@ -1704,7 +1608,7 @@ func TestResolveConditionChecks_ConditionDoesNotExist(t *testing.T) { ConditionRef: "does-not-exist", }}, }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { @@ -1716,9 +1620,6 @@ func TestResolveConditionChecks_ConditionDoesNotExist(t *testing.T) { return nil, xerrors.Errorf("getTaskRun called with unexpected name %s", name) } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, xerrors.New("should not get called") } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("should not get called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return nil, errors.NewNotFound(v1alpha1.Resource("condition"), name) } @@ -1728,7 +1629,7 @@ func TestResolveConditionChecks_ConditionDoesNotExist(t *testing.T) { }, } - _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, pts, providedResources) + _, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, pts, providedResources) switch err := err.(type) { case nil: @@ -1762,7 +1663,7 @@ func TestResolveConditionCheck_UseExistingConditionCheckName(t *testing.T) { TaskRef: v1alpha1.TaskRef{Name: "task"}, Conditions: []v1alpha1.PipelineTaskCondition{ptc}, }} - providedResources := map[string]v1alpha1.PipelineResourceBinding{} + providedResources := map[string]*v1alpha1.PipelineResource{} getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { @@ -1774,9 +1675,6 @@ func TestResolveConditionCheck_UseExistingConditionCheckName(t *testing.T) { return nil, xerrors.Errorf("getTaskRun called with unexpected name %s", name) } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, xerrors.New("should not get called") } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - return nil, xerrors.New("should not get called") - } getCondition := func(name string) (*v1alpha1.Condition, error) { return &condition, nil } ccStatus := make(map[string]*v1alpha1.PipelineRunConditionCheckStatus) @@ -1798,7 +1696,7 @@ func TestResolveConditionCheck_UseExistingConditionCheckName(t *testing.T) { }, } - pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, pts, providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, pts, providedResources) if err != nil { t.Fatalf("Did not expect error when resolving PipelineRun without Conditions: %v", err) } @@ -1807,10 +1705,11 @@ func TestResolveConditionCheck_UseExistingConditionCheckName(t *testing.T) { Condition: &condition, ConditionCheck: v1alpha1.NewConditionCheck(cc), PipelineTaskCondition: &ptc, + ResolvedResources: providedResources, }} - if d := cmp.Diff(pipelineState[0].ResolvedConditionChecks, expectedConditionChecks, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { - t.Fatalf("ConditionChecks did not resolve as expected : %s", d) + if d := cmp.Diff(expectedConditionChecks, pipelineState[0].ResolvedConditionChecks, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { + t.Fatalf("ConditionChecks did not resolve as expected (-want, +got): %s", d) } } @@ -1841,12 +1740,6 @@ func TestResolvedConditionCheck_WithResources(t *testing.T) { getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil } getTaskRun := func(name string) (*v1alpha1.TaskRun, error) { return nil, nil } getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, xerrors.New("should not get called") } - getResource := func(name string) (*v1alpha1.PipelineResource, error) { - if name == "some-repo" { - return gitResource, nil - } - return nil, xerrors.Errorf("getResource called with unexpected name: %s", name) - } getCondition := func(name string) (*v1alpha1.Condition, error) { return condition, nil } @@ -1859,29 +1752,22 @@ func TestResolvedConditionCheck_WithResources(t *testing.T) { tcs := []struct { name string - providedResources map[string]v1alpha1.PipelineResourceBinding + providedResources map[string]*v1alpha1.PipelineResource wantErr bool expected map[string]*v1alpha1.PipelineResource }{{ - name: "resource exists", - providedResources: map[string]v1alpha1.PipelineResourceBinding{ - "blah": { - Name: "some-repo", - }, - }, - expected: map[string]*v1alpha1.PipelineResource{}, + name: "resource exists", + providedResources: map[string]*v1alpha1.PipelineResource{"blah": gitResource}, + expected: map[string]*v1alpha1.PipelineResource{"workspace": gitResource}, }, { - name: "undeclared resource", - providedResources: map[string]v1alpha1.PipelineResourceBinding{ - "foo": { - Name: "some-repo", - }}, - wantErr: true, + name: "resource does not exist", + providedResources: map[string]*v1alpha1.PipelineResource{}, + wantErr: true, }} for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getResource, getCondition, pts, tc.providedResources) + pipelineState, err := ResolvePipelineRun(pr, getTask, getTaskRun, getClusterTask, getCondition, pts, tc.providedResources) if tc.wantErr { if err == nil { @@ -1897,10 +1783,51 @@ func TestResolvedConditionCheck_WithResources(t *testing.T) { PipelineTaskCondition: &ptc, ResolvedResources: tc.expected, }} - if d := cmp.Diff(pipelineState[0].ResolvedConditionChecks, expectedConditionChecks, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { - t.Fatalf("ConditionChecks did not resolve as expected : %s", d) + if d := cmp.Diff(expectedConditionChecks, pipelineState[0].ResolvedConditionChecks, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" { + t.Fatalf("ConditionChecks did not resolve as expected (-want, +got): %s", d) } } }) } } + +func TestValidateResourceBindings(t *testing.T) { + p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("git-resource", "git"), + )) + pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", + tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), + )) + err := ValidateResourceBindings(p, pr) + if err != nil { + t.Fatalf("didn't expect error getting resources from bindings but got: %v", err) + } +} + +func TestValidateResourceBindings_Missing(t *testing.T) { + p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("git-resource", "git"), + tb.PipelineDeclaredResource("image-resource", "image"), + )) + pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", + tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), + )) + err := ValidateResourceBindings(p, pr) + if err == nil { + t.Fatalf("Expected error indicating `image-resource` was missing but got no error") + } +} + +func TestGetResourcesFromBindings_Extra(t *testing.T) { + p := tb.Pipeline("pipelines", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("git-resource", "git"), + )) + pr := tb.PipelineRun("pipelinerun", "namespace", tb.PipelineRunSpec("pipeline", + tb.PipelineRunResourceBinding("git-resource", tb.PipelineResourceBindingRef("sweet-resource")), + tb.PipelineRunResourceBinding("image-resource", tb.PipelineResourceBindingRef("sweet-resource2")), + )) + err := ValidateResourceBindings(p, pr) + if err == nil { + t.Fatalf("Expected error indicating `image-resource` was extra but got no error") + } +} diff --git a/pkg/reconciler/taskrun/resources/input_resources.go b/pkg/reconciler/taskrun/resources/input_resources.go index 7223e0f1ab8..44a3005bca7 100644 --- a/pkg/reconciler/taskrun/resources/input_resources.go +++ b/pkg/reconciler/taskrun/resources/input_resources.go @@ -25,7 +25,6 @@ import ( "go.uber.org/zap" "golang.org/x/xerrors" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" ) @@ -122,26 +121,6 @@ func AddInputResource( return taskSpec, nil } -func getResource(r *v1alpha1.TaskResourceBinding, getter GetResource) (*v1alpha1.PipelineResource, error) { - // Check both resource ref or resource Spec are not present. Taskrun webhook should catch this in validation error. - if r.ResourceRef.Name != "" && r.ResourceSpec != nil { - return nil, xerrors.New("Both ResourseRef and ResourceSpec are defined. Expected only one") - } - - if r.ResourceRef.Name != "" { - return getter(r.ResourceRef.Name) - } - if r.ResourceSpec != nil { - return &v1alpha1.PipelineResource{ - ObjectMeta: metav1.ObjectMeta{ - Name: r.Name, - }, - Spec: *r.ResourceSpec, - }, nil - } - return nil, xerrors.New("Neither ResourseRef not ResourceSpec is defined") -} - func destinationPath(name, path string) string { if path == "" { return filepath.Join(workspaceDir, name) diff --git a/pkg/reconciler/taskrun/resources/taskresourceresolution.go b/pkg/reconciler/taskrun/resources/taskresourceresolution.go index 9273524f0e6..b877b95f10c 100644 --- a/pkg/reconciler/taskrun/resources/taskresourceresolution.go +++ b/pkg/reconciler/taskrun/resources/taskresourceresolution.go @@ -19,6 +19,7 @@ package resources import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "golang.org/x/xerrors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // ResolvedTaskResources contains all the data that is needed to execute @@ -70,3 +71,24 @@ func ResolveTaskResources(ts *v1alpha1.TaskSpec, taskName string, kind v1alpha1. } return &rtr, nil } + +// getResource will return an instance of a PipelineResource to use for r, either by getting it with getter or by +// instantiating it from the embedded spec. +func getResource(r *v1alpha1.TaskResourceBinding, getter GetResource) (*v1alpha1.PipelineResource, error) { + if r.ResourceRef.Name != "" && r.ResourceSpec != nil { + return nil, xerrors.New("Both ResourseRef and ResourceSpec are defined. Expected only one") + } + + if r.ResourceRef.Name != "" { + return getter(r.ResourceRef.Name) + } + if r.ResourceSpec != nil { + return &v1alpha1.PipelineResource{ + ObjectMeta: metav1.ObjectMeta{ + Name: r.Name, + }, + Spec: *r.ResourceSpec, + }, nil + } + return nil, xerrors.New("Neither ResourseRef nor ResourceSpec is defined") +} diff --git a/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go b/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go index 9197d09c83f..2eb151d75dd 100644 --- a/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go +++ b/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go @@ -207,3 +207,34 @@ func TestResolveTaskRun_noResources(t *testing.T) { t.Errorf("Did not expect any outputs to be resolved when none specified but had %v", rtr.Outputs) } } + +func TestResolveTaskRun_InvalidBothSpecified(t *testing.T) { + inputs := []v1alpha1.TaskResourceBinding{{ + Name: "repoToBuildFrom", + // Can't specify both ResourceRef and ResourceSpec + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "git-repo", + }, + ResourceSpec: &v1alpha1.PipelineResourceSpec{ + Type: v1alpha1.PipelineResourceTypeGit, + }, + }} + gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil } + + _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", v1alpha1.NamespacedTaskKind, inputs, []v1alpha1.TaskResourceBinding{}, gr) + if err == nil { + t.Fatalf("Expected to get error because both ref and spec were used") + } +} + +func TestResolveTaskRun_InvalidNeitherSpecified(t *testing.T) { + inputs := []v1alpha1.TaskResourceBinding{{ + Name: "repoToBuildFrom", + }} + gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil } + + _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", v1alpha1.NamespacedTaskKind, inputs, []v1alpha1.TaskResourceBinding{}, gr) + if err == nil { + t.Fatalf("Expected to get error because neither spec or ref were used") + } +}