TEP-0022: Switch to immutable input event bodies #828

dibyom · 2020-11-13T18:15:58Z

Changes

There are two commits in this PR:

TEP-0022: Add interface to support event bodies

This commit adds two new types for InterceptorRequest and
InterceptorResponse. Instead of just forwarding the incoming request,
EventListeners will use these types to send requests to interceptors.

This commit introduces an extensions field where Interceptors can add
additional data that is available to TriggerBindings. This replaces the
previous mechanism where Interceptors could freely modify the input
bodies. With this change, the input bodies are immutable, and any extra
fields added by Interceptors can only be done within the extensions
field.

This commit simply adds the new interface types and plumbing. Future
commits will change the Interceptors to adopt the new Interface.

Migrate CEL to new Interceptor Interface

This commit updates the CEL interceptor to use the new interface that supports
immutable input event bodies. This represents a BREAKING CHANGE for users using
overlays. Overlays will no longer modify the request body. Instead keys
from overlays will be added to a new top-level extensions field that is
accessible to TriggerBindings.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

action required: If you are using `overlays` in the CEL Interceptor, please update your bindings to use $(extensions.<overlay-key>)  instead of $(body.<overlay-key>)

BREAKING CHANGE:
CEL overlays now add fields to a new top level `extensions` field instead of the modifying the incoming event body. TriggerBindings can access values within this new `extensions` field using `$(extensions.<key>)` syntax.

tekton-robot · 2020-11-13T20:26:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	82.4%	92.3%	10.0

tekton-robot · 2020-11-13T20:38:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	92.3%	10.0

tekton-robot · 2020-11-13T20:51:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	92.3%	10.0

tekton-robot · 2020-11-13T21:04:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	92.3%	10.0

tekton-robot · 2020-11-13T21:07:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4

pkg/interceptors/cel/cel.go

pkg/apis/triggers/v1alpha1/interceptor_types.go

pkg/sink/sink.go

bigkevmcd · 2020-11-16T10:00:37Z

Looks good to me.

tekton-robot · 2020-11-16T19:22:34Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4

wlynch

Overall looking good!

pkg/apis/triggers/v1alpha1/interceptor_types.go

pkg/interceptors/cel/cel.go

pkg/sink/sink.go

wlynch · 2020-11-16T21:11:53Z

pkg/sink/sink.go

+		if interceptorInterface, ok := interceptor.(triggersv1.InterceptorInterface); ok {
+			// Set per interceptor config params to the request
+			request.InterceptorParams = interceptors.GetInterceptorParams(i)
+			interceptorResponse = interceptorInterface.Process(context.Background(), &request)


We should add the context as a parameter so that we can accurately pass through timeouts and tracing information (if any).

ack. the only reason I did not do it here was because there was no existing context to plumb through. Was planning on adding it a follow up PR where we move the interceptors to run in a separate HTTP service. What do you think?

tekton-robot · 2020-11-17T20:47:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/triggerrun/cmd/root.go	45.5%	45.7%	0.2
pkg/apis/triggers/v1alpha1/interceptor_types.go	Do not exist	100.0%
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4
pkg/sink/sink.go	83.3%	82.4%	-0.9

This commit adds two new types for InterceptorRequest and InterceptorResponse. Instead of just forwarding the incoming request, EventListeners will use these types to send requests to interceptors. This commit introduces an `extensions` field where Interceptors can add additional data that is available to TriggerBindings. This replaces the previous mechanism where Interceptors could freely modify the input bodies. With this change, the input bodies are immutable, and any extra fields added by Interceptors can only be done within the `extensions` field. This commit simply adds the new interface types and plumbing. Future commits will change the Interceptors to adopt the new Interface. Part of tektoncd#271 and tektoncd#778 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot · 2020-11-17T20:59:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/triggerrun/cmd/root.go	45.5%	45.7%	0.2
pkg/apis/triggers/v1alpha1/interceptor_types.go	Do not exist	100.0%
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4
pkg/sink/sink.go	83.3%	83.1%	-0.3

tekton-robot · 2020-11-18T01:32:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/triggerrun/cmd/root.go	45.5%	45.7%	0.2
pkg/apis/triggers/v1alpha1/interceptor_types.go	Do not exist	100.0%
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4
pkg/sink/sink.go	83.3%	83.9%	0.5

tekton-robot · 2020-11-18T01:33:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/triggerrun/cmd/root.go	45.5%	45.7%	0.2
pkg/apis/triggers/v1alpha1/interceptor_types.go	Do not exist	100.0%
pkg/interceptors/cel/cel.go	89.2%	87.4%	-1.8
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4
pkg/sink/sink.go	83.3%	83.9%	0.5

This commit updates the CEL interceptor to use the new interface that supports immutable input event bodies. This represents a BREAKING CHANGE for users using `overlays`. Overlays will no longer modify the request body. Instead `keys` from overlays will be added to a new top-level `extensions` field that is accessible to TriggerBindings. Part of tektoncd#271 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot · 2020-11-18T01:44:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/triggerrun/cmd/root.go	45.5%	45.7%	0.2
pkg/apis/triggers/v1alpha1/interceptor_types.go	Do not exist	100.0%
pkg/interceptors/cel/cel.go	89.2%	88.5%	-0.7
pkg/interceptors/interceptors.go	82.4%	94.7%	12.4
pkg/sink/sink.go	83.3%	83.9%	0.5

wlynch

/lgtm

tekton-robot · 2020-11-18T20:50:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlynch

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:

~~OWNERS~~ [wlynch]

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

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 13, 2020

tekton-robot requested review from bobcatfish and dlorenc November 13, 2020 18:16

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 13, 2020

dibyom force-pushed the tep-0022 branch 3 times, most recently from 71fba76 to 42173f5 Compare November 13, 2020 20:24

tektoncd deleted a comment from tekton-robot Nov 13, 2020

dibyom force-pushed the tep-0022 branch from 42173f5 to 6aa4b37 Compare November 13, 2020 20:37

dibyom force-pushed the tep-0022 branch from 6aa4b37 to cab1d6a Compare November 13, 2020 20:50

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 13, 2020

dibyom requested review from bigkevmcd, wlynch and khrm and removed request for bobcatfish and dlorenc November 13, 2020 20:57

dibyom marked this pull request as ready for review November 13, 2020 20:57

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2020

dibyom force-pushed the tep-0022 branch from cab1d6a to d25037e Compare November 13, 2020 21:02

dibyom force-pushed the tep-0022 branch from d25037e to b403d6f Compare November 13, 2020 21:06

bigkevmcd reviewed Nov 16, 2020

View reviewed changes

pkg/interceptors/cel/cel.go Outdated Show resolved Hide resolved

pkg/interceptors/cel/cel.go Outdated Show resolved Hide resolved

pkg/apis/triggers/v1alpha1/interceptor_types.go Outdated Show resolved Hide resolved

pkg/sink/sink.go Outdated Show resolved Hide resolved

dibyom force-pushed the tep-0022 branch from b403d6f to f9b73b3 Compare November 16, 2020 19:20

wlynch requested changes Nov 16, 2020

View reviewed changes

dibyom force-pushed the tep-0022 branch from f9b73b3 to 0990a39 Compare November 17, 2020 20:46

dibyom force-pushed the tep-0022 branch from 0990a39 to 336e2e8 Compare November 17, 2020 20:57

dibyom force-pushed the tep-0022 branch 2 times, most recently from e96bacd to daacebf Compare November 18, 2020 01:32

dibyom force-pushed the tep-0022 branch from daacebf to adbf9cc Compare November 18, 2020 01:43

dibyom requested a review from wlynch November 18, 2020 15:53

wlynch approved these changes Nov 18, 2020

View reviewed changes

tekton-robot assigned wlynch Nov 18, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020

tekton-robot merged commit f1b26d3 into tektoncd:master Nov 18, 2020

dibyom deleted the tep-0022 branch February 24, 2021 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0022: Switch to immutable input event bodies #828

TEP-0022: Switch to immutable input event bodies #828

dibyom commented Nov 13, 2020 •

edited

Loading

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

bigkevmcd commented Nov 16, 2020

tekton-robot commented Nov 16, 2020

wlynch left a comment

wlynch Nov 16, 2020

dibyom Nov 17, 2020

wlynch Nov 18, 2020

tekton-robot commented Nov 17, 2020

tekton-robot commented Nov 17, 2020

tekton-robot commented Nov 18, 2020

tekton-robot commented Nov 18, 2020

tekton-robot commented Nov 18, 2020

wlynch left a comment

tekton-robot commented Nov 18, 2020

TEP-0022: Switch to immutable input event bodies #828

TEP-0022: Switch to immutable input event bodies #828

Conversation

dibyom commented Nov 13, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

tekton-robot commented Nov 13, 2020

bigkevmcd commented Nov 16, 2020

tekton-robot commented Nov 16, 2020

wlynch left a comment

Choose a reason for hiding this comment

wlynch Nov 16, 2020

Choose a reason for hiding this comment

dibyom Nov 17, 2020

Choose a reason for hiding this comment

wlynch Nov 18, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 17, 2020

tekton-robot commented Nov 17, 2020

tekton-robot commented Nov 18, 2020

tekton-robot commented Nov 18, 2020

tekton-robot commented Nov 18, 2020

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 18, 2020

dibyom commented Nov 13, 2020 •

edited

Loading