Support trigger via CloudEvents #1439

afrittoli · 2022-09-01T12:05:32Z

Feature request

Support CloudEvents compliant responses.

I see a few options:

When a cloudevent is received, respond with a cloudevent. The current body could be placed in the payload.
When a cloudevent is received, respond with no payload
Allow users to disable the response payload platform wise

Use case

When the project was started, it was decided to support generic HTTP requests with JSON payload rather than focus specifically on CloudEvents. The current behaviour of triggers is not compliant with the CloudEvents spec though, which makes it hard to use Triggers as a sink for CloudEvents.

When using triggers with Knative broker, the broker checks if the response is a cloud event, or empty, and reports a 502 back otherwise. See the code and the logs from a test cluster:

{"level":"debug","ts":"2022-09-01T10:59:16.299Z","logger":"mt_broker_filter","caller":"filter/filter_handler.go:173","msg":"Received message","commit":"56edb3b","triggerRef":"default/all-to-tekton-el-events"}
{"level":"debug","ts":"2022-09-01T10:59:16.299Z","logger":"mt_broker_filter","caller":"filter/filter_handler.go:353","msg":"Applying attributes filter.","commit":"56edb3b","trigger":"default/all-to-tekton-el-events","filter":{}}
{"level":"debug","ts":"2022-09-01T10:59:16.350Z","logger":"mt_broker_filter","caller":"filter/filter_handler.go:222","msg":"Successfully dispatched message","commit":"56edb3b","target":"http://el-events.default.svc.cluster.local:8080"}
{"level":"error","ts":"2022-09-01T10:59:16.350Z","logger":"mt_broker_filter","caller":"filter/filter_handler.go:227","msg":"failed to write response","commit":"56edb3b","error":"received a non-empty response not recognized as CloudEvent. The response MUST be either empty or a valid CloudEvent","stacktrace":"knative.dev/eventing/pkg/broker/filter.(*Handler).send\n\tknative.dev/eventing/pkg/broker/filter/filter_handler.go:227\nknative.dev/eventing/pkg/broker/filter.(*Handler).ServeHTTP\n\tknative.dev/eventing/pkg/broker/filter/filter_handler.go:209\ngo.opencensus.io/plugin/ochttp.(*Handler).ServeHTTP\n\tgo.opencensus.io@v0.23.0/plugin/ochttp/server.go:92\nknative.dev/pkg/network/handlers.(*Drainer).ServeHTTP\n\tknative.dev/pkg@v0.0.0-20220524202603-19adf798efb8/network/handlers/drain.go:110\nnet/http.serverHandler.ServeHTTP\n\tnet/http/server.go:2879\nnet/http.(*conn).serve\n\tnet/http/server.go:1930"}

The text was updated successfully, but these errors were encountered:

We use gitea as SCM provider, so this includes some values for deploying gitea on the IBM cloud cluster. Repo can be cloned anonymously but anything else requires login. Tekton triggers and pipeline are going to be used, in combination with a new tool, "cdeventer", to react to webhook/events and produce cdevents. We can use vanilla Tekton here so pretty much just install the components for now. The Tekton dashboard is handy for the demo, and it's deployed behind an oauth-proxy to secure it via GitHub login. cdeventer itself is a controller plus tekton resources. The controller for now is added on the cluster using ko, and it's not yet included in this commit. The Tekton resources are developed in the POC and will be copied over to cdeventer later on to make the more reusable. The sink for CDEvents is a CloudEvent broker provided by Knative with its in-memory channel implementation. Knative triggers are used to define subscriptions and get CDEvents to the interested parties. The flow to produce CDEvent for now goes as follows: - demo developer merges a PR in gitea - webhook goes to an event listener in the tekton-ci namespace - the tekton trigger/binding extracts the data required for the change CDEvent from the webhook headers and payload - the tekton trigger template creates a "Run" i.e. a custom task run with the data for the CDEvent - the cdeventer custom task reconciler reconciles the "Run". It uses the new go-sdk to build, validate and send a CDEvent to the CloudEvents broker Now, to consume / display the CDEvent, the flows for now is: - an all catching knative trigger gets the CDEvent to a different Tekton event-listener, el-events, in the default namespace - *note* the knative broker must be set to 0 retries because of tektoncd/triggers#1439 - A Tekton trigger matched on CDEvents runs a display pipeline that shows headers and payload of the CDEvent in the Tekton dashboard Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>

dibyom · 2022-09-01T18:38:47Z

We can probably add a cloud event response type behind a opt in flag?

afrittoli · 2022-09-01T19:18:00Z

We can probably add a cloud event response type behind a opt in flag?

It sounds good, but I think it should be only for incoming CloudEvents.

dibyom · 2022-09-01T20:04:20Z

Are you saying we return the current response if the input is not a cloud event? I was thinking we could eventually stabilize on always returning a cloud event?

afrittoli · 2022-09-01T20:11:38Z

Are you saying we return the current response if the input is not a cloud event? I was thinking we could eventually stabilize on always returning a cloud event?

I guess we could... I'm not sure if there is any spec about expected replies for a webhook.
If we use CloudEvents binary format, the body would not change at all, only HTTP headers would be added... so I guess we could send that to all eventually.

dibyom · 2022-09-14T16:14:42Z

Yeah, there are no expected replies as far as I know (other than the HTTP status code)

Fabian-K · 2022-09-23T09:32:40Z

Just to add some context/motivation. This is especially an issue in combination with knatives retry function (https://knative.dev/docs/eventing/event-delivery/). Because of the unexpected response, it is treated as a failure and retried. The retry results in duplicated resources created by triggers.

bigkevmcd · 2022-10-05T08:10:43Z

I ran into this too, and I had a look at the code...

https://github.com/tektoncd/triggers/blob/main/pkg/sink/sink.go#L211-L223

We could detect CloudEvent requests there, and either write the response as JSON, or, as a CloudEvent message

afrittoli · 2022-10-05T08:54:47Z

I think the response is JSON today, but there are no CloudEvents headers, so it's rejected by the broker

bigkevmcd · 2022-10-05T09:24:52Z

That's why we'd write it as a CloudEvent message (using the cloudevents code) which would set the response correctly.

bigkevmcd · 2022-10-05T10:43:37Z

I had a crack at this earlier this morning main...bigkevmcd:triggers:cloudevents-triggers it needs a bit of tidying but the test passes.

treyhyde · 2022-10-27T01:17:53Z

I make extensive use of triggering pipelines via cloudevents delivered from knative... so that works... but I'm forced to disable retry since knative thinks all the responses are bogus regardless of the 202 since it responds with JSON that isn't a CE.

Doing any of the things in the original topic here works for me, even if guarded by a feature flag.

My old discussion on the subject: https://knative.slack.com/archives/C017X0PFC0P/p1652901710903979

tekton-robot · 2023-01-25T02:11:14Z

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.

daper · 2023-01-25T09:17:41Z

/remove-lifecycle stale

tekton-robot · 2023-04-25T09:20:37Z

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.

daper · 2023-04-25T10:46:30Z

/remove-lifecycle stale

afrittoli · 2023-04-25T11:07:40Z

@dibyom i think this has been implemented, right?

savitaashture · 2023-05-30T04:18:25Z

There is a implementation related to slack interceptor which also supports http form data as part of this #1548 which is released in Triggers v0.24

@afrittoli does that PR helps ?

afrittoli · 2023-05-30T10:41:17Z

Thanks @savitaashture - that's a nice feature to have, but I don't think it would help with CloudEvents specifically.
The problem with CloudEvents is that the specification mandates the reply to be compliant with the CloudEvents specification.

I believe that has been addressed and solved in #1469

khrm · 2023-05-30T13:00:11Z

Yes, it has been resolved by #1469 by following 1st approach. I don't see a strong need for disabling the response.

afrittoli added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 1, 2022

dibyom added the area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) label Sep 23, 2022

khrm assigned bigkevmcd Oct 19, 2022

treyhyde mentioned this issue Oct 27, 2022

Feature: EventListener response customization #788

Open

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 25, 2023

tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 25, 2023

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2023

tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2023

khrm closed this as completed May 30, 2023

afrittoli mentioned this issue Sep 27, 2023

Event listener restarts when under minimal stress #1618

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support trigger via CloudEvents #1439

Support trigger via CloudEvents #1439

afrittoli commented Sep 1, 2022

dibyom commented Sep 1, 2022

afrittoli commented Sep 1, 2022

dibyom commented Sep 1, 2022

afrittoli commented Sep 1, 2022

dibyom commented Sep 14, 2022

Fabian-K commented Sep 23, 2022

bigkevmcd commented Oct 5, 2022

afrittoli commented Oct 5, 2022

bigkevmcd commented Oct 5, 2022

bigkevmcd commented Oct 5, 2022

treyhyde commented Oct 27, 2022

tekton-robot commented Jan 25, 2023

daper commented Jan 25, 2023

tekton-robot commented Apr 25, 2023

daper commented Apr 25, 2023

afrittoli commented Apr 25, 2023

savitaashture commented May 30, 2023

afrittoli commented May 30, 2023

khrm commented May 30, 2023

Support trigger via CloudEvents #1439

Support trigger via CloudEvents #1439

Comments

afrittoli commented Sep 1, 2022

Feature request

Use case

dibyom commented Sep 1, 2022

afrittoli commented Sep 1, 2022

dibyom commented Sep 1, 2022

afrittoli commented Sep 1, 2022

dibyom commented Sep 14, 2022

Fabian-K commented Sep 23, 2022

bigkevmcd commented Oct 5, 2022

afrittoli commented Oct 5, 2022

bigkevmcd commented Oct 5, 2022

bigkevmcd commented Oct 5, 2022

treyhyde commented Oct 27, 2022

tekton-robot commented Jan 25, 2023

daper commented Jan 25, 2023

tekton-robot commented Apr 25, 2023

daper commented Apr 25, 2023

afrittoli commented Apr 25, 2023

savitaashture commented May 30, 2023

afrittoli commented May 30, 2023

khrm commented May 30, 2023