brigadecore / brigade

Event-driven scripting for Kubernetes
https://brigade.sh/
Apache License 2.0
2.41k stars 246 forks source link

[Design] CloudEvents support for Generic Gateway #798

Closed dgkanatsios closed 5 years ago

dgkanatsios commented 5 years ago

This is the design proposal for CloudEvents support for Generic Gateway, as discussed in #764

Currently, Generic Gateway listens to "/webhook" for a simple JSON payload and will raise a "webhook" event that should be handled by brigade.js.

Generic Gateway could support latest version of CloudEvents v0.2. As per proposed implementation, Generic Gateway could listen to "/cloudevent" for such an event and handle it via a "cloudevent" event handler on brigade.js.

Some points:

  1. Relevant Brigade data (e.g. ref and commit) could be stored on Event's Data field. Brigade.js consumer should be able to see the entire payload, of course. 2 CloudEvents could support the existing security mechanism we already have for Generic Gateway (with a custom secret per Project)
  2. It should be made clear in the documentation that Generic Gateway supports a Generic webhook endpoint as well as a CloudEvents endpoint. In this context, maybe rename the Generic webhook endpoint to simple webhook?
  3. Brigade should have a dependency to github.com/cloudevents/sdk-go/v02

Thoughts?

radu-matei commented 5 years ago

I think the design described here would be a great first iteration!

vdice commented 5 years ago

My only thoughts are at a higher-level around the future of gateways and Brigade. I'm curious if it would be desirable that any additional gateways besides "legacy" (container registry, generic) should exist in separate repos and be pluggable by way of sub-charts to the main Brigade chart?

We're exploring this approach for the Github gateway as we transition to using a pluggable but separate brigade-github-app for this gateway functionality (and thus, pulling out the classic GH gateway code; see https://github.com/Azure/brigade/pull/797)

Each additional gateway being built as a standalone project might be preferable if Brigade is in a position to support this (or could be with a small amount of work). I suppose one drawback of standalone-but-composable gateways might be separate service IPs for each and the overhead of covering each w/ ingress/TLS?

However, as @dgkanatsios has the most recent experience with gateway work, I'd definitely defer to his experience and opinions around this idea.

radu-matei commented 5 years ago

@vdice - even with the current generic gateway, we have a separate IP for the service - the way we could help here would be to document how to properly set up an ingress with TLS, then expose a single public IP for your ingress, and not one for all gateways.

That being said, I fully agree on having multiple repos for gateways and composing them at the chart level.

dgkanatsios commented 5 years ago

thanks both for comments/review! A few things, mainly on context/naming:

  1. @vdice you mention generic gateway. Just to be on the same page, I suppose you mean the single generic gateway with the two webhooks (the simple one - which is already implemented - and the cloudevents one - which this issue tries to design)
  2. also @vdice you mention that generic gateway is considered legacy. I suppose again you mean the single generic gateway with the two endpoints (simple and cloudevents), correct? So, you think that the generic gateway should stay in the brigade repo?
  3. If the answer to (1) and (2) is false, then are we considering a separate gateway for cloud events? So the idea is to have two gateways, the already existing one and the one designed by this issue?
  4. @radu-matei yup, a simple tutorial with nginx/cert-manager for TLS and a couple of gateways behind that would be ideal!

I personally do agree that it would be good from an organizational perspective to have all gateways on their separate repos; I don't really see any cons on that. We could also create a fake gateway (on its separate repo, of course) that could serve as a starting point for people that want to design their own gateway (with their custom events etc.).

Looking forward to your responses on 1,2,3 to see how we will proceed.

vdice commented 5 years ago
  1. I indeed had in mind the current generic gateway with the one, current simple webhook

  2. I'm using the legacy term here to denote gateways that were added directly to the Brigade project itself before we started following the pattern of creating them in separate repos/projects and including them into a Brigade setup at time of install.

  3. Indeed, I was thinking that any new gateway would be built as a separate project and included at install time (read: added as an optional sub-chart to the Brigade chart). Naturally, since this is still just being discussed, we'd want to put this in writing in Brigade's docs before expecting this approach to be followed by contributors.

That being said, I think I didn't quite understand the vision of the generic gateway until the helpful clarifications above. If this gateway was always intended to grow in utility via additional endpoints (cloudevents being the next), then I can definitely see just adding this logic to the gateway itself, in this repo.

If for some reason a new endpoint made more sense to exist in the context of a separate/distinct new gateway, this is when I'd propose it live via a separate project. (In fact, down the road, I wonder if we'll want to break out the existing Container Registry gateway into a separate repo/chart.)

dgkanatsios commented 5 years ago

got it, thanks @vdice! so, the question is whether to add 'cloudevents' endpoint to the existing generic gateway or create a completely new gateway. Some thoughts:

PS. Once we decide about it, we can discuss where to write the code (here or in a different repo)

radu-matei commented 5 years ago

I think it makes sense to add another endpoint to the existing gateway.

vdice commented 5 years ago

I think it makes sense to add another endpoint to the existing gateway.

Agreed! Thanks for helping me understand.

dgkanatsios commented 5 years ago

thanks both, I'll work towards a PR for it on this repo. Maybe after v1.0 we can take the entire generic gateway out.

radu-matei commented 5 years ago

Sure, we can look at having a separate repo in the new org.