Skip to content

Commit

Permalink
Events of type "repository" now avoid checking hook id.
Browse files Browse the repository at this point in the history
Events of type "repository" having "action" delete must trigger the
deletion of registered repositories. Given we use the same
deserialization struct for both "repository" and "meta" events, we
erroneously checked the received hook id against the one stored in the
database, thus consistently failing.

Added fix, test case, and improved logs.
  • Loading branch information
blkt committed May 30, 2024
1 parent 16e3ca0 commit f866e6b
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 5 deletions.
26 changes: 21 additions & 5 deletions internal/controlplane/handlers_githubwebhooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,9 @@ func (s *Server) HandleGitHubAppWebhook() http.HandlerFunc {
Str("webhook-event-type", m.Metadata[events.GithubWebhookEventTypeKey]).
Str("providertype", m.Metadata[events.ProviderTypeKey]).
Str("upstream-delivery-id", m.Metadata[events.ProviderDeliveryIdKey]).
// This is added for consistency with logs in
// Entity Execution Aggregator
Str("event", m.UUID).
Logger()
ctx = l.WithContext(ctx)

Expand Down Expand Up @@ -380,14 +383,19 @@ func (s *Server) HandleGitHubAppWebhook() http.HandlerFunc {
l.Info().Str("message-id", m.UUID).Msg("publishing event for execution")
if err := res.iiw.ToMessage(m); err != nil {
wes.Error = true
log.Printf("Error creating event: %v", err)
l.Error().Err(err).Msg("Error creating event")
w.WriteHeader(http.StatusInternalServerError)
return
}

// This ensures that loggers on downstream
// processors have all log attributes
// available.
m.SetContext(ctx)

if err := s.evt.Publish(res.topic, m); err != nil {
wes.Error = true
log.Printf("Error publishing message: %v", err)
l.Error().Err(err).Msg("Error publishing message")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -442,6 +450,9 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc {
Str("webhook-event-type", m.Metadata[events.GithubWebhookEventTypeKey]).
Str("providertype", m.Metadata[events.ProviderTypeKey]).
Str("upstream-delivery-id", m.Metadata[events.ProviderDeliveryIdKey]).
// This is added for consistency with logs in
// Entity Execution Aggregator
Str("event", m.UUID).
Logger()
ctx = l.WithContext(ctx)

Expand Down Expand Up @@ -509,15 +520,20 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc {
if res != nil && res.eiw != nil {
if err := res.eiw.ToMessage(m); err != nil {
wes.Error = true
log.Printf("Error creating event: %v", err)
l.Error().Err(err).Msg("Error creating event")
w.WriteHeader(http.StatusInternalServerError)
return
}

// This ensures that loggers on downstream
// processors have all log attributes
// available.
m.SetContext(ctx)

// Publish the message to the event router
if err := s.evt.Publish(res.topic, m); err != nil {
wes.Error = true
log.Printf("Error publishing message: %v", err)
l.Error().Err(err).Msg("Error publishing message")
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -678,7 +694,7 @@ func (s *Server) processRelevantRepositoryEvent(
}

// This only makes sense for "meta" event type
if dbrepo.WebhookID.Valid {
if event.GetHookID() != 0 && dbrepo.WebhookID.Valid {
// Check if the payload webhook ID matches the one we
// have stored in the DB for this repository
if event.GetHookID() != dbrepo.WebhookID.Int64 {
Expand Down
44 changes: 44 additions & 0 deletions internal/controlplane/handlers_githubwebhooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1528,6 +1528,50 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
require.Equal(t, repositoryID.String(), received.Metadata["repository_id"])
},
},
{
name: "repository deleted had hook",
// https://docs.github.com/en/webhooks/webhook-events-and-payloads#repository
event: "repository",
// https://pkg.go.dev/github.com/google/go-github/[email protected]/github#RepositoryEvent
payload: &github.RepositoryEvent{
Action: github.String("deleted"),
Repo: &github.Repository{
ID: github.Int64(12345),
Name: github.String("minder"),
FullName: github.String("stacklok/minder"),
HTMLURL: github.String("https://github.com/stacklok/minder"),
},
},
mockStoreFunc: newMockStore(
withSuccessfulGetRepositoryByRepoID(
db.Repository{
ID: repositoryID,
ProjectID: projectID,
RepoID: 12345,
Provider: providerName,
ProviderID: providerID,
WebhookID: sql.NullInt64{
Int64: 12345,
Valid: true,
},
},
),
),
topic: events.TopicQueueReconcileEntityDelete,
statusCode: http.StatusOK,
//nolint:thelper
queued: func(t *testing.T, event string, ch <-chan *message.Message) {
timeout := 1 * time.Second
received := withTimeout(ch, timeout)
require.NotNilf(t, received, "no event received after waiting %s", timeout)
require.Equal(t, "12345", received.Metadata["id"])
require.Equal(t, event, received.Metadata["type"])
require.Equal(t, "https://api.github.com/", received.Metadata["source"])
require.Equal(t, providerID.String(), received.Metadata["provider_id"])
require.Equal(t, projectID.String(), received.Metadata[entities.ProjectIDEventKey])
require.Equal(t, repositoryID.String(), received.Metadata["repository_id"])
},
},
{
name: "repository edited",
// https://docs.github.com/en/webhooks/webhook-events-and-payloads#repository
Expand Down

0 comments on commit f866e6b

Please sign in to comment.