Compare commits
1 Commits
fec6876c42
...
ec92992450
| Author | SHA1 | Date | |
|---|---|---|---|
| ec92992450 |
8
Makefile
8
Makefile
@@ -8,14 +8,6 @@
|
|||||||
# unavailable, which is how the Dockerfile passes its build arg in.
|
# unavailable, which is how the Dockerfile passes its build arg in.
|
||||||
VERSION ?= $(shell script/version)
|
VERSION ?= $(shell script/version)
|
||||||
|
|
||||||
# An empty override (`make build VERSION=`, or a `--build-arg VERSION=`
|
|
||||||
# landing on the Dockerfile's `make build VERSION="$VERSION"`) means unset,
|
|
||||||
# exactly as it does in script/version -- stamping "" would leave the binary
|
|
||||||
# reporting no version and the footer back on its "dev" fallback. `override`
|
|
||||||
# is required: a plain assignment loses to the command-line definition it
|
|
||||||
# exists to correct.
|
|
||||||
override VERSION := $(or $(strip $(VERSION)),$(shell script/version))
|
|
||||||
|
|
||||||
# Extra linker flags for the build target. The static relink in the
|
# Extra linker flags for the build target. The static relink in the
|
||||||
# Dockerfile adds -extldflags here rather than passing its own -ldflags,
|
# Dockerfile adds -extldflags here rather than passing its own -ldflags,
|
||||||
# so composing flags cannot drop the version stamp.
|
# so composing flags cannot drop the version stamp.
|
||||||
|
|||||||
38
README.md
38
README.md
@@ -719,9 +719,7 @@ reports `unknown` is a build nobody told what it was; it is not a
|
|||||||
failure, but it cannot be traced back to a commit.
|
failure, but it cannot be traced back to a commit.
|
||||||
|
|
||||||
`make version` prints what the current checkout would stamp, and
|
`make version` prints what the current checkout would stamp, and
|
||||||
`make build VERSION=v1.2.3` overrides it. An empty override — from
|
`make build VERSION=v1.2.3` overrides it.
|
||||||
`make build VERSION=` or from `--build-arg VERSION=` — means unset
|
|
||||||
rather than `""`, and resolves the way an absent one does.
|
|
||||||
|
|
||||||
Nothing that varies between two builds of the same commit is stamped —
|
Nothing that varies between two builds of the same commit is stamped —
|
||||||
no timestamp, no hostname, no builder identity — so two builds of one
|
no timestamp, no hostname, no builder identity — so two builds of one
|
||||||
@@ -1723,40 +1721,6 @@ gauge. The outcome counters move only after the status change has been
|
|||||||
written, so a transition the database rejected is never reported as an
|
written, so a transition the database rejected is never reported as an
|
||||||
outcome that happened.
|
outcome that happened.
|
||||||
|
|
||||||
#### Inbound HTTP metrics
|
|
||||||
|
|
||||||
The middleware records three more on the same registry:
|
|
||||||
|
|
||||||
| Metric | Type | Labels |
|
|
||||||
| ------ | ---- | ------ |
|
|
||||||
| `http_request_duration_seconds` | histogram | `service`, `handler`, `method`, `code` |
|
|
||||||
| `http_response_size_bytes` | histogram | `service`, `handler`, `method`, `code` |
|
|
||||||
| `http_requests_inflight` | gauge | `service`, `handler` |
|
|
||||||
|
|
||||||
Two of those labels are written once per request from bytes the client
|
|
||||||
chose, so both are bounded to something this service registers:
|
|
||||||
|
|
||||||
- `handler` is the chi route pattern — `/webhook/{uuid}`, never the
|
|
||||||
concrete path. A request matching no route carries `(unmatched)`,
|
|
||||||
and no entrypoint UUID ever reaches a label.
|
|
||||||
- `method` is the request method when the router can route it, and
|
|
||||||
`(unmatched)` otherwise. `net/http` accepts any RFC 9110 token as a
|
|
||||||
method, so the raw value bounds the label at nothing; the nine chi
|
|
||||||
matches routes for stay distinguishable, and a token that could only
|
|
||||||
ever have produced a 405 does not get a series of its own.
|
|
||||||
|
|
||||||
The other two are not request-controlled: `code` is the status one of
|
|
||||||
this service's own handlers wrote, and `service` is a fixed empty
|
|
||||||
string.
|
|
||||||
|
|
||||||
`http_requests_inflight` is deliberately aggregate — its `handler` is
|
|
||||||
always `(all)`, one series counting the requests in flight across the
|
|
||||||
whole service. The gauge is incremented before routing and decremented
|
|
||||||
after the handler returns, and the route pattern exists only between
|
|
||||||
those two moments, so labelling it by pattern would increment one
|
|
||||||
series and decrement another, leaving every pattern permanently off by
|
|
||||||
the number of requests it served.
|
|
||||||
|
|
||||||
### Rate Limiting
|
### Rate Limiting
|
||||||
|
|
||||||
Global blanket rate limiting middleware (e.g., a per-IP throttle shared
|
Global blanket rate limiting middleware (e.g., a per-IP throttle shared
|
||||||
|
|||||||
@@ -23,13 +23,6 @@ type ConfigField struct {
|
|||||||
Value string
|
Value string
|
||||||
}
|
}
|
||||||
|
|
||||||
// deletedNameSuffix marks the name of a target that no longer
|
|
||||||
// exists. Deletes are soft and delivery history outlives the
|
|
||||||
// target, so the event log shows names of targets that are gone;
|
|
||||||
// an operator reading one needs to know it cannot be delivered
|
|
||||||
// to, replayed to, or configured.
|
|
||||||
const deletedNameSuffix = " (deleted)"
|
|
||||||
|
|
||||||
// TargetView is the display-safe projection of a target for
|
// TargetView is the display-safe projection of a target for
|
||||||
// the UI. It deliberately has no raw configuration field, so
|
// the UI. It deliberately has no raw configuration field, so
|
||||||
// no template — present or future — can render the stored
|
// no template — present or future — can render the stored
|
||||||
@@ -37,37 +30,14 @@ const deletedNameSuffix = " (deleted)"
|
|||||||
type TargetView struct {
|
type TargetView struct {
|
||||||
ID string
|
ID string
|
||||||
Name string
|
Name string
|
||||||
|
|
||||||
// Deleted reports that this target's row is soft deleted.
|
|
||||||
// Only views built for historical display carry it set:
|
|
||||||
// every other projection is of a live row.
|
|
||||||
Deleted bool
|
|
||||||
|
|
||||||
Type database.TargetType
|
Type database.TargetType
|
||||||
Active bool
|
Active bool
|
||||||
Config []ConfigField
|
Config []ConfigField
|
||||||
}
|
}
|
||||||
|
|
||||||
// DisplayName is the name to render, marked when the target has
|
|
||||||
// been deleted. Templates showing a name against historical data
|
|
||||||
// must use it rather than Name, which stays the stored name.
|
|
||||||
func (v TargetView) DisplayName() string {
|
|
||||||
if v.Deleted {
|
|
||||||
return v.Name + deletedNameSuffix
|
|
||||||
}
|
|
||||||
|
|
||||||
return v.Name
|
|
||||||
}
|
|
||||||
|
|
||||||
// NewTargetViews projects targets for rendering, replacing
|
// NewTargetViews projects targets for rendering, replacing
|
||||||
// each stored configuration blob with named, display-safe
|
// each stored configuration blob with named, display-safe
|
||||||
// fields.
|
// fields.
|
||||||
//
|
|
||||||
// A soft-deleted row projects exactly as a live one does, minus
|
|
||||||
// the deleted marker on its name: masking is a property of the
|
|
||||||
// projection, not of the row's state, so a deleted target's
|
|
||||||
// credential is as unreachable from a template as a live
|
|
||||||
// target's.
|
|
||||||
func NewTargetViews(
|
func NewTargetViews(
|
||||||
targets []database.Target,
|
targets []database.Target,
|
||||||
) []TargetView {
|
) []TargetView {
|
||||||
@@ -79,7 +49,6 @@ func NewTargetViews(
|
|||||||
views = append(views, TargetView{
|
views = append(views, TargetView{
|
||||||
ID: t.ID,
|
ID: t.ID,
|
||||||
Name: t.Name,
|
Name: t.Name,
|
||||||
Deleted: t.DeletedAt.Valid,
|
|
||||||
Type: t.Type,
|
Type: t.Type,
|
||||||
Active: t.Active,
|
Active: t.Active,
|
||||||
Config: targetConfigFields(t),
|
Config: targetConfigFields(t),
|
||||||
|
|||||||
@@ -2,11 +2,9 @@ package delivery_test
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
"gorm.io/gorm"
|
|
||||||
"sneak.berlin/go/webhooker/internal/database"
|
"sneak.berlin/go/webhooker/internal/database"
|
||||||
"sneak.berlin/go/webhooker/internal/delivery"
|
"sneak.berlin/go/webhooker/internal/delivery"
|
||||||
)
|
)
|
||||||
@@ -19,14 +17,6 @@ const (
|
|||||||
slackWebhookURL = "https://hooks.slack.com" +
|
slackWebhookURL = "https://hooks.slack.com" +
|
||||||
slackSecretPath
|
slackSecretPath
|
||||||
|
|
||||||
// slackMaskedURL is what a Slack webhook URL renders as
|
|
||||||
// once masked: scheme and host, path elided.
|
|
||||||
slackMaskedURL = "https://hooks.slack.com/..."
|
|
||||||
|
|
||||||
// slackTargetName is the target name the Slack projection
|
|
||||||
// tests use.
|
|
||||||
slackTargetName = "slack-target"
|
|
||||||
|
|
||||||
viewExampleOrigin = "https://example.com"
|
viewExampleOrigin = "https://example.com"
|
||||||
viewExampleHook = viewExampleOrigin + "/hook"
|
viewExampleHook = viewExampleOrigin + "/hook"
|
||||||
viewMaskedOrigin = viewExampleOrigin + "/..."
|
viewMaskedOrigin = viewExampleOrigin + "/..."
|
||||||
@@ -43,7 +33,7 @@ func TestMaskedWebhookURL(t *testing.T) {
|
|||||||
}{
|
}{
|
||||||
"slack webhook": {
|
"slack webhook": {
|
||||||
url: slackWebhookURL,
|
url: slackWebhookURL,
|
||||||
want: slackMaskedURL,
|
want: "https://hooks.slack.com/...",
|
||||||
},
|
},
|
||||||
"query string dropped": {
|
"query string dropped": {
|
||||||
url: viewExampleOrigin + "/a?token=secret",
|
url: viewExampleOrigin + "/a?token=secret",
|
||||||
@@ -135,61 +125,23 @@ func viewFor(
|
|||||||
return views[0]
|
return views[0]
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestNewTargetViews_DeletedTarget proves the projection marks
|
func TestNewTargetViews_Slack(t *testing.T) {
|
||||||
// a soft-deleted target's name and masks its configuration by
|
|
||||||
// the same rules a live target's is. Delivery history outlives
|
|
||||||
// the target it names, so this projection is what an operator
|
|
||||||
// reads about a target that no longer exists.
|
|
||||||
func TestNewTargetViews_DeletedTarget(t *testing.T) {
|
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
target := slackTarget()
|
view := viewFor(t, database.Target{
|
||||||
target.DeletedAt = gorm.DeletedAt{
|
Name: "slack-target",
|
||||||
Time: time.Now(),
|
|
||||||
Valid: true,
|
|
||||||
}
|
|
||||||
|
|
||||||
view := viewFor(t, target)
|
|
||||||
|
|
||||||
assert.True(t, view.Deleted)
|
|
||||||
assert.Equal(t, slackTargetName, view.Name)
|
|
||||||
assert.Equal(
|
|
||||||
t, slackTargetName+" (deleted)", view.DisplayName(),
|
|
||||||
)
|
|
||||||
assert.Equal(
|
|
||||||
t,
|
|
||||||
map[string]string{"Webhook URL": slackMaskedURL},
|
|
||||||
fieldMap(view.Config),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// slackTarget is the live Slack target the projection tests
|
|
||||||
// share.
|
|
||||||
func slackTarget() database.Target {
|
|
||||||
return database.Target{
|
|
||||||
Name: slackTargetName,
|
|
||||||
Type: database.TargetTypeSlack,
|
Type: database.TargetTypeSlack,
|
||||||
Active: true,
|
Active: true,
|
||||||
Config: `{"webhookUrl":"` +
|
Config: `{"webhookUrl":"` +
|
||||||
slackWebhookURL + `"}`,
|
slackWebhookURL + `"}`,
|
||||||
}
|
})
|
||||||
}
|
|
||||||
|
|
||||||
func TestNewTargetViews_Slack(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
view := viewFor(t, slackTarget())
|
|
||||||
|
|
||||||
assert.Equal(t, slackTargetName, view.Name)
|
|
||||||
|
|
||||||
// A live target is never marked, so the marker cannot
|
|
||||||
// reach a name that still exists.
|
|
||||||
assert.False(t, view.Deleted)
|
|
||||||
assert.Equal(t, slackTargetName, view.DisplayName())
|
|
||||||
|
|
||||||
|
assert.Equal(t, "slack-target", view.Name)
|
||||||
assert.Equal(
|
assert.Equal(
|
||||||
t,
|
t,
|
||||||
map[string]string{"Webhook URL": slackMaskedURL},
|
map[string]string{
|
||||||
|
"Webhook URL": "https://hooks.slack.com/...",
|
||||||
|
},
|
||||||
fieldMap(view.Config),
|
fieldMap(view.Config),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -260,7 +212,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) {
|
|||||||
|
|
||||||
assert.Equal(
|
assert.Equal(
|
||||||
t,
|
t,
|
||||||
slackMaskedURL,
|
"https://hooks.slack.com/...",
|
||||||
fields["Destination URL"],
|
fields["Destination URL"],
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -368,7 +368,7 @@ func (h *Handlers) finishReplay(
|
|||||||
// The page is read from the form rather than the query string:
|
// The page is read from the form rather than the query string:
|
||||||
// this is a POST, and its query string is what logs and Referer
|
// this is a POST, and its query string is what logs and Referer
|
||||||
// headers record.
|
// headers record.
|
||||||
if page := pageOrFirst(
|
if page := parseNonNegativeInt(
|
||||||
r.PostFormValue("page"),
|
r.PostFormValue("page"),
|
||||||
); page > 1 {
|
); page > 1 {
|
||||||
dest += "&page=" + strconv.Itoa(page)
|
dest += "&page=" + strconv.Itoa(page)
|
||||||
|
|||||||
@@ -267,7 +267,7 @@ func (h *Handlers) finishResubmit(
|
|||||||
// The page is read from the form rather than the query string:
|
// The page is read from the form rather than the query string:
|
||||||
// this is a POST, and its query string is what logs and Referer
|
// this is a POST, and its query string is what logs and Referer
|
||||||
// headers record.
|
// headers record.
|
||||||
if page := pageOrFirst(
|
if page := parseNonNegativeInt(
|
||||||
r.PostFormValue("page"),
|
r.PostFormValue("page"),
|
||||||
); page > 1 {
|
); page > 1 {
|
||||||
dest += "&page=" + strconv.Itoa(page)
|
dest += "&page=" + strconv.Itoa(page)
|
||||||
|
|||||||
@@ -27,17 +27,6 @@ const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes
|
|||||||
// per-delivery attempt ceiling to the handlers_test package.
|
// per-delivery attempt ceiling to the handlers_test package.
|
||||||
const MaxRenderedAttemptsForTest = maxRenderedAttempts
|
const MaxRenderedAttemptsForTest = maxRenderedAttempts
|
||||||
|
|
||||||
// MaxTargetRetriesForTest exposes the target max_retries 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
|
|
||||||
|
|
||||||
// PageOrFirstForTest exposes pageOrFirst for use in the handlers_test
|
|
||||||
// package.
|
|
||||||
func PageOrFirstForTest(s string) int {
|
|
||||||
return pageOrFirst(s)
|
|
||||||
}
|
|
||||||
|
|
||||||
// DummyVerificationsForTest reports how many equivalent-cost
|
// DummyVerificationsForTest reports how many equivalent-cost
|
||||||
// verifications were charged for usernames that do not exist. It
|
// verifications were charged for usernames that do not exist. It
|
||||||
// lets a test prove the anti-enumeration path ran without timing
|
// lets a test prove the anti-enumeration path ran without timing
|
||||||
|
|||||||
@@ -1,144 +0,0 @@
|
|||||||
package handlers_test
|
|
||||||
|
|
||||||
import (
|
|
||||||
"net/http"
|
|
||||||
"net/http/httptest"
|
|
||||||
"testing"
|
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
|
||||||
"github.com/stretchr/testify/require"
|
|
||||||
"sneak.berlin/go/webhooker/internal/database"
|
|
||||||
"sneak.berlin/go/webhooker/internal/handlers"
|
|
||||||
"sneak.berlin/go/webhooker/internal/session"
|
|
||||||
)
|
|
||||||
|
|
||||||
// deletedMarker is the suffix the event log appends to the name
|
|
||||||
// of a target that no longer exists.
|
|
||||||
const deletedMarker = " (deleted)"
|
|
||||||
|
|
||||||
// deleteTargetThroughHandler removes a target through the real
|
|
||||||
// deletion handler, so the test soft-deletes exactly the way the
|
|
||||||
// UI does rather than by writing the timestamp itself.
|
|
||||||
func deleteTargetThroughHandler(
|
|
||||||
t *testing.T,
|
|
||||||
h *handlers.Handlers,
|
|
||||||
sess *session.Session,
|
|
||||||
webhookID, targetID string,
|
|
||||||
) {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
req := postRequest(
|
|
||||||
"/source/"+webhookID+"/targets/"+targetID+"/delete",
|
|
||||||
authenticatedCookies(
|
|
||||||
t, sess, deleteTestUserID, deleteTestUsername,
|
|
||||||
),
|
|
||||||
map[string]string{
|
|
||||||
paramSourceID: webhookID,
|
|
||||||
paramTargetID: targetID,
|
|
||||||
},
|
|
||||||
)
|
|
||||||
w := httptest.NewRecorder()
|
|
||||||
|
|
||||||
h.HandleTargetDelete().ServeHTTP(w, req)
|
|
||||||
|
|
||||||
require.Equal(t, http.StatusSeeOther, w.Code)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestHandleSourceLogs_NamesDeletedTarget proves a delivery
|
|
||||||
// produced by a since-deleted target still names it on the event
|
|
||||||
// log, marked as deleted.
|
|
||||||
//
|
|
||||||
// Deletes are soft and deliveries carry no foreign key to the
|
|
||||||
// target row, so the history outlives the target. Against a
|
|
||||||
// scoped lookup the delivery resolves to a zero view and the page
|
|
||||||
// renders ": delivered" with nothing saying what it was delivered
|
|
||||||
// to.
|
|
||||||
func TestHandleSourceLogs_NamesDeletedTarget(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
var (
|
|
||||||
h *handlers.Handlers
|
|
||||||
sess *session.Session
|
|
||||||
db *database.Database
|
|
||||||
dbMgr *database.WebhookDBManager
|
|
||||||
)
|
|
||||||
|
|
||||||
app := newTestApp(t, &h, &sess, &db, &dbMgr)
|
|
||||||
app.RequireStart()
|
|
||||||
|
|
||||||
t.Cleanup(app.RequireStop)
|
|
||||||
|
|
||||||
wh := seedWebhook(t, db)
|
|
||||||
tgt := seedTarget(t, db, wh.ID, database.TargetTypeLog)
|
|
||||||
|
|
||||||
seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
|
|
||||||
|
|
||||||
// The control: the name is on the page while the target
|
|
||||||
// lives, and is not yet marked as deleted.
|
|
||||||
before := renderSourceLogsPage(t, h, sess, wh.ID)
|
|
||||||
assert.Contains(t, before, tgt.Name)
|
|
||||||
assert.NotContains(t, before, tgt.Name+deletedMarker)
|
|
||||||
|
|
||||||
deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
|
|
||||||
|
|
||||||
after := renderSourceLogsPage(t, h, sess, wh.ID)
|
|
||||||
|
|
||||||
assert.Contains(
|
|
||||||
t, after, tgt.Name+deletedMarker,
|
|
||||||
"a delivery from a deleted target must keep its name, "+
|
|
||||||
"marked as no longer existing",
|
|
||||||
)
|
|
||||||
assert.Contains(
|
|
||||||
t, after, "delivered",
|
|
||||||
"the delivery history itself must survive the delete",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestHandleSourceLogs_MasksDeletedTargetConfig proves that
|
|
||||||
// naming a deleted target does not widen what the page shows of
|
|
||||||
// it: its stored configuration stays masked by exactly the rules
|
|
||||||
// a live target's is.
|
|
||||||
//
|
|
||||||
// The lookup behind the name reads soft-deleted rows, so it
|
|
||||||
// carries a full target row — credential blob included — into the
|
|
||||||
// place a zero value used to sit. The projection to TargetView is
|
|
||||||
// what keeps that blob away from the template, and it must hold
|
|
||||||
// for a deleted row too.
|
|
||||||
func TestHandleSourceLogs_MasksDeletedTargetConfig(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
var (
|
|
||||||
h *handlers.Handlers
|
|
||||||
sess *session.Session
|
|
||||||
db *database.Database
|
|
||||||
dbMgr *database.WebhookDBManager
|
|
||||||
)
|
|
||||||
|
|
||||||
app := newTestApp(t, &h, &sess, &db, &dbMgr)
|
|
||||||
app.RequireStart()
|
|
||||||
|
|
||||||
t.Cleanup(app.RequireStop)
|
|
||||||
|
|
||||||
wh := seedWebhook(t, db)
|
|
||||||
tgt := seedConfiguredTarget(
|
|
||||||
t, db, wh.ID,
|
|
||||||
database.TargetTypeSlack,
|
|
||||||
`{"webhookUrl":"`+slackWebhookURL+`"}`,
|
|
||||||
)
|
|
||||||
|
|
||||||
seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
|
|
||||||
deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
|
|
||||||
|
|
||||||
body := renderSourceLogsPage(t, h, sess, wh.ID)
|
|
||||||
|
|
||||||
assert.NotContains(t, body, slackSecretPath)
|
|
||||||
assert.NotContains(t, body, "T00000000")
|
|
||||||
assert.NotContains(t, body, "B00000000")
|
|
||||||
assert.NotContains(
|
|
||||||
t, body, "XXXXXXXXXXXXXXXXXXXXXXXX",
|
|
||||||
)
|
|
||||||
assert.NotContains(t, body, "webhookUrl")
|
|
||||||
|
|
||||||
// The name is there; only the credential is not.
|
|
||||||
assert.Contains(t, body, tgt.Name+deletedMarker)
|
|
||||||
}
|
|
||||||
@@ -860,16 +860,11 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
|
|||||||
//
|
//
|
||||||
// The load is Unscoped because deleting a target only soft
|
// The load is Unscoped because deleting a target only soft
|
||||||
// deletes the row while its deliveries survive in the
|
// deletes the row while its deliveries survive in the
|
||||||
// per-webhook database. Both halves of the map need those rows:
|
// per-webhook database: a scoped load leaves those deliveries
|
||||||
// a scoped load leaves an old delivery with a zero redactor,
|
// with a zero redactor, which renders their response bodies
|
||||||
// which renders its response bodies unredacted, and with a zero
|
// unredacted. Only the redactor half of the map is built from
|
||||||
// view, which renders its target as a blank name.
|
// deleted rows. The view half, which is what the page lists,
|
||||||
//
|
// stays scoped.
|
||||||
// This map is historical display only. It is built for the event
|
|
||||||
// log page and reaches nothing but DeliveryView.Target: the
|
|
||||||
// target list on the source detail page, the edit form and the
|
|
||||||
// replay path each resolve targets themselves, and a deleted row
|
|
||||||
// is refused there as before.
|
|
||||||
func (h *Handlers) loadTargetMap(
|
func (h *Handlers) loadTargetMap(
|
||||||
webhookID string,
|
webhookID string,
|
||||||
) (map[string]eventLogTarget, error) {
|
) (map[string]eventLogTarget, error) {
|
||||||
@@ -885,18 +880,21 @@ func (h *Handlers) loadTargetMap(
|
|||||||
targetMap := make(
|
targetMap := make(
|
||||||
map[string]eventLogTarget, len(targets),
|
map[string]eventLogTarget, len(targets),
|
||||||
)
|
)
|
||||||
|
live := make([]database.Target, 0, len(targets))
|
||||||
|
|
||||||
for i := range targets {
|
for i := range targets {
|
||||||
targetMap[targets[i].ID] = eventLogTarget{
|
targetMap[targets[i].ID] = eventLogTarget{
|
||||||
Redactor: delivery.NewRedactor(&targets[i]),
|
Redactor: delivery.NewRedactor(&targets[i]),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if !targets[i].DeletedAt.Valid {
|
||||||
|
live = append(live, targets[i])
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// The views come from NewTargetViews rather than being
|
// The views come from NewTargetViews rather than being
|
||||||
// rebuilt here, so the masking rules stay in one place and a
|
// rebuilt here, so the masking rules stay in one place.
|
||||||
// deleted target's configuration is masked by the same code
|
for _, v := range delivery.NewTargetViews(live) {
|
||||||
// that masks a live one's.
|
|
||||||
for _, v := range delivery.NewTargetViews(targets) {
|
|
||||||
entry := targetMap[v.ID]
|
entry := targetMap[v.ID]
|
||||||
entry.View = v
|
entry.View = v
|
||||||
targetMap[v.ID] = entry
|
targetMap[v.ID] = entry
|
||||||
@@ -907,7 +905,16 @@ func (h *Handlers) loadTargetMap(
|
|||||||
|
|
||||||
// parsePage extracts a page number from the query string.
|
// parsePage extracts a page number from the query string.
|
||||||
func (h *Handlers) parsePage(r *http.Request) int {
|
func (h *Handlers) parsePage(r *http.Request) int {
|
||||||
return pageOrFirst(r.URL.Query().Get("page"))
|
page := 1
|
||||||
|
|
||||||
|
if p := r.URL.Query().Get("page"); p != "" {
|
||||||
|
v, err := strconv.Atoi(p)
|
||||||
|
if err == nil && v > 0 {
|
||||||
|
page = v
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return page
|
||||||
}
|
}
|
||||||
|
|
||||||
// loadEventsWithDeliveries loads paginated events and their
|
// loadEventsWithDeliveries loads paginated events and their
|
||||||
@@ -1436,6 +1443,7 @@ func (h *Handlers) processTargetCreate(
|
|||||||
// Referer headers and error trackers record.
|
// Referer headers and error trackers record.
|
||||||
name := r.PostFormValue("name")
|
name := r.PostFormValue("name")
|
||||||
targetType := database.TargetType(r.PostFormValue("type"))
|
targetType := database.TargetType(r.PostFormValue("type"))
|
||||||
|
maxRetriesStr := r.PostFormValue("max_retries")
|
||||||
|
|
||||||
if name == "" {
|
if name == "" {
|
||||||
http.Error(
|
http.Error(
|
||||||
@@ -1461,14 +1469,7 @@ func (h *Handlers) processTargetCreate(
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
// A new target has no stored retry count, so an absent field
|
maxRetries := parseNonNegativeInt(maxRetriesStr)
|
||||||
// takes the fire-and-forget default. A field the operator filled
|
|
||||||
// in with something invalid is rejected rather than becoming
|
|
||||||
// that default.
|
|
||||||
maxRetries, ok := targetMaxRetries(w, r, 0)
|
|
||||||
if !ok {
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
target := &database.Target{
|
target := &database.Target{
|
||||||
WebhookID: webhook.ID,
|
WebhookID: webhook.ID,
|
||||||
@@ -1504,22 +1505,19 @@ func isValidTargetType(tt database.TargetType) bool {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// pageOrFirst parses a paginated page number, answering 1 for
|
// parseNonNegativeInt parses s as a non-negative integer,
|
||||||
// anything empty, unparseable or out of range.
|
// returning 0 if s is empty or invalid.
|
||||||
//
|
func parseNonNegativeInt(s string) int {
|
||||||
// Falling back rather than rejecting is correct here and only here:
|
if s == "" {
|
||||||
// a page number is where to send the browser next, not configuration
|
return 0
|
||||||
// the operator is storing, and the actions that submit one have
|
|
||||||
// already completed by the time it is read — answering 400 would
|
|
||||||
// report a failure that did not happen. Anything an operator SETS
|
|
||||||
// must be validated instead; see parseMaxRetries.
|
|
||||||
func pageOrFirst(s string) int {
|
|
||||||
v, err := strconv.Atoi(strings.TrimSpace(s))
|
|
||||||
if err != nil || v < 1 {
|
|
||||||
return 1
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
v, err := strconv.Atoi(s)
|
||||||
|
if err == nil && v >= 0 {
|
||||||
return v
|
return v
|
||||||
|
}
|
||||||
|
|
||||||
|
return 0
|
||||||
}
|
}
|
||||||
|
|
||||||
// targetFormInput carries the raw form values describing a target's
|
// targetFormInput carries the raw form values describing a target's
|
||||||
|
|||||||
@@ -133,28 +133,20 @@ func (h *Handlers) applyTargetEdit(
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
target.Name = name
|
||||||
|
target.Config = configJSON
|
||||||
|
|
||||||
// Retries are offered only by the forms for target types that
|
// Retries are offered only by the forms for target types that
|
||||||
// retry, so an absent field means "this form does not edit
|
// retry, so an absent field means "this form does not edit
|
||||||
// retries" rather than "set them to zero". Reading it
|
// retries" rather than "set them to zero". Reading it
|
||||||
// unconditionally would silently disable retries on any target
|
// unconditionally would silently disable retries on any target
|
||||||
// saved from a form that does not render the input.
|
// saved from a form that does not render the input.
|
||||||
//
|
|
||||||
// A field that IS submitted but does not parse is a 400, through
|
|
||||||
// the same validator the create path uses. It is rejected before
|
|
||||||
// anything is written, so a typo cannot destroy the retry count
|
|
||||||
// the target is already delivering with.
|
|
||||||
if r.PostForm.Has("max_retries") {
|
if r.PostForm.Has("max_retries") {
|
||||||
retries, ok := targetMaxRetries(w, r, target.MaxRetries)
|
target.MaxRetries = parseNonNegativeInt(
|
||||||
if !ok {
|
r.PostFormValue("max_retries"),
|
||||||
return
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
target.MaxRetries = retries
|
|
||||||
}
|
|
||||||
|
|
||||||
target.Name = name
|
|
||||||
target.Config = configJSON
|
|
||||||
|
|
||||||
err = h.db.DB().Save(target).Error
|
err = h.db.DB().Save(target).Error
|
||||||
if err != nil {
|
if err != nil {
|
||||||
h.serverError(w, "failed to update target", err)
|
h.serverError(w, "failed to update target", err)
|
||||||
|
|||||||
@@ -1,119 +0,0 @@
|
|||||||
package handlers
|
|
||||||
|
|
||||||
import (
|
|
||||||
"errors"
|
|
||||||
"net/http"
|
|
||||||
"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(
|
|
||||||
"retries must be a whole number of attempts",
|
|
||||||
)
|
|
||||||
|
|
||||||
// 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("retries 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.
|
|
||||||
//
|
|
||||||
// A target stored with a count above the ceiling before this
|
|
||||||
// validation existed keeps rendering and keeps delivering — nothing
|
|
||||||
// clamps the row. Re-saving it from the edit form does have to bring
|
|
||||||
// it into range, because the form submits the pre-filled value back
|
|
||||||
// and accepting it would be the ceiling not applying to the edit
|
|
||||||
// path. The 400 names the ceiling, so the fix is one field.
|
|
||||||
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) +
|
|
||||||
" retries"
|
|
||||||
}
|
|
||||||
|
|
||||||
return errRetriesInvalid.Error() +
|
|
||||||
", or 0 for fire-and-forget"
|
|
||||||
}
|
|
||||||
|
|
||||||
// targetMaxRetries reads and validates max_retries from a target form
|
|
||||||
// submission, answering the request with a 400 and reporting false
|
|
||||||
// when the value is set but invalid.
|
|
||||||
//
|
|
||||||
// Both the create and the edit path go through here, so the two
|
|
||||||
// cannot come to disagree about what a valid retry count is. The
|
|
||||||
// wording matches the timeout control on the same submission.
|
|
||||||
func targetMaxRetries(
|
|
||||||
w http.ResponseWriter,
|
|
||||||
r *http.Request,
|
|
||||||
fallback int,
|
|
||||||
) (int, bool) {
|
|
||||||
retries, err := parseMaxRetries(
|
|
||||||
r.PostFormValue("max_retries"), fallback,
|
|
||||||
)
|
|
||||||
if err != nil {
|
|
||||||
http.Error(
|
|
||||||
w,
|
|
||||||
"Invalid max retries: "+retriesErrorMessage(err),
|
|
||||||
http.StatusBadRequest,
|
|
||||||
)
|
|
||||||
|
|
||||||
return 0, false
|
|
||||||
}
|
|
||||||
|
|
||||||
return retries, true
|
|
||||||
}
|
|
||||||
@@ -1,402 +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,
|
|
||||||
"/source/"+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,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestPageOrFirst_CoercesRatherThanRejects pins the one place a
|
|
||||||
// non-numeric form value legitimately falls back. A page number says
|
|
||||||
// where to send the browser after an action that has already
|
|
||||||
// happened, so it is not configuration and rejecting it would report
|
|
||||||
// a failure that did not occur.
|
|
||||||
func TestPageOrFirst_CoercesRatherThanRejects(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
for _, s := range []string{"", "abc", "0", "-1", "2.7", " "} {
|
|
||||||
assert.Equal(t, 1, handlers.PageOrFirstForTest(s),
|
|
||||||
"%q should fall back to the first page", s)
|
|
||||||
}
|
|
||||||
|
|
||||||
assert.Equal(t, 4, handlers.PageOrFirstForTest("4"))
|
|
||||||
assert.Equal(t, 4, handlers.PageOrFirstForTest(" 4 "))
|
|
||||||
}
|
|
||||||
@@ -4,32 +4,8 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"net/http"
|
"net/http"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
httpmetrics "github.com/slok/go-http-metrics/metrics"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
// MetricsMiddlewareForTest builds the metrics recording middleware
|
|
||||||
// against a caller-supplied recorder, so a test can gather from its
|
|
||||||
// own Prometheus registry rather than the process-wide default one
|
|
||||||
// that Middleware.Metrics uses.
|
|
||||||
func MetricsMiddlewareForTest(
|
|
||||||
rec httpmetrics.Recorder,
|
|
||||||
) func(http.Handler) http.Handler {
|
|
||||||
return metricsMiddleware(rec)
|
|
||||||
}
|
|
||||||
|
|
||||||
// UnmatchedRouteConst exposes the sentinel that stands in for a
|
|
||||||
// request matching no route pattern.
|
|
||||||
const UnmatchedRouteConst = unmatchedRoute
|
|
||||||
|
|
||||||
// InflightHandlerConst exposes the fixed handler label on the
|
|
||||||
// inflight gauge.
|
|
||||||
const InflightHandlerConst = inflightHandler
|
|
||||||
|
|
||||||
// UnmatchedMethodConst exposes the sentinel that stands in for a
|
|
||||||
// method the router can never route.
|
|
||||||
const UnmatchedMethodConst = unmatchedMethod
|
|
||||||
|
|
||||||
// NewLoggingResponseWriterForTest wraps newLoggingResponseWriter
|
// NewLoggingResponseWriterForTest wraps newLoggingResponseWriter
|
||||||
// for use in external test packages.
|
// for use in external test packages.
|
||||||
func NewLoggingResponseWriterForTest(
|
func NewLoggingResponseWriterForTest(
|
||||||
|
|||||||
@@ -1,182 +0,0 @@
|
|||||||
package middleware
|
|
||||||
|
|
||||||
import (
|
|
||||||
"context"
|
|
||||||
"net/http"
|
|
||||||
"time"
|
|
||||||
|
|
||||||
"github.com/go-chi/chi"
|
|
||||||
httpmetrics "github.com/slok/go-http-metrics/metrics"
|
|
||||||
prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
|
|
||||||
ghmm "github.com/slok/go-http-metrics/middleware"
|
|
||||||
"github.com/slok/go-http-metrics/middleware/std"
|
|
||||||
)
|
|
||||||
|
|
||||||
// inflightHandler is the fixed `handler` label on
|
|
||||||
// http_requests_inflight, the one HTTP metric here that cannot carry
|
|
||||||
// a route pattern.
|
|
||||||
//
|
|
||||||
// The gauge is incremented before the wrapped handler runs and
|
|
||||||
// decremented after it returns, and the pattern only exists between
|
|
||||||
// those two moments. Deriving the label from the route would
|
|
||||||
// therefore increment one series and decrement another, leaving every
|
|
||||||
// pattern permanently off by the number of requests it served — a
|
|
||||||
// broken gauge, on top of the per-path cardinality this file exists
|
|
||||||
// to remove. So the gauge is deliberately aggregate: one series,
|
|
||||||
// counting the requests in flight across the whole service.
|
|
||||||
const inflightHandler = "(all)"
|
|
||||||
|
|
||||||
// unmatchedMethod is the `method` label for a request whose method
|
|
||||||
// the router can never route.
|
|
||||||
//
|
|
||||||
// It is deliberately the same sentinel as unmatchedRoute rather than
|
|
||||||
// a spelling of its own: both stand for a client-chosen token that
|
|
||||||
// matched nothing this service registers, and giving one idea two
|
|
||||||
// spellings would read in a scrape as two different unmatched states.
|
|
||||||
const unmatchedMethod = unmatchedRoute
|
|
||||||
|
|
||||||
// routePatternID is the `handler` label for a request: the chi route
|
|
||||||
// pattern, never the concrete path.
|
|
||||||
//
|
|
||||||
// The pattern is what bounds the label's domain to the routes the
|
|
||||||
// service registers. The path does not bound it at all — every byte
|
|
||||||
// after /webhook/ is client-chosen, so labelling by path lets any
|
|
||||||
// unauthenticated client mint permanent series at will, and publishes
|
|
||||||
// the entrypoint UUID (the receiver's only credential) in the scrape
|
|
||||||
// while doing it.
|
|
||||||
//
|
|
||||||
// chi populates the route context during routeHTTP, so this is only
|
|
||||||
// valid once routing has run. Every caller below is on the recording
|
|
||||||
// side of the middleware, which go-http-metrics defers until after
|
|
||||||
// the wrapped handler returns.
|
|
||||||
func routePatternID(ctx context.Context) string {
|
|
||||||
if rc := chi.RouteContext(ctx); rc != nil {
|
|
||||||
if pattern := rc.RoutePattern(); pattern != "" {
|
|
||||||
return pattern
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return unmatchedRoute
|
|
||||||
}
|
|
||||||
|
|
||||||
// methodID is the `method` label for a request: the request method
|
|
||||||
// when the router can route it, and the unmatched sentinel otherwise.
|
|
||||||
//
|
|
||||||
// net/http accepts any RFC 9110 token as a method and hands it
|
|
||||||
// through verbatim, so the raw method is client-chosen bytes and
|
|
||||||
// bounds the label at nothing — the same unauthenticated
|
|
||||||
// series-minting the handler label carried, reached through a second
|
|
||||||
// dimension. What bounds it is the set chi's router will match a
|
|
||||||
// route for: its methodMap, which is unexported, so it is restated
|
|
||||||
// here against the net/http constants it is built from. A token
|
|
||||||
// outside that set can only ever produce chi's 405, so folding every
|
|
||||||
// one of them onto a single series loses no information a scrape
|
|
||||||
// could have used, while the nine methods that can reach a handler
|
|
||||||
// stay distinguishable.
|
|
||||||
//
|
|
||||||
// chi.RegisterMethod would extend the router's set at runtime; this
|
|
||||||
// service never calls it, and a caller that started to would have to
|
|
||||||
// extend this switch with it.
|
|
||||||
func methodID(method string) string {
|
|
||||||
switch method {
|
|
||||||
case http.MethodConnect,
|
|
||||||
http.MethodDelete,
|
|
||||||
http.MethodGet,
|
|
||||||
http.MethodHead,
|
|
||||||
http.MethodOptions,
|
|
||||||
http.MethodPatch,
|
|
||||||
http.MethodPost,
|
|
||||||
http.MethodPut,
|
|
||||||
http.MethodTrace:
|
|
||||||
return method
|
|
||||||
default:
|
|
||||||
return unmatchedMethod
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// boundedLabelRecorder wraps a go-http-metrics recorder and replaces
|
|
||||||
// the request-controlled labels on every observation with bounded
|
|
||||||
// ones: the handler id becomes the request's route pattern, and the
|
|
||||||
// method becomes one the router can route.
|
|
||||||
//
|
|
||||||
// This is the seam that makes the pattern usable at all. The metrics
|
|
||||||
// middleware is global (see Server.setupGlobalMiddleware), so it is
|
|
||||||
// entered before chi has matched anything, and go-http-metrics fixes
|
|
||||||
// its handler id up front — passing the pattern in as that id is not
|
|
||||||
// possible, and leaving the id empty makes the library substitute the
|
|
||||||
// raw URL path, which is the defect. What the library does hand over
|
|
||||||
// is the request context, unchanged, on each recorder call; that
|
|
||||||
// context carries the same *chi.Context pointer routing mutates in
|
|
||||||
// place, and the duration and size calls happen after the wrapped
|
|
||||||
// handler has returned. Reading the pattern there is what the access
|
|
||||||
// log already does in accessLogURL.
|
|
||||||
//
|
|
||||||
// Recording after the whole chain returns is also what makes this
|
|
||||||
// hold for requests the route-level receiver rate limiter rejects.
|
|
||||||
// Those never reach a handler, but chi has already matched the route
|
|
||||||
// by the time the limiter runs, so their 429s land on the pattern
|
|
||||||
// like any other response.
|
|
||||||
type boundedLabelRecorder struct {
|
|
||||||
inner httpmetrics.Recorder
|
|
||||||
}
|
|
||||||
|
|
||||||
func (r boundedLabelRecorder) ObserveHTTPRequestDuration(
|
|
||||||
ctx context.Context,
|
|
||||||
props httpmetrics.HTTPReqProperties,
|
|
||||||
duration time.Duration,
|
|
||||||
) {
|
|
||||||
props.ID = routePatternID(ctx)
|
|
||||||
props.Method = methodID(props.Method)
|
|
||||||
r.inner.ObserveHTTPRequestDuration(ctx, props, duration)
|
|
||||||
}
|
|
||||||
|
|
||||||
func (r boundedLabelRecorder) ObserveHTTPResponseSize(
|
|
||||||
ctx context.Context,
|
|
||||||
props httpmetrics.HTTPReqProperties,
|
|
||||||
sizeBytes int64,
|
|
||||||
) {
|
|
||||||
props.ID = routePatternID(ctx)
|
|
||||||
props.Method = methodID(props.Method)
|
|
||||||
r.inner.ObserveHTTPResponseSize(ctx, props, sizeBytes)
|
|
||||||
}
|
|
||||||
|
|
||||||
func (r boundedLabelRecorder) AddInflightRequests(
|
|
||||||
ctx context.Context,
|
|
||||||
props httpmetrics.HTTPProperties,
|
|
||||||
quantity int,
|
|
||||||
) {
|
|
||||||
props.ID = inflightHandler
|
|
||||||
r.inner.AddInflightRequests(ctx, props, quantity)
|
|
||||||
}
|
|
||||||
|
|
||||||
var _ httpmetrics.Recorder = boundedLabelRecorder{}
|
|
||||||
|
|
||||||
// Metrics returns middleware that records Prometheus HTTP metrics on
|
|
||||||
// the default registry, which is the one the /metrics route gathers.
|
|
||||||
func (s *Middleware) Metrics() func(http.Handler) http.Handler {
|
|
||||||
return metricsMiddleware(
|
|
||||||
prommetrics.NewRecorder(prommetrics.Config{}),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// metricsMiddleware builds the recording middleware against a given
|
|
||||||
// recorder, so tests can gather from a registry of their own instead
|
|
||||||
// of the process-wide default.
|
|
||||||
func metricsMiddleware(
|
|
||||||
rec httpmetrics.Recorder,
|
|
||||||
) func(http.Handler) http.Handler {
|
|
||||||
mdlw := ghmm.New(ghmm.Config{
|
|
||||||
Recorder: boundedLabelRecorder{inner: rec},
|
|
||||||
})
|
|
||||||
|
|
||||||
return func(next http.Handler) http.Handler {
|
|
||||||
// The handler id is unmatchedRoute rather than "" so that
|
|
||||||
// the client-chosen URL path never enters the metrics
|
|
||||||
// pipeline at all: an empty id is the library's signal to
|
|
||||||
// substitute it. boundedLabelRecorder overwrites this value
|
|
||||||
// on every observation, so it is reachable only if that
|
|
||||||
// decorator is removed — in which case the metrics collapse
|
|
||||||
// to one series instead of leaking again.
|
|
||||||
return std.Handler(unmatchedRoute, mdlw, next)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,309 +0,0 @@
|
|||||||
package middleware_test
|
|
||||||
|
|
||||||
import (
|
|
||||||
"net/http"
|
|
||||||
"net/http/httptest"
|
|
||||||
"strings"
|
|
||||||
"testing"
|
|
||||||
|
|
||||||
"github.com/google/uuid"
|
|
||||||
"github.com/prometheus/client_golang/prometheus"
|
|
||||||
"github.com/prometheus/client_golang/prometheus/promhttp"
|
|
||||||
dto "github.com/prometheus/client_model/go"
|
|
||||||
"github.com/stretchr/testify/assert"
|
|
||||||
"github.com/stretchr/testify/require"
|
|
||||||
"sneak.berlin/go/webhooker/internal/middleware"
|
|
||||||
)
|
|
||||||
|
|
||||||
const (
|
|
||||||
// metricsProbeMethods is how many distinct invented method tokens
|
|
||||||
// each cardinality assertion drives. The measurement on the issue
|
|
||||||
// took 300 tokens from 106 exposition lines to 7,631 — about 25
|
|
||||||
// permanent lines per token, never reclaimed — so a probe of this
|
|
||||||
// size puts a regression thousands of lines over the bound rather
|
|
||||||
// than leaving it to a rounding argument.
|
|
||||||
metricsProbeMethods = 300
|
|
||||||
|
|
||||||
// probeMethodLen is how many characters each invented method
|
|
||||||
// token carries, matching the 12 the issue measured with.
|
|
||||||
probeMethodLen = 12
|
|
||||||
|
|
||||||
// methodLabel is the label these tests are about.
|
|
||||||
methodLabel = "method"
|
|
||||||
)
|
|
||||||
|
|
||||||
// realMethods is the positive control's domain: the methods chi's
|
|
||||||
// router can match a route for, every one of which a client
|
|
||||||
// legitimately sends and every one of which must keep a series of its
|
|
||||||
// own. Bounding the label by collapsing these into one bucket would
|
|
||||||
// destroy the metric it is meant to protect.
|
|
||||||
func realMethods() []string {
|
|
||||||
return []string{
|
|
||||||
http.MethodConnect, http.MethodDelete, http.MethodGet,
|
|
||||||
http.MethodHead, http.MethodOptions, http.MethodPatch,
|
|
||||||
http.MethodPost, http.MethodPut, http.MethodTrace,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// methodProbePath returns the one receiver path a method probe
|
|
||||||
// targets. Holding the path fixed leaves the method as the only
|
|
||||||
// dimension varying, so any series growth a probe produces is the
|
|
||||||
// method label's and nothing else's.
|
|
||||||
func methodProbePath() string {
|
|
||||||
return "/webhook/" + uuid.NewString()
|
|
||||||
}
|
|
||||||
|
|
||||||
// inventedMethods returns n distinct RFC 9110 method tokens that no
|
|
||||||
// router will ever match: uppercase hex from a fresh UUID, which is
|
|
||||||
// both the shape and the length an unauthenticated flood would send.
|
|
||||||
// net/http accepts any token as a method, so every one of these
|
|
||||||
// reaches the metrics pipeline exactly as a real method does.
|
|
||||||
func inventedMethods(n int) []string {
|
|
||||||
methods := make([]string, 0, n)
|
|
||||||
|
|
||||||
for range n {
|
|
||||||
token := strings.ToUpper(
|
|
||||||
strings.ReplaceAll(uuid.NewString(), "-", ""),
|
|
||||||
)
|
|
||||||
methods = append(methods, token[:probeMethodLen])
|
|
||||||
}
|
|
||||||
|
|
||||||
return methods
|
|
||||||
}
|
|
||||||
|
|
||||||
// driveMethods sends one request per supplied method to a single
|
|
||||||
// fixed path.
|
|
||||||
func driveMethods(
|
|
||||||
t *testing.T,
|
|
||||||
h http.Handler,
|
|
||||||
path string,
|
|
||||||
methods []string,
|
|
||||||
) map[int]int {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
probes := make([]probe, 0, len(methods))
|
|
||||||
|
|
||||||
for _, m := range methods {
|
|
||||||
probes = append(probes, probe{method: m, path: path})
|
|
||||||
}
|
|
||||||
|
|
||||||
return drive(t, h, probes)
|
|
||||||
}
|
|
||||||
|
|
||||||
// methodLabels returns the set of distinct `method` values across
|
|
||||||
// every gathered series that carries the label at all. The inflight
|
|
||||||
// gauge does not carry it, and so contributes nothing rather than an
|
|
||||||
// empty-string member.
|
|
||||||
func methodLabels(families []*dto.MetricFamily) map[string]struct{} {
|
|
||||||
seen := make(map[string]struct{})
|
|
||||||
|
|
||||||
for _, fam := range families {
|
|
||||||
for _, m := range fam.GetMetric() {
|
|
||||||
for _, pair := range m.GetLabel() {
|
|
||||||
if pair.GetName() == methodLabel {
|
|
||||||
seen[pair.GetValue()] = struct{}{}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return seen
|
|
||||||
}
|
|
||||||
|
|
||||||
// scrapeLines renders the registry through the same promhttp handler
|
|
||||||
// /metrics is mounted on and counts the sample lines it produced.
|
|
||||||
//
|
|
||||||
// This is the quantity the issue measured and the one a Prometheus
|
|
||||||
// server pays for on every scrape: one histogram label set is a
|
|
||||||
// single gathered series but around 25 lines of exposition, which is
|
|
||||||
// why 300 method tokens cost thousands of lines rather than hundreds.
|
|
||||||
func scrapeLines(t *testing.T, reg *prometheus.Registry) int {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
h := promhttp.HandlerFor(reg, promhttp.HandlerOpts{})
|
|
||||||
req := httptest.NewRequestWithContext(
|
|
||||||
t.Context(), http.MethodGet, "/metrics", nil,
|
|
||||||
)
|
|
||||||
w := httptest.NewRecorder()
|
|
||||||
h.ServeHTTP(w, req)
|
|
||||||
require.Equal(t, http.StatusOK, w.Code)
|
|
||||||
|
|
||||||
lines := 0
|
|
||||||
|
|
||||||
for line := range strings.SplitSeq(w.Body.String(), "\n") {
|
|
||||||
if line == "" || strings.HasPrefix(line, "#") {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
lines++
|
|
||||||
}
|
|
||||||
|
|
||||||
return lines
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_MethodSentinelIsTheRouteSentinel pins the convention
|
|
||||||
// rather than the mechanism. An unroutable method and an unmatched
|
|
||||||
// path are the same fact — a client-chosen token matching nothing
|
|
||||||
// this service registers — so they carry one spelling. Two spellings
|
|
||||||
// would read in a scrape as two different unmatched states.
|
|
||||||
func TestMetrics_MethodSentinelIsTheRouteSentinel(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t,
|
|
||||||
middleware.UnmatchedRouteConst,
|
|
||||||
middleware.UnmatchedMethodConst,
|
|
||||||
"the unmatched sentinel must have exactly one spelling",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_InventedMethodsMintOneLabelSet is the direct assertion
|
|
||||||
// the issue asks for: N requests carrying N distinct invented method
|
|
||||||
// tokens must produce exactly ONE method label. Before the fix this
|
|
||||||
// produced N of them, on an unauthenticated route with no rate
|
|
||||||
// limiter.
|
|
||||||
func TestMetrics_InventedMethodsMintOneLabelSet(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
methods := inventedMethods(metricsProbeMethods)
|
|
||||||
|
|
||||||
codes := driveMethods(t, h, methodProbePath(), methods)
|
|
||||||
require.Equal(
|
|
||||||
t, metricsProbeMethods, codes[http.StatusMethodNotAllowed],
|
|
||||||
"every invented token should have been unroutable",
|
|
||||||
)
|
|
||||||
|
|
||||||
labels := methodLabels(gatherMetrics(t, reg))
|
|
||||||
|
|
||||||
// Asserted on the count rather than on the set, so that a
|
|
||||||
// regression reports one number instead of dumping every token it
|
|
||||||
// minted.
|
|
||||||
distinct := len(labels)
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, 1, distinct,
|
|
||||||
"invented methods must collapse onto one label",
|
|
||||||
)
|
|
||||||
assert.Contains(
|
|
||||||
t, keys(labels), middleware.UnmatchedMethodConst,
|
|
||||||
"that one label must be the unmatched sentinel",
|
|
||||||
)
|
|
||||||
|
|
||||||
// The scrape must not republish the tokens it was driven with
|
|
||||||
// either: a label that merely looks bounded while still echoing
|
|
||||||
// client bytes is the same defect wearing a different name.
|
|
||||||
echoed := 0
|
|
||||||
|
|
||||||
for _, m := range methods {
|
|
||||||
for label := range labels {
|
|
||||||
if strings.Contains(label, m) {
|
|
||||||
echoed++
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, 0, echoed,
|
|
||||||
"invented method tokens reached the metrics labels",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_MethodSeriesCountIsFlatUnderAFlood reproduces the
|
|
||||||
// measurement on the issue in miniature: scrape, drive several
|
|
||||||
// hundred distinct method tokens, scrape again, and require the
|
|
||||||
// second scrape to be no larger than the first. The first batch
|
|
||||||
// establishes every label set the route can produce; a flood five
|
|
||||||
// times its size must land on exactly those.
|
|
||||||
func TestMetrics_MethodSeriesCountIsFlatUnderAFlood(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
path := methodProbePath()
|
|
||||||
|
|
||||||
driveMethods(t, h, path, inventedMethods(metricsProbeMethods))
|
|
||||||
seededSeries := seriesCount(gatherMetrics(t, reg))
|
|
||||||
seededLines := scrapeLines(t, reg)
|
|
||||||
|
|
||||||
driveMethods(t, h, path, inventedMethods(metricsProbeMethods*4))
|
|
||||||
floodedSeries := seriesCount(gatherMetrics(t, reg))
|
|
||||||
floodedLines := scrapeLines(t, reg)
|
|
||||||
|
|
||||||
t.Logf(
|
|
||||||
"after %d invented methods: %d series, %d lines; "+
|
|
||||||
"after %d more: %d series, %d lines",
|
|
||||||
metricsProbeMethods, seededSeries, seededLines,
|
|
||||||
metricsProbeMethods*4, floodedSeries, floodedLines,
|
|
||||||
)
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, seededSeries, floodedSeries,
|
|
||||||
"a flood of invented methods must not mint series",
|
|
||||||
)
|
|
||||||
assert.Equal(
|
|
||||||
t, seededLines, floodedLines,
|
|
||||||
"a flood of invented methods must not grow the scrape",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_RealMethodsStayDistinct is the positive control. The
|
|
||||||
// bound is worth nothing if it is bought by flattening the metric:
|
|
||||||
// every method the router can route must still carry a series of its
|
|
||||||
// own, one sample each, under the route pattern it was sent to.
|
|
||||||
func TestMetrics_RealMethodsStayDistinct(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
methods := realMethods()
|
|
||||||
|
|
||||||
codes := driveMethods(t, h, methodProbePath(), methods)
|
|
||||||
require.Equal(
|
|
||||||
t, len(methods), codes[http.StatusNotFound],
|
|
||||||
"every real method should have reached the receiver",
|
|
||||||
)
|
|
||||||
|
|
||||||
families := gatherMetrics(t, reg)
|
|
||||||
|
|
||||||
want := make(map[string]struct{}, len(methods))
|
|
||||||
for _, m := range methods {
|
|
||||||
want[m] = struct{}{}
|
|
||||||
}
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, want, methodLabels(families),
|
|
||||||
"real methods must remain distinguishable",
|
|
||||||
)
|
|
||||||
|
|
||||||
// Appearing somewhere in the scrape is not enough: each method
|
|
||||||
// must own its duration series, holding the one sample it sent.
|
|
||||||
observed := 0
|
|
||||||
|
|
||||||
for _, fam := range families {
|
|
||||||
if !strings.HasSuffix(fam.GetName(), "request_duration_seconds") {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, m := range fam.GetMetric() {
|
|
||||||
observed++
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, receiverRoutePattern,
|
|
||||||
labelValue(m, "handler"),
|
|
||||||
)
|
|
||||||
assert.Equal(
|
|
||||||
t, uint64(1),
|
|
||||||
m.GetHistogram().GetSampleCount(),
|
|
||||||
"method %q shares a series",
|
|
||||||
labelValue(m, methodLabel),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, len(methods), observed,
|
|
||||||
"one duration series per routable method",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
@@ -1,457 +0,0 @@
|
|||||||
package middleware_test
|
|
||||||
|
|
||||||
import (
|
|
||||||
"log/slog"
|
|
||||||
"net/http"
|
|
||||||
"net/http/httptest"
|
|
||||||
"strings"
|
|
||||||
"testing"
|
|
||||||
|
|
||||||
"github.com/go-chi/chi"
|
|
||||||
"github.com/google/uuid"
|
|
||||||
"github.com/prometheus/client_golang/prometheus"
|
|
||||||
dto "github.com/prometheus/client_model/go"
|
|
||||||
prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
|
|
||||||
"github.com/stretchr/testify/assert"
|
|
||||||
"github.com/stretchr/testify/require"
|
|
||||||
"sneak.berlin/go/webhooker/internal/config"
|
|
||||||
"sneak.berlin/go/webhooker/internal/middleware"
|
|
||||||
)
|
|
||||||
|
|
||||||
const (
|
|
||||||
// metricsProbePaths is how many distinct receiver paths each
|
|
||||||
// cardinality assertion drives. The defect these tests pin cost
|
|
||||||
// roughly 26 permanent series per distinct path, so a couple of
|
|
||||||
// hundred puts a regression thousands of series over the bound
|
|
||||||
// rather than leaving it to a rounding argument.
|
|
||||||
metricsProbePaths = 250
|
|
||||||
|
|
||||||
// receiverRoutePattern is the one handler label every receiver
|
|
||||||
// request must produce, however the client varies the path.
|
|
||||||
receiverRoutePattern = "/webhook/{uuid}"
|
|
||||||
|
|
||||||
// okRoute is a static route used to pin that the response-writer
|
|
||||||
// interceptor still reports status and size after the handler id
|
|
||||||
// stopped coming from the URL.
|
|
||||||
okRoute = "/ok"
|
|
||||||
|
|
||||||
// okBody is what okRoute writes, so the recorded response size is
|
|
||||||
// a number the test knows.
|
|
||||||
okBody = "ok"
|
|
||||||
|
|
||||||
// generousReceiverLimit is a per-entrypoint receiver limit high
|
|
||||||
// enough that no probe in this file trips the limiter unless it
|
|
||||||
// means to.
|
|
||||||
generousReceiverLimit = 100000
|
|
||||||
|
|
||||||
// tightReceiverLimit forces the receiver's aggregate limiter to
|
|
||||||
// reject: the aggregate ceiling is ten times this, so a probe of
|
|
||||||
// metricsProbePaths requests spends it many times over.
|
|
||||||
tightReceiverLimit = 1
|
|
||||||
)
|
|
||||||
|
|
||||||
// metricsTestRouter builds a router whose middleware ordering mirrors
|
|
||||||
// the real server's: the metrics recorder is GLOBAL, installed by
|
|
||||||
// Server.setupGlobalMiddleware before chi has matched anything, and
|
|
||||||
// the receiver rate limiter is ROUTE-LEVEL, installed by
|
|
||||||
// Server.setupWebhookRoutes inside it. That ordering is the whole
|
|
||||||
// defect, so a test that flattens it would prove nothing.
|
|
||||||
//
|
|
||||||
// The recorder writes to a registry of the test's own rather than the
|
|
||||||
// process-wide default one, so each test observes only its own
|
|
||||||
// traffic.
|
|
||||||
func metricsTestRouter(
|
|
||||||
t *testing.T,
|
|
||||||
receiverLimit int,
|
|
||||||
) (http.Handler, *prometheus.Registry) {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
log := slog.New(slog.DiscardHandler)
|
|
||||||
cfg := &config.Config{
|
|
||||||
Environment: "prod",
|
|
||||||
ReceiverRateLimit: receiverLimit,
|
|
||||||
}
|
|
||||||
m := middleware.NewForTest(
|
|
||||||
log, cfg, newTestSessionManager(cfg, log, nil),
|
|
||||||
)
|
|
||||||
|
|
||||||
reg := prometheus.NewRegistry()
|
|
||||||
rec := prommetrics.NewRecorder(prommetrics.Config{Registry: reg})
|
|
||||||
|
|
||||||
r := chi.NewRouter()
|
|
||||||
r.Use(middleware.MetricsMiddlewareForTest(rec))
|
|
||||||
|
|
||||||
r.Get(okRoute, func(w http.ResponseWriter, _ *http.Request) {
|
|
||||||
_, _ = w.Write([]byte(okBody))
|
|
||||||
})
|
|
||||||
|
|
||||||
// The real receiver answers 404 for a UUID naming no stored
|
|
||||||
// entrypoint, which is what every invented path here is.
|
|
||||||
r.With(m.ReceiverRateLimit()).HandleFunc(
|
|
||||||
receiverRoutePattern,
|
|
||||||
func(w http.ResponseWriter, _ *http.Request) {
|
|
||||||
w.WriteHeader(http.StatusNotFound)
|
|
||||||
},
|
|
||||||
)
|
|
||||||
|
|
||||||
return r, reg
|
|
||||||
}
|
|
||||||
|
|
||||||
// probe is one request a cardinality assertion sends. Both label
|
|
||||||
// dimensions that have leaked are request-controlled — the path and
|
|
||||||
// the method — so both vary here and one driver sends them.
|
|
||||||
type probe struct {
|
|
||||||
method string
|
|
||||||
path string
|
|
||||||
}
|
|
||||||
|
|
||||||
// drive sends every probe and returns how many responses carried each
|
|
||||||
// status code.
|
|
||||||
func drive(t *testing.T, h http.Handler, probes []probe) map[int]int {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
codes := make(map[int]int)
|
|
||||||
|
|
||||||
for _, p := range probes {
|
|
||||||
req := httptest.NewRequestWithContext(
|
|
||||||
t.Context(), p.method, p.path, nil,
|
|
||||||
)
|
|
||||||
w := httptest.NewRecorder()
|
|
||||||
h.ServeHTTP(w, req)
|
|
||||||
codes[w.Code]++
|
|
||||||
}
|
|
||||||
|
|
||||||
return codes
|
|
||||||
}
|
|
||||||
|
|
||||||
// drivePaths sends one POST per supplied path.
|
|
||||||
func drivePaths(
|
|
||||||
t *testing.T,
|
|
||||||
h http.Handler,
|
|
||||||
paths []string,
|
|
||||||
) map[int]int {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
probes := make([]probe, 0, len(paths))
|
|
||||||
|
|
||||||
for _, p := range paths {
|
|
||||||
probes = append(
|
|
||||||
probes, probe{method: http.MethodPost, path: p},
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
return drive(t, h, probes)
|
|
||||||
}
|
|
||||||
|
|
||||||
// receiverPaths returns n distinct /webhook/ paths, each naming a
|
|
||||||
// fresh UUID exactly as an unauthenticated flood would.
|
|
||||||
func receiverPaths(n int) []string {
|
|
||||||
paths := make([]string, 0, n)
|
|
||||||
|
|
||||||
for range n {
|
|
||||||
paths = append(paths, "/webhook/"+uuid.NewString())
|
|
||||||
}
|
|
||||||
|
|
||||||
return paths
|
|
||||||
}
|
|
||||||
|
|
||||||
// gatherMetrics returns the registry's current families, failing the
|
|
||||||
// test if gathering does.
|
|
||||||
func gatherMetrics(
|
|
||||||
t *testing.T,
|
|
||||||
reg *prometheus.Registry,
|
|
||||||
) []*dto.MetricFamily {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
families, err := reg.Gather()
|
|
||||||
require.NoError(t, err)
|
|
||||||
|
|
||||||
return families
|
|
||||||
}
|
|
||||||
|
|
||||||
// labelValue returns the named label from a gathered metric.
|
|
||||||
func labelValue(m *dto.Metric, name string) string {
|
|
||||||
for _, pair := range m.GetLabel() {
|
|
||||||
if pair.GetName() == name {
|
|
||||||
return pair.GetValue()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return ""
|
|
||||||
}
|
|
||||||
|
|
||||||
// handlerLabels returns the set of distinct `handler` label values
|
|
||||||
// across every gathered series.
|
|
||||||
func handlerLabels(families []*dto.MetricFamily) map[string]struct{} {
|
|
||||||
seen := make(map[string]struct{})
|
|
||||||
|
|
||||||
for _, fam := range families {
|
|
||||||
for _, m := range fam.GetMetric() {
|
|
||||||
seen[labelValue(m, "handler")] = struct{}{}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return seen
|
|
||||||
}
|
|
||||||
|
|
||||||
// seriesCount is the number of distinct label sets held across every
|
|
||||||
// family: the quantity that grew without bound and was never
|
|
||||||
// reclaimed.
|
|
||||||
func seriesCount(families []*dto.MetricFamily) int {
|
|
||||||
total := 0
|
|
||||||
|
|
||||||
for _, fam := range families {
|
|
||||||
total += len(fam.GetMetric())
|
|
||||||
}
|
|
||||||
|
|
||||||
return total
|
|
||||||
}
|
|
||||||
|
|
||||||
// keys returns the members of a set, for assertion messages.
|
|
||||||
func keys(set map[string]struct{}) []string {
|
|
||||||
out := make([]string, 0, len(set))
|
|
||||||
|
|
||||||
for k := range set {
|
|
||||||
out = append(out, k)
|
|
||||||
}
|
|
||||||
|
|
||||||
return out
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_DistinctReceiverPathsMintOneLabelSet is the direct
|
|
||||||
// assertion the issue asks for: N requests to N distinct
|
|
||||||
// /webhook/<uuid> paths must produce exactly ONE handler label, the
|
|
||||||
// route pattern. Before the fix this produced N of them.
|
|
||||||
func TestMetrics_DistinctReceiverPathsMintOneLabelSet(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
paths := receiverPaths(metricsProbePaths)
|
|
||||||
codes := drivePaths(t, h, paths)
|
|
||||||
require.Equal(
|
|
||||||
t, metricsProbePaths, codes[http.StatusNotFound],
|
|
||||||
"every invented UUID should have reached the receiver",
|
|
||||||
)
|
|
||||||
|
|
||||||
families := gatherMetrics(t, reg)
|
|
||||||
labels := handlerLabels(families)
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t,
|
|
||||||
map[string]struct{}{
|
|
||||||
receiverRoutePattern: {},
|
|
||||||
middleware.InflightHandlerConst: {},
|
|
||||||
},
|
|
||||||
labels,
|
|
||||||
"receiver traffic must collapse onto the route pattern",
|
|
||||||
)
|
|
||||||
|
|
||||||
// The scrape must not republish the UUIDs it was driven with.
|
|
||||||
// They are the receiver's only credential.
|
|
||||||
for _, p := range paths {
|
|
||||||
id := strings.TrimPrefix(p, "/webhook/")
|
|
||||||
for label := range labels {
|
|
||||||
assert.NotContains(
|
|
||||||
t, label, id,
|
|
||||||
"an entrypoint UUID reached a metrics label",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_SeriesCountIsFlatUnderAFlood pins the property the
|
|
||||||
// issue measured against a live instance: driving thousands more
|
|
||||||
// distinct paths must not add series. The first batch establishes
|
|
||||||
// every label set the route can produce; the second must land on
|
|
||||||
// exactly those.
|
|
||||||
func TestMetrics_SeriesCountIsFlatUnderAFlood(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
drivePaths(t, h, receiverPaths(metricsProbePaths))
|
|
||||||
before := seriesCount(gatherMetrics(t, reg))
|
|
||||||
|
|
||||||
drivePaths(t, h, receiverPaths(metricsProbePaths*4))
|
|
||||||
after := seriesCount(gatherMetrics(t, reg))
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, before, after,
|
|
||||||
"a flood of distinct paths must not mint series",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_RateLimitedRequestsCarryTheRoutePattern covers the
|
|
||||||
// majority case: most of the leaked series were 429s. Those requests
|
|
||||||
// never reach a handler, so they take a different path through the
|
|
||||||
// stack — but chi has already matched the route by the time the
|
|
||||||
// route-level limiter rejects them, and the recording happens after
|
|
||||||
// the whole chain returns, so they must land on the pattern too.
|
|
||||||
func TestMetrics_RateLimitedRequestsCarryTheRoutePattern(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, tightReceiverLimit)
|
|
||||||
|
|
||||||
codes := drivePaths(t, h, receiverPaths(metricsProbePaths))
|
|
||||||
require.Positive(
|
|
||||||
t, codes[http.StatusTooManyRequests],
|
|
||||||
"the probe must actually exhaust the aggregate limiter",
|
|
||||||
)
|
|
||||||
|
|
||||||
families := gatherMetrics(t, reg)
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t,
|
|
||||||
map[string]struct{}{
|
|
||||||
receiverRoutePattern: {},
|
|
||||||
middleware.InflightHandlerConst: {},
|
|
||||||
},
|
|
||||||
handlerLabels(families),
|
|
||||||
"rejected requests must collapse onto the route pattern",
|
|
||||||
)
|
|
||||||
|
|
||||||
rejected := 0
|
|
||||||
|
|
||||||
for _, fam := range families {
|
|
||||||
for _, m := range fam.GetMetric() {
|
|
||||||
if labelValue(m, "code") != "429" {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
rejected++
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t, receiverRoutePattern,
|
|
||||||
labelValue(m, "handler"),
|
|
||||||
"a 429 series carried a non-pattern handler",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
assert.Positive(
|
|
||||||
t, rejected, "no 429 series was recorded at all",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_UnmatchedPathsCollapseToTheSentinel decides and pins the
|
|
||||||
// unmatched-route case. A path matching no route has no pattern, so
|
|
||||||
// it carries the same fixed sentinel the access log uses. Without
|
|
||||||
// that, an unmatched flood leaks exactly as the receiver did.
|
|
||||||
func TestMetrics_UnmatchedPathsCollapseToTheSentinel(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
paths := make([]string, 0, metricsProbePaths)
|
|
||||||
|
|
||||||
for i := range metricsProbePaths {
|
|
||||||
id := uuid.NewString()
|
|
||||||
|
|
||||||
// Two shapes: one matching no prefix at all, and one under
|
|
||||||
// the receiver prefix but with a segment count the pattern
|
|
||||||
// cannot match.
|
|
||||||
if i%2 == 0 {
|
|
||||||
paths = append(paths, "/"+id)
|
|
||||||
} else {
|
|
||||||
paths = append(paths, "/webhook/"+id+"/"+id)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
codes := drivePaths(t, h, paths)
|
|
||||||
require.Equal(
|
|
||||||
t, metricsProbePaths, codes[http.StatusNotFound],
|
|
||||||
"every probe path should have gone unmatched",
|
|
||||||
)
|
|
||||||
|
|
||||||
labels := handlerLabels(gatherMetrics(t, reg))
|
|
||||||
|
|
||||||
assert.Equal(
|
|
||||||
t,
|
|
||||||
map[string]struct{}{
|
|
||||||
middleware.UnmatchedRouteConst: {},
|
|
||||||
middleware.InflightHandlerConst: {},
|
|
||||||
},
|
|
||||||
labels,
|
|
||||||
"unmatched paths must collapse onto one sentinel, got %v",
|
|
||||||
keys(labels),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_InflightGaugeIsAggregateAndBalanced pins the one metric
|
|
||||||
// that cannot carry a pattern. It is incremented before routing and
|
|
||||||
// decremented after, so it gets a fixed label -- and the two calls
|
|
||||||
// must therefore agree, leaving the gauge at zero once the traffic
|
|
||||||
// has drained rather than stuck above it.
|
|
||||||
func TestMetrics_InflightGaugeIsAggregateAndBalanced(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
drivePaths(t, h, receiverPaths(metricsProbePaths))
|
|
||||||
|
|
||||||
var inflight []*dto.Metric
|
|
||||||
|
|
||||||
for _, fam := range gatherMetrics(t, reg) {
|
|
||||||
if strings.HasSuffix(fam.GetName(), "requests_inflight") {
|
|
||||||
inflight = fam.GetMetric()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
require.Len(
|
|
||||||
t, inflight, 1,
|
|
||||||
"the inflight gauge must hold exactly one series",
|
|
||||||
)
|
|
||||||
assert.Equal(
|
|
||||||
t, middleware.InflightHandlerConst,
|
|
||||||
labelValue(inflight[0], "handler"),
|
|
||||||
)
|
|
||||||
assert.InDelta(
|
|
||||||
t, 0.0, inflight[0].GetGauge().GetValue(), 0.0,
|
|
||||||
"the gauge must balance back to zero",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestMetrics_StatusAndSizeStillRecorded guards the response-writer
|
|
||||||
// interceptor the recording middleware wraps around every request.
|
|
||||||
// The handler label changed; what the interceptor reports must not
|
|
||||||
// have.
|
|
||||||
func TestMetrics_StatusAndSizeStillRecorded(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
h, reg := metricsTestRouter(t, generousReceiverLimit)
|
|
||||||
|
|
||||||
req := httptest.NewRequestWithContext(
|
|
||||||
t.Context(), http.MethodGet, okRoute, nil,
|
|
||||||
)
|
|
||||||
w := httptest.NewRecorder()
|
|
||||||
h.ServeHTTP(w, req)
|
|
||||||
|
|
||||||
require.Equal(t, http.StatusOK, w.Code)
|
|
||||||
require.Equal(t, okBody, w.Body.String())
|
|
||||||
|
|
||||||
var size *dto.Metric
|
|
||||||
|
|
||||||
for _, fam := range gatherMetrics(t, reg) {
|
|
||||||
if !strings.HasSuffix(fam.GetName(), "response_size_bytes") {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, m := range fam.GetMetric() {
|
|
||||||
if labelValue(m, "handler") == okRoute {
|
|
||||||
size = m
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
require.NotNil(
|
|
||||||
t, size, "no response size series for the static route",
|
|
||||||
)
|
|
||||||
assert.Equal(t, "200", labelValue(size, "code"))
|
|
||||||
assert.Equal(t, uint64(1), size.GetHistogram().GetSampleCount())
|
|
||||||
assert.InDelta(
|
|
||||||
t, float64(len(okBody)),
|
|
||||||
size.GetHistogram().GetSampleSum(), 0.0,
|
|
||||||
"the interceptor must still count written bytes",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
@@ -13,6 +13,9 @@ import (
|
|||||||
"github.com/go-chi/chi"
|
"github.com/go-chi/chi"
|
||||||
"github.com/go-chi/chi/middleware"
|
"github.com/go-chi/chi/middleware"
|
||||||
"github.com/go-chi/cors"
|
"github.com/go-chi/cors"
|
||||||
|
metrics "github.com/slok/go-http-metrics/metrics/prometheus"
|
||||||
|
ghmm "github.com/slok/go-http-metrics/middleware"
|
||||||
|
"github.com/slok/go-http-metrics/middleware/std"
|
||||||
"go.uber.org/fx"
|
"go.uber.org/fx"
|
||||||
"sneak.berlin/go/webhooker/internal/config"
|
"sneak.berlin/go/webhooker/internal/config"
|
||||||
"sneak.berlin/go/webhooker/internal/globals"
|
"sneak.berlin/go/webhooker/internal/globals"
|
||||||
@@ -26,15 +29,10 @@ const (
|
|||||||
// preflight response can be cached.
|
// preflight response can be cached.
|
||||||
corsMaxAge = 300
|
corsMaxAge = 300
|
||||||
|
|
||||||
// unmatchedRoute stands in for a request that matched no route
|
// unmatchedRoute is logged in the access log's url field when a
|
||||||
// pattern at all. Every byte of such a path is client-chosen, so
|
// redirected or rejected request matched no route pattern at
|
||||||
// none of it is kept.
|
// all. Every byte of such a path is client-chosen, so none of it
|
||||||
//
|
// is logged.
|
||||||
// It is the access log's url field on a redirected or rejected
|
|
||||||
// request, and it is the metrics `handler` label on the same
|
|
||||||
// request; see metrics.go. Both surfaces are written once per
|
|
||||||
// request from a path the client picks, so both have to collapse
|
|
||||||
// the unmatched case into one fixed value.
|
|
||||||
unmatchedRoute = "(unmatched)"
|
unmatchedRoute = "(unmatched)"
|
||||||
|
|
||||||
// redactedQuery stands in for the query string on the access log
|
// redactedQuery stands in for the query string on the access log
|
||||||
@@ -440,6 +438,17 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Metrics returns middleware that records Prometheus HTTP metrics.
|
||||||
|
func (s *Middleware) Metrics() func(http.Handler) http.Handler {
|
||||||
|
mdlw := ghmm.New(ghmm.Config{
|
||||||
|
Recorder: metrics.NewRecorder(metrics.Config{}),
|
||||||
|
})
|
||||||
|
|
||||||
|
return func(next http.Handler) http.Handler {
|
||||||
|
return std.Handler("", mdlw, next)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// MetricsAuth returns middleware that protects metrics endpoints
|
// MetricsAuth returns middleware that protects metrics endpoints
|
||||||
// with basic auth.
|
// with basic auth.
|
||||||
func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler {
|
func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler {
|
||||||
|
|||||||
@@ -11,7 +11,6 @@ import (
|
|||||||
)
|
)
|
||||||
|
|
||||||
const (
|
const (
|
||||||
repoRoot = "../.."
|
|
||||||
scriptPath = "../../script/version"
|
scriptPath = "../../script/version"
|
||||||
makefilePath = "../../Makefile"
|
makefilePath = "../../Makefile"
|
||||||
dockerfilePath = "../../Dockerfile"
|
dockerfilePath = "../../Dockerfile"
|
||||||
@@ -167,61 +166,6 @@ func TestMakefile_BuildComposesVersionAndExtraFlags(t *testing.T) {
|
|||||||
require.Contains(t, makefile, "VERSION ?= $(shell script/version)")
|
require.Contains(t, makefile, "VERSION ?= $(shell script/version)")
|
||||||
}
|
}
|
||||||
|
|
||||||
// A caller can define VERSION as the empty string -- `make build
|
|
||||||
// VERSION=`, or a `--build-arg VERSION=` reaching the Dockerfile's `make
|
|
||||||
// build VERSION="$VERSION"`. script/version's own guard does not cover
|
|
||||||
// that: the value never passes through the script. Stamping "" would
|
|
||||||
// leave the binary reporting no version and the footer on "dev", which
|
|
||||||
// is the defect this package exists for.
|
|
||||||
func TestMakefile_EmptyOverrideResolvesLikeAnUnsetOne(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
// A plain assignment would be ignored here: a command-line
|
|
||||||
// definition outranks it, and that is the case being corrected.
|
|
||||||
require.Contains(t, read(t, makefilePath), "override VERSION :=")
|
|
||||||
|
|
||||||
requireMake(t)
|
|
||||||
|
|
||||||
derived := makeVersion(t)
|
|
||||||
require.NotEmpty(t, derived)
|
|
||||||
|
|
||||||
require.Equal(t, derived, makeVersion(t, "VERSION="),
|
|
||||||
"an empty VERSION must resolve the way an unset one does")
|
|
||||||
require.Equal(t, "v9.9.9", makeVersion(t, "VERSION=v9.9.9"),
|
|
||||||
"the empty guard must not clobber a real override")
|
|
||||||
}
|
|
||||||
|
|
||||||
// makeVersion runs this repository's `version` target, which prints the
|
|
||||||
// value `make build` would stamp, with the given command-line
|
|
||||||
// definitions.
|
|
||||||
func makeVersion(t *testing.T, defs ...string) string {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
//nolint:gosec // fixed argv, arguments are test constants
|
|
||||||
cmd := exec.CommandContext(t.Context(), "make",
|
|
||||||
append([]string{"--no-print-directory", "version"}, defs...)...)
|
|
||||||
cmd.Dir = repoRoot
|
|
||||||
|
|
||||||
// Only the command-line definitions may decide the outcome: an
|
|
||||||
// inherited VERSION would change what an unset one resolves to, and
|
|
||||||
// an inherited MAKEFLAGS carries the parent's jobserver.
|
|
||||||
cmd.Env = append(os.Environ(), "VERSION=", "MAKEFLAGS=", "MAKELEVEL=")
|
|
||||||
|
|
||||||
out, err := cmd.CombinedOutput()
|
|
||||||
require.NoError(t, err, string(out))
|
|
||||||
|
|
||||||
return strings.TrimSpace(string(out))
|
|
||||||
}
|
|
||||||
|
|
||||||
func requireMake(t *testing.T) {
|
|
||||||
t.Helper()
|
|
||||||
|
|
||||||
_, err := exec.LookPath("make")
|
|
||||||
if err != nil {
|
|
||||||
t.Skipf("make is not installed: %v", err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Every compile in the image goes through the build target, so the
|
// Every compile in the image goes through the build target, so the
|
||||||
// static relink cannot replace the flags that carry the stamp.
|
// static relink cannot replace the flags that carry the stamp.
|
||||||
func TestDockerfile_BuildsThroughTheMakeTarget(t *testing.T) {
|
func TestDockerfile_BuildsThroughTheMakeTarget(t *testing.T) {
|
||||||
|
|||||||
@@ -39,7 +39,7 @@
|
|||||||
<div class="flex items-center gap-4">
|
<div class="flex items-center gap-4">
|
||||||
{{range .Deliveries}}
|
{{range .Deliveries}}
|
||||||
<span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">
|
<span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">
|
||||||
{{.Target.DisplayName}}: {{.Status}}
|
{{.Target.Name}}: {{.Status}}
|
||||||
</span>
|
</span>
|
||||||
{{end}}
|
{{end}}
|
||||||
<span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05"}}</span>
|
<span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05"}}</span>
|
||||||
@@ -74,7 +74,7 @@
|
|||||||
<div class="py-2" x-data="{ attempts: false }">
|
<div class="py-2" x-data="{ attempts: false }">
|
||||||
<div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
|
<div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
|
||||||
<div class="flex items-center gap-3">
|
<div class="flex items-center gap-3">
|
||||||
<span class="text-sm text-gray-700">{{.Target.DisplayName}}</span>
|
<span class="text-sm text-gray-700">{{.Target.Name}}</span>
|
||||||
<span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
|
<span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
|
||||||
</div>
|
</div>
|
||||||
<div class="flex items-center gap-3">
|
<div class="flex items-center gap-3">
|
||||||
|
|||||||
Reference in New Issue
Block a user