Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Do not modify the origin request in RoundTripper #4033

Merged
merged 7 commits into from Jul 13, 2023
Merged

fix: Do not modify the origin request in RoundTripper #4033

merged 7 commits into from Jul 13, 2023

Conversation

gastonqiu
Copy link
Contributor

@gastonqiu gastonqiu commented Jun 30, 2023 •
edited

According to http RoundTripper spec, we shouldn't modify the incoming request. I believe the correct way is cloning the request.
ref: https://github.com/golang/oauth2/blob/989acb1bfed17be45134185bd228d89675a68f19/transport.go#L50

See:

close #3969

@gastonqiu gastonqiu requested review from a team, Aneurysm9 and dmathieu as code owners June 30, 2023 15:55
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 30, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 30, 2023 •
edited

Codecov Report

Merging #4033 (a7191a9) into main (6d9a05e) will not change coverage.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4033   +/-   ##
=====================================
  Coverage   79.2%   79.2%           
=====================================
  Files        165     165           
  Lines      10318   10318           
=====================================
  Hits        8173    8173           
  Misses      2008    2008           
  Partials     137     137
Impacted Files Coverage Δ
instrumentation/net/http/otelhttp/transport.go 94.7% <100.0%> (ø)

@gastonqiu gastonqiu requested a review from Aneurysm9 June 30, 2023 16:35
@gastonqiu gastonqiu changed the title fix: shouldn't modify the origin request fix: shouldn't modify the origin request in RoundTripper Jul 8, 2023
dashpole
dashpole reviewed Jul 8, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
gastonqiu and others added 2 commits July 11, 2023 00:10
@gastonqiu @dashpole
Update CHANGELOG.md
925c1ab 
Co-authored-by: David Ashpole <dashpole@google.com>
@hanyuancheung
Merge branch 'main' into main
a7191a9
@gastonqiu gastonqiu changed the title fix: shouldn't modify the origin request in RoundTripper fix: do not modify the origin request in RoundTripper Jul 11, 2023
@gastonqiu gastonqiu changed the title fix: do not modify the origin request in RoundTripper fix: Do not modify the origin request in RoundTripper Jul 11, 2023
@pellared pellared merged commit 9437f9b into open-telemetry:main Jul 13, 2023
22 checks passed
@MrAlias MrAlias added this to the v0.43.0 milestone Aug 28, 2023
@MrAlias MrAlias mentioned this pull request Aug 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole left review comments

@dmathieu dmathieu dmathieu approved these changes

@pellared pellared pellared approved these changes

@MrAlias MrAlias MrAlias approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.43.0
Development

Successfully merging this pull request may close these issues.

otelhttp.Transport should not mutate the incoming request.
7 participants
@gastonqiu @dmathieu @Aneurysm9 @dashpole @pellared @MrAlias @hanyuancheung