From cf5ef8793441d8686f9c42a8f292e7d6ed3f07e5 Mon Sep 17 00:00:00 2001 From: tubt Date: Mon, 3 Aug 2026 16:01:06 +0700 Subject: [PATCH 1/2] fix: Set explicit upstream dial timeout and retry failed dials Record and proxy modes built their upstream client as &fasthttp.Client{} with no Dial set, so fasthttp fell back to its DefaultDialTimeout of 3s and never retried. That is shorter than a rolling load-balancer or ingress update: while the balancer's targets drain, SYNs go unanswered, the dial times out, and the caller gets a 502 even though the upstream is healthy. The 502 body is fasthttp's ErrDialTimeout wrapped by ErrDialWithUpstream, surfaced through goodmock's proxy-error handler. Add two settings, both applying to record and proxy modes: - PROXY_DIAL_TIMEOUT sets the TCP connect timeout, default 30s. - PROXY_DIAL_ATTEMPTS caps the total number of dial attempts, initial dial included, default 3. Setting it to 1 disables retrying. Retrying is scoped to fasthttp.ErrDialTimeout and nothing else. That error means the TCP connection was never established, so the upstream cannot have seen or applied the request, which makes the retry safe for non-idempotent methods too. Once bytes are on the wire nothing is retried, because there is no way to know whether the upstream acted on them. errors.Is reaches the sentinel through ErrDialWithUpstream, which implements Unwrap. The dialer is owned by the client rather than using the package-level fasthttp.DialTimeout, which delegates to a process-global TCPDialer. Owning it scopes the DNS cache and the concurrency limiter to this client instead of sharing them with anything else in the process that dials via fasthttp. Concurrency is set to 1000 to match fasthttp's own default so that taking ownership changes nothing else. Read and write timeouts stay unset. Upstream responses here can legitimately take tens of seconds, and capping them would convert slow-but-successful calls into proxy errors. Note on multiple upstream addresses: attempts tend to land on different ones because getTCPAddrs advances a round-robin index once per dial and dial() abandons the walk as soon as an address times out rather than trying the rest. This rotates over fasthttp's cached set; names are only re-resolved once DNSCacheDuration (1 minute) has elapsed. Tests cover the configuration getters and the retry contract with a counting dialer: the attempt budget is total, verified at 3, 1 and 4; the count holds for GET as well as POST, which guards against fasthttp's own idempotent-retry loop stacking on top and multiplying the dials; and a non-dial-timeout error is neither retried nor misclassified as a dial timeout. The default-case getter tests set their variables unconditionally so an inherited environment cannot decide the result. --- CHANGELOG.md | 41 +++++++-------- README.md | 12 +++++ VERSION | 2 +- internal/common/common.go | 45 +++++++++++++++++ internal/common/common_test.go | 54 ++++++++++++++++++++ internal/proxy/proxy.go | 45 ++++++++++++++++- internal/proxy/proxy_test.go | 89 +++++++++++++++++++++++++++++++++ internal/pureproxy/pureproxy.go | 2 +- internal/record/record.go | 2 +- 9 files changed, 265 insertions(+), 27 deletions(-) create mode 100644 internal/common/common_test.go create mode 100644 internal/proxy/proxy_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f5a8531..628d931 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,18 +1,26 @@ # Changelog - 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.11.0] - 2026-04-02 +## [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 +### 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 - `X-GDC-TRACE-ID` response header is now preserved in proxy and record modes (previously stripped along with all `X-GDC*` headers) - In record mode, `X-GDC-TRACE-ID` is only forwarded to the client — it is still excluded from saved mappings to keep recordings clean ## [0.10.0] - 2026-03-18 - ### Changed - Moved replay mode into its own `internal/replay` package, matching the pattern of `record` and `pureproxy` - Each mode now has its own admin handler (`handleReplayAdmin`, `handleRecordAdmin`) — admin endpoints are scoped to the mode where they make sense instead of sharing a single catch-all handler @@ -24,12 +32,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Record mode `/__admin/reset` no longer calls `ClearMappings` (mappings are never loaded in record mode) ## [0.9.0] - 2026-03-15 - ### Added - `install.sh` script for downloading and installing prebuilt binaries with checksum verification ## [0.8.0] - 2026-03-15 - ### Added - Cross-platform binary releases attached to GitHub Releases (linux/amd64, linux/arm64, darwin/amd64, darwin/arm64, windows/amd64, windows/arm64) with SHA-256 checksums @@ -37,23 +43,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Binaries are now built with `-ldflags="-s -w"` to strip debug symbols and reduce binary size ## [0.7.0] - 2026-03-15 - ### Added - `--version` / `-v` flag to print the embedded version and exit - Version is now embedded in the binary at compile time via `//go:embed VERSION` ## [0.6.0] - 2026-03-10 - ### Added - `BINARY_CONTENT_TYPES` environment variable (record + replay) — comma-separated list of Content-Types whose response bodies should be stored as base64-encoded strings in the `body` field. In replay mode, responses with matching Content-Types are automatically base64-decoded before serving. ## [0.5.1] - 2026-02-20 - ### Changed - Increased default maximum request body size to 16MB ## [0.5.0] - 2026-02-11 - ### Added - `application/json` response bodies are now always stored as structured JSON (`jsonBody`) instead of escaped strings, improving diffability of mapping files - `JSON_CONTENT_TYPES` environment variable (record mode) — comma-separated list of additional Content-Types to also store as `jsonBody` (e.g. `application/vnd.gooddata.api+json`) @@ -62,21 +64,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `SORT_ARRAY_MEMBERS` environment variable (record mode) — when set, recursively sorts JSON array elements by their stringified value (bottom-up) in both request and response bodies, eliminating diffs caused by non-deterministic array ordering from upstream - Recorded mappings are now sorted deterministically by name (with method + URL + query params + body as tiebreaker for duplicate names), eliminating spurious diffs caused by request-arrival ordering -### Removed -- Removed `id` and `uuid` fields from recorded mappings — they were random UUIDs that caused spurious diffs on every re-record and were never used for matching or lookup - ### Changed - **Breaking (output only):** Mapping files produced by record mode are no longer WireMock-compatible as of v0.5.0 — `application/json` responses use `jsonBody` (structured JSON) instead of `body` (escaped strings), and mappings omit `id`/`uuid` fields. Replay mode remains fully backwards-compatible: old WireMock-format mapping files (with `body` strings and `id`/`uuid` fields) still load and work without changes. The admin API also remains WireMock-compatible. - **Breaking (key order):** JSON keys in both request bodies (`equalToJson`) and response bodies (`jsonBody`) are now sorted alphabetically by default for deterministic diffs. If your consumers rely on original key ordering from the upstream server, set `PRESERVE_JSON_KEY_ORDER=true` to restore the previous behaviour. - `VERBOSE` environment variable now accepts any non-empty value (previously required `true`, `1`, or `yes`) -## [0.4.0] - 2026-02-11 +### Removed +- Removed `id` and `uuid` fields from recorded mappings — they were random UUIDs that caused spurious diffs on every re-record and were never used for matching or lookup +## [0.4.0] - 2026-02-11 ### Added - Proxy mode (`goodmock proxy`) — forwards all traffic to upstream without recording, applying the same header transformations and response filtering as record mode ## [0.3.2] - 2026-02-11 - ### Changed - Split monolithic `main` package into `internal/` sub-packages: `types`, `server`, `record`, `matching`, `logging`, `proxy`, `common` - Exported Server struct fields (`Mappings`, `ProxyHost`, `RefererPath`, `Verbose`, `Mu`) for cross-package access @@ -84,7 +84,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Moved shared helpers (`GetPort`, `IsVerbose`) into `internal/common` ## [0.3.1] - 2026-02-11 - ### Changed - Refactored codebase from OOP style to functional style — all Server and RecordServer methods converted to free functions - Pure functions (applyResponseHeaders, evaluateMapping, logMismatch, logVerboseRequest, transformRequestHeaders) no longer take a server receiver @@ -92,7 +91,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Request handlers use closures instead of method values ## [0.3.0] - 2026-02-10 - ### Added - Record mode (`goodmock record`) — proxies to upstream and captures request/response pairs - `/__admin/recordings/snapshot` endpoint with URL pattern filtering and scenario support @@ -101,30 +99,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `VERBOSE` environment variable for full request/response traffic logging ## [0.2.1] - 2026-02-10 - ### Changed - Added mode as primary CLI arg (`goodmock replay`), defaults to `replay` - Replaced `-port` flag with `PORT` environment variable (default: 8080) ## [0.2.0] - 2026-02-10 - -### Fixed -- URL path matching now preserves percent-encoding (e.g. `%3A`) by using raw request URI instead of fasthttp's decoded path - ### Added - Request header rewriting (Origin, Referer, Accept-Encoding) to match recorded stubs - `REFERER_PATH` environment variable for app-specific Referer header path -## [0.1.1] - 2026-02-10 +### Fixed +- URL path matching now preserves percent-encoding (e.g. `%3A`) by using raw request URI instead of fasthttp's decoded path +## [0.1.1] - 2026-02-10 ### Changed - Refactor and minor improvements ## [0.1.0] - 2026-02-09 - ### 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..b35e8ff 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,16 @@ 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. + +A retry happens **only** when the TCP connection could not be established. At that point no bytes have reached the upstream, so it cannot have acted on the request — which is what makes retrying safe for non-idempotent methods such as `POST`. 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..ac454c6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.11.0 +0.12.0 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..a369525 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -4,11 +4,39 @@ package proxy import ( "bufio" "bytes" + "errors" + "goodmock/internal/common" + "log" + "net" "strings" "github.com/valyala/fasthttp" ) +// 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() *fasthttp.Client { + timeout := common.ProxyDialTimeout() + // 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 &fasthttp.Client{ + Dial: func(addr string) (net.Conn, error) { + return dialer.DialTimeout(addr, timeout) + }, + } +} + // 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) { req := fasthttp.AcquireRequest() @@ -34,7 +62,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 := common.ProxyDialAttempts() + var err error + for attempt := 1; ; attempt++ { + err = client.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..e2d7bf5 --- /dev/null +++ b/internal/proxy/proxy_test.go @@ -0,0 +1,89 @@ +// (C) 2026 GoodData Corporation +package proxy + +import ( + "errors" + "net" + "testing" + + "github.com/valyala/fasthttp" +) + +// TestProxyRequestDialAttempts pins the PROXY_DIAL_ATTEMPTS contract: the value +// is a budget of total dial attempts, initial dial included. +// +// 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 + env string + wantDials int + }{ + {name: "default budget, POST", method: "POST", env: "", wantDials: 3}, + {name: "default budget, GET", method: "GET", env: "", wantDials: 3}, + {name: "retrying disabled, POST", method: "POST", env: "1", wantDials: 1}, + {name: "retrying disabled, GET", method: "GET", env: "1", wantDials: 1}, + {name: "explicit budget, POST", method: "POST", env: "4", wantDials: 4}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("PROXY_DIAL_ATTEMPTS", tt.env) + + dials := 0 + client := &fasthttp.Client{ + Dial: func(_ string) (net.Conn, error) { + dials++ + return nil, 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) + } + }) + } +} + +// 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) { + t.Setenv("PROXY_DIAL_ATTEMPTS", "3") + + sentinel := errors.New("connection refused by test") + dials := 0 + client := &fasthttp.Client{ + Dial: func(_ string) (net.Conn, error) { + dials++ + return nil, 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..08e8cbb 100644 --- a/internal/pureproxy/pureproxy.go +++ b/internal/pureproxy/pureproxy.go @@ -25,7 +25,7 @@ func NewProxyServer(upstream, proxyHost, refererPath string, verbose bool) *Prox 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..5b40147 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -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, From d8bd1ad99a0105b12dc4054cf87e1f53eadb8c44 Mon Sep 17 00:00:00 2001 From: tubt Date: Mon, 10 Aug 2026 10:40:48 +0700 Subject: [PATCH 2/2] fix: Resolve dial attempts at client construction Address review feedback on #20. common.ProxyDialAttempts() was re-read on every proxied request. It calls log.Fatalf on an invalid value, so a bad PROXY_DIAL_ATTEMPTS passed startup, the pod went Ready, and the proxy then died on its first request. It also left the two dial settings on different snapshots: the timeout was captured in NewClient's dialer closure while the attempts were re-read per request. Both are now resolved once in NewClient, which returns a proxy.Client bundling the fasthttp client with its attempt budget. The retry-loop tests set the budget on the Client directly instead of through t.Setenv; TestNewClientAttempts covers the environment wiring they no longer exercise. Docs: state the retry is scoped to a dial timeout (connection refused is not retried), and spell out the compound attempts x timeout worst case with its two consequences for callers. Revert the unrelated CHANGELOG reformatting - blank lines after old version headings and section reordering within 0.5.0 and 0.2.0. --- CHANGELOG.md | 33 +++++++++--- README.md | 13 ++++- VERSION | 2 +- internal/proxy/proxy.go | 33 +++++++++--- internal/proxy/proxy_test.go | 91 ++++++++++++++++++++++----------- internal/pureproxy/pureproxy.go | 2 +- internal/record/record.go | 2 +- 7 files changed, 128 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 628d931..4dff532 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,16 @@ # Changelog + 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 +- `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. @@ -16,11 +19,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 - `X-GDC-TRACE-ID` response header is now preserved in proxy and record modes (previously stripped along with all `X-GDC*` headers) - In record mode, `X-GDC-TRACE-ID` is only forwarded to the client — it is still excluded from saved mappings to keep recordings clean ## [0.10.0] - 2026-03-18 + ### Changed - Moved replay mode into its own `internal/replay` package, matching the pattern of `record` and `pureproxy` - Each mode now has its own admin handler (`handleReplayAdmin`, `handleRecordAdmin`) — admin endpoints are scoped to the mode where they make sense instead of sharing a single catch-all handler @@ -32,10 +37,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Record mode `/__admin/reset` no longer calls `ClearMappings` (mappings are never loaded in record mode) ## [0.9.0] - 2026-03-15 + ### Added - `install.sh` script for downloading and installing prebuilt binaries with checksum verification ## [0.8.0] - 2026-03-15 + ### Added - Cross-platform binary releases attached to GitHub Releases (linux/amd64, linux/arm64, darwin/amd64, darwin/arm64, windows/amd64, windows/arm64) with SHA-256 checksums @@ -43,19 +50,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Binaries are now built with `-ldflags="-s -w"` to strip debug symbols and reduce binary size ## [0.7.0] - 2026-03-15 + ### Added - `--version` / `-v` flag to print the embedded version and exit - Version is now embedded in the binary at compile time via `//go:embed VERSION` ## [0.6.0] - 2026-03-10 + ### Added - `BINARY_CONTENT_TYPES` environment variable (record + replay) — comma-separated list of Content-Types whose response bodies should be stored as base64-encoded strings in the `body` field. In replay mode, responses with matching Content-Types are automatically base64-decoded before serving. ## [0.5.1] - 2026-02-20 + ### Changed - Increased default maximum request body size to 16MB ## [0.5.0] - 2026-02-11 + ### Added - `application/json` response bodies are now always stored as structured JSON (`jsonBody`) instead of escaped strings, improving diffability of mapping files - `JSON_CONTENT_TYPES` environment variable (record mode) — comma-separated list of additional Content-Types to also store as `jsonBody` (e.g. `application/vnd.gooddata.api+json`) @@ -64,19 +75,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `SORT_ARRAY_MEMBERS` environment variable (record mode) — when set, recursively sorts JSON array elements by their stringified value (bottom-up) in both request and response bodies, eliminating diffs caused by non-deterministic array ordering from upstream - Recorded mappings are now sorted deterministically by name (with method + URL + query params + body as tiebreaker for duplicate names), eliminating spurious diffs caused by request-arrival ordering +### Removed +- Removed `id` and `uuid` fields from recorded mappings — they were random UUIDs that caused spurious diffs on every re-record and were never used for matching or lookup + ### Changed - **Breaking (output only):** Mapping files produced by record mode are no longer WireMock-compatible as of v0.5.0 — `application/json` responses use `jsonBody` (structured JSON) instead of `body` (escaped strings), and mappings omit `id`/`uuid` fields. Replay mode remains fully backwards-compatible: old WireMock-format mapping files (with `body` strings and `id`/`uuid` fields) still load and work without changes. The admin API also remains WireMock-compatible. - **Breaking (key order):** JSON keys in both request bodies (`equalToJson`) and response bodies (`jsonBody`) are now sorted alphabetically by default for deterministic diffs. If your consumers rely on original key ordering from the upstream server, set `PRESERVE_JSON_KEY_ORDER=true` to restore the previous behaviour. - `VERBOSE` environment variable now accepts any non-empty value (previously required `true`, `1`, or `yes`) -### Removed -- Removed `id` and `uuid` fields from recorded mappings — they were random UUIDs that caused spurious diffs on every re-record and were never used for matching or lookup - ## [0.4.0] - 2026-02-11 + ### Added - Proxy mode (`goodmock proxy`) — forwards all traffic to upstream without recording, applying the same header transformations and response filtering as record mode ## [0.3.2] - 2026-02-11 + ### Changed - Split monolithic `main` package into `internal/` sub-packages: `types`, `server`, `record`, `matching`, `logging`, `proxy`, `common` - Exported Server struct fields (`Mappings`, `ProxyHost`, `RefererPath`, `Verbose`, `Mu`) for cross-package access @@ -84,6 +97,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Moved shared helpers (`GetPort`, `IsVerbose`) into `internal/common` ## [0.3.1] - 2026-02-11 + ### Changed - Refactored codebase from OOP style to functional style — all Server and RecordServer methods converted to free functions - Pure functions (applyResponseHeaders, evaluateMapping, logMismatch, logVerboseRequest, transformRequestHeaders) no longer take a server receiver @@ -91,6 +105,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Request handlers use closures instead of method values ## [0.3.0] - 2026-02-10 + ### Added - Record mode (`goodmock record`) — proxies to upstream and captures request/response pairs - `/__admin/recordings/snapshot` endpoint with URL pattern filtering and scenario support @@ -99,23 +114,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `VERBOSE` environment variable for full request/response traffic logging ## [0.2.1] - 2026-02-10 + ### Changed - Added mode as primary CLI arg (`goodmock replay`), defaults to `replay` - Replaced `-port` flag with `PORT` environment variable (default: 8080) ## [0.2.0] - 2026-02-10 -### Added -- Request header rewriting (Origin, Referer, Accept-Encoding) to match recorded stubs -- `REFERER_PATH` environment variable for app-specific Referer header path ### Fixed - URL path matching now preserves percent-encoding (e.g. `%3A`) by using raw request URI instead of fasthttp's decoded path +### Added +- Request header rewriting (Origin, Referer, Accept-Encoding) to match recorded stubs +- `REFERER_PATH` environment variable for app-specific Referer header path + ## [0.1.1] - 2026-02-10 + ### Changed - Refactor and minor improvements ## [0.1.0] - 2026-02-09 + ### Added - Initial release diff --git a/README.md b/README.md index b35e8ff..01aca13 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,18 @@ goodmock --version # print version 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. -A retry happens **only** when the TCP connection could not be established. At that point no bytes have reached the upstream, so it cannot have acted on the request — which is what makes retrying safe for non-idempotent methods such as `POST`. Once a connection exists, no failure is retried, because there is no way to tell whether the upstream applied the request. +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. diff --git a/VERSION b/VERSION index ac454c6..d33c3a2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.12.0 +0.12.0 \ No newline at end of file diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index a369525..50a6c67 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -13,6 +13,21 @@ import ( "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; @@ -25,20 +40,24 @@ import ( // 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() *fasthttp.Client { +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 &fasthttp.Client{ - Dial: func(addr string) (net.Conn, error) { - return dialer.DialTimeout(addr, timeout) + 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) @@ -67,10 +86,10 @@ func ProxyRequest(client *fasthttp.Client, upstream string, ctx *fasthttp.Reques // 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 := common.ProxyDialAttempts() + attempts := client.Attempts var err error for attempt := 1; ; attempt++ { - err = client.Do(req, resp) + err = client.HTTP.Do(req, resp) if err == nil || attempt >= attempts || !errors.Is(err, fasthttp.ErrDialTimeout) { break } diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index e2d7bf5..89a6d34 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -9,8 +9,29 @@ import ( "github.com/valyala/fasthttp" ) -// TestProxyRequestDialAttempts pins the PROXY_DIAL_ATTEMPTS contract: the value -// is a budget of total dial attempts, initial dial included. +// 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 @@ -20,27 +41,19 @@ func TestProxyRequestDialAttempts(t *testing.T) { tests := []struct { name string method string - env string + attempts int wantDials int }{ - {name: "default budget, POST", method: "POST", env: "", wantDials: 3}, - {name: "default budget, GET", method: "GET", env: "", wantDials: 3}, - {name: "retrying disabled, POST", method: "POST", env: "1", wantDials: 1}, - {name: "retrying disabled, GET", method: "GET", env: "1", wantDials: 1}, - {name: "explicit budget, POST", method: "POST", env: "4", wantDials: 4}, + {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) { - t.Setenv("PROXY_DIAL_ATTEMPTS", tt.env) - - dials := 0 - client := &fasthttp.Client{ - Dial: func(_ string) (net.Conn, error) { - dials++ - return nil, fasthttp.ErrDialTimeout - }, - } + client, dials := dialCountingClient(tt.attempts, fasthttp.ErrDialTimeout) ctx := &fasthttp.RequestCtx{} ctx.Request.Header.SetMethod(tt.method) @@ -50,8 +63,34 @@ func TestProxyRequestDialAttempts(t *testing.T) { 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) + 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) } }) } @@ -61,16 +100,8 @@ func TestProxyRequestDialAttempts(t *testing.T) { // 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) { - t.Setenv("PROXY_DIAL_ATTEMPTS", "3") - sentinel := errors.New("connection refused by test") - dials := 0 - client := &fasthttp.Client{ - Dial: func(_ string) (net.Conn, error) { - dials++ - return nil, sentinel - }, - } + client, dials := dialCountingClient(3, sentinel) ctx := &fasthttp.RequestCtx{} ctx.Request.Header.SetMethod("POST") @@ -83,7 +114,7 @@ func TestProxyRequestDoesNotRetryNonDialErrors(t *testing.T) { 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) + 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 08e8cbb..70b9583 100644 --- a/internal/pureproxy/pureproxy.go +++ b/internal/pureproxy/pureproxy.go @@ -18,7 +18,7 @@ import ( type ProxyServer struct { server *types.Server upstream string - client *fasthttp.Client + client *proxy.Client } func NewProxyServer(upstream, proxyHost, refererPath string, verbose bool) *ProxyServer { diff --git a/internal/record/record.go b/internal/record/record.go index 5b40147..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