[usage] Add a placeholder Stripe webhook #11776
Conversation
|
started the job as gitpod-build-af-stripe-webhook.7 because the annotations in the pull request description changed |
|
I've put the webhook on the An alternative, would be to put the hook on |
andrew-farries
force-pushed
the
af/stripe-webhook
branch
from
ad83286 to
54cfb33
Compare
Register the handler with the underlying baseserver.
andrew-farries
force-pushed
the
af/stripe-webhook
branch
from
54cfb33 to
14a1607
Compare
| @@ -133,3 +136,7 @@ func registerGRPCServices(srv *baseserver.Server, conn *gorm.DB, stripeClient *s | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func registerHttpHandlers(srv *baseserver.Server, h *stripe.WebhookHandler) { | |||
| srv.HTTPMux().HandleFunc("/webhook", h.Handle) | |||
There was a problem hiding this comment.
I'd recommend prefixing this with something like /stripe/invoices/webhook. This makes it easier to debug when you see just a request HTTP path without knowing what it does.
There was a problem hiding this comment.
done: b0d3d89
| } | ||
|
|
||
| func (h *WebhookHandler) Handle(w http.ResponseWriter, req *http.Request) { | ||
| const maxBodyBytes = int64(65536) |
There was a problem hiding this comment.
I believe there' a middleware for this
There was a problem hiding this comment.
I couldn't find any middleware that does exactly this. Did you have something in mind?
There was a problem hiding this comment.
Found one in the echo library but I think it's not worth importing.
| payload, err := ioutil.ReadAll(req.Body) | ||
| if err != nil { | ||
| log.WithError(err).Error("Stripe webhook error when reading request body") | ||
| w.WriteHeader(http.StatusServiceUnavailable) | ||
| return | ||
| } | ||
|
|
||
| event := stripe.Event{} | ||
| if err := json.Unmarshal(payload, &event); err != nil { | ||
| log.WithError(err).Error("Stripe webhook error while parsing event payload") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
// pseudo from memory
dec := json.Unmarshaller(http.Body)
if err := dec.Unmarshal(&event); err != nil { ... }
This should work. You shouldn't need to read all of the body into memory first and only then start unmarshalling. By using the above, you can stream unmarshal and avoid some buffering.
| return &WebhookHandler{} | ||
| } | ||
|
|
||
| func (h *WebhookHandler) Handle(w http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
Also worth wrapping in middleware which rejects non application/json content type.
I'd avoid this. We don't have AuthN on the usage component so it shouldn't be directly exposed to the internet.
Do this, but proxy from Server to usage - server becomes the authN ingress, but you can keep logic in usage. |
|
Thinking about this, I'd actually implement the Usage part as a gRPC method which the |
Without AuthN on the My preference for putting the webhook on the |
Give it a more descriptive name.
To ensure requests have `application/json` `Content-Type` header set.
That's fair, my worry is the added complexity where you now have to reason through some parts of it being public, while others not. It increases the threat surface for the system but worse it causes extra cognitive load on the engineers. The current (unfavourable) case where only |
|
Closing, in favour of #11806 now that we've agreed that the public-api is a better home for the webhook. |
Description
Add a placeholder Stripe webhook to the usage component.
In later PRs, the webhook will receive
invoice.finalizedevents from Stripe so that we can update instance usage records to show that they have been included in a particular invoice.Related Issue(s)
Part of #9036 and #10937
How to test
stripeCLI into the workspace and runstripe login.usagecomponent locally:stripeevents to the local webhook endpoint:This should start a series of Stripe events (setting up a customer and payment methods), each of which should be handled with a
200 OKby the webhook:Release Notes
Documentation
Werft options: