An invalid signature template passes startup and then crashes the process on the first delivery. There is no recover on the delivery path, so the panic takes the process down — and since the event is still queued, it dies again on restart.
Two distinct classes of invalid template reach this, and they fail for different reasons.
Class 1: the template doesn't parse
NewSignatureFormatter panics on a parse error. It's called from CreatePublisher, which runs when a destination first receives an event — not at startup. So a malformed template is accepted by a process that reports healthy, and detonates later.
Easy to hit in practice, because escaping is transport-dependent. A value carrying literal backslashes into the template — common when set from a .env file, a compose environment: block, or a plain YAML scalar — is a parse error:
template: signature:1: unexpected "\\" in operand
Class 2: the template parses but references a field the payload doesn't have
The two templates render against different types. SignaturePayload has .EventID, .Topic, .Timestamp, .Body. HeaderPayload has .EventID, .Topic, .Timestamp, .Signatures — no .Body.
Borrowing a field from the wrong one is valid syntax, so construction accepts it:
DESTINATIONS_WEBHOOK_SIGNATURE_CONTENT_TEMPLATE="v1:{{.Timestamp.Unix}}:{{.Signatures | join \",\"}}"
Startup succeeds. Then, on the first event:
PANIC at Format(): signature content template execution failed:
template: signature:1:25: executing "signature" at <.Signatures>:
can't evaluate field Signatures in type destwebhook.SignaturePayload
Parsing validates syntax; it cannot validate field references, which are only resolved against a concrete value at execution. So construction-time checking cannot catch this class — it needs an actual render.
Format panics on the execution error, under a comment asserting this can't happen:
// Template was validated at construction time, so execution errors
// indicate a bug (e.g., nil field). Panic to surface it immediately.
That assumption is wrong, and it's what turns a config typo into a crash.
Proposed change
Validate at startup, in internal/config
The config layer already has a validation framework (internal/config/validation.go) and already imports the providers package, so building the formatters there introduces no new dependency direction.
Construct both formatters and dry-run each against a synthetic payload. Class 1 fails at construction; class 2 fails at the render, because field resolution depends on the payload's type rather than its values — {{.Signatures}} against a SignaturePayload produces the same error whatever the values are. Both become startup errors naming the offending template.
Use realistic shape rather than zero values — an empty .Body or a nil slice passes things that real data wouldn't:
SignaturePayload{
EventID: "evt_validation",
Topic: "validation.topic",
Timestamp: time.Now(),
Body: `{"validation":true}`,
}
HeaderPayload{
EventID: "evt_validation",
Topic: "validation.topic",
Timestamp: time.Now(),
Signatures: []string{"sig_current", "sig_previous"},
}
Two signatures rather than one, since rotation is when .Signatures holds more than one element and when a template that mishandles the list would show it.
Stop panicking in Format
No synthetic payload catches everything. Value-dependent failures slip through in both directions: {{index .Signatures 2}} is valid with three secrets configured but fails a two-element mock, and a helper that errors on particular input passes the mock and fails on real data. A mock permissive enough to avoid rejecting valid templates cannot also prove them.
So the dry run is a smoke test, not a proof — and the cases it misses must degrade to a failed delivery rather than a dead process.
The path for this already exists and is only blocked by the panic. GenerateSignatureHeader is called from WebhookPublisher.Format, and Publish already handles a Format failure:
httpReq, err := p.Format(ctx, event)
if err != nil {
return destregistry.NewFormatError("webhook", "", err)
}
NewFormatError is documented for exactly this case — "formatting an event fails before it can be sent (e.g. an invalid key/partition template or an unparseable payload)" — and produces a failed attempt with the cause recorded, acked rather than nacked. So this is a matter of returning the error instead of panicking and letting the existing path carry it.
Worth deciding: retry behaviour
NewFormatError returns an ErrDestinationPublishAttempt, which shouldScheduleRetry treats as retryable. A template error is deterministic and will fail identically on every attempt, so it consumes the whole retry budget to no purpose.
Pre-existing behaviour for all format errors rather than something introduced here, but this makes it visible. Marking deterministic format failures as terminal would be a separate, larger change.
Related
An invalid signature template passes startup and then crashes the process on the first delivery. There is no
recoveron the delivery path, so the panic takes the process down — and since the event is still queued, it dies again on restart.Two distinct classes of invalid template reach this, and they fail for different reasons.
Class 1: the template doesn't parse
NewSignatureFormatterpanics on a parse error. It's called fromCreatePublisher, which runs when a destination first receives an event — not at startup. So a malformed template is accepted by a process that reports healthy, and detonates later.Easy to hit in practice, because escaping is transport-dependent. A value carrying literal backslashes into the template — common when set from a
.envfile, a composeenvironment:block, or a plain YAML scalar — is a parse error:Class 2: the template parses but references a field the payload doesn't have
The two templates render against different types.
SignaturePayloadhas.EventID,.Topic,.Timestamp,.Body.HeaderPayloadhas.EventID,.Topic,.Timestamp,.Signatures— no.Body.Borrowing a field from the wrong one is valid syntax, so construction accepts it:
DESTINATIONS_WEBHOOK_SIGNATURE_CONTENT_TEMPLATE="v1:{{.Timestamp.Unix}}:{{.Signatures | join \",\"}}"Startup succeeds. Then, on the first event:
Parsing validates syntax; it cannot validate field references, which are only resolved against a concrete value at execution. So construction-time checking cannot catch this class — it needs an actual render.
Formatpanics on the execution error, under a comment asserting this can't happen:That assumption is wrong, and it's what turns a config typo into a crash.
Proposed change
Validate at startup, in
internal/configThe config layer already has a validation framework (
internal/config/validation.go) and already imports the providers package, so building the formatters there introduces no new dependency direction.Construct both formatters and dry-run each against a synthetic payload. Class 1 fails at construction; class 2 fails at the render, because field resolution depends on the payload's type rather than its values —
{{.Signatures}}against aSignaturePayloadproduces the same error whatever the values are. Both become startup errors naming the offending template.Use realistic shape rather than zero values — an empty
.Bodyor a nil slice passes things that real data wouldn't:Two signatures rather than one, since rotation is when
.Signaturesholds more than one element and when a template that mishandles the list would show it.Stop panicking in
FormatNo synthetic payload catches everything. Value-dependent failures slip through in both directions:
{{index .Signatures 2}}is valid with three secrets configured but fails a two-element mock, and a helper that errors on particular input passes the mock and fails on real data. A mock permissive enough to avoid rejecting valid templates cannot also prove them.So the dry run is a smoke test, not a proof — and the cases it misses must degrade to a failed delivery rather than a dead process.
The path for this already exists and is only blocked by the panic.
GenerateSignatureHeaderis called fromWebhookPublisher.Format, andPublishalready handles aFormatfailure:NewFormatErroris documented for exactly this case — "formatting an event fails before it can be sent (e.g. an invalid key/partition template or an unparseable payload)" — and produces a failed attempt with the cause recorded, acked rather than nacked. So this is a matter of returning the error instead of panicking and letting the existing path carry it.Worth deciding: retry behaviour
NewFormatErrorreturns anErrDestinationPublishAttempt, whichshouldScheduleRetrytreats as retryable. A template error is deterministic and will fail identically on every attempt, so it consumes the whole retry budget to no purpose.Pre-existing behaviour for all format errors rather than something introduced here, but this makes it visible. Marking deterministic format failures as terminal would be a separate, larger change.
Related