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

feat: implement plan-all command #2694

Closed
wants to merge 1 commit into from

Conversation

secustor
Copy link
Contributor

Closes #254

This PR implement a plan-all command which will trigger plans for all defined projects regardless if they have been modified or not.

E2E tested on this repo https://github.com/secustor/renovate_terraform_lock_in_subdirectory/pull/12

@secustor secustor requested a review from a team as a code owner November 18, 2022 22:14
@jamengual
Copy link
Contributor

jamengual commented Nov 18, 2022

We were talking about this earlier and this could potentially be pretty dangerous if a person is allowed to plan on all projects and some show secrets on the plan, and I think if you plan all projects you can then run one apply and will apply all by default, no?

@jamengual jamengual added waiting-on-review Waiting for a review from a maintainer feature New functionality/enhancement labels Nov 18, 2022
@nitrocode
Copy link
Member

@secustor thank you for your hardwork. At the very least, could you add a new flag to enable this and set that flag to false as this is a bit of a security risk. As pepe already mentioned, we're already dealing with an issue #1508 and we'd like to reduce that blast radius. I'd be more inclined to approving if there is a flag to gate this.

@@ -608,6 +608,160 @@ projects:
}
}

func TestDefaultProjectCommandBuilder_BuildPlanAllCommands(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test! Could you also create a separate test for the new function shouldUpdateRepo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer scope tests this already, but I will add additional tests to cover this.

@nitrocode nitrocode added waiting-on-response Waiting for a response from the user and removed waiting-on-review Waiting for a review from a maintainer labels Nov 19, 2022
@secustor
Copy link
Contributor Author

We were talking about this earlier and this could potentially be pretty dangerous if a person is allowed to plan on all projects and some show secrets on the plan, and I think if you plan all projects you can then run one apply and will apply all by default, no?

I'm new to the code base and usage of Atlantis, but how would this expose more things than opening a PR with changes to the atlantis.yaml or adding changes to files of a project.
Yes, it will then apply all. This actual my use case, as I want to trigger a plan for all projects to test Terraform version and provider upgrades.

@secustor thank you for your hardwork. At the very least, could you add a new flag to enable this and set that flag to false as this is a bit of a security risk. As pepe already mentioned, we're already dealing with an issue #1508 and we'd like to reduce that blast radius. I'd be more inclined to approving if there is a flag to gate this.

I would prefer not to hide this behind a flag, but would this PR ( #2696 ) clear up your reservations if it is merged? That way users/admins can set apply_requirements and prevent unreviewed changes to be merged/applied.

@nitrocode
Copy link
Member

Usually we gate new features behind a feature flag until it's been tested enough. Some users may like this but others may not and it would be good to be able to enable it with a default of disabled.

Eventually if most people enable it then we can discuss removing the need for a flag. I do not think PR #2696 would relieve my reservations but I'm open to changing my mind.

cc: @runatlantis/maintainers thoughts?

@jamengual
Copy link
Contributor

I agree with @nitrocode, this can be enabled by default, and it should be enabled by default and enabled explicitly.
For many people like me, the fact that a user can plan any project is considered a security risk.

@nitrocode
Copy link
Member

nitrocode commented Nov 20, 2022

@jamengual mentioned that there is also this flag --enable-regexp-cmd (created by pr #1419)

E.g.

atlantis plan -p .*

The regexp command may be sufficient enough to not need a plan-all command. @secustor could you see if this meets your needs?

@nitrocode
Copy link
Member

I think the feature we'd want is to expand on the previous pr #1419 which implemented the flag and wildcard for -p and allow it to work with -d as well so one doesn't have to define all the directories to run the plan.

Ref #259

@secustor
Copy link
Contributor Author

From what I can deduct from the docs this should work. I will try to confirm this on our instance and close this if it solves my problem.

@nitrocode nitrocode marked this pull request as draft November 24, 2022 14:49
@nitrocode nitrocode added the needs discussion Large change that needs review from community/maintainers label Nov 24, 2022
@nitrocode
Copy link
Member

I converted this to a draft for now. Please let us know if the --enable-regexp-cmd flag works for you or if you want to repurpose this to allow -d to work with a regex

@secustor
Copy link
Contributor Author

secustor commented Jan 9, 2023

The --enable-regexp-cmd flag has solved my use case.

@secustor secustor closed this Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement needs discussion Large change that needs review from community/maintainers waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Suggestion - Allow atlantis plan for all defined projects regardless of files changed
3 participants