fix: issue-3044 add retries to webhook (#236)

* fix: issue-3044 add retries to webhook Signed-off-by: Prema devi Kuppuswamy <[email protected]> * fix lint Signed-off-by: Prema devi Kuppuswamy <[email protected]> * fix lint Signed-off-by: Prema devi Kuppuswamy <[email protected]> * fix fmt Signed-off-by: Prema devi Kuppuswamy <[email protected]> * update go.mod Signed-off-by: Prema devi Kuppuswamy <[email protected]> * update docs Signed-off-by: Prema devi Kuppuswamy <[email protected]> --------- Signed-off-by: Prema devi Kuppuswamy <[email protected]>
argoproj · Oct 27, 2023 · a8d185e · a8d185e
1 parent 5aca046
commit a8d185e
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 15 deletions.
diff --git a/docs/services/webhook.md b/docs/services/webhook.md
@@ -11,6 +11,15 @@ The Webhook notification service configuration includes following settings:
 - `headers` - optional, the headers to pass along with the webhook
 - `basicAuth` - optional, the basic authentication to pass along with the webhook
 - `insecureSkipVerify` - optional bool, true or false
+- `retryWaitMin` - Optional, the minimum wait time between retries. Default value: 1s.
+- `retryWaitMax` - Optional, the maximum wait time between retries. Default value: 5s.
+- `retryMax` - Optional, the maximum number of retries. Default value: 3.
+
+## Retry Behavior
+
+The webhook service will automatically retry the request if it fails due to network errors or if the server returns a 5xx status code. The number of retries and the wait time between retries can be configured using the `retryMax`, `retryWaitMin`, and `retryWaitMax` parameters.
+
+The wait time between retries is between `retryWaitMin` and `retryWaitMax`. If all retries fail, the `Send` method will return an error.
 
 ## Configuration
 

diff --git a/go.mod b/go.mod
@@ -113,7 +113,7 @@ require (
 	github.com/aws/aws-sdk-go-v2 v1.17.3
 	github.com/aws/aws-sdk-go-v2/config v1.18.8
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
-	github.com/hashicorp/go-retryablehttp v0.5.3 // indirect
+	github.com/hashicorp/go-retryablehttp v0.5.3
 )
 
 replace github.com/prometheus/client_golang => github.com/prometheus/client_golang v1.14.0
diff --git a/pkg/services/webhook.go b/pkg/services/webhook.go
@@ -7,6 +7,9 @@ import (
 	"net/http"
 	"strings"
 	texttemplate "text/template"
+	"time"
+
+	"github.com/hashicorp/go-retryablehttp"
 
 	log "github.com/sirupsen/logrus"
 
@@ -77,13 +80,26 @@ type BasicAuth struct {
 }
 
 type WebhookOptions struct {
-	URL                string     `json:"url"`
-	Headers            []Header   `json:"headers"`
-	BasicAuth          *BasicAuth `json:"basicAuth"`
-	InsecureSkipVerify bool       `json:"insecureSkipVerify"`
+	URL                string        `json:"url"`
+	Headers            []Header      `json:"headers"`
+	BasicAuth          *BasicAuth    `json:"basicAuth"`
+	InsecureSkipVerify bool          `json:"insecureSkipVerify"`
+	RetryWaitMin       time.Duration `json:"retryWaitMin"`
+	RetryWaitMax       time.Duration `json:"retryWaitMax"`
+	RetryMax           int           `json:"retryMax"`
 }
 
 func NewWebhookService(opts WebhookOptions) NotificationService {
+	// Set default values if fields are zero
+	if opts.RetryWaitMin == 0 {
+		opts.RetryWaitMin = 1 * time.Second
+	}
+	if opts.RetryWaitMax == 0 {
+		opts.RetryWaitMax = 5 * time.Second
+	}
+	if opts.RetryMax == 0 {
+		opts.RetryMax = 3
+	}
 	return &webhookService{opts: opts}
 }
 
@@ -133,31 +149,37 @@ func (r *request) applyOverridesFrom(notification WebhookNotification) {
 	}
 }
 
-func (r *request) intoHttpRequest(service *webhookService) (*http.Request, error) {
-	req, err := http.NewRequest(r.method, r.url, bytes.NewBufferString(r.body))
+func (r *request) intoRetryableHttpRequest(service *webhookService) (*retryablehttp.Request, error) {
+	retryReq, err := retryablehttp.NewRequest(r.method, r.url, bytes.NewBufferString(r.body))
 	if err != nil {
 		return nil, err
 	}
 	for _, header := range service.opts.Headers {
-		req.Header.Set(header.Name, header.Value)
+		retryReq.Header.Set(header.Name, header.Value)
 	}
 	if service.opts.BasicAuth != nil {
-		req.SetBasicAuth(service.opts.BasicAuth.Username, service.opts.BasicAuth.Password)
+		retryReq.SetBasicAuth(service.opts.BasicAuth.Username, service.opts.BasicAuth.Password)
 	}
-	return req, nil
+	return retryReq, nil
 }
 
 func (r *request) execute(service *webhookService) (*http.Response, error) {
-	req, err := r.intoHttpRequest(service)
+	req, err := r.intoRetryableHttpRequest(service)
 	if err != nil {
 		return nil, err
 	}
 
-	client := http.Client{
-		Transport: httputil.NewLoggingRoundTripper(
-			httputil.NewTransport(r.url, service.opts.InsecureSkipVerify),
-			log.WithField("service", r.destService)),
+	transport := httputil.NewLoggingRoundTripper(
+		httputil.NewTransport(r.url, service.opts.InsecureSkipVerify),
+		log.WithField("service", r.destService))
+
+	client := retryablehttp.NewClient()
+	client.HTTPClient = &http.Client{
+		Transport: transport,
 	}
+	client.RetryWaitMin = service.opts.RetryWaitMin
+	client.RetryWaitMax = service.opts.RetryWaitMax
+	client.RetryMax = service.opts.RetryMax
 
 	return client.Do(req)
 }
diff --git a/pkg/services/webhook_test.go b/pkg/services/webhook_test.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"text/template"
 
@@ -123,3 +124,40 @@ func TestGetTemplater_Webhook(t *testing.T) {
 	assert.Equal(t, notification.Webhook["github"].Body, "hello")
 	assert.Equal(t, notification.Webhook["github"].Path, "world")
 }
+
+func TestWebhookService_Send_Retry(t *testing.T) {
+	// Set up a mock server to receive requests
+	count := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		count++
+		if count < 5 {
+			w.WriteHeader(http.StatusInternalServerError)
+		} else {
+			w.WriteHeader(http.StatusOK)
+		}
+	}))
+	defer server.Close()
+
+	service := NewWebhookService(WebhookOptions{
+		BasicAuth:          &BasicAuth{Username: "testUsername", Password: "testPassword"},
+		URL:                server.URL,
+		Headers:            []Header{{Name: "testHeader", Value: "testHeaderValue"}},
+		InsecureSkipVerify: true,
+	})
+	err := service.Send(
+		Notification{
+			Webhook: map[string]WebhookNotification{
+				"test": {Body: "hello world", Method: http.MethodPost},
+			},
+		}, Destination{Recipient: "test", Service: "test"})
+
+	// Check if the error is due to a server error after retries
+	if !strings.Contains(err.Error(), "giving up after 4 attempts") {
+		t.Errorf("Expected giving up after 4 attempts, got %v", err)
+	}
+
+	// Check that the mock server received 4 requests
+	if count != 4 {
+		t.Errorf("Expected 4 requests, got %d", count)
+	}
+}