--gh-allow-mergeable-bypass-apply flag will panic if no required checks on GH branch protection settings

[nikolaik]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
• If you are interested in working on this issue or have submitted a pull request, please leave a comment.

---

Overview of the Issue

Atlantis v0.20.1 panicked when running atlantis apply on a GitHub PR configured with AllowMergeableBypassApply turned on.

runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:260 (0x44c995)
runtime/signal_unix.go:835 (0x44c965)
github.com/runatlantis/atlantis/server/events/vcs/github_client.go:329 (0xb8b7cb)
github.com/runatlantis/atlantis/server/events/vcs/github_client.go:398 (0xb8bd97)
github.com/runatlantis/atlantis/server/events/vcs/instrumented_client.go:179 (0xb9278b)
github.com/runatlantis/atlantis/server/events/vcs/proxy.go:72 (0xb94184)
github.com/runatlantis/atlantis/server/events/vcs/pull_status_fetcher.go:28 (0xb94a05)
github.com/runatlantis/atlantis/server/events/apply_command_runner.go:108 (0xd74415)
github.com/runatlantis/atlantis/server/events/command_runner.go:296 (0xd79023)
runtime/asm_amd64.s:1594 (0x467ca0)

Doing another atlantis apply a bit later ran successfully. Could something in the responses from github in Repositories.GetBranchProtection or Checks.ListCheckSuitesForRef be nil in some cases?

Reproduction Steps

• Turn on AllowMergeableBypassApply
• Create a PR in a repo with other checks
• Comment with atlantis plan
• Comment with atlantis apply
• Roll dice and observe if you get the panic or not.

[nitrocode]

For what it's worth, I'm running with the same --allow-mergeable-bypass-apply flag with github and without hitting the error somehow.

Perhaps there is an err that needs to be checked? Or just like you said, perhaps something is returning nil and we're trying to call a function off a nil reference?

Here is the full stack trace if you or others would like to contribute.

atlantis/server/events/vcs/github_client.go

Lines 307 to 310 in 9cfcd13

	required, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref)
	if err != nil {
	return false, errors.Wrap(err, "getting required status checks")
	}

atlantis/server/events/vcs/github_client.go

Lines 327 to 329 in 9cfcd13

	for _, r := range suite.CheckRuns {
	//check to see if the check is required
	if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) {

atlantis/server/events/vcs/github_client.go

Lines 394 to 398 in 9cfcd13

	if g.config.AllowMergeableBypassApply {
	g.logger.Debug("AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements")
	if state == "blocked" {
	//check status excluding atlantis apply
	status, err := g.GetCombinedStatusMinusApply(repo, githubPR, vcsstatusname)

atlantis/server/events/vcs/instrumented_client.go

Line 179 in 9cfcd13

mergeable, err := c.Client.PullIsMergeable(repo, pull, vcsstatusname)

atlantis/server/events/vcs/proxy.go

Line 72 in 9cfcd13

return d.clients[repo.VCSHost.Type].PullIsMergeable(repo, pull, vcsstatusname)

atlantis/server/events/vcs/pull_status_fetcher.go

Line 28 in 9cfcd13

mergeable, err := f.client.PullIsMergeable(repo, pull, vcsstatusname)

atlantis/server/events/apply_command_runner.go

Line 108 in 9cfcd13

ctx.PullRequestStatus, err = a.pullReqStatusFetcher.FetchPullStatus(baseRepo, pull, a.VCSStatusName)

atlantis/server/events/command_runner.go

Line 296 in 9cfcd13

cmdRunner.Run(ctx, cmd)

[nitrocode]

This was the PR #2470 that implemented this.

cc: @stasostrovskyi in case you might be able to see a quick solution for this

[stasostrovskyi]

@nikolaik what are the branch protection rules you have that trigger this error? Technically, both Repositories.GetBranchProtection and Checks.ListCheckSuitesForRef can be nil, but the whole feature kind of implies that a) there is branch protection setup b) there is at least one required check - atlantis/apply.

We are also running this version for a pretty long time without issues, so would be really interesting to know your setup

[stasostrovskyi]

My extremely quick analysis tells me that one possible reason for panic would be to have no required checks in branch protection. But as I said above, this feature expects at least one to exist, otherwise you don't need to use it

[nikolaik]

My extremely quick analysis tells me that one possible reason for panic would be to have no required checks in branch protection. But as I said above, this feature expects at least one to exist, otherwise you don't need to use it

@stasostrovskyi That is probably it! We have a generic setup consumed by a range of repos, and some will be in a state without required checks.

In this case, when no checks are required, I guess it would make sense to allow merging then? just an early return in GetCombinedStatusMinusApply?

[stasostrovskyi]

Well, probably the logic in this case should be something like that:

If there is no atlantis/apply required check in the repo then we should just ask GitHub if PR is mergeable (as if this feature doesn't exist).

Unless someone wants to jump on it, I will try to find some time next week to fix it.

[nitrocode]

There is a GetRequiredStatusChecks where we can check if it returns something.

https://github.com/google/go-github/blob/db32fc6d79d6d282b09849020650928e8b16f24b/github/repos.go#L1274

 requiredChecks, _, err := required.GetRequiredStatusChecks(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref)

[nitrocode]

Friendly ping @stasostrovskyi

[stasostrovskyi]

Yeah, sorry, life happened :) I've tried to reproduce this issue, both with tests and in the wild and wasn't able to.. @nikolaik can you screenshot/list your protected branches? Is it consistent per repo or happens randomly on the same PR/repo?

[nitrocode]

cc @nikolaik friendly ping

[nikolaik]

Sorry for not replying sooner! I have not seen this lately, but will try to make reproduction and report back

[nikolaik]

Here is a screenshot of it happening again with version 0.23.2

With the checks:

[jamengual]

I wonder if this is related to : #2979

[nitrocode]

I don't believe it's related to that since PR 2979 was merged in Jan 12 2023 and this issue was created Nov 11 2022.

I think this issue stems from assuming there is at least 1 required check when the flag is enabled, instead of checking to see if there are required checks. It's probably an easy fix. I think @nikolaik was reproducing it in order to supply a fix.

[stasostrovskyi]

@nikolaik So it happens during the plan now? Can you please add a screenshot of your branch protection settings? I wasn't able to reproduce it myself by just removing all required checks, so there must be something else in play.. Also, does it happen constantly on this PR/repo or does it work fine eventually?

[nikolaik]

@stasostrovskyi I enabled the single non-atlantis check to be a required and it started working

The Require status checks to pass before merging flag was off completely. Could that be it?

[nikolaik]

Just confirmed on two disctinct repos that checking the Require status checks to pass before merging flag works around the panic

[tpolekhin]

My guess would be that this line

atlantis/server/events/vcs/github_client.go

Line 431 in 7a33828

if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) {

just assumes that if branch is protected then it must have required checks set.
I think trying to access .Contexts on empty object could result in invalid memory address panic

BTW, looks like the Contexts being deprecated
https://pkg.go.dev/github.com/google/go-github/v50@v50.1.0/github#RequiredStatusChecks