feat: MultiEnv step added

[tapaszto]

MultiEnv step is new workflow step, a combination of RunStep and EnvStep, executes a command returning an array of name-value pairs, in case of success these pairs are getting added as environment variables before running plan and apply otherwise the errorMessage is getting displayed in the output.
It allows adding dynamic number of envionment variables depending on the command execution result.

The result of executed command must have a fixed format:
{ "success":true, "errorMessage":null, "result": [ {"name":"TF_VAR_VARIABLE_ONE","value":"value1"}, {"name":"TF_VAR_VARIABLE_TWO","value":"value2"}, {"name":"TF_VAR_VARIABLE_THREE","value":"value3"} ] }

Using in workflow definition:
- multienv: bash -c /home/tapaist/atlantisbuild/config/multienv.sh

Our use case that requires this new feature:
Atlantis is being used by several teams within our company. The teams are not allowed to override the global workflow defined in the server config as it contains specific checks and other special items that are required for our Atlantis instance to run e.g. defining the required Azure credentials which is always tied to the selected repository-Azure subscription. The various teams often have the requirement to be able to define additional environment variables, mostly secrets that need to be used for Terraform apply. These secrets are team dependent and must be isolated, one team must not be able to use the secrets of an other team. Overriding the workflow in repo config to have custom team dependent env steps is not permitted and we cannot have dozens of team/repo dependent workflows on server side with continuously increasing count. We need to have one single point in the workflow config on server side where the dynamic number of team/repo specific environment variables with dynamic name/value pairs can be injected safely from client side. However we've got the env step but it can only define one single environment variable, and also the name of it is hardwired in the config.

The solution:
Due to the new multienv step multiple environment variables can be added dynamically just before executing Terraform commands via Atlantis. We specifically use this step in Atlantis workflow to call a Linux script hosted on Atlantis server, the script reads the reference to an Azure key vault from a file with specific name KeyVault.ref committed to the repo and having authorization reads the secrets from the key vault. In our case the Linux script is nothing more than a bridge to call further functionality and the committed extra file in the repo does not contain any sensitive info but a name of a key vault. All the sensitive info comes from a safe key vault on-the-fly and the result is passed to Atlantis workflow on server side as environment variables similarly to the original env workflow step. By this method the different teams can define one single KeyVault.ref file per repo, they enlist the required environment variable name-value pairs in the key vault, grant the repo dependent Azure service principal the required permission to read the key vault and Atlantis injects the secrets (as environment variables) that can be referenced in the Terraform scripts.

[tapaszto]

Hi @acastle,

Can You review this please?

[jamengual]

Thanks @tapaszto for your contribution!

few comments:
New features need documentation
Tests are required too

and can you please expand on the problem this functionality is trying to solve?

Thanks.

[tapaszto]

Hi @jamengual / @nishkrishnan,

I added the requested automated test and updated the documentation and also expanded the PR with the details of our use case. Please check my updated opening comment on the top.

[nishkrishnan]

Sorry for getting to this super late, my overall thoughts on this:

Instead of having another run step (multienv) can we just make a breaking change to the env step so it'd look like:

env:
  # this is the command you're implementing and by having under run, we can also have a file option as well.
  run: some_command.sh
  # legacy implementation
  vars:
    - name: <name>
       value: <value>

We would then basically just release a new major version of Atlantis at this point.

Another thing I wanted to bring out was the format of the resulting env vars. I think in general having json be a requirement of the run command can be annoying for users that just want to write simple scripts. Would like us to just simplify here and have a simple multi-line env key pair output.. I don't think we need other fields such as success and error message as we'll just base errors off non-zero exit codes like the rest of the run commands in atlantis.

so the output of the command could look like:

ENV_VAR1=VALUE1
ENV_VAR2=VALUE2

where name and value are separated by "=" (standard env var definition) and key pairs are separated by new lines.

What do you think?

[tapaszto]

Sorry for getting to this super late, my overall thoughts on this:

Instead of having another run step (multienv) can we just make a breaking change to the env step so it'd look like:

env:
  # this is the command you're implementing and by having under run, we can also have a file option as well.
  run: some_command.sh
  # legacy implementation
  vars:
    - name: <name>
       value: <value>

We would then basically just release a new major version of Atlantis at this point.

Another thing I wanted to bring out was the format of the resulting env vars. I think in general having json be a requirement of the run command can be annoying for users that just want to write simple scripts. Would like us to just simplify here and have a simple multi-line env key pair output.. I don't think we need other fields such as success and error message as we'll just base errors off non-zero exit codes like the rest of the run commands in atlantis.

so the output of the command could look like:

ENV_VAR1=VALUE1
ENV_VAR2=VALUE2

where name and value are separated by "=" (standard env var definition) and key pairs are separated by new lines.

What do you think?

We are concerned that your proposal for modifying the existing env step with the breaking change is too complex for us and considering the limited resources we have, we are advising a simpler implementation which fulfils the requirement to avoid creating a new run step (multienv step) but does not require any breaking change in the implementation: the original env step has got name, value and command attributes, let the env step be as it is now + in case of the name and value equal to a reserved constant e.g. name == "dynamic" && value == "dynamic" the step would call the command attribute and add the resulted variable number of environment variables to the workflow and would work as we previously implemented the multienv step. By this we can fulfil your request to avoid having a new run step but we would not have a breaking chance. We are capable to do either this or our original implementation with the new multienv step. What is your view regarding our suggestion?

[Dilergore]

Hi @nishkrishnan,

Could you please take a look at the proposal above? We need this change in our environment.

Thanks!

[tapaszto]

Hi @nishkrishnan , @lkysow, @acastle ,

Guys, can you please answer the proposal above? We really need this feature.

[tapaszto]

Hi,

Can you check the proposal above please?

[jamengual]

@tapaszto Hi, we talked about your feature and usually we try to merge features that are popular and requested by people and it seems that you guys are the only ones doing this way, maybe? if you could get some votes and maybe other people to comment we could potentially merge this feature. Thanks.

[Dilergore]

@jamengual There was a feature request earlier here - Multiple people are seeking this funtionality:
#1769

Theoretically I could start to ask our entire firm to start to comment here I do not know what change that will make. We are trying to add here a feature which could be used in many ways and would make Atlantis more flexible.

TBH We contributed already and planned to contribute more but this is really not encouraging us to do so. We do similar activities for other open source projects and the way I see Atlantis is the most problematic and the slowest one.

[atakacs90]

Would like to use this feature as well.

[szferle]

It would be useful for us as well

[hd214]

I need it to.

[apoorvabelsare]

This feature will be very useful for us.

[jamengual]

@jamengual There was a feature request earlier here - Multiple people are seeking this funtionality: #1769

Theoretically I could start to ask our entire firm to start to comment here I do not know what change that will make. We are trying to add here a feature which could be used in many ways and would make Atlantis more flexible.

TBH We contributed already and planned to contribute more but this is really not encouraging us to do so. We do similar activities for other open source projects and the way I see Atlantis is the most problematic and the slowest one.

I'm sorry that you feel not encouraged to contribute, we are just 5 people with full-time jobs trying to help the project to the best of our abilities so time is very restrictive right now.

As you might know from your experience with other projects there is always a popularity aspect to the new features so that is why I ask about the 👍🏽 , how those happen is not important.

There are a few comments on this PR that need to be addressed still so I hope once those are resolved we can have another review and see if we can move forward.

Thanks.
@tapaszto

[Dilergore]

Hi @jamengual,

Have you had the meeting with the team?

[jamengual]

Yes sir.

In about a week I will have more details but I think is looking good, one thing we did talk about was the output format, I think it will be better if the output is like this:

KEY=val
KEY2=val2
```​
instead of being opinionated and using json, it is simple to JSON encode the output if the user needs it.

Thanks.

[jamengual]

@tapaszto do you think you can change the output to key value of ENV variables instead of a json output?

then we can merge this.

Thanks.

[tapaszto]

Hi @jamengual,

The error message is also very important for us as the called web service performs several checks and passing the error to the end users is essential, I added a very detailed comment 2 moths ago, just to summarize we need to handled cases such as:

1. The committed key vault file contains invalid reference
2. The key vault does not exist
3. The team specific Azure deployment user does not have the proper permission to access the key vault
4. Missing permission to read the secrets from the key vault
5. Atlantis workflow is not able to call the web api, maybe network issue

Can we add a special ErrorMessage key as well for error handling purposes? This would not be a valid envvar. Or can you propose an alternative how to pass error message from the multienv script to Atlantis to get it displayed for the end user?

[jamengual]

Hi @tapaszto

I think that should be added to another PR to be discussed, I definitely do not recommend adding functionality to this PR otherwise we will need to review it again and it is just never going to get merged.

If you can address the comment I made earlier about the json output then we can work on merging this and you can open a PR for the error handling after.

Thanks.

[jamengual]

@tapaszto we will like to merge and release this on the prerelease image we are preparing, do you think you will have time to make those changes?

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[jamengual]

Does anyone else want to take this and make the changes?

[tapaszto]

Hi @jamengual,

Sorry, I was assigned to other projects I will complete this

[tapaszto]

Hi @jamengual,

I performed the requested modifications. The website_link_check fails but I think this is not related to my changes.
Can you merge the PR please?

[jamengual]

We will do one last review and we will then merge it to be released on the pre-release then we will leave the pre-release available for 2-4 weeks and if we do not get any issues reported we will release it in the base release.

[jamengual]

https://github.com/runatlantis/atlantis/releases/tag/v0.19.4-pre.20220513

[Jonnymcc]

I've tried this out and even though multienv is the first step in my workflow it appears to run last. I'm trying to export a number of TF_VAR_ variables and in the plan step Terraform (w/ Terragrunt) says that they are undefined.