Values injected into the template should be escaped #258

bobcatfish · 2019-12-09T18:09:13Z

Expected Behavior

It should be possible for me to pass along the body of a github pull request event to a TaskRun + Task.

Actual Behavior

When I'm trying to trigger from a pull request and pass along the description of the pull request to a Task, I run into trouble b/c the description contains characters like \r and \n and I can't figure out how to escape this in the template properly :S

Steps to Reproduce the Problem

Example trigger binding I was using:

---
apiVersion: tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: bobcatfish-review-pipelinebinding
spec:
  params:
    - name: gitrevision
      value: $(body.pull_request.head.sha)
    - name: gitrepositoryurl
      value: "https://github.com/$(body.repository.full_name)"
    - name: pullrequesturl
      value: $(body.pull_request.html_url)
    - name: body
      value: $(body.pull_request.body) # <-- this is where im getting the description of the pull request

Here is an example of the body field of the pull request:

"body": "# Changes\r\n\r\nHI THERE HAHAHA HAHAHA\r\n\r\n# Submitter Checklist\r\n\r\nThese are the criteria that every PR should meet, please check them off as you\r\nreview them:\r\n\r\n- [ ] Includes [tests](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if functionality changed/added)\r\n- [ ] Includes [docs](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if user facing)\r\n- [ ] Commit messages follow [commit message best practices](https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)\r\n\r\n_See [the contribution guide](https://github.com/tektoncd/pipeline/blob/master/CONTRIBUTING.md) for more details._\r\n\r\nDouble check this list of stuff that's easy to miss:\r\n\r\n- If you are adding [a new binary/image to the `cmd` dir](../cmd), please update\r\n  [the release Task](../tekton/publish.yaml) to build and release this image.\r\n\r\n## Reviewer Notes\r\n\r\nIf [API changes](https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md) are included, [additive changes](https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md#additive-changes) must be approved by at least two [OWNERS](https://github.com/tektoncd/pipeline/blob/master/OWNERS) and [backwards incompatible changes](https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md#backwards-incompatible-changes) must be approved by [more than 50% of the OWNERS](https://github.com/tektoncd/pipeline/blob/master/OWNERS), and they must first be added [in a backwards compatible way](https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md#backwards-compatible-changes-first).\r\n\r\n# Release Notes\r\n\r\n```\r\nDescribe any user facing changes here, or delete this block.\r\n\r\nExamples of user facing changes:\r\n- API changes\r\n- Bug fixes\r\n- Any changes in behavior\r\n\r\n```\r\n",

Here is the template I was trying to use:

apiVersion: tekton.dev/v1alpha1
kind: TriggerTemplate
metadata:
  name: bobcatfish-review-triggertemplate
spec:
  params:
    - name: gitrevision
      description: The git revision
      default: master
    - name: gitrepositoryurl
      description: The git repository url
    - name: pullrequesturl
      description: The url of the pull reuqest
    - name: body
  resourcetemplates:
    - apiVersion: tekton.dev/v1alpha1
      kind: PipelineResource
      metadata:
        name: pr-$(uid)
      spec:
        type: pullRequest
        params:
        - name: url
          value: $(params.pullrequesturl)
        secrets:
        - secretName: webhook-secret
          secretKey: token
          fieldName: githubToken
    - apiVersion: tekton.dev/v1alpha1
      kind: PipelineResource
      metadata:
        name: source-repo-$(uid)
      spec:
        type: git
        params:
        - name: revision
          value: $(params.gitrevision)
        - name: url
          value: $(params.gitrepositoryurl)
    - apiVersion: tekton.dev/v1alpha1
      kind: TaskRun
      metadata:
        name: tr-bobcatfish-review-$(uid)
      spec:
        taskRef:
          name: release-notes
        inputs:
          params:
          - name: message
            value: $(params.body) # <-- this is where it all falls apart
          resources:
            - name: repo
              resourceRef:
                name: source-repo-$(uid)

I end up with an error like:

"the object provided is unrecognized (must be of type TaskRun): couldn't get version/kind; json parse error: invalid character '\r' in string literal ({"apiVersion":"tekton.dev/v1al ...)"

Additional Info

I can't tell if this is a bug or (I'm hoping!) there is some way I can specify the $(params.body) in the trigger template such that the values being passed along can be properly escaped.

The text was updated successfully, but these errors were encountered:

ncskier · 2019-12-09T18:51:19Z

Unfortunately, I think that this is a bug. This is related to issue #257.

However, it might be possible to get around this by escaping the body in the interceptor, and passing the escaped body out of the interceptor?

bobcatfish · 2019-12-09T20:11:06Z

However, it might be possible to get around this by escaping the body in the interceptor, and passing the escaped body out of the interceptor?

Oh interesting! Maybe that'll work 🤔 that means I'd have to write it in a service tho - i feel like fixing it might be easier XD. This was for a POC I was putting together, if the bug's still around when i get back to it I'll try that out @ncskier :pray :D

Just like with reconcilers / controllers, adding functions of as members of the Sink (or controller) object itself has some downsides: it's hard to tell what the responsibilities of the functions are and how they are coupled to the Sink, and it's harder to write more focused unit tests. I am working on tektoncd#258 in another branch and in that branch I wanted to make some updates to the unit tests for this logic; it's easier to do if it's factored out a bit. There is more factoring we can do here: now that the functions are moved out of Sink we can make decisions around their interfaces and maybe make them even more focused and unit testable. (Though it was a happy surprise to see they were unit tested even though they were members of Sink!)

Just like with reconcilers / controllers, adding functions of as members of the Sink (or controller) object itself has some downsides: it's hard to tell what the responsibilities of the functions are and how they are coupled to the Sink, and it's harder to write more focused unit tests. I am working on tektoncd#258 in another branch and in that branch I wanted to make some updates to the unit tests for this logic; it's easier to do if it's factored out a bit. There is more factoring we can do here: now that the functions are moved out of Sink we can make decisions around their interfaces and maybe make them even more focused and unit testable. (Though it was a happy surprise to see they were unit tested even though they were members of Sink!) Also change usage of xerrors to fmt.Errorf since we're using >= go 1.13 now and we get the error wrapping for free :D

Just like with reconcilers / controllers, adding functions of as members of the Sink (or controller) object itself has some downsides: it's hard to tell what the responsibilities of the functions are and how they are coupled to the Sink, and it's harder to write more focused unit tests. I am working on #258 in another branch and in that branch I wanted to make some updates to the unit tests for this logic; it's easier to do if it's factored out a bit. There is more factoring we can do here: now that the functions are moved out of Sink we can make decisions around their interfaces and maybe make them even more focused and unit testable. (Though it was a happy surprise to see they were unit tested even though they were members of Sink!) Also change usage of xerrors to fmt.Errorf since we're using >= go 1.13 now and we get the error wrapping for free :D

Just like with reconcilers / controllers, adding functions of as members of the Sink (or controller) object itself has some downsides: it's hard to tell what the responsibilities of the functions are and how they are coupled to the Sink, and it's harder to write more focused unit tests. I am working on tektoncd#258 in another branch and in that branch I wanted to make some updates to the unit tests for this logic; it's easier to do if it's factored out a bit. There is more factoring we can do here: now that the functions are moved out of Sink we can make decisions around their interfaces and maybe make them even more focused and unit testable. (Though it was a happy surprise to see they were unit tested even though they were members of Sink!)

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. Fixes tektoncd#258 Co-authored-by: Dibyo Mukherjee <[email protected]>

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]>

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 #258 Co-authored-by: Dibyo Mukherjee <[email protected]>

ncskier added kind/bug Categorizes issue or PR as related to a bug. kind/question Issues or PRs that are questions around the project or a particular feature labels Dec 9, 2019

ncskier removed the kind/question Issues or PRs that are questions around the project or a particular feature label Dec 10, 2019

dibyom mentioned this issue Dec 10, 2019

JSONPath Support for TriggerBindings #241

Merged

3 tasks

bobcatfish mentioned this issue Dec 12, 2019

Refactor functions to not be members of Sink 🚰 #274

Merged

2 tasks

bobcatfish self-assigned this Dec 13, 2019

bobcatfish mentioned this issue Dec 16, 2019

Preserve escaped characters when parsing json path 🦟 #279

Merged

2 tasks

tekton-robot closed this as completed in #279 Dec 20, 2019

dibyom mentioned this issue Apr 8, 2020

Add example to print entire request body $(body) #487

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Values injected into the template should be escaped #258

Values injected into the template should be escaped #258

bobcatfish commented Dec 9, 2019 •

edited

Loading

ncskier commented Dec 9, 2019

bobcatfish commented Dec 9, 2019

Values injected into the template should be escaped #258

Values injected into the template should be escaped #258

Comments

bobcatfish commented Dec 9, 2019 • edited Loading

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Additional Info

ncskier commented Dec 9, 2019

bobcatfish commented Dec 9, 2019

bobcatfish commented Dec 9, 2019 •

edited

Loading