Escaping quotes in TriggerTemplates.

[bigkevmcd]

This is a proposal to allow escaping non-JSON friendly strings when rendering
TriggerTemplate resources.

[dibyom]

/kind tep

[dibyom]

/assign

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dibyom
You can assign the PR to them by writing /assign @dibyom in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• teps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dibyom]

Escape is a bit generic...what kind of escaping does it mean? I think the alternative where we have different "strategies" sounds a bit more descriptive.

[dibyom]

Another option I was discussing with @wlynch was having the escaping done at the TriggerBinding level not at the TriggerTemplate level. Is there ever a case where we extract a value that contains an escaped quote but then we'd sometimes want to escape it but not always?

[bigkevmcd]

I mulled this over a lot, both when implementing tektoncd/triggers#823 and writing this up.

I had thought about applying it to TriggerBindings something like this:

apiVersion: triggers.tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: pipeline-binding
spec:
  params:
  - name: gitrevision
    value: $(body.head_commit.id)
    escapeQuotes: true

And when I was writing this:

Ultimately, this is about escaping external data being received into the system,
it does feel somewhat odd that the template insertion has to know whether or not
the data should be escaped...

I reasoned that the entire purpose of triggers is to generate templated resources from incoming data, and so differentiation on whether or not "tainted" values should be escaped could be deemed to be moot, because all data is "tainted", which got me to the view that perhaps the issue is with the way it's escaped (string replace quotes), and not with the escaping itself, which is what got me to different strategies for escaping data.

This then got me back to, yes, the TriggerTemplate is probably the best place for this, because you're inserting the value into the body, and it has the context of where the value is being inserted, and so, I reconsidered the %(param.name) approach.

Another approach entirely, which was also suggested by @chmouel (and is probably not in scope for this particular change) would be to apply Jinja (or other another templating languge) to the resources?

[dibyom]

Another approach entirely, which was also suggested by @chmouel (and is probably not in scope for this particular change) would be to apply Jinja (or other another templating languge) to the resources?

Yeah using an actual templating engine has come up before. I think in the past we decided that we wanted to keep things as simple as possible so just simple variable interpolation would be sufficient. Though as we found out there are a few edge cases (like newlines, quotes) with such an approach.

This then got me back to, yes, the TriggerTemplate is probably the best place for this, because you're inserting the value into the body, and it has the context of where the value is being inserted, and so, I reconsidered the %(param.name) approach.

I think I agree but the way I was thinking about adding it to Bindings instead is that bindings should only capture "valid" values. For our use case, any value that is extracted has to be usable in a JSON document. If we do this at the binding level, we only need to do it once per binding. Otherwise, we'd have to do it for each trigger template that uses that binding which sounds less user friendly.

[afrittoli]

In Tekton based CI we use bindings to create an interface for CI jobs - we provide a fixed set of parameters, so that a user writing a CI job does not have to worry about events, interceptors or bindings. With this in mind, the person writing the bindings does not have the context to decide whether a parameter should be escaped or not, because they don't actually know how the parameter is going to be used.

[dibyom]

/lgtm

[tekton-robot]

New changes are detected. LGTM label has been removed.

[bobcatfish]

is it possible to include some examples of how other systems handle this kind of issue?

[afrittoli]

If I understand correctly, in the example above, the title was already escaped Revert \"Test Commit\", so to match that example we could have escape false here or refer to a different field perhaps?

The problem I see with this approach is that the escaping is controlled on the receiving side, which is not specific to the content producer, so using escaping in a template would bind the template to a specific content producer.

Trigger bindings are by nature specific to a content producer, so it feels to me that they would be a better place where to specify if escaping is needed or not.

[afrittoli]

But at the same time, as well written by @bigkevmcd in https://github.com/tektoncd/community/pull/321/files#diff-9129936dcf33d43ca73c0a506952028c7f718c3e15d512ad586044d61579ad0bR174-R181, the receiving side also has context about the need (or not) for escaping.

I like, among the alternatives proposed, the idea of controlling escaping when the variable associated to a parameter is used, for instance using an alternative syntax like %(tt.params.name) or some other syntax like $(tt.params.name|escape).

[bigkevmcd]

I suspect %(tt.params.name) is easier to implement $(tt.params.name|escape) implies (from Django/Jinja2) that the result of tt.params.name should be passed through a "filter" called escape?

[dibyom]

I like %(tt.params.name) over $(tt.params.name|escape).

[tekton-robot]

@bigkevmcd: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vdemeester]

/assign @afrittoli
/assign

[afrittoli]

@bigkevmcd the more I think about it, the more I like the option of escaping at insertion point.

[chmouel]

Maybe I am not understanding the problem correctly, but having the user be in control of how the parameters is going to be escaped is not going to change much, because the user usually doesn't have the ability to make sure what's coming to its payload is in a certain format.

Is the problem perhaps an issue with the standard go json library parsing?

another json library https://github.com/buger/jsonparser has an extensive section on escaping :

https://github.com/buger/jsonparser/blob/master/escape.go

perhaps we should look at how it's done there?

[bigkevmcd]

@chmouel You can vary it, if you use a simple $(param.value) then sometimes you want that escaped (you're inserting it within the body of another string) and sometimes it can be standalone, if you wrap it in quotes"$(param.value)" for example, then quotes within quotes fail.

[dibyom]

So, it sounds like the 2 most likely options are:

1. We allow a %(tt.params.name) type syntax in the trigger templates to indicate quotes should be escaped or not. This adds additional flexibility in that escaping is decided where the parameter is actually used.

2. We add a escapeQuotes field to the TriggerTemplate param field. This does mean that every usage of that param would be escaped but avoids new syntax.

Does this sound like an accurate summary or am I missing something? @bigkevmcd @afrittoli

[dibyom]

I like %(tt.params.name) over $(tt.params.name|escape).

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.