fix(config): deep copy the header in cloneRequest - #982
Open
SaiPisey2 wants to merge 3 commits into
Open
Conversation
The shallow struct copy leaves r2.Header aliasing r.Header, so maps.Copy(r.Header, r2.Header) copies the map onto itself and the clone shares the caller's header map. Every round tripper that adds a header therefore mutates the request it was given. headersRoundTripper uses Header.Add, so a reused request accumulates another copy of every configured header on each round trip, without bound, until the server rejects the request. Clone the header instead, keeping the map non-nil so round trippers that call Header.Set on a request built without one still work. Signed-off-by: SaiPisey2 <piseysai0202@gmail.com>
Covers both halves of the clone: the copy must not be shared with the caller, and a request built without a header must still be usable afterwards, which previously panicked with "assignment to entry in nil map" on the first Header.Set. Signed-off-by: SaiPisey2 <piseysai0202@gmail.com>
testifylint requires the f-suffixed assertions when a message is passed. Signed-off-by: SaiPisey2 <piseysai0202@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #981.
cloneRequestintends a deep copy of the header but does not make one:After
*r2 = *r,r2.Headeris already the same map asr.Header, somaps.Copycopies it onto itself and the clone shares the caller's header map.Every round tripper here clones and then modifies, so each one mutates the request it was given.
headersRoundTripperusesHeader.Add, so a reused request gains another copy of every configured header on each round trip, without bound. grafana/alloy#7016 reports ~1000 identicalX-Api-Keylines in a single request, with the target returning 431 and profiling stopping.Introduced in 56870db, which replaced the manual copy loop with
maps.Copy. The arguments are also the wrong way round, but swapping them is not enough — the clone needs its own map.Header.Clone()returns nil for a nil header, while the previous code always produced a usable map, so the nil case is kept:userAgentRoundTripperand others callHeader.Setstraight after cloning, which panics withassignment to entry in nil mapon a request built without one. That path is broken on main today as well.Tests
Two tests, both failing on main and passing here:
TestCloneRequest— the clone must not share the caller's map, and a request without a header must still be usable afterwards.TestHeadersRoundTripperReusedRequest— reusing a request must not accumulate headers, and must not modify the caller's request.go build ./...,go test ./...andgo test -race ./config/are green on darwin/arm64, go1.26.5.