Implementing GitHub webhook handler #331
Conversation
There was a problem hiding this comment.
Overall LGTM. We might want to merge the base of this PR into master though, or its PR will become massive by the time we're done.
@r0mant, would you rather we file a bunch of smaller broken/development/not building PRs (which you'll need to review as only eng leads are codeowners), or one massive one later?
| head.GitHubHookInstallationTargetID = r.Header.Get("X-GitHub-Hook-Installation-Target-ID") | ||
| head.HubSignature256 = r.Header.Get("X-Hub-Signature-256") | ||
|
|
||
| payload, err := github.ValidatePayload(r, h.secretToken) // If secretToken is empty, the signature will not be verified. |
There was a problem hiding this comment.
I think that we should error if the secret token is empty and a signature is provided. If this happens, we've probably misconfigured something.
libs/github/webhook/webhook.go
Outdated
|
|
||
| payload, err := github.ValidatePayload(r, h.secretToken) // If secretToken is empty, the signature will not be verified. | ||
| if err != nil { | ||
| h.log.Warn("webhook validation failed", "headers", head) |
There was a problem hiding this comment.
In all these cases we should probably include some information about the request itself in the log, for debugging purposes. Or maybe the handler (or possibly the ingress gateway) should be able to log the entire request if debug logging is enabled?
| }) | ||
| mux.Handle("/webhook", webhook.NewHandler( | ||
| eventProcessor, | ||
| webhook.WithSecretToken([]byte("secret-token")), // TODO: get from config |
There was a problem hiding this comment.
Maybe this should always take a string? It's a string when configured with GH, and I don't see when we would want to pass a byte array to this func.
| ghes.srv.Shutdown(context.Background()) // Ignore error - we're already handling one | ||
| case <-ctx.Done(): | ||
| err = ghes.srv.Shutdown(context.Background()) |
There was a problem hiding this comment.
These should probably take a time-limited shutdown context stemming from context.Background().
| deployReviewChan <- event | ||
| return nil | ||
| default: | ||
| return trace.Errorf("unknown event type: %T", event) |
There was a problem hiding this comment.
This library only exists for Teleport error handling, so we probably shouldn't be using it anymore outside of the product (here and elsewhere)
Created an implementation for the GitHub webhook. It is just a small wrapper around the go-github library.
Made some changes to the main function to use
errgroupto run theeventSourcesconcurrently. Theerrgrouplibrary handles propagating cancellations that result from errors and is similar in functionality to waitgroups. In the event of an error from one of theeventSources, theerrgroupwill cancel the context which all goroutines in the group can react to appropriately.