Update to Helm v3.7.1, allow to pass credentials and new OCI support

[sathieu]

Fixes #7364.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[sathieu]

I need help on this one. Since Helm v3.6.1, Helm doesn't pass credentials (username and password) to chart URLs if the domain doesn't match the index URL.

I tried to add the --pass-credentials parameter, in a lot of places, but I don't know how to add it to the failing command:

time="2021-09-17T14:40:32Z" level=info msg="../../dist/argocd app create test-helm-with-dependencies --repo file:///tmp/argo-e2e/testdata.git --dest-server https://kubernetes.default.svc --helm-version  --path helm-with-dependencies --project default --dest-namespace argocd-e2e--test-helm-with-dependencies-pdila --plaintext --server 127.0.0.1:8088 --auth-token *** --insecure" dir= execID=2C6qB
time="2021-09-17T14:40:32Z" level=debug duration=271.641053ms execID=2C6qB
time="2021-09-17T14:40:32Z" level=error msg="`../../dist/argocd app create test-helm-with-dependencies --repo file:///tmp/argo-e2e/testdata.git --dest-server https://kubernetes.default.svc --helm-version  --path helm-with-dependencies --project default --dest-namespace argocd-e2e--test-helm-with-dependencies-pdila --plaintext --server 127.0.0.1:8088 --auth-token *** --insecure` failed exit status 20: time=\"2021-09-17T14:40:32Z\" level=fatal msg=\"rpc error: code = InvalidArgument desc = application spec for test-helm-with-dependencies is invalid: InvalidSpecError: Unable to generate manifests in helm-with-dependencies: rpc error: code = Unknown desc = `helm dependency build` failed exit status 1: WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/runner/.kube/config\\nWARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/runner/.kube/config\\nindex.go:346: skipping loading invalid entry for chart \\\"helm\\\" \\\"1.0.0\\\" from https://localhost:9444/argo-e2e/testdata.git/helm-repo/local: validation: chart.metadata.name is required\\nindex.go:346: skipping loading invalid entry for chart \\\"helm\\\" \\\"1.0.0\\\" from /tmp/helm218116912/cache/helm/repository/custom-repo-index.yaml: validation: chart.metadata.name is required\\nindex.go:346: skipping loading invalid entry for chart \\\"helm\\\" \\\"1.0.0\\\" from /tmp/helm218116912/cache/helm/repository/custom-repo-index.yaml: validation: chart.metadata.name is required\\nindex.go:346: skipping loading invalid entry for chart \\\"helm\\\" \\\"1.0.0\\\" from /tmp/helm218116912/cache/helm/repository/custom-repo-index.yaml: validation: chart.metadata.name is required\\nError: could not download http://127.0.0.1:9080/argo-e2e/testdata.git/helm-repo/helm-1.0.0.tgz: failed to fetch http://127.0.0.1:9080/argo-e2e/testdata.git/helm-repo/helm-1.0.0.tgz : 401 Unauthorized\"" execID=2C6qB

(ref)

@jannfis Could you help me?

NB: Having to wait approval to get the CI is annoying. I've created sathieu#1 to workaround this.

[codecov]

Codecov Report

Merging #7249 (c63b0c2) into master (0dbffb8) will increase coverage by 0.01%.
The diff coverage is 36.73%.

@@            Coverage Diff             @@
##           master    #7249      +/-   ##
==========================================
+ Coverage   41.39%   41.40%   +0.01%     
==========================================
  Files         161      161              
  Lines       21642    21656      +14     
==========================================
+ Hits         8958     8967       +9     
- Misses      11421    11424       +3     
- Partials     1263     1265       +2     

Impacted Files	Coverage Δ
cmd/argocd/commands/app.go	0.53% <0.00%> (-0.01%)	⬇️
util/helm/cmd.go	28.65% <0.00%> (-1.23%)	⬇️
util/helm/helmver.go	80.00% <ø> (ø)
util/helm/client.go	48.50% <21.42%> (+2.21%)	⬆️
util/helm/helm.go	60.81% <50.00%> (ø)
cmd/util/app.go	37.19% <60.00%> (+0.31%)	⬆️
reposerver/repository/repository.go	60.90% <90.00%> (+0.21%)	⬆️
pkg/apis/application/v1alpha1/types.go	57.51% <100.00%> (ø)
util/settings/settings.go	46.83% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43cba9c...c63b0c2. Read the comment docs.

[jannfis]

@sathieu Thanks for trying to tackle this.

IMHO, this should not be a setting on repository (or repocreds) level, but for each application, in the .spec.source.helm section, i.e.

apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  source:
     helm:
        helmOptions:
           passCredentials: true

[jannfis]

Also, if you modify the CLI (for the argocd app ... commands), the parameter name should be prefixed with helm, i.e. --helm-pass-credentials instead of just --pass-credentials, to be less of a confusion.

[sathieu]

@jannfis:

IMHO, this should not be a setting on repository (or repocreds) level, but for each application

Repos have either all artifacts under the same domain, or all artifacts under a different domain, so this looks more like a repo setting. This is also very tightly linked with the username and password, already stored in repo and repocreds.

--helm-pass-credentials instead of just --pass-credentials, to be less of a confusion.

Done.

Still, I couldn't get the CI to pass, any idea?

EDIT: I've fixed one more item:

time="2021-09-30T09:52:32Z" level=error msg="helm chart save /home/runner/work/argo-cd/argo-cd/test/e2e/testdata/helm-values localhost:5000/myrepo/helm-values:1.0.0 failed exit status 1: WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/runner/.kube/config\nWARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/runner/.kube/config\nError: unknown command \"chart\" for \"helm\"\nRun 'helm --help' for usage." execID=p6adC
helm chart save was renamed helm package

[sathieu]

@jannfis Please review again, CI is now passing. This was an tedious interesting job ...

[alexmt]

Thanks for working on it @samath117 !

Added a couple of questions. Just want to confirm that there is a way for end users to downgrade to helm v3.6 and proposing to document the breaking change.

Did not actually request any changes, so LGTM
(waiting for feedback from @jannfis as well)

[alexmt]

Just double-checked that helm package is available in helm 3.6, so users should be able to downgrade to v3.6 if they are not ready for v3.7. Is your understanding the same?

[sathieu]

Unfortunately not. The way Helm charts are stored in the repository is completely different.

To extract an OCI chart

# In Helm v3.6.x:
helm chart pull $repo/$chart:$version # goes in helm local cache
helm chart export $repo/$chart:$version --destination $tempdest
tar -zcvf /argo/cache/$chart-$version.tgz $tempdest/$chart
# -> chart is available in /argo/cache/$chart-$version.tgz


# In Helm v3.7.x:
helm pull oci://$repo/$chart --version $version --destination $tempdest
# -> chart is available in $tempdest/$chart-$version.tgz

If e need to support both versions, I'll need to improve the code.

[sathieu]

Also note that an OCI chart for v3.6.x is not usable in helm v3.7.x and vice-versa.

[alexmt]

Debating about introducing a breaking change: potentially we can assume that pass-credentials is true by default to simplify the migration. This is probably a bad idea due to CVE-2021-32690. So it is better to break backward compatibility.

In this case can you please document the change in https://github.com/argoproj/argo-cd/tree/master/docs/operator-manual/upgrading/2.1-2.2

@jannfis are you thinking the same?

[alexmt]

@jannfis , I've noticed your comment about adding app-level setting: #7249 (comment)

I'm curious why do you think this is necessary? It feels like per repo setting is a better option since it is per repo setting in the helm itself. E.g. helm dep up does have --pass-credentials flag. Why do you think the flag should be available in app-level settings? I might be missing something.

[sathieu]

@jannfis

Actually, I was just thinking that maybe an app in the repository has a Chart with a dependency in another repository that you don't want to share your credentials with, so the big toggle on the repo is a little bit "dangerous". If the setting is on the repository level, it's an all-or-nothing approach. It might lead to nasty surprises. If it's on the application level, you declare your consent as in "Yes, this chart has a dependency to a chart in repository X, and for accessing repository X, I want to use the same credentials".

This really makes sense. Thanks. I'll update this PR to move this setting in the application (just wait some days, this takes time...)

Also, I've updated Helm to v3.71, which adds support for pre-3.7 oci charts (helm/helm@52fbd4e). This addresses #7249 (comment).

[alexmt]

Thanks a lot @sathieu ! Looking forward for this PR

[alexmt]

Thank you for the awesome contribution @sathieu!

[jannfis]

LGTM. Thanks for this awesome contribution and for going the extra mile with us @sathieu, and sorry for the lagging communication from my side.

Your efforts and patience are much appreciated!

[jannfis]

Reminder /cc @alexmt We will need some UI changes for this change in a a separate PR.

[sathieu]

@jannfis Thanks. However the tests are not passing. The passCredentials parameter is not passed thru all steps.

See my tries in sathieu#2.

[sathieu]

@jannfis The tests are now passing (the fix was just one line in test/e2e/helm_test.go, but it took me hours 🐌 ). Please start the CI and hopefully merge 🙏!

The fix:

diff --git a/test/e2e/helm_test.go b/test/e2e/helm_test.go
index b6148e8ab..e44ef9d1a 100644
--- a/test/e2e/helm_test.go
+++ b/test/e2e/helm_test.go
@@ -358,6 +358,7 @@ func TestHelmWithMultipleDependencies(t *testing.T) {
                // these are slow tests
                Timeout(30).
                HelmHTTPSCredentialsUserPassAdded().
+               HelmPassCredentials().
                When().
                Create().
                Sync().

[sathieu]

@jannfis @alexmt Thanks for reviewing and merging. What is the expected release date of ArgoCD 2.2.0?