fix: Ignore 422 response while creating commit statuses

Signed-off-by: Ragnar Paide <[email protected]>

Author: ragnarpa

docs/services/github.md

@@ -111,3 +111,6 @@ template.app-deployed: |
 - The `github.pullRequestComment.commentTag` parameter is used to identify the comment. If a comment with the specified tag is found, it will be updated (upserted). If no comment with the tag is found, a new comment will be created.
 - 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) allows a maximum of 1000 attempts using the same commit SHA and context. Once this limit is reached, the API returns validation errors (HTTP 422). The notification engine ignores these errors and marks the notification attempts as completed.

pkg/controller/controller.go

@@ -232,9 +232,15 @@ 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))
+
+						retry := services.HandleSendError(err, logEntry)
+
+						if retry {
+							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)

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.TooManyGitHubCommitStatusesError{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()

pkg/services/github.go

@@ -3,6 +3,7 @@ package services
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -82,6 +83,15 @@ type GitHubPullRequestComment struct {
 	CommentTag string `json:"commentTag,omitempty"`
 }
 
+type TooManyGitHubCommitStatusesError struct {
+	Sha     string
+	Context string
+}
+
+func (e *TooManyGitHubCommitStatusesError) 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}}"
@@ -519,7 +529,11 @@ func (g gitHubService) Send(notification Notification, _ Destination) error {
 				TargetURL:   &notification.GitHub.Status.TargetURL,
 			},
 		)
-		if err != nil {
+
+		var ghErr *github.ErrorResponse
+		if ok := errors.As(err, &ghErr); ok && ghErr.Response.StatusCode == 422 {
+			return &TooManyGitHubCommitStatusesError{Sha: notification.GitHub.revision, Context: notification.GitHub.Status.Label}
+		} else if err != nil {
 			return err
 		}
 	}

pkg/services/services.go

@@ -8,6 +8,7 @@ import (
 	texttemplate "text/template"
 	_ "time/tzdata"
 
+	"github.com/sirupsen/logrus"
 	"sigs.k8s.io/yaml"
 )
 
@@ -113,6 +114,17 @@ type NotificationService interface {
 	Send(notification Notification, dest Destination) error
 }
 
+// HandleSendError inspects the error and signals the caller if the Send operation should be retried.
+func HandleSendError(err error, logEntry *logrus.Entry) (retry bool) {
+	switch e := err.(type) {
+	case *TooManyGitHubCommitStatusesError:
+		logEntry.Warn(e)
+		return false
+	default:
+		return true
+	}
+}
+
 func NewService(serviceType string, optsData []byte) (NotificationService, error) {
 	switch serviceType {
 	case "awssqs":