From 5db00ce77c6446d3e54e6262a6398980c91617f8 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 18 Jul 2024 16:28:18 +0200 Subject: [PATCH 1/4] Extend wait period for test; drive-by fix for possibly flaky test Signed-off-by: Danny Kopping --- coderd/notifications/metrics_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index 53fb8279789e2..072b88f9598a6 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -263,9 +263,10 @@ func TestPendingUpdatesMetric(t *testing.T) { interceptor.proceed.Broadcast() // Validate that the store synced the expected number of updates. - require.Eventually(t, func() bool { + require.Eventuallyf(t, func() bool { return syncer.sent.Load() == 1 && syncer.failed.Load() == 1 - }, testutil.WaitShort, testutil.IntervalFast) + }, testutil.WaitLong, testutil.IntervalFast, + "sent: %d, failed: %d", syncer.sent.Load(), syncer.failed.Load()) // Wait for the updates to be synced and the metric to reflect that. require.Eventually(t, func() bool { From 07b4a69519eabfd57458ecea36207569ff05a030 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 19 Jul 2024 11:03:16 +0200 Subject: [PATCH 2/4] chore: add webhook tests Signed-off-by: Danny Kopping --- coderd/notifications/dispatch/webhook.go | 11 +- coderd/notifications/dispatch/webhook_test.go | 144 ++++++++++++++++++ coderd/notifications/metrics_test.go | 5 +- 3 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 coderd/notifications/dispatch/webhook_test.go diff --git a/coderd/notifications/dispatch/webhook.go b/coderd/notifications/dispatch/webhook.go index c1fb47ea35692..4a548b40e4c2f 100644 --- a/coderd/notifications/dispatch/webhook.go +++ b/coderd/notifications/dispatch/webhook.go @@ -78,11 +78,16 @@ func (w *WebhookHandler) dispatch(msgPayload types.MessagePayload, title, body, return false, xerrors.Errorf("create HTTP request: %v", err) } req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Message-Id", msgID.String()) // Send request. resp, err := w.cl.Do(req) if err != nil { - return true, xerrors.Errorf("failed to send HTTP request: %v", err) + if errors.Is(err, context.DeadlineExceeded) { + return true, xerrors.Errorf("request timeout: %w", err) + } + + return true, xerrors.Errorf("request failed: %w", err) } defer resp.Body.Close() @@ -93,11 +98,11 @@ func (w *WebhookHandler) dispatch(msgPayload types.MessagePayload, title, body, lr := io.LimitReader(resp.Body, int64(len(respBody))) n, err := lr.Read(respBody) if err != nil && !errors.Is(err, io.EOF) { - return true, xerrors.Errorf("non-200 response (%d), read body: %w", resp.StatusCode, err) + return true, xerrors.Errorf("non-2xx response (%d), read body: %w", resp.StatusCode, err) } w.log.Warn(ctx, "unsuccessful delivery", slog.F("status_code", resp.StatusCode), slog.F("response", respBody[:n]), slog.F("msg_id", msgID)) - return true, xerrors.Errorf("non-200 response (%d)", resp.StatusCode) + return true, xerrors.Errorf("non-2xx response (%d)", resp.StatusCode) } return false, nil diff --git a/coderd/notifications/dispatch/webhook_test.go b/coderd/notifications/dispatch/webhook_test.go new file mode 100644 index 0000000000000..623850169806d --- /dev/null +++ b/coderd/notifications/dispatch/webhook_test.go @@ -0,0 +1,144 @@ +package dispatch_test + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/serpent" + + "github.com/coder/coder/v2/coderd/notifications/dispatch" + "github.com/coder/coder/v2/coderd/notifications/types" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +func TestWebhook(t *testing.T) { + t.Parallel() + + const ( + titleTemplate = "this is the title ({{.Labels.foo}})" + bodyTemplate = "this is the body ({{.Labels.baz}})" + ) + + msgPayload := types.MessagePayload{ + Version: "1.0", + NotificationName: "test", + Labels: map[string]string{ + "foo": "bar", + "baz": "quux", + }, + } + + tests := []struct { + name string + serverURL string + serverTimeout time.Duration + serverFn func(uuid.UUID, http.ResponseWriter, *http.Request) + + expectSuccess bool + expectRetryable bool + expectErr string + }{ + { + name: "successful", + serverFn: func(msgID uuid.UUID, w http.ResponseWriter, r *http.Request) { + var payload dispatch.WebhookPayload + err := json.NewDecoder(r.Body).Decode(&payload) + require.NoError(t, err) + require.Equal(t, "application/json", r.Header.Get("Content-Type")) + require.Equal(t, msgID, payload.MsgID) + require.Equal(t, msgID.String(), r.Header.Get("X-Message-Id")) + + w.WriteHeader(http.StatusOK) + _, err = w.Write([]byte(fmt.Sprintf("received %s", payload.MsgID))) + require.NoError(t, err) + }, + expectSuccess: true, + }, + { + name: "invalid endpoint", + // Build a deliberately invalid URL to fail validation. + serverURL: "invalid .com", + expectSuccess: false, + expectErr: "invalid URL escape", + expectRetryable: false, + }, + { + name: "timeout", + serverTimeout: time.Millisecond, + expectSuccess: false, + expectRetryable: true, + expectErr: "request timeout", + }, + { + name: "non-200 response", + serverFn: func(_ uuid.UUID, w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + expectSuccess: false, + expectRetryable: true, + expectErr: "non-2xx response (500)", + }, + } + + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + + // nolint:paralleltest // Irrelevant as of Go v1.22 + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + timeout := testutil.WaitLong + if tc.serverTimeout > 0 { + timeout = tc.serverTimeout + } + + var ( + err error + ctx = testutil.Context(t, timeout) + msgID = uuid.New() + ) + + var endpoint *url.URL + if tc.serverURL != "" { + endpoint = &url.URL{Host: tc.serverURL} + } else { + // Mock server to simulate webhook endpoint. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tc.serverFn(msgID, w, r) + })) + defer server.Close() + + endpoint, err = url.Parse(server.URL) + require.NoError(t, err) + } + + cfg := codersdk.NotificationsWebhookConfig{ + Endpoint: *serpent.URLOf(endpoint), + } + handler := dispatch.NewWebhookHandler(cfg, logger.With(slog.F("test", tc.name))) + deliveryFn, err := handler.Dispatcher(msgPayload, titleTemplate, bodyTemplate) + require.NoError(t, err) + + retryable, err := deliveryFn(ctx, msgID) + if tc.expectSuccess { + require.NoError(t, err) + require.False(t, retryable) + return + } + + require.ErrorContains(t, err, tc.expectErr) + require.Equal(t, tc.expectRetryable, retryable) + }) + } +} diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go index 072b88f9598a6..53fb8279789e2 100644 --- a/coderd/notifications/metrics_test.go +++ b/coderd/notifications/metrics_test.go @@ -263,10 +263,9 @@ func TestPendingUpdatesMetric(t *testing.T) { interceptor.proceed.Broadcast() // Validate that the store synced the expected number of updates. - require.Eventuallyf(t, func() bool { + require.Eventually(t, func() bool { return syncer.sent.Load() == 1 && syncer.failed.Load() == 1 - }, testutil.WaitLong, testutil.IntervalFast, - "sent: %d, failed: %d", syncer.sent.Load(), syncer.failed.Load()) + }, testutil.WaitShort, testutil.IntervalFast) // Wait for the updates to be synced and the metric to reflect that. require.Eventually(t, func() bool { From 6dbd9f40a87e59fdedd2ad1c5f7a9909ab05dd98 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 19 Jul 2024 11:27:27 +0200 Subject: [PATCH 3/4] assert Signed-off-by: Danny Kopping --- coderd/notifications/dispatch/webhook_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/coderd/notifications/dispatch/webhook_test.go b/coderd/notifications/dispatch/webhook_test.go index 623850169806d..1e2bd6adc1491 100644 --- a/coderd/notifications/dispatch/webhook_test.go +++ b/coderd/notifications/dispatch/webhook_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "cdr.dev/slog" @@ -54,14 +55,14 @@ func TestWebhook(t *testing.T) { serverFn: func(msgID uuid.UUID, w http.ResponseWriter, r *http.Request) { var payload dispatch.WebhookPayload err := json.NewDecoder(r.Body).Decode(&payload) - require.NoError(t, err) - require.Equal(t, "application/json", r.Header.Get("Content-Type")) - require.Equal(t, msgID, payload.MsgID) - require.Equal(t, msgID.String(), r.Header.Get("X-Message-Id")) + assert.NoError(t, err) + assert.Equal(t, "application/json", r.Header.Get("Content-Type")) + assert.Equal(t, msgID, payload.MsgID) + assert.Equal(t, msgID.String(), r.Header.Get("X-Message-Id")) w.WriteHeader(http.StatusOK) _, err = w.Write([]byte(fmt.Sprintf("received %s", payload.MsgID))) - require.NoError(t, err) + assert.NoError(t, err) }, expectSuccess: true, }, From 18009b1a6c36d2ba7fc0d6d986229628156717fe Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 19 Jul 2024 11:46:41 +0200 Subject: [PATCH 4/4] Provoke timeout immediately Signed-off-by: Danny Kopping --- coderd/notifications/dispatch/webhook_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/notifications/dispatch/webhook_test.go b/coderd/notifications/dispatch/webhook_test.go index 1e2bd6adc1491..546fbc2e88057 100644 --- a/coderd/notifications/dispatch/webhook_test.go +++ b/coderd/notifications/dispatch/webhook_test.go @@ -76,7 +76,7 @@ func TestWebhook(t *testing.T) { }, { name: "timeout", - serverTimeout: time.Millisecond, + serverTimeout: time.Nanosecond, expectSuccess: false, expectRetryable: true, expectErr: "request timeout",