Repository: shipwright-io/build
Pull request #710 (2021-04-01 14:13:08)

Fix nil-pointer dereference of BuildSpec Strategy
# Changes

There were cases in which the Build had a `nil` Strategy, which is copied into
the BuildRun `Status.BuildSpec` and later used internally, for example the
metrics code is using the `Strategy` field. In case it is `nil`, the function
call for the metrics failed with a nil-pointer dereference.

Introduce `BuildSpec` convenience function called `StrategyName` to return the
name of the strategy, which includes a `nil` safety check.

Replace all occurrences of `BuildSpec.Strategy.Name` with the newly introduced
function to avoid panics.

# Submitter Checklist

- [X] Includes tests if functionality changed/was added
- [ ] Includes docs if changes are user-facing
- [ ] [Set a kind label on this PR](https://prow.k8s.io/command-help#kind)  
- [x] Release notes block has been filled in, or marked NONE

See [the contributor guide](https://github.com/shipwright-io/build/blob/master/CONTRIBUTING.md)
for details on coding conventions, github and prow interactions, and the code review process.

# Release Notes

```release-note
Fix nil-pointer dereference with panic in case Build spec Strategy field was not set.
```



Patch:
diff --git a/pkg/apis/build/v1alpha1/build_types.go b/pkg/apis/build/v1alpha1/build_types.go
index a6b3192a..17b2a3fe 100644
--- a/pkg/apis/build/v1alpha1/build_types.go
+++ b/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.
diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go
index 189cc989..c8235cb1 100644
--- a/pkg/reconciler/buildrun/buildrun.go
+++ b/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),
-						)
-
-					}
 				}
 			}
 


Test patch:
diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go
index 0c0af469..9cf54d44 100644
--- a/pkg/reconciler/buildrun/buildrun_test.go
+++ b/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() {