Fix nil-pointer dereference of BuildSpec Strategy

pkg/apis/build/v1alpha1/build_types.go

@@ -105,6 +105,16 @@ type BuildSpec struct {
 	Timeout *metav1.Duration `json:"timeout,omitempty"`
 }
 
+// StrategyName returns the name of the configured strategy, or 'undefined' in
+// case the strategy is nil (not set)
+func (buildSpec *BuildSpec) StrategyName() string {
+	if buildSpec.Strategy == nil {
+		return "undefined (nil strategy)"
+	}
+
+	return buildSpec.Strategy.Name
+}
+
 // Image refers to an container image with credentials
 type Image struct {
 	// Image is the reference of the image.

pkg/reconciler/buildrun/buildrun.go

@@ -204,11 +204,16 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 			}
 
 			// Increase BuildRun count in metrics
-			buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.Strategy.Name, buildRun.Namespace, buildRun.Spec.BuildRef.Name, buildRun.Name)
+			buildmetrics.BuildRunCountInc(
+				buildRun.Status.BuildSpec.StrategyName(),
+				buildRun.Namespace,
+				buildRun.Spec.BuildRef.Name,
+				buildRun.Name,
+			)
 
 			// Report buildrun ramp-up duration (time between buildrun creation and taskrun creation)
 			buildmetrics.BuildRunRampUpDurationObserve(
-				buildRun.Status.BuildSpec.Strategy.Name,
+				buildRun.Status.BuildSpec.StrategyName(),
 				buildRun.Namespace,
 				buildRun.Spec.BuildRef.Name,
 				buildRun.Name,
@@ -274,7 +279,7 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 
 				// Report the buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
 				buildmetrics.BuildRunEstablishObserve(
-					buildRun.Status.BuildSpec.Strategy.Name,
+					buildRun.Status.BuildSpec.StrategyName(),
 					buildRun.Namespace,
 					buildRun.Spec.BuildRef.Name,
 					buildRun.Name,
@@ -285,46 +290,43 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 			if lastTaskRun.Status.CompletionTime != nil && buildRun.Status.CompletionTime == nil {
 				buildRun.Status.CompletionTime = lastTaskRun.Status.CompletionTime
 
-				if buildRun.Status.BuildSpec.Strategy != nil {
-					// buildrun completion duration (total time between the creation of the buildrun and the buildrun completion)
-					buildmetrics.BuildRunCompletionObserve(
-						buildRun.Status.BuildSpec.Strategy.Name,
+				// buildrun completion duration (total time between the creation of the buildrun and the buildrun completion)
+				buildmetrics.BuildRunCompletionObserve(
+					buildRun.Status.BuildSpec.StrategyName(),
+					buildRun.Namespace,
+					buildRun.Spec.BuildRef.Name,
+					buildRun.Name,
+					buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
+				)
+
+				// Look for the pod created by the taskrun
+				var pod = &corev1.Pod{}
+				if err := r.client.Get(ctx, types.NamespacedName{Namespace: request.Namespace, Name: lastTaskRun.Status.PodName}, pod); err == nil {
+					if len(pod.Status.InitContainerStatuses) > 0 {
+
+						lastInitPodIdx := len(pod.Status.InitContainerStatuses) - 1
+						lastInitPod := pod.Status.InitContainerStatuses[lastInitPodIdx]
+
+						if lastInitPod.State.Terminated != nil {
+							// taskrun pod ramp-up (time between pod creation and last init container completion)
+							buildmetrics.TaskRunPodRampUpDurationObserve(
+								buildRun.Status.BuildSpec.StrategyName(),
+								buildRun.Namespace,
+								buildRun.Spec.BuildRef.Name,
+								buildRun.Name,
+								lastInitPod.State.Terminated.FinishedAt.Sub(pod.CreationTimestamp.Time),
+							)
+						}
+					}
+
+					// taskrun ramp-up duration (time between taskrun creation and taskrun pod creation)
+					buildmetrics.TaskRunRampUpDurationObserve(
+						buildRun.Status.BuildSpec.StrategyName(),
 						buildRun.Namespace,
 						buildRun.Spec.BuildRef.Name,
 						buildRun.Name,
-						buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
+						pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time),
 					)
-
-					// Look for the pod created by the taskrun
-					var pod = &corev1.Pod{}
-					if err := r.client.Get(ctx, types.NamespacedName{Namespace: request.Namespace, Name: lastTaskRun.Status.PodName}, pod); err == nil {
-						if len(pod.Status.InitContainerStatuses) > 0 {
-
-							lastInitPodIdx := len(pod.Status.InitContainerStatuses) - 1
-							lastInitPod := pod.Status.InitContainerStatuses[lastInitPodIdx]
-
-							if lastInitPod.State.Terminated != nil {
-								// taskrun pod ramp-up (time between pod creation and last init container completion)
-								buildmetrics.TaskRunPodRampUpDurationObserve(
-									buildRun.Status.BuildSpec.Strategy.Name,
-									buildRun.Namespace,
-									buildRun.Spec.BuildRef.Name,
-									buildRun.Name,
-									lastInitPod.State.Terminated.FinishedAt.Sub(pod.CreationTimestamp.Time),
-								)
-							}
-						}
-
-						// taskrun ramp-up duration (time between taskrun creation and taskrun pod creation)
-						buildmetrics.TaskRunRampUpDurationObserve(
-							buildRun.Status.BuildSpec.Strategy.Name,
-							buildRun.Namespace,
-							buildRun.Spec.BuildRef.Name,
-							buildRun.Name,
-							pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time),
-						)
-
-					}
 				}
 			}
 

pkg/reconciler/buildrun/buildrun_test.go

@@ -244,7 +244,21 @@ var _ = Describe("Reconcile BuildRun", func() {
 				Expect(serviceAccount.Name).To(Equal(buildRunSample.Name + "-sa"))
 				Expect(serviceAccount.Namespace).To(Equal(buildRunSample.Namespace))
 			})
+
+			It("should not panic in case the build spec strategy is nil", func() {
+				// As long as the Strategy is a pointer, it can happen that the
+				// field is nil. During processing, the BuildSpec is copied
+				// from the respective Build. In case Strategy is nil, the
+				// controller should not panic.
+				buildRunSample.Status.BuildSpec.Strategy = nil
+				Expect(func() {
+					result, err := reconciler.Reconcile(taskRunRequest)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(result).ToNot(BeNil())
+				}).ShouldNot(Panic())
+			})
 		})
+
 		Context("from an existing TaskRun with Conditions", func() {
 			BeforeEach(func() {