Drop escaping of strings in the JSON.

Add support for the old escaping mechanism through an annotation on the
TriggerTemplate.

This removes the old replacement of " with \" in parameters, which was yielding
invalid JSON when the strings were already quoted.

docs/triggertemplates.md

@@ -120,3 +120,42 @@ As of Tekton Pipelines version
 embed resource specs. It is a best practice to embed each resource specs in the
 PipelineRun or TaskRun that uses the resource spec. Embedding the resource spec
 avoids a race condition between creating and using resources.
+
+## Templating Params
+
+When templating parameters into resources, a simple replacement on the string
+with the parameter name e.g. `$(tt.params.name)` is carried out.
+
+This means that for simple string / number values, replacements in the
+YAML should work fine.
+
+If the string could begin with a number e.g. `012abcd`, it might be misinterpreted by YAML as a
+number, which could cause an error, in which case you can put quotes around the
+templated parameter key, and it should solve the problem.
+
+## Escaping quoted strings.
+
+TriggerTemplate parameter values were previously escaped by simply replacing
+`"` with `\"` this could lead to problems when strings were already escaped, and
+generating invalid resources from the TriggerTemplate.
+
+This behaviour has been deprecated, if this breaks your templates, you can add
+an annotation to the TriggerTemplate.
+
+No escaping is done on the templated variables, if you are inserting a JSON
+object as a template var, then you should not put it within a quoted string.
+
+```yaml
+apiVersion: triggers.tekton.dev/v1alpha1
+kind: TriggerTemplate
+metadata:
+  name: escaped-tt
+  annotations:
+    tekton.dev/old-escape-quotes: "true"
+spec:
+  params:
+  - name: gitrevision
+    description: The git revision
+```
+
+This will retain the previous behaviour.

pkg/template/event.go

@@ -22,9 +22,20 @@ import (
 	"net/http"
 	"strings"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
 )
 
+const (
+	// OldEscapeAnnotation is used to determine whether or not a TriggerTemplate
+	// should retain the old "replace quotes with backslack quote" behaviour
+	// when templating in params.
+	//
+	// This can be removed when this functionality is no-longer needed.
+	OldEscapeAnnotation = "triggers.tekton.dev/old-escape-quotes"
+)
+
 // ResolveParams takes given triggerbindings and produces the resulting
 // resource params.
 func ResolveParams(rt ResolvedTrigger, body []byte, header http.Header) ([]triggersv1.Param, error) {
@@ -45,8 +56,11 @@ func ResolveParams(rt ResolvedTrigger, body []byte, header http.Header) ([]trigg
 func ResolveResources(template *triggersv1.TriggerTemplate, params []triggersv1.Param) []json.RawMessage {
 	resources := make([]json.RawMessage, len(template.Spec.ResourceTemplates))
 	uid := UID()
+
+	oldEscape := metav1.HasAnnotation(template.ObjectMeta, OldEscapeAnnotation)
+
 	for i := range template.Spec.ResourceTemplates {
-		resources[i] = applyParamsToResourceTemplate(params, template.Spec.ResourceTemplates[i].RawExtension.Raw)
+		resources[i] = applyParamsToResourceTemplate(params, template.Spec.ResourceTemplates[i].RawExtension.Raw, oldEscape)
 		resources[i] = applyUIDToResourceTemplate(resources[i], uid)
 	}
 	return resources

pkg/template/event_test.go

@@ -427,6 +427,13 @@ func TestResolveParams_Error(t *testing.T) {
 	}
 }
 
+func addOldEscape(t *triggersv1.TriggerTemplate) *triggersv1.TriggerTemplate {
+	t.Annotations = map[string]string{
+		OldEscapeAnnotation: "yes",
+	}
+	return t
+}
+
 func TestResolveResources(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -498,7 +505,7 @@ func TestResolveResources(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Seeded for UID() to return "cbhtc"
 			utilrand.Seed(0)
-			got := ResolveResources(tt.template, tt.params)
+			got := ResolveResources(addOldEscape(tt.template), tt.params)
 			// Use toString so that it is easy to compare the json.RawMessage diffs
 			if diff := cmp.Diff(toString(tt.want), toString(got)); diff != "" {
 				t.Errorf("didn't get expected resource template -want + got: %s", diff)

pkg/template/resource.go

@@ -121,24 +121,26 @@ func resolveBindingsToParams(bindings []*triggersv1.TriggerSpecBinding, getTB ge
 
 // applyParamsToResourceTemplate returns the TriggerResourceTemplate with the
 // param values substituted for all matching param variables in the template
-func applyParamsToResourceTemplate(params []triggersv1.Param, rt json.RawMessage) json.RawMessage {
+func applyParamsToResourceTemplate(params []triggersv1.Param, rt json.RawMessage, oldEscape bool) json.RawMessage {
 	// Assume the params are valid
 	for _, param := range params {
-		rt = applyParamToResourceTemplate(param, rt)
+		rt = applyParamToResourceTemplate(param, rt, oldEscape)
 	}
 	return rt
 }
 
 // applyParamToResourceTemplate returns the TriggerResourceTemplate with the
 // param value substituted for all matching param variables in the template
-func applyParamToResourceTemplate(param triggersv1.Param, rt json.RawMessage) json.RawMessage {
+func applyParamToResourceTemplate(param triggersv1.Param, rt json.RawMessage, oldEscape bool) json.RawMessage {
 	// Assume the param is valid
 	paramVariable := fmt.Sprintf("$(tt.params.%s)", param.Name)
 	// Escape quotes so that that JSON strings can be appended to regular strings.
 	// See #257 for discussion on this behavior.
-	paramValue := strings.Replace(param.Value, `"`, `\"`, -1)
-	rt = bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1)
-	return rt
+	if oldEscape {
+		paramValue := strings.Replace(param.Value, `"`, `\"`, -1)
+		return bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1)
+	}
+	return bytes.Replace(rt, []byte(paramVariable), []byte(param.Value), -1)
 }
 
 // UID generates a random string like the Kubernetes apiserver generateName metafield postfix.

pkg/template/resource_test.go

@@ -45,15 +45,18 @@ func Test_applyParamToResourceTemplate(t *testing.T) {
 		wantRtOneParamVar         = json.RawMessage(`{"foo": "bar-onevalue-bar"}`)
 		rtMultipleParamVars       = json.RawMessage(`{"$(tt.params.oneid)": "bar-$(tt.params.oneid)-$(tt.params.oneid)$(tt.params.oneid)$(tt.params.oneid)-$(tt.params.oneid)-bar"}`)
 		wantRtMultipleParamVars   = json.RawMessage(`{"onevalue": "bar-onevalue-onevalueonevalueonevalue-onevalue-bar"}`)
+		quotedString              = `this is a \"quoted\" string`
+		quotedValue               = `{"a": "this is a \"quoted\" string"}`
 	)
 	type args struct {
 		param triggersv1.Param
 		rt    json.RawMessage
 	}
 	tests := []struct {
-		name string
-		args args
-		want json.RawMessage
+		name      string
+		args      args
+		want      json.RawMessage
+		oldEscape bool
 	}{
 		{
 			name: "replace no param vars",
@@ -87,23 +90,73 @@ func Test_applyParamToResourceTemplate(t *testing.T) {
 			},
 			want: wantRtMultipleParamVars,
 		}, {
-			name: "espcae quotes in param val",
+			name: "escape quotes in param val",
+			args: args{
+				param: triggersv1.Param{
+					Name:  "p1",
+					Value: `{"a":"b"}`,
+				},
+				rt: json.RawMessage(`{"foo": $(tt.params.p1)}`),
+			},
+			want: json.RawMessage(`{"foo": {"a":"b"}}`),
+		}, {
+			name: "escape quotes in param val - old escaping",
 			args: args{
 				param: triggersv1.Param{
 					Name:  "p1",
 					Value: `{"a":"b"}`,
 				},
 				rt: json.RawMessage(`{"foo": "$(tt.params.p1)"}`),
 			},
-			want: json.RawMessage(`{"foo": "{\"a\":\"b\"}"}`),
+			want:      json.RawMessage(`{"foo": "{\"a\":\"b\"}"}`),
+			oldEscape: true,
+		}, {
+			name: "escape string with quoted message inside",
+			args: args{
+				param: triggersv1.Param{
+					Name:  "p1",
+					Value: quotedString,
+				},
+				rt: json.RawMessage(`{"foo": "$(tt.params.p1)"}`),
+			},
+			want: json.RawMessage(`{"foo": "this is a \"quoted\" string"}`),
+		}, {
+			name: "join string with quoted message",
+			args: args{
+				param: triggersv1.Param{
+					Name:  "p1",
+					Value: quotedString,
+				},
+				rt: json.RawMessage(`{"foo": "bar-$(tt.params.p1)-bar"}`),
+			},
+			want: json.RawMessage(`{"foo": "bar-this is a \"quoted\" string-bar"}`),
+		}, {
+			name: "escape string with object with quoted string",
+			args: args{
+				param: triggersv1.Param{
+					Name:  "p1",
+					Value: quotedValue,
+				},
+				rt: json.RawMessage(`{"foo": $(tt.params.p1)}`),
+			},
+			want: json.RawMessage(`{"foo": {"a": "this is a \"quoted\" string"}}`),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := applyParamToResourceTemplate(tt.args.param, tt.args.rt)
-			if diff := cmp.Diff(tt.want, got); diff != "" {
+			temp := map[string]interface{}{}
+			if err := json.Unmarshal(tt.want, &temp); err != nil {
+				t.Errorf("the wanted value is not valid JSON: %s", err)
+			}
+			got := applyParamToResourceTemplate(tt.args.param, tt.args.rt, tt.oldEscape)
+			if diff := cmp.Diff(string(tt.want), string(got)); diff != "" {
 				t.Errorf("applyParamToResourceTemplate(): -want +got: %s", diff)
 			}
+			if !tt.oldEscape {
+				if err := json.Unmarshal(got, &temp); err != nil {
+					t.Errorf("failed to parse result json %s: %s", got, err)
+				}
+			}
 		})
 	}
 }
@@ -116,9 +169,10 @@ func Test_ApplyParamsToResourceTemplate(t *testing.T) {
 		rt     json.RawMessage
 	}
 	tests := []struct {
-		name string
-		args args
-		want json.RawMessage
+		name      string
+		args      args
+		oldEscape bool
+		want      json.RawMessage
 	}{
 		{
 			name: "no params",
@@ -138,6 +192,17 @@ func Test_ApplyParamsToResourceTemplate(t *testing.T) {
 			},
 			want: json.RawMessage(`{"oneparam": "onevalue", "twoparam": "$(tt.params.twoid)", "threeparam": "$(tt.params.threeid)"`),
 		},
+		{
+			name: "old escape behaviour",
+			args: args{
+				params: []triggersv1.Param{
+					{Name: "oneid", Value: "this \"is a value\""},
+				},
+				rt: rt,
+			},
+			want:      json.RawMessage(`{"oneparam": "this \"is a value\"", "twoparam": "$(tt.params.twoid)", "threeparam": "$(tt.params.threeid)"`),
+			oldEscape: true,
+		},
 		{
 			name: "multiple params",
 			args: args{
@@ -165,9 +230,9 @@ func Test_ApplyParamsToResourceTemplate(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := applyParamsToResourceTemplate(tt.args.params, tt.args.rt)
-			if diff := cmp.Diff(tt.want, got); diff != "" {
-				t.Errorf("applyParamsToResourceTemplate(): -want +got: %s", diff)
+			got := applyParamsToResourceTemplate(tt.args.params, tt.args.rt, tt.oldEscape)
+			if diff := cmp.Diff(string(tt.want), string(got)); diff != "" {
+				t.Errorf("applyParamsToResourceTemplate(): -want +got: %s\n%s\n", diff, string(got))
 			}
 		})
 	}

test/eventlistener_test.go

@@ -62,13 +62,14 @@ const (
 	examplePRJsonFilename = "pr.json"
 )
 
-func loadExamplePREventBytes() ([]byte, error) {
+func loadExamplePREventBytes(t *testing.T) []byte {
+	t.Helper()
 	path := filepath.Join("testdata", examplePRJsonFilename)
 	bytes, err := ioutil.ReadFile(path)
 	if err != nil {
-		return nil, fmt.Errorf("couldn't load testdata example PullRequest event data: %v", err)
+		t.Fatalf("Couldn't load test data example PullREquest event data: %v", err)
 	}
-	return bytes, nil
+	return bytes
 }
 
 func impersonateRBAC(t *testing.T, sa, namespace string, kubeClient kubernetes.Interface) {
@@ -182,6 +183,9 @@ func TestEventListenerCreate(t *testing.T) {
 	// TriggerTemplate
 	tt, err := c.TriggersClient.TriggersV1alpha1().TriggerTemplates(namespace).Create(context.Background(),
 		bldr.TriggerTemplate("my-triggertemplate", "",
+			bldr.TriggerTemplateMeta(
+				bldr.Label("triggers.tekton.dev/old-escape-quotes", "true"),
+			),
 			bldr.TriggerTemplateSpec(
 				bldr.TriggerTemplateParam("oneparam", "", ""),
 				bldr.TriggerTemplateParam("twoparamname", "", ""),
@@ -321,10 +325,7 @@ func TestEventListenerCreate(t *testing.T) {
 	t.Log("EventListener is ready")
 
 	// Load the example pull request event data
-	eventBodyJSON, err := loadExamplePREventBytes()
-	if err != nil {
-		t.Fatalf("Couldn't load test data: %v", err)
-	}
+	eventBodyJSON := loadExamplePREventBytes(t)
 
 	// Event body & Expected ResourceTemplates after instantiation
 	wantPr1 := v1alpha1.PipelineResource{