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

Feature: Allow Triggers to invoke other triggers after interceptor processing #945

Closed
jmcshane opened this issue Feb 5, 2021 · 4 comments
Assignees

Comments

@jmcshane
Copy link
Contributor

jmcshane commented Feb 5, 2021

Feature request

Screen Shot 2021-02-05 at 11 06 40 AM

Triggers processed in the sink right now can currently only resolve to a single template and set of bindings. As outlined in #820, there is often a goal to do some initial interceptor processing before invoking additional triggers. In this implementation, a Triggers field would be added to the current TriggerSpec that would be a list of refs to Trigger objects accessible to the EventListener sink. After interceptor processing, the sub-triggers would be invoked alongside any existing binding/template pair set on the trigger.

Some changes that would have to happen to make this work:

  • Template/Bindings would become optional in the Trigger CRD - update the openapi spec for Triggers to force one-of either template/bindings or triggers.
  • The sink Trigger resolution function would have to distinguish between "exposed" triggers and "internal" triggers (Propose using a label for this)
  • Need to prevent infinite loop of triggers somehow
  • Determine what triggers are available for "internal" processing. In my mind, all triggers would be available to use as a trigger ref, we are just filtering some of them from the sink.
  • Determine if a Trigger can have both a template/binding and trigger refs.
  • Determine how the event listener will respond to these requests. Should it wait for all the "sub"-triggers to be invoked to determine if it should response 201 or 202? For our use case its not essential, but I may not be aware of other use cases that take advantage of that status code.

Use case

The use case is fairly well outlined in #820 as well. My team's use case is that we are passing additional metadata along with each repository and having a webhook add that metadata via a webhook extension. We want to run that webhook once for each invocation of the eventlistener, then separate processing into a set of subsequent triggers after that initial extension is processed.

@dibyom
Copy link
Member

dibyom commented Feb 5, 2021

Thanks for writing this up. So this looks like a solution to the problem discussed in #820 It's a problem that I've been thinking through a bit myself :)

We want to run that webhook once for each invocation of the eventlistener, then separate processing into a set of subsequent triggers after that initial extension is processed.

Is the main reason you want this maintainability of the YAML configurations or some performance concerns (or both)?

From going through the slack discussion, it seems to me that maintainability of the trigger configs was one big concern (if possible, could you share some of the trigger configs?)

So, my initial thoughts are:

  1. It seems like that having filters as an implementation of interceptors and having interceptors only under triggers leads to reusability issues e.g. you can't share a common CEL filter interceptor if you need to. (this is the issue that@jmcshane mentioned)

  2. In addition, calling one interceptor multiple times for a single event is inefficient and might lead to performance issues (though I'm not sure if that is the case currently) (this is the issue @afrittoli mentioned)

  3. One issue is that this solution changes what constitutes a Trigger and makes it a bit more complex. This is something we can do but before we do that I'd like to see if there are other alternatives that might work as well.

  4. Thinking out loud here. A few alternatives that you have already considered -- some notion of a shareable interceptor configuration, interceptors at the EventListener level, a simple caching layer for interceptor calls, some notion of a "group" of triggers that is distinct from the single Trigger that we have today.

  5. There is also the issue of whether or not Triggers is the right place for this type of processing or whether it should be done in a different layer (@bigkevmcd brought this up)

Overall, looks like a lot to discuss so before we jump into a solution. Let's discuss the use cases (maintainability, performance) and the tradeoffs (complexity of the Trigger type). I can add this to the agenda for next week's WG.

In the meantime, we can keep discussing in this issue (and I'll try to spend some more time thinking through this problem!). Appreciate any feedback!

/cc @afrittoli @jmcshane @bigkevmcd

@jmcshane
Copy link
Contributor Author

jmcshane commented Feb 5, 2021

Thanks for your feedback @dibyom. I'll definitely plan to join the WG on Wednesday. I'm trying to sanitize out my company specific information from the Trigger objects, but I'll share some examples before WG.

One alternative I want to address is that idea of a "group" of triggers. I'm very willing to go with that approach, but the downside here is that you need to somehow select the bindings that tie to that trigger. For example, we're adding extensions on the event based on an API call as we are tracking the apps "onboarded" to our tekton platform. Then, these extensions allow a repository to get a number of different pipelines invoked based on different git events (PR, merge, tag, release, comment). With a group of triggers, we need to be able to conditionally select which ones in the group are valid for this event. This leads back to "how do you make that determination" and triggers already provides that answer: interceptors.

A shareable interceptor chain across multiple triggers is an interesting alternative, but it still leads to an explosion of triggers that are processed on invocation (especially if that requires multiple external API calls). One of the challenges here is that trigger templates provision static k8s resources (specifically, workspace configuration is something we are trying to control with this logic). The proposed solution reduces this because you can reduce the filter steps to exactly the point that you know that this one specific trigger is valid.

As we continue in the discussion, check out the changes I made to the API packages in #946. I don't mean to say its the "right" solution, but it gives a starting point of how this could be implemented.

jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 5, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 5, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 6, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 8, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 8, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

Tracking nested trigger via extensions
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 8, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

As is assumed in the resolve trigger
function, we assume that the nested trigger must
be in the same namespace as the invoking trigger.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 8, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger object. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

As is assumed in the resolve trigger
function, we assume that the nested trigger must
be in the same namespace as the invoking trigger.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 8, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger and EventListener
objects. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

As is assumed in the resolve trigger
function, we assume that the nested trigger must
be in the same namespace as the invoking trigger.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 9, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger and EventListener
objects. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

As is assumed in the resolve trigger
function, we assume that the nested trigger must
be in the same namespace as the invoking trigger.
jmcshane pushed a commit to jmcshane/triggers that referenced this issue Feb 11, 2021
As described in tektoncd#945, this feature allows triggers to invoke other
triggers by adding a trigger ref onto the Trigger and EventListener
objects. Hidden
triggers are created by setting the `tekton.dev/sink=hidden` label on
the Trigger CRD object. Hidden triggers are not invoked in the main
HandleFunc of the sink, but rather invoked when referenced in a trigger
that has completed interceptor processing.

Nested triggers can also retrieve the extensions from the interceptor
chain of parent triggers. This can be helpful in the future in case a
set of triggers want to create a default set of extensions for trigger
processing.

We protect the nested triggers from an infinite loop of triggers by
passing a custom extension down through the trigger nesting and tracking
how deep in the nesting tree we are at that time.

Further discussion continues in tektoncd#945, but wanted to get something out
here to demonstrate the approach.

As is assumed in the resolve trigger
function, we assume that the nested trigger must
be in the same namespace as the invoking trigger.
@tekton-robot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2021
@dibyom dibyom removed kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 12, 2021
@dibyom dibyom added this to the Triggers Post Beta milestone May 12, 2021
@dibyom dibyom modified the milestones: Triggers v0.16, Triggers v0.17 Aug 31, 2021
@dibyom dibyom self-assigned this Sep 22, 2021
dibyom pushed a commit to dibyom/triggers that referenced this issue Oct 11, 2021
This feature allows an operator to specify a set of interceptors that will be executed
before a group of triggers are selected and executed. This allows common data
to be passed from interceptor execution down to multiple triggers to solve
a set of common use cases across multiple Triggers.

This feature is enabled for now inline in the EventListener spec, but in the future
may be enabled only in alpha once the feature gates proposal is implemented within
this project.

Addresses tektoncd#945
dibyom pushed a commit to dibyom/triggers that referenced this issue Oct 11, 2021
This feature allows an operator to specify a set of interceptors that will be executed
before a group of triggers are selected and executed. This allows common data
to be passed from interceptor execution down to multiple triggers to solve
a set of common use cases across multiple Triggers.

This feature is enabled for now inline in the EventListener spec, but in the future
may be enabled only in alpha once the feature gates proposal is implemented within
this project.

Addresses tektoncd#945
dibyom pushed a commit to dibyom/triggers that referenced this issue Oct 11, 2021
This feature allows an operator to specify a set of interceptors that will be executed
before a group of triggers are selected and executed. This allows common data
to be passed from interceptor execution down to multiple triggers to solve
a set of common use cases across multiple Triggers.

This feature is enabled for now inline in the EventListener spec, but in the future
may be enabled only in alpha once the feature gates proposal is implemented within
this project.

Addresses tektoncd#945
tekton-robot pushed a commit that referenced this issue Oct 12, 2021
This feature allows an operator to specify a set of interceptors that will be executed
before a group of triggers are selected and executed. This allows common data
to be passed from interceptor execution down to multiple triggers to solve
a set of common use cases across multiple Triggers.

This feature is enabled for now inline in the EventListener spec, but in the future
may be enabled only in alpha once the feature gates proposal is implemented within
this project.

Addresses #945
@savitaashture
Copy link
Contributor

Fixes as part of this PR #1232

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

Successfully merging a pull request may close this issue.

4 participants