diff --git a/docs/services/github.md b/docs/services/github.md index 1dc72d83..fedd8774 100644 --- a/docs/services/github.md +++ b/docs/services/github.md @@ -108,3 +108,7 @@ template.app-deployed: | For more information see the [GitHub Deployment API Docs](https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#create-a-deployment). - If `github.pullRequestComment.content` is set to 65536 characters or more, it will be truncated. - Reference is optional. When set, it will be used as the ref to deploy. If not set, the revision will be used as the ref to deploy. + +## Commit Statuses + +The [method for generating commit statuses](https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status) only allows a maximum of 1000 attempts using the same commit SHA and context. Once this limit is hit, the API begins to produce validation errors (HTTP 422). The notification engine disregards these errors and labels these notification attempts as completed. \ No newline at end of file diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index f5f6db31..fa1d49d8 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -3,6 +3,7 @@ package controller import ( "context" "encoding/json" + "errors" "fmt" "reflect" "runtime/debug" @@ -232,9 +233,16 @@ func (c *notificationController) processResourceWithAPI(api api.API, resource v1 if err := api.Send(un.Object, cr.Templates, to); err != nil { logEntry.Errorf("Failed to notify recipient %s defined in resource %s/%s: %v using the configuration in namespace %s", to, resource.GetNamespace(), resource.GetName(), err, apiNamespace) - notificationsState.SetAlreadyNotified(c.isSelfServiceConfigureApi(api), apiNamespace, trigger, cr, to, false) - c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, false) - eventSequence.addError(fmt.Errorf("failed to deliver notification %s to %s: %v using the configuration in namespace %s", trigger, to, err, apiNamespace)) + + var ghErr *services.TooManyCommitStatusesError + if ok := errors.As(err, &ghErr); ok { + logEntry.Warnf("We seem to have created too many commit statuses for sha %s and context %s. Abandoning the notification.", ghErr.Sha, ghErr.Context) + } else { + notificationsState.SetAlreadyNotified(c.isSelfServiceConfigureApi(api), apiNamespace, trigger, cr, to, false) + c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, false) + eventSequence.addError(fmt.Errorf("failed to deliver notification %s to %s: %v using the configuration in namespace %s", trigger, to, err, apiNamespace)) + } + } else { logEntry.Debugf("Notification %s was sent using the configuration in namespace %s", to.Recipient, apiNamespace) c.metricsRegistry.IncDeliveriesCounter(trigger, to.Service, true) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index a75af455..0a56ee8b 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -188,6 +188,79 @@ func TestDoesNotSendNotificationIfAnnotationPresent(t *testing.T) { assert.NoError(t, err) } +func TestDoesNotSendNotificationIfTooManyCommitStatusesReceived(t *testing.T) { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + setAnnotations := func(notifiedAnnoationKeyValue string) func(obj *unstructured.Unstructured) { + return withAnnotations(map[string]string{ + subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", + notifiedAnnotationKey: notifiedAnnoationKeyValue, + }) + } + + state := NotificationsState{} + _ = state.SetAlreadyNotified(false, "", "my-trigger", triggers.ConditionResult{}, services.Destination{Service: "mock", Recipient: "recipient"}, false) + app := newResource("test", setAnnotations(mustToJson(state))) + ctrl, api, err := newController(t, ctx, newFakeClient(app)) + assert.NoError(t, err) + + api.EXPECT().GetConfig().Return(notificationApi.Config{}).AnyTimes() + api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: true, Templates: []string{"test"}}}, nil).Times(2) + api.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(&services.TooManyCommitStatusesError{Sha: "sha", Context: "context"}).Times(1) + + // First attempt should hit the TooManyCommitStatusesError. + // Returned annotations1 should contain the information about processed notification + // as a result of hitting the ToomanyCommitStatusesError error. + annotations1, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{}) + + assert.NoError(t, err) + + // Persist the notification state in the annotations. + setAnnotations(annotations1[notifiedAnnotationKey])(app) + + // The second attempt should see that the notification has already been processed + // and the value of the notification annotation should not change. In the second attempt api.Send is not called. + annotations2, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{}) + + assert.NoError(t, err) + assert.Equal(t, annotations1[notifiedAnnotationKey], annotations2[notifiedAnnotationKey]) +} + +func TestRetriesNotificationIfSendThrows(t *testing.T) { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + setAnnotations := func(notifiedAnnoationKeyValue string) func(obj *unstructured.Unstructured) { + return withAnnotations(map[string]string{ + subscriptions.SubscribeAnnotationKey("my-trigger", "mock"): "recipient", + notifiedAnnotationKey: notifiedAnnoationKeyValue, + }) + } + + state := NotificationsState{} + _ = state.SetAlreadyNotified(false, "", "my-trigger", triggers.ConditionResult{}, services.Destination{Service: "mock", Recipient: "recipient"}, false) + app := newResource("test", setAnnotations(mustToJson(state))) + ctrl, api, err := newController(t, ctx, newFakeClient(app)) + assert.NoError(t, err) + + api.EXPECT().GetConfig().Return(notificationApi.Config{}).AnyTimes() + api.EXPECT().RunTrigger("my-trigger", gomock.Any()).Return([]triggers.ConditionResult{{Triggered: true, Templates: []string{"test"}}}, nil).Times(2) + api.EXPECT().Send(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("boom")).Times(2) + + // First attempt. The returned annotations should not contain the notification state due to the error. + annotations, err := ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{}) + + assert.NoError(t, err) + assert.Empty(t, annotations[notifiedAnnotationKey]) + + // Second attempt. The returned annotations should not contain the notification state due to the error. + annotations, err = ctrl.processResourceWithAPI(api, app, logEntry, &NotificationEventSequence{}) + + assert.NoError(t, err) + assert.Empty(t, annotations[notifiedAnnotationKey]) +} + func TestRemovesAnnotationIfNoTrigger(t *testing.T) { ctx, cancel := context.WithCancel(context.TODO()) defer cancel() diff --git a/pkg/services/github.go b/pkg/services/github.go index b004ed15..a942878e 100644 --- a/pkg/services/github.go +++ b/pkg/services/github.go @@ -3,6 +3,7 @@ package services import ( "bytes" "context" + "errors" "fmt" "net/http" "regexp" @@ -81,6 +82,15 @@ type GitHubPullRequestComment struct { Content string `json:"content,omitempty"` } +type TooManyCommitStatusesError struct { + Sha string + Context string +} + +func (e *TooManyCommitStatusesError) Error() string { + return fmt.Sprintf("too many commit statuses for sha %s and context %s", e.Sha, e.Context) +} + const ( repoURLtemplate = "{{.app.spec.source.repoURL}}" revisionTemplate = "{{.app.status.operationState.syncResult.revision}}" @@ -460,7 +470,11 @@ func (g gitHubService) Send(notification Notification, _ Destination) error { TargetURL: ¬ification.GitHub.Status.TargetURL, }, ) - if err != nil { + + var ghErr *github.ErrorResponse + if ok := errors.As(err, &ghErr); ok && ghErr.Response.StatusCode == 422 { + return &TooManyCommitStatusesError{Sha: notification.GitHub.revision, Context: notification.GitHub.Status.Label} + } else if err != nil { return err } }