fix(controller): fix tricky expr evaluation cases dealing with quotes…

… and newlines (#3170)

Signed-off-by: Kent Rancourt <[email protected]>
(cherry picked from commit afae9dd)

internal/expressions/json_templates.go

@@ -100,6 +100,13 @@ func evaluateExpressions(collection any, env map[string]any, exprOpts ...expr.Op
 // environment. Note that a single template string can contain multiple
 // expressions.
 func EvaluateTemplate(template string, env map[string]any, exprOpts ...expr.Option) (any, error) {
+	if !strings.Contains(template, "${{") {
+		// Don't do anything fancy if the "template" doesn't contain any
+		// expressions. If we did, a simple string like "42" would be evaluated as
+		// the number 42. That would force users to use ${{ quote(42) }} when it
+		// would be more intuitive to just use "42".
+		return template, nil
+	}
 	if exprOpts == nil {
 		exprOpts = make([]expr.Option, 0, 1)
 	}
@@ -122,7 +129,15 @@ func EvaluateTemplate(template string, env map[string]any, exprOpts ...expr.Opti
 	if _, err := t.ExecuteFunc(out, getExpressionEvaluator(env, exprOpts...)); err != nil {
 		return nil, err
 	}
+	// If there is a trailing newline, remove it. If the | operator was used in
+	// the original YAML, the result will have a trailing newline, which can
+	// cause problems with the logic that follows.
 	result := out.String()
+	var removedNewline bool
+	if strings.HasSuffix(result, "\n") {
+		result = strings.TrimSuffix(out.String(), "\n")
+		removedNewline = true
+	}
 	// If the result is enclosed in quotes, this is probably the result of an
 	// expression that deliberately enclosed the result in quotes to prevent it
 	// from being mistaken for a number, bool, etc. e.g. ${{ quote(true) }}
@@ -134,7 +149,11 @@ func EvaluateTemplate(template string, env map[string]any, exprOpts ...expr.Opti
 	// which we are using this function is so low that it's not worth sacrificing
 	// the convenience of this behavior.
 	if len(result) > 1 && strings.HasPrefix(result, `"`) && strings.HasSuffix(result, `"`) {
-		return result[1 : len(result)-1], nil
+		result = result[1 : len(result)-1]
+		if removedNewline {
+			result += "\n"
+		}
+		return result, nil
 	}
 	// If the result is parseable as a bool return that.
 	if resBool, err := strconv.ParseBool(result); err == nil {
@@ -151,6 +170,9 @@ func EvaluateTemplate(template string, env map[string]any, exprOpts ...expr.Opti
 		return resMap, nil
 	}
 	// If we get to here, just return the string.
+	if removedNewline {
+		result += "\n"
+	}
 	return result, nil
 }
 

internal/expressions/json_templates_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"sigs.k8s.io/yaml"
 )
 
 func TestEvaluateJSONTemplate(t *testing.T) {
@@ -44,6 +45,7 @@ func TestEvaluateJSONTemplate(t *testing.T) {
 	testCases := []struct {
 		name         string
 		jsonTemplate string
+		yamlTemplate string
 		assertions   func(t *testing.T, jsonOutput []byte, err error)
 	}{
 		{
@@ -218,10 +220,59 @@ func TestEvaluateJSONTemplate(t *testing.T) {
 				require.Equal(t, `{"foo":"bar"}`, parsed.AString)
 			},
 		},
+		{
+			name: "a variety of tricky cases dealing with YAML and quotes",
+			yamlTemplate: `
+value1: {"foo": "bar"} # This is a JSON object
+value2: '${{ quote({"foo": "bar"}) }}' # This is a string
+value3: | # This is a string
+  {"foo": "bar"}
+value4: | # This is a JSON object
+  ${{ {"foo": "bar"} }}
+value5: | # This is a string
+  ${{ quote({"foo": "bar"}) }}
+value6: | # Make sure we're not tripped up by multiple newlines
+  ${{ quote({"foo": "bar"}) }}
+  
+value7: 42 # This is a number
+value8: "42" # This is a string
+value9: '42'	# This is a string
+value10: '${{ quote(42) }}' # This is a string
+value11: | # This is a string
+  42
+value12: | # This is a number
+  ${{ 42 }}
+value13: | # This is a string
+  ${{ quote(42) }}
+`,
+			assertions: func(t *testing.T, jsonOutput []byte, err error) {
+				require.NoError(t, err)
+				parsed := map[string]any{}
+				require.NoError(t, json.Unmarshal(jsonOutput, &parsed))
+				require.Equal(t, map[string]any{"foo": "bar"}, parsed["value1"])
+				require.Equal(t, `{"foo":"bar"}`, parsed["value2"])
+				require.Equal(t, "{\"foo\": \"bar\"}\n", parsed["value3"])
+				require.Equal(t, map[string]any{"foo": "bar"}, parsed["value4"])
+				require.Equal(t, "{\"foo\":\"bar\"}\n", parsed["value5"])
+				require.Equal(t, "{\"foo\":\"bar\"}\n", parsed["value6"])
+				require.Equal(t, float64(42), parsed["value7"])
+				require.Equal(t, "42", parsed["value8"])
+				require.Equal(t, "42", parsed["value9"])
+				require.Equal(t, "42", parsed["value10"])
+				require.Equal(t, "42\n", parsed["value11"])
+				require.Equal(t, float64(42), parsed["value12"])
+				require.Equal(t, "42\n", parsed["value13"])
+			},
+		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
+			if testCase.yamlTemplate != "" {
+				jsonBytes, err := yaml.YAMLToJSON([]byte(testCase.yamlTemplate))
+				require.NoError(t, err)
+				testCase.jsonTemplate = string(jsonBytes)
+			}
 			jsonOutput, err := EvaluateJSONTemplate([]byte(testCase.jsonTemplate), testEnv)
 			testCase.assertions(t, jsonOutput, err)
 		})