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

apply_requirements not being evaluated #2605

Open
theautoroboto opened this issue Oct 20, 2022 · 12 comments · May be fixed by #3830
Open

apply_requirements not being evaluated #2605

theautoroboto opened this issue Oct 20, 2022 · 12 comments · May be fixed by #3830
Labels
bug Something isn't working help wanted Good feature for contributors

Comments

@theautoroboto
Copy link

theautoroboto commented Oct 20, 2022

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

Using GitLab Community Edition [15.2.3] with Atlantis deployed on kubernetes using a statefulset, the apply_requirements settings defined in the environment variable ATLANTIS_REPO_CONFIG_JSON are not being evaluated.

Reproduction Steps

Deploy to kubernetes as a statefulset and define the environment variable. PRs are never checked for approval before allowing an apply comment to be posted and the atlantis apply process to be completed

- name: ATLANTIS_REPO_CONFIG_JSON
  value: '{"repos":[{"id":"/.*/", "apply_requirements":["approved"]}]}'

I have verified that the environment variable is present on the running pod

ATLANTIS_REPO_CONFIG_JSON={"repos":[{"id":"/.*/", "apply_requirements":["approved"]}]}

Logs

"level":"info","ts":"2022-10-13T13:51:14.477Z","caller":"events/instrumented_project_command_runner.go:53","msg":"plan success. output available at: https://xxxx/bsmith/runatlantis-terraform/-/merge_requests/12","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:08.158Z","caller":"events/events_controller.go:533","msg":"parsed comment as command=\"apply\" verbose=false dir=\"\" workspace=\"\" project=\"\" flags=\"\"","json":{}}
{"level":"info","ts":"2022-10-13T13:55:08.614Z","caller":"events/project_command_context_builder.go:313","msg":"detected module requires version: \"0.14.7\"","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:08.694Z","caller":"runtime/apply_step_runner.go:39","msg":"starting apply","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:55.302Z","caller":"models/shell_command_runner.go:156","msg":"successfully ran \"/atlantis/bin/terraform0.14.7 apply -input=false \\\"/atlantis/repos/bsmith/runatlantis-terraform/12/default/demo/default.tfplan\\\"\" in \"/atlantis/repos/bsmith/runatlantis-terraform/12/default/demo\"","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:55.302Z","caller":"runtime/apply_step_runner.go:58","msg":"apply successful, deleting planfile","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:55.386Z","caller":"events/instrumented_project_command_runner.go:53","msg":"apply success. output available at: https://xxx/bsmith/runatlantis-terraform/-/merge_requests/12","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:55.636Z","caller":"events/automerger.go:32","msg":"automerging pull request","json":{"repo":"bsmith/runatlantis-terraform","pull":"12"}}
{"level":"info","ts":"2022-10-13T13:55:56.269Z","caller":"events/events_controller.go:588","msg":"identified event as type \"closed\"","json":{}}
{"level":"info","ts":"2022-10-13T13:55:56.269Z","caller":"events/instrumented_pull_closed_executor.go:40","msg":"Initiating cleanup of pull data.","json":{"repository":"bsmith/runatlantis-terraform","pull-num":"12"}}
{"level":"info","ts":"2022-10-13T13:55:56.362Z","caller":"events/events_controller.go:461","msg":"deleted locks and workspace for repo bsmith/runatlantis-terraform, pull 12","json":{}}

If not already included, please provide the following:

  • Atlantis version: atlantis 0.20.1
@theautoroboto theautoroboto added the bug Something isn't working label Oct 20, 2022
@theautoroboto
Copy link
Author

Could this limitation be caused by the version of GitLab we are running? It appears that required approvals are only available on GitLab Premium and GitLab Ultimate, while we are running Community Edition.
https://docs.gitlab.com/ee/user/project/merge_requests/approvals/index.html

@jamengual jamengual added the help wanted Good feature for contributors label Oct 20, 2022
@TravisSRE
Copy link

TravisSRE commented Oct 27, 2022

Our company has Gitlab Enterprise and we've noticed a similar issue as well @theautoroboto. Atlantis applies even though there's things that need to be rebased. The (mergeable) apply_requirement is being ignored. We didn't have this issue until recently. It's important to note that we recently updated to a 15.x Gitlab version.

@aandre-wise
Copy link

Hello,
I have the same problem with apply_requirements that are not evaluated.
I am using the aws terraform module which runs Atlantis on fargate (version 3.25.0).
I am using GitLab enterprise edition 15.7.0-pre.
Did you find solution to solve this problem?

@nitrocode
Copy link
Member

Has this always been an issue with atlantis? Do older versions work as expected? If not, then this may not be a regression and may be related to the gitlab enterprise api change.

Either way, we welcome contributions. If you folks can spot the issue, please feel free to build locally, test it out, and propose a PR.

@remiflament
Copy link

Hello 👋🏻 I got the same issue on Gitlab SaaS.
I tried 2 ways to add my server-side config using the flag --repo-config or --repo-config-json
This is not working, I can comment an apply, and it's done without the approval protection.

It's important to notice that I successfully set up an approval protection with the deprecated flag --require-approval
But the problem is that this flag fixed the "approved" requirements on plan,apply and import. I only want to configure "approved" on apply

My env :
Atlantis latest 0.24.3
GitLab Enterprise 16.2.0
Atlantis installed with the binary on EC2
Permission for Atlantis : group access token with "developer" access

My config :
server configuration

atlantis-url: 'https://atlantis.xxxxxx.io/'
gitlab-token: 'xxxxxxxx'
gitlab-user: 'atlantis'
gitlab-webhook-secret: 'xxxxxxxx'
log-level: 'info'
repo-allowlist: 'gitlab.com/xxxx/yyyy/*,gitlab.com/xxxxx/zzzzz/aaaaa/*'
repo-config-json: '{"repos":[{"apply_requirements":["approved"],"id":"/.*/","repo_locking":true}]}'
web-basic-auth: 'true'
web-password: 'xxxxxxxxxxxx'
web-username: 'skello'
write-git-creds: 'true'

service side config

repos:
- id: /.*/
  repo_locking: true
  apply_requirements: [approved]

@Angelin01
Copy link

I tried the deprecated --require-approval flag with Atlantis v0.24.4 and Gitlab v16.4.1 and it seemed to have no effect.

@jukie , I believe you mentioned that you'd volunteer as a Gitlab maintainer, this issue is basically a complete blocker to us adopting Atlantis, do you have some idea on what could be causing this? I'm not familiar with Go or Atlantis' code base at all, but if I get a few pointers I'm willing to dig into what's causing this issue.

@jamengual
Copy link
Contributor

@lukemassa do you have any ideas on this? I do not know if you have touched that part of the code

@lukemassa
Copy link
Contributor

lukemassa commented Oct 6, 2023

(Note I deleted my earlier comment because it was slightly wrong and confusing)

I did some testing and I learned this has to do with Merge Request Approval Rules (https://docs.gitlab.com/ee/user/project/merge_requests/approvals/rules.html). I'm running atlantis 0.24.4 and gitlab enterprise 16.4.1.

- apply_requirements: [approved] apply_requirements: []
Project has MR Approval Rule Fails to apply Applies
Project does not have MR Approval Rule Applies Applies

So my current understanding is that you need to have both apply_requirement: [approved] and an approval rule on the project in order for atlantis to refuse to apply an unapproved MR.

By my read, this behavior is contrary to the documentation https://www.runatlantis.io/docs/command-requirements.html#approved. However I'm not sure the best thing to do here. I think the options are:

  1. Update the documentation to say "approved respects the approval rules of the project" (which I think is what's going on)
  2. Update the code to respect the approval rule if it exists, otherwise still require an approval
  3. Update the code to ignore the approval rules and require approval if apply_requirements says approved.

My personal vote is 1 (in fact I've already unknowingly pushed us in that direction: #3744). But I'd like to understand what other VCSs do. Thoughts @jamengual?

In the meantime, @Angelin01 (or others) can you try setting up a Merge Request Approval Rule (https://docs.gitlab.com/ee/user/project/merge_requests/approvals/rules.html) on your project to see if that blocks unapproved applies? I would expect if both these are set to get an comment back from atlantis that says something like "Apply Failed: Pull request must be approved by at least one person other than the author before running apply."

@jamengual
Copy link
Contributor

I agree with @lukemassa here, if you have apply_requirements is becaude you want to have approval rules, therefore, Atlantis should not try to interpret the desired outcome.

I vote for number 1.

@lukemassa
Copy link
Contributor

@jamengual I've thought about it some more, and I actually think the right solution here is to have approved mean "someone other than the author has approved the PR" across all VCSs, and incorporate "PR meets its approval requirements, whatever they are" as part of mergeable.

This would consist in:

  1. Revert fix: Change message for when request is not approved to be more accurate #3744
  2. Change PullIsApproved logic in for gitlab to be true if there is at least one approval other than author
  3. Move logic for "meets approval requirements" of gitlab into PullIsMergeable
  4. Update documentation to clarify that approved is a consistent notion across VCSs, whereas mergeable is much more complex and deferred to the VCS itself.
  5. See if we can get someone who uses bitbucket or azure to update PullIsMergeable to incorporate approval logic (it's possible they already do I don't know enough about these VCSs to even know what the analogy of an "approval" is).

Looking at Bitbucket and Azure, both attempt to do what github does, which is say a thing is "approved" if someone other than the author has "approved" it, and false otherwise, so I figured it's better to change gitlab to be me more in line with the others, than change the notion of "approved" across the board?

@lukemassa lukemassa linked a pull request Oct 7, 2023 that will close this issue
@jamengual
Copy link
Contributor

Ahhh that is an interesting point and from the user's point of view it does make sense to think that approved means approved other than the author which is the correct assumption for all VCSs.

I'm all for consistency so I agree with this change, it does make sense.

@GenPage any opinion on this?

@lukemassa
Copy link
Contributor

lukemassa commented Oct 7, 2023

I implemented the above in #3830 if you want to take a look. It turns out no change was needed for part 3) since PullIsMergeable already implemented "passing merge request approval rules" for gitlab. Part 5) I'd be happy to look into as a followup, but I don't think that needs to be a blocker here.

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

Successfully merging a pull request may close this issue.

8 participants