From 39deccf818982cca6a623c5968197cc63d45e11e Mon Sep 17 00:00:00 2001 From: Ragnar Paide <16119863+ragnarpa@users.noreply.github.com> Date: Tue, 21 May 2024 22:16:39 +0300 Subject: [PATCH] fix: Ignore 422 response while creating commit statuses Signed-off-by: Ragnar Paide <16119863+ragnarpa@users.noreply.github.com> --- docs/services/github.md | 4 ++ pkg/controller/controller.go | 14 ++++-- pkg/controller/controller_test.go | 73 +++++++++++++++++++++++++++++++ pkg/services/github.go | 16 ++++++- 4 files changed, 103 insertions(+), 4 deletions(-) diff --git a/docs/services/github.md b/docs/services/github.md index 4cd25239..136820d6 100644 --- a/docs/services/github.md +++ b/docs/services/github.md @@ -95,3 +95,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 1e1ae7ab..144196f8 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 e25af5f2..780874d6 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" @@ -62,6 +63,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}}" @@ -341,7 +351,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 } }