From 3e8c705021b4a9f15e91fe54d69a344143d7d457 Mon Sep 17 00:00:00 2001 From: Kevin McDermott Date: Wed, 21 Oct 2020 13:59:36 +0100 Subject: [PATCH] Drop escaping of strings in the JSON. Add support for the old escaping mechanism through an annotation on the TriggerTemptlate. This removes the old replacement of " with \" in parameters, which was yielding invalid JSON when the strings were already quoted. --- docs/triggertemplates.md | 25 ++++++++++ pkg/template/event.go | 11 ++++- pkg/template/event_test.go | 9 +++- pkg/template/resource.go | 14 +++--- pkg/template/resource_test.go | 91 ++++++++++++++++++++++++++++++----- test/eventlistener_test.go | 3 ++ 6 files changed, 132 insertions(+), 21 deletions(-) diff --git a/docs/triggertemplates.md b/docs/triggertemplates.md index 43811fb218..920033b892 100644 --- a/docs/triggertemplates.md +++ b/docs/triggertemplates.md @@ -120,3 +120,28 @@ 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. + + +## Quote-Escaping + +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. + +```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. diff --git a/pkg/template/event.go b/pkg/template/event.go index d44ffaf96d..0868e1b46b 100644 --- a/pkg/template/event.go +++ b/pkg/template/event.go @@ -22,9 +22,15 @@ import ( "net/http" "strings" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" ) +const ( + OldEscapeAnnotation = "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 +51,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 diff --git a/pkg/template/event_test.go b/pkg/template/event_test.go index 0af4a73ee5..d5f5eb7bcc 100644 --- a/pkg/template/event_test.go +++ b/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) diff --git a/pkg/template/resource.go b/pkg/template/resource.go index 559ab36464..3419bdd900 100644 --- a/pkg/template/resource.go +++ b/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. diff --git a/pkg/template/resource_test.go b/pkg/template/resource_test.go index a124428a96..62dc276d4e 100644 --- a/pkg/template/resource_test.go +++ b/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,7 +90,17 @@ 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", @@ -95,15 +108,55 @@ func Test_applyParamToResourceTemplate(t *testing.T) { }, 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)) } }) } diff --git a/test/eventlistener_test.go b/test/eventlistener_test.go index 5e60f5e0a8..07027c8f9e 100644 --- a/test/eventlistener_test.go +++ b/test/eventlistener_test.go @@ -182,6 +182,9 @@ func TestEventListenerCreate(t *testing.T) { // TriggerTemplate tt, err := c.TriggersClient.TriggersV1alpha1().TriggerTemplates(namespace).Create(context.Background(), bldr.TriggerTemplate("my-triggertemplate", "", + bldr.EventListenerMeta( + bldr.Label("tekton.dev/old-escape-quotes", "true"), + ), bldr.TriggerTemplateSpec( bldr.TriggerTemplateParam("oneparam", "", ""), bldr.TriggerTemplateParam("twoparamname", "", ""),