From b36254c50bb55ffba4a413dfe72160ab60ed7cba Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Thu, 1 Apr 2021 16:03:22 +0200 Subject: [PATCH] Fix nil-pointer dereference of BuildSpec Strategy 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. --- pkg/apis/build/v1alpha1/build_types.go | 10 +++ pkg/reconciler/buildrun/buildrun.go | 80 ++++++++++++------------ pkg/reconciler/buildrun/buildrun_test.go | 14 +++++ 3 files changed, 65 insertions(+), 39 deletions(-) diff --git a/pkg/apis/build/v1alpha1/build_types.go b/pkg/apis/build/v1alpha1/build_types.go index a6b3192a3c..17b2a3fe08 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 189cc9890a..c8235cb19b 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), - ) - - } } } diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go index 0c0af4695b..9cf54d4413 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() {