tektoncd / triggers

Event triggering with Tekton!
Apache License 2.0
556 stars 417 forks source link

Add PagerDuty interceptor #1337

Open georgettica opened 2 years ago

georgettica commented 2 years ago

Feature request

I would like to have a PagerDuty Interceptor (possibly I will implement it myself if this has enough traction

Use case

As a developer, I want to act on pagerduty webhook actions and run tekton pipelines vai that info. Currently, adding the pagerduty event listener validation is not something easily done via CEL interceptors. I currently mitigated some of the risks I might have with this, but having an interceptor built in for this would make my life much easier

dibyom commented 2 years ago

hi @georgettica looking at the PagerDuty usage docs it does seem like payload validation would be hard with just CEL.

A custom interceptor for PagerDuty sounds good. I'll mention that the interceptor doesn't even have to be part of the Triggers code base if that makes it easier. You can write your own PagerDuty interceptor, install it in any cluster and use it. Eventually we'd like to have a catalog of interceptors that folks could install and use.

georgettica commented 2 years ago

so I will create an interceptor and link it here when it's created. this way once the catalog is created it will be added

dibyom commented 2 years ago

sounds great!

georgettica commented 2 years ago

@dibyom could you help me make this work https://github.com/georgettica/pagerduty-tekton-interceptor

the code looks ok but I am not sure how to invoke this

dibyom commented 2 years ago

hey @georgettica you are on the right track. I'll work on adding better docs for writing cluster interceptors but in the meantime you can take a look at this PR where I add a cluster interceptor for adding the Pull request body for a PR comment event: https://github.com/tektoncd/plumbing/pull/967/files

In particular, you'd need the main HTTP server as in tekton/ci/cluster-interceptors/add-pr-body/cmd/interceptor/main.go Also take a look at the test file to see how to unit test it https://github.com/tektoncd/plumbing/blob/4b254e24a51319ca3d3962d4472eb19a32b2bd06/tekton/ci/cluster-interceptors/add-pr-body/pkg/pr_test.go

To test it manually, you'd start the server and send it a HTTP request whose body would be a JSON of the InterceptorRequest struct.

georgettica commented 2 years ago

And to add this I will just add the custom interceptor? I'll take a look at you suggestions tomorrow

dibyom commented 2 years ago

And to add this I will just add the custom interceptor?

So, the PR I mentioned adds another cluster interceptor called add-pr-body so I thought it would be a helpful reference as you are writing your own pagerduty one. At its core, its just a HTTP server that receives requests in the InterceptorRequest format and responds with a InterceptorResponse.

bigkevmcd commented 2 years ago

@dibyom Reading this does make me wonder about adding a simple hmac('sha256', body, '', 'secret-name', 'namespace') type function to the CEL library which would make it fairly simple to handle signed requests from a upstreams.

I have a use for this for another project...

georgettica commented 2 years ago

If that wouy exist, I could write the whole interceptor by this

dibyom commented 2 years ago

@bigkevmcd yeah I think that would be a good addition!

shibumi commented 2 years ago

I don't know if an implementation in CEL is really that useful. I spotted two major vendors with webhook signatures so far:

The hooks are both very different and I would be afraid that a hmac('sha256', body, '', 'secret-name', 'namespace') function would only work for one specific vendor.

Also, for pagerduty you have to remove the v1= prefix + loop through multiple signatures and return true if one of them matches.

For Datatrans for instance the whole mechanism is already different. Instead of getting multiple signatures you get one signature and a timestamp and you have to use both for signature validation.

I am very confident that if you find a third vendor with signatures in webhooks the third vendor may have a different mechanism.

georgettica commented 2 years ago

Please TAL again at the repo, the code works with local testing

tekton-robot commented 2 years ago

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.

georgettica commented 2 years ago

/remove-lifecycle stale Please TAL as I don't want this issue to die

dibyom commented 2 years ago

hi @georgettica sorry for the delay - are you looking for a review of https://github.com/georgettica/pagerduty-tekton-interceptor or are you looking to get that merged into the triggers source tree?

georgettica commented 2 years ago

First a review to see if this is good. Second help to merge into this repo

tekton-robot commented 1 year ago

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.

georgettica commented 1 year ago

/lifecycle frozen

As I am looking for someone to look at my code and I think adding this to upstream might be useful for some people

vdemeester commented 1 year ago

/area roadmap