Preserve escaped characters when parsing json path 🦟

I was trying to use Triggers to deliver a payload that included some
newlines and carriage returns into the value of a Pipeline param, but
along the way they were being turned from actual \ + n to the newline
character, etc., which when embedded into the yaml of the CRD I wanted
to create was invalid.

Now we will treat json strings as json when using the values via json
path instead of decoding the the escaped characters.

Dropping the quotation marks seems like a pretty big hack but it's the
best option we could think of and seems to consistently give us the
results we'd expect as users.

In the end to end test, when json dictionaries are passed through an
EventListener, the order may change, so we are deserializing the example
data so that we can compare it in an order agnostic manner.

Fixes tektoncd#258

Co-authored-by: Dibyo Mukherjee <[email protected]>

pkg/resources/create.go

@@ -63,7 +63,7 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
 	// Assume the TriggerResourceTemplate is valid (it has an apiVersion and Kind)
 	data := new(unstructured.Unstructured)
 	if err := data.UnmarshalJSON(rt); err != nil {
-		return err
+		return fmt.Errorf("couldn't unmarshal json: %v", err)
 	}
 
 	data = AddLabels(data, map[string]string{
@@ -81,7 +81,7 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
 	// Resolve resource kind to the underlying API Resource type.
 	apiResource, err := FindAPIResource(data.GetAPIVersion(), data.GetKind(), c)
 	if err != nil {
-		return err
+		return fmt.Errorf("couldn't find API resource for json: %v", err)
 	}
 
 	name := data.GetName()
@@ -96,8 +96,12 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
 		Resource: apiResource.Name,
 	}
 
-	_, err = dc.Resource(gvr).Namespace(namespace).Create(data, metav1.CreateOptions{})
-	return err
+	logger.Infof("For event ID %q creating resource %v", eventID, gvr)
+
+	if _, err := dc.Resource(gvr).Namespace(namespace).Create(data, metav1.CreateOptions{}); err != nil {
+		return fmt.Errorf("couldn't create resource with group version kind %q: %v", gvr, err)
+	}
+	return nil
 }
 
 // AddLabels adds autogenerated Tekton labels to created resources.

pkg/resources/create_test.go

@@ -140,22 +140,12 @@ func TestCreateResource(t *testing.T) {
 	logger, _ := logging.NewLogger("", "")
 
 	tests := []struct {
-		name     string
-		resource pipelinev1.PipelineResource
-		want     pipelinev1.PipelineResource
+		name string
+		json []byte
+		want pipelinev1.PipelineResource
 	}{{
 		name: "PipelineResource without namespace",
-		resource: pipelinev1.PipelineResource{
-			TypeMeta: metav1.TypeMeta{
-				APIVersion: "tekton.dev/v1alpha1",
-				Kind:       "PipelineResource",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Name:   "my-pipelineresource",
-				Labels: map[string]string{"woriginal-label-1": "label-1"},
-			},
-			Spec: pipelinev1.PipelineResourceSpec{},
-		},
+		json: json.RawMessage(`{"kind":"PipelineResource","apiVersion":"tekton.dev/v1alpha1","metadata":{"name":"my-pipelineresource","creationTimestamp":null,"labels":{"woriginal-label-1":"label-1"}},"spec":{"type":"","params":[{"name":"foo","value":"bar\r\nbaz"}]},"status":{}}`),
 		want: pipelinev1.PipelineResource{
 			TypeMeta: metav1.TypeMeta{
 				APIVersion: "tekton.dev/v1alpha1",
@@ -170,22 +160,16 @@ func TestCreateResource(t *testing.T) {
 					eventIDLabel:        eventID,
 				},
 			},
-			Spec: pipelinev1.PipelineResourceSpec{},
+			Spec: pipelinev1.PipelineResourceSpec{
+				Params: []pipelinev1.ResourceParam{{
+					Name:  "foo",
+					Value: "bar\r\nbaz",
+				}},
+			},
 		},
 	}, {
 		name: "PipelineResource with namespace",
-		resource: pipelinev1.PipelineResource{
-			TypeMeta: metav1.TypeMeta{
-				APIVersion: "tekton.dev/v1alpha1",
-				Kind:       "PipelineResource",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "foo",
-				Name:      "my-pipelineresource",
-				Labels:    map[string]string{"woriginal-label-1": "label-1"},
-			},
-			Spec: pipelinev1.PipelineResourceSpec{},
-		},
+		json: json.RawMessage(`{"kind":"PipelineResource","apiVersion":"tekton.dev/v1alpha1","metadata":{"name":"my-pipelineresource","namespace":"foo","creationTimestamp":null,"labels":{"woriginal-label-1":"label-1"}},"spec":{"type":"","params":null},"status":{}}`),
 		want: pipelinev1.PipelineResource{
 			TypeMeta: metav1.TypeMeta{
 				APIVersion: "tekton.dev/v1alpha1",
@@ -207,12 +191,7 @@ func TestCreateResource(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			dynamicClient.ClearActions()
-
-			b, err := json.Marshal(tt.resource)
-			if err != nil {
-				t.Fatalf("error marshalling resource: %v", tt.resource)
-			}
-			if err := Create(logger, b, triggerName, eventID, elName, elNamespace, kubeClient.Discovery(), dynamicSet); err != nil {
+			if err := Create(logger, tt.json, triggerName, eventID, elName, elNamespace, kubeClient.Discovery(), dynamicSet); err != nil {
 				t.Errorf("createResource() returned error: %s", err)
 			}
 

pkg/sink/sink.go

@@ -128,10 +128,10 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) {
 				result <- http.StatusAccepted
 				return
 			}
-			log.Info("params: %+v", params)
+			log.Infof("params: %+v", params)
 			res := template.ResolveResources(rt.TriggerTemplate, params)
 			if err := r.createResources(res, t.Name, eventID); err != nil {
-				log.Error(err)
+				log.Errorf("Could not create resources for %q: %v", t.Name, err)
 			}
 			result <- http.StatusCreated
 		}(t)

pkg/sink/sink_test.go

@@ -24,6 +24,7 @@ import (
 	"net/http/httptest"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
@@ -56,9 +57,24 @@ func init() {
 	template.UID = func() string { return eventID }
 }
 
+func waitTimeout(wg *sync.WaitGroup, timeout time.Duration) bool {
+	c := make(chan struct{})
+	go func() {
+		defer close(c)
+		wg.Wait()
+	}()
+	select {
+	case <-c:
+		return false
+	case <-time.After(timeout):
+		return true
+	}
+}
+
 func TestHandleEvent(t *testing.T) {
 	namespace := "foo"
-	eventBody := json.RawMessage(`{"head_commit": {"id": "testrevision"}, "repository": {"url": "testurl"}}`)
+	eventBody := json.RawMessage(`{"head_commit": {"id": "testrevision"}, "repository": {"url": "testurl"}, "foo": "bar\t\r\nbaz昨"}`)
+
 	pipelineResource := pipelinev1.PipelineResource{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "tekton.dev/v1alpha1",
@@ -75,6 +91,7 @@ func TestHandleEvent(t *testing.T) {
 				{Name: "url", Value: "$(params.url)"},
 				{Name: "revision", Value: "$(params.revision)"},
 				{Name: "contenttype", Value: "$(params.contenttype)"},
+				{Name: "foo", Value: "$(params.foo)"},
 			},
 		},
 	}
@@ -89,13 +106,15 @@ func TestHandleEvent(t *testing.T) {
 			bldr.TriggerTemplateParam("revision", "", ""),
 			bldr.TriggerTemplateParam("appLabel", "", "foo"),
 			bldr.TriggerTemplateParam("contenttype", "", ""),
+			bldr.TriggerTemplateParam("foo", "", ""),
 			bldr.TriggerResourceTemplate(json.RawMessage(pipelineResourceBytes)),
 		))
 	tb := bldr.TriggerBinding("my-triggerbinding", namespace,
 		bldr.TriggerBindingSpec(
 			bldr.TriggerBindingParam("url", "$(body.repository.url)"),
 			bldr.TriggerBindingParam("revision", "$(body.head_commit.id)"),
 			bldr.TriggerBindingParam("contenttype", "$(header.Content-Type)"),
+			bldr.TriggerBindingParam("foo", "$(body.foo)"),
 		))
 	el := bldr.EventListener("my-eventlistener", namespace,
 		bldr.EventListenerSpec(bldr.EventListenerTrigger("my-triggerbinding", "my-triggertemplate", "v1alpha1")))
@@ -166,7 +185,11 @@ func TestHandleEvent(t *testing.T) {
 		t.Errorf("did not get expected response back -want,+got: %s", diff)
 	}
 
-	wg.Wait()
+	// We expect that the EventListener will be able to immediately handle the event so we
+	// can use a very short timeout
+	if waitTimeout(&wg, time.Second) {
+		t.Fatalf("timed out waiting for reactor to fire")
+	}
 	wantResource := pipelinev1.PipelineResource{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "tekton.dev/v1alpha1",
@@ -188,6 +211,7 @@ func TestHandleEvent(t *testing.T) {
 				{Name: "url", Value: "testurl"},
 				{Name: "revision", Value: "testrevision"},
 				{Name: "contenttype", Value: "application/json"},
+				{Name: "foo", Value: "bar\t\r\nbaz昨"},
 			},
 		},
 	}

pkg/template/event_test.go

@@ -21,13 +21,15 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
+	"github.com/tektoncd/triggers/test"
 	bldr "github.com/tektoncd/triggers/test/builder"
 	"k8s.io/apimachinery/pkg/util/rand"
 )
 
-// TODO(#252): Split NewResourcesTests into separate tests for ResolveParams and ResolveResources
+// TODO(#252): Split testcases from NewResourcesTests into TestResolveParams and TestResolveResources
 func Test_NewResources(t *testing.T) {
 	type args struct {
 		body    []byte
@@ -315,6 +317,56 @@ func Test_NewResources(t *testing.T) {
 	}
 }
 
+func TestResolveParams(t *testing.T) {
+	tests := []struct {
+		name     string
+		body     []byte
+		params   []pipelinev1.ParamSpec
+		bindings []*triggersv1.TriggerBinding
+		want     []pipelinev1.Param
+	}{{
+		name: "values with newlines",
+		body: json.RawMessage(`{"foo": "bar\r\nbaz"}`),
+		params: []pipelinev1.ParamSpec{{
+			Name: "param1",
+		}, {
+			Name: "param2",
+		}},
+		bindings: []*triggersv1.TriggerBinding{
+			bldr.TriggerBinding("tb", "namespace",
+				bldr.TriggerBindingSpec(
+					bldr.TriggerBindingParam("param1", "qux"),
+					bldr.TriggerBindingParam("param2", "$(body.foo)"),
+				),
+			),
+		},
+		want: []pipelinev1.Param{{
+			Name: "param1",
+			Value: pipelinev1.ArrayOrString{
+				StringVal: "qux",
+				Type:      pipelinev1.ParamTypeString,
+			},
+		}, {
+			Name: "param2",
+			Value: pipelinev1.ArrayOrString{
+				StringVal: "bar\\r\\nbaz",
+				Type:      pipelinev1.ParamTypeString,
+			},
+		}},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			params, err := ResolveParams(tt.bindings, tt.body, map[string][]string{}, tt.params)
+			if err != nil {
+				t.Fatalf("ResolveParams() returned unexpected error: %s", err)
+			}
+			if diff := cmp.Diff(tt.want, params, cmpopts.SortSlices(test.CompareParams)); diff != "" {
+				t.Errorf("didn't get expected params -want + got: %s", diff)
+			}
+		})
+	}
+}
+
 func convertJSONRawMessagesToString(rawMessages []json.RawMessage) []string {
 	stringMessages := make([]string, len(rawMessages))
 	for i := range rawMessages {

pkg/template/jsonpath.go

@@ -89,7 +89,16 @@ func textValue(v reflect.Value) ([]byte, error) {
 
 	switch t.Kind() {
 	// evalToText() returns <map> ....; return JSON string instead.
-	case reflect.Map, reflect.Slice:
+	case reflect.String:
+		b, err := json.Marshal(v.Interface())
+		if err != nil {
+			return nil, fmt.Errorf("unable to marshal string value %v: %v", v, err)
+		}
+		// A valid json string is surrounded by quotation marks; we are using this function to
+		// create a representation of the json value that can be embedded in a CRD definition and
+		// we want to leave it up to the user if they want the surrounding quotation marks or not.
+		return b[1 : len(b)-1], nil
+	case reflect.Map, reflect.Slice, reflect.Int:
 		return json.Marshal(v.Interface())
 	default:
 		return evalToText(v)

pkg/template/jsonpath_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-var objects = `{"a":"v","c":{"d":"e"},"empty": "","null": null, "number": 42}`
+var objects = `{"a":"v\r\n烈","c":{"d":"e"},"empty": "","null": null, "number": 42}`
 var arrays = `[{"a": "b"}, {"c": "d"}, {"e": "f"}]`
 
 // Checks that we print JSON strings when the JSONPath selects
@@ -41,12 +41,7 @@ func TestParseJSONPath(t *testing.T) {
 		name: "string values",
 		in:   objectBody,
 		expr: "$(body.a)",
-		want: "v",
-	}, {
-		name: "string values",
-		in:   objectBody,
-		expr: "$(body.a)",
-		want: "v",
+		want: "v\\r\\n烈",
 	}, {
 		name: "empty string",
 		in:   objectBody,

pkg/template/resource_test.go

@@ -25,16 +25,12 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
+	"github.com/tektoncd/triggers/test"
 	bldr "github.com/tektoncd/triggers/test/builder"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/rand"
 )
 
-// Allows us to sort Params by Name
-func compareParam(x, y pipelinev1.Param) bool {
-	return x.Name < y.Name
-}
-
 func Test_MergeInDefaultParams(t *testing.T) {
 	var (
 		oneParam = pipelinev1.Param{
@@ -114,7 +110,7 @@ func Test_MergeInDefaultParams(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got := MergeInDefaultParams(tt.args.params, tt.args.paramSpecs)
-			if diff := cmp.Diff(tt.want, got, cmpopts.SortSlices(compareParam)); diff != "" {
+			if diff := cmp.Diff(tt.want, got, cmpopts.SortSlices(test.CompareParams)); diff != "" {
 				t.Errorf("MergeInDefaultParams(): -want +got: %s", diff)
 			}
 		})