Add new "mergeable" apply requirement

[atlantisbot]

Issue by @lkysow
Thursday Nov 30, 2017 at 06:54 GMT
Migrated from hootsuite/atlantis#210
Why was it migrated?

---

GitHub has lots of branch protections that we could support in Atlantis by requiring them to "pass" before we allow apply's. Right now you can specify --require-approval but this only looks for an approval, not necessarily the type of approval specified in the branch protections.

• The API for branch protections is here: https://developer.github.com/v3/repos/branches/
• We'd have to decide which ones to support (might be all of them)
• The benefit here would be that there's more configuration available around who can apply

[atlantisbot]

Comment by @mechastorm
Wednesday Jan 10, 2018 at 21:10 GMT

---

This will be especially useful for scenarios where we want to allow direct applies for lower environments but still want strict approvals for production environments.

[atlantisbot]

Comment by @matthiasr
Friday Feb 16, 2018 at 09:09 GMT

---

As an alternative to explicitly supporting (and tracking) all the possible permutations of settings, could the condition simply be "branch can be merged"? That way GitHub will do the hard work and the status will be obvious from the PR itself.

[atlantisbot]

Comment by @lkysow
Sunday Feb 18, 2018 at 06:57 GMT

---

@matthiasr thank you! That's a great idea. I just tested it out and the field mergeable_state is what we need to check if the protected branch restrictions are passing. It looks like it gets set to blocked if they're not passing and clean if they are.

The only tricky thing is that some people were asking me to have Atlantis set a status on the pull request to "not passing" until any pending apply's are applied. This would prevent the PR from being merged until the apply is complete. This would need to be configurable of course.

If people wanted both, we'd have to do something like checking if all the statuses were passing except for our special "pending apply's" status and then enable the apply.

[atlantisbot]

Comment by @grobie
Tuesday Feb 27, 2018 at 16:31 GMT

---

From a security perspective I don't that proposed shortcut makes sense. An attacker gaining access to a Github account with admin capabilities could simply turn off all the branch protection features and the PR state will become passing.

IMO Atlantis should verify that all the branch protection features are enabled before running apply.

[atlantisbot]

Comment by @matthiasr
Tuesday Feb 27, 2018 at 16:32 GMT

---

How does Atlantis know which features are desired/required to be enabled?

[atlantisbot]

Comment by @matthiasr
Tuesday Feb 27, 2018 at 16:34 GMT

---

(My reading of the issue was that this is about checking which branch protections are set in GitHub and honoring them, not about enforcing that certain protections are set)

[atlantisbot]

Comment by @grobie
Tuesday Feb 27, 2018 at 16:40 GMT

---

In order to be able to enforce that e.g. a change gets approval from a second person before being applied, these two things can't be separated. Atlantis would need to be configured (via flags or protected atlantis.yml) to verify that both certain protection settings are set and fulfilled. Otherwise a single compromised account (or evil employee) could simply disable all checks and cause Atlantis to apply changes.

[atlantisbot]

Comment by @majormoses
Tuesday Feb 27, 2018 at 17:03 GMT

---

@grobie true it does pose a risk and I agree we could make it harder for an attacker but at the end of the day if someone is an admin in github they probably also have access to the other repo that would generate the server side atlantis.yaml. That being said I generally see security in forms of layers and I think we should support all the branch protection options from github and allow defining server side if you want say 2 approvals before atlantis will apply. This is the best of both worlds.

[mwarkentin]

Github has support for requiring multiple approvals now (but doesn't change what most of this discussion is about).

[sstarcher]

We we prefer to implement this by adding a new flag or by modifying the existing logic of --require-approval

[majormoses]

I can see it both ways, technically I think they should be two separate options as there are other branch protections in play such as repo not being up to date, signed commits, etc. Only downside I can see of this is if someone specified say --no-require-approval and --require-mergable on the same line but we could throw a useful validation error. This also maintains backwards compatibility. Just my $0.02

[sstarcher]

Another question arrises as this implementation is github specific if someone specified --require-mergable would we throw an error on any non-github VCS or would we ignore the setting for those VCS's?

[majormoses]

From what I recall gitlab also has it, I could be wrong though.

[brndnmtthws]

If anyone wants to test #385 and provide any feedback, that would be great :)