Skip to content

Commit

Permalink
fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty
Browse files Browse the repository at this point in the history
In this particular example `mr.HeadPipeline.SHA` panics on a nil pointer dereference because HeadPipeline is empty.

This seems to be caused by the lack of permission to update the commit status.

```go
runtime.gopanic
        runtime/panic.go:1038
runtime.panicmem
        runtime/panic.go:221
runtime.sigpanic
        runtime/signal_unix.go:735
github.com/runatlantis/atlantis/server/events/vcs.(*GitlabClient).PullIsMergeable
        github.com/runatlantis/atlantis/server/events/vcs/gitlab_client.go:208
github.com/runatlantis/atlantis/server/events/vcs.(*ClientProxy).PullIsMergeable
        github.com/runatlantis/atlantis/server/events/vcs/proxy.go:72
github.com/runatlantis/atlantis/server/events/vcs.(*pullReqStatusFetcher).FetchPullStatus
        github.com/runatlantis/atlantis/server/events/vcs/pull_status_fetcher.go:28
github.com/runatlantis/atlantis/server/events.(*ApplyCommandRunner).Run
        github.com/runatlantis/atlantis/server/events/apply_command_runner.go:105
github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand
        github.com/runatlantis/atlantis/server/events/command_runner.go:252
```

The least invasive solution is to simply use the commit-hash from pull and guess that the pipeline was "skipped" unless the HeadPipeline is there.

The outcome is:

When mr.HeadPipeline is present:
- use the commit hash and status from the HeadPipeline
When mr.HeadPipeline is NOT present:
- use the commit hash from pull request struct
- assume the pipeline was "skipped"

In cases where GitLab is configured to require a pipeline to pass, this results on a message saying the MR is not mergeable.

More info:
- runatlantis#1852
  • Loading branch information
marceloboeira committed Aug 4, 2023
1 parent fbb12fe commit 1bc01d3
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 4 deletions.
2 changes: 1 addition & 1 deletion scripts/e2e-deps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ unzip terraform_${TERRAFORM_VERSION}_linux_amd64.zip
chmod +x terraform
cp terraform /home/circleci/go/bin
# Download ngrok to create a tunnel to expose atlantis server
wget https://bin.equinox.io/c/bNyj1mQVY4c/ngrok-v2-stable-linux-amd64.zip -O ngrok-stable-linux-amd64.zip
wget https://bin.equinox.io/c/bNyj1mQVY4c/ngrok-v2-stable-linux-amd64.zip -O ngrok-stable-linux-amd64.zip
unzip ngrok-stable-linux-amd64.zip
chmod +x ngrok
# Download jq
Expand Down
12 changes: 10 additions & 2 deletions server/events/vcs/gitlab_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,23 @@ func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
return false, err
}

// Prevent nil pointer error when mr.HeadPipeline is empty
// See: https://github.com/runatlantis/atlantis/issues/1852
commit := pull.HeadCommit
isPipelineSkipped := true
if mr.HeadPipeline != nil {
commit = mr.HeadPipeline.SHA
isPipelineSkipped = mr.HeadPipeline.Status == "skipped"
}

// Get project configuration
project, _, err := g.Client.Projects.GetProject(mr.ProjectID, nil)
if err != nil {
return false, err
}

// Get Commit Statuses
statuses, _, err := g.Client.Commits.GetCommitStatuses(mr.ProjectID, mr.HeadPipeline.SHA, nil)
statuses, _, err := g.Client.Commits.GetCommitStatuses(mr.ProjectID, commit, nil)
if err != nil {
return false, err
}
Expand All @@ -302,7 +311,6 @@ func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
}
}

isPipelineSkipped := mr.HeadPipeline.Status == "skipped"
allowSkippedPipeline := project.AllowMergeOnSkippedPipeline && isPipelineSkipped

ok, err := g.SupportsDetailedMergeStatus()
Expand Down
29 changes: 28 additions & 1 deletion server/events/vcs/gitlab_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,40 +323,62 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) {
gitlabVersionUnder15_6 := "15.3.2-ce"
gitlabServerVersions := []string{gitlabVersionOver15_6, gitlabVersion15_6, gitlabVersionUnder15_6}
vcsStatusName := "atlantis-test"
defaultMR = 1
noHeadPipelineMR = 2
cases := []struct {
statusName string
status models.CommitStatus
gitlabVersion []string
mrId int
expState bool
}{
{
fmt.Sprintf("%s/apply: resource/default", vcsStatusName),
models.FailedCommitStatus,
gitlabServerVersions,
defaultMr,
true,
},
{
fmt.Sprintf("%s/apply", vcsStatusName),
models.FailedCommitStatus,
gitlabServerVersions,
defaultMr,
true,
},
{
fmt.Sprintf("%s/plan: resource/default", vcsStatusName),
models.FailedCommitStatus,
gitlabServerVersions,
defaultMr,
false,
},
{
fmt.Sprintf("%s/plan", vcsStatusName),
models.PendingCommitStatus,
gitlabServerVersions,
defaultMr,
false,
},
{
fmt.Sprintf("%s/plan", vcsStatusName),
models.PendingCommitStatus,
gitlabServerVersions,
noHeadPipelineMR,
false,
},
{
fmt.Sprintf("%s/plan", vcsStatusName),
models.FailedCommitStatus,
gitlabServerVersions,
noHeadPipelineMR,
false,
},
{
fmt.Sprintf("%s/plan", vcsStatusName),
models.SuccessCommitStatus,
gitlabServerVersions,
noHeadPipelineMR,
true,
},
}
Expand All @@ -372,6 +394,9 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) {
case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/1":
w.WriteHeader(http.StatusOK)
w.Write([]byte(pipelineSuccess)) // nolint: errcheck
case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/2":
w.WriteHeader(http.StatusOK)
w.Write([]byte(pipelineNotAvailable)) // nolint: errcheck
case "/api/v4/projects/4580910":
w.WriteHeader(http.StatusOK)
w.Write([]byte(projectSuccess)) // nolint: errcheck
Expand Down Expand Up @@ -409,11 +434,13 @@ func TestGitlabClient_PullIsMergeable(t *testing.T) {
Hostname: "gitlab.com",
},
}

mergeable, err := client.PullIsMergeable(repo, models.PullRequest{
Num: 1,
Num: c.mrId,
BaseRepo: repo,
HeadCommit: "sha",
}, vcsStatusName)

Ok(t, err)
Equals(t, c.expState, mergeable)
})
Expand Down

0 comments on commit 1bc01d3

Please sign in to comment.