Adding support for CloudEvents WebHook Spec

[n3wscott]

Problem
The CloudEvents WG has defined a webhook spec that we at Knative have mostly overlooked.

Persona:
Knative Developers and Integrators

Exit Criteria
Knative can be subscribers for producers that follow the webhook specification. And we can deliver to subscribers that implement the webhook spec.

Time Estimate (optional):
1 week

Additional context (optional)
Work has started to add support for webhook spec in the GoLang sdk: cloudevents/sdk-go#491

[slinkydeveloper]

@n3wscott I don't get where do you plan to support it, can you elaborate it a bit further?

[n3wscott]

This would be need to be added in the following locations:

• For any http based consumers (HTTP Ingress source that is consuming CloudEvents)
• Channel ingress would be a Consumerm(if exposed off cluster)
• Channel Invokers could choose to implement the Producer side.
• Broker ingress would be a Consumer (if exposed off cluster)
• Broker Invokers could choose to implement the Producer side.
• Flows as well if they implement any active layers between the underlying channels. (if exposed off cluster)

FOLLOWUP: the above list will only be needed if each component is in different clusters talking over https.

Basically everywhere that accepts or sends a CloudEvents over http.

NOTE: the webhook spec is optional, but it is also optional to send to a consumer that does not implement it. So in strict mode cloudevents, Knative components will not interoperate with other cloudevents implementors that use the webhook spec.

[n3wscott]

OK! I am miss-understanding the spec.

It seems like the webhook spec is for anything that would like to bridge between two systems, such as HTTP ingress for cloudevents.

the proposal for next actions is this:

• Implement an ingress source that implements the webhook spec (consumer side).
• Implement an egress sink that implements the wehbook spec (producer side).

[n3wscott]

The webhook spec would still apply to any subscriber or sink from a broker or channel that is using an off-cluster url. That off-cluster consumer might implement the webhook spec and reject events and we will not understand how to deal with that.

[slinkydeveloper]

• Implement an ingress source that implements the webhook spec (consumer side).
• Implement an egress sink that implements the wehbook spec (producer side).

So If i understand correctly:

• for sources we just need to do it in the sdk
• for channel, we need to add this handshake to https://github.com/knative/eventing/blob/master/pkg/kncloudevents/message_sender.go and https://github.com/knative/eventing/blob/master/pkg/kncloudevents/message_receiver.go

What I'm thinking is: do we really need it on the sender side? The receiver webhook compliant expects always the options dance? Or can we skip that part?

[slinkydeveloper]

If we don't implement any auth/rate limiting feature described by the webhook, can we just skip, for now, the implementation of the handshake on sender side?

[n3wscott]

The options dance is optional for the producer, but not optional for a consumer to respond to if it would like to accept events from an options-enabled producer if that makes sense.

[slinkydeveloper]

Good, I can work on that for https://github.com/knative/eventing/blob/master/pkg/kncloudevents/message_receiver.go as soon as the sdk is enabled for supporting it

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[slinkydeveloper]

/reopen

[knative-prow-robot]

@slinkydeveloper: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[slinkydeveloper]

/unassign

I'm not working actively on this anymore

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[evankanderson]

/remove-lifecycle stale

[evankanderson]

It would be nice for Broker/Channel to implement this on both sides:

1. Ingress: respond to HTTP OPTIONS with Allow: Post and Webhook-Allowed-Origin: *
2. Sending: preflight HTTP OPTIONS with Webhook-Request-Origin of a DNS name for the subscriber (?). In a variance from the webhook spec, I'd suggest that no throttling or rate limiting be applied for now. At some point, we might want to have a default rate limit of e.g. 120 events/minute if the client doesn't provide otherwise, but I don't think we do any pacing right now so I wouldn't worry about that yet.