Feature: TriggerGroups #1232

dibyom · 2021-10-01T21:01:29Z

Changes

This is a port of #1052 - mostly just fixes so that it can merge cleanly with main

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.

So far I've done the following

Rebased Initial implementation for trigger groups #1052 with latest main branch
Removed some old code that returned that waited for triggers to finish processing before returning
Move the types to v1beta1 (still need to delete it from v1alpha1)
Fix the correct body/header to triggers within a trigger group
Ensure we wait for triggers in triggerGroups to process to fix sink tests
Ported webhook validation to v1beta1
Added an extra validation check + tests to enable the field only when enable-api-fields is set to alpha
Moved the examples to v1beta1

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
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Adds a new TriggerGroups field to EventListener spec that allows users to specify a set of interceptors that will be executed
before a group of triggers are selected and executed. To enable this feature set `enable-api-fields` to `alpha` in the `feature-flags-triggers` config map.

tekton-robot · 2021-10-01T21:03:43Z

The following is the coverage report on the affected files.
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	97.7%	97.9%	0.1
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.8%	2.2

tekton-robot · 2021-10-06T18:04:45Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.8%	2.2

dibyom · 2021-10-06T18:09:41Z

Move behind an alpha feature gate

Added validation in the webhook. Should probably add another check in the reconciler too

Move examples to behind v1beta1 - would have to figure out how to enable feature gate for the example tests

Due to the allowlist in https://github.com/tektoncd/triggers/blob/main/test/e2e-tests-examples.sh#L138, the triggerGroup example would not be run. I'll work on something like https://github.com/tektoncd/pipeline/tree/main/docs/developers#example-yamls in a separate PR to enable the YAML test for this example.

tekton-robot · 2021-10-06T18:18:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.6%	2.1

dibyom · 2021-10-06T20:18:19Z

I'll add some extra validation in the reconciler so that the alpha fields cannot be used and rebase the commits but in the meantime, this should be ready for a review
/assign @wlynch
/assign @savitaashture

docs/eventlisteners.md

examples/v1beta1/triggergroups/eventlistener-triggergroup.yaml

pkg/apis/triggers/v1beta1/event_listener_validation.go

tekton-robot · 2021-10-11T15:48:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.6%	2.1

tekton-robot · 2021-10-11T16:37:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.6%	2.1

wlynch

Mostly LGTM! Just minor nits.

wlynch · 2021-10-11T17:53:15Z

pkg/apis/triggers/v1alpha1/event_listener_types.go

+type EventListenerTriggerGroup struct {
+	Name            string                       `json:"name"`
+	Interceptors    []*TriggerInterceptor        `json:"interceptors"`
+	TriggerSelector EventListenerTriggerSelector `json:"triggerSelector"`
+}
+
+type EventListenerTriggerSelector struct {
+	NamespaceSelector NamespaceSelector     `json:"namespaceSelector,omitempty"`
+	LabelSelector     *metav1.LabelSelector `json:"labelSelector,omitempty"`
+}
+


Missing godoc comments

Hmm, I thought I removed the types from v1alpha1.

I've removed the types from v1alpha1. They are only available in v1beta1

pkg/apis/triggers/v1beta1/event_listener_types.go

pkg/interceptors/server/server.go

wlynch · 2021-10-11T18:12:36Z

pkg/sink/sink.go

+}
+
+func (r Sink) selectTriggers(namespaceSelector triggersv1.NamespaceSelector, labelSelector *metav1.LabelSelector) ([]*triggersv1.Trigger, error) {
+	var trItems []*triggersv1.Trigger


Move this definition futher down to where its used.

ack. Keeping it here since it is also called by the HandleEvent function above.

wlynch · 2021-10-11T18:13:24Z

pkg/sink/sink.go

+
+func (r Sink) selectTriggers(namespaceSelector triggersv1.NamespaceSelector, labelSelector *metav1.LabelSelector) ([]*triggersv1.Trigger, error) {
+	var trItems []*triggersv1.Trigger
+	var err error


I think this can be removed? AFAICT in this func all error handling in this func is immediately followed with

if err != nil { return err }

I think its because targetLabels := labels.Everything() and then assigned to in inside the if, we have to pre-declare the err variable.

pkg/sink/sink.go

wlynch · 2021-10-11T18:28:29Z

pkg/sink/sink.go

@@ -264,10 +328,14 @@ func (r Sink) processTrigger(t triggersv1.Trigger, request *http.Request, event
 	go r.recordResourceCreation(resources)
 }

+func (r Sink) ExecuteTriggerInterceptors(t triggersv1.Trigger, in *http.Request, event []byte, log *zap.SugaredLogger, eventID string, extensions map[string]interface{}) ([]byte, http.Header, *triggersv1.InterceptorResponse, error) {
+	return r.ExecuteInterceptors(t.Spec.Interceptors, in, event, log, eventID, fmt.Sprintf("namespaces/%s/triggers/%s", t.Namespace, t.Name), t.Namespace, extensions)


Optional for this PR, but we might want to look into adding something like https://github.com/tektoncd/pipeline/blob/f8c2eea6ad6370cd290d937d143358e99c2d7ad6/pkg/apis/pipeline/v1beta1/pipelinerun_types.go#L114-L117

ack. will do in a new PR

wlynch · 2021-10-11T18:32:37Z

third_party/vendor/golang.org/x/crypto/LICENSE

@@ -1,27 +0,0 @@
-Copyright (c) 2009 The Go Authors. All rights reserved.


Do we now why these licenses are being deleted?

Not sure, its just what the hack/update-deps.sh script is doing. (I've seen the x/crypto/LICENSE being added/deleted a few times before).

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 · 2021-10-11T20:29:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.8%	2.2

wlynch

One last formatting fix, otherwise LGTM

docs/eventlisteners.md

This commit adds validation to ensure that triggerGroups can only be used in the v1beta1 apiVersion when `enable-api-fields` is set to `alpha`.

tekton-robot · 2021-10-12T01:07:06Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1beta1/event_listener_validation.go	97.7%	97.9%	0.2
pkg/interceptors/server/server.go	82.1%	82.5%	0.4
pkg/sink/sink.go	73.6%	75.8%	2.2

wlynch

/lgtm

tekton-robot · 2021-10-12T04:49:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

jeffmaury · 2021-10-21T16:41:26Z

pkg/apis/triggers/v1beta1/event_listener_types.go

-	LabelSelector      *metav1.LabelSelector  `json:"labelSelector,omitempty"`
-	Resources          Resources              `json:"resources,omitempty"`
+	// Trigger groups allow for centralized processing of an interceptor chain
+	TriggerGroups     []EventListenerTriggerGroup `json:"triggerGroups"`


Trying to update the JSON schema bundled with the IntelliJ and VSCode tooling but realized both Triggers and TriggerGroups fields are required. Shouldn't we add the omitempty annotation ?

Yup we should - they aren't both required in the sense that the validation will both are not provided.

Addresses comment: tektoncd#1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]>

Addresses comment: #1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]>

Addresses comment: tektoncd#1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]> (cherry picked from commit 4a9db17)

Addresses comment: #1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]> (cherry picked from commit 4a9db17)

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 1, 2021

tekton-robot requested review from khrm and wlynch October 1, 2021 21:01

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 1, 2021

dibyom force-pushed the feature/trigger-groups-implementation branch 3 times, most recently from aee6879 to 7f9eacd Compare October 5, 2021 18:55

dibyom added this to the Triggers v0.17 milestone Oct 6, 2021

tektoncd deleted a comment from tekton-robot Oct 6, 2021

dibyom force-pushed the feature/trigger-groups-implementation branch from 6227ffe to 0eee40a Compare October 6, 2021 18:14

tekton-robot assigned savitaashture and wlynch Oct 6, 2021

dibyom marked this pull request as ready for review October 6, 2021 20:18

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2021

savitaashture reviewed Oct 9, 2021

View reviewed changes

docs/eventlisteners.md Outdated Show resolved Hide resolved

savitaashture reviewed Oct 9, 2021

View reviewed changes

docs/eventlisteners.md Outdated Show resolved Hide resolved

savitaashture reviewed Oct 9, 2021

View reviewed changes

examples/v1beta1/triggergroups/eventlistener-triggergroup.yaml Show resolved Hide resolved

savitaashture reviewed Oct 9, 2021

View reviewed changes

pkg/apis/triggers/v1beta1/event_listener_validation.go Show resolved Hide resolved

dibyom force-pushed the feature/trigger-groups-implementation branch from 0eee40a to bce959a Compare October 11, 2021 15:47

dibyom force-pushed the feature/trigger-groups-implementation branch from bce959a to f86b654 Compare October 11, 2021 16:35

wlynch reviewed Oct 11, 2021

View reviewed changes

dibyom force-pushed the feature/trigger-groups-implementation branch from f86b654 to 691b204 Compare October 11, 2021 20:27

wlynch reviewed Oct 11, 2021

View reviewed changes

docs/eventlisteners.md Outdated Show resolved Hide resolved

Put triggerGroups behind alpha feature-gate

7da5283

This commit adds validation to ensure that triggerGroups can only be used in the v1beta1 apiVersion when `enable-api-fields` is set to `alpha`.

dibyom force-pushed the feature/trigger-groups-implementation branch from 691b204 to 7da5283 Compare October 12, 2021 01:04

wlynch approved these changes Oct 12, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2021

tekton-robot merged commit c24f7e2 into tektoncd:main Oct 12, 2021

This was referenced Oct 12, 2021

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

Closed

Initial implementation for trigger groups #1052

Closed

jeffmaury reviewed Oct 21, 2021

View reviewed changes

dibyom deleted the feature/trigger-groups-implementation branch October 21, 2021 20:02

dibyom added a commit to dibyom/triggers that referenced this pull request Oct 22, 2021

Add omitempty tag for TriggerGroups type

b971a24

Addresses comment: tektoncd#1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom mentioned this pull request Oct 22, 2021

Add omitempty tag for TriggerGroups type #1247

Merged

1 task

dibyom added a commit to dibyom/triggers that referenced this pull request Oct 22, 2021

Add omitempty tag for TriggerGroups type

4ec48fb

Addresses comment: tektoncd#1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]>

tekton-robot pushed a commit that referenced this pull request Oct 25, 2021

Add omitempty tag for TriggerGroups type

4a9db17

Addresses comment: #1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom added a commit to dibyom/triggers that referenced this pull request Nov 3, 2021

Add omitempty tag for TriggerGroups type

045711b

Addresses comment: tektoncd#1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]> (cherry picked from commit 4a9db17)

tekton-robot pushed a commit that referenced this pull request Nov 3, 2021

Add omitempty tag for TriggerGroups type

70f5497

Addresses comment: #1232 (comment) Signed-off-by: Dibyo Mukherjee <[email protected]> (cherry picked from commit 4a9db17)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: TriggerGroups #1232

Feature: TriggerGroups #1232

dibyom commented Oct 1, 2021 •

edited

Loading

tekton-robot commented Oct 1, 2021

tekton-robot commented Oct 6, 2021

dibyom commented Oct 6, 2021

tekton-robot commented Oct 6, 2021

dibyom commented Oct 6, 2021

tekton-robot commented Oct 11, 2021

tekton-robot commented Oct 11, 2021

wlynch left a comment

wlynch Oct 11, 2021

dibyom Oct 11, 2021

dibyom Oct 11, 2021

wlynch Oct 11, 2021

dibyom Oct 11, 2021

wlynch Oct 11, 2021

dibyom Oct 11, 2021

wlynch Oct 11, 2021

dibyom Oct 11, 2021

wlynch Oct 11, 2021

dibyom Oct 11, 2021

tekton-robot commented Oct 11, 2021

wlynch left a comment

tekton-robot commented Oct 12, 2021

wlynch left a comment

tekton-robot commented Oct 12, 2021

jeffmaury Oct 21, 2021

dibyom Oct 21, 2021

		@@ -1,27 +0,0 @@
		Copyright (c) 2009 The Go Authors. All rights reserved.

Feature: TriggerGroups #1232

Feature: TriggerGroups #1232

Conversation

dibyom commented Oct 1, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Oct 1, 2021

tekton-robot commented Oct 6, 2021

dibyom commented Oct 6, 2021

tekton-robot commented Oct 6, 2021

dibyom commented Oct 6, 2021

tekton-robot commented Oct 11, 2021

tekton-robot commented Oct 11, 2021

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

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

tekton-robot commented Oct 11, 2021

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 12, 2021

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Oct 1, 2021 •

edited

Loading