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: GitHub mergeability bypassing apply #4193

Conversation

henriklundstrom
Copy link
Contributor

@henriklundstrom henriklundstrom commented Jan 31, 2024

what

First of all, this PR is only concerned with how Atlantis determines if it should proceed with apply when the --gh-allow-mergeable-bypass-apply is used on a GitHub PR that is in the blocked state because Atlantis apply is a required check.

The following changes to its behaviour are made:

This is all accomplished by using the GraphQL API instead of the REST API, because the former conveniently provides the current state of all statuses and checks on the PR, including if they are required or not.

This PR does not solve the issue #4116, but it does open up for the possibility of implementing a solution. The implementation in this PR does list all required checks from rulesets and branch protection, and could easily be extended to also get the conditions for bypassing in the same query. However, there's a philosophical question as to who should be regarded as the user in such case, the Atlantis GitHub App, the PR author, or the author of the atlantis apply comment? For this reason I will leave it to a future PR to deal with bypassers.

In an ideal world, the GitHub API would provide some sort of is-mergeable-for-given-actor status on PRs, which would take into account all required checks/statuses and bypass rules. Then Atlantis could use only that to determine if it should proceed with apply+merge. But there is no such thing available. So we are stuck with this approach of listing all required checks and checking their status.

Do note that there are more reasons why a PR may not be mergeable than the status of checks and reviews, which Atlantis ignores, and which are not addressed by this PR. For example, a ruleset may define requirements on the author's email, among many other things...

Another thing to note i that as of this change, the Atlantis GitHub credential needs read access to Actions, in order to fetch any workflow information about a given check. This was not needed before, so should be mentioned in any eventual release notes.

why

The previous implementation had the following flaws:

  • All statuses were treated as required
    • Atlantis may refuse to apply and merge a PR even if only non-required statuses are unsuccessful
  • All uncompleted checks were ignored, even if they are required
  • Only branch protection was used to determine if a check is required, rulesets were ignored
    • Atlantis may apply when it ought not to
    • Atlantis may try to merge a PR but find itself unable to do so

tests

Updated test cases that run under make test.

references

@henriklundstrom henriklundstrom requested review from a team as code owners January 31, 2024 15:58
@henriklundstrom henriklundstrom requested review from jamengual, lukemassa and nitrocode and removed request for a team January 31, 2024 15:58
@github-actions github-actions bot added go Pull requests that update Go code provider/github labels Jan 31, 2024
@henriklundstrom henriklundstrom changed the title fix: GitHub merge ability bypassing apply fix: GitHub mergeability bypassing apply Jan 31, 2024
@GenPage GenPage added the waiting-on-review Waiting for a review from a maintainer label Jan 31, 2024
@jamengual
Copy link
Contributor

Thanks, @henriklundstrom. We will find some time to review, but it could take a while.
We are int he process of deciding on ADRs for statuses, locks and such

@jamengual jamengual added the needs discussion Large change that needs review from community/maintainers label Feb 7, 2024
@IchordeDionysos
Copy link

Awesome work 😍 This is exactly the issue we are running into now!
Is there anything we can help with to get it merged? 😌

@varsis
Copy link

varsis commented Apr 26, 2024

+1 We ran into the same issue the other day.

@secustor
Copy link
Contributor

secustor commented May 3, 2024

Looking forward to this PR as it should enable implementing MergeQueue with required checks pretty easily.

@henriklundstrom
Copy link
Contributor Author

If you want to try this out, I made a release on the feature-branch, here: https://github.com/nordnet/atlantis/releases/tag/v0.27.4-pre.fix-required-checks-minus-apply-20240515.

You can pull that Docker image: ghcr.io/nordnet/atlantis:v0.27.4-pre.fix-required-checks-minus-apply-20240515-alpine.

@stasostrovskyi
Copy link
Contributor

@jamengual @lukemassa Do you think you will be able to check this PR any time soon? We have been running it in our prod for several months, and everything works well.

@raxod502-plaid
Copy link
Contributor

@jamengual @lukemassa Would you be able to take a look at this PR, please? #3811 is a considerable security hazard because a malicious actor could use it to bypass any number of automated checks, and this PR would solve it.

@jamengual
Copy link
Contributor

Sorry, guys, this is taking so long, but we are at the mercy of time right now, and we have had very little of it lately.
We know this is important and we will try to make some time for this.

@lukemassa
Copy link
Contributor

I was on vacation last week, diving back in this week. I will take a look at this today or tomorrow.

Copy link
Contributor

@lukemassa lukemassa left a comment

Choose a reason for hiding this comment

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

@henriklundstrom makes a lot of sense, and very thoroughly done, thanks for the contribution!

@jamengual jamengual merged commit ed7b744 into runatlantis:main Sep 5, 2024
28 checks passed
@dimisjim
Copy link
Contributor

dimisjim commented Oct 9, 2024

I am still experiencing with v0.30.0 the same issue:

  1. atlantis/apply is chosen as a required check in the gh protected main branch
  2. ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true

but when trying to apply the already approved PR, we get Failure running apply operation: Pull request must be approved according to the project's approval rules before running apply.

@stasostrovskyi
Copy link
Contributor

There can be many possible reasons for it. Can you see if there are any errors in the log?

@Roberdvs
Copy link

Roberdvs commented Oct 9, 2024

I am still experiencing with v0.30.0 the same issue:

1. atlantis/apply is chosen as a required check in the gh protected main branch

2. `ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true`

but when trying to apply the already approved PR, we get Failure running apply operation: Pull request must be approved according to the project's approval rules before running apply.

Make sure to add the new required Actions:Read-Only required permission to the Atlantis GitHub app when you upgrade to 0.30. We had the same happen to us when upgrading from 0.29 and came across this when reviewing the changelog.

@dimisjim
Copy link
Contributor

dimisjim commented Oct 9, 2024

Error thrown was:

"msg":"unable to get pull request status: fetching mergeability status for repo: <redacted>, and pull number: 395: getting pull request status: fetching rulesets, branch protections and status checks from GraphQL: Resource not accessible by integration. Continuing with mergeable and approved assumed false",

@Roberdvs The error above could be indicating the missing permission, thanks!

EDIT: Indeed, the missing permission in the GH app did the trick, and now it works as expected 💯

a1k0u pushed a commit to a1k0u/atlantis that referenced this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code needs discussion Large change that needs review from community/maintainers provider/github waiting-on-review Waiting for a review from a maintainer
Projects
None yet