fix: side-effect on r.SecretsFiles when having multiple successive call

[rasta-rocket]

Hey folks 👋

I'm using helm-secrets with the vault driver and I'm facing that issue:

HELM_SECRETS_DRIVER=vault helmsman -f state_v3.toml -target foo -show-diff -debug

 _          _ 
| |        | | 
| |__   ___| |_ __ ___  ___ _ __ ___   __ _ _ __
| '_ \ / _ \ | '_ ` _ \/ __| '_ ` _ \ / _` | '_ \ 
| | | |  __/ | | | | | \__ \ | | | | | (_| | | | | 
|_| |_|\___|_|_| |_| |_|___/_| |_| |_|\__,_|_| |_| version: v3.6.3
A Helm-Charts-as-Code tool.

2021-01-19 23:35:05 DEBUG: helm version --short -c
2021-01-19 23:35:05 DEBUG: helm version --short -c
2021-01-19 23:35:05 DEBUG: kubectl version --client --short
2021-01-19 23:35:05 DEBUG: helm plugin list
2021-01-19 23:35:05 INFO: Parsed TOML [[ state_v3.toml ]] successfully and found [ X ] apps
2021-01-19 23:35:05 INFO: Validating desired state definition

.........


Applications: 
--------------- 

	name:  foo
	description:  
	namespace:  foo
	enabled:  true
	chart:  foo
	version:  0.3.0
	valuesFile:  
	valuesFiles:  .helmsman-tmp/tmp326815726/177983580foo.values.yaml
	postRenderer:  
	test:  false
	protected:  false
	wait:  true
	priority:  0
	SuccessCondition:  <nil>
	SuccessTimeout:  <nil>
	DeleteOnSuccess:  <nil>
	preInstall:  <nil>
	postInstall:  <nil>
	preUpgrade:  <nil>
	postUpgrade:  <nil>
	preDelete:  <nil>
	postDelete:  <nil>
	no-hooks:  false
	timeout:  0
	values to override from env:

Targets: 
--------------- 
foo
2021-01-19 23:35:05 INFO: Setting up kubectl
2021-01-19 23:35:05 DEBUG: kubectl config use-context dev.jt
2021-01-19 23:35:05 INFO: Setting up helm
2021-01-19 23:35:05 DEBUG: helm repo list --output json
2021-01-19 23:35:06 DEBUG: helm version --short -c
2021-01-19 23:35:06 DEBUG: helm repo update
2021-01-19 23:35:39 INFO: Getting chart information
2021-01-19 23:35:39 DEBUG: helm show chart foo --version 0.3.0
2021-01-19 23:35:41 INFO: Charts validated.
2021-01-19 23:35:41 INFO: Preparing plan
2021-01-19 23:35:41 INFO: Acquiring current Helm state from cluster
2021-01-19 23:35:41 DEBUG: helm list --all --max 0 --output json -n foo
2021-01-19 23:35:45 DEBUG: kubectl get secret -n foo -l owner=helm -l name=foo -o jsonpath='{.items[-1].metadata.labels.HELMSMAN_CONTEXT}'
2021-01-19 23:35:47 DEBUG: helm plugin list
2021-01-19 23:35:47 DEBUG: helm secrets dec .helmsman-tmp/tmp200982133/921295371foo.secrets.yaml
2021-01-19 23:35:49 DEBUG: helm diff  --suppress-secrets upgrade foo foo --version 0.3.0 --namespace foo -f .helmsman-tmp/tmp326815726/177983580foo.values.yaml -f .helmsman-tmp/tmp200982133/921295371foo.secrets.yaml.dec
  ...
2021-01-19 23:35:57 DEBUG: helm plugin list
2021-01-19 23:35:57 DEBUG: helm secrets dec .helmsman-tmp/tmp200982133/921295371foo.secrets.yaml.dec <---------
2021-01-19 23:35:57 CRITICAL: Error: plugin "secrets" exited with error

By doing a bit of investigation I came to that conclusion:

In the case there is several successive action called in the same run (diff, upgrade, ...) the field r.SecretsFiles is overwritten during the first call. Therefore the field r.SecretsFiles is different between the first call and the following ones.

One side effect of that can be seen by doing helmsman -show-diff with the secretFiles enabled (helm-secrets plugin + vault driver): helmsman will try do decrypt a file already decrypted because the list of secretFiles changed.

Don't hesitate to ask more details if needed

Cheers 😉

[luisdavim]

Hi, thanks for your contribution. I think this in fact a bug and your solution would work but, unless I'm missing anything, wouldn't it be better to just continue if the secret is already decrypted?

Something like:

		for i := 0; i < len(r.SecretsFiles); i++ {
			if isOfType(r.SecretsFiles[i], []string{".dec"}) {
				// if .dec extension is added before to the secret filename, don't add it again.
				// This happens at upgrade time (where diff and upgrade both call this function)
				// and we don't need to decrypt the file again
				continue
			}
			if err := decryptSecret(r.SecretsFiles[i]); err != nil {
				log.Fatal(err.Error())
			}
			r.SecretsFiles[i] = r.SecretsFiles[i] + ".dec"
		}

What do you think?

[rasta-rocket]

Hi @luisdavim thanks for the review.

Well if it works and it fixes my issue I'm ok with your proposition 😄

I don't have the big picture 😅 , but I don't think that is a good idea to modify the release object in such a function which return the files decrypted (functional approach)

Tell me if you need more time for investigation otherwise I can push your proposal

Thanks

[luisdavim]

The reason for my suggestion is that, unless the decoded secrets are deleted between actions in the plan, it would be more efficient to just decode the secrets once and then re-use the decrypted files. I think that was the original intent of that code, the problem was that it was trying to call the helm secrets plugin with the decoded files which caused the problem you're trying to fix.

If you could run a test in your scenario with the change I'm proposing, that would be much appreciated.

Thanks

[rasta-rocket]

Regarding the test are you mentioning that folder : https://github.com/Praqma/helmsman/tree/master/tests ?

[rasta-rocket]

(proposal has been pushed)

[rasta-rocket]

Ok ok for the test I think I got it
For the secret driver I'm using Vault do you want to handle the test with a service aside (vault) ?

Or maybe I have to play with sops (the other driver of helm-secrets) ... never use it

[luisdavim]

Ok ok for the test I think I got it
For the secret driver I'm using Vault do you want to handle the test with a service aside (vault) ?

Or maybe I have to play with sops (the other driver of helm-secrets) ... never use it

Sorry, I just meant running a build with these changes in the same scenario that you described in the initial post, where you found the bug.
Thanks for the changes 👍

[luisdavim]

LGTM

[rasta-rocket]

Ok cool 👍 (I tried it and it worked)

@luisdavim do you know more or less when will be the next release of helmsman ?

[luisdavim]

I'll try to cut a release today.