fix(controller): JSON-unmarshal marshaled expression template before evaluating

[crenshaw-dev]

Signed-off-by: Michael Crenshaw michael@crenshaw.dev

Fixes #6260 and #6325

The problem

FastTemplate plucks the {{=whatever}} expression template from the stringified JSON representation of the (Argo script/container) template and replaces it with whatever the expression evaluates to.

So for example, you might start with:

"env": [
  {
    "name": "A",
    "value": "{{=toJson('a')}}"
  }
]

That becomes

"env": [
  {
    "name": "A",
    "value": ""a""
  }
]

When it should be

"env": [
  {
    "name": "A",
    "value": "\"a\""
  }
]

Testing

I tested with this workflow, which would have previously failed:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: expression-template-double-quotes
spec:
  entrypoint: main
  templates:
    - name: main
      script:
        env:
          - name: A
            value: '{{="test"}}'
        image: debian:9.4
        command: [bash]
        source: |
          echo "$A"

And this workflow, which would have previously failed:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: expression-template-double-quotes
spec:
  entrypoint: main
  templates:
    - name: main
      script:
        env:
          - name: A
            value: '{{=toJson('a')}}'
        image: debian:9.4
        command: [bash]
        source: |
          echo "$A"

[simster7]

If you want to separate your changes to docs/running-locally.md and hack/recurl.sh from the rest into a separate PR, I can merge those immediately.

[crenshaw-dev]

@simster7 I haven't really thought through the behavior of expr w.r.t. escaped backslashes... I might add another replace line for \\ -> \.

[crenshaw-dev]

@simster7 I changed from a naive string replace solution to what I think is a more robust JSON-unmarshal approach.

Apologies if there are test failures, I'm on a machine with an incomplete dev env right now.

I'll open a PR with just the docs/hack changes.

[crenshaw-dev]

Note to self: add a test for templates that contain newlines. Seems reasonable to break a long expression template across a couple lines, and I think the JSON-encoding would break the expression evaluation.

[crenshaw-dev]

I don't know how to interpret this error when running make pre-commit -B locally:

?       github.com/argoproj/argo-workflows/v3/workflow/verify   [no test files]
FAIL
make: *** [test] Error 1

Must be a local config issue, because I don't see it in the CI tests.

[crenshaw-dev]

Just realized the output needs to be marshaled as a JSON string to avoid errors when unmarshaling the template again.

The lack of that step to marshal as a string causes this to fail in 3.1.1:

{{=toJson(map(0..4, {sprig.randNumeric(5)}))}}

[crenshaw-dev]

Methinks the tests are flaky.

[alexec]

@markterm what help do you need from us to get this over the line?

[crenshaw-dev]

@alexec I think you might have auto-completed the wrong name, but happy for more help if @markterm has time. ;-)

The only thing I want to do is to run a workflow or two with templating in the metrics fields (that's where I made the most invasive changes).

Unfortunately, I can't bill this work to my employer (big yuck), so I'm having to push it forward on weekends. If an Argo dev could verify that metrics templating works fine, I'll be happy with this PR. Otherwise, I'll do my best to carve out a few minutes some evening to give it a test.

[crenshaw-dev]

I've been reading the unit test output incorrectly. I see there's a metrics-related failure. Maybe strconv.Quote and JSON marshaling have a bad interaction. Will look at it.

[crenshaw-dev]

@alexec now that I've figured out how to read the test logs, I've knocked out a couple of other bugs.

This one has me stumped:

--- FAIL: TestGetPodByNode (0.10s)
    operator_test.go:3534: 
        	Error Trace:	operator_test.go:3534
        	Error:      	Expected value not to be nil.
        	Test:       	TestGetPodByNode

[crenshaw-dev]

It passes locally in GoLand, so... flakiness?

[alexec]

That test is flakey. See #6458

[crenshaw-dev]

Sweet. Looks like I missed a codegen thingy again. Will run it tomorrow.

[crenshaw-dev]

@alexec looks like this is gonna pass all the checks. I still don't love the code, but it'll fix the issue until we can go to your per-field evaluation approach.

x

[SVilgelm]

Could you please bump new version with this fix?