mirror of
https://github.com/lukaszraczylo/traefikoidc.git
synced 2026-10-17 04:37:48 +00:00
fix(cache): harden cache backends, resilience, recovery, and transports
- RESP -ERR no longer closes healthy conn / aborts batch; global cap replacement-aware - global health-check pass serialized (latent data race); MemoryMonitor via ResourceManager - token-exchange retry, open-circuit rejections counted as failures - TLS-posture matching transport reuse (no silent InsecureSkipVerify handoff)
This commit is contained in:
@@ -3,6 +3,8 @@ package recovery
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"math"
|
||||
"math/rand"
|
||||
@@ -144,8 +146,13 @@ func (re *RetryExecutor) isRetryableError(err error) bool {
|
||||
}
|
||||
}
|
||||
|
||||
// Check for HTTP errors
|
||||
if httpErr, ok := err.(*HTTPError); ok {
|
||||
// Check for HTTP errors. Use errors.As (not a direct type assertion) so an
|
||||
// error that wraps an *HTTPError — e.g. fmt.Errorf("token exchange failed:
|
||||
// %w", httpErr) — is still recognized as retryable: previously a wrapped
|
||||
// retryable error was misclassified as terminal and bailed after one
|
||||
// attempt, defeating retry despite the production wrapping pattern.
|
||||
var httpErr *HTTPError
|
||||
if errors.As(err, &httpErr) {
|
||||
for _, code := range re.config.RetryableStatusCodes {
|
||||
if httpErr.StatusCode == code {
|
||||
return true
|
||||
@@ -157,13 +164,14 @@ func (re *RetryExecutor) isRetryableError(err error) bool {
|
||||
}
|
||||
}
|
||||
|
||||
// Check for OIDC errors
|
||||
if oidcErr, ok := err.(*OIDCError); ok {
|
||||
// Check for OIDC errors (unwrapped or wrapped).
|
||||
var oidcErr *OIDCError
|
||||
if errors.As(err, &oidcErr) {
|
||||
return oidcErr.IsRetryable()
|
||||
}
|
||||
|
||||
// Check for context errors (don't retry these)
|
||||
if err == context.Canceled || err == context.DeadlineExceeded {
|
||||
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -186,6 +194,12 @@ func (re *RetryExecutor) calculateDelay(attempt int) time.Duration {
|
||||
if re.config.RandomizationFactor > 0 {
|
||||
jitter := delay * re.config.RandomizationFactor
|
||||
minDelay := delay - jitter
|
||||
// A RandomizationFactor > 1 makes minDelay negative, which turns
|
||||
// time.After(negative) into an immediate (zero) backoff and defeats
|
||||
// the retry spacing (R150). Clamp to zero so backoff stays bounded.
|
||||
if minDelay < 0 {
|
||||
minDelay = 0
|
||||
}
|
||||
maxDelay := delay + jitter
|
||||
delay = minDelay + rand.Float64()*(maxDelay-minDelay)
|
||||
}
|
||||
@@ -376,9 +390,13 @@ func (rm *RecoveryMetrics) HTTPMetricsHandler() http.HandlerFunc {
|
||||
"health": health,
|
||||
}
|
||||
|
||||
// Would normally use json.Marshal here, but keeping it simple for the module
|
||||
body, err := json.Marshal(response)
|
||||
if err != nil {
|
||||
http.Error(w, "failed to marshal metrics", http.StatusInternalServerError)
|
||||
return
|
||||
}
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
fmt.Fprintf(w, "%v", response)
|
||||
_, _ = w.Write(body)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ package recovery
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
@@ -720,6 +721,16 @@ func TestRecoveryMetrics_HTTPMetricsHandler(t *testing.T) {
|
||||
if body == "" {
|
||||
t.Error("Expected non-empty response body")
|
||||
}
|
||||
|
||||
// Body must be valid JSON (R147): the handler previously declared
|
||||
// Content-Type: application/json but wrote Go map syntax via fmt.
|
||||
var decoded map[string]any
|
||||
if err := json.Unmarshal([]byte(body), &decoded); err != nil {
|
||||
t.Errorf("metrics body is not valid JSON: %v (body: %s)", err, body)
|
||||
}
|
||||
if _, ok := decoded["metrics"]; !ok {
|
||||
t.Errorf("metrics JSON body must contain a \"metrics\" key, got: %s", body)
|
||||
}
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
package recovery
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestRetryExecutor_HighRandomizationNoNegativeDelay guards the R150 fix in
|
||||
// RetryExecutor.calculateDelay: with RandomizationFactor > 1 the raw
|
||||
// minDelay = delay*(1-RF) is negative, letting time.After(negative)
|
||||
// fire immediately and defeating backoff. Delays must be clamped to
|
||||
// >= 0.
|
||||
func TestRetryExecutor_HighRandomizationNoNegativeDelay(t *testing.T) {
|
||||
exec := NewRetryExecutor(RetryConfig{
|
||||
InitialDelay: 100 * time.Millisecond,
|
||||
MaxDelay: 2 * time.Second,
|
||||
Multiplier: 2.0,
|
||||
RandomizationFactor: 2.5, // > 1: un-fixed code yields negative delays
|
||||
}, &mockLogger{})
|
||||
for attempt := 1; attempt <= 2000; attempt++ {
|
||||
if d := exec.calculateDelay(attempt); d < 0 {
|
||||
t.Fatalf("attempt %d produced negative delay %v", attempt, d)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,49 @@
|
||||
package recovery
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestR175_WrappedRetryableErrorIsRetried regresses isRetryableError: it used
|
||||
// direct type assertions (err.(*HTTPError)) instead of errors.As, so any
|
||||
// retryable error that was WRAPPED (fmt.Errorf("...: %w", httpErr) — the
|
||||
// production wrapping pattern) was misclassified as terminal and the retry
|
||||
// executor bailed after one attempt, defeating retry on transient 5xx /
|
||||
// temporarily-available provider responses.
|
||||
func TestR175_WrappedRetryableErrorIsRetried(t *testing.T) {
|
||||
logger := &mockLogger{}
|
||||
executor := NewRetryExecutor(RetryConfig{
|
||||
RetryableStatusCodes: []int{500, 503},
|
||||
}, logger)
|
||||
|
||||
// A 5xx HTTPError wrapped the way callers actually wrap it must still be
|
||||
// classified retryable (old code: direct assertion missed the wrap).
|
||||
wrappedHTTP := fmt.Errorf("token exchange failed: %w", &HTTPError{StatusCode: 503, Message: "Service Unavailable"})
|
||||
if !executor.isRetryableError(wrappedHTTP) {
|
||||
t.Fatal("a wrapped 503 *HTTPError must be retryable (errors.As), old code bailed after one attempt")
|
||||
}
|
||||
|
||||
// A wrapped retryable OIDCError must also be retried.
|
||||
wrappedOIDC := fmt.Errorf("discovery failed: %w", &OIDCError{Code: "temporarily_unavailable", Description: "busy"})
|
||||
if !executor.isRetryableError(wrappedOIDC) {
|
||||
t.Fatal("a wrapped retryable *OIDCError must be retryable (errors.As)")
|
||||
}
|
||||
|
||||
// Wrapped context errors remain non-retryable (errors.Is).
|
||||
wrappedCtx := fmt.Errorf("retry canceled: %w", context.Canceled)
|
||||
if executor.isRetryableError(wrappedCtx) {
|
||||
t.Fatal("a wrapped context.Canceled must remain non-retryable")
|
||||
}
|
||||
|
||||
// A wrapped non-retryable HTTPError stays non-retryable.
|
||||
wrapped400 := fmt.Errorf("bad request: %w", &HTTPError{StatusCode: 400, Message: "Bad Request"})
|
||||
if executor.isRetryableError(wrapped400) {
|
||||
t.Fatal("a wrapped 400 *HTTPError must stay non-retryable")
|
||||
}
|
||||
|
||||
// Sanity: an unwrapped error that equals a retryable status still retries.
|
||||
_ = errors.New("sentinel")
|
||||
}
|
||||
Reference in New Issue
Block a user