feat: prevent planning and applying directories outside PR scope using --restrict-file-list
#2440
Conversation
jamengual
added
waiting-on-review
|
@Fabianoshz can you post an example of a user story? |
|
@jamengual I've reproduced the behavior in this pull request. The terragrunt code itself it's not working but it's possible to see that another directory unrelated to the PR can be planned. The expected is to:
But currently is possible to:
If the plan with |
|
is this a monorepo case?
…On Thu., Aug. 11, 2022, 6:31 p.m. Fabiano Soares Honorato, < ***@***.***> wrote:
@jamengual <https://github.com/jamengual> I've reproduced the behavior in this
pull request <Fabianoshz/atlantis-test#3>. The
terragrunt code itself it's not working, but it's possible to see that
another directory unrelated to the PR can be planned.
The expected is to:
- Open a PR
- Plan
- Apply the planned resources
But currently is possible to:
- Open a PR
- Run atlantis plan -d another-dir-not-related-to-pr
- Then run atlantis apply -d another-dir-not-related-to-pr
If the plan with -d fails the pipeline status will stay failed until a
new commit is added to solve it.
[image: image]
<https://user-images.githubusercontent.com/14815404/184268549-0a51a7a3-0fe2-4654-84c2-dd139a8aa810.png>
—
Reply to this email directly, view it on GitHub
<#2440 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ3ERGQLYB7ZGQPI7GAWXDVYWSQRANCNFSM56J2WIEQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
In this case yes, to explain further, different teams working on the same repository might try to plan and apply resources that don't belong to them. |
|
The problem we see here is that nothing stops a user to add an empty line to a dir outside the scope of the PR and I do not think this change will make a difference. maybe https://www.runatlantis.io/docs/policy-checking.html#how-it-works could help here but not sure. Atlantis was not designed for monorepo in mind but many people use it but it need ingenuity in some cases |
|
Yes, it's possible to open a PR with an empty line but if that's the case we can prevent that user from applying by enforcing a code owner of that directory to approve the PR. In the case described it's possible to bypass the code owners by simply opening a PR which the user should be able to change and then planing and applying a directory outside of the user normal boundaries after the code owners approval. By allowing this breach the user may also create an unrecoverable failed step if the plan/apply fails which will require a new commit adding the directory that the user should have never changed in the first place. |
|
Thanks @Fabianoshz we will try to look into your PR soon |
|
@Fabianoshz thank you for adding this PR. I added a few comments. Please review and resolve conflicts. |
nitrocode
changed the title
|
Hi @nitrocode, I'll take a look at this this week. |
|
@Fabianoshz fixed conflicts and addressed feedback |
|
@Fabianoshz friendly ping |
|
@Fabianoshz fixed tests by running |
nitrocode
added
waiting-on-response
|
Sorry for my absence, I've had some personal issues. Came back today and managed to add the logic to projects too but I'm still not satisfied with the code yet. |
|
@nitrocode @jamengual how should I approach writing a test for this? Looking at the code here the values are defined for all the cases but since this is a server side flag I believe it should have 1 or 2 cases instead of changing for everyone. Maybe because this is false by default we don't test? I don't know the policy for this. |
yes, do no need to change for everyone, just add 1 or 2 more functions. |
|
Ok, I'll try to add the tests tomorrow. |
Fabianoshz
requested review from
jamengual and
nitrocode
and removed request for
jamengual and
nitrocode
|
Tests added, I believe this should be good to go, sorry this took more time than I expected. |
|
Thank you @Fabianoshz for the tests. It's looking very good. Just had another comment and want to get some more eyes on this. |
|
Thank you again @Fabianoshz . I added 2 last comments regarding the documentation. Please address and we can get this merged. |
nitrocode
changed the title
--restrict-file-list
|
Thanks @Fabianoshz for all your efforts here! We really appreciate your work. Let us know if you want to contribute more. By the way, we're also looking for more maintainers if you're interested, please message us on slack. |
|
Hey @nitrocode, I sure want to contribute more, I'll reach out on slack! |
…g `--restrict-file-list` (runatlantis#2440) * Add --strict-plan-file-list config * Update server.go * Update server-configuration.md * Update server_test.go * Run gofmt -w * Add --strict-plan-file-list for projects * Add --strict-plan-file-list tests * Change --strict-plan-file-list to --restrict-file-list * Update --restrict-file-list documentation * Update --restrict-file-list and --enable-regexp-cmd documentation Co-authored-by: Fabiano Honorato <[email protected]> Co-authored-by: nitrocode <[email protected]> Co-authored-by: PePe Amengual <[email protected]>
Right now it's possible to plan directories outside the scope of the PR changed list using
-d. This blocks that behavior if the server is configured with the--restrict-file-list.This closes #1508.