Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/nil pointer reference in updateBuildRunUsingTaskRunCondition #592

Merged
merged 2 commits into from
Feb 16, 2021
Merged

Fix/nil pointer reference in updateBuildRunUsingTaskRunCondition #592

merged 2 commits into from
Feb 16, 2021

Conversation

HeavyWombat
Copy link
Contributor

Changes

There seems to be a rather rare case where we have a setup in which a TaskRun
has a completion timestamp and condition reason TaskRunReasonFailed, while
the respective pod has containers which are not terminated, (yet). In this
case the code ends up with a nil pointer dereferences and crashes the running
operator pod. See #591 for details.

Fixes #591

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Fixed an issue where the build-controller fails to Reconcile with a panic due to a nil pointer dereference. The issue can arise in case there is a failed TaskRun with a pod that has still running (unfinished) containers.

Run `gofmt -s -w` to fix issue with different formatting.
There seems to be a rather rare case where we have a setup in which a TaskRun
has a completion timestamp and condition reason `TaskRunReasonFailed`, while
the respective pod has containers which are not terminated, (yet). In this
case the code ends up with a nil pointer dereferences and crashes the running
operator pod. See #591 for details.

Change `updateBuildRunUsingTaskRunCondition` function to set the failed
container name only if a failed container could be looked-up.

Add unit-test to construct a represenation of the error scenario and test that
the `Reconcile` runs through without a nil pointer dereference.
@openshift-ci-robot openshift-ci-robot added the release-note Label for when a PR has specified a release note label Feb 16, 2021
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @HeavyWombat

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2021
@openshift-merge-robot openshift-merge-robot merged commit 121b16a into shipwright-io:master Feb 16, 2021
@HeavyWombat HeavyWombat deleted the fix/nil-pointer-reference branch February 16, 2021 16:33
@gabemontero
Copy link
Member

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: nil pointer dereference in updateBuildRunUsingTaskRunCondition
5 participants