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 tektoncd#1785 Signed-off-by: Dibyo Mukherjee <[email protected]>
dibyom · Jan 10, 2020 · efba79d · efba79d
1 parent 6070b2f
commit efba79d
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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"),