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

Closed
nachocano opened this issue Mar 20, 2019 · 19 comments
Closed

Advanced Filtering on Triggers #930

nachocano opened this issue Mar 20, 2019 · 19 comments

Comments

@nachocano
Copy link
Contributor

nachocano 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
Copy link
Member

matzew 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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

grantr 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 vaikas added this to the v0.6.0 milestone Apr 1, 2019
@nachocano
Copy link
Contributor Author

Grant is taking care of this one!

/assign @grantr

@grantr
Copy link
Contributor

grantr 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
Copy link
Member

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

@grantr
Copy link
Contributor

grantr 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
Copy link

wlynch 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
Copy link
Member

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
Copy link
Contributor

vaikas 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
Copy link
Contributor

grantr commented Jun 14, 2019

/milestone clear

@n3wscott
Copy link
Contributor

@slinkydeveloper is also trying to fix this.

@grantr grantr added this to the Backlog milestone Aug 24, 2020
@slinkydeveloper
Copy link
Contributor

This issue is duplicate/more specific i think #3359

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 26, 2020
@lionelvillard
Copy link
Member

/remove-lifecycle stale

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2020
@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 28, 2021
@slinkydeveloper
Copy link
Contributor

/remove-lifecycle stale

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2021
@grantr grantr removed their assignment May 14, 2021
@devguyio
Copy link
Contributor

There are many duplicates of the same issue. I'm cleaning old ones like this for the favor of the newest more up to date #5204.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.