TEP-0009: Trigger CRD #148

khrm · 2020-07-14T16:05:35Z

This proposal is to extract out Trigger definition from EventListener Spec into its own CRD
to resolve the problem of Multitenant EventListener.

This is a TEP for the implementation of the Trigger CRD that were
discussed in the following docs:

bobcatfish

Thanks for putting this together @khrm ! I think there are a few more sections to flesh out before we'd be able to properly evaluate this proposal.

bobcatfish · 2020-07-16T14:41:30Z

teps/000X-trigger-crd.md

+  - "@kbaig"
+creation-date: 2020-07-14
+last-updated: 2020-07-14
+status: proposed


im not sure this is technically true since afaik the triggers CRD is already merged? 😬

Yes, but it isn't being used by Triggers but cli. So don't what to put there except proposed.

I think proposed is the right status here. While there is a Trigger CRD type, its main use is for experimentation with the CLI and it not usable by an end user. If we decide to withdraw this proposal for any reason, we can do it without any changes to the API. In addition, there is more to this proposal than just adding the CRD type (e.g. selectors, path based triggers etc.)

I tend to agree with @dibyom and @khrm 😉

I don't totally agree; fortunately nothing in the controller is operating on the new CRD, but it is committed to the codebase - while we haven't explicitly created an API compatibility policy for our Go libs (tektoncd/pipeline#2748) I think it's probably going to end up being in there. So removing or changing this type would be a breaking change imo

A different way of handling this could have been in a branch or a fork?

teps/000X-trigger-crd.md

bobcatfish · 2020-07-16T14:47:01Z

teps/000X-trigger-crd.md

+know that this has succeeded?
+-->
+1. Reduction in resource cosumption due to EventListener.
+2. Ability to have single EventListener to cater to whole cluster.


could you expand a bit on the motivation and goals? in my opinion these are one of the most important sections in the TEP because they help the reader understand the problem being solved fully. it's hard to judge a proposal if you don't fully understand why the thing is being proposed.

Maybe you could elaborate on each of the points you already have, e.g.:

Today, EventListener commonly are created for every namespace and handle TriggerBinding and TriggerTemplate in that same namespace.
Creation of EventListener causes pod proliferation.

Can you explain a bit more, maybe with some examples? e.g. how many namespaces you are dealing with, how many pods.

For every namespace that requires handling of webhook, we need EventListener which in turns lead to
this pod proliferation and causes excess resource consumption.

Does 'handling a webhook' here mean instantiating resources within that namespace?

Can you elaborate on "resource consumption"?

Another goal - separation of concerns. Today listeners are the type that a user creates. Say I have a git repo and I want to create a push trigger, I create an EventListener for it. Next, if I want to add an additional trigger, (say a pull req trigger, or an image push trigger), do I create a new EL or do I add an additional trigger to an EL? What if I add a new repo? A new EL means a new Pod/Deployment/Service. Besides just the resource consumption, there are other concerns. Usually you'd have to expose your EL via an Ingress or some other means. The more ELs I have the more additional configuration I need - in some cases this additional bit of configuration is something that a cluster admin/operator would perform not an user/app developer.

For declarative config as code scenarios, it would be easier for a user to have create the Trigger configuration part as part of config with code. The cluster admin/operator can create the EventListeners, expose it to a public address, etc. And the same listener can be shared among many Triggers. This does reduce the number of listener Pods but also allows for a better separation of concerns among the user and the operator.

teps/000X-trigger-crd.md

dibyom · 2020-07-21T14:15:29Z

teps/000X-trigger-crd.md

+  name: my-repo-trigger
+spec:
+  serviceAccountName: "blah"
+  boundnamespace:


Instead of just a namespace selector, why not use a label based selector mechanism:
https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/

khrm has some YAML for the label selector scenario: https://gist.github.com/khrm/4f189926a62fcbed72fb4dd4273e21d1#file-label-selector-el-L9

One question with label selectors is -- is it possible to limit the selection to a single namespace if needed? I tried looking into other k8s resources that use selectors -> NetworkPolicy, for instance, uses both an explicit namespaceSelector and a podSelector : https://kubernetes.io/docs/concepts/services-networking/network-policies/#networkpolicy-resource

So, if we want to support the use case where an EL is limited to only one namespace, it seems like we'd want to have something similar i.e. both a namespaceSelector and a podSelector.

Another question I have is that for the kinds of multi-tenanted scenarios that we are considering, do users usually have access to add labels to their namespaces? If not, maybe we'd need to have some way of selecting by namesapceName (like this proposal already states).

Adding both namespaceSelector as well as individual pod selectors initially seems complex. So, what I'm thinking is perhaps something like:

# intial release - just select on name spec: namespaceSelector: matchName: [ns-1, ns-2]

and then later we could add the label seelctors:

spec: namespaceSelector: matchName: [ns-1, ns-2] matchLabels: - someLabel: value triggerSelector: matchLabels: - someLabel: value

What do you think?

https://groups.google.com/forum/#!msg/kubernetes-sig-network/4Q1Xk1Jm4-M/UyNIW53XBAAJ

https://kubernetes.io/docs/concepts/services-networking/network-policies/#behavior-of-to-and-from-selectors

Yes, this looks quite promising.

I added this in Design section.

great thanks! what I'm thinking is we get started with just a namespaceSelector with matchName to start off with and then proceed to have the more complex selectors as needed

dibyom · 2020-07-21T14:30:10Z

teps/000X-trigger-crd.md

+  - "@kbaig"
+creation-date: 2020-07-14
+last-updated: 2020-07-14
+status: proposed


I think proposed is the right status here. While there is a Trigger CRD type, its main use is for experimentation with the CLI and it not usable by an end user. If we decide to withdraw this proposal for any reason, we can do it without any changes to the API. In addition, there is more to this proposal than just adding the CRD type (e.g. selectors, path based triggers etc.)

teps/000X-trigger-crd.md

dibyom · 2020-07-21T14:54:45Z

teps/000X-trigger-crd.md

+know that this has succeeded?
+-->
+1. Reduction in resource cosumption due to EventListener.
+2. Ability to have single EventListener to cater to whole cluster.


Another goal - separation of concerns. Today listeners are the type that a user creates. Say I have a git repo and I want to create a push trigger, I create an EventListener for it. Next, if I want to add an additional trigger, (say a pull req trigger, or an image push trigger), do I create a new EL or do I add an additional trigger to an EL? What if I add a new repo? A new EL means a new Pod/Deployment/Service. Besides just the resource consumption, there are other concerns. Usually you'd have to expose your EL via an Ingress or some other means. The more ELs I have the more additional configuration I need - in some cases this additional bit of configuration is something that a cluster admin/operator would perform not an user/app developer.

For declarative config as code scenarios, it would be easier for a user to have create the Trigger configuration part as part of config with code. The cluster admin/operator can create the EventListeners, expose it to a public address, etc. And the same listener can be shared among many Triggers. This does reduce the number of listener Pods but also allows for a better separation of concerns among the user and the operator.

dibyom · 2020-07-21T14:57:03Z

teps/000X-trigger-crd.md

+
+[experience reports]: https://github.com/golang/go/wiki/ExperienceReports
+-->
+Today, EventListener commonly are created for every namespace and handle TriggerBinding and


My understanding is that we'd still recommend one EL per namespace (though if one wants they could setup an EL to cater to a whole cluster). This should still be beneficial from a resource consumption standpoint since currently we have many ELs per namespace (one per repo/trigger etc.)

Yes. Should I rephrase this?

dibyom · 2020-07-22T13:57:11Z

teps/000X-trigger-crd.md

+
+### Path based EventListener
+Inside EventListener, we will deduce Trigger to execute based on the path of the request URL.
+To refer to trigger resource foo in namespace bar, we would url ```/bar/foo```. Instead of 


In this model, is there a way to serve multiple Triggers from the same webhook without having to deliver the webhook twice?

No. I don't think so.

Hmm, so this is a breaking change in how Triggers works today i.e. the lack of support of multiple triggers from the same webhook event. We should highlight this and document possible alternatives/workarounds.

one idea: Maybe one Trigger CRD can contain multiple triggers?

I found a workaround on this. We can pass label information using query in url to select TriggerCRD. Will that work?

That's an interesting alternative....we can try out an example maybe like creating a GitHub webhook with query params and see if that works?

If the labels change, we'd have to reconfigure the webhook.

I tested Github Webhook with query params and it works with the query params.

Thanks, query params seem like an interesting idea though I still think it might be worth exploring if we can put multiple configs in the same Trigger or other alternatives which do not require label manipulation. I do think that we can work on this iteratively i.e we can get this TEP merged as proposed and then work on this part of the design more?

Yes, that works for me.

teps/000X-trigger-crd.md

gabemontero · 2020-08-04T13:22:58Z

teps/000X-trigger-crd.md

+metadata:
+  name: my-repo-trigger
+spec:
+  serviceAccountName: "blah"


Observation: specifying just the SA name here, with the implication that the SA is in the same namespace as the Trigger is a change from https://github.com/tektoncd/triggers/blob/master/pkg/apis/triggers/v1alpha1/trigger_types.go#L40-L42 where a "full" object ref, including namespace was provided.

Now, from a "security" perspective, this is a positive move IMO.

That said, in our prior discussions on this, I am not sure if we nailed down the existing scenarios that required a full object reference there.

Do any of the reviewers recall?

From a TEP perspective, calling out that difference is probably important.

I still somewhat wonder if independent of this TEP, we should switch that ref from global to namespace scope ...

The implication being that a SA from a privileged namespace could be specified with a ObjectReference vs. a LocalObjectReference.

That said, in our prior discussions on this, I am not sure if we nailed down the existing scenarios that required a full object reference there.

I think you are correct. And I agree we should call this out. We should discuss this - this has come up in tektoncd/triggers#679 maybe we can continue the discussion there?

teps/0008-trigger-crd.md

dibyom · 2020-08-07T01:40:27Z

teps/0008-trigger-crd.md

+We have two type of users:
+1. End users who requires his event to be processed.
+2. Operator/admin who managed EventListener.
+#### Trigger Ref inside the EventListener


we can call this section end users and describe how it works for end users? In addition to what you already have, maybe we can add how a end user goes about using it once a trigger is created i.e. getting listener address from status, setting up webhook of the form http(s)://url/namespace/trigger . Also, how they can find the logs - find listener pod and then get logs for those?

Yeah I would have expected something like "As an end user, I am only concerned with defining the Trigger* objects in my namespace in order to have my SCM system trigger my Tekton pipielines, and do not want to have to manage the event listener sink myself."

And then "As a cluster administrator, I want to be able to manage the ingress points into my cluster's various use of tekton triggers, minimizing the amount of EventListerners/Sinks needed, and thus be able to absolve my end users from managing these http endpoints."

I updated this section.

teps/0008-trigger-crd.md

dibyom · 2020-08-07T01:44:53Z

teps/0008-trigger-crd.md

+Trigger will contain serviceaccount, triggerbinding, triggertemplate and interceptor.ServiceAccount
+will be optional. If not defined, ServiceAccount of EventListener will be used.
+
+#### Specifying Namespaces where EventListener can serve


Similar to above, this section looks like that the user story would be for an operator user. We could describe that an operator can create one or more listeners and then use a combination of namespace and trigger selectors to "select" which triggers/namespaces to serve. They can expose the Listener via Ingress/LoadBalancer and can retain write access to the Listener objects. They can choose to restrict write access to Listener objects while allowing users with read access to use the Listener/get logs etc.

+1 on the user story feel .... "As a cluster administrator, I want ot control which namespaces the HTTP endpoints needed for tekton triggers reside in".

I updated this section.

teps/0008-trigger-crd.md

dibyom · 2020-08-07T02:22:16Z

Some comments mainly on the user stories section. But beyond that, I'd like to get a few other folks to review this as well!
maybe @bobcatfish since you already did a first pass. And @wlynch @gabemontero @bigkevmcd @vdemeester ?

gabemontero

Finally able to give this more of a thorough read. Apologies for the delay. High level, perhaps still a few blind spots where folks of the WG may know the specifics/details, but the uninitiated reader would not.

teps/0008-trigger-crd.md

gabemontero · 2020-08-10T01:34:30Z

teps/0008-trigger-crd.md

+We have two type of users:
+1. End users who requires his event to be processed.
+2. Operator/admin who managed EventListener.
+#### Trigger Ref inside the EventListener


Yeah I would have expected something like "As an end user, I am only concerned with defining the Trigger* objects in my namespace in order to have my SCM system trigger my Tekton pipielines, and do not want to have to manage the event listener sink myself."

And then "As a cluster administrator, I want to be able to manage the ingress points into my cluster's various use of tekton triggers, minimizing the amount of EventListerners/Sinks needed, and thus be able to absolve my end users from managing these http endpoints."

gabemontero · 2020-08-10T01:35:23Z

teps/0008-trigger-crd.md

+Trigger will contain serviceaccount, triggerbinding, triggertemplate and interceptor.ServiceAccount
+will be optional. If not defined, ServiceAccount of EventListener will be used.
+
+#### Specifying Namespaces where EventListener can serve


+1 on the user story feel .... "As a cluster administrator, I want ot control which namespaces the HTTP endpoints needed for tekton triggers reside in".

teps/0008-trigger-crd.md

gabemontero · 2020-08-10T01:43:45Z

teps/0008-trigger-crd.md

+  template: "ref-to-my-template"
+ status:
+   address:
+      url: "el-my-svc.cluster.local" # could also be an IP address?


Indicated by some of the questions/comments below, some detail in this section on what exactly this URL gets the Trigger author, etc. is needed.

Is it simply and identifier? Is it the URL the trigger author configures in their SCM (github/gitlab, etc) webhook configuration? If the trigger author does a curl against that URL, what should they expect?

gabemontero · 2020-08-10T01:46:05Z

teps/0008-trigger-crd.md

+will function in the same way as controller. It will be the responsibility of Operator
+or admin to manage that.
+2. Log: How will user access the EventListener log to debug their events. This can be addressed via
+emitting kubernetes events by EventListener and tkn-cli.


I know in WG we discussed some form of trigger specific log snippet from the EL log into the trigger status ... did you decide to abandon this @khrm ?

Yes, we decided to abandon it. It might require another CRD itself.

teps/0008-trigger-crd.md

gabemontero

Just a minor comment on user story wording, but I'm deferring if everybody else is OK, and merging this as proposed.

gabemontero · 2020-08-13T20:22:56Z

teps/0008-trigger-crd.md

+1. End users who requires his event to be processed.
+2. Operator/admin who managed EventListener.
+
+#### End User handling Webhook Use Cases using TriggerCR


Still not using the classic user story wording of "As a , I want ..."

But I won't block on that if everybody else is ok with it. The separation of concerns that is at the heart of the user stories and different roles is written down / captured in some form.

dibyom · 2020-08-13T21:16:02Z

Looking good! Like @gabemontero said, there are a couple of minor things we could fix. But that can be done once this is merged as proposed as well -- I'm happy to volunteer if you want :)

In terms of next steps, I think:

Merge this PR
Make any minor fixes to the PR
Open a new PR to change status to "implementable".
Gather feedback on that PR (I think we should call it out in the API WG and the Tekton WG on Wednesdays as well)
If we absolutely need to (though hopefully not 🤞), we can reduce some of the scope around paths/selectors and only proceed with the Trigger CRD with references and open new TEPs for those!

/approve

tekton-robot · 2020-08-13T21:16:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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:

~~teps/OWNERS~~ [dibyom]

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

teps/0008-trigger-crd.md

teps/0009-trigger-crd.md

dibyom · 2020-08-18T01:35:53Z

A few minor comments on the Table of Contents but otherwise 👍 from me!

vdemeester · 2020-08-18T10:05:19Z

teps/0009-trigger-crd.md

+URL ```/bar?label=app-foo```,  EventListener will only process all the Triggers matching labels
+```app-foo```.


nit: I think this messes up the GitHub render, there should be only one backtic instead of three.

This proposal is to extract out Trigger definition from EventListener Spec into its own CRD to resolve the problem of Multitenant EventListener.

vdemeester · 2020-08-18T10:25:33Z

/lgtm

I had initially intendend to give this to malancas@ which is the user right above in this file, must have put it in the wrong spot. Signed-off-by: Priya Wadhwa <[email protected]> Signed-off-by: Priya Wadhwa <[email protected]>

tekton-robot requested review from abayer and vdemeester July 14, 2020 16:05

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 14, 2020

khrm changed the title ~~Create 000X-trigger-crd.md~~ TEP-000X: Trigger CRD Jul 14, 2020

khrm force-pushed the trigger-crd branch from d33fb8b to ec7f621 Compare July 14, 2020 16:17

bobcatfish reviewed Jul 16, 2020

View reviewed changes

dibyom mentioned this pull request Jul 20, 2020

Add a new CRD type called Trigger tektoncd/triggers#628

Merged

3 tasks

dibyom reviewed Jul 21, 2020

View reviewed changes

dibyom reviewed Jul 22, 2020

View reviewed changes

teps/000X-trigger-crd.md Outdated Show resolved Hide resolved

teps/000X-trigger-crd.md Outdated Show resolved Hide resolved

savitaashture reviewed Jul 24, 2020

View reviewed changes

teps/000X-trigger-crd.md Outdated Show resolved Hide resolved

savitaashture reviewed Jul 24, 2020

View reviewed changes

teps/000X-trigger-crd.md Outdated Show resolved Hide resolved

khrm force-pushed the trigger-crd branch 2 times, most recently from 0b209f9 to f60a275 Compare July 28, 2020 16:07

gabemontero reviewed Aug 4, 2020

View reviewed changes

khrm force-pushed the trigger-crd branch 3 times, most recently from bb50864 to bdca2cf Compare August 6, 2020 20:20

khrm changed the title ~~TEP-000X: Trigger CRD~~ TEP-0008: Trigger CRD Aug 6, 2020

khrm force-pushed the trigger-crd branch from bdca2cf to ea3030f Compare August 6, 2020 20:26

dibyom reviewed Aug 7, 2020

View reviewed changes

gabemontero suggested changes Aug 10, 2020

View reviewed changes

This was referenced Aug 10, 2020

Proposal: Make triggers less generic and more coupled to Pipelines tektoncd/triggers#697

Open

switch trigger sa ref from global to namespace scoped tektoncd/triggers#704

Merged

dibyom reviewed Aug 10, 2020

View reviewed changes

teps/0008-trigger-crd.md Outdated Show resolved Hide resolved

khrm force-pushed the trigger-crd branch 4 times, most recently from 2242434 to af63904 Compare August 11, 2020 16:08

khrm force-pushed the trigger-crd branch 3 times, most recently from eb7b4b3 to e1d8467 Compare August 12, 2020 19:18

khrm changed the title ~~TEP-0008: Trigger CRD~~ TEP-0009: Trigger CRD Aug 12, 2020

khrm force-pushed the trigger-crd branch 2 times, most recently from 6b5ccda to e531d08 Compare August 12, 2020 19:40

gabemontero reviewed Aug 13, 2020

View reviewed changes

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

dibyom reviewed Aug 13, 2020

View reviewed changes

teps/0008-trigger-crd.md Outdated Show resolved Hide resolved

teps/0008-trigger-crd.md Outdated Show resolved Hide resolved

teps/0008-trigger-crd.md Outdated Show resolved Hide resolved

AlanGreene mentioned this pull request Aug 15, 2020

Multi-Tenant EventListener tektoncd/triggers#456

Closed

khrm force-pushed the trigger-crd branch 2 times, most recently from 4a037a9 to b2204ee Compare August 17, 2020 16:42

dibyom reviewed Aug 18, 2020

View reviewed changes

khrm force-pushed the trigger-crd branch 2 times, most recently from 4bb45c6 to e4ca8a4 Compare August 18, 2020 09:32

vdemeester reviewed Aug 18, 2020

View reviewed changes

khrm force-pushed the trigger-crd branch 2 times, most recently from ad7af26 to 8b0983f Compare August 18, 2020 10:23

TEP-0009: Trigger CRD

c4a12a0

This proposal is to extract out Trigger definition from EventListener Spec into its own CRD to resolve the problem of Multitenant EventListener.

khrm force-pushed the trigger-crd branch from 8b0983f to c4a12a0 Compare August 18, 2020 10:23

tekton-robot assigned vdemeester Aug 18, 2020

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

tekton-robot merged commit 4a1bea5 into tektoncd:master Aug 18, 2020

dibyom mentioned this pull request Aug 26, 2020

Changing the Triggers CRD TEP Status to Implementable #184

Merged

khrm deleted the trigger-crd branch July 29, 2021 11:43

		URL ```/bar?label=app-foo```, EventListener will only process all the Triggers matching labels
		```app-foo```.

TEP-0009: Trigger CRD #148

TEP-0009: Trigger CRD #148

Conversation

khrm commented Jul 14, 2020 • edited Loading

bobcatfish 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 Jul 21, 2020 • 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

dibyom Aug 4, 2020 • 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

dibyom Jul 21, 2020 • 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

khrm Jul 28, 2020 • 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

gabemontero Aug 4, 2020 • 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

dibyom commented Aug 7, 2020

gabemontero 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

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

dibyom commented Aug 18, 2020

Choose a reason for hiding this comment

vdemeester commented Aug 18, 2020

khrm commented Jul 14, 2020 •

edited

Loading

dibyom Jul 21, 2020 •

edited

Loading

dibyom Aug 4, 2020 •

edited

Loading

dibyom Jul 21, 2020 •

edited

Loading

khrm Jul 28, 2020 •

edited

Loading

gabemontero Aug 4, 2020 •

edited

Loading