fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

marceloboeira · 2023-05-22T08:42:56Z

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.

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:

runtime error: invalid memory address or nil pointer dereference #1852

what

why

tests

references

nitrocode · 2023-05-22T12:29:20Z

server/events/vcs/gitlab_client.go

@@ -198,14 +198,23 @@ func (g *GitlabClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
 		return false, err
 	}

+	// Prevent nil pointer error when mr.HeadPipeline is empty


Please add a test for this

The test is at

atlantis/server/events/vcs/gitlab_client_test.go

Line 316 in a31d3ab

func TestGitlabClient_PullIsMergeable(t *testing.T) {

. Feel free to ping me directly if you need assistance building the test.

I just haven't got the time yet. I'm gonna try doing this over the long weekend here and get back to you

@GenPage finally got the time to work on this, the tests are here, could you review them?

nitrocode · 2023-05-22T12:29:44Z

Please fill out the pr template

nitrocode · 2023-05-23T14:17:47Z

cc: @jamengual thoughts on this pr?

jamengual · 2023-05-23T15:35:26Z

cc: @jamengual thoughts on this pr?

we have been going back and forth about this in the issues and slack and I think we agreed on merging this since gitlab have no ETA on fixing the api.

github-actions · 2023-07-02T02:11:54Z

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

camaeel · 2023-07-24T11:40:09Z

Any chance for having this merged and released soon?

GenPage · 2023-07-24T17:58:16Z

@marceloboeira friendly ping, were you able to write a test for this?

jamengual · 2023-07-24T18:17:26Z

Anyone can write the test, if anyone is able to do so, that works too.

GenPage · 2023-07-28T21:05:27Z

I can attempt to write the test next week.

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

marceloboeira · 2023-08-04T14:28:06Z

Folks, sorry for the delay, finally, the tests are here 🎈

GenPage · 2023-08-04T17:00:43Z

Thank you for the contribution, LGTM

By fixing nil pointer reference in 3428 we unveiled yet another bug now that the code is able to run further: When an MR with no HeadPipeline exists its commits are set to skipped by default and thus create some mess when running "PullMergeable" (rendering the MR useless) The bug was caught by the tests, however, in an attempt to make the build pass and unblock we create a patch to ignore that test. In order to prevent further problems, this MR: * Updates the test stack to run all plan/apply commands in the context of "HeadLess" Pipelines * Fixes the default of skipped pipeline to false (as it is better to assume it is NOT skipped since that prevents the merge in most cases) * Make all integratiion tests pass References: * Original MR: runatlantis#3428 * Tests Patch MR : runatlantis#3653

#3695) By fixing nil pointer reference in 3428 we unveiled yet another bug now that the code is able to run further: When an MR with no HeadPipeline exists its commits are set to skipped by default and thus create some mess when running "PullMergeable" (rendering the MR useless) The bug was caught by the tests, however, in an attempt to make the build pass and unblock we create a patch to ignore that test. In order to prevent further problems, this MR: * Updates the test stack to run all plan/apply commands in the context of "HeadLess" Pipelines * Fixes the default of skipped pipeline to false (as it is better to assume it is NOT skipped since that prevents the merge in most cases) * Make all integratiion tests pass References: * Original MR: #3428 * Tests Patch MR : #3653

…ty (runatlantis#3428) 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

runatlantis#3695) By fixing nil pointer reference in 3428 we unveiled yet another bug now that the code is able to run further: When an MR with no HeadPipeline exists its commits are set to skipped by default and thus create some mess when running "PullMergeable" (rendering the MR useless) The bug was caught by the tests, however, in an attempt to make the build pass and unblock we create a patch to ignore that test. In order to prevent further problems, this MR: * Updates the test stack to run all plan/apply commands in the context of "HeadLess" Pipelines * Fixes the default of skipped pipeline to false (as it is better to assume it is NOT skipped since that prevents the merge in most cases) * Make all integratiion tests pass References: * Original MR: runatlantis#3428 * Tests Patch MR : runatlantis#3653

…ty (runatlantis#3428) 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

runatlantis#3695) By fixing nil pointer reference in 3428 we unveiled yet another bug now that the code is able to run further: When an MR with no HeadPipeline exists its commits are set to skipped by default and thus create some mess when running "PullMergeable" (rendering the MR useless) The bug was caught by the tests, however, in an attempt to make the build pass and unblock we create a patch to ignore that test. In order to prevent further problems, this MR: * Updates the test stack to run all plan/apply commands in the context of "HeadLess" Pipelines * Fixes the default of skipped pipeline to false (as it is better to assume it is NOT skipped since that prevents the merge in most cases) * Make all integratiion tests pass References: * Original MR: runatlantis#3428 * Tests Patch MR : runatlantis#3653

marceloboeira requested a review from a team as a code owner May 22, 2023 08:42

github-actions bot added go Pull requests that update Go code provider/gitlab labels May 22, 2023

nitrocode reviewed May 22, 2023

View reviewed changes

nitrocode added this to the v0.24.0 milestone May 22, 2023

nitrocode added the needs tests Change requires tests label May 22, 2023

nitrocode modified the milestones: v0.24.0, v0.25.0 May 22, 2023

GenPage added the waiting-on-response Waiting for a response from the user label May 26, 2023

nitrocode mentioned this pull request Jun 13, 2023

runtime error: invalid memory address or nil pointer dereference #1852

Closed

github-actions bot added the Stale label Jul 2, 2023

github-actions bot removed the Stale label Jul 25, 2023

marceloboeira force-pushed the issue-1852 branch 4 times, most recently from 1bc01d3 to 87cd1ba Compare August 4, 2023 13:43

marceloboeira force-pushed the issue-1852 branch from 87cd1ba to bdcc10c Compare August 4, 2023 13:44

marceloboeira requested review from GenPage and nitrocode August 4, 2023 14:36

GenPage approved these changes Aug 4, 2023

View reviewed changes

GenPage merged commit 89a028f into runatlantis:main Aug 4, 2023

marceloboeira deleted the issue-1852 branch August 6, 2023 09:19

ghaiszaher mentioned this pull request Aug 6, 2023

fix: gitlab client failing test #3653

Merged

jamengual mentioned this pull request Aug 14, 2023

Panic encountered in the version 0.24 #3671

Closed

ghaiszaher mentioned this pull request Aug 16, 2023

ci: use dorny/paths-filter to filter paths when running go-tests #3675

Merged

marceloboeira mentioned this pull request Aug 22, 2023

fix(gitlab): Prevent considering non-head pipelines skipped by default #3695

Merged

pbobba-ocrolus mentioned this pull request Apr 24, 2024

Atlantis is unable to download the latest stable terraform version #4473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

marceloboeira commented May 22, 2023 •

edited by nitrocode

Loading

nitrocode May 22, 2023

GenPage May 25, 2023

marceloboeira May 26, 2023

marceloboeira Aug 4, 2023

nitrocode commented May 22, 2023

nitrocode commented May 23, 2023

jamengual commented May 23, 2023 •

edited

Loading

github-actions bot commented Jul 2, 2023

camaeel commented Jul 24, 2023

GenPage commented Jul 24, 2023

jamengual commented Jul 24, 2023

GenPage commented Jul 28, 2023

marceloboeira commented Aug 4, 2023

GenPage commented Aug 4, 2023

fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

fix(gitlab): Prevent nil pointer dereference when HeadPipeline is empty #3428

Conversation

marceloboeira commented May 22, 2023 • edited by nitrocode Loading

what

why

tests

references

nitrocode May 22, 2023

Choose a reason for hiding this comment

GenPage May 25, 2023

Choose a reason for hiding this comment

marceloboeira May 26, 2023

Choose a reason for hiding this comment

marceloboeira Aug 4, 2023

Choose a reason for hiding this comment

nitrocode commented May 22, 2023

nitrocode commented May 23, 2023

jamengual commented May 23, 2023 • edited Loading

github-actions bot commented Jul 2, 2023

camaeel commented Jul 24, 2023

GenPage commented Jul 24, 2023

jamengual commented Jul 24, 2023

GenPage commented Jul 28, 2023

marceloboeira commented Aug 4, 2023

GenPage commented Aug 4, 2023

marceloboeira commented May 22, 2023 •

edited by nitrocode

Loading

jamengual commented May 23, 2023 •

edited

Loading