Commas and dollar signs improperly passed to Helm

[benjaminxie]

Checklist:

• I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
• I've included steps to reproduce the bug.
• I've pasted the output of argocd version.

Describe the bug

Values with consecutive commas or dollar signs are improperly escaped and passed to Helm (despite PR 1720.)

To Reproduce

Create a new app from a helm chart. Set one of the parameters to ,,,,,,,,$$$$$ (or just any string with consecutive commas and consecutive dollar signs). Click create. To simplify things, I've attached an Application yaml that creates the helm-guestbook example and has an overridden parameter:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: guestbook
spec:
  destination:
    name: ''
    namespace: default
    server: 'https://kubernetes.default.svc'
  source:
    path: helm-guestbook
    repoURL: 'https://github.com/argoproj/argocd-example-apps.git'
    targetRevision: HEAD
    helm:
      parameters:
        - name: containerPort
          value: ',,,,,,,,$$$$'
  sources: []
  project: default
  syncPolicy:
    automated: null

Attempting to create this app, we will see a popup with the error message:
Unable to create application: application spec for guestbook is invalid: InvalidSpecError: Unable to generate manifests in helm-guestbook: rpc error: code = Unknown desc = helm template . --name-template guestbook --namespace default --kube-version 1.28 --set containerPort=,\,,\,,\,,\,$$ <api versions removed> --include-crds failed exit status 1: Error: failed parsing --set data: key "," has no value (cannot end with ,)

Note that flag --set containerPort=,\,,\,,\,,\,$$. Only about half the commas are escaped and we end up with half the $'s we wanted.

Additionally, using forceString: true on the parameter does not help.
Expected behavior

I should be able to create my app.

Helm's set flag should be receive my value but with all the commas properly escaped as well as my dollar signs (I'm guessing whatever shell Argo CD is using, $$ is how you get a literal dollar sign).

Version

$ argocd version
argocd: v2.10.3+0fd6344.dirty
  BuildDate: 2024-03-13T21:01:52Z
  GitCommit: 0fd6344537eb948cff602824a1d060421ceff40e
  GitTreeState: dirty
  GoVersion: go1.22.1
  Compiler: gc
  Platform: darwin/arm64
argocd-server: v2.9.14+d3955f4

Helm has a --set-literal flag that allows us to avoid all this escaping. I understand that Argo CD might not be able to take advantage due to using an older version of Helm or some other technical reason but I thought I'd throw it out there.

[daengdaengLee]

Hello, @benjaminxie.

I have opened a PR #20002 that can partially resolve your issue. In this PR, when the forceString option is set to true, it does not interpolate environment variables and preserves the $ character as it is.

However, I cannot be sure if this PR will be merged.

If you need to resolve this issue immediately, I think you can assume that $$ will be replaced with $ and write the value accordingly. In the code, $$ (consecutive $) is used to escape the $ character.

Thanks!

[daengdaengLee]

Hello, @benjaminxie.

The PR #20002 is closed without merge.
So I think you should escape $ using $$.

My friends and I are working on solving a different problem related to the ,.

Thanks!

[agaudreault]

This is caused by https://argo-cd.readthedocs.io/en/stable/user-guide/helm/#build-environment. It is the desired behavior, and as @daengdaengLee suggested, the correct fix is to escape $ char as they have a special meaning.

An enhancement could be to have a new flag, similar to forceString, like skipInterpolation: true that would not execute variable substitution when specified. In my opinion, unless you have a very compelling reason and escaping $ char is difficult in your context, I don't think we should add an extra parameter and complexify the codebase unnecessarily.

[agaudreault]

Adding back the bug, because it would seem that half the , are properly escaped, and as you found out, they should be escaped with #1720. It seems to be missing a unit test for multiple consecutive commas.

[eogns47]

Hello, @agaudreault

In #20113, added unit tests for consecutive , and fixed a bug. ☺️

Please review, thanks!

[benjaminxie]

@daengdaengLee @agaudreault First off, thank you both for taking the time to look at this.

I think I should explain my actual use case as my example was chosen for its ease of illustrating this bug. My use case actually involves entering these strings from the web UI. These strings are passwords that I didn't want to hardcode into my Helm charts (if there's a better solution than entering them via the web UI, I'm happy to hear them). So I might enter "ShouldEndWithTwoDollars$$" as a parameter during application creation at the UI but when this parameter is used to template, say, a Secret, then the Secret instead holds "ShouldEndWithTwoDollars$". So instead of getting a error popup like in my example, my installation succeeds as far as Argo CD is concerned but has arguably silently failed.

I agree that the behavior for forceString should not change, and I acknowledge that I could escape my input's $'s. However I think it's still an issue that strings entered via the web UI are undergoing variable substitution.

It's an issue because substitution taking place in the app creation UI is unintuitive. I think most users would expect a web form to just use their strings literally. So when pairs of $'s start getting replaced by a single $, it's not clear why it's happening or that it is intended behavior. Even worse, users may even be unaware that the substitution has taken place because it didn't prevent their app creation from succeeding. But the wrong value making it into the manifests can still lead to incorrect app behavior. Moreover, in the case of the value being used in a secret, it's even more opaque due the base64 encoding as well as Argo CD not displaying the entry's value in the secret.

So I think we should have a flag like skipInterpolation: true that is automatically applied when setting parameters from the web UI. It would make the UI work as expected and prevent silent failures.

[eogns47]

@benjaminxie I agree! To prevent failures, we need to modify the Argo CD to disallow $ input on the UI side.
By the way, can you tell me what exactly the flag skipInterpolation: true does?

[benjaminxie]

@eogns47 skipInterpolation:true is just a proposed flag but it would make it so something when a parameter is set to, say, "$ARGOCD_APP_NAME", instead of the parameter becoming "my-app" it stays as "$ARGOCD_APP_NAME". For my specific case, a parameter set to "$$x$$x$$" would not become "$x$x$".

To reiterate for the sake of clarity, this flag does not exist yet.

[agaudreault]

@benjaminxie I don't think you should add password there. These are plain text. You should look into using a Secret Management solution when using ArgoCD. Some popular OSS solutions are:

• https://external-secrets.io/latest/
• https://github.com/bitnami-labs/sealed-secrets
• https://argocd-vault-plugin.readthedocs.io/en/stable/

I think a value added there must be escaped properly, at least for $ char. The , is indeed a bug that needs to be fixed, I'll try to review that when I have some time 😬

[eogns47]

@benjaminxie thanks to your explain!

[crenshaw-dev]

+1, no secrets in the App spec, it's a really bad place for secrets.

I agree that the $ experience in the UI is confusing. I'd recommend detecting the presence of the $ character and then displaying a warning so the user's aware of how dollar signs are handled.

[andrii-korotkov-verkada]

ArgoCD versions 2.10 and below have reached EOL. Can you upgrade and let us know if the issue is still present, please?