Skip to content

fix(webhook): avoid panics #671

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.

Sign up for GitHub

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

Merged
wass3r merged 4 commits into from
Jul 15, 2022
Merged

fix(webhook): avoid panics #671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
722cbc2
wip - exploring changes to prevent panics
wass3r Jul 11, 2022
4f0b1b2
additional error checking and comments
wass3r Jul 13, 2022
033b5ac
Merge branch 'master' into fix/panics
wass3r Jul 14, 2022
c0f87af
spelling
wass3r Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion api/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1455,21 +1455,36 @@ func getPRNumberFromBuild(b *library.Build) (int, error) {
// planBuild is a helper function to plan the build for
// execution. This creates all resources, like steps
// and services, for the build in the configured backend.
// TODO:
// - return build and error
func planBuild(database database.Service, p *pipeline.Build, b *library.Build, r *library.Repo) error {
// update fields in build object
b.SetCreated(time.Now().UTC().Unix())

// send API call to create the build
// TODO: return created build and error instead of just error
err := database.CreateBuild(b)
if err != nil {
// clean up the objects from the pipeline in the database
// TODO:
// - return build in CreateBuild
// - even if it was created, we need to get the new build id
// otherwise it will be 0, which attempts to INSERT instead
// of UPDATE-ing the existing build - which results in
// a constraint error (repo_id, number)
// - do we want to update the build or just delete it?
cleanBuild(database, b, nil, nil)

return fmt.Errorf("unable to create new build for %s: %w", r.GetFullName(), err)
}

// send API call to capture the created build
b, _ = database.GetBuild(b.GetNumber(), r)
// TODO: this can be dropped once we return
// the created build above
b, err = database.GetBuild(b.GetNumber(), r)
Copy link
Collaborator Author

@wass3r wass3r Jul 14, 2022

Choose a reason for hiding this comment

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

if we don't handle any potential errors here, we will get issues downstream from here

if err != nil {
return fmt.Errorf("unable to get new build for %s: %w", r.GetFullName(), err)
}

// plan all services for the build
services, err := planServices(database, p, b)
Expand Down
53 changes: 38 additions & 15 deletions api/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,14 +402,6 @@ func PostWebhook(c *gin.Context) {
if err != nil {
retErr := fmt.Errorf("%s: unable to get pipeline configuration for %s: %w", baseErr, r.GetFullName(), err)

// check if the retry limit has been exceeded
if i < retryLimit {
logrus.WithError(retErr).Warning("retrying")

// continue to the next iteration of the loop
continue
}

Comment on lines -405 to -412
Copy link
Collaborator Author

@wass3r wass3r Jul 14, 2022

Choose a reason for hiding this comment

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

opted to remove this retry logic since ConfigBackoff is already retrying 5x. if that fails it's likely this is just failing, period.

util.HandleError(c, http.StatusNotFound, retErr)

h.SetStatus(constants.StatusFailure)
Expand All @@ -427,8 +419,8 @@ func PostWebhook(c *gin.Context) {
retErr := fmt.Errorf("%s: unable to get repo %s: %w", baseErr, r.GetFullName(), err)

// check if the retry limit has been exceeded
if i < retryLimit {
logrus.WithError(retErr).Warning("retrying")
if i < retryLimit-1 {
logrus.WithError(retErr).Warningf("retrying #%d", i+1)

// continue to the next iteration of the loop
continue
Expand Down Expand Up @@ -499,6 +491,7 @@ func PostWebhook(c *gin.Context) {

return
}

// reset the pipeline type for the repo
//
// The pipeline type for a repo can change at any time which can break compiling
Expand Down Expand Up @@ -537,8 +530,8 @@ func PostWebhook(c *gin.Context) {
retErr := fmt.Errorf("%s: failed to create pipeline for %s: %w", baseErr, r.GetFullName(), err)

// check if the retry limit has been exceeded
if i < retryLimit {
logrus.WithError(retErr).Warning("retrying")
if i < retryLimit-1 {
logrus.WithError(retErr).Warningf("retrying #%d", i+1)

// continue to the next iteration of the loop
continue
Expand Down Expand Up @@ -569,13 +562,19 @@ func PostWebhook(c *gin.Context) {
b.SetPipelineID(pipeline.GetID())

// create the objects from the pipeline in the database
// TODO:
// - if a build gets created and something else fails midway,
// the next loop will attempt to create the same build,
// using the same Number and thus create a constraint
// conflict; consider deleting the partially created
// build object in the database
err = planBuild(database.FromContext(c), p, b, r)
if err != nil {
retErr := fmt.Errorf("%s: %w", baseErr, err)

// check if the retry limit has been exceeded
if i < retryLimit {
logrus.WithError(retErr).Warning("retrying")
if i < retryLimit-1 {
logrus.WithError(retErr).Warningf("retrying #%d", i+1)

// reset fields set by cleanBuild for retry
b.SetError("")
Expand All @@ -596,7 +595,7 @@ func PostWebhook(c *gin.Context) {

// break the loop because everything was successful
break
}
} // end of retry loop

// send API call to update repo for ensuring counter is incremented
err = database.FromContext(c).UpdateRepo(r)
Expand All @@ -610,6 +609,28 @@ func PostWebhook(c *gin.Context) {
return
}

// return error if pipeline didn't get populated
if p == nil {
retErr := fmt.Errorf("%s: failed to set pipeline for %s: %w", baseErr, r.GetFullName(), err)
util.HandleError(c, http.StatusBadRequest, retErr)

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())

return
}

// return error if build didn't get populated
if b == nil {
retErr := fmt.Errorf("%s: failed to set build for %s: %w", baseErr, r.GetFullName(), err)
util.HandleError(c, http.StatusBadRequest, retErr)

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())

return
}

// send API call to capture the triggered build
b, err = database.FromContext(c).GetBuild(b.GetNumber(), r)
if err != nil {
Expand All @@ -618,6 +639,8 @@ func PostWebhook(c *gin.Context) {

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())

return
Copy link
Collaborator Author

@wass3r wass3r Jul 14, 2022

Choose a reason for hiding this comment

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

we probably don't want to return a build if there was an error. this also creates a conflict because this sets a 500 return code that later gets changed to a 200.

}

// set the BuildID field
Expand Down