Argo: withParam/arguments is escaping items

Created on 7 Jul 2020 · 32Comments · Source: argoproj/argo

Checklist:

[x] I've included the version.
[x] I've included reproduction steps.
[x] I've included the workflow YAML.
[ ] I've included the logs.

What happened:

Upgraded from 2.2.1 to 2.9.1 and tried to run a workflow which was flawlessly running in the previous version with no luck. After inspecting the steps, it seems that the iteration of withParam is escaping the arguments, so the next step receives an escaped string:

export CURRENT_DATETIME=\"`date +%!Y(MISSING)%!m(MISSING)%!d(MISSING)%!H(MISSING)%!M(MISSING)%!S(MISSING)`\" \u0026\u0026 kubectl patch configmap  cache-breaker -p \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default

Which breaks on running it on SH

What you expected to happen:

Step should have received

export CURRENT_DATETIME="`date +%Y%m%d%H%M%S`" && kubectl patch configmap cache-breaker -p "{ \"data\"  : { \"cache-breaker\" : \"${CURRENT_DATETIME}\"}}" -n default

How to reproduce it (as minimally and precisely as possible):

(loop exampled modified to get an & in the strings)

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: loops-
spec:
  entrypoint: loop-example
  templates:
  - name: loop-example
    steps:
    - - name: print-message
        template: whalesay
        arguments:
          parameters:
          - name: message
            value: "{{item}}"
        withItems:
        - hello wo&rld
        - goodbye world

  - name: whalesay
    inputs:
      parameters:
      - name: message
    container:
      image: docker/whalesay:latest
      command: [cowsay]
      args: ["{{inputs.parameters.message}}"]

Should print "hello wo&rld" but the & gets escaped

Anything else we need to know?:

Tried on 2.8.2 and same behavior is observed

Environment:

N/A

Message from the maintainers:

If you are impacted by this bug please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

bug wontfix

Source

Point-less

👍2

All 32 comments

Ampersand & is replaced with unicode for ampersand \u0026.

alexec on 7 Jul 2020

See:

https://stackoverflow.com/questions/24656624/golang-display-character-not-ascii-like-not-0026
https://stackoverflow.com/questions/28595664/how-to-stop-json-marshal-from-escaping-and/28596225

alexec on 7 Jul 2020

So this is some pretty fundamental Golang stuff and the fix may not be easy - or even possible.

alexec on 7 Jul 2020

This test passes:

func Test_ampersand(t *testing.T) {
    item := Item{}
    err := json.Unmarshal([]byte(`"&"`), &item)
    assert.NoError(t, err)
    val := item.GetStrVal()
    assert.Equal(t, "&", val)
}

alexec on 7 Jul 2020

@alexmt @jessesuen @sarabala1979 have you seen this problem before? Some core Golang escaping of ampersands breaking JSON encoding.

alexec on 7 Jul 2020

Other than the ampersand, %Y gets replaced by %!Y(MISSING) in my workflow, which is really bad.

Point-less on 7 Jul 2020

Other than the ampersand, %Y gets replaced by %!Y(MISSING) in my workflow, which is really bad.

Do you have an example of this YAML please?

alexec on 8 Jul 2020

Something like this:
```apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: releasemanager-deploy-
spec:
ttlSecondsAfterFinished: 604800
entrypoint: main
templates:

name: main
inputs:
parameters:
- name: commands
  
  steps:
  
  - - name: execute-commands
  
  template: execute-command
  
  arguments:
  
  parameters:
  - name: command
    
    value: "{{item.command}}"
  - name: image
    
    value: "{{item.image}}"
    
    withParam: '{{inputs.parameters.commands}}'

name: execute-command
inputs:
parameters:
- name: command
- name: image
  
  script:
  
  image: '{{inputs.parameters.image}}'
  
  command:
- sh
  
  source: '{{inputs.parameters.command}}'


executed:

Name: _
Namespace: default
ServiceAccount: default
Status: Pending
Created: Tue Jul 07 22:12:47 +0000 (now)
Parameters:
commands: [{"image": "_", "command": "export CURRENT_DATETIME=\"date +%Y%m%d%H%M%S\" && kubectl patch configmap cache-breaker -p \"{ \\"data\\" : { \\"cache-breaker\\" : \\"${CURRENT_DATETIME}\\"}}\" -n default"}]
```

I can't currently test with 2.9.2 as I've already downgraded to 2.2.1 in the cluster :(

Point-less on 8 Jul 2020

so you

argo submit 3409.yaml -p 'commands=[{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && kubectl patch configmap cache-breaker -p \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"}]'

alexec on 8 Jul 2020

yeap
on argoUI on the root node it shows the command as unescaped and on the execute-command step it's escaped

Point-less on 8 Jul 2020

So this works for me ok:

kind: Workflow
metadata:
  generateName: releasemanager-deploy-
spec:
  ttlSecondsAfterFinished: 604800
  entrypoint: main
  templates:
  - name: main
    inputs:
      parameters:
      - name: commands
    steps:
    - - name: execute-commands
        template: execute-command
        arguments:
          parameters:
          - name: command
            value: "{{item.command}}"
          - name: image
            value: "{{item.image}}"
        withParam: '{{inputs.parameters.commands}}'


  - name: execute-command
    inputs:
      parameters:
      - name: command
      - name: image
    script:
      image: '{{inputs.parameters.image}}'
      command:
      - sh
      source: '{{inputs.parameters.command}}'

 argo submit 3409.yaml -p 'commands=[{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"}]' --log
Name:                releasemanager-deploy-66pq7
Namespace:           argo
ServiceAccount:      default
Status:              Pending
Created:             Tue Jul 07 16:32:12 -0700 (now)
Parameters:          
  commands:          {1 0 [{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"}]}
releasemanager-deploy-66pq7-3064451249: { "data" : { "cache-breaker" : "20200707233216"}} -n default

alexec on 8 Jul 2020

There are two issues contained in this single issue. That will get confusing so we should probably split it at some point.

alexec on 8 Jul 2020

Upgraded to 2.9.1 again to test this case and this is the result

So, the two issues are the escaping of & (or other characters) and the escaping of %Y ?

Point-less on 8 Jul 2020

correct - they are different problems

& is due to Golang core JSON library and how our GRPC layer uses it - I'm not sure this is fixable
%Y is something else - I've not managed to repo

alexec on 8 Jul 2020

https://github.com/kubernetes/kubernetes/issues/73803

alexec on 8 Jul 2020

https://github.com/golang/go/issues/8592

alexec on 8 Jul 2020

looking at stack frames we have

Good at this point:

github.com/argoproj/argo/workflow/util.FromUnstructured at util.go:128
...
hello wo&rld

But by the next time we have control, it has been garbaged:

github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.(*ParallelSteps).UnmarshalJSON at workflow_types.go:376
...
[{"arguments":{"parameters":[{"name":"message","value":"{{item}} \u0026 "}]},"name":"print-message","template":"whalesay","withItems":["hello wo\u0026rld","goodbye world"]}]

alexec on 8 Jul 2020

Can you please attache the YAML?

alexec on 16 Jul 2020

So this works for me ok:

kind: Workflow
metadata:
  generateName: releasemanager-deploy-
spec:
  ttlSecondsAfterFinished: 604800
  entrypoint: main
  templates:
  - name: main
    inputs:
      parameters:
      - name: commands
    steps:
    - - name: execute-commands
        template: execute-command
        arguments:
          parameters:
          - name: command
            value: "{{item.command}}"
          - name: image
            value: "{{item.image}}"
        withParam: '{{inputs.parameters.commands}}'


  - name: execute-command
    inputs:
      parameters:
      - name: command
      - name: image
    script:
      image: '{{inputs.parameters.image}}'
      command:
      - sh
      source: '{{inputs.parameters.command}}'

 argo submit 3409.yaml -p 'commands=[{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"}]' --log
Name:                releasemanager-deploy-66pq7
Namespace:           argo
ServiceAccount:      default
Status:              Pending
Created:             Tue Jul 07 16:32:12 -0700 (now)
Parameters:          
  commands:          {1 0 [{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"}]}
releasemanager-deploy-66pq7-3064451249: { "data" : { "cache-breaker" : "20200707233216"}} -n default

This same yaml with that input

I've run that with 2.9.1 and got the & and the %Y %m %d escaped

Point-less on 16 Jul 2020

So escaping repeatedly works?

alexec on 16 Jul 2020

I've not tested if this happens or not with \ and i have not check what happens if i escape with \ the escaped tokens

Point-less on 16 Jul 2020

I've had the same problem with 2.9.x wherein I have JSON data with \" escaped quotes in strings. The strings appear to be double escaped before they get set as inputs.

I've tried the above example,

.argo submit 3409.yaml -p 'commands=[{"image": "docker/whalesay:latest", "command": "export CURRENT_DATETIME=\"`date +%Y%m%d%H%M%S`\" && echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default"

and can confirm that it does not work as expected in 2.9.3, logging export: "{: bad variable name with the command input:

export CURRENT_DATETIME=\"`date +%!Y(MISSING)%!m(MISSING)%!d(MISSING)%!H(MISSING)%!M(MISSING)%!S(MISSING)`\" \u0026\u0026 echo \"{ \\\"data\\\" : { \\\"cache-breaker\\\" : \\\"${CURRENT_DATETIME}\\\"}}\" -n default

However, it does work in v2.10.0-rc3, logging { "data" : { "cache-breaker" : "20200727133013"}} -n default with the input:

export CURRENT_DATETIME="`date +%Y%m%d%H%M%S`" && echo "{ \"data\" : { \"cache-breaker\" : \"${CURRENT_DATETIME}\"}}" -n default

Which looks to have been escaped correctly.

tanenbaum on 27 Jul 2020

Unfortunately this is once again broken in v2.10.0-rc5.

As above, it was working in v2.10.0-rc4. It must be related to the fixes in #3653

A simple workflow to test with is:

kind: Workflow
metadata:
  generateName: escape-test-
spec:
  entrypoint: main

  templates:
  - name: main
    inputs:
      parameters:
      - name: things
        value: '[{"prop": "\"A\""}]'
    steps:
    - - name: echoitems
        template: echo
        arguments:
          parameters:
            - name: thing
              value: "{{item.prop}}"
        withParam: "{{inputs.parameters.things}}"

  - name: echo
    inputs:
      parameters:
        - name: thing
    container:
      image: busybox
      command: [echo]
      args: ["{{inputs.parameters.thing}}"]

This should echo "A" but is \"A\" in 2.9.x and v2.10.0-rc5.

tanenbaum on 4 Aug 2020

metadata:
  name: escape-test-9k2pj
  generateName: escape-test-
  namespace: argo
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/argo/workflows/escape-test-9k2pj
  uid: 6480c08a-87a6-4581-936d-b7a668327c01
  resourceVersion: '120992'
  generation: 6
  creationTimestamp: '2020-08-04T15:21:15Z'
  labels:
    workflows.argoproj.io/completed: 'true'
    workflows.argoproj.io/creator: admin
    workflows.argoproj.io/phase: Succeeded
spec:
  templates:
    - name: main
      arguments: {}
      inputs:
        parameters:
          - name: things
            value: '[{"prop": "\"A\""}]'
      outputs: {}
      metadata: {}
      steps:
        - - name: echoitems
            template: echo
            arguments:
              parameters:
                - name: thing
                  value: '{{item.prop}}'
            withParam: '{{inputs.parameters.things}}'
    - name: echo
      arguments: {}
      inputs:
        parameters:
          - name: thing
      outputs: {}
      metadata: {}
      container:
        name: ''
        image: busybox
        command:
          - echo
        args:
          - '{{inputs.parameters.thing}}'
        resources: {}
  entrypoint: main
  arguments: {}
status:
  phase: Succeeded
  startedAt: '2020-08-04T15:21:15Z'
  finishedAt: '2020-08-04T15:21:18Z'
  nodes:
    escape-test-9k2pj:
      id: escape-test-9k2pj
      name: escape-test-9k2pj
      displayName: escape-test-9k2pj
      type: Steps
      templateName: main
      templateScope: local/escape-test-9k2pj
      phase: Succeeded
      startedAt: '2020-08-04T15:21:15Z'
      finishedAt: '2020-08-04T15:21:18Z'
      inputs:
        parameters:
          - name: things
            value: '[{"prop": "\"A\""}]'
      children:
        - escape-test-9k2pj-12505430
      outboundNodes:
        - escape-test-9k2pj-1906724094
    escape-test-9k2pj-12505430:
      id: escape-test-9k2pj-12505430
      name: 'escape-test-9k2pj[0]'
      displayName: '[0]'
      type: StepGroup
      templateName: main
      templateScope: local/escape-test-9k2pj
      phase: Succeeded
      boundaryID: escape-test-9k2pj
      startedAt: '2020-08-04T15:21:15Z'
      finishedAt: '2020-08-04T15:21:18Z'
      children:
        - escape-test-9k2pj-1906724094
    escape-test-9k2pj-1906724094:
      id: escape-test-9k2pj-1906724094
      name: 'escape-test-9k2pj[0].echoitems(0:prop:\"A\")'
      displayName: 'echoitems(0:prop:\"A\")'
      type: Pod
      templateName: echo
      templateScope: local/escape-test-9k2pj
      phase: Succeeded
      boundaryID: escape-test-9k2pj
      startedAt: '2020-08-04T15:21:15Z'
      finishedAt: '2020-08-04T15:21:18Z'
      resourcesDuration:
        cpu: 2
        memory: 1
      inputs:
        parameters:
          - name: thing
            value: \"A\"
      outputs:
        artifacts:
          - name: main-logs
            archiveLogs: true
            s3:
              endpoint: 'minio:9000'
              bucket: my-bucket
              insecure: true
              accessKeySecret:
                name: my-minio-cred
                key: accesskey
              secretKeySecret:
                name: my-minio-cred
                key: secretkey
              key: escape-test-9k2pj/escape-test-9k2pj-1906724094/main.log
        exitCode: '0'
      hostNodeName: k3d-k3s-default-server
  conditions:
    - type: Completed
      status: 'True'
  resourcesDuration:
    cpu: 2
    memory: 1

alexec on 4 Aug 2020

I've been looking at the code involved, it's clearly to do with JSON serdes with v1alpha1.Item using json.RawMessage and the use of GetMapVal in operator.go.

This commit 2b6db45b2775cf8bff22b89b0a30e4dda700ecf9 fixes the nesting issue but reintroduces this double encoding problem. I'm not sure there's a nice solution.

tanenbaum on 4 Aug 2020

👍1

I'm not sure what to do about this ATM. Fix one issue we introduce the other. As the other issue changes pod ID which cause workflow running when you upgrade to fail - it's a more serious issue that this one.

This is all because of the "convenience" of Item.String().

alexec on 4 Aug 2020

@tanenbaum I do want to thank you for raising this problem and responding back so quickly. It is key in helping us fixing these issues.

alexec on 4 Aug 2020

👍1

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] on 3 Oct 2020

This is effecting us as well, i.e. any workflows handling parameter values with & included are failing. Anyone have a workaround ATM? @alexec @tanenbaum @Point-less

mecampbellsoup on 12 Oct 2020

It looks like Go added an option to not escape specific characters like &:

mecampbellsoup on 12 Oct 2020

@mecampbellsoup I ended up taking my more complex data types and putting them into ConfigMaps, auto-created in the workflow using the resource type https://argoproj.github.io/argo/fields/#resourcetemplate and setting setOwnerReference to true so they get cleaned up with the workflows.

I then only use simple array types for any iterating at the argo job level, using Ids etc. to mount well namespaced configmaps into jobs. Hopefully I'll avoid any other serialization problems this way, though I do still end up having to use %% in some places where I need a single %.

While it's a bit more effort to do this, it does have the advantage of minimising the number amount of repetitive arguments/parameters.

tanenbaum on 12 Oct 2020

I've looked into this - and it is possible to fix it by implementing a custom marshaller. However, we'd need to do this every time we marshal an object to JSON, and never-ever write any new code that forgets to do this. Obviously, this is impractical.

It is rather frustrating to have this bug because a Golang coder years ago thought that you'd definitely want this on by default. Just a bizarre choice.

alexec on 13 Oct 2020

Was this page helpful?

0 / 5 - 0 ratings

Related issues

WorkflowTemplate parameters are not substituted when referenced

CermakM · 3Comments

Hello Wold example stuck at ContainerCreating with microk8s

srados · 3Comments

Difficulty finding documentation about policies

vicaire · 4Comments

Recommended resources for workflow controller

ddseapy · 3Comments

nested template outputs produce runtime error: invalid memory address or nil pointer dereference

CermakM · 3Comments