diff --git a/CHANGELOG.md b/CHANGELOG.md index f5a8531..4dff532 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.12.0] - 2026-08-03 + +### Added +- `PROXY_DIAL_TIMEOUT` sets the TCP connect timeout for upstream requests (record and proxy modes), default `30s` +- `PROXY_DIAL_ATTEMPTS` caps the total number of TCP dial attempts when the connection cannot be established at all, initial dial included, default `3`; set to `1` to disable retrying. Only a dial timeout is retried — other failures, connection refused among them, surface on the first attempt +- Both variables are resolved once at startup, so an invalid value fails the process at boot rather than on the first proxied request + +### Changed +- Dial timeout default raised from fasthttp's implicit 3s to 30s. Read and write timeouts remain unset, so long-running upstream responses are unaffected. + +### Fixed +- Upstream requests no longer fail after 3s when the upstream is briefly unreachable. The HTTP client was constructed as `&fasthttp.Client{}` with no `Dial`, so it fell back to `fasthttp.DefaultDialTimeout` (3s) with no retry — shorter than a rolling load-balancer or ingress update, during which SYNs go unanswered. Callers saw `502 {"error": "proxy error: error when dialing : dialing to the given TCP address timed out"}` while the upstream was healthy. The dial timeout is now explicit and a failed dial is retried. + ## [0.11.0] - 2026-04-02 ### Changed @@ -125,6 +138,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Initial release +[0.12.0]: https://github.com/gooddata/gooddata-goodmock/compare/v0.11.0...v0.12.0 [0.11.0]: https://github.com/gooddata/gooddata-goodmock/compare/v0.10.0...v0.11.0 [0.10.0]: https://github.com/gooddata/gooddata-goodmock/compare/v0.9.0...v0.10.0 [0.9.0]: https://github.com/gooddata/gooddata-goodmock/compare/v0.8.0...v0.9.0 diff --git a/README.md b/README.md index 8636c46..01aca13 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,8 @@ goodmock --version # print version | `PORT` | `8080` | all | Port to listen on | | `PROXY_HOST` | `http://localhost` | all | Upstream host (record: proxy target; replay: header rewriting) | | `REFERER_PATH` | `/` | all | App-specific path appended to `PROXY_HOST` for Referer header | +| `PROXY_DIAL_TIMEOUT` | `30s` | record, proxy | TCP connect timeout for upstream requests (Go duration, e.g. `10s`) | +| `PROXY_DIAL_ATTEMPTS` | `3` | record, proxy | Maximum total TCP dial attempts when the connection cannot be established, initial dial included; `1` disables retrying | | `MAPPINGS_DIR` | _(unset)_ | replay | Directory of JSON mapping files to load on startup | | `VERBOSE` | _(unset)_ | all | Log all request/response traffic (any value enables) | | `JSON_CONTENT_TYPES` | _(unset)_ | record | Additional Content-Types to store as structured JSON (see below) | @@ -77,6 +79,27 @@ goodmock --version # print version | `PRESERVE_JSON_KEY_ORDER` | _(unset)_ | record | Preserve original key order in JSON request and response bodies (any value enables) | | `SORT_ARRAY_MEMBERS` | _(unset)_ | record | Recursively sort JSON array elements by stringified value for deterministic diffs (any value enables) | +### Upstream Connection Handling + +Record and proxy modes dial the upstream with a `PROXY_DIAL_TIMEOUT` connect timeout and make at most `PROXY_DIAL_ATTEMPTS` dial attempts in total. The initial dial counts towards that budget, so the default of `3` means one dial plus up to two retries, and `1` disables retrying altogether. + +Both values are read once at startup, so an invalid setting fails the process at boot rather than on the first proxied request. + +The two settings compound: with the defaults a request can be held for `PROXY_DIAL_ATTEMPTS × PROXY_DIAL_TIMEOUT` = **90s** before the caller sees a 502, up from 3s previously. Two consequences worth knowing: + +- Most callers (e2e runners, browsers) have a shorter timeout of their own and will hit it first. From their side the failure mode changes from a fast 502 to their own client timeout. +- fasthttp's server does not propagate client disconnects into the handler, so GoodMock keeps dialing for the full budget on behalf of a caller that has already given up. The dialer's `Concurrency: 1000` caps how far that can pile up. + +Lower `PROXY_DIAL_ATTEMPTS` or `PROXY_DIAL_TIMEOUT` if a faster failure matters more than riding out a rolling upstream restart. + +A retry happens **only** when the dial fails with `fasthttp.ErrDialTimeout` — the upstream accepted no SYN within `PROXY_DIAL_TIMEOUT`. Other connection failures are **not** retried, including connection refused: those return an answer immediately, so they indicate something other than the rolling-restart window this budget exists to cover, and retrying would just repeat the same result. + +Retrying a timed-out dial is safe for non-idempotent methods such as `POST` because no bytes reached the upstream, so it cannot have acted on the request. Once a connection exists, no failure is retried, because there is no way to tell whether the upstream applied the request. + +When the upstream resolves to several addresses (a load balancer with one node per availability zone, for example), consecutive attempts tend to land on different ones: fasthttp advances a round-robin index over the resolved set once per dial, and abandons a dial as soon as an address times out instead of walking the remainder. This rotates over fasthttp's cached address set — the names are only re-resolved after its DNS cache duration (1 minute) expires. + +Read and write timeouts are intentionally unset: upstream responses may legitimately take tens of seconds, and capping them would convert slow-but-successful calls into proxy errors. + ### Loading Mappings on Startup Set `MAPPINGS_DIR` to a directory containing WireMock-format JSON files: diff --git a/VERSION b/VERSION index d9df1bb..d33c3a2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.11.0 +0.12.0 \ No newline at end of file diff --git a/internal/common/common.go b/internal/common/common.go index 3ad7c56..2d1c0bd 100644 --- a/internal/common/common.go +++ b/internal/common/common.go @@ -5,6 +5,7 @@ import ( "os" "strconv" "strings" + "time" ) func GetPort() int { @@ -21,6 +22,50 @@ func IsVerbose() bool { return os.Getenv("VERBOSE") != "" } +// ProxyDialTimeout returns the TCP connect timeout used when dialing the +// upstream. A fasthttp.Client with no Dial set falls back to +// fasthttp.DefaultDialTimeout, which is 3s — shorter than a rolling +// load-balancer or ingress update. While the balancer's targets are draining, +// SYNs go unanswered, the dial times out, and the caller gets a 502 even +// though the upstream itself is healthy. Record and proxy modes. +func ProxyDialTimeout() time.Duration { + if v := os.Getenv("PROXY_DIAL_TIMEOUT"); v != "" { + d, err := time.ParseDuration(v) + if err != nil { + log.Fatalf("Invalid PROXY_DIAL_TIMEOUT value: %s (expected a Go duration, e.g. 30s)", v) + } + if d <= 0 { + log.Fatalf("Invalid PROXY_DIAL_TIMEOUT value: %s (must be positive)", v) + } + return d + } + return 30 * time.Second +} + +// ProxyDialAttempts returns the maximum total number of times a request may be +// dialed when the TCP connection cannot be established at all. The initial dial +// counts towards the budget, so 3 permits one dial plus two retries and 1 +// disables retrying. Re-dialing is safe for every HTTP method: no bytes reached +// the upstream, so the request cannot have been applied. Record and proxy modes. +// +// Attempts also tend to land on different addresses when the upstream resolves +// to several — a load balancer with one node per availability zone, say. +// fasthttp advances a round-robin index over the resolved set once per dial, and +// it abandons a dial as soon as one address times out rather than walking the +// rest, so consecutive attempts start from consecutive addresses. Note this +// rotates over fasthttp's cached set; the addresses are only re-resolved once +// DNSCacheDuration (1 minute) has elapsed. +func ProxyDialAttempts() int { + if v := os.Getenv("PROXY_DIAL_ATTEMPTS"); v != "" { + n, err := strconv.Atoi(v) + if err != nil || n < 1 { + log.Fatalf("Invalid PROXY_DIAL_ATTEMPTS value: %s (expected an integer >= 1)", v) + } + return n + } + return 3 +} + // PreserveJSONKeyOrder returns true if JSON response body key order should be // preserved from the upstream server. When false (default), keys are sorted // alphabetically for deterministic diffs. Record mode only. diff --git a/internal/common/common_test.go b/internal/common/common_test.go new file mode 100644 index 0000000..e454c75 --- /dev/null +++ b/internal/common/common_test.go @@ -0,0 +1,54 @@ +package common + +import ( + "testing" + "time" +) + +func TestProxyDialTimeout(t *testing.T) { + tests := []struct { + name string + env string + want time.Duration + }{ + // The default must stay comfortably longer than fasthttp's own 3s + // DefaultDialTimeout, which is what this setting exists to override. + {name: "unset falls back to the default", env: "", want: 30 * time.Second}, + {name: "seconds", env: "5s", want: 5 * time.Second}, + {name: "sub-second", env: "250ms", want: 250 * time.Millisecond}, + {name: "compound", env: "1m30s", want: 90 * time.Second}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Always set it, empty included: an empty value is what the getter + // treats as unset, and setting it unconditionally stops a value + // inherited from the caller's environment from deciding the result. + t.Setenv("PROXY_DIAL_TIMEOUT", tt.env) + if got := ProxyDialTimeout(); got != tt.want { + t.Errorf("ProxyDialTimeout() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestProxyDialAttempts(t *testing.T) { + tests := []struct { + name string + env string + want int + }{ + {name: "unset falls back to the default", env: "", want: 3}, + {name: "one disables retrying", env: "1", want: 1}, + {name: "explicit value", env: "5", want: 5}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("PROXY_DIAL_ATTEMPTS", tt.env) + if got := ProxyDialAttempts(); got != tt.want { + t.Errorf("ProxyDialAttempts() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index b2b63b9..50a6c67 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -4,13 +4,60 @@ package proxy import ( "bufio" "bytes" + "errors" + "goodmock/internal/common" + "log" + "net" "strings" "github.com/valyala/fasthttp" ) +// Client is the upstream HTTP client together with the dial-attempt budget it +// was built with. +// +// Both settings are resolved once, in NewClient. Reading Attempts per request +// instead would defer the config check to the first proxied request: +// common.ProxyDialAttempts calls log.Fatalf on an invalid value, so the process +// would start, the pod would go Ready, and the proxy would then die on its first +// request rather than at boot. Pairing the two here also keeps them on one +// snapshot — the timeout is captured in the dialer closure, so re-reading only +// the attempts would let the two drift apart. +type Client struct { + HTTP *fasthttp.Client + Attempts int +} + +// NewClient builds the HTTP client used to reach the upstream. +// +// The dial timeout is set explicitly rather than left to fasthttp's 3s default; +// see common.ProxyDialTimeout for why. Read and write timeouts are deliberately +// left unset — upstream calls here include long polls that legitimately run for +// tens of seconds, and capping them would turn slow-but-fine responses into +// proxy errors. +// +// The dialer is owned by this client instead of using the package-level +// fasthttp.DialTimeout, which delegates to a process-global TCPDialer. Owning it +// keeps the DNS cache and the concurrency limiter scoped to this client rather +// than shared with anything else in the process that dials via fasthttp. +func NewClient() *Client { + timeout := common.ProxyDialTimeout() + attempts := common.ProxyDialAttempts() + // Concurrency mirrors fasthttp's own default dialer so that owning the + // dialer does not silently change how many dials may run at once. + dialer := &fasthttp.TCPDialer{Concurrency: 1000} + return &Client{ + HTTP: &fasthttp.Client{ + Dial: func(addr string) (net.Conn, error) { + return dialer.DialTimeout(addr, timeout) + }, + }, + Attempts: attempts, + } +} + // ProxyRequest forwards a request to the upstream server and returns the response details. -func ProxyRequest(client *fasthttp.Client, upstream string, ctx *fasthttp.RequestCtx) (int, map[string][]string, []byte, error) { +func ProxyRequest(client *Client, upstream string, ctx *fasthttp.RequestCtx) (int, map[string][]string, []byte, error) { req := fasthttp.AcquireRequest() resp := fasthttp.AcquireResponse() defer fasthttp.ReleaseRequest(req) @@ -34,7 +81,22 @@ func ProxyRequest(client *fasthttp.Client, upstream string, ctx *fasthttp.Reques req.SetBody(body) } - if err := client.Do(req, resp); err != nil { + // Retry only on a dial timeout. That error means the TCP connection was + // never established, so the upstream cannot have seen — let alone applied — + // the request, which makes a retry safe for every method including POST. + // Any other failure is returned as-is: once bytes are on the wire we have no + // way to know whether the upstream acted on them. + attempts := client.Attempts + var err error + for attempt := 1; ; attempt++ { + err = client.HTTP.Do(req, resp) + if err == nil || attempt >= attempts || !errors.Is(err, fasthttp.ErrDialTimeout) { + break + } + log.Printf("Upstream dial timed out (attempt %d/%d), retrying: %v", attempt, attempts, err) + resp.Reset() + } + if err != nil { return 0, nil, nil, err } diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go new file mode 100644 index 0000000..89a6d34 --- /dev/null +++ b/internal/proxy/proxy_test.go @@ -0,0 +1,120 @@ +// (C) 2026 GoodData Corporation +package proxy + +import ( + "errors" + "net" + "testing" + + "github.com/valyala/fasthttp" +) + +// dialCountingClient returns a Client whose dialer always fails with err, plus a +// pointer to the dial counter, so a test can assert how many dials a single +// ProxyRequest performed for a given attempt budget. +func dialCountingClient(attempts int, err error) (*Client, *int) { + dials := 0 + return &Client{ + HTTP: &fasthttp.Client{ + Dial: func(_ string) (net.Conn, error) { + dials++ + return nil, err + }, + }, + Attempts: attempts, + }, &dials +} + +// TestProxyRequestDialAttempts pins the Client.Attempts contract: the value is a +// budget of total dial attempts, initial dial included. +// +// The budget is set on the Client directly rather than through the environment. +// That it comes from PROXY_DIAL_ATTEMPTS is common.ProxyDialAttempts's contract +// (covered in common_test.go) and NewClient's (TestNewClientAttempts below); +// ProxyRequest only has to honour whatever budget it was handed. +// +// The method is varied on purpose. fasthttp's own HostClient retries idempotent +// requests (GET/HEAD/PUT) internally up to DefaultMaxIdemponentCallAttempts, so +// this also guards against that stacking on top of our loop and multiplying the +// dial count. +func TestProxyRequestDialAttempts(t *testing.T) { + tests := []struct { + name string + method string + attempts int + wantDials int + }{ + {name: "default budget, POST", method: "POST", attempts: 3, wantDials: 3}, + {name: "default budget, GET", method: "GET", attempts: 3, wantDials: 3}, + {name: "retrying disabled, POST", method: "POST", attempts: 1, wantDials: 1}, + {name: "retrying disabled, GET", method: "GET", attempts: 1, wantDials: 1}, + {name: "explicit budget, POST", method: "POST", attempts: 4, wantDials: 4}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client, dials := dialCountingClient(tt.attempts, fasthttp.ErrDialTimeout) + + ctx := &fasthttp.RequestCtx{} + ctx.Request.Header.SetMethod(tt.method) + ctx.Request.SetRequestURI("/api/thing") + + _, _, _, err := ProxyRequest(client, "http://upstream.invalid", ctx) + if !errors.Is(err, fasthttp.ErrDialTimeout) { + t.Fatalf("ProxyRequest() error = %v, want it to wrap fasthttp.ErrDialTimeout", err) + } + if *dials != tt.wantDials { + t.Errorf("dial count = %d, want %d", *dials, tt.wantDials) + } + }) + } +} + +// TestNewClientAttempts covers the wiring the table above no longer exercises: +// NewClient resolves PROXY_DIAL_ATTEMPTS once, at construction. That is what +// keeps an invalid value fatal at startup instead of on the first proxied +// request, and what keeps the attempt budget on the same snapshot as the dial +// timeout baked into the dialer. +func TestNewClientAttempts(t *testing.T) { + tests := []struct { + name string + env string + want int + }{ + {name: "default when unset", env: "", want: 3}, + {name: "value from environment", env: "5", want: 5}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("PROXY_DIAL_ATTEMPTS", tt.env) + + if got := NewClient().Attempts; got != tt.want { + t.Errorf("NewClient().Attempts = %d, want %d", got, tt.want) + } + }) + } +} + +// TestProxyRequestDoesNotRetryNonDialErrors makes sure the retry is scoped to +// dial failures. Once a connection exists we cannot know whether the upstream +// applied the request, so anything else must surface on the first attempt. +func TestProxyRequestDoesNotRetryNonDialErrors(t *testing.T) { + sentinel := errors.New("connection refused by test") + client, dials := dialCountingClient(3, sentinel) + + ctx := &fasthttp.RequestCtx{} + ctx.Request.Header.SetMethod("POST") + ctx.Request.SetRequestURI("/api/thing") + + _, _, _, err := ProxyRequest(client, "http://upstream.invalid", ctx) + if err == nil { + t.Fatal("ProxyRequest() error = nil, want the dialer's error") + } + if errors.Is(err, fasthttp.ErrDialTimeout) { + t.Fatalf("ProxyRequest() error = %v, should not be classified as a dial timeout", err) + } + if *dials != 1 { + t.Errorf("dial count = %d, want 1 — a non-dial-timeout error must not be retried", *dials) + } +} diff --git a/internal/pureproxy/pureproxy.go b/internal/pureproxy/pureproxy.go index 522984d..70b9583 100644 --- a/internal/pureproxy/pureproxy.go +++ b/internal/pureproxy/pureproxy.go @@ -18,14 +18,14 @@ import ( type ProxyServer struct { server *types.Server upstream string - client *fasthttp.Client + client *proxy.Client } func NewProxyServer(upstream, proxyHost, refererPath string, verbose bool) *ProxyServer { return &ProxyServer{ server: server.NewServer(proxyHost, refererPath, verbose, nil), upstream: upstream, - client: &fasthttp.Client{}, + client: proxy.NewClient(), } } diff --git a/internal/record/record.go b/internal/record/record.go index c674570..cdb8790 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -38,7 +38,7 @@ type RecordServer struct { mu sync.Mutex exchanges []RecordedExchange upstream string - client *fasthttp.Client + client *proxy.Client jsonContentTypes []string binaryContentTypes []string preserveKeyOrder bool @@ -51,7 +51,7 @@ func NewRecordServer(upstream, proxyHost, refererPath string, verbose bool, json server: server.NewServer(proxyHost, refererPath, verbose, nil), exchanges: make([]RecordedExchange, 0), upstream: upstream, - client: &fasthttp.Client{}, + client: proxy.NewClient(), jsonContentTypes: jsonContentTypes, binaryContentTypes: binaryContentTypes, preserveKeyOrder: preserveKeyOrder,