Store a target's number of delivery attempts, at least one (closes #540)
check / check (push) Waiting to run
check / check (push) Waiting to run
Target.MaxRetries (max_retries) becomes Target.MaxAttempts (max_attempts), and the form field, parser and ceiling are renamed to match. 1 is a single attempt with no circuit breaker; above 1 retries behind the breaker up to that many attempts. Both target forms refuse 0 and less and default a blank field to 1. The target page shows the plain number, and the README's two paragraphs on 0 against 1 become one sentence. Model: opus-5-5
This commit is contained in:
@@ -1568,29 +1568,27 @@ did not arrive on the URL and counts in neither.
|
||||
A delivery destination for events. Each target defines where and how events
|
||||
should be forwarded.
|
||||
|
||||
| Field | Type | Description |
|
||||
| ------------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `id` | UUID | Primary key |
|
||||
| `webhook_id` | UUID | Foreign key → Webhook |
|
||||
| `name` | string | Human-readable name |
|
||||
| `type` | TargetType | One of: `http`, `slack`, `database`, `log` |
|
||||
| `active` | boolean | Whether deliveries are enabled (default: true) |
|
||||
| `config` | JSON text | Type-specific configuration |
|
||||
| `max_retries` | integer | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets. The web UI labels it Delivery attempts |
|
||||
| Field | Type | Description |
|
||||
| -------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `id` | UUID | Primary key |
|
||||
| `webhook_id` | UUID | Foreign key → Webhook |
|
||||
| `name` | string | Human-readable name |
|
||||
| `type` | TargetType | One of: `http`, `slack`, `database`, `log` |
|
||||
| `active` | boolean | Whether deliveries are enabled (default: true) |
|
||||
| `config` | JSON text | Type-specific configuration |
|
||||
| `max_attempts` | integer | How many times an `http` or `slack` target attempts each delivery in all: at least 1, and 1 by default. Ignored by `database` and `log` targets. The web UI labels it Delivery attempts |
|
||||
|
||||
**Relations:** Belongs to Webhook. Has many Deliveries.
|
||||
|
||||
**Target types:**
|
||||
|
||||
- **`http`** — Forward the event as an HTTP POST to a configured URL.
|
||||
`max_retries` is the total number of delivery attempts, not retries on top of
|
||||
the first: when `max_retries` is 0 (the default), the target operates in
|
||||
fire-and-forget mode, a single attempt with no retries and no circuit breaker;
|
||||
a value of N makes up to N attempts in all, retrying failed deliveries with
|
||||
exponential backoff and protecting them with a per-target circuit breaker.
|
||||
- **`http`** — Forward the event as an HTTP POST to a configured URL. With
|
||||
`max_attempts` at 1 the target makes a single attempt; above 1 it retries a
|
||||
failed delivery with exponential backoff, up to that many attempts in all,
|
||||
behind a per-target circuit breaker.
|
||||
- **`slack`** — Post the event as a formatted message to a Slack-compatible
|
||||
incoming webhook URL (`webhookUrl` in `config`). It is built on the same HTTP
|
||||
core as `http` and honours `max_retries` identically, circuit breaker
|
||||
core as `http` and honours `max_attempts` identically, circuit breaker
|
||||
included. See the Slack target section under "Per-Webhook Event Databases" for
|
||||
the message format.
|
||||
- **`database`** — Archive the full event as a row into the target's own archive
|
||||
@@ -2165,11 +2163,11 @@ External Service
|
||||
▼ ▼ ▼
|
||||
┌────────────┐ ┌────────────┐ ┌────────────┐
|
||||
│ HTTP Target│ │ HTTP Target│ │ Log Target │
|
||||
│(max_retries│ │(max_retries│ │ (stdout) │
|
||||
│ == 0) │ │ > 0, │ └────────────┘
|
||||
│ fire+forget│ │ backoff + │
|
||||
│max_attempts│ │max_attempts│ │ (stdout) │
|
||||
│ == 1, │ │ > 1, │ └────────────┘
|
||||
│ one attempt│ │ backoff + │
|
||||
└────────────┘ │ circuit │
|
||||
│ breaker) │
|
||||
│ breaker │
|
||||
└────────────┘
|
||||
```
|
||||
|
||||
@@ -2246,7 +2244,7 @@ re-inject it to every currently active target.
|
||||
|
||||
### Circuit Breaker (HTTP and Slack Targets with Retries)
|
||||
|
||||
`http` and `slack` targets with `max_retries` > 0 are protected by a
|
||||
`http` and `slack` targets with `max_attempts` > 1 are protected by a
|
||||
**per-target circuit breaker** that prevents hammering a down target with
|
||||
repeated failed delivery attempts. The circuit breaker is in-memory only and
|
||||
resets on restart (which is fine — startup recovery rescans the database
|
||||
@@ -2287,10 +2285,10 @@ anyway).
|
||||
- **Cooldown:** 30 seconds in open state before probing
|
||||
|
||||
**Scope:** Circuit breakers apply to **`http` and `slack` targets with
|
||||
`max_retries` > 0**. The Slack target is built on the same HTTP core and hands
|
||||
its own `max_retries` to the same retry path, so it gets a breaker with the same
|
||||
5-failure / 30-second defaults. Fire-and-forget targets of either type
|
||||
(`max_retries` == 0), database targets (local operations), and log targets
|
||||
`max_attempts` > 1**. The Slack target is built on the same HTTP core and hands
|
||||
its own `max_attempts` to the same retry path, so it gets a breaker with the
|
||||
same 5-failure / 30-second defaults. Single-attempt targets of either type
|
||||
(`max_attempts` == 1), database targets (local operations), and log targets
|
||||
(stdout) do not use circuit breakers.
|
||||
|
||||
When a circuit is open and a new delivery arrives, the engine marks the delivery
|
||||
|
||||
@@ -29,9 +29,11 @@ type Target struct {
|
||||
// handler that marshals the model itself.
|
||||
Config string `gorm:"type:text" json:"-"` // JSON configuration
|
||||
|
||||
// For HTTP targets (max_retries=0 means fire-and-forget,
|
||||
// >0 enables retries with backoff)
|
||||
MaxRetries int `json:"maxRetries,omitempty"`
|
||||
// MaxAttempts is how many times an HTTP or Slack target attempts
|
||||
// each delivery in all, at least 1. 1 is a single attempt with no
|
||||
// circuit breaker; above 1, a failed attempt is retried with
|
||||
// backoff behind the breaker.
|
||||
MaxAttempts int `json:"maxAttempts,omitempty"`
|
||||
|
||||
// Relations. No model marshals the record it belongs to:
|
||||
// Webhook.Targets leads back here, and the JSON could loop.
|
||||
|
||||
@@ -87,13 +87,13 @@ func TestMainSchemaHoldsEveryModel(t *testing.T) {
|
||||
Active: true,
|
||||
})
|
||||
assertRoundTrip(t, db, &database.Target{
|
||||
BaseModel: newBaseModel(),
|
||||
WebhookID: uuid.New().String(),
|
||||
Name: "target",
|
||||
Type: database.TargetTypeHTTP,
|
||||
Active: true,
|
||||
Config: `{"url":"https://example.com/"}`,
|
||||
MaxRetries: 3,
|
||||
BaseModel: newBaseModel(),
|
||||
WebhookID: uuid.New().String(),
|
||||
Name: "target",
|
||||
Type: database.TargetTypeHTTP,
|
||||
Active: true,
|
||||
Config: `{"url":"https://example.com/"}`,
|
||||
MaxAttempts: 3,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -80,7 +80,7 @@ CREATE TABLE targets (
|
||||
type text NOT NULL,
|
||||
active numeric DEFAULT true,
|
||||
config text,
|
||||
max_retries integer,
|
||||
max_attempts integer,
|
||||
PRIMARY KEY (id),
|
||||
CONSTRAINT fk_webhooks_targets
|
||||
FOREIGN KEY (webhook_id) REFERENCES webhooks (id)
|
||||
|
||||
@@ -107,7 +107,7 @@ type Task struct {
|
||||
TargetName string
|
||||
TargetType database.TargetType
|
||||
TargetConfig string
|
||||
MaxRetries int
|
||||
MaxAttempts int
|
||||
|
||||
Method string
|
||||
RawQuery string
|
||||
@@ -1769,10 +1769,10 @@ func buildEventFromTask(task *Task) database.Event {
|
||||
|
||||
func buildTargetFromTask(task *Task) database.Target {
|
||||
target := database.Target{
|
||||
Name: task.TargetName,
|
||||
Type: task.TargetType,
|
||||
Config: task.TargetConfig,
|
||||
MaxRetries: task.MaxRetries,
|
||||
Name: task.TargetName,
|
||||
Type: task.TargetType,
|
||||
Config: task.TargetConfig,
|
||||
MaxAttempts: task.MaxAttempts,
|
||||
}
|
||||
|
||||
target.ID = task.TargetID
|
||||
@@ -2028,7 +2028,7 @@ func (e *Engine) countAttemptsBatch(
|
||||
// One delivery id per recorded attempt, tallied here rather than
|
||||
// grouped in SQL: internal/gormlog forbids (*gorm.DB).Scan, which
|
||||
// a GROUP BY into a struct would need, and an attempt row per
|
||||
// delivery is bounded by the target's MaxRetries.
|
||||
// delivery is bounded by the target's MaxAttempts.
|
||||
var attemptIDs []string
|
||||
|
||||
err := webhookDB.
|
||||
@@ -2106,7 +2106,7 @@ func buildRecoveryTask(
|
||||
TargetName: target.Name,
|
||||
TargetType: target.Type,
|
||||
TargetConfig: target.Config,
|
||||
MaxRetries: target.MaxRetries,
|
||||
MaxAttempts: target.MaxAttempts,
|
||||
Method: event.Method,
|
||||
RawQuery: event.RawQuery,
|
||||
Headers: event.Headers,
|
||||
|
||||
@@ -170,7 +170,7 @@ func iTask(
|
||||
d database.Delivery,
|
||||
event database.Event,
|
||||
webhookID, targetID, name, config string,
|
||||
maxRetries, attemptNum int,
|
||||
maxAttempts, attemptNum int,
|
||||
body *string,
|
||||
) delivery.Task {
|
||||
return delivery.Task{
|
||||
@@ -181,7 +181,7 @@ func iTask(
|
||||
TargetName: name,
|
||||
TargetType: database.TargetTypeHTTP,
|
||||
TargetConfig: config,
|
||||
MaxRetries: maxRetries,
|
||||
MaxAttempts: maxAttempts,
|
||||
Method: event.Method,
|
||||
Headers: event.Headers,
|
||||
ContentType: event.ContentType,
|
||||
@@ -242,7 +242,7 @@ func TestProcessNewTask_InlineBody(t *testing.T) {
|
||||
cfg := iHTTPConfig(ts.URL)
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"test-target", cfg, 0, 1, &bodyStr,
|
||||
"test-target", cfg, 1, 1, &bodyStr,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(
|
||||
@@ -293,7 +293,7 @@ func TestProcessNewTask_LargeBody_FetchFromDB(
|
||||
cfg := iHTTPConfig(ts.URL)
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"test-large", cfg, 0, 1, nil,
|
||||
"test-large", cfg, 1, 1, nil,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(
|
||||
@@ -320,7 +320,7 @@ func TestProcessNewTask_InvalidWebhookID(t *testing.T) {
|
||||
TargetName: "test",
|
||||
TargetType: database.TargetTypeHTTP,
|
||||
TargetConfig: iHTTPConfig("http://localhost:9999"),
|
||||
MaxRetries: 0,
|
||||
MaxAttempts: 1,
|
||||
Body: nil,
|
||||
AttemptNum: 1,
|
||||
}
|
||||
@@ -645,7 +645,7 @@ func TestRecoverPendingDeliveries(t *testing.T) {
|
||||
|
||||
iCreateTarget(t, s.MainDB, targetID,
|
||||
s.WebhookID, "recovery-target",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
iSeedPendingDeliveries(
|
||||
@@ -686,17 +686,17 @@ func iCreateTarget(
|
||||
targetID, webhookID, name string,
|
||||
targetType database.TargetType,
|
||||
config string,
|
||||
maxRetries int,
|
||||
maxAttempts int,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
target := database.Target{
|
||||
WebhookID: webhookID,
|
||||
Name: name,
|
||||
Type: targetType,
|
||||
Active: true,
|
||||
Config: config,
|
||||
MaxRetries: maxRetries,
|
||||
WebhookID: webhookID,
|
||||
Name: name,
|
||||
Type: targetType,
|
||||
Active: true,
|
||||
Config: config,
|
||||
MaxAttempts: maxAttempts,
|
||||
}
|
||||
target.ID = targetID
|
||||
|
||||
@@ -1077,7 +1077,7 @@ func TestRecoverInFlight_WithPendingDeliveries(
|
||||
|
||||
iCreateTarget(t, s.MainDB, targetID,
|
||||
s.WebhookID, "recover-target",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
@@ -1232,7 +1232,7 @@ func TestDeliverHTTP_CustomTargetHeaders(t *testing.T) {
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"auth-target", string(cfgJSON),
|
||||
0, 1, &bodyStr,
|
||||
1, 1, &bodyStr,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(
|
||||
@@ -1277,7 +1277,7 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) {
|
||||
|
||||
task, d := iHTTPTaskAndDelivery(
|
||||
event, del, "timeout-target",
|
||||
string(cfgJSON), 0, 1,
|
||||
string(cfgJSON), 1, 1,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTP(context.TODO(), db, d, task)
|
||||
@@ -1339,7 +1339,7 @@ func TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog(
|
||||
bodyStr := event.Body
|
||||
task := iTask(
|
||||
del, event, s.WebhookID, targetID,
|
||||
"cut-target", cfgJSON, 0, 1, &bodyStr,
|
||||
"cut-target", cfgJSON, 1, 1, &bodyStr,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(context.TODO(), &task)
|
||||
@@ -1403,7 +1403,7 @@ func iHTTPTaskAndDelivery(
|
||||
event database.Event,
|
||||
del database.Delivery,
|
||||
name, config string,
|
||||
maxRetries, attemptNum int,
|
||||
maxAttempts, attemptNum int,
|
||||
) (*delivery.Task, *database.Delivery) {
|
||||
task := &delivery.Task{
|
||||
DeliveryID: del.ID,
|
||||
@@ -1413,7 +1413,7 @@ func iHTTPTaskAndDelivery(
|
||||
TargetName: name,
|
||||
TargetType: database.TargetTypeHTTP,
|
||||
TargetConfig: config,
|
||||
MaxRetries: maxRetries,
|
||||
MaxAttempts: maxAttempts,
|
||||
AttemptNum: attemptNum,
|
||||
}
|
||||
|
||||
@@ -1466,7 +1466,7 @@ func TestDeliverHTTP_InvalidConfig(t *testing.T) {
|
||||
)
|
||||
|
||||
task, d := iHTTPTaskAndDelivery(
|
||||
event, del, "bad-config", `not-json`, 0, 1,
|
||||
event, del, "bad-config", `not-json`, 1, 1,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTP(context.TODO(), db, d, task)
|
||||
@@ -1490,7 +1490,7 @@ func TestDeliverHTTP_InvalidConfigUnrecordedStaysPending(t *testing.T) {
|
||||
)
|
||||
|
||||
task, d := iHTTPTaskAndDelivery(
|
||||
event, del, "bad-config", `not-json`, 0, 1,
|
||||
event, del, "bad-config", `not-json`, 1, 1,
|
||||
)
|
||||
|
||||
require.NoError(t, db.Exec("drop table delivery_results").Error)
|
||||
|
||||
@@ -278,7 +278,7 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
|
||||
}
|
||||
|
||||
// seedHeldDelivery gives the setup's webhook an http target with
|
||||
// maxRetries, pointed at a receiver that keeps every request open until
|
||||
// maxAttempts, pointed at a receiver that keeps every request open until
|
||||
// the client gives up on it, and seeds one delivery to that target at
|
||||
// status. It returns the task that sends attempt attemptNum of the
|
||||
// delivery, and a channel that is closed when a request reaches the
|
||||
@@ -286,7 +286,7 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
|
||||
func seedHeldDelivery(
|
||||
t *testing.T,
|
||||
s iSetup,
|
||||
maxRetries int,
|
||||
maxAttempts int,
|
||||
status database.DeliveryStatus,
|
||||
attemptNum int,
|
||||
) (delivery.Task, <-chan struct{}) {
|
||||
@@ -315,7 +315,7 @@ func seedHeldDelivery(
|
||||
iCreateWebhook(t, s.MainDB, s.WebhookID, "hook")
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "held",
|
||||
database.TargetTypeHTTP, cfg, maxRetries,
|
||||
database.TargetTypeHTTP, cfg, maxAttempts,
|
||||
)
|
||||
|
||||
event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"held":true}`)
|
||||
@@ -325,7 +325,7 @@ func seedHeldDelivery(
|
||||
|
||||
return iTask(
|
||||
d, event, s.WebhookID, targetID, "held", cfg,
|
||||
maxRetries, attemptNum, &bodyStr,
|
||||
maxAttempts, attemptNum, &bodyStr,
|
||||
), arrived
|
||||
}
|
||||
|
||||
@@ -380,7 +380,7 @@ func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) {
|
||||
lc := startEngineViaHook(t, s.Engine)
|
||||
|
||||
task, arrived := seedHeldDelivery(
|
||||
t, s, 0, database.DeliveryStatusPending, 1,
|
||||
t, s, 1, database.DeliveryStatusPending, 1,
|
||||
)
|
||||
|
||||
s.Engine.Notify([]delivery.Task{task})
|
||||
@@ -404,13 +404,13 @@ func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) {
|
||||
func TestEngine_StopLeavesLastRetryAttemptRetrying(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const maxRetries = 2
|
||||
const maxAttempts = 2
|
||||
|
||||
s := newISetup(t)
|
||||
lc := startEngineViaHook(t, s.Engine)
|
||||
|
||||
task, arrived := seedHeldDelivery(
|
||||
t, s, maxRetries, database.DeliveryStatusRetrying, maxRetries,
|
||||
t, s, maxAttempts, database.DeliveryStatusRetrying, maxAttempts,
|
||||
)
|
||||
iSeedFailedResult(t, s.WebhookDB, task.DeliveryID)
|
||||
|
||||
|
||||
@@ -142,7 +142,7 @@ func buildHTTPFixture(
|
||||
dlv database.Delivery,
|
||||
event database.Event,
|
||||
targetID, name, config string,
|
||||
maxRetries, attemptNum int,
|
||||
maxAttempts, attemptNum int,
|
||||
) httpDeliveryFixture {
|
||||
task := &delivery.Task{
|
||||
DeliveryID: dlv.ID,
|
||||
@@ -152,7 +152,7 @@ func buildHTTPFixture(
|
||||
TargetName: name,
|
||||
TargetType: database.TargetTypeHTTP,
|
||||
TargetConfig: config,
|
||||
MaxRetries: maxRetries,
|
||||
MaxAttempts: maxAttempts,
|
||||
AttemptNum: attemptNum,
|
||||
}
|
||||
|
||||
@@ -162,10 +162,10 @@ func buildHTTPFixture(
|
||||
Status: dlv.Status,
|
||||
Event: event,
|
||||
Target: database.Target{
|
||||
Name: name,
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: config,
|
||||
MaxRetries: maxRetries,
|
||||
Name: name,
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: config,
|
||||
MaxAttempts: maxAttempts,
|
||||
},
|
||||
}
|
||||
d.ID = dlv.ID
|
||||
@@ -277,7 +277,7 @@ func TestDeliverHTTP_Success(t *testing.T) {
|
||||
|
||||
cfg := newHTTPTargetConfig(ts.URL)
|
||||
fix := buildHTTPFixture(
|
||||
dlv, event, targetID, "test-http", cfg, 0, 1,
|
||||
dlv, event, targetID, "test-http", cfg, 1, 1,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTP(
|
||||
@@ -324,7 +324,7 @@ func TestDeliverHTTP_Failure(t *testing.T) {
|
||||
cfg := newHTTPTargetConfig(ts.URL)
|
||||
fix := buildHTTPFixture(
|
||||
dlv, event, targetID,
|
||||
"test-http-fail", cfg, 0, 1,
|
||||
"test-http-fail", cfg, 1, 1,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTP(
|
||||
@@ -473,14 +473,23 @@ func TestDeliverHTTP_WithRetries_Success(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
func TestDeliverHTTP_MaxRetriesExhausted(t *testing.T) {
|
||||
// TestDeliverHTTP_MaxAttemptsExhausted proves a target with three
|
||||
// delivery attempts makes at most three: the first two failures are
|
||||
// retried, the third is final, and each attempt goes through the
|
||||
// target's circuit breaker.
|
||||
func TestDeliverHTTP_MaxAttemptsExhausted(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const maxAttempts = 3
|
||||
|
||||
db := testWebhookDB(t)
|
||||
|
||||
var requests atomic.Int32
|
||||
|
||||
ts := httptest.NewServer(
|
||||
http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
requests.Add(1)
|
||||
w.WriteHeader(http.StatusBadGateway)
|
||||
},
|
||||
),
|
||||
@@ -489,26 +498,103 @@ func TestDeliverHTTP_MaxRetriesExhausted(t *testing.T) {
|
||||
|
||||
e := testEngine(t, 1)
|
||||
targetID := uuid.New().String()
|
||||
|
||||
// It opens on the last failure only if every attempt reached it.
|
||||
cb := delivery.NewTestCircuitBreaker(maxAttempts, time.Minute)
|
||||
e.ExportSetCircuitBreaker(targetID, cb)
|
||||
|
||||
event := seedEvent(t, db, `{"retry":"exhaust"}`)
|
||||
|
||||
dlv := seedDelivery(
|
||||
t, db, event.ID, targetID,
|
||||
database.DeliveryStatusRetrying,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
cfg := newHTTPTargetConfig(ts.URL)
|
||||
sched := &recordingScheduler{}
|
||||
|
||||
for attempt := 1; attempt <= maxAttempts; attempt++ {
|
||||
fix := buildHTTPFixture(
|
||||
dlv, event, targetID,
|
||||
"test-http-exhaust", cfg, maxAttempts, attempt,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTPWithScheduler(
|
||||
context.TODO(), db, fix.Delivery, fix.Task, sched,
|
||||
)
|
||||
}
|
||||
|
||||
assert.Equal(t, int32(maxAttempts), requests.Load())
|
||||
assert.Len(t, sched.delays, maxAttempts-1,
|
||||
"every failure but the last should be retried",
|
||||
)
|
||||
assertDeliveryStatus(t, db, dlv.ID,
|
||||
database.DeliveryStatusFailed,
|
||||
)
|
||||
assert.Equal(t, delivery.CircuitOpen, cb.State(),
|
||||
"every attempt should be counted by the circuit breaker",
|
||||
)
|
||||
}
|
||||
|
||||
// TestDeliverHTTP_SingleAttemptSkipsCircuitBreaker proves a target
|
||||
// with one delivery attempt makes exactly that attempt, even while
|
||||
// its circuit breaker is open, and that a failure is final.
|
||||
func TestDeliverHTTP_SingleAttemptSkipsCircuitBreaker(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := testWebhookDB(t)
|
||||
|
||||
var requests atomic.Int32
|
||||
|
||||
ts := httptest.NewServer(
|
||||
http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
requests.Add(1)
|
||||
w.WriteHeader(http.StatusBadGateway)
|
||||
},
|
||||
),
|
||||
)
|
||||
defer ts.Close()
|
||||
|
||||
e := testEngine(t, 1)
|
||||
targetID := uuid.New().String()
|
||||
|
||||
cb := e.ExportGetCircuitBreaker(targetID)
|
||||
|
||||
for range delivery.ExportDefaultFailureThreshold {
|
||||
cb.RecordFailure()
|
||||
}
|
||||
|
||||
require.Equal(t, delivery.CircuitOpen, cb.State())
|
||||
|
||||
event := seedEvent(t, db, `{"retry":"none"}`)
|
||||
|
||||
dlv := seedDelivery(
|
||||
t, db, event.ID, targetID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
cfg := newHTTPTargetConfig(ts.URL)
|
||||
fix := buildHTTPFixture(
|
||||
dlv, event, targetID,
|
||||
"test-http-exhaust", cfg, 3, 3,
|
||||
"test-http-single", cfg, 1, 1,
|
||||
)
|
||||
sched := &recordingScheduler{}
|
||||
|
||||
e.ExportDeliverHTTPWithScheduler(
|
||||
context.TODO(), db, fix.Delivery, fix.Task, sched,
|
||||
)
|
||||
|
||||
e.ExportDeliverHTTP(
|
||||
context.TODO(), db, fix.Delivery, fix.Task,
|
||||
assert.Equal(t, int32(1), requests.Load(),
|
||||
"the open circuit breaker should not hold the attempt back",
|
||||
)
|
||||
|
||||
assert.Empty(t, sched.delays, "no retry should be scheduled")
|
||||
assertDeliveryStatus(t, db, dlv.ID,
|
||||
database.DeliveryStatusFailed,
|
||||
)
|
||||
assertDeliveryResult(
|
||||
t, db, dlv.ID, false, http.StatusBadGateway,
|
||||
)
|
||||
}
|
||||
|
||||
func TestDeliverHTTP_SchedulesRetryOnFailure(
|
||||
@@ -741,7 +827,7 @@ func seedConcurrencyTasks(
|
||||
TargetName: tasks[i].Target.Name,
|
||||
TargetType: tasks[i].Target.Type,
|
||||
TargetConfig: tasks[i].Target.Config,
|
||||
MaxRetries: 0,
|
||||
MaxAttempts: 1,
|
||||
AttemptNum: 1,
|
||||
}
|
||||
}
|
||||
@@ -2051,10 +2137,10 @@ func buildSlackRetryDelivery(
|
||||
Status: database.DeliveryStatusPending,
|
||||
Event: event,
|
||||
Target: database.Target{
|
||||
Name: "test-slack-retry",
|
||||
Type: database.TargetTypeSlack,
|
||||
Config: cfg,
|
||||
MaxRetries: 5,
|
||||
Name: "test-slack-retry",
|
||||
Type: database.TargetTypeSlack,
|
||||
Config: cfg,
|
||||
MaxAttempts: 5,
|
||||
},
|
||||
}
|
||||
d.ID = dlv.ID
|
||||
@@ -2089,11 +2175,11 @@ func TestDeliverSlack_WithRetries_SchedulesRetry(
|
||||
)
|
||||
|
||||
task := &delivery.Task{
|
||||
DeliveryID: dlv.ID,
|
||||
TargetID: targetID,
|
||||
TargetType: database.TargetTypeSlack,
|
||||
MaxRetries: 5,
|
||||
AttemptNum: 1,
|
||||
DeliveryID: dlv.ID,
|
||||
TargetID: targetID,
|
||||
TargetType: database.TargetTypeSlack,
|
||||
MaxAttempts: 5,
|
||||
AttemptNum: 1,
|
||||
}
|
||||
|
||||
e.ExportProcessDelivery(context.TODO(), db, d, task)
|
||||
|
||||
@@ -152,7 +152,7 @@ func TestEventDBHoldsNoTargetRowsOnFailedDelivery(t *testing.T) {
|
||||
)
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"leaky-target", cfg, 0, 1, &body,
|
||||
"leaky-target", cfg, 1, 1, &body,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(context.TODO(), &task)
|
||||
|
||||
@@ -176,7 +176,7 @@ func tsTask(
|
||||
TargetName: target.Name,
|
||||
TargetType: database.TargetTypeSlack,
|
||||
TargetConfig: target.Config,
|
||||
MaxRetries: 0,
|
||||
MaxAttempts: 1,
|
||||
Method: event.Method,
|
||||
Headers: event.Headers,
|
||||
ContentType: event.ContentType,
|
||||
|
||||
@@ -229,7 +229,7 @@ func TestOwnershipIsReleasedAfterDelivery(t *testing.T) {
|
||||
|
||||
iCreateTarget(t, s.MainDB, targetID,
|
||||
s.WebhookID, "released",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
|
||||
@@ -291,20 +291,20 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
|
||||
body := event.Body
|
||||
cfg := iHTTPConfig(ts.URL)
|
||||
|
||||
// A retry budget above the failure threshold, so the breaker
|
||||
// An attempt budget above the failure threshold, so the breaker
|
||||
// rather than the budget is what stops the delivery.
|
||||
maxRetries := delivery.ExportDefaultFailureThreshold + 5
|
||||
maxAttempts := delivery.ExportDefaultFailureThreshold + 5
|
||||
|
||||
// The retries below are only run if their target still exists;
|
||||
// see https://git.eeqj.de/sneak/webhooker/issues/107.
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "metrics-trip",
|
||||
database.TargetTypeHTTP, cfg, maxRetries,
|
||||
database.TargetTypeHTTP, cfg, maxAttempts,
|
||||
)
|
||||
|
||||
first := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"metrics-trip", cfg, maxRetries, 1, &body,
|
||||
"metrics-trip", cfg, maxAttempts, 1, &body,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(context.TODO(), &first)
|
||||
@@ -316,7 +316,7 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
|
||||
ExportDefaultFailureThreshold; attempt++ {
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"metrics-trip", cfg, maxRetries, attempt, &body,
|
||||
"metrics-trip", cfg, maxAttempts, attempt, &body,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessRetryTask(
|
||||
@@ -364,16 +364,16 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
|
||||
|
||||
body := event.Body
|
||||
cfg := iHTTPConfig(ts.URL)
|
||||
maxRetries := delivery.ExportDefaultFailureThreshold + 5
|
||||
maxAttempts := delivery.ExportDefaultFailureThreshold + 5
|
||||
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "metrics-blocked",
|
||||
database.TargetTypeHTTP, cfg, maxRetries,
|
||||
database.TargetTypeHTTP, cfg, maxAttempts,
|
||||
)
|
||||
|
||||
first := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"metrics-blocked", cfg, maxRetries, 1, &body,
|
||||
"metrics-blocked", cfg, maxAttempts, 1, &body,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessNewTask(context.TODO(), &first)
|
||||
@@ -382,7 +382,7 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
|
||||
ExportDefaultFailureThreshold; attempt++ {
|
||||
task := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"metrics-blocked", cfg, maxRetries, attempt, &body,
|
||||
"metrics-blocked", cfg, maxAttempts, attempt, &body,
|
||||
)
|
||||
|
||||
s.Engine.ExportProcessRetryTask(context.TODO(), &task)
|
||||
@@ -405,7 +405,7 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
|
||||
|
||||
blocked := iTask(
|
||||
d, event, s.WebhookID, targetID,
|
||||
"metrics-blocked", cfg, maxRetries,
|
||||
"metrics-blocked", cfg, maxAttempts,
|
||||
delivery.ExportDefaultFailureThreshold+1, &body,
|
||||
)
|
||||
|
||||
|
||||
@@ -66,7 +66,7 @@ func TestRecoverySkipsPendingWithSuccessfulResult(
|
||||
|
||||
iCreateTarget(t, s.MainDB, targetID,
|
||||
s.WebhookID, "already-delivered",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
@@ -129,7 +129,7 @@ func TestRecoveryContinuesTheAttemptNumbering(t *testing.T) {
|
||||
|
||||
iCreateTarget(t, s.MainDB, targetID,
|
||||
s.WebhookID, "numbering",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
@@ -444,10 +444,10 @@ func TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable(
|
||||
Status: database.DeliveryStatusPending,
|
||||
Event: event,
|
||||
Target: database.Target{
|
||||
Name: "unwritable",
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: iHTTPConfig(ts.URL),
|
||||
MaxRetries: 3,
|
||||
Name: "unwritable",
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: iHTTPConfig(ts.URL),
|
||||
MaxAttempts: 3,
|
||||
},
|
||||
}
|
||||
full.ID = d.ID
|
||||
|
||||
@@ -120,8 +120,8 @@ func unavailableConfigFields() []ConfigField {
|
||||
}
|
||||
|
||||
// slackConfigFields describes a Slack target: its masked
|
||||
// webhook URL and its retry count. Only the masked URL is
|
||||
// shown; the full URL is the credential.
|
||||
// webhook URL and its number of delivery attempts. Only the
|
||||
// masked URL is shown; the full URL is the credential.
|
||||
func slackConfigFields(t *database.Target) []ConfigField {
|
||||
cfg, err := parseSlackConfig(t.Config)
|
||||
if err != nil {
|
||||
@@ -131,7 +131,7 @@ func slackConfigFields(t *database.Target) []ConfigField {
|
||||
return []ConfigField{{
|
||||
Label: "Webhook URL",
|
||||
Value: cfg.MaskedWebhookURL(),
|
||||
}, maxRetriesField(t)}
|
||||
}, maxAttemptsField(t)}
|
||||
}
|
||||
|
||||
// httpConfigFields describes an HTTP target: its destination
|
||||
@@ -178,23 +178,18 @@ func httpConfigFields(t *database.Target) []ConfigField {
|
||||
})
|
||||
}
|
||||
|
||||
fields = append(fields, maxRetriesField(t))
|
||||
fields = append(fields, maxAttemptsField(t))
|
||||
|
||||
return fields
|
||||
}
|
||||
|
||||
// maxRetriesField describes a target's retry count, which lives
|
||||
// on the target row rather than in its configuration blob. A
|
||||
// stored 0 makes a single attempt, so it is shown as 1.
|
||||
func maxRetriesField(t *database.Target) ConfigField {
|
||||
attempts := strconv.Itoa(t.MaxRetries)
|
||||
if t.MaxRetries == 0 {
|
||||
attempts = "1 (fire-and-forget: no retries, no circuit breaker)"
|
||||
}
|
||||
|
||||
// maxAttemptsField describes a target's number of delivery
|
||||
// attempts, which lives on the target row rather than in its
|
||||
// configuration blob.
|
||||
func maxAttemptsField(t *database.Target) ConfigField {
|
||||
return ConfigField{
|
||||
Label: "Delivery attempts",
|
||||
Value: attempts,
|
||||
Value: strconv.Itoa(t.MaxAttempts),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -32,7 +32,7 @@ const (
|
||||
viewMaskedOrigin = viewExampleOrigin + "/..."
|
||||
viewUnavailable = "(unavailable)"
|
||||
viewExpiryNever = "never"
|
||||
viewMaxRetries = "Delivery attempts"
|
||||
viewMaxAttempts = "Delivery attempts"
|
||||
)
|
||||
|
||||
func TestMaskedWebhookURL(t *testing.T) {
|
||||
@@ -171,6 +171,7 @@ func slackTarget() database.Target {
|
||||
Active: true,
|
||||
Config: `{"webhookUrl":"` +
|
||||
slackWebhookURL + `"}`,
|
||||
MaxAttempts: 1,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -189,28 +190,29 @@ func TestNewTargetViews_Slack(t *testing.T) {
|
||||
assert.Equal(
|
||||
t,
|
||||
map[string]string{
|
||||
"Webhook URL": slackMaskedURL,
|
||||
viewMaxRetries: "1 (fire-and-forget: no retries, no circuit breaker)",
|
||||
"Webhook URL": slackMaskedURL,
|
||||
viewMaxAttempts: "1",
|
||||
},
|
||||
fieldMap(view.Config),
|
||||
)
|
||||
}
|
||||
|
||||
// TestNewTargetViews_SlackRetries proves a Slack target shows
|
||||
// its retry count the same way an HTTP target does.
|
||||
func TestNewTargetViews_SlackRetries(t *testing.T) {
|
||||
// TestNewTargetViews_SlackAttempts proves a Slack target shows
|
||||
// its number of delivery attempts the same way an HTTP target
|
||||
// does.
|
||||
func TestNewTargetViews_SlackAttempts(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
target := slackTarget()
|
||||
target.MaxRetries = 2
|
||||
target.MaxAttempts = 2
|
||||
|
||||
view := viewFor(t, target)
|
||||
|
||||
assert.Equal(
|
||||
t,
|
||||
map[string]string{
|
||||
"Webhook URL": slackMaskedURL,
|
||||
viewMaxRetries: "2",
|
||||
"Webhook URL": slackMaskedURL,
|
||||
viewMaxAttempts: "2",
|
||||
},
|
||||
fieldMap(view.Config),
|
||||
)
|
||||
@@ -225,7 +227,7 @@ func TestNewTargetViews_HTTP(t *testing.T) {
|
||||
`"timeout":30,` +
|
||||
`"headers":{"Authorization":"Bearer sekrit"},` +
|
||||
`"forwardQuery":true}`,
|
||||
MaxRetries: 5,
|
||||
MaxAttempts: 5,
|
||||
})
|
||||
|
||||
fields := fieldMap(view.Config)
|
||||
@@ -237,7 +239,7 @@ func TestNewTargetViews_HTTP(t *testing.T) {
|
||||
"Timeout": "30s",
|
||||
"Headers": "1 configured",
|
||||
"Query string": "passed on to this target",
|
||||
viewMaxRetries: "5",
|
||||
viewMaxAttempts: "5",
|
||||
},
|
||||
fields,
|
||||
)
|
||||
@@ -248,19 +250,22 @@ func TestNewTargetViews_HTTP(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewTargetViews_HTTPFireAndForget(t *testing.T) {
|
||||
// TestNewTargetViews_HTTPSingleAttempt proves a single attempt is
|
||||
// shown as the plain number, like any other.
|
||||
func TestNewTargetViews_HTTPSingleAttempt(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
view := viewFor(t, database.Target{
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: `{"url":"` + viewExampleHook + `"}`,
|
||||
Type: database.TargetTypeHTTP,
|
||||
Config: `{"url":"` + viewExampleHook + `"}`,
|
||||
MaxAttempts: 1,
|
||||
})
|
||||
|
||||
assert.Equal(
|
||||
t,
|
||||
map[string]string{
|
||||
"Destination URL": viewMaskedOrigin,
|
||||
viewMaxRetries: "1 (fire-and-forget: no retries, no circuit breaker)",
|
||||
viewMaxAttempts: "1",
|
||||
},
|
||||
fieldMap(view.Config),
|
||||
)
|
||||
|
||||
@@ -53,10 +53,10 @@ type httpCore struct {
|
||||
}
|
||||
|
||||
// deliver runs one delivery attempt through the retry core.
|
||||
// A maxRetries of 0 is fire-and-forget: a single attempt is
|
||||
// recorded and no circuit breaker is consulted. A positive
|
||||
// maxRetries gates the attempt on the circuit breaker and
|
||||
// schedules a backed-off retry on failure.
|
||||
// A maxAttempts of 1 is a single attempt, recorded without
|
||||
// consulting a circuit breaker. A higher maxAttempts gates the
|
||||
// attempt on the circuit breaker and schedules a backed-off retry
|
||||
// on failure, until maxAttempts attempts have been made.
|
||||
//
|
||||
// ctx is the context attempt sends its request with. Stopping the
|
||||
// engine cancels it; see interruptedByStop.
|
||||
@@ -66,17 +66,17 @@ func (c *httpCore) deliver(
|
||||
d *database.Delivery,
|
||||
task *Task,
|
||||
sched Scheduler,
|
||||
maxRetries int,
|
||||
maxAttempts int,
|
||||
attempt func() attemptResult,
|
||||
) {
|
||||
if maxRetries == 0 {
|
||||
if maxAttempts <= 1 {
|
||||
c.fireAndForget(ctx, webhookDB, d, attempt())
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
c.withRetry(
|
||||
ctx, webhookDB, d, task, sched, maxRetries, attempt,
|
||||
ctx, webhookDB, d, task, sched, maxAttempts, attempt,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -153,7 +153,7 @@ func (c *httpCore) withRetry(
|
||||
d *database.Delivery,
|
||||
task *Task,
|
||||
sched Scheduler,
|
||||
maxRetries int,
|
||||
maxAttempts int,
|
||||
attempt func() attemptResult,
|
||||
) {
|
||||
cb := c.getCircuitBreaker(task.TargetID)
|
||||
@@ -205,7 +205,7 @@ func (c *httpCore) withRetry(
|
||||
cb.RecordFailure()
|
||||
|
||||
c.handleRetry(
|
||||
webhookDB, d, task, sched, maxRetries, attemptNum,
|
||||
webhookDB, d, task, sched, maxAttempts, attemptNum,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -266,10 +266,10 @@ func (c *httpCore) handleRetry(
|
||||
d *database.Delivery,
|
||||
task *Task,
|
||||
sched Scheduler,
|
||||
maxRetries int,
|
||||
maxAttempts int,
|
||||
attemptNum int,
|
||||
) {
|
||||
if attemptNum >= maxRetries {
|
||||
if attemptNum >= maxAttempts {
|
||||
c.eng.settleStatus(
|
||||
webhookDB, d, d.Target.Type,
|
||||
database.DeliveryStatusFailed,
|
||||
@@ -439,7 +439,7 @@ func (t *httpTarget) Deliver(
|
||||
|
||||
t.deliver(
|
||||
ctx, webhookDB, d, task, sched,
|
||||
d.Target.MaxRetries, attempt,
|
||||
d.Target.MaxAttempts, attempt,
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -30,9 +30,8 @@ type SlackTargetConfig struct {
|
||||
// slackTarget delivers events to Slack incoming webhooks. It
|
||||
// formats the event into a Slack message and posts it as
|
||||
// JSON. It shares the retry core with the HTTP target: a
|
||||
// MaxRetries of 0 stays single-attempt fire-and-forget
|
||||
// (preserving existing Slack targets), while a positive
|
||||
// MaxRetries adds backoff and circuit breaking.
|
||||
// MaxAttempts of 1 is a single attempt, while a higher one
|
||||
// adds backoff and circuit breaking.
|
||||
type slackTarget struct {
|
||||
*httpCore
|
||||
|
||||
@@ -83,7 +82,7 @@ func (t *slackTarget) Deliver(
|
||||
|
||||
t.deliver(
|
||||
ctx, webhookDB, d, task, sched,
|
||||
d.Target.MaxRetries, attempt,
|
||||
d.Target.MaxAttempts, attempt,
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -743,7 +743,7 @@ func TestSendRecoveredDeliveries_TargetMissingFromMap(
|
||||
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "found-on-lookup",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
@@ -790,7 +790,7 @@ func TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone(
|
||||
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "healthy",
|
||||
database.TargetTypeLog, "", 0,
|
||||
database.TargetTypeLog, "", 1,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
|
||||
@@ -308,7 +308,7 @@ func createReplayDelivery(
|
||||
TargetName: target.Name,
|
||||
TargetType: target.Type,
|
||||
TargetConfig: target.Config,
|
||||
MaxRetries: target.MaxRetries,
|
||||
MaxAttempts: target.MaxAttempts,
|
||||
Method: event.Method,
|
||||
RawQuery: event.RawQuery,
|
||||
Headers: event.Headers,
|
||||
|
||||
@@ -496,7 +496,7 @@ const deliveryIDChunkSize = 500
|
||||
//
|
||||
// Each response body is cut by SQLite rather than in Go, for
|
||||
// the reason deliveryResultColumns gives. How many attempts a
|
||||
// delivery has is the target's MaxRetries, which the
|
||||
// delivery has is bounded by the target's MaxAttempts, which the
|
||||
// authenticated operator sets; how many of them reach the page
|
||||
// is bounded again by maxRenderedAttempts.
|
||||
func (h *Handlers) loadDeliveryResults(
|
||||
|
||||
@@ -37,10 +37,10 @@ const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes
|
||||
// per-delivery attempt ceiling to the handlers_test package.
|
||||
const MaxRenderedAttemptsForTest = maxRenderedAttempts
|
||||
|
||||
// MaxTargetRetriesForTest exposes the target max_retries ceiling to
|
||||
// MaxTargetAttemptsForTest exposes the target max_attempts ceiling to
|
||||
// the handlers_test package, so the tests assert against the constant
|
||||
// the handlers enforce rather than a number copied beside it.
|
||||
const MaxTargetRetriesForTest = maxTargetRetries
|
||||
const MaxTargetAttemptsForTest = maxTargetAttempts
|
||||
|
||||
// EventDBLeftMsgForTest and SidecarLeftMsgForTest expose the two
|
||||
// messages the webhook delete handler logs when a file of the event
|
||||
|
||||
@@ -0,0 +1,81 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"strconv"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// maxTargetAttempts bounds a target's max_attempts.
|
||||
//
|
||||
// Both target forms already declare max="20" on the input, so this
|
||||
// enforces server-side what the UI has always advertised rather than
|
||||
// introducing a new limit.
|
||||
//
|
||||
// The number is not cosmetic. Every attempt writes a delivery_results
|
||||
// row that the event log then loads and renders, and the engine backs
|
||||
// off by 2^(n-1) seconds, so attempt 20 is already about six days
|
||||
// after the first. A value beyond this buys no additional durability
|
||||
// and only costs rows.
|
||||
const maxTargetAttempts = 20
|
||||
|
||||
// Errors returned when a max_attempts form value cannot be turned into
|
||||
// a number of attempts.
|
||||
var (
|
||||
// errAttemptsInvalid signals a max_attempts form value that is not
|
||||
// a whole number of at least 1.
|
||||
errAttemptsInvalid = errors.New(
|
||||
"must be a whole number, at least 1",
|
||||
)
|
||||
|
||||
// errAttemptsTooLarge signals a max_attempts form value that is a
|
||||
// whole number but above maxTargetAttempts. It is distinguished
|
||||
// from errAttemptsInvalid so the message can name the ceiling
|
||||
// instead of implying the input was not a number.
|
||||
errAttemptsTooLarge = errors.New("out of range")
|
||||
)
|
||||
|
||||
// parseMaxAttempts interprets a max_attempts form value.
|
||||
//
|
||||
// An ABSENT value — the field empty or not submitted — yields
|
||||
// fallback, which lets the create path apply its default and the edit
|
||||
// path leave the stored value alone. A value that is SET BUT INVALID
|
||||
// is an error: unparseable, below 1, or above maxTargetAttempts.
|
||||
//
|
||||
// The distinction is the whole point of this function. Returning a
|
||||
// default for input the operator typed but that did not parse would
|
||||
// silently turn retries off on a store-and-forward proxy — and on the
|
||||
// edit path destroy a working configuration over a typo. A default
|
||||
// answers a question that was not asked; it never answers one that
|
||||
// was asked badly.
|
||||
func parseMaxAttempts(raw string, fallback int) (int, error) {
|
||||
raw = strings.TrimSpace(raw)
|
||||
if raw == "" {
|
||||
return fallback, nil
|
||||
}
|
||||
|
||||
v, err := strconv.Atoi(raw)
|
||||
if err != nil || v < 1 {
|
||||
return 0, errAttemptsInvalid
|
||||
}
|
||||
|
||||
if v > maxTargetAttempts {
|
||||
return 0, errAttemptsTooLarge
|
||||
}
|
||||
|
||||
return v, nil
|
||||
}
|
||||
|
||||
// attemptsErrorMessage returns the message the create and edit forms
|
||||
// show for a rejected max_attempts value. Any error other than
|
||||
// errAttemptsTooLarge falls back to the generic wording, so an
|
||||
// unrecognised parse failure still produces a sensible 400.
|
||||
func attemptsErrorMessage(err error) string {
|
||||
if errors.Is(err, errAttemptsTooLarge) {
|
||||
return errAttemptsTooLarge.Error() +
|
||||
": at most " + strconv.Itoa(maxTargetAttempts) +
|
||||
" attempts"
|
||||
}
|
||||
|
||||
return errAttemptsInvalid.Error()
|
||||
}
|
||||
@@ -0,0 +1,429 @@
|
||||
package handlers_test
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
)
|
||||
|
||||
// attemptsTargetURL is the destination the attempts-validation targets
|
||||
// point at. It is a literal public address rather than a hostname so
|
||||
// the SSRF check resolves nothing and a sandbox without DNS cannot
|
||||
// make these cases pass or fail for the wrong reason.
|
||||
const attemptsTargetURL = "https://93.184.216.34/hooks/attempts"
|
||||
|
||||
const (
|
||||
// wayAboveCeiling is the typo'd-extra-zero case from the report.
|
||||
wayAboveCeiling = "999999999"
|
||||
|
||||
// notANumber is the plainest garbage an operator can type, and
|
||||
// the value the report submitted on the edit form.
|
||||
notANumber = "abc"
|
||||
|
||||
// workingAttempts is the number of attempts a seeded target is
|
||||
// already delivering with, which a rejected submission must not
|
||||
// disturb.
|
||||
workingAttempts = 2
|
||||
)
|
||||
|
||||
// aboveCeiling is the smallest rejected whole number.
|
||||
func aboveCeiling() string {
|
||||
return strconv.Itoa(handlers.MaxTargetAttemptsForTest + 1)
|
||||
}
|
||||
|
||||
// overCeilingAttempts is whole-number input past the limit, which is
|
||||
// rejected with the limit named.
|
||||
func overCeilingAttempts() []string {
|
||||
return []string{aboveCeiling(), wayAboveCeiling}
|
||||
}
|
||||
|
||||
// belowOneAttempts is whole-number input under the floor of one
|
||||
// attempt, which is rejected with the floor named.
|
||||
func belowOneAttempts() []string {
|
||||
return []string{"0", "-5"}
|
||||
}
|
||||
|
||||
// unparseableAttempts is input an operator can type into the field
|
||||
// that is not a number of attempts. Each must be REJECTED: silently
|
||||
// reading any of them as a default turns retries off on a
|
||||
// store-and-forward proxy without saying so.
|
||||
//
|
||||
// The twenty-digit case is here because it parses as digits but
|
||||
// overflows int, which is the one failure the field's own min/max
|
||||
// attributes cannot describe.
|
||||
func unparseableAttempts() []string {
|
||||
return []string{
|
||||
notANumber,
|
||||
"2.7",
|
||||
"12345678901234567890",
|
||||
"1e3",
|
||||
}
|
||||
}
|
||||
|
||||
// createAttemptsForm is a complete, otherwise-valid HTTP target
|
||||
// creation, so the only thing any case below varies is max_attempts.
|
||||
func createAttemptsForm(attempts string) url.Values {
|
||||
form := url.Values{}
|
||||
form.Set("name", "attempts-target")
|
||||
form.Set("type", string(database.TargetTypeHTTP))
|
||||
form.Set("url", attemptsTargetURL)
|
||||
|
||||
if attempts != absentField {
|
||||
form.Set("max_attempts", attempts)
|
||||
}
|
||||
|
||||
return form
|
||||
}
|
||||
|
||||
// absentField marks a field the form does not submit at all, which is
|
||||
// the case that legitimately takes a default and must stay distinct
|
||||
// from a field submitted with garbage in it.
|
||||
const absentField = "\x00absent"
|
||||
|
||||
// absentAttempts is every way of saying "the operator did not set
|
||||
// this", each of which takes the default rather than a 400. Blank and
|
||||
// whitespace-only count as absent here because they do in the timeout
|
||||
// and retention controls on the same forms; a rule the fields do not
|
||||
// share would be its own surprise.
|
||||
func absentAttempts() []string {
|
||||
return []string{absentField, "", " "}
|
||||
}
|
||||
|
||||
// createWithAttempts posts the target create form for a fresh webhook
|
||||
// and returns the webhook and the response.
|
||||
func createWithAttempts(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
attempts string,
|
||||
) (database.Webhook, int, string) {
|
||||
t.Helper()
|
||||
|
||||
webhook := seedWebhookWithRetention(t, env.db, 30)
|
||||
|
||||
w := serveTarget(
|
||||
env, http.MethodPost,
|
||||
"/hook/"+webhook.ID+"/targets",
|
||||
createAttemptsForm(attempts),
|
||||
)
|
||||
|
||||
return webhook, w.Code, w.Body.String()
|
||||
}
|
||||
|
||||
// TestTargetCreate_AttemptsAboveCeilingRejected proves the create form
|
||||
// enforces a ceiling at all, and that the 400 names it — a rejection
|
||||
// that does not say what the limit is leaves the operator guessing.
|
||||
func TestTargetCreate_AttemptsAboveCeilingRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
ceiling := strconv.Itoa(handlers.MaxTargetAttemptsForTest)
|
||||
|
||||
for _, attempts := range overCeilingAttempts() {
|
||||
webhook, code, body := createWithAttempts(t, env, attempts)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, code,
|
||||
"max_attempts=%s should be rejected", attempts)
|
||||
assert.Contains(t, body, ceiling,
|
||||
"the rejection for %s should name the ceiling",
|
||||
attempts)
|
||||
assert.Empty(t,
|
||||
targetsForWebhook(t, env.db, webhook.ID),
|
||||
"no target should be created for %s", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_AttemptsBelowOneRejected proves the create form
|
||||
// refuses 0 and less, naming the floor: every delivery is attempted
|
||||
// at least once.
|
||||
func TestTargetCreate_AttemptsBelowOneRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range belowOneAttempts() {
|
||||
webhook, code, body := createWithAttempts(t, env, attempts)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, code,
|
||||
"max_attempts=%s should be rejected", attempts)
|
||||
assert.Contains(t, body, "at least 1",
|
||||
"the rejection for %s should name the floor",
|
||||
attempts)
|
||||
assert.Empty(t,
|
||||
targetsForWebhook(t, env.db, webhook.ID),
|
||||
"no target should be created for %s", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_UnparseableAttemptsRejected is the core of the
|
||||
// defect: each of these was accepted with HTTP 200 and stored as a
|
||||
// default.
|
||||
func TestTargetCreate_UnparseableAttemptsRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range unparseableAttempts() {
|
||||
webhook, code, body := createWithAttempts(t, env, attempts)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, code,
|
||||
"max_attempts=%q should be rejected, not coerced",
|
||||
attempts)
|
||||
assert.Contains(t, body, "whole number",
|
||||
"the rejection for %q should say why", attempts)
|
||||
assert.Empty(t,
|
||||
targetsForWebhook(t, env.db, webhook.ID),
|
||||
"no target should be created for %q", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_ValidAttemptsStored covers the accepting half,
|
||||
// including both bounds: a bound that rejects its own limit would
|
||||
// make the advertised range unreachable.
|
||||
func TestTargetCreate_ValidAttemptsStored(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, want := range []int{1, 3, handlers.MaxTargetAttemptsForTest} {
|
||||
webhook, code, body := createWithAttempts(
|
||||
t, env, strconv.Itoa(want),
|
||||
)
|
||||
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
assert.Equal(t, want, targets[0].MaxAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_AbsentAttemptsTakesDefault keeps the two cases
|
||||
// distinct. An omitted field is not an operator asking for something
|
||||
// invalid, so it still gets the default of one attempt rather than a
|
||||
// 400 — otherwise the fix above would make the form unusable.
|
||||
func TestTargetCreate_AbsentAttemptsTakesDefault(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range absentAttempts() {
|
||||
webhook, code, body := createWithAttempts(t, env, attempts)
|
||||
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
assert.Equal(t, 1, targets[0].MaxAttempts,
|
||||
"an absent max_attempts should take the default")
|
||||
}
|
||||
}
|
||||
|
||||
// seedAttemptsTarget creates an HTTP target already delivering with
|
||||
// workingAttempts attempts, through the real create handler.
|
||||
func seedAttemptsTarget(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
) (database.Webhook, database.Target) {
|
||||
t.Helper()
|
||||
|
||||
webhook, code, body := createWithAttempts(
|
||||
t, env, strconv.Itoa(workingAttempts),
|
||||
)
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
require.Equal(t, workingAttempts, targets[0].MaxAttempts)
|
||||
|
||||
return webhook, targets[0]
|
||||
}
|
||||
|
||||
// editAttemptsForm is a complete edit submission that changes the
|
||||
// target's name as well, so a rejected submission can be shown to
|
||||
// have written nothing at all rather than merely to have left
|
||||
// max_attempts alone.
|
||||
func editAttemptsForm(attempts string) url.Values {
|
||||
form := url.Values{}
|
||||
form.Set("name", "renamed-by-edit")
|
||||
form.Set("url", attemptsTargetURL)
|
||||
|
||||
if attempts != absentField {
|
||||
form.Set("max_attempts", attempts)
|
||||
}
|
||||
|
||||
return form
|
||||
}
|
||||
|
||||
// assertEditRejectedAndUnchanged submits an edit expected to fail and
|
||||
// checks both halves of the requirement: the 400 explains itself, and
|
||||
// the target it was submitted against is untouched.
|
||||
func assertEditRejectedAndUnchanged(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
attempts, wantReason string,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
webhook, target := seedAttemptsTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID, editAttemptsForm(attempts),
|
||||
)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, w.Code,
|
||||
"max_attempts=%q should be rejected on edit", attempts)
|
||||
assert.Contains(t, w.Body.String(), wantReason,
|
||||
"the rejection for %q should say why", attempts)
|
||||
|
||||
stored := storedTarget(t, env, target.ID)
|
||||
assert.Equal(t, workingAttempts, stored.MaxAttempts,
|
||||
"a rejected edit must not destroy the working number "+
|
||||
"of attempts with %q", attempts)
|
||||
assert.Equal(t, "attempts-target", stored.Name,
|
||||
"a rejected edit must write nothing at all")
|
||||
}
|
||||
|
||||
// TestTargetEdit_UnparseableAttemptsRejected is the damaging half of
|
||||
// the defect. A target delivering with retries, re-saved with a typo
|
||||
// in the field, returned 200 and was left with retries disabled and
|
||||
// nothing said.
|
||||
func TestTargetEdit_UnparseableAttemptsRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range unparseableAttempts() {
|
||||
assertEditRejectedAndUnchanged(
|
||||
t, env, attempts, "whole number",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_AttemptsBelowOneRejected proves the floor applies
|
||||
// to the edit path too.
|
||||
func TestTargetEdit_AttemptsBelowOneRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range belowOneAttempts() {
|
||||
assertEditRejectedAndUnchanged(t, env, attempts, "at least 1")
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_AttemptsAboveCeilingRejected proves the ceiling
|
||||
// applies to the edit path too, naming itself, so the two paths
|
||||
// cannot disagree about what is storable.
|
||||
func TestTargetEdit_AttemptsAboveCeilingRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
ceiling := strconv.Itoa(handlers.MaxTargetAttemptsForTest)
|
||||
|
||||
for _, attempts := range overCeilingAttempts() {
|
||||
assertEditRejectedAndUnchanged(t, env, attempts, ceiling)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_ValidAttemptsStored covers the accepting half of the
|
||||
// edit path, so the bounds cannot be enforced by simply refusing
|
||||
// every submission.
|
||||
func TestTargetEdit_ValidAttemptsStored(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, want := range []int{1, 9, handlers.MaxTargetAttemptsForTest} {
|
||||
webhook, target := seedAttemptsTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editAttemptsForm(strconv.Itoa(want)),
|
||||
)
|
||||
require.Equal(t,
|
||||
http.StatusSeeOther, w.Code, w.Body.String(),
|
||||
)
|
||||
|
||||
assert.Equal(t, want,
|
||||
storedTarget(t, env, target.ID).MaxAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_AbsentAttemptsLeavesStoredValue is the edit path's
|
||||
// absent-versus-invalid case. The field is only offered by the forms
|
||||
// for types that retry, so a submission without it must leave the
|
||||
// stored count alone rather than be rejected or reset.
|
||||
func TestTargetEdit_AbsentAttemptsLeavesStoredValue(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, attempts := range absentAttempts() {
|
||||
webhook, target := seedAttemptsTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editAttemptsForm(attempts),
|
||||
)
|
||||
require.Equal(t,
|
||||
http.StatusSeeOther, w.Code, w.Body.String(),
|
||||
)
|
||||
|
||||
assert.Equal(t, workingAttempts,
|
||||
storedTarget(t, env, target.ID).MaxAttempts,
|
||||
"an absent max_attempts must leave the stored "+
|
||||
"count alone (%q)", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetAttempts_CreateAndEditAgreeOnEveryCase proves the two
|
||||
// paths cannot disagree, which is what let the create form and the
|
||||
// edit form drift apart in the first place. Every input is submitted
|
||||
// to both and the accept/reject verdicts are compared.
|
||||
func TestTargetAttempts_CreateAndEditAgreeOnEveryCase(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
accepted := []string{
|
||||
"1", "2",
|
||||
strconv.Itoa(handlers.MaxTargetAttemptsForTest),
|
||||
}
|
||||
overCeiling := overCeilingAttempts()
|
||||
belowOne := belowOneAttempts()
|
||||
unparseable := unparseableAttempts()
|
||||
|
||||
cases := make(
|
||||
[]string, 0,
|
||||
len(accepted)+len(overCeiling)+len(belowOne)+
|
||||
len(unparseable),
|
||||
)
|
||||
cases = append(cases, accepted...)
|
||||
cases = append(cases, overCeiling...)
|
||||
cases = append(cases, belowOne...)
|
||||
cases = append(cases, unparseable...)
|
||||
|
||||
for _, attempts := range cases {
|
||||
_, createCode, _ := createWithAttempts(t, env, attempts)
|
||||
|
||||
webhook, target := seedAttemptsTarget(t, env)
|
||||
editCode := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editAttemptsForm(attempts),
|
||||
).Code
|
||||
|
||||
assert.Equal(t,
|
||||
createCode == http.StatusBadRequest,
|
||||
editCode == http.StatusBadRequest,
|
||||
"create and edit must agree on max_attempts=%q "+
|
||||
"(create %d, edit %d)",
|
||||
attempts, createCode, editCode,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -102,9 +102,10 @@ func (h *Handlers) newTarget(
|
||||
in targetFormInput,
|
||||
) (*database.Target, string, error) {
|
||||
target := &database.Target{
|
||||
WebhookID: webhookID,
|
||||
Type: in.Type,
|
||||
Active: true,
|
||||
WebhookID: webhookID,
|
||||
Type: in.Type,
|
||||
Active: true,
|
||||
MaxAttempts: 1,
|
||||
}
|
||||
|
||||
errMsg, err := h.setTargetFromForm(ctx, target, in)
|
||||
@@ -117,9 +118,10 @@ func (h *Handlers) newTarget(
|
||||
|
||||
// setTargetFromForm validates a target form against the target's type
|
||||
// and, when it accepts it, sets the target's name, configuration and
|
||||
// retry count from it. It returns the message the form shows for
|
||||
// anything it refuses, an unknown type among them, and then leaves the
|
||||
// target unchanged; an error is the server's fault, as for newTarget.
|
||||
// number of delivery attempts from it. It returns the message the form
|
||||
// shows for anything it refuses, an unknown type among them, and then
|
||||
// leaves the target unchanged; an error is the server's fault, as for
|
||||
// newTarget.
|
||||
// The add target form and the target edit form both go through here,
|
||||
// so the two cannot come to disagree about what a target may be.
|
||||
func (h *Handlers) setTargetFromForm(
|
||||
@@ -136,20 +138,20 @@ func (h *Handlers) setTargetFromForm(
|
||||
return errMsg, err
|
||||
}
|
||||
|
||||
// An empty max_retries keeps the target's count: the
|
||||
// fire-and-forget default of 0 for a new target, and the stored
|
||||
// count for an edited one, since the forms for target types that
|
||||
// do not retry have no such field. A value that is filled in but
|
||||
// invalid is refused rather than becoming that count, so a typo
|
||||
// cannot destroy the count a target is delivering with.
|
||||
maxRetries, err := parseMaxRetries(in.MaxRetries, target.MaxRetries)
|
||||
// An empty max_attempts keeps the target's count: the default of
|
||||
// 1 for a new target, and the stored count for an edited one,
|
||||
// since the forms for target types that do not retry have no such
|
||||
// field. A value that is filled in but invalid is refused rather
|
||||
// than becoming that count, so a typo cannot destroy the count a
|
||||
// target is delivering with.
|
||||
maxAttempts, err := parseMaxAttempts(in.MaxAttempts, target.MaxAttempts)
|
||||
if err != nil {
|
||||
return "Invalid delivery attempts: " + retriesErrorMessage(err), nil
|
||||
return "Invalid delivery attempts: " + attemptsErrorMessage(err), nil
|
||||
}
|
||||
|
||||
target.Name = in.Name
|
||||
target.Config = configJSON
|
||||
target.MaxRetries = maxRetries
|
||||
target.MaxAttempts = maxAttempts
|
||||
|
||||
return "", nil
|
||||
}
|
||||
@@ -176,8 +178,8 @@ type targetFormInput struct {
|
||||
// ForwardQuery is an HTTP target's checkbox that passes each
|
||||
// event's query string on to it.
|
||||
ForwardQuery bool
|
||||
// MaxRetries is an HTTP or Slack target's max_retries.
|
||||
MaxRetries string
|
||||
// MaxAttempts is an HTTP or Slack target's max_attempts.
|
||||
MaxAttempts string
|
||||
// Expiry is a database (archive) target's row expiry.
|
||||
Expiry string
|
||||
// Rotation is a database (archive) target's rotation.
|
||||
@@ -204,7 +206,7 @@ func targetFormInputFrom(r *http.Request) targetFormInput {
|
||||
Headers: r.PostFormValue("headers"),
|
||||
Timeout: r.PostFormValue("timeout"),
|
||||
ForwardQuery: r.PostFormValue("forward_query") != "",
|
||||
MaxRetries: r.PostFormValue("max_retries"),
|
||||
MaxAttempts: r.PostFormValue("max_attempts"),
|
||||
Expiry: r.PostFormValue("expiry"),
|
||||
Rotation: r.PostFormValue("rotation"),
|
||||
}
|
||||
|
||||
@@ -196,7 +196,7 @@ func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType(
|
||||
|
||||
w, _ := postTargetCreate(
|
||||
t, env, webhook.ID,
|
||||
"name=leaky&type=slack&max_retries=9&expiry=30d&url="+
|
||||
"name=leaky&type=slack&max_attempts=9&expiry=30d&url="+
|
||||
url.QueryEscape(targetSecretURL),
|
||||
url.Values{},
|
||||
)
|
||||
|
||||
@@ -21,24 +21,24 @@ func TestHandleTargetCreate_EveryType(t *testing.T) {
|
||||
|
||||
// fields is the rest of each submission, as a query string.
|
||||
cases := []struct {
|
||||
targetType database.TargetType
|
||||
fields string
|
||||
wantConfig string
|
||||
wantRetries int
|
||||
targetType database.TargetType
|
||||
fields string
|
||||
wantConfig string
|
||||
wantAttempts int
|
||||
}{
|
||||
{
|
||||
database.TargetTypeHTTP, "timeout=12&max_retries=3",
|
||||
database.TargetTypeHTTP, "timeout=12&max_attempts=3",
|
||||
`{"url":"` + editOriginalURL + `","timeout":12}`, 3,
|
||||
},
|
||||
{
|
||||
database.TargetTypeSlack, "max_retries=4",
|
||||
database.TargetTypeSlack, "max_attempts=4",
|
||||
`{"webhookUrl":"` + editOriginalURL + `"}`, 4,
|
||||
},
|
||||
{
|
||||
database.TargetTypeDatabase, "expiry=720h",
|
||||
`{"expiry":"720h"}`, 0,
|
||||
`{"expiry":"720h"}`, 1,
|
||||
},
|
||||
{database.TargetTypeLog, "", "", 0},
|
||||
{database.TargetTypeLog, "", "", 1},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
@@ -62,7 +62,7 @@ func TestHandleTargetCreate_EveryType(t *testing.T) {
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
assert.Equal(t, tc.targetType, targets[0].Type)
|
||||
assert.Equal(t, tc.wantRetries, targets[0].MaxRetries)
|
||||
assert.Equal(t, tc.wantAttempts, targets[0].MaxAttempts)
|
||||
|
||||
if tc.wantConfig == "" {
|
||||
assert.Empty(t, targets[0].Config)
|
||||
@@ -90,11 +90,11 @@ func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) {
|
||||
{
|
||||
database.TargetTypeHTTP,
|
||||
"name=private&url=" + editBlockedURL +
|
||||
"&timeout=12&max_retries=3",
|
||||
"&timeout=12&max_attempts=3",
|
||||
"Invalid target URL",
|
||||
},
|
||||
{
|
||||
database.TargetTypeSlack, "name=no-url&max_retries=4",
|
||||
database.TargetTypeSlack, "name=no-url&max_attempts=4",
|
||||
"Webhook URL is required for Slack targets",
|
||||
},
|
||||
{
|
||||
@@ -133,8 +133,8 @@ func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) {
|
||||
assert.Contains(t, page, html.EscapeString(tc.reason))
|
||||
|
||||
// Each value comes back in a data attribute of the targets
|
||||
// section named after its field (max_retries as
|
||||
// data-max-retries), except url, which comes back in
|
||||
// section named after its field (max_attempts as
|
||||
// data-max-attempts), except url, which comes back in
|
||||
// data-destination; templates/source_detail.html says why.
|
||||
for field := range typed {
|
||||
attr := "data-" + strings.ReplaceAll(field, "_", "-")
|
||||
|
||||
@@ -82,7 +82,7 @@ func (h *Handlers) HandleTargetEdit() http.HandlerFunc {
|
||||
Headers: cfg.Headers,
|
||||
Timeout: cfg.Timeout,
|
||||
ForwardQuery: cfg.ForwardQuery,
|
||||
MaxRetries: strconv.Itoa(target.MaxRetries),
|
||||
MaxAttempts: strconv.Itoa(target.MaxAttempts),
|
||||
Expiry: cfg.Expiry,
|
||||
Rotation: cfg.Rotation,
|
||||
}
|
||||
|
||||
@@ -117,7 +117,7 @@ func seedHTTPTarget(
|
||||
form.Set("url", editOriginalURL)
|
||||
form.Set("headers", headers)
|
||||
form.Set("timeout", timeout)
|
||||
form.Set("max_retries", "3")
|
||||
form.Set("max_attempts", "3")
|
||||
|
||||
w := serveTarget(
|
||||
env, http.MethodPost,
|
||||
@@ -179,7 +179,7 @@ func editForm(targetURL, headers, timeout string) url.Values {
|
||||
form.Set("url", targetURL)
|
||||
form.Set("headers", headers)
|
||||
form.Set("timeout", timeout)
|
||||
form.Set("max_retries", "5")
|
||||
form.Set("max_attempts", "5")
|
||||
|
||||
return form
|
||||
}
|
||||
@@ -280,9 +280,9 @@ func TestHandleTargetEditSubmit_Saves(t *testing.T) {
|
||||
assertEditClearingRemovesThem(t, env)
|
||||
})
|
||||
|
||||
t.Run("absent max_retries is not zeroed", func(t *testing.T) {
|
||||
t.Run("absent max_attempts is kept", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
assertEditKeepsAbsentMaxRetries(t, env)
|
||||
assertEditKeepsAbsentMaxAttempts(t, env)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -307,7 +307,7 @@ func assertEditChangesDestination(
|
||||
|
||||
reloaded := storedTarget(t, env, target.ID)
|
||||
assert.Equal(t, "edited-name", reloaded.Name)
|
||||
assert.Equal(t, 5, reloaded.MaxRetries)
|
||||
assert.Equal(t, 5, reloaded.MaxAttempts)
|
||||
assert.Equal(
|
||||
t, database.TargetTypeHTTP, reloaded.Type,
|
||||
"the edit form must not change a target's type",
|
||||
@@ -368,25 +368,25 @@ func assertEditClearingRemovesThem(
|
||||
assert.Zero(t, cfg.Timeout)
|
||||
}
|
||||
|
||||
// Retries are offered only by the forms for target types that retry.
|
||||
// An absent field means the form does not edit retries, not that
|
||||
// they should be turned off.
|
||||
func assertEditKeepsAbsentMaxRetries(
|
||||
// Delivery attempts are offered only by the forms for target types
|
||||
// that retry. An absent field means the form does not edit them, not
|
||||
// that retries should be turned off.
|
||||
func assertEditKeepsAbsentMaxAttempts(
|
||||
t *testing.T, env *sourceTestEnv,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
webhook, target := seedHTTPTarget(t, env, "", "")
|
||||
require.Equal(t, 3, target.MaxRetries)
|
||||
require.Equal(t, 3, target.MaxAttempts)
|
||||
|
||||
form := editForm(editOriginalURL, "", "")
|
||||
form.Del("max_retries")
|
||||
form.Del("max_attempts")
|
||||
|
||||
w := submitTargetEdit(env, webhook.ID, target.ID, form)
|
||||
require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
|
||||
|
||||
assert.Equal(
|
||||
t, 3, storedTarget(t, env, target.ID).MaxRetries,
|
||||
t, 3, storedTarget(t, env, target.ID).MaxAttempts,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -659,12 +659,12 @@ func TestHandleTargetEditSubmit_RefusedFormComesBack(t *testing.T) {
|
||||
{
|
||||
database.TargetTypeHTTP,
|
||||
"name=edited&url=" + editBlockedURL +
|
||||
"&headers=X-Edited:+kept&timeout=12&max_retries=3",
|
||||
"&headers=X-Edited:+kept&timeout=12&max_attempts=3",
|
||||
"Invalid target URL",
|
||||
},
|
||||
{
|
||||
database.TargetTypeSlack,
|
||||
"name=edited&url=" + editOriginalURL + "&max_retries=25",
|
||||
"name=edited&url=" + editOriginalURL + "&max_attempts=25",
|
||||
"Invalid delivery attempts",
|
||||
},
|
||||
{
|
||||
|
||||
@@ -1,83 +0,0 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"strconv"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// maxTargetRetries bounds a target's max_retries.
|
||||
//
|
||||
// Both target forms already declare max="20" on the input, so this
|
||||
// enforces server-side what the UI has always advertised rather than
|
||||
// introducing a new limit.
|
||||
//
|
||||
// The number is not cosmetic. Every attempt writes a delivery_results
|
||||
// row that the event log then loads and renders, and the engine backs
|
||||
// off by 2^(n-1) seconds, so attempt 20 is already about six days
|
||||
// after the first. A value beyond this buys no additional durability
|
||||
// and only costs rows.
|
||||
const maxTargetRetries = 20
|
||||
|
||||
// Errors returned when a max_retries form value cannot be turned into
|
||||
// a retry count.
|
||||
var (
|
||||
// errRetriesInvalid signals a max_retries form value that is not
|
||||
// a non-negative whole number.
|
||||
errRetriesInvalid = errors.New(
|
||||
"must be a whole number",
|
||||
)
|
||||
|
||||
// errRetriesTooLarge signals a max_retries form value that is a
|
||||
// whole number but above maxTargetRetries. It is distinguished
|
||||
// from errRetriesInvalid so the message can name the ceiling
|
||||
// instead of implying the input was not a number.
|
||||
errRetriesTooLarge = errors.New("out of range")
|
||||
)
|
||||
|
||||
// parseMaxRetries interprets a max_retries form value.
|
||||
//
|
||||
// An ABSENT value — the field empty or not submitted — yields
|
||||
// fallback, which lets the create path apply its default and the edit
|
||||
// path leave the stored value alone. A value that is SET BUT INVALID
|
||||
// is an error: unparseable, negative, or above maxTargetRetries.
|
||||
//
|
||||
// The distinction is the whole point of this function. max_retries=0
|
||||
// means fire-and-forget, so returning 0 for input the operator typed
|
||||
// but that did not parse silently disables retries on a
|
||||
// store-and-forward proxy — and on the edit path it destroys a
|
||||
// working retry configuration over a typo. A default answers a
|
||||
// question that was not asked; it never answers one that was asked
|
||||
// badly.
|
||||
func parseMaxRetries(raw string, fallback int) (int, error) {
|
||||
raw = strings.TrimSpace(raw)
|
||||
if raw == "" {
|
||||
return fallback, nil
|
||||
}
|
||||
|
||||
v, err := strconv.Atoi(raw)
|
||||
if err != nil || v < 0 {
|
||||
return 0, errRetriesInvalid
|
||||
}
|
||||
|
||||
if v > maxTargetRetries {
|
||||
return 0, errRetriesTooLarge
|
||||
}
|
||||
|
||||
return v, nil
|
||||
}
|
||||
|
||||
// retriesErrorMessage returns the message the create and edit forms
|
||||
// show for a rejected max_retries value. Any error other than
|
||||
// errRetriesTooLarge falls back to the generic wording, so an
|
||||
// unrecognised parse failure still produces a sensible 400.
|
||||
func retriesErrorMessage(err error) string {
|
||||
if errors.Is(err, errRetriesTooLarge) {
|
||||
return errRetriesTooLarge.Error() +
|
||||
": at most " + strconv.Itoa(maxTargetRetries) +
|
||||
" attempts"
|
||||
}
|
||||
|
||||
return errRetriesInvalid.Error() +
|
||||
", or 0 for fire-and-forget"
|
||||
}
|
||||
@@ -1,385 +0,0 @@
|
||||
package handlers_test
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
)
|
||||
|
||||
// retriesTargetURL is the destination the retry-validation targets
|
||||
// point at. It is a literal public address rather than a hostname so
|
||||
// the SSRF check resolves nothing and a sandbox without DNS cannot
|
||||
// make these cases pass or fail for the wrong reason.
|
||||
const retriesTargetURL = "https://93.184.216.34/hooks/retries"
|
||||
|
||||
const (
|
||||
// wayAboveCeiling is the typo'd-extra-zero case from the report.
|
||||
wayAboveCeiling = "999999999"
|
||||
|
||||
// notANumber is the plainest garbage an operator can type, and
|
||||
// the value the report submitted on the edit form.
|
||||
notANumber = "abc"
|
||||
|
||||
// workingRetries is the retry count a seeded target is already
|
||||
// delivering with, which a rejected submission must not disturb.
|
||||
workingRetries = 2
|
||||
)
|
||||
|
||||
// aboveCeiling is the smallest rejected whole number.
|
||||
func aboveCeiling() string {
|
||||
return strconv.Itoa(handlers.MaxTargetRetriesForTest + 1)
|
||||
}
|
||||
|
||||
// overCeilingRetries is whole-number input past the limit, which is
|
||||
// rejected with the limit named.
|
||||
func overCeilingRetries() []string {
|
||||
return []string{aboveCeiling(), wayAboveCeiling}
|
||||
}
|
||||
|
||||
// unparseableRetries is input an operator can type into the field
|
||||
// that is not a retry count. Each must be REJECTED: silently reading
|
||||
// any of them as 0 turns a store-and-forward proxy into
|
||||
// fire-and-forget without saying so.
|
||||
//
|
||||
// The twenty-digit case is here because it parses as digits but
|
||||
// overflows int, which is the one failure the field's own min/max
|
||||
// attributes cannot describe.
|
||||
func unparseableRetries() []string {
|
||||
return []string{
|
||||
notANumber,
|
||||
"2.7",
|
||||
"-5",
|
||||
"12345678901234567890",
|
||||
"1e3",
|
||||
}
|
||||
}
|
||||
|
||||
// createRetriesForm is a complete, otherwise-valid HTTP target
|
||||
// creation, so the only thing any case below varies is max_retries.
|
||||
func createRetriesForm(retries string) url.Values {
|
||||
form := url.Values{}
|
||||
form.Set("name", "retries-target")
|
||||
form.Set("type", string(database.TargetTypeHTTP))
|
||||
form.Set("url", retriesTargetURL)
|
||||
|
||||
if retries != absentField {
|
||||
form.Set("max_retries", retries)
|
||||
}
|
||||
|
||||
return form
|
||||
}
|
||||
|
||||
// absentField marks a field the form does not submit at all, which is
|
||||
// the case that legitimately takes a default and must stay distinct
|
||||
// from a field submitted with garbage in it.
|
||||
const absentField = "\x00absent"
|
||||
|
||||
// absentRetries is every way of saying "the operator did not set
|
||||
// this", each of which takes the default rather than a 400. Blank and
|
||||
// whitespace-only count as absent here because they do in the timeout
|
||||
// and retention controls on the same forms; a rule the fields do not
|
||||
// share would be its own surprise.
|
||||
func absentRetries() []string {
|
||||
return []string{absentField, "", " "}
|
||||
}
|
||||
|
||||
// createWithRetries posts the target create form for a fresh webhook
|
||||
// and returns the webhook and the response.
|
||||
func createWithRetries(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
retries string,
|
||||
) (database.Webhook, int, string) {
|
||||
t.Helper()
|
||||
|
||||
webhook := seedWebhookWithRetention(t, env.db, 30)
|
||||
|
||||
w := serveTarget(
|
||||
env, http.MethodPost,
|
||||
"/hook/"+webhook.ID+"/targets",
|
||||
createRetriesForm(retries),
|
||||
)
|
||||
|
||||
return webhook, w.Code, w.Body.String()
|
||||
}
|
||||
|
||||
// TestTargetCreate_RetriesAboveCeilingRejected proves the create form
|
||||
// enforces a ceiling at all, and that the 400 names it — a rejection
|
||||
// that does not say what the limit is leaves the operator guessing.
|
||||
func TestTargetCreate_RetriesAboveCeilingRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest)
|
||||
|
||||
for _, retries := range overCeilingRetries() {
|
||||
webhook, code, body := createWithRetries(t, env, retries)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, code,
|
||||
"max_retries=%s should be rejected", retries)
|
||||
assert.Contains(t, body, ceiling,
|
||||
"the rejection for %s should name the ceiling",
|
||||
retries)
|
||||
assert.Empty(t,
|
||||
targetsForWebhook(t, env.db, webhook.ID),
|
||||
"no target should be created for %s", retries)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_UnparseableRetriesRejected is the core of the
|
||||
// defect: each of these was accepted with HTTP 200 and stored as 0.
|
||||
func TestTargetCreate_UnparseableRetriesRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, retries := range unparseableRetries() {
|
||||
webhook, code, body := createWithRetries(t, env, retries)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, code,
|
||||
"max_retries=%q should be rejected, not coerced",
|
||||
retries)
|
||||
assert.Contains(t, body, "whole number",
|
||||
"the rejection for %q should say why", retries)
|
||||
assert.Empty(t,
|
||||
targetsForWebhook(t, env.db, webhook.ID),
|
||||
"no target should be created for %q", retries)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_ValidRetriesStored covers the accepting half,
|
||||
// including the ceiling itself: a bound that rejects its own limit
|
||||
// would make the advertised maximum unreachable.
|
||||
func TestTargetCreate_ValidRetriesStored(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, want := range []int{0, 3, handlers.MaxTargetRetriesForTest} {
|
||||
webhook, code, body := createWithRetries(
|
||||
t, env, strconv.Itoa(want),
|
||||
)
|
||||
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
assert.Equal(t, want, targets[0].MaxRetries)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetCreate_AbsentRetriesTakesDefault keeps the two cases
|
||||
// distinct. An omitted field is not an operator asking for something
|
||||
// invalid, so it still gets the fire-and-forget default rather than a
|
||||
// 400 — otherwise the fix above would make the form unusable.
|
||||
func TestTargetCreate_AbsentRetriesTakesDefault(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, retries := range absentRetries() {
|
||||
webhook, code, body := createWithRetries(t, env, retries)
|
||||
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
assert.Equal(t, 0, targets[0].MaxRetries,
|
||||
"an absent max_retries should take the default")
|
||||
}
|
||||
}
|
||||
|
||||
// seedRetriesTarget creates an HTTP target already delivering with
|
||||
// workingRetries retries, through the real create handler.
|
||||
func seedRetriesTarget(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
) (database.Webhook, database.Target) {
|
||||
t.Helper()
|
||||
|
||||
webhook, code, body := createWithRetries(
|
||||
t, env, strconv.Itoa(workingRetries),
|
||||
)
|
||||
require.Equal(t, http.StatusSeeOther, code, body)
|
||||
|
||||
targets := targetsForWebhook(t, env.db, webhook.ID)
|
||||
require.Len(t, targets, 1)
|
||||
require.Equal(t, workingRetries, targets[0].MaxRetries)
|
||||
|
||||
return webhook, targets[0]
|
||||
}
|
||||
|
||||
// editRetriesForm is a complete edit submission that changes the
|
||||
// target's name as well, so a rejected submission can be shown to
|
||||
// have written nothing at all rather than merely to have left
|
||||
// max_retries alone.
|
||||
func editRetriesForm(retries string) url.Values {
|
||||
form := url.Values{}
|
||||
form.Set("name", "renamed-by-edit")
|
||||
form.Set("url", retriesTargetURL)
|
||||
|
||||
if retries != absentField {
|
||||
form.Set("max_retries", retries)
|
||||
}
|
||||
|
||||
return form
|
||||
}
|
||||
|
||||
// assertEditRejectedAndUnchanged submits an edit expected to fail and
|
||||
// checks both halves of the requirement: the 400 explains itself, and
|
||||
// the target it was submitted against is untouched.
|
||||
func assertEditRejectedAndUnchanged(
|
||||
t *testing.T,
|
||||
env *sourceTestEnv,
|
||||
retries, wantReason string,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
webhook, target := seedRetriesTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID, editRetriesForm(retries),
|
||||
)
|
||||
|
||||
assert.Equal(t, http.StatusBadRequest, w.Code,
|
||||
"max_retries=%q should be rejected on edit", retries)
|
||||
assert.Contains(t, w.Body.String(), wantReason,
|
||||
"the rejection for %q should say why", retries)
|
||||
|
||||
stored := storedTarget(t, env, target.ID)
|
||||
assert.Equal(t, workingRetries, stored.MaxRetries,
|
||||
"a rejected edit must not destroy the working retry "+
|
||||
"count with %q", retries)
|
||||
assert.Equal(t, "retries-target", stored.Name,
|
||||
"a rejected edit must write nothing at all")
|
||||
}
|
||||
|
||||
// TestTargetEdit_UnparseableRetriesRejected is the damaging half of
|
||||
// the defect. A target delivering with two retries, re-saved with a
|
||||
// typo in the field, returned 200 and was left with retries disabled
|
||||
// and nothing said.
|
||||
func TestTargetEdit_UnparseableRetriesRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, retries := range unparseableRetries() {
|
||||
assertEditRejectedAndUnchanged(
|
||||
t, env, retries, "whole number",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_RetriesAboveCeilingRejected proves the ceiling
|
||||
// applies to the edit path too, naming itself, so the two paths
|
||||
// cannot disagree about what is storable.
|
||||
func TestTargetEdit_RetriesAboveCeilingRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest)
|
||||
|
||||
for _, retries := range overCeilingRetries() {
|
||||
assertEditRejectedAndUnchanged(t, env, retries, ceiling)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_ValidRetriesStored covers the accepting half of the
|
||||
// edit path, so the ceiling cannot be enforced by simply refusing
|
||||
// every submission.
|
||||
func TestTargetEdit_ValidRetriesStored(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, want := range []int{0, 9, handlers.MaxTargetRetriesForTest} {
|
||||
webhook, target := seedRetriesTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editRetriesForm(strconv.Itoa(want)),
|
||||
)
|
||||
require.Equal(t,
|
||||
http.StatusSeeOther, w.Code, w.Body.String(),
|
||||
)
|
||||
|
||||
assert.Equal(t, want,
|
||||
storedTarget(t, env, target.ID).MaxRetries)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetEdit_AbsentRetriesLeavesStoredValue is the edit path's
|
||||
// absent-versus-invalid case. Retries are only offered by the forms
|
||||
// for types that retry, so a submission without the field must leave
|
||||
// the stored count alone rather than be rejected or zeroed.
|
||||
func TestTargetEdit_AbsentRetriesLeavesStoredValue(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
for _, retries := range absentRetries() {
|
||||
webhook, target := seedRetriesTarget(t, env)
|
||||
|
||||
w := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editRetriesForm(retries),
|
||||
)
|
||||
require.Equal(t,
|
||||
http.StatusSeeOther, w.Code, w.Body.String(),
|
||||
)
|
||||
|
||||
assert.Equal(t, workingRetries,
|
||||
storedTarget(t, env, target.ID).MaxRetries,
|
||||
"an absent max_retries must leave the stored "+
|
||||
"count alone (%q)", retries)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTargetRetries_CreateAndEditAgreeOnEveryCase proves the two
|
||||
// paths cannot disagree, which is what let the create form and the
|
||||
// edit form drift apart in the first place. Every input is submitted
|
||||
// to both and the accept/reject verdicts are compared.
|
||||
func TestTargetRetries_CreateAndEditAgreeOnEveryCase(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
|
||||
accepted := []string{
|
||||
"0", "1",
|
||||
strconv.Itoa(handlers.MaxTargetRetriesForTest),
|
||||
}
|
||||
overCeiling := overCeilingRetries()
|
||||
unparseable := unparseableRetries()
|
||||
|
||||
cases := make(
|
||||
[]string, 0,
|
||||
len(accepted)+len(overCeiling)+len(unparseable),
|
||||
)
|
||||
cases = append(cases, accepted...)
|
||||
cases = append(cases, overCeiling...)
|
||||
cases = append(cases, unparseable...)
|
||||
|
||||
for _, retries := range cases {
|
||||
_, createCode, _ := createWithRetries(t, env, retries)
|
||||
|
||||
webhook, target := seedRetriesTarget(t, env)
|
||||
editCode := submitTargetEdit(
|
||||
env, webhook.ID, target.ID,
|
||||
editRetriesForm(retries),
|
||||
).Code
|
||||
|
||||
assert.Equal(t,
|
||||
createCode == http.StatusBadRequest,
|
||||
editCode == http.StatusBadRequest,
|
||||
"create and edit must agree on max_retries=%q "+
|
||||
"(create %d, edit %d)",
|
||||
retries, createCode, editCode,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -59,7 +59,7 @@ func TestHandleTargetToggle_DoesNotUndoAnEdit(t *testing.T) {
|
||||
stored := storedTarget(t, env, tgt.ID)
|
||||
assert.False(t, stored.Active)
|
||||
assert.Equal(t, "edited-name", stored.Name)
|
||||
assert.Equal(t, 5, stored.MaxRetries)
|
||||
assert.Equal(t, 5, stored.MaxAttempts)
|
||||
assert.Equal(
|
||||
t, editReplacedURL, storedHTTPConfig(t, env, tgt.ID).URL,
|
||||
)
|
||||
|
||||
@@ -348,21 +348,19 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// maxRetriesHelp is the wording both target forms must carry. The
|
||||
// delivery core makes max_retries attempts in total, not that many
|
||||
// maxAttemptsHelp is the wording both target forms must carry. The
|
||||
// delivery core makes max_attempts attempts in total, not that many
|
||||
// retries on top of a first try (a fresh delivery starts at attempt 1
|
||||
// and target_http gives up once the attempt number reaches
|
||||
// max_retries), and 0 is special-cased to a single fire-and-forget
|
||||
// attempt with no circuit breaker.
|
||||
const maxRetriesHelp = "How many times each delivery is attempted in " +
|
||||
"all, the first attempt included. 0 means a single attempt with no " +
|
||||
"retries and no circuit breaker."
|
||||
// max_attempts).
|
||||
const maxAttemptsHelp = "How many times each delivery is attempted in " +
|
||||
"all, the first attempt included."
|
||||
|
||||
// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries
|
||||
// TestTargetFormMaxAttemptsCopyMatchesBehaviour pins the max_attempts
|
||||
// label, "Delivery attempts", and help text on both the create form
|
||||
// (the add-target form on the webhook detail page) and the edit form,
|
||||
// so the copy cannot drift back to calling the number a retry count.
|
||||
func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
|
||||
func TestTargetFormMaxAttemptsCopyMatchesBehaviour(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var h *handlers.Handlers
|
||||
@@ -394,11 +392,11 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
|
||||
|
||||
assert.Contains(t, createBody, "Delivery attempts:</label>")
|
||||
assert.Contains(
|
||||
t, createBody, maxRetriesHelp,
|
||||
"the add-target form must explain max_retries as total attempts",
|
||||
t, createBody, maxAttemptsHelp,
|
||||
"the add-target form must explain max_attempts as total attempts",
|
||||
)
|
||||
|
||||
// A slack target exercises the same max_retries field while needing
|
||||
// A slack target exercises the same max_attempts field while needing
|
||||
// only a URL from the edit template, so the test data stays
|
||||
// minimal. The Target and TargetForm keys mirror the field names
|
||||
// the template reads off the handler's values.
|
||||
@@ -412,8 +410,8 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
|
||||
"Active": true,
|
||||
},
|
||||
"TargetForm": map[string]any{
|
||||
"URL": "https://hooks.slack.com/services/x",
|
||||
"MaxRetries": "3",
|
||||
"URL": "https://hooks.slack.com/services/x",
|
||||
"MaxAttempts": "3",
|
||||
},
|
||||
dataKeyError: "",
|
||||
},
|
||||
@@ -421,8 +419,8 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
|
||||
|
||||
assert.Contains(t, editBody, `class="label">Delivery attempts</label>`)
|
||||
assert.Contains(
|
||||
t, editBody, maxRetriesHelp,
|
||||
"the target edit form must explain max_retries as total attempts",
|
||||
t, editBody, maxAttemptsHelp,
|
||||
"the target edit form must explain max_attempts as total attempts",
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -442,7 +442,7 @@ func buildDeliveryTasks(
|
||||
TargetName: targets[i].Name,
|
||||
TargetType: targets[i].Type,
|
||||
TargetConfig: targets[i].Config,
|
||||
MaxRetries: targets[i].MaxRetries,
|
||||
MaxAttempts: targets[i].MaxAttempts,
|
||||
Method: event.Method,
|
||||
RawQuery: event.RawQuery,
|
||||
Headers: event.Headers,
|
||||
|
||||
@@ -354,11 +354,11 @@ func checkAddEachTargetType(ctx context.Context, t *testing.T, url string) {
|
||||
}{
|
||||
{
|
||||
"http", "http",
|
||||
"csrf_token name type url headers timeout max_retries",
|
||||
"csrf_token name type url headers timeout max_attempts",
|
||||
map[string]string{"url": publicTargetURL},
|
||||
},
|
||||
{
|
||||
"slack", "slack", "csrf_token name type url max_retries",
|
||||
"slack", "slack", "csrf_token name type url max_attempts",
|
||||
map[string]string{"url": publicTargetURL},
|
||||
},
|
||||
{
|
||||
@@ -630,11 +630,11 @@ func checkRefusedEdits(
|
||||
values map[string]string
|
||||
}{
|
||||
{page + "/targets/" + targetID + "/edit", map[string]string{
|
||||
"#name": "edited-target",
|
||||
"#url": "http://127.0.0.1/hook",
|
||||
"#headers": "X-Edited: kept",
|
||||
"#timeout": "12",
|
||||
"#max_retries": "3",
|
||||
"#name": "edited-target",
|
||||
"#url": "http://127.0.0.1/hook",
|
||||
"#headers": "X-Edited: kept",
|
||||
"#timeout": "12",
|
||||
"#max_attempts": "3",
|
||||
}},
|
||||
{page + "/edit", map[string]string{
|
||||
"#name": "edited-webhook",
|
||||
|
||||
+3
-3
@@ -139,7 +139,7 @@ document.addEventListener("alpine:init", function () {
|
||||
headers: "",
|
||||
timeout: "",
|
||||
forwardQuery: false,
|
||||
maxRetries: "",
|
||||
maxAttempts: "",
|
||||
expiry: "",
|
||||
rotation: "",
|
||||
init() {
|
||||
@@ -153,7 +153,7 @@ document.addEventListener("alpine:init", function () {
|
||||
this.timeout = refused.timeout;
|
||||
this.forwardQuery =
|
||||
this.$root.hasAttribute("data-forward-query");
|
||||
this.maxRetries = refused.maxRetries;
|
||||
this.maxAttempts = refused.maxAttempts;
|
||||
this.expiry = refused.expiry;
|
||||
this.rotation = refused.rotation;
|
||||
},
|
||||
@@ -173,7 +173,7 @@ document.addEventListener("alpine:init", function () {
|
||||
this.headers = "";
|
||||
this.timeout = "";
|
||||
this.forwardQuery = false;
|
||||
this.maxRetries = "";
|
||||
this.maxAttempts = "";
|
||||
this.expiry = "";
|
||||
this.rotation = "";
|
||||
this.$refs.form.reset();
|
||||
|
||||
@@ -136,7 +136,7 @@
|
||||
data-headers="{{.TargetForm.Headers}}"
|
||||
data-timeout="{{.TargetForm.Timeout}}"
|
||||
{{if .TargetForm.ForwardQuery}}data-forward-query{{end}}
|
||||
data-max-retries="{{.TargetForm.MaxRetries}}"
|
||||
data-max-attempts="{{.TargetForm.MaxAttempts}}"
|
||||
data-expiry="{{.TargetForm.Expiry}}"
|
||||
data-rotation="{{.TargetForm.Rotation}}">
|
||||
<div class="p-4 border-b border-gray-200 flex justify-between items-center">
|
||||
@@ -194,9 +194,9 @@
|
||||
<div>
|
||||
<div class="flex gap-2 items-center">
|
||||
<label class="text-sm text-gray-700">Delivery attempts:</label>
|
||||
<input type="number" name="max_retries" :value="maxRetries" placeholder="0" min="0" max="20" class="input text-sm w-24">
|
||||
<input type="number" name="max_attempts" :value="maxAttempts" placeholder="1" min="1" max="20" class="input text-sm w-24">
|
||||
</div>
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.</p>
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included.</p>
|
||||
</div>
|
||||
</div>
|
||||
</template>
|
||||
@@ -210,9 +210,9 @@
|
||||
<div>
|
||||
<div class="flex gap-2 items-center">
|
||||
<label class="text-sm text-gray-700">Delivery attempts:</label>
|
||||
<input type="number" name="max_retries" :value="maxRetries" placeholder="0" min="0" max="20" class="input text-sm w-24">
|
||||
<input type="number" name="max_attempts" :value="maxAttempts" placeholder="1" min="1" max="20" class="input text-sm w-24">
|
||||
</div>
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.</p>
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included.</p>
|
||||
</div>
|
||||
</div>
|
||||
</template>
|
||||
|
||||
@@ -89,9 +89,9 @@
|
||||
|
||||
{{if or (eq .Target.Type "http") (eq .Target.Type "slack")}}
|
||||
<div class="form-group">
|
||||
<label for="max_retries" class="label">Delivery attempts</label>
|
||||
<input type="number" id="max_retries" name="max_retries" value="{{.TargetForm.MaxRetries}}" min="0" max="20" class="input">
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.</p>
|
||||
<label for="max_attempts" class="label">Delivery attempts</label>
|
||||
<input type="number" id="max_attempts" name="max_attempts" value="{{.TargetForm.MaxAttempts}}" min="1" max="20" class="input">
|
||||
<p class="text-xs text-gray-500 mt-1">How many times each delivery is attempted in all, the first attempt included.</p>
|
||||
</div>
|
||||
{{end}}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user