Cel interceptor #285

bigkevmcd · 2019-12-19T10:22:42Z

Changes

This implements a CEL based interceptor that allows matching on request bodies, and request headers.

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

See the contribution guide for more details.

Release Notes

This adds a new CEL interceptor, able to use expressions to determine whether or not to filter out incoming request bodies.

tekton-robot · 2019-12-19T10:22:58Z

Hi @bigkevmcd. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

bigkevmcd · 2019-12-19T10:26:22Z

Some thoughts I've had since the initial implementation...

Instead of one expression there could be several.

expressions:
 - "headers.match(...)"
 - "body.pull_request.head.repo.fork == false"

Where each of the expressions would need to return true, this would improve readability (over very long expressions).

Also, there's no way currently to add a custom response header, this could be implemented similar to the values: key.

This is related to #247

bigkevmcd · 2019-12-19T10:41:06Z

Also, in order to avoid the invisible coupling between the interceptor modifying the request body, and bindings, CEL could be added to the list of expression languages for bindings.

dibyom · 2019-12-19T15:00:57Z

/ok-to-test

tekton-robot · 2019-12-19T15:02:50Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	Do not exist	80.4%

bigkevmcd · 2019-12-19T15:12:08Z

/test pull-tekton-triggers-build-tests

tekton-robot · 2019-12-19T15:23:21Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/cel/cel.go	Do not exist	80.4%

dibyom

Thanks for the PR @bigkevmcd I started playing around with your branch the other day and overall it looks great.

The main thing that I'd like to see changed is the modification of the payload (the Value field) -- I think its a super useful feature that we'd probably want to add but I think we should do that in a followup. For the 0.2 release, I think having just the expressions for filtering would be great!

Oh, and would you mind squashing the commits 😄 ?

pkg/apis/triggers/v1alpha1/event_listener_types.go

dibyom · 2019-12-19T15:05:22Z

pkg/apis/triggers/v1alpha1/event_listener_validation.go

@@ -67,7 +67,7 @@ func (s *EventListenerSpec) validate(ctx context.Context, el *EventListener) *ap

 func (i *EventInterceptor) validate(ctx context.Context, namespace string) *apis.FieldError {
 	// Validate at least one
-	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil {
+	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil && i.CEL == nil {


Should we also validate that if CEL has the expression field set?

Add validation for the renamed filter expression.

dibyom · 2019-12-19T15:17:40Z

pkg/interceptors/cel/cel.go

+func (w *Interceptor) ExecuteTrigger(payload []byte, request *http.Request, _ *triggersv1.EventListenerTrigger, _ string) ([]byte, error) {
+	env, err := makeCelEnv()
+	if err != nil {
+		return nil, err


Could we add some extra context when returning errors in this file?
e.g. fmt.Errof("error creating cel environment: %w, err)

dibyom · 2019-12-19T15:43:51Z

pkg/interceptors/cel/cel_test.go

+		{
+			name: "simple header check with non matching header",
+			CEL: &triggersv1.CELInterceptor{
+				Expression: "headers['X-Test'][0] == 'unknown'",


The headers part here is slightly different from headers in TriggerBindings.
In Bindings, the keyword is header not headers -- though headers does sound like the more correct term.
Also, you do not have to use array accessors e.g. $(headers.foo) not $(headers.foo[0])
Its probably worth being consistent here.

Just to clarify, you would prefer it to emulate JSON path expressions in CEL?

I'm wondering if CEL is the right expression language to use, maybe we could augment the JSON path expressions?

To be clear, the syntax header['X-Test'][0] is one possible option.

The header value is a https://golang.org/pkg/net/http/#Header value, but, it's translated in CEL into a map, so, there's a custom match overload, which uses .Get() on the value, this is doing more than a simple key-value lookup.

I'm not sure how easy it is to emulate JSON path expressions in CEL without a significant amount of work.

By "emulating", I was just referring to making it easy to access single valued headers i.e header['X-Test'][0] vs header['X-Test']. From reading @wlynch 's comment it seems like that might already be the case...
We definitely should not try to do anything more complicated than that 😅

dibyom · 2019-12-19T15:57:03Z

Instead of one expression there could be several.

Another more verbose option could be the multiple interceptor support that @wlynch is working on

Also, in order to avoid the invisible coupling between the interceptor modifying the request body, and bindings, CEL could be added to the list of expression languages for bindings

This could be an interesting add on for the future...(JSONPath also supports filtering, maybe that could be a interceptor too 😛 )

bigkevmcd · 2019-12-19T16:31:37Z

Instead of one expression there could be several.

Another more verbose option could be the multiple interceptor support that @wlynch is working on

This would be slightly different, in that it would be more about readability to avoid having to write long expressions like filter1 && filter2 && filter3 && filter4

tekton-robot · 2019-12-19T17:14:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

bigkevmcd · 2019-12-19T17:24:39Z

The main driver for this is to get rid of an external interceptor that is doing matching, and extracting the SHA and trimming it (amongst other things).

The "values" behaviour emulates this behaviour, so, I'd like to see some discussion about what alternatives could look like.

tekton-robot · 2019-12-19T17:31:52Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

tekton-robot · 2019-12-19T17:45:04Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

wlynch

We should make sure to add a reference to the issue (#49) in the PR body / commit message.

Otherwise looking good! :D

docs/eventlisteners.md

wlynch · 2019-12-19T19:23:46Z

pkg/apis/triggers/v1alpha1/event_listener_validation.go

@@ -67,7 +67,7 @@ func (s *EventListenerSpec) validate(ctx context.Context, el *EventListener) *ap

 func (i *EventInterceptor) validate(ctx context.Context, namespace string) *apis.FieldError {
 	// Validate at least one
-	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil {
+	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil && i.CEL == nil {
 		return apis.ErrMissingField("interceptor")
 	}



We need to add logic for numSet below.

Let's also add a test to make sure CEL and another Interceptor can't be set at the same time.

I think this still needs to be added

pkg/apis/triggers/v1alpha1/event_listener_validation_test.go

pkg/interceptors/cel/cel.go

wlynch · 2019-12-19T19:47:54Z

pkg/interceptors/cel/cel.go

+	return cel.Functions(
+		&functions.Overload{
+			Operator: "match",
+			Function: matchHeader},


IIUC, this function is to simplify accessing headers with only 1 value, correct? Could we just use the built-in in function instead?

e.g. in('foo', header['myheader']). See https://github.com/google/cel-spec/blob/master/doc/langdef.md for more details.

That's not quite what it does, the value of header is https://golang.org/pkg/net/http/#Header and the match delegates to the Get method, which does a bit more than accessing the header for single-value headers.

wlynch · 2019-12-19T19:50:53Z

pkg/interceptors/cel/cel.go

+func evaluate(expr string, env cel.Env, data map[string]interface{}) (ref.Val, error) {
+	parsed, issues := env.Parse(expr)
+	if issues != nil && issues.Err() != nil {
+		return nil, issues.Err()


Do we know what will be returned in this error message? e.g. will it be clear what exactly is failing? We may want to consider wrapping this error to make it clear what went wrong.

The error message from CEL is pretty good, the test I added only really tests a subset of the message.

failed to evaluate expression 'header['X-Test': ERROR: <input>:1:8: Syntax error: token recognition error at: ''X-Test' | header['X-Test | .......^ ERROR: <input>:1:15: Syntax error: mismatched input '<EOF>' expecting {'[', '{', '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, NUM_UINT, STRING, BYTES, IDENTIFIER} | header['X-Test | ..............^

pkg/interceptors/cel/cel_test.go

pkg/sink/sink.go

dibyom · 2019-12-19T20:50:58Z

~~Can't comment on the actual comment for some reason~~ Now I magically can! 😕

tekton-robot · 2019-12-20T09:55:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	93.9%	0.3

tekton-robot · 2019-12-20T11:38:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	93.9%	0.3

tekton-robot · 2019-12-20T11:58:51Z

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

File	Old Coverage	New Coverage	Delta
test/builder/eventlistener.go	93.6%	93.9%	0.3

tekton-robot · 2019-12-20T12:02:53Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	93.9%	0.3

dibyom

hey @bigkevmcd , sorry about the long review time. This is looking almost good to go - just needs a rebase, and a couple of minor issues.

One thing is that this is going to conflict with #272 depending on which gets in first

dibyom · 2020-01-06T17:46:56Z

docs/eventlisteners.md

+[CEL](https://github.com/google/cel-go) expression language.
+
+Supported features include case-insensitive matching on request headers, and the
+ability to add new keys (which can be extracted in a TemplateBinding) to the response


I think we decided to remove the part about adding new keys for now

dibyom · 2020-01-06T18:09:45Z

pkg/apis/triggers/v1alpha1/event_listener_validation.go

@@ -67,7 +67,7 @@ func (s *EventListenerSpec) validate(ctx context.Context, el *EventListener) *ap

 func (i *EventInterceptor) validate(ctx context.Context, namespace string) *apis.FieldError {
 	// Validate at least one
-	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil {
+	if i.Webhook == nil && i.Github == nil && i.Gitlab == nil && i.CEL == nil {
 		return apis.ErrMissingField("interceptor")
 	}



I think this still needs to be added

dibyom · 2020-01-06T18:13:57Z

We should make sure to add a reference to the issue (#49) in the PR body / commit message.

Looks like we need to add this as well!

tekton-robot · 2020-01-06T21:16:57Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

tekton-robot · 2020-01-06T21:36:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

tekton-robot · 2020-01-06T21:38:30Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5

tekton-robot · 2020-01-06T22:01:55Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	92.2%	-1.5

wlynch · 2020-01-07T16:13:14Z

docs/eventlisteners.md

+CEL interceptors parse expressions to filter requests based on JSON bodies and request headers, using the
+[CEL](https://github.com/google/cel-go) expression language.
+
+Supported features include case-insensitive matching on request headers.


Let's document the custom methods we add similar to https://github.com/google/cel-spec/blob/master/doc/langdef.md#list-of-standard-definitions

Is this ok for a start?

https://github.com/tektoncd/triggers/blob/5d0379587187bef6ced05c6469b10d272d28009f/docs/cel_expressions.md

Looks good! 😄 Thanks!

tekton-robot · 2020-01-07T16:13:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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~~ [dibyom]

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

tekton-robot · 2020-01-07T16:27:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	92.2%	-1.5

This adds a cel interceptor, that uses a a CEL expression to filter request bodies. This implements issue tektoncd#49.

tekton-robot · 2020-01-07T16:53:55Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	92.2%	-1.5

tekton-robot · 2020-01-07T16:55:03Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	91.5%	92.0%	0.5
test/builder/eventlistener.go	93.6%	92.2%	-1.5

wlynch · 2020-01-07T17:08:23Z

/lgtm

tekton-robot requested review from iancoffey and vtereso December 19, 2019 10:22

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 19, 2019

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 19, 2019

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2019

dibyom reviewed Dec 19, 2019

View reviewed changes

bigkevmcd force-pushed the cel-interceptor branch 2 times, most recently from 6229aed to 33bc5b8 Compare December 19, 2019 17:13

bigkevmcd force-pushed the cel-interceptor branch from 33bc5b8 to 9d0be43 Compare December 19, 2019 17:29

bigkevmcd force-pushed the cel-interceptor branch from 9d0be43 to 1478ee9 Compare December 19, 2019 17:42

wlynch reviewed Dec 19, 2019

View reviewed changes

bigkevmcd force-pushed the cel-interceptor branch from 1478ee9 to ee729b1 Compare December 20, 2019 09:53

bigkevmcd force-pushed the cel-interceptor branch from e841acb to 5a0182f Compare December 20, 2019 11:56

bigkevmcd force-pushed the cel-interceptor branch from 5a0182f to 45ec5b9 Compare December 20, 2019 12:00

dibyom approved these changes Jan 6, 2020

View reviewed changes

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

bigkevmcd force-pushed the cel-interceptor branch from 45ec5b9 to 34980e8 Compare January 6, 2020 21:15

bigkevmcd force-pushed the cel-interceptor branch from 70975dc to 08a2057 Compare January 6, 2020 21:35

bigkevmcd force-pushed the cel-interceptor branch from 08a2057 to aa10c18 Compare January 6, 2020 21:59

wlynch approved these changes Jan 7, 2020

View reviewed changes

bigkevmcd force-pushed the cel-interceptor branch from aa10c18 to c5662b2 Compare January 7, 2020 16:25

Add an interceptor to that can filter based on CEL expressions.

5d03795

This adds a cel interceptor, that uses a a CEL expression to filter request bodies. This implements issue tektoncd#49.

bigkevmcd force-pushed the cel-interceptor branch from b5fd2e6 to 5d03795 Compare January 7, 2020 16:53

tekton-robot assigned wlynch Jan 7, 2020

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

tekton-robot merged commit 9ace5c1 into tektoncd:master Jan 7, 2020

dibyom mentioned this pull request Feb 3, 2020

Update CEL docs #407

Merged

3 tasks

bigkevmcd deleted the cel-interceptor branch March 10, 2020 10:28

Cel interceptor #285

Cel interceptor #285

Conversation

bigkevmcd commented Dec 19, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 19, 2019

bigkevmcd commented Dec 19, 2019 • edited Loading

bigkevmcd commented Dec 19, 2019

dibyom commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

bigkevmcd commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

dibyom left a comment • 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

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

dibyom commented Dec 19, 2019

bigkevmcd commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

bigkevmcd commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

wlynch left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Dec 19, 2019 • edited Loading

tekton-robot commented Dec 20, 2019

tekton-robot commented Dec 20, 2019

tekton-robot commented Dec 20, 2019

tekton-robot commented Dec 20, 2019

dibyom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Jan 6, 2020

tekton-robot commented Jan 6, 2020

tekton-robot commented Jan 6, 2020

tekton-robot commented Jan 6, 2020

tekton-robot commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 7, 2020

tekton-robot commented Jan 7, 2020

tekton-robot commented Jan 7, 2020

tekton-robot commented Jan 7, 2020

wlynch commented Jan 7, 2020

bigkevmcd commented Dec 19, 2019 •

edited

Loading

dibyom left a comment •

edited

Loading

dibyom commented Dec 19, 2019 •

edited

Loading