From efba79d9e1eae119fc7616a8744bd8de68e48449 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Fri, 10 Jan 2020 15:38:59 -0500 Subject: [PATCH] Use correct name for embedded pipelines to prevent reconciler panic In a couple of places we were using `pr.Spec.PipelineRef.Name`. pipelineRef is nil for embedded pipelineSpecs leading to reconciler panics. Also added some test cases for that catches these scenarios. Fixes #1785 Signed-off-by: Dibyo Mukherjee --- pkg/reconciler/pipelinerun/pipelinerun.go | 6 ++-- .../pipelinerun/pipelinerun_test.go | 34 ++++++++++++++++++- test/builder/pipeline.go | 19 +++++++++-- test/builder/pipeline_test.go | 29 ++++++++++++++++ 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index f2639467270..c8e0513f105 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -283,7 +283,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er Status: corev1.ConditionFalse, Reason: ReasonFailedValidation, Message: fmt.Sprintf("Pipeline %s can't be Run; it has an invalid spec: %s", - fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err), + fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pipelineMeta.Name), err), }) return nil } @@ -295,7 +295,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er Status: corev1.ConditionFalse, Reason: ReasonInvalidBindings, Message: fmt.Sprintf("PipelineRun %s doesn't bind Pipeline %s's PipelineResources correctly: %s", - fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pr.Spec.PipelineRef.Name), err), + fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err), }) return nil } @@ -322,7 +322,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er Status: corev1.ConditionFalse, Reason: ReasonParameterTypeMismatch, Message: fmt.Sprintf("PipelineRun %s parameters have mismatching types with Pipeline %s's parameters: %s", - fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pr.Spec.PipelineRef.Name), err), + fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err), }) return nil } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index b4a9dfd5b68..c99274bfec9 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -291,6 +291,9 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { tb.TaskInputs(tb.InputsParamSpec("some-param", v1alpha1.ParamTypeString)))), tb.Task("a-task-that-needs-array-params", "foo", tb.TaskSpec( tb.TaskInputs(tb.InputsParamSpec("some-param", v1alpha1.ParamTypeArray)))), + tb.Task("a-task-that-needs-a-resource", "ns", tb.TaskSpec( + tb.TaskInputs(tb.InputsResource("workspace", "git")), + )), } ps := []*v1alpha1.Pipeline{ tb.Pipeline("pipeline-missing-tasks", "foo", tb.PipelineSpec( @@ -320,6 +323,18 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { tb.PipelineRun("pipeline-resources-not-declared", "foo", tb.PipelineRunSpec("a-pipeline-that-should-be-caught-by-admission-control")), tb.PipelineRun("pipeline-mismatching-param-type", "foo", tb.PipelineRunSpec("a-pipeline-with-array-params", tb.PipelineRunParam("some-param", "stringval"))), tb.PipelineRun("pipeline-conditions-missing", "foo", tb.PipelineRunSpec("a-pipeline-with-missing-conditions")), + tb.PipelineRun("embedded-pipeline-resources-not-bound", "foo", tb.PipelineRunSpec("", tb.PipelineRunPipelineSpec( + tb.PipelineTask("some-task", "a-task-that-needs-a-resource"), + tb.PipelineDeclaredResource("workspace", "git"), + ))), + tb.PipelineRun("embedded-pipeline-invalid", "foo", tb.PipelineRunSpec("", tb.PipelineRunPipelineSpec( + tb.PipelineTask("bad-t@$k", "b@d-t@$k"), + ))), + tb.PipelineRun("embedded-pipeline-mismatching-param-type", "foo", tb.PipelineRunSpec("", tb.PipelineRunPipelineSpec( + tb.PipelineParamSpec("some-param", v1alpha1.ParamTypeArray), + tb.PipelineTask("some-task", "a-task-that-needs-array-params")), + tb.PipelineRunParam("some-param", "stringval"), + )), } d := test.Data{ Tasks: ts, @@ -365,6 +380,18 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { name: "invalid-pipeline-missing-conditions-shd-stop-reconciling", pipelineRun: prs[7], reason: ReasonCouldntGetCondition, + }, { + name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling", + pipelineRun: prs[8], + reason: ReasonInvalidBindings, + }, { + name: "invalid-embedded-pipeline-bad-name-shd-stop-reconciling", + pipelineRun: prs[9], + reason: ReasonFailedValidation, + }, { + name: "invalid-embedded-pipeline-mismatching-parameter-types", + pipelineRun: prs[10], + reason: ReasonParameterTypeMismatch, }, } @@ -394,7 +421,12 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { t.Errorf("Expected failure to be because of reason %q but was %s", tc.reason, condition.Reason) } if !tc.hasNoDefaultLabels { - expectedLabels := map[string]string{pipeline.GroupName + pipeline.PipelineLabelKey: tc.pipelineRun.Spec.PipelineRef.Name} + expectedPipelineLabel := tc.pipelineRun.Name + // Embedded pipelines use the pipelinerun name + if tc.pipelineRun.Spec.PipelineRef != nil { + expectedPipelineLabel = tc.pipelineRun.Spec.PipelineRef.Name + } + expectedLabels := map[string]string{pipeline.GroupName + pipeline.PipelineLabelKey: expectedPipelineLabel} if len(tc.pipelineRun.ObjectMeta.Labels) != len(expectedLabels) { t.Errorf("Expected labels : %v, got %v", expectedLabels, tc.pipelineRun.ObjectMeta.Labels) } diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go index 9e160a5b52c..27079c4d99e 100644 --- a/test/builder/pipeline.go +++ b/test/builder/pipeline.go @@ -386,27 +386,40 @@ func PipelineRunNilTimeout(prs *v1alpha1.PipelineRunSpec) { prs.Timeout = nil } -// PipelineRunNodeSelector sets the Node selector to the PipelineSpec. +// PipelineRunNodeSelector sets the Node selector to the PipelineRunSpec. func PipelineRunNodeSelector(values map[string]string) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { prs.PodTemplate.NodeSelector = values } } -// PipelineRunTolerations sets the Node selector to the PipelineSpec. +// PipelineRunTolerations sets the Node selector to the PipelineRunSpec. func PipelineRunTolerations(values []corev1.Toleration) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { prs.PodTemplate.Tolerations = values } } -// PipelineRunAffinity sets the affinity to the PipelineSpec. +// PipelineRunAffinity sets the affinity to the PipelineRunSpec. func PipelineRunAffinity(affinity *corev1.Affinity) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { prs.PodTemplate.Affinity = affinity } } +// PipelineRunPipelineSpec adds a PipelineSpec to the PipelineRunSpec. +// Any number of PipelineSpec modifiers can be passed to transform it. +func PipelineRunPipelineSpec(ops ...PipelineSpecOp) PipelineRunSpecOp { + return func(prs *v1alpha1.PipelineRunSpec) { + ps := &v1alpha1.PipelineSpec{} + prs.PipelineRef = nil + for _, op := range ops { + op(ps) + } + prs.PipelineSpec = ps + } +} + // PipelineRunStatus sets the PipelineRunStatus to the PipelineRun. // Any number of PipelineRunStatus modifier can be passed to transform it. func PipelineRunStatus(ops ...PipelineRunStatusOp) PipelineRunOp { diff --git a/test/builder/pipeline_test.go b/test/builder/pipeline_test.go index 6c5356e701d..1954764633d 100644 --- a/test/builder/pipeline_test.go +++ b/test/builder/pipeline_test.go @@ -264,6 +264,35 @@ func TestPipelineRunWithResourceSpec(t *testing.T) { } } +func TestPipelineRunWithPipelineSpec(t *testing.T) { + pipelineRun := tb.PipelineRun("pear", "foo", tb.PipelineRunSpec("", tb.PipelineRunPipelineSpec( + tb.PipelineTask("a-task", "some-task")), + tb.PipelineRunServiceAccountName("sa"), + )) + + expectedPipelineRun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pear", + Namespace: "foo", + }, + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: nil, + PipelineSpec: &v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{{ + Name: "a-task", + TaskRef: v1alpha1.TaskRef{Name: "some-task"}, + }}, + }, + ServiceAccountName: "sa", + Timeout: &metav1.Duration{Duration: 1 * time.Hour}, + }, + } + + if diff := cmp.Diff(expectedPipelineRun, pipelineRun); diff != "" { + t.Fatalf("PipelineRun diff -want, +got: %s", diff) + } +} + func TestPipelineResource(t *testing.T) { pipelineResource := tb.PipelineResource("git-resource", "foo", tb.PipelineResourceSpec( v1alpha1.PipelineResourceTypeGit, tb.PipelineResourceSpecParam("URL", "https://foo.git"),