Skip to content

Commit

Permalink
fix(controller): JSON-unmarshal marshaled expression template before …
Browse files Browse the repository at this point in the history
…evaluating (argoproj#6285)

Signed-off-by: Michael Crenshaw <[email protected]>
  • Loading branch information
crenshaw-dev committed Aug 5, 2021
1 parent 7bf825b commit 2a2ecc9
Show file tree
Hide file tree
Showing 8 changed files with 270 additions and 41 deletions.
24 changes: 24 additions & 0 deletions docs/fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ Workflow is the definition of a workflow resource

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -527,6 +529,8 @@ WorkflowSpec is the specification of a Workflow.

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -947,6 +951,8 @@ CronWorkflowSpec is the specification of a CronWorkflow

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -1324,6 +1330,8 @@ WorkflowTemplateSpec is a spec of WorkflowTemplate.

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -1660,6 +1668,8 @@ Arguments to a template

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -2387,6 +2397,8 @@ Parameter indicate a passed string parameter to a service template with an optio

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -2901,6 +2913,8 @@ Inputs are the mechanism for passing parameters, artifacts, volumes from one tem

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -3106,6 +3120,8 @@ ScriptTemplate is a template subtype to enable scripting through code steps

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)

- [`loops-param-result.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/loops-param-result.yaml)
Expand Down Expand Up @@ -4077,6 +4093,8 @@ DataSource sources external data into a data template

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)

- [`loops-param-result.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/loops-param-result.yaml)
Expand Down Expand Up @@ -4708,6 +4726,8 @@ ObjectMeta is metadata that all persisted resources must have, which includes al

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down Expand Up @@ -5584,6 +5604,8 @@ EnvVar represents an environment variable present in a Container.

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`secrets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/secrets.yaml)

- [`sidecar-dind.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/sidecar-dind.yaml)
Expand Down Expand Up @@ -5989,6 +6011,8 @@ PersistentVolumeClaimSpec describes the common attributes of storage devices and

- [`expression-destructure-json.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-destructure-json.yaml)

- [`expression-reusing-verbose-snippets.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-reusing-verbose-snippets.yaml)

- [`expression-tag-template-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/expression-tag-template-workflow.yaml)

- [`fibonacci-seq-conditional-param.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/fibonacci-seq-conditional-param.yaml)
Expand Down
44 changes: 44 additions & 0 deletions examples/expression-reusing-verbose-snippets.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Expressions do not support variables. Rather than repeating verbose expressions to reuse output, you can map over the
# expression output and use its value which is aliased as `#`. Then you can place your "variables" in a JSON object to
# be used elsewhere.
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: expression-reusing-verbose-snippets-
spec:
arguments:
parameters:
- name: weather
# The base64 string is this JSON: {"temps": [34, 27, 15, 57, 46]}
value: '{"weekWeather": "eyJ0ZW1wcyI6IFszNCwgMjcsIDE1LCA1NywgNDZdfQo="}'
entrypoint: main
templates:
- name: main
inputs:
parameters:
- name: week-temps
# The line being mapped over is verbose. Rather than repeat it, we use `map` to alias its output as #.
value: >-
{{=
map([
jsonpath(sprig.b64dec(jsonpath(workflow.parameters.weather, '$.weekWeather')), '$.temps')
], {
toJson({
avg: sprig.add(#[0], #[1], #[2], #[3], #[4]) / 5,
min: sprig.min(#[0], #[1], #[2], #[3], #[4]),
max: sprig.max(#[0], #[1], #[2], #[3], #[4])
})
})[0]
}}
script:
env:
- name: AVG
value: "{{=jsonpath(inputs.parameters['week-temps'], '$.avg')}}"
- name: MIN
value: "{{=jsonpath(inputs.parameters['week-temps'], '$.min')}}"
- name: MAX
value: "{{=jsonpath(inputs.parameters['week-temps'], '$.max')}}"
image: debian:9.4
command: [bash]
source: |
echo "The week's average temperature was $AVG with a minimum of $MIN and a maximum of $MAX."
29 changes: 26 additions & 3 deletions util/template/expression_template.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package template

import (
"encoding/json"
"fmt"
"io"
"os"
Expand All @@ -17,12 +18,22 @@ func init() {
}

func expressionReplace(w io.Writer, expression string, env map[string]interface{}, allowUnresolved bool) (int, error) {
if _, ok := env["retries"]; !ok && hasRetries(expression) && allowUnresolved {
// The template is JSON-marshaled. This JSON-unmarshals the expression to undo any character escapes.
var unmarshalledExpression string
err := json.Unmarshal([]byte(fmt.Sprintf(`"%s"`, expression)), &unmarshalledExpression)
if err != nil && allowUnresolved {
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}
if err != nil {
return 0, fmt.Errorf("failed to unmarshall JSON expression: %w", err)
}

if _, ok := env["retries"]; !ok && hasRetries(unmarshalledExpression) && allowUnresolved {
// this is to make sure expressions like `sprig.int(retries)` don't get resolved to 0 when `retries` don't exist in the env
// See https://github.com/argoproj/argo-workflows/issues/5388
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}
result, err := expr.Eval(expression, env)
result, err := expr.Eval(unmarshalledExpression, env)
if (err != nil || result == nil) && allowUnresolved { // <nil> result is also un-resolved, and any error can be unresolved
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}
Expand All @@ -32,7 +43,19 @@ func expressionReplace(w io.Writer, expression string, env map[string]interface{
if result == nil {
return 0, fmt.Errorf("failed to evaluate expression %q", expression)
}
return w.Write([]byte(fmt.Sprintf("%v", result)))
resultMarshaled, err := json.Marshal(fmt.Sprintf("%v", result))
if (err != nil || resultMarshaled == nil) && allowUnresolved {
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}
if err != nil {
return 0, fmt.Errorf("failed to marshal evaluated expression: %w", err)
}
if resultMarshaled == nil {
return 0, fmt.Errorf("failed to marshal evaluated marshaled expression %q", expression)
}
// Trim leading and trailing quotes. The value is being inserted into something that's already a string.
marshaledLength := len(resultMarshaled)
return w.Write(resultMarshaled[1 : marshaledLength-1])
}

func envMap(replaceMap map[string]string) map[string]interface{} {
Expand Down
22 changes: 21 additions & 1 deletion util/template/replace.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
package template

import (
"encoding/json"
"errors"
)

// Replace takes a json-formatted string and performs variable replacement.
func Replace(s string, replaceMap map[string]string, allowUnresolved bool) (string, error) {
if !json.Valid([]byte(s)) {
return "", errors.New("cannot do template replacements with invalid JSON")
}

t, err := NewTemplate(s)
if err != nil {
return "", err
}
return t.Replace(replaceMap, allowUnresolved)

replacedString, err := t.Replace(replaceMap, allowUnresolved)
if err != nil {
return s, err
}

if !json.Valid([]byte(replacedString)) {
return s, errors.New("cannot finish template replacement because the result was invalid JSON")
}

return replacedString, nil
}
70 changes: 38 additions & 32 deletions util/template/replace_test.go
Original file line number Diff line number Diff line change
@@ -1,56 +1,62 @@
package template

import (
"encoding/json"
"testing"

"github.com/stretchr/testify/assert"
)

func toJsonString(v interface{}) string {
jsonString, _ := json.Marshal(v)
return string(jsonString)
}

func Test_Replace(t *testing.T) {
t.Run("InvailedTemplate", func(t *testing.T) {
_, err := Replace("{{", nil, false)
t.Run("InvalidTemplate", func(t *testing.T) {
_, err := Replace(toJsonString("{{"), nil, false)
assert.Error(t, err)
})
t.Run("Simple", func(t *testing.T) {
t.Run("Valid", func(t *testing.T) {
r, err := Replace("{{foo}}", map[string]string{"foo": "bar"}, false)
r, err := Replace(toJsonString("{{foo}}"), map[string]string{"foo": "bar"}, false)
assert.NoError(t, err)
assert.Equal(t, "bar", r)
assert.Equal(t, toJsonString("bar"), r)
})
t.Run("Unresolved", func(t *testing.T) {
t.Run("Allowed", func(t *testing.T) {
_, err := Replace("{{foo}}", nil, true)
_, err := Replace(toJsonString("{{foo}}"), nil, true)
assert.NoError(t, err)
})
t.Run("Disallowed", func(t *testing.T) {
_, err := Replace("{{foo}}", nil, false)
_, err := Replace(toJsonString("{{foo}}"), nil, false)
assert.EqualError(t, err, "failed to resolve {{foo}}")
})
})
})
t.Run("Expression", func(t *testing.T) {
t.Run("Valid", func(t *testing.T) {
r, err := Replace("{{=foo}}", map[string]string{"foo": "bar"}, false)
r, err := Replace(toJsonString("{{=foo}}"), map[string]string{"foo": "bar"}, false)
assert.NoError(t, err)
assert.Equal(t, "bar", r)
assert.Equal(t, toJsonString("bar"), r)
})
t.Run("Unresolved", func(t *testing.T) {
t.Run("Allowed", func(t *testing.T) {
_, err := Replace("{{=foo}}", nil, true)
_, err := Replace(toJsonString("{{=foo}}"), nil, true)
assert.NoError(t, err)
})
t.Run("AllowedRetries", func(t *testing.T) {
replaced, err := Replace("{{=sprig.int(retries)}}", nil, true)
replaced, err := Replace(toJsonString("{{=sprig.int(retries)}}"), nil, true)
assert.NoError(t, err)
assert.Equal(t, replaced, "{{=sprig.int(retries)}}")
assert.Equal(t, replaced, toJsonString("{{=sprig.int(retries)}}"))
})
t.Run("Disallowed", func(t *testing.T) {
_, err := Replace("{{=foo}}", nil, false)
_, err := Replace(toJsonString("{{=foo}}"), nil, false)
assert.EqualError(t, err, "failed to evaluate expression \"foo\"")
})
})
t.Run("Error", func(t *testing.T) {
_, err := Replace("{{=!}}", nil, false)
_, err := Replace(toJsonString("{{=!}}"), nil, false)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "failed to evaluate expression")
}
Expand All @@ -61,53 +67,53 @@ func Test_Replace(t *testing.T) {
func TestNestedReplaceString(t *testing.T) {
replaceMap := map[string]string{"inputs.parameters.message": "hello world"}

test := `{{- with secret "{{inputs.parameters.message}}" -}}
test := toJsonString(`{{- with secret "{{inputs.parameters.message}}" -}}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)
replacement, err := Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "{{- with secret \"hello world\" -}}\n {{ .Data.data.gitcreds }}\n {{- end }}", replacement)
assert.Equal(t, toJsonString("{{- with secret \"hello world\" -}}\n {{ .Data.data.gitcreds }}\n {{- end }}"), replacement)
}

test = `{{- with {{ secret "{{inputs.parameters.message}}" -}}
test = toJsonString(`{{- with {{ secret "{{inputs.parameters.message}}" -}}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)

replacement, err = Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "{{- with {{ secret \"hello world\" -}}\n {{ .Data.data.gitcreds }}\n {{- end }}", replacement)
assert.Equal(t, toJsonString("{{- with {{ secret \"hello world\" -}}\n {{ .Data.data.gitcreds }}\n {{- end }}"), replacement)
}

test = `{{- with {{ secret "{{inputs.parameters.message}}" -}} }}
test = toJsonString(`{{- with {{ secret "{{inputs.parameters.message}}" -}} }}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)

replacement, err = Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "{{- with {{ secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}", replacement)
assert.Equal(t, toJsonString("{{- with {{ secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}"), replacement)
}

test = `{{- with secret "{{inputs.parameters.message}}" -}} }}
test = toJsonString(`{{- with secret "{{inputs.parameters.message}}" -}} }}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)

replacement, err = Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "{{- with secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}", replacement)
assert.Equal(t, toJsonString("{{- with secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}"), replacement)
}

test = `{{- with {{ {{ }} secret "{{inputs.parameters.message}}" -}} }}
test = toJsonString(`{{- with {{ {{ }} secret "{{inputs.parameters.message}}" -}} }}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)

replacement, err = Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "{{- with {{ {{ }} secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}", replacement)
assert.Equal(t, toJsonString("{{- with {{ {{ }} secret \"hello world\" -}} }}\n {{ .Data.data.gitcreds }}\n {{- end }}"), replacement)
}

test = `{{- with {{ {{ }} secret "{{does-not-exist}}" -}} }}
test = toJsonString(`{{- with {{ {{ }} secret "{{does-not-exist}}" -}} }}
{{ .Data.data.gitcreds }}
{{- end }}`
{{- end }}`)

replacement, err = Replace(test, replaceMap, true)
if assert.NoError(t, err) {
Expand All @@ -118,9 +124,9 @@ func TestNestedReplaceString(t *testing.T) {
func TestReplaceStringWithWhiteSpace(t *testing.T) {
replaceMap := map[string]string{"inputs.parameters.message": "hello world"}

test := `{{ inputs.parameters.message }}`
test := toJsonString(`{{ inputs.parameters.message }}`)
replacement, err := Replace(test, replaceMap, true)
if assert.NoError(t, err) {
assert.Equal(t, "hello world", replacement)
assert.Equal(t, toJsonString("hello world"), replacement)
}
}
Loading

0 comments on commit 2a2ecc9

Please sign in to comment.