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

0.17.5 removes all mergeability checks from apply, even review requirements. #1895

Closed
grimm26 opened this issue Nov 11, 2021 · 13 comments · Fixed by #1976
Closed

0.17.5 removes all mergeability checks from apply, even review requirements. #1895

grimm26 opened this issue Nov 11, 2021 · 13 comments · Fixed by #1976
Labels
bug Something isn't working

Comments

@grimm26
Copy link
Contributor

grimm26 commented Nov 11, 2021

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

#1856 causes atlantis to completely ignore mergeability on PRs to the extent that required reviews are ignored.

Reproduction Steps

Use atlantis 0.17.5 with github

  apply_requirements: [mergeable]

Have a CODEOWNERS setup or something that requires reviews on PRs.
Open a PR with a successful plan and a required review. Run atlantis apply and it will happily do the apply without the required review.

Logs

{
    "level": "info",
    "ts": "2021-11-11T15:50:23.820Z",
    "caller": "events/apply_command_runner.go:110",
    "msg": "pull request mergeable status: true",
    "json": {
        "repo": "terraforms/production_8787878787",
        "pull": "6835"
    }
}

Environment details

Additional Context

Everything was working fine with 0.17.4. When I upgraded to 0.17.5, I added a branch protection requirement for atlantis/apply on my terraform repos. Now anyone can apply anything.

@grimm26 grimm26 added the bug Something isn't working label Nov 11, 2021
@grimm26
Copy link
Contributor Author

grimm26 commented Nov 11, 2021

nevermind, I forgot to add approved to apply_requirements. There is a problem with that, but that's a separate issue.

@grimm26 grimm26 closed this as completed Nov 11, 2021
@grimm26
Copy link
Contributor Author

grimm26 commented Nov 11, 2021

sorry, this is valid. It broke running with apply_requirements: [mergeable] on github where mergable with branch protection requires approvals. Adding approved is not an option because it is satisfied with only 1 approval no matter what the requirements of the branch protection are.

@grimm26 grimm26 reopened this Nov 11, 2021
@mralanlee
Copy link

sorry, this is valid. It broke running with apply_requirements: [mergeable] on github where mergable with branch protection requires approvals. Adding approved is not an option because it is satisfied with only 1 approval no matter what the requirements of the branch protection are.

Is Atlantis given permissions with admin abilities? If so, a PR is deemed mergeable for administrators even without codeowners approval.

I am using v0.17.5 where Atlantis does not have admin permissions, and I have mergeable set for apply_requirements.

The error message isn't properly reflecting it:

Ran Apply for dir: . workspace: default
Apply Failed: Pull request must be approved by at least one person other than the author before running apply.

In this scenario, we have one approval on the pull request but it's missing an approval from the codeowner.

@grimm26
Copy link
Contributor Author

grimm26 commented Nov 18, 2021

sorry, this is valid. It broke running with apply_requirements: [mergeable] on github where mergable with branch protection requires approvals. Adding approved is not an option because it is satisfied with only 1 approval no matter what the requirements of the branch protection are.

Is Atlantis given permissions with admin abilities? If so, a PR is deemed mergeable for administrators even without codeowners approval.

the github user that atlantis is an admin, but that has not changed.

I am using v0.17.5 where Atlantis does not have admin permissions, and I have mergeable set for apply_requirements.

What permissions does your atlantis user have?

The error message isn't properly reflecting it:

Ran Apply for dir: . workspace: default
Apply Failed: Pull request must be approved by at least one person other than the author before running apply.

In this scenario, we have one approval on the pull request but it's missing an approval from the codeowner.

the approved apply requirement is insuffcient and we do not use it because it only cares about if there is 1 approval, no matter what CODEOWNERS says.

@packrat386
Copy link

So I believe the problem is this PR https://github.com/runatlantis/atlantis/pull/1856/files which changed

https://github.com/runatlantis/atlantis/blob/v0.17.4/server/events/vcs/github_client.go#L282-L284

to

https://github.com/runatlantis/atlantis/blob/v0.17.5/server/events/vcs/github_client.go#L286-L294

The problem is that the new check implicitly assumes that the only reason a pull request can be in a "blocked" state is because of a failing status as checked by GetCombinedStatus (https://github.com/runatlantis/atlantis/blob/v0.17.5/server/events/vcs/github_client.go#L368), but that only matters for external checks that apply a status to a ref. There are other things that can cause a PR to be unmergeable in a "blocked" state, notably lacking a required review.

It seems like this is hard to generally solve without totally reimplementing GitHub's logic for mergeability inside of atlantis. To solve this particular case you'd need to call the API to see if the branch is protected and has required reviews, and then if it does cross reference the reviews on the pull request to see if they satisfy the constraint. Even if you did that it wouldn't solve all the possible things GitHub protected branches can require (https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches).

@grimm26
Copy link
Contributor Author

grimm26 commented Nov 23, 2021

@packrat386 thanks for the research. It seems like #1856 needs to be backed out if we want the mergeable check to mean anything.

@georgekaz
Copy link

I've just seen this behaviour. A PR that requires a codeowner review was approved by a non codeowner (therefore not green) but he was then able to apply the change.

Nothing has changed in our setup other than the upgrade last week to 0.17.5 and this has never happened before. I've nothing to add, just confirming with a screenshot. I think this should be resolved as a matter of high priority. Thanks

atlantisbug

@grimm26 grimm26 changed the title 0.17.5 removes all mergability checks from apply, even review requirements. 0.17.5 removes all mergeability checks from apply, even review requirements. Nov 24, 2021
@xarses
Copy link

xarses commented Dec 2, 2021

I've also run into this problem, and we reverted back to the prior version as this is a required part of our workflow. I would also propose reverting #1856 until a workable solution is found

@mubarak-j
Copy link

@nishkrishnan any thoughts on @packrat386's findings here? I'm afraid this bug will block many from upgrading to newer releases.

@grimm26
Copy link
Contributor Author

grimm26 commented Dec 30, 2021

Another release gone by with this breaking change in place. Looking for some visibility on this @chenrui333 @lkysow @nishkrishnan

@georgekaz
Copy link

I agree, unfortunately whatever other improvements are made, there's no way we can move forward from 17.4 with a bug that allows non approved PRs to be applied. This is a critical bug and it should be highlighted so that anyone who is unaware of it can make an informed choice about the version they are running. Is there any reason this has not simply been rolled back?

@nishkrishnan
Copy link
Contributor

So I believe the problem is this PR https://github.com/runatlantis/atlantis/pull/1856/files which changed

https://github.com/runatlantis/atlantis/blob/v0.17.4/server/events/vcs/github_client.go#L282-L284

to

https://github.com/runatlantis/atlantis/blob/v0.17.5/server/events/vcs/github_client.go#L286-L294

The problem is that the new check implicitly assumes that the only reason a pull request can be in a "blocked" state is because of a failing status as checked by GetCombinedStatus (https://github.com/runatlantis/atlantis/blob/v0.17.5/server/events/vcs/github_client.go#L368), but that only matters for external checks that apply a status to a ref. There are other things that can cause a PR to be unmergeable in a "blocked" state, notably lacking a required review.

This is correct, for our usecase we never saw this issue surface since we only use external checks.

It seems like this is hard to generally solve without totally reimplementing GitHub's logic for mergeability inside of atlantis. To solve this particular case you'd need to call the API to see if the branch is protected and has required reviews, and then if it does cross reference the reviews on the pull request to see if they satisfy the constraint. Even if you did that it wouldn't solve all the possible things GitHub protected branches can require (https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches).

I agree with this. We have some plans in the pipeline to play a bit nicer with githubs merge process and apply changes after a PR is merged. None of this will matter at that point. But for now, I will revert this commit.

@nishkrishnan
Copy link
Contributor

#1968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants