Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced Filtering on Triggers #930

Open
nachocano opened this issue Mar 20, 2019 · 12 comments

Comments

@nachocano
Copy link
Contributor

commented Mar 20, 2019

Problem
We need more advanced filtering mechanisms than just exact match on Cloud Event types and sources.

Persona:
Event Consumer

Exit Criteria
Being able to configure advanced filters on triggers, either with CEL or some other language. E2E tests are necessary.

Time Estimate (optional):
~2 weeks, depending on how long the design discussions take.

@matzew

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@evankanderson while I like the idea, not sure if - for 0.6 we should introduce some "language" for advanced filters

@bbrowning

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

I feel like something a bit more advanced is warranted. Even if it's not a full-blown expression language, it would be great to expand exact matching to fields in the Cloud Event data or allow some kind of basic wildcard matching.

To give some concrete samples of more advanced filtering I'd like:

  • I want to receive all events from the K8s Event Source for namespace foo with Reason: RevisionReady
  • I want to receive all events from the K8s Event Source for all Pods in namespace foo
@evankanderson

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Discussing with @lindydonna, it may make sense to actually go all the way in and then figure out what the commonalities are. (i.e. start with a full CEL expression or some equivalent, and then figure out that most customers end up writing "source like X, type is Y, and extracted field Z is present.")

The rationale being that once we have the advanced model, it's easy to implement the common items as syntactic sugar that gets expanded server-side, whereas if we started with smaller models, we may end up rewriting the filtering side multiple times. (And this way, the feature is "X is even easier" rather than "complicated use case Y is now more-possible without writing code".)

@grantr

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

👍 Also consider that CEL may be a similar amount of work to prototype as homegrown simpler filtering. See https://github.com/grantr/cel-playground.

@vaikas-google vaikas-google added this to the v0.6.0 milestone Apr 1, 2019

@nachocano

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Grant is taking care of this one!

/assign @grantr

@grantr

This comment has been minimized.

Copy link
Member

commented May 14, 2019

/milestone v0.7.0

@knative-prow-robot knative-prow-robot modified the milestones: v0.6.0, v0.7.0 May 14, 2019

@lionelvillard

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Have you guys thought about providing the same capability to Subscription? @nachocano @grantr @evankanderson

@grantr

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@lionelvillard yep, that's come up before. At the time it was argued that adding filtering to the Subscription interface is a burden for channel authors because the dispatcher is more complex to implement. From a user perspective, a world in which each channel implements filters slightly differently makes channels less interchangeable than they are today.

@wlynch

This comment has been minimized.

Copy link

commented Jun 3, 2019

Hi! Just want to chime in here after talking to @vaikas-google at Kubecon.

I suspect that we might want to extend advanced filtering past CEL expressions, since there will often be scenarios where triggers might want to fetch additional data to make decisions on triggering.

The motivating example here is that for source triggering you might need to make calls back to GitHub to fetch additional metadata that was not included in the webhook event, such as files that were modified in the change, or whether the author of the change was a collaborator on the repository.

Something that would be nice is to allow the user to provide an image in their trigger configuration that could be invoked with an event to make arbitrary filtering decisions, returning a simple allow/deny response.

Thoughts?

@evankanderson

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Looking up additional information outside the event itself seems like it could be the first step in a Pipeline, and the filter could then choose to either return no event (drop event) or the same event (identity transformation).

This can also be accomplished today by delivering from a Trigger to a Channel -> Subscription (Filter) -> Channel configuration, which is what Pipeline would be wrapping.

I'm concerned that by introducing general-purpose compute into Trigger, we'll be losing the ability to efficiently solve common-case problems like "all events of type X" or "all events about objects > 4MB" (assuming that object size is included in the objectChange event).

@vaikas-google

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Yeah, so my thinking was that we should be just building a "filtering function", that looks like just a normal function that would then handle these more advanced filtering capabilities. At least until we more experience with them, it seems like general purpose compute should be handled by a function and then it can either emit events that the "real function" can take action on (if using triggers), or as @evankanderson mentioned, it could be the first step in the pipeline and either emit a decorated event for downstream processing, or not at all, which effectively filters the event from further processing.

@grantr

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

/milestone clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
10 participants
You can’t perform that action at this time.