Interceptors Need Trigger Information

[dibyom]

Problem

The current gh-validate binary that is used as an EventInterceptor is very basic. For each request that comes in, it validates the webhook payload. To do this, it uses a single static webhook that is passed as an enviornment variable.

This means that users using this binary have to create a new deployment per webhook secret or use the same webhook secret across multiple webhooks. Both of these are less than ideal.

Ideas

Currently, the event listener sink forwards incoming requests as is to the interceptor. The interceptor needs a way to identify the incoming payload and associate it with an secretRef.key to use to fetch the right secret. A few ideas:

Option 1: Use a well-known field in the event payload e.g. repository.full_name for Github webhook events.

Option 2: Pass an identifier from the event listener e.g. the trigger.name field as a well defined header that the interceptor can use.

Option 3: Add an additional field to EventInterceptor that allows users to pass in key value pairs:

  triggers:
    - name: foo-trig
      interceptor:
       params:
         - name: "secretKeyRef"
           value: "secretKeyRefValue"
        objectRef:
          kind: Service
          name: gh-validate
          apiVersion: v1
          namespace: default

Options 1 and 2 do not require changing the API but it means that the interceptor needs a way to resolve the identifier to the secretRef.key -- either following some kind of naming convention or by using something like a ConfigMap that resolves the identifier to the secretRef

Option 3 is the simplest way forward for the interceptor but it does involve adding additional fields to the API.

[a-roberts]

I think this has came about based on the discussions @dibbles and I had with @vtereso and yourself @dibyom so this proposal is certainly welcome and necessary for the work we'd like to do moving off Knative in favour of using Triggers.

Specifically, we don't want every Trigger to be kicked off when a webhook event occurs (e.g. a Git push), and we only wish to have one EventListener we will be updating through our Go code (with a React frontend - the existing create webhook page anyone can use).

Option 3 looks to be the most powerful and flexible, and what I'm personally looking for. We're also looking for the headers to be available too (don't think this is there atm - it's just the body right?). Trigger name would definitely be useful too.

[dibbles]

Agree with @a-roberts (but then we did have a chat about this, so no big surprises there). I would add that:

• passing the trigger name would allow the logging system of the interceptor service to clearly highlight which decisions where made for which trigger - or at least log information that can be tracked to decisions made for a specific trigger

• making both the request body and headers available to the interceptor service allows the service to make decisions based on the content of the headers (not just the body) - this might be important to some people

• passing additional params allows an interceptor service to be coded in a generic way to check matching values from the event or header against param values - rather than interrogating other k8s objects .... admittedly the author of the service could code to interrogate values in other k8s resources.

In our current use case we'd be looking to do a simple header.eventtype=param1.value && body.reponame = param2.value (or some such)

[vtereso]

/assign

[dibyom]

We're also looking for the headers to be available too (don't think this is there atm - it's just the body right?)

At the moment, the sink forwards the entire request to the interceptor so the header should be available. If its not, that's a bug :)

[dibyom]

Specifically, we don't want every Trigger to be kicked off when a webhook event occurs (e.g. a Git push), and we only wish to have one EventListener we will be updating through our Go code (with a React frontend - the existing create webhook page anyone can use).

This sounds a lot like Filtering/Conditionals which I think @khrm is working on in #49 😄

passing the trigger name would allow the logging system of the interceptor service to clearly highlight which decisions where made for which trigger - or at least log information that can be tracked to decisions made for a specific trigger

I agree! We should definitely add this.

passing additional params allows an interceptor service to be coded in a generic way to check matching values from the event or header against param values - rather than interrogating other k8s objects .... admittedly the author of the service could code to interrogate values in other k8s resources.

Indeed! So, how does the user flow work for dashboard? The user specifies the secretKey and the secretRef and we either store that in the eventlistener (using the new header/params) or in a configMap (with a key that references the trigger name)?

Another idea I proposed was built-in interceptors

[dibbles]

Specifically, we don't want every Trigger to be kicked off when a webhook event occurs (e.g. a Git push), and we only wish to have one EventListener we will be updating through our Go code (with a React frontend - the existing create webhook page anyone can use).

This sounds a lot like Filtering/Conditionals which I think @khrm is working on in #49 😄

Indeed! it is exactly that.

passing additional params allows an interceptor service to be coded in a generic way to check matching values from the event or header against param values - rather than interrogating other k8s objects .... admittedly the author of the service could code to interrogate values in other k8s resources.

Indeed! So, how does the user flow work for dashboard? The user specifies the secretKey and the secretRef and we either store that in the eventlistener (using the new header/params) or in a configMap (with a key that references the trigger name)?

I would go with the name/value params on the eventlistener - as it is going to be more logical for a user to setup their eventlistener in one place. This does not prohibit an end user however from storing information in a configmap - given that they themselves will write their own interceptor service, they can write it to access their configmap should they wish.