Able to apply despite GitHub's PR not being mergeable

[andoriyu]

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 applies change set even when PR has pending CODEOWNERS reviews.

Merge button is read:

Mergeable_status is blocked:

  "author_association": "CONTRIBUTOR",
  "auto_merge": null,
  "active_lock_reason": null,
  "merged": false,
  "mergeable": true,
  "rebaseable": true,
  "mergeable_state": "blocked",
  "merged_by": null,

Reproduction Steps

Open a PR that requires CODEOWNERS approval. Comment atlantis apply Atlantis will apply plan, but fail automerge because its blocked.

Logs

Environment details

If not already included, please provide the following:

• Atlantis version: 0.18.1
• If not running the latest Atlantis version have you tried to reproduce this issue on the latest version: Not yet
• Atlantis flags: atlantis server --config /etc/atlantis/config.yaml

Atlantis server-side config file:

---
repos:
- id: /.*/
  apply_requirements: [mergeable]

Repo atlantis.yaml file:

version: 3
delete_source_branch_on_merge: true
projects:
- dir: some/project
- dir: someother/project

Additional Context

[chenrui333]

@andoriyu is it similar to this issue ?

[andoriyu]

@chenrui333 Hard to tell. According to atlantis documentation, mergeable requirement includes review from CODEOWNERS already.

My issue isn't that I want PR to mergeable only after atlantis did apply, my issue is that atlantis applies unapproved changes.

[chenrui333]

Got you.

[jeff-caylent]

I found that adding the env var of ATLANTIS_REQUIRE_MERGEABLE=true solves the problem for me in 0.18.2, but it does not listen to [mergeable] argument in the server-side configuration YAML.

[andoriyu]

@chenrui333

Noticed another thing, I was able to trigger apply as Org admin that can technically merge anytime by clicking checkbox. A regular contributor wasn't able to apply before approval. It kinda makes sense, but it still goes against what documentation (and code comments) say what should happen - can't apply before button is green.

[gwkunze]

Ran into same issue, we have a GitHub organization with default read permissions, and have a GitHub team with write access in the CODEOWNERS file, when someone - who we thought was in the team but wasn't - approved, it did apply but afterwards automerging failed with:

merging pull request: PUT https://api.github.com/repos/redacted/redacted/pulls/14/merge: 405 At least 1 approving review is required by reviewers with write access. []

[danielgblanco]

I think there's potentially a regression after https://github.com/runatlantis/atlantis/pull/2053

If I understand correctly, the condition in https://github.com/runatlantis/atlantis/blob/76d7080b60b09445c686b35d2697fa9308842a53/server/events/vcs/github_client.go#L319 would apply to a PR that's passed all checks except atlantis/apply and it'd return true for PullIsMergeable even if it hasn't been approved.

Furthermore, even if we have approved in apply_requirements, this takes in any approval not necessarily one by CODEOWNERS. So, in our setup, even with

apply_requirements: [mergeable, approved]

anyone can approve the PR and merge it, as long as the only check missing is atlantis/apply.

[danielgblanco]

I've got a workaround working using a custom apply step (we use terragrunt already so we have it configured elsewhere). We run on GitHub Enterprise hence the /api URL. Our apply steps look like this:

apply:
  steps:
    - env:
        name: TERRAGRUNT_TFPATH
        command: 'echo "terraform\${ATLANTIS_TERRAFORM_VERSION}"'
    - run: 'if curl --silent --location --request POST "https://${ATLANTIS_GH_HOSTNAME}/api/graphql" --header "Authorization: Bearer ${GH_TOKEN}" --data-raw "{\"query\":\"{repository(name:\\\"${BASE_REPO_NAME}\\\", owner:\\\"${BASE_REPO_OWNER}\\\") {pullRequest(number:${PULL_NUM}) {reviewDecision}}}\"}" | grep -q ''{"data":{"repository":{"pullRequest":{"reviewDecision":"APPROVED"}}}}''; then echo "Pull Request approved by CODEOWNERS"; else echo "Pull Request must be approved by CODEOWNERS" && false; fi'
    - run: terragrunt apply -no-color $PLANFILE

This uses the GraphQL API to query for the PR reviewDecision as documented in https://docs.github.com/en/graphql/reference/objects#pullrequest and https://docs.github.com/en/graphql/reference/enums#pullrequestreviewdecision. The PR is not in APPROVED state unless a code owner has reviewed it.

It's definitely a quick workaround and could be improved, but it saves us from having changes applied with any review (not coming from a code owner). I'd like to find some time and add this back into the codebase. Hopefully next week.

[majormoses]

We recently upgraded to 0.19.0 and are confirming this behavior as well. We have reverted to 0.18.5 for the time being. Pushing forward to 0.19.3 still exhibits the behavior.

CVSS Score: 8.0/10

[majormoses]

I reached out as requested to security[AT]runatlantis[DOT]io per https://github.com/runatlantis/atlantis/blob/master/SECURITY.md pointing them to this thread.

[jamengual]

https://github.com/runatlantis/atlantis/pull/2053 was reverted and #1968 was merged on Jan 1.

so I'm correct to think that this was introduced in another PR?

[jamengual]

if someone has time to create a PR or look into where the issue was introduced, please let me know.

[lilincmu]

@andoriyu

We reverted the code that caused the regression. Could you help verify if the issue is resolved in v0.19.7-pre.20220713? Thank you!

[chicocvenancio]

Seems to me it would be better to move PullIsApproved to the graphql endpoint with reviewDecision taking codeowners into account.

[andoriyu]

@lilincmu I will try to find time to test it out next week.

[chicocvenancio]

Seems to me it would be better to move PullIsApproved to the graphql endpoint with reviewDecision taking codeowners into account.

Any chance we could go this route instead of giving up on having a way to enforce users don't merge PRs without applying?

[lilincmu]

@chicocvenancio Unfortunately we had to revert it due to the regression. However, https://github.com/runatlantis/atlantis/pull/2436 is another attempt to add in the feature without breaking existing use cases. It would be highly appreciated if you can share your knowledge there. Hopefully, we can get it right this time 🤞

[jamengual]

is this still happening with v0.19.8?

[majormoses]

I will see if I can test this in our org.

[Roberdvs]

This seems to have been fixed for us. Previously it was possible to run atlantis apply with anyone's approval on a PR even if CODEOWNERS approval was required in the branch protection but now Atlantis properly prevents the apply from executing.

Info from our environment in case it helps anyone:

• Updated Atlantis to v0.20.1
• Atlantis server side config with apply_requirements: [approved, mergeable]
• GitHub branch protection with Require review from Code Owners enabled.
• GitHub branch protection with Require status checks to pass before merging including atlantis/plan and atlantis/apply status checks.
  • ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY set to true in order to work with the atlantis/apply status check.
• Updated GitHub app permissions to include Administration -> read as commented here
  • Otherwise Atlantis fails with "unable to get pull request status: fetching mergeability status for repo: omitted, and pull number: omitted: getting pull request status: getting required status checks: GET https://api.github.com/repos/omitted/omitted/branches/main/protection: 403 Resource not accessible by integration []

[nitrocode]

@Roberdvs

Make sure to change ATLANTIS_GH_MERGEABLE_BYPASS_APPLY to ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY

ref: https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply

[Roberdvs]

@Roberdvs

Make sure to change ATLANTIS_GH_MERGEABLE_BYPASS_APPLY to ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY

ref: runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply

I actually realized earlier and made this other comment about it and still pasted here the wrong name 🤦🏻‍♂️ . Edited now, thanks! 😄

[nitrocode]

I'm closing this for now since this seems fixed with the new flag mentioned above. Please use that flag with the latest version to see if it works as expected.