fix(curl): deduplicate propagation headers on outgoing request#655
fix(curl): deduplicate propagation headers on outgoing request#655PuvaanRaaj wants to merge 1 commit into
Conversation
When both auto-curl and an upstream injector (auto-guzzle/auto-psr18) are enabled, propagation headers were injected twice and array_merge appended both to CURLOPT_HTTPHEADER, putting duplicate traceparent/ tracestate on the wire (rejected by some APIs). Deduplicate the final header list by name so exactly one set is sent. Fixes open-telemetry/opentelemetry-php#1898
|
cc @open-telemetry/php-approvers @brettmc for review 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
============================================
+ Coverage 80.09% 80.85% +0.75%
+ Complexity 1597 1450 -147
============================================
Files 118 95 -23
Lines 6179 5353 -826
============================================
- Hits 4949 4328 -621
+ Misses 1230 1025 -205 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d03cd8f4b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach ($this->headers as $header) { | ||
| $name = strtolower(trim(explode(':', $header, 2)[0])); | ||
| $headers[$name] = $header; |
There was a problem hiding this comment.
When any propagation header is injected, this loop rekeys all existing CURLOPT_HTTPHEADER entries by lower-cased name before adding the propagation headers. If a caller intentionally configured repeated non-propagation headers, such as multiple Cookie or custom header entries, only the last one survives, whereas the previous array_merge($this->headersToPropagate, $this->headers) kept them all. Please restrict deduplication to the propagated header names so instrumentation does not alter unrelated user-supplied request headers.
Useful? React with 👍 / 👎.
1 participant
Problem
With both
open-telemetry/opentelemetry-auto-guzzleandopen-telemetry/opentelemetry-auto-curlenabled, every outgoing Guzzle request (which uses cURL as its default handler) ends up carryingtraceparent/tracestatetwice on the wire. Some third-party APIs reject requests with duplicate propagation headers.Root cause
src/Instrumentation/Curl/src/CurlHandleMetadata.php,getRequestHeadersToSend()(line 57 onmain):Sequence of events:
Client::transfer()and callsinject()on the PSR-7 request, addingtraceparent/tracestateto the request headers.CurlFactory::applyHeaders()copies those headers intoCURLOPT_HTTPHEADER. The curl instrumentation'supdateFromCurlOption()picks this list up and stores it inCurlHandleMetadata::$headers.curl_exec()and callsinject()again, this time onCurlHandleMetadataviaHeadersPropagator, which appends a second set of propagation headers into$headersToPropagate.getRequestHeadersToSend()merges$headersToPropagateand$headerswitharray_merge(). SinceCURLOPT_HTTPHEADERis a plain numerically-indexed list of"Name: value"strings (not an associative map keyed by header name),array_merge()just concatenates the two lists — bothtraceparententries survive and both go out withCURLOPT_HTTPHEADER.Same class of bug applies to
auto-psr18+auto-curl(any instrumentation that injects into the PSR-7/PSR-18 request before curl's own hook runs).Fix
Deduplicate the final header list by header name (case-insensitive) in
getRequestHeadersToSend(), building an associative map keyed by lowercased header name from$this->headersfirst, then overlaying$this->headersToPropagate(so the freshly-injected, most-recent propagation context wins for any header name collision), then flattening back to the"Name: value"list form cURL expects. Non-propagation headers and their original casing/values are left untouched.Invariant satisfied: exactly one
traceparent/tracestate/baggageheader reaches the wire, regardless of how many instrumentations injected into the request beforehand.Tests
Added
src/Instrumentation/Curl/tests/Unit/CurlHandleMetadataTest.php:test_get_request_headers_to_send_deduplicates_already_propagated_header: seedsCURLOPT_HTTPHEADERwith atraceparent/tracestatepair (simulating an upstream Guzzle/PSR-18 injection), then callssetHeaderToPropagate()again for the same header names (simulating curl's own inject), and asserts the resulting list has exactly onetraceparentand onetracestate, with the freshly-injected value, plus the untouchedContent-Typeheader.test_get_request_headers_to_send_returns_null_when_nothing_to_propagate: unchanged existing behavior.Red/green check: reverted
CurlHandleMetadata.phpto thearray_merge()version onmainand confirmed the new test fails (Failed asserting that actual size 5 matches expected size 3, i.e. duplicate headers present). Restored the fix and confirmed it passes.Verification (Docker,
PHP_VERSION=8.2)make test(Instrumentation/Curl): 19 tests, 131 assertions, all green (existing integration tests unaffected).make style(php-cs-fixer): clean, 0 files changed.make psalm: no errors.make phpstan: no errors.No public API changes.
Fixes open-telemetry/opentelemetry-php#1898