feat: Add compatibility for ArgoCD 2.4 prefixed environment variables

[edjmao]

Description

ArgoCD 2.4 adds a security fix where all environment variables passed to the repo server during the init and generate phases are prefixed with ARGOCD_ENV to prevent users from setting potentially sensitive environment variables. The argocd-vault-plugin binary needs to read the required values from env vars with the new prefix. This PR adds another flag to set (USE_PREFIX) to switch from reading un-prefixed to reading prefixed env vars.

Checklist

Please make sure that your PR fulfills the following requirements:

• [ X] Reviewed the guidelines for contributing to this repository
• [ ]X The commit message follows the Conventional Commits Guidelines.
• [ X] Tests for the changes have been updated
• Are you adding dependencies? If so, please run go mod tidy -compat=1.17 to ensure only the minimum is pulled in.
• [ X] Docs have been added / updated
• Optional. My organization is added to USERS.md.

Type of Change

• Bugfix
• [ X] Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• New tests
• Build/CI related changes
• [ X] Documentation content changes
• Other (please describe)

Other information

The ArgoCD 2.4 breaking change was mentioned in #352. I tried looking into ways to have the config.go module to read from both the prefixed and unprefixed env vars, but I couldn't get Viper to do that without having to rewrite how those variables read out of the config manager. This was the least intrusive way I could find to make this compatible with 2.4.

I have tested it on our own ArgoCD 2.4 instance with no issues so far.

[CLAassistant]

All committers have signed the CLA.

[werne2j]

Thanks for the PR @edjmao! I would really like to find a way that allows seamless use with 2.4 without introducing a new annotation. Will try to take a look on Monday to maybe come up with some options

[werne2j]

One thing we could do is, in the config, specifically https://github.com/argoproj-labs/argocd-vault-plugin/blob/main/pkg/config/config.go#L271, we could find all env vars with the ARGOCD_ENV prefix and then strip the prefix and set the new var without the prefix.

@jkayani what are your thoughts on this? Or any other ideas?

[edjmao]

I agree that the best is if a seamless transition could occur. This was just my quick attempt to get something working for my team, as we had to upgrade ArgoCD. I'm still trying to figure out how Viper works, and I must have missed that for loop at the bottom. I'm happy to close this PR if a different approach is decided upon.

[werne2j]

@edjmao do you want to try giving this..

One thing we could do is, in the config, specifically https://github.com/argoproj-labs/argocd-vault-plugin/blob/main/pkg/config/config.go#L271, we could find all env vars with the ARGOCD_ENV prefix and then strip the prefix and set the new var without the prefix.

a shot?

[edjmao]

Yeah, I can try this in my spare evenings this week.

[jkayani]

Hi all, sorry for my absence on this discussion - I agree with @werne2j that it'd be best to quietly make things work, rather than having an explicit config option a user would have to set for compatibility with ArgoCD 2.4+ vs earlier releases.

The way Viper is being used right now is that it looks for env variables that match the config key on each Viper lookup (via viper.AutomaticEnv()) - I don't think we'd want to duplicate config keys for Argo CD 2.4 vs earlier and replace each v.Get(thing) with v.Get(thing) or v.Get(ARGOCD_ENV_thing) so we'd actually need to do something like this pseudo code at line 270 https://github.com/argoproj-labs/argocd-vault-plugin/blob/main/pkg/config/config.go#L270:

argocd24 := "ARGOCD_ENV_(.*)"
for k, value := range os.Getenv() {
   if k.Matches(argocd24) {
     configKey := k.Execute(argocd24)[1] // get the capturing group contents as the config key
     v.Set(configKey, value)
   }
}

for k, viperValue := range v.AllSettings() {
   ... rest of code from line 272 onward here

That way any non-AVP specific keys set at the app manifest (ARGOCD_ENV_VAULT_*) are forwarded to the backend SDK and AVP specific keys (ARGOCD_ENV_AVP_KV_VERSION) are read and honored. We can't rely on finding them in viper.AllSettings() without the above because the ARGOCD_ENV keys are only present as environment variables and aren't explicitly loaded into Viper anywhere.

@edjmao Let me know if this makes sense or not. I appreciate the effort you've made so far on this PR, hoping we can add this on and without too much trouble!

[sidewinder12s]

May want to make sure to mention the change in behavior. I hadn't upgraded yet but I was going to need to figure out how to deal with setting AWS_REGION for my setup since that is then used by the AWS SDK/Secrets Manager to talk to AWS.

[jkayani]

Goal is not have any change in behavior and have things work seamlessly regardless of the version of Argo CD being used (set var X in app manifest, and AVP will use it regardless of whether it sees X or ARGOCD_ENV_X) - it's true though that we'll need to update the docs in a few places like here: https://argocd-vault-plugin.readthedocs.io/en/stable/usage/#with-helm

[edjmao]

Found some time to revisit this PR this week. The quickest change I was able to make was to simply replace the lookup of another environment variable with the presence of ARGOCD_ENV_AVP_TYPE, since I assume that every usage of this plugin necessitates this environment variable.

_, prefixFound := os.LookupEnv(fmt.Sprintf("%s_%s", types.EnvArgoCDPrefix, types.EnvAvpType))
if prefixFound {
    v.SetEnvPrefix(types.EnvArgoCDPrefix)
}

I did end up going the route that @jkayani recommended for completeness. It's got more steps, but it's more versatile. I've update the PR with those changes.

	for _, envVar := range os.Environ() {
		if strings.HasPrefix(envVar, types.EnvArgoCDPrefix) {
			envVarPair := strings.SplitN(envVar, "=", 2)
			key := strings.TrimPrefix(envVarPair[0], types.EnvArgoCDPrefix+"_")
			val := envVarPair[1]
			v.Set(key, val)
		}
	}

[werne2j]

@edjmao Can you rebase your branch with main? Thanks!

[jkayani]

Code, tests, docs all look good to me. Thanks!

But, please rebase onto latest main so we can merge

[codecov-commenter]

Codecov Report

Merging #356 (0655f9b) into main (45a4c25) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #356      +/-   ##
==========================================
+ Coverage   77.53%   77.64%   +0.11%     
==========================================
  Files          22       22              
  Lines        1006     1011       +5     
==========================================
+ Hits          780      785       +5     
  Misses        142      142              
  Partials       84       84              

Impacted Files	Coverage Δ
pkg/config/config.go	84.31% <100.00%> (+0.52%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[edjmao]

Rebased from main.

[werne2j]

Thanks @edjmao!