From ad0ac693de05c513be5f6088c0fd930d2ecc3c68 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 29 Apr 2022 14:23:25 +0200 Subject: [PATCH] switch to go-pkgz/notify package: webhook --- backend/app/cmd/server.go | 19 ++--- backend/app/notify/webhook.go | 93 ++++++++--------------- backend/app/notify/webhook_test.go | 117 ++++++----------------------- 3 files changed, 66 insertions(+), 163 deletions(-) diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 9ed9e220..e9121729 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -16,6 +16,7 @@ import ( "github.com/go-pkgz/jrpc" "github.com/go-pkgz/lcw/eventbus" log "github.com/go-pkgz/lgr" + ntf "github.com/go-pkgz/notify" "github.com/golang-jwt/jwt" "github.com/kyokomi/emoji/v2" bolt "go.etcd.io/bbolt" @@ -233,10 +234,10 @@ type NotifyGroup struct { Channel string `long:"chan" env:"CHAN" description:"slack channel for admin notifications"` } `group:"slack" namespace:"slack" env-namespace:"SLACK"` Webhook struct { - WebhookURL string `long:"url" env:"URL" description:"webhook URL for admin notifications"` - Template string `long:"template" env:"TEMPLATE" description:"webhook authentication template" default:"{\"text\": \"{{.Text}}\"}"` - Headers []string `long:"headers" description:"webhook authentication headers in format --notify.webhook.headers=Header1:Value1,Value2,..."` // env NOTIFY_WEBHOOK_HEADERS split in code bellow to allow , inside "" - Timeout time.Duration `long:"timeout" env:"TIMEOUT" description:"webhook timeout" default:"5s"` + URL string `long:"url" env:"URL" description:"webhook URL for admin notifications"` + Template string `long:"template" env:"TEMPLATE" description:"webhook authentication template" default:"{\"text\": \"{{.Text}}\"}"` + Headers []string `long:"headers" description:"webhook authentication headers in format --notify.webhook.headers=Header1:Value1,Value2,..."` // env NOTIFY_WEBHOOK_HEADERS split in code bellow to allow , inside "" + Timeout time.Duration `long:"timeout" env:"TIMEOUT" description:"webhook timeout" default:"5s"` } `group:"webhook" namespace:"webhook" env-namespace:"WEBHOOK"` } @@ -975,18 +976,18 @@ func (s *ServerCommand) makeNotifyDestinations(authenticator *auth.Service) ([]n destinations := make([]notify.Destination, 0) if contains("webhook", s.Notify.Admins) { - client := &http.Client{Timeout: 5 * time.Second} webhookHeaders := s.Notify.Webhook.Headers if len(webhookHeaders) == 0 { webhookHeaders = splitAtCommas(os.Getenv("NOTIFY_WEBHOOK_HEADERS")) // env value may have comma inside "", parsed separately } whParams := notify.WebhookParams{ - WebhookURL: s.Notify.Webhook.WebhookURL, - Template: s.Notify.Webhook.Template, - Headers: webhookHeaders, + URL: s.Notify.Webhook.URL, + Template: s.Notify.Webhook.Template, + Headers: webhookHeaders, + Timeout: time.Second * 5, } - webhook, err := notify.NewWebhook(client, whParams) + webhook, err := notify.NewWebhook(whParams) if err != nil { return destinations, fmt.Errorf("failed to create webhook notification destination: %w", err) } diff --git a/backend/app/notify/webhook.go b/backend/app/notify/webhook.go index 722968a7..0e7c2980 100644 --- a/backend/app/notify/webhook.go +++ b/backend/app/notify/webhook.go @@ -4,110 +4,81 @@ import ( "bytes" "context" "fmt" - "io" - "net/http" - "strings" "text/template" + "time" log "github.com/go-pkgz/lgr" - "github.com/pkg/errors" + ntf "github.com/go-pkgz/notify" ) const ( webhookDefaultTemplate = `{"text": "{{.Text}}"}` ) -// WebhookClient defines an interface of client for webhook -type WebhookClient interface { - Do(*http.Request) (*http.Response, error) -} - // WebhookParams contain settings for webhook notifications type WebhookParams struct { - WebhookURL string - Template string - Headers []string + URL string + Template string + Headers []string + Timeout time.Duration } // Webhook implements notify.Destination for Webhook notifications type Webhook struct { - WebhookParams - webhookClient WebhookClient - webhookTemplate *template.Template + *ntf.Webhook + + url string + template *template.Template } // NewWebhook makes Webhook -func NewWebhook(client WebhookClient, params WebhookParams) (*Webhook, error) { - res := &Webhook{WebhookParams: params} - if res.WebhookURL == "" { +func NewWebhook(params WebhookParams) (*Webhook, error) { + res := &Webhook{ + Webhook: ntf.NewWebhook(ntf.WebhookParams{ + Timeout: params.Timeout, + Headers: params.Headers, + }), + url: params.URL, + } + + if res.url == "" { return nil, fmt.Errorf("webhook URL is required for webhook notifications") } - if res.Template == "" { - res.Template = webhookDefaultTemplate + if params.Template == "" { + params.Template = webhookDefaultTemplate } - payloadTmpl, err := template.New("webhook").Parse(res.Template) + payloadTmpl, err := template.New("webhook").Parse(params.Template) if err != nil { return nil, fmt.Errorf("unable to parse webhook template: %w", err) } - res.webhookClient = client - res.webhookTemplate = payloadTmpl + res.template = payloadTmpl - log.Printf("[DEBUG] create new webhook notifier for %s", res.WebhookURL) + log.Printf("[DEBUG] create new webhook notifier for %s", res.url) return res, nil } // Send sends Webhook notification -func (t *Webhook) Send(ctx context.Context, req Request) error { +func (w *Webhook) Send(ctx context.Context, req Request) error { + log.Printf("[DEBUG] send webhook notification, comment id %s", req.Comment.ID) var payload bytes.Buffer - err := t.webhookTemplate.Execute(&payload, req.Comment) + err := w.template.Execute(&payload, req.Comment) if err != nil { return fmt.Errorf("unable to compile webhook template: %w", err) } - httpReq, err := http.NewRequestWithContext(ctx, "POST", t.WebhookURL, &payload) - if err != nil { - return fmt.Errorf("unable to create webhook request: %w", err) - } - - for _, h := range t.Headers { - elems := strings.Split(h, ":") - if len(elems) != 2 { - continue - } - httpReq.Header.Set(strings.TrimSpace(elems[0]), strings.TrimSpace(elems[1])) - } - - resp, err := t.webhookClient.Do(httpReq) - if err != nil { - return fmt.Errorf("webhook request failed: %w", err) - } - - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - errMsg := fmt.Sprintf("webhook request failed with non-OK status code: %d", resp.StatusCode) - respBody, e := io.ReadAll(resp.Body) - if e != nil { - return errors.New(errMsg) - } - return fmt.Errorf("%s, body: %s", errMsg, respBody) - } - - log.Printf("[DEBUG] send webhook notification, comment id %s", req.Comment.ID) - - return nil + return w.Webhook.Send(ctx, w.url, payload.String()) } // SendVerification is not implemented for Webhook -func (t *Webhook) SendVerification(_ context.Context, _ VerificationRequest) error { +func (w *Webhook) SendVerification(_ context.Context, _ VerificationRequest) error { return nil } // String describes the webhook instance -func (t *Webhook) String() string { - return fmt.Sprintf("webhook notification to %s", t.WebhookURL) +func (w *Webhook) String() string { + return fmt.Sprintf("%s to %s", w.Webhook.String(), w.url) } diff --git a/backend/app/notify/webhook_test.go b/backend/app/notify/webhook_test.go index abcd2bed..1faab30c 100644 --- a/backend/app/notify/webhook_test.go +++ b/backend/app/notify/webhook_test.go @@ -1,12 +1,9 @@ package notify import ( - "bytes" "context" - "fmt" - "io" - "net/http" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,70 +11,33 @@ import ( "github.com/umputun/remark42/backend/app/store" ) -type funcWebhookClient func(*http.Request) (*http.Response, error) - -func (c funcWebhookClient) Do(r *http.Request) (*http.Response, error) { - return c(r) -} - -var okWebhookClient = funcWebhookClient(func(*http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(bytes.NewBufferString("ok")), - }, nil -}) - -type errReader struct { -} - -func (errReader) Read(p []byte) (n int, err error) { - return 0, fmt.Errorf("test error") -} - func TestWebhook_NewWebhook(t *testing.T) { - wh, err := NewWebhook(okWebhookClient, WebhookParams{ - WebhookURL: "https://example.org/webhook", - Headers: []string{"Authorization:Basic AXVubzpwQDU1dzByYM=="}, + wh, err := NewWebhook(WebhookParams{ + URL: "https://example.org/webhook", + Headers: []string{"Authorization:Basic AXVubzpwQDU1dzByYM=="}, }) assert.NoError(t, err) assert.NotNil(t, wh) - assert.Equal(t, "https://example.org/webhook", wh.WebhookURL) + assert.Equal(t, "https://example.org/webhook", wh.url) assert.Equal(t, []string{"Authorization:Basic AXVubzpwQDU1dzByYM=="}, wh.Headers) - assert.Equal(t, `{"text": "{{.Text}}"}`, wh.Template) + assert.NotNil(t, wh.template) - wh, err = NewWebhook(okWebhookClient, WebhookParams{ - WebhookURL: "https://example.org/webhook", - Headers: []string{"Authorization:Basic AXVubzpwQDU1dzByYM=="}, - Template: "{{.Text}}", - }) - assert.NoError(t, err) - assert.NotNil(t, wh) - assert.Equal(t, "{{.Text}}", wh.Template) - - wh, err = NewWebhook(okWebhookClient, WebhookParams{}) + wh, err = NewWebhook(WebhookParams{}) assert.Nil(t, wh) assert.Error(t, err) assert.Equal(t, "webhook URL is required for webhook notifications", err.Error()) - wh, err = NewWebhook(okWebhookClient, WebhookParams{WebhookURL: "https://example.org/webhook", Template: "{{.Text"}) + wh, err = NewWebhook(WebhookParams{URL: "https://example.org/webhook", Template: "{{.Text"}) assert.Nil(t, wh) assert.Error(t, err) assert.Contains(t, err.Error(), "unable to parse webhook template") } func TestWebhook_Send(t *testing.T) { - wh, err := NewWebhook(funcWebhookClient(func(r *http.Request) (*http.Response, error) { - assert.Len(t, r.Header, 1) - assert.Equal(t, r.Header.Get("Content-Type"), "application/json,text/plain") - - return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(bytes.NewBufferString("")), - }, nil - }), WebhookParams{ - WebhookURL: "https://example.org/webhook", - Headers: []string{"Content-Type:application/json,text/plain", ""}, + wh, err := NewWebhook(WebhookParams{ + URL: "bad-url", + Headers: []string{"Content-Type:application/json,text/plain", ""}, }) assert.NoError(t, err) assert.NotNil(t, wh) @@ -85,74 +45,45 @@ func TestWebhook_Send(t *testing.T) { c := store.Comment{Text: "some text", ParentID: "1", ID: "999"} c.User.Name = "from" - err = wh.Send(context.TODO(), Request{Comment: c}) - assert.NoError(t, err) + err = wh.Send(context.Background(), Request{Comment: c}) + assert.Error(t, err) - wh, err = NewWebhook(okWebhookClient, WebhookParams{ - WebhookURL: "https://example.org/webhook", - Template: "{{.InvalidProperty}}", + wh, err = NewWebhook(WebhookParams{ + URL: "https://example.org/webhook", + Template: "{{.InvalidProperty}}", }) assert.NoError(t, err) - err = wh.Send(context.TODO(), Request{Comment: c}) + err = wh.Send(context.Background(), Request{Comment: c}) require.Error(t, err) assert.Contains(t, err.Error(), "webhook template") - wh, err = NewWebhook(okWebhookClient, WebhookParams{WebhookURL: "https://example.org/webhook"}) + wh, err = NewWebhook(WebhookParams{URL: "https://example.org/webhook"}) assert.NoError(t, err) err = wh.Send(nil, Request{Comment: c}) // nolint require.Error(t, err) assert.Contains(t, err.Error(), "unable to create webhook request") - wh, err = NewWebhook(funcWebhookClient(func(*http.Request) (*http.Response, error) { - return nil, fmt.Errorf("request failed") - }), WebhookParams{WebhookURL: "https://not-existing-url.net"}) + wh, err = NewWebhook(WebhookParams{URL: "https://not-existing-url.net"}) assert.NoError(t, err) - err = wh.Send(context.TODO(), Request{Comment: c}) + err = wh.Send(context.Background(), Request{Comment: c}) require.Error(t, err) assert.Contains(t, err.Error(), "webhook request failed") - - wh, err = NewWebhook(funcWebhookClient(func(*http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusNotFound, - Body: io.NopCloser(bytes.NewBufferString("not found")), - }, nil - }), WebhookParams{ - WebhookURL: "http:/example.org/invalid-url", - }) - assert.NoError(t, err) - err = wh.Send(context.TODO(), Request{Comment: c}) - require.Error(t, err) - assert.Contains(t, err.Error(), "non-OK status code: 404, body: not found") - - wh, err = NewWebhook(funcWebhookClient(func(*http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusNotFound, - Body: io.NopCloser(errReader{}), - }, nil - }), WebhookParams{ - WebhookURL: "http:/example.org/invalid-url", - }) - assert.NoError(t, err) - err = wh.Send(context.TODO(), Request{Comment: c}) - require.Error(t, err) - assert.Contains(t, err.Error(), "non-OK status code: 404") - assert.NotContains(t, err.Error(), "body") } func TestWebhook_SendVerification(t *testing.T) { - wh, err := NewWebhook(okWebhookClient, WebhookParams{WebhookURL: "https://example.org/webhook"}) + wh, err := NewWebhook(WebhookParams{URL: "https://example.org/webhook"}) assert.NoError(t, err) assert.NotNil(t, wh) - err = wh.SendVerification(context.TODO(), VerificationRequest{}) + err = wh.SendVerification(context.Background(), VerificationRequest{}) assert.NoError(t, err) } func TestWebhook_String(t *testing.T) { - wh, err := NewWebhook(okWebhookClient, WebhookParams{WebhookURL: "https://example.org/webhook"}) + wh, err := NewWebhook(WebhookParams{URL: "https://example.org/webhook", Timeout: time.Minute * 5}) assert.NoError(t, err) assert.NotNil(t, wh) str := wh.String() - assert.Equal(t, "webhook notification to https://example.org/webhook", str) + assert.Equal(t, "webhook notification with timeout 5m0s to https://example.org/webhook", str) }