TEP-0022: Triggers - Immutable Input Events #218

wlynch · 2020-09-29T20:22:38Z

This TEP proposes a change to Trigger interceptor handling to ensure
that incoming input events are not mutated through the triggering
process.

TEP Design for tektoncd/triggers#778

wlynch · 2020-09-29T20:48:31Z

/cc @dibyom @bigkevmcd

This TEP proposes a change to Trigger interceptor handling to ensure that incoming input events are not mutated through the triggering process. TEP Design for tektoncd/triggers#778

bigkevmcd · 2020-09-30T09:51:18Z

What does the new Interceptor Interface look like?

// Interceptor is the interface that all interceptors implement.
//
// Implementations can return extensions, which will be made available to subsequent calls in `inputs.<interceptor name>`.
type Interceptor interface {
    ExecuteTrigger(input map[string]interface{}) (extensions map[string]interface{}, err error)
}

bigkevmcd · 2020-09-30T09:56:16Z

teps/0022-trigger-immutable-input.md

+Webhook interceptors may no longer modify headers as part of the interceptor
+chain.
+
+If no response body is returned by the interceptor, no `interceptors.<name>`


This might complicate bindings, if you have several interceptors, and the values go in as...

{ "interceptors": { "github-push-repo-a-interceptor": { "short_sha": "bca6b70" }, "github-push-repo-b-interceptor": { "short_sha": "3e29f82b" } } }

Getting a binding that can build and tag with the short_sha in a generic form will be tricky.

Yeah, what if this is a flat list i.e we do not key by interceptor name (we could call this field extensions or something?)

The main drawback I see is if two interceptors both produce the same field. In that case, maybe we follow a simple rule -- the last interceptor in the chain can overwrite the previous one? (Another alternative might be to just throw an error in this case)

The scope for interceptors to do this is fairly small, there's no "global" interceptor, so if it's happening, it's happening within a fairly small stanza in your YAML.

To me, this suggests a low likelihood of having to debug a mysterious value, and with "last-one-wins", it should be fairly easy to debug (it's not like we're subject to the vagaries of hash ordering).

dibyom · 2020-09-30T14:40:43Z

teps/0022-trigger-immutable-input.md

+field) we will include the entire input event in a reserved body field named
+`input`. This field will not be mutated throughout trigger processing, so that
+inteceptors and trigger bindings can rely on assumptions on the incoming event
+type. Incoming event data will be stored within `input` as `body` and `headers`


Will users have to now use input.body or input.header for bindings/interceptors? If so, can we consider just keeping the body and headers as is today but make them immutable while adding a new modifiable field (interceptors) as described below?

Looking at it again, maybe the interface becomes:

interface Interceptor { ExecuteTrigger(headers http.Header, body map[string]interface{}) (extensions map[string]interface{}, err error) }

The Webhook interceptor would just do a POST, transferring the headers, encoding the body, and being expected to return an optional JSON body (which would become extensions) ?

I think we'll want to encapsulate this into Request/Response types, following what is proposed in the pluggable interceptor proposal by @dibyom. This will make it easier to add additional fields into the request/response objects as needed (i.e. the additional trigger fields proposed by Dibyo):

interface Interceptor { ExecuteTrigger(req InterceptorRequest) (InterceptorResponse, error) } type InterceptorRequest struct { Headers http.Headers Body json.RawMessage Interceptors map[string]interface{} } type InterceptorResponse struct { Body json.RawMessage }

teps/0022-trigger-immutable-input.md

dibyom · 2020-09-30T14:58:21Z

teps/0022-trigger-immutable-input.md

+Webhook interceptors may no longer modify headers as part of the interceptor
+chain.
+
+If no response body is returned by the interceptor, no `interceptors.<name>`


Yeah, what if this is a flat list i.e we do not key by interceptor name (we could call this field extensions or something?)

The main drawback I see is if two interceptors both produce the same field. In that case, maybe we follow a simple rule -- the last interceptor in the chain can overwrite the previous one? (Another alternative might be to just throw an error in this case)

teps/0022-trigger-immutable-input.md

wlynch · 2020-09-30T17:40:03Z

teps/0022-trigger-immutable-input.md

+Webhook interceptors may no longer modify headers as part of the interceptor
+chain.
+
+If no response body is returned by the interceptor, no `interceptors.<name>`


teps/0022-trigger-immutable-input.md

wlynch · 2020-09-30T19:03:49Z

teps/0022-trigger-immutable-input.md

+payloads.
+
+Any response received from interceptors will be included in following paylods
+under a new `interceptors` field, keyed by interceptor name for uniqueness.


Any thoughts on naming?

interceptors

extensions

plugins

???

very slight preference towards extensions but open to all of them!

wlynch · 2020-09-30T19:04:22Z

teps/0022-trigger-immutable-input.md

+field) we will include the entire input event in a reserved body field named
+`input`. This field will not be mutated throughout trigger processing, so that
+inteceptors and trigger bindings can rely on assumptions on the incoming event
+type. Incoming event data will be stored within `input` as `body` and `headers`


I think we'll want to encapsulate this into Request/Response types, following what is proposed in the pluggable interceptor proposal by @dibyom. This will make it easier to add additional fields into the request/response objects as needed (i.e. the additional trigger fields proposed by Dibyo):

interface Interceptor { ExecuteTrigger(req InterceptorRequest) (InterceptorResponse, error) } type InterceptorRequest struct { Headers http.Headers Body json.RawMessage Interceptors map[string]interface{} } type InterceptorResponse struct { Body json.RawMessage }

This primarily removes the proposed `input` field in favor of direct `headers` and `body` field to reduce the impact this would have on existing users. Other minor changes: - Make feature flag an annotation. - Flesh out details around interface changes (highly influenced by Pluggable Interceptors proposal). - Add more details around CEL Interceptor changes.

dibyom · 2020-10-05T15:03:03Z

/approve

tekton-robot · 2020-10-05T15:03:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~teps/OWNERS~~ [dibyom]

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

wlynch · 2020-10-05T15:08:21Z

/kind tep

bobcatfish · 2020-10-13T15:06:46Z

teps/0022-trigger-immutable-input.md

+<<[UNRESOLVED wlynch ]>>
+TODO based on feedback
+<<[/UNRESOLVED]>>
+```


i think it's worth generating some of these as part of the proposal, even if the alternatives you come up with aren't very appealing; if that's the case then it increase confidence in the proposal at least :D

some ideas: one alternative is always to NOT do the thing, i.e. what does it look like to try to meet the same goals but without the explicit feature? or we could make this something you opt into (kinda already the case with the feature flag, but maybe at a different level, e.g. configuring mutability in the binding?)? or maybe some kind of interceptor you add into your interceptors list that ensures the event hasn't been mutated?

(none of these are great but again i think it's worth generating and recording)

oh and you do have one below! "TODO based on feedback" made me think there wouldn't be any

anyway here are a few more :D

bobcatfish · 2020-10-13T15:08:35Z

teps/0022-trigger-immutable-input.md

+<<[UNRESOLVED wlynch ]>>
+TODO based on feedback
+<<[/UNRESOLVED]>>
+```


one potential drawback that comes to mind: can't as easily use interceptors to filter out sensitive data? e.g. if you could imagine wanting to pass the entire body of an event to a resulting Run

i suppose you can still do this, but you need to do it by duplicating all the data into a new field?

ooh, great point! Added (though I don't think we want to support this).

alternatives. Adds drawbacks / alternatives raised in review: - Payload redaction - What if we keep immutable behavior?

bobcatfish · 2020-11-09T20:11:26Z

Discussed merging at proposed in the API working group today - seems like it's likely we can go to "implementable" quickly after that, don't seem to be any outstanding issues atm

/lgtm

tekton-robot requested review from bobcatfish and sm43 September 29, 2020 20:22

tekton-robot added the size/L label Sep 29, 2020

tekton-robot requested review from bigkevmcd and dibyom September 29, 2020 20:48

wlynch changed the title ~~Triggers TEP: Immutable Input Events~~ TEP-0022: Triggers - Immutable Input Events Sep 29, 2020

wlynch force-pushed the trigger-immutable-tep branch from 602d714 to bca6b70 Compare September 29, 2020 21:16

bigkevmcd reviewed Sep 30, 2020

View reviewed changes

dibyom reviewed Sep 30, 2020

View reviewed changes

teps/0022-trigger-immutable-input.md Outdated Show resolved Hide resolved

wlynch commented Sep 30, 2020

View reviewed changes

tekton-robot added the approved label Oct 5, 2020

tekton-robot added the kind/tep label Oct 5, 2020

bobcatfish reviewed Oct 13, 2020

View reviewed changes

TEP 0022 - Trigger Immutable events: add additional drawbacks /

5db3472

alternatives. Adds drawbacks / alternatives raised in review: - Payload redaction - What if we keep immutable behavior?

bobcatfish mentioned this pull request Oct 26, 2020

TEP-0026- Add TEP for interceptor plugins #229

Merged

tekton-robot assigned bobcatfish Nov 9, 2020

tekton-robot added the lgtm label Nov 9, 2020

tekton-robot merged commit d938803 into tektoncd:master Nov 9, 2020

This was referenced Nov 10, 2020

FR: Core Interceptor plugins tektoncd/triggers#271

Closed

Update immutable input events to implementable #262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0022: Triggers - Immutable Input Events #218

TEP-0022: Triggers - Immutable Input Events #218

wlynch commented Sep 29, 2020

wlynch commented Sep 29, 2020

bigkevmcd commented Sep 30, 2020

bigkevmcd Sep 30, 2020

dibyom Sep 30, 2020

bigkevmcd Sep 30, 2020

wlynch Sep 30, 2020

dibyom Sep 30, 2020

bigkevmcd Sep 30, 2020

wlynch Sep 30, 2020 •

edited

Loading

dibyom Sep 30, 2020

wlynch Sep 30, 2020

wlynch Sep 30, 2020

dibyom Oct 1, 2020

wlynch Sep 30, 2020 •

edited

Loading

dibyom commented Oct 5, 2020

tekton-robot commented Oct 5, 2020

wlynch commented Oct 5, 2020

bobcatfish Oct 13, 2020

bobcatfish Oct 13, 2020

wlynch Oct 15, 2020

bobcatfish Oct 13, 2020

wlynch Oct 15, 2020

bobcatfish commented Nov 9, 2020

TEP-0022: Triggers - Immutable Input Events #218

TEP-0022: Triggers - Immutable Input Events #218

Conversation

wlynch commented Sep 29, 2020

wlynch commented Sep 29, 2020

bigkevmcd commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlynch Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlynch Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

dibyom commented Oct 5, 2020

tekton-robot commented Oct 5, 2020

wlynch commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Nov 9, 2020

wlynch Sep 30, 2020 •

edited

Loading

wlynch Sep 30, 2020 •

edited

Loading