Use url.PathUnescape rather than url.QueryUnescape in baggage parsing #4667

n-oden · 2023-10-24T19:58:49Z

I believe this addresses the majority of the cases described in #3601

Golang's url.QueryUnescape will render url path elements (e.g. /, +) as spaces: foo+bar is rendered as foo bar. Path elements are (as I read the spec) legal W3C baggage values, and replacing them with spaces fails the value validation regex.

url.PathEscape allows path elements through unmolested.

I believe this addresses the majority of the cases described in open-telemetry#3601 Golang's url.QueryUnescape will render url _path_ elements (e.g. /, +) as spaces: `foo+bar` is rendered as `foo bar`. Path elements are (as I read the spec) legal W3C baggage values, and replacing them with spaces fails the value validation regex. url.PathEscape allows path elements through unmolested. Signed-off-by: Nathan J. Mehl <n@oden.io>

codecov · 2023-10-24T20:26:40Z

Codecov Report

Merging #4667 (59af688) into main (ce7b40a) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #4667     +/-   ##
=======================================
- Coverage   81.7%   81.7%   -0.1%     
=======================================
  Files        225     225             
  Lines      18056   18056             
=======================================
- Hits       14764   14762      -2     
- Misses      2994    2996      +2     
  Partials     298     298

Files	Coverage Δ
baggage/baggage.go	`98.9% <100.0%> (ø)`

... and 1 file with indirect coverage changes

pellared · 2023-10-24T20:29:38Z

I do not have time review at this moment, but could you please also confront your changes with the following issues which seem to be related?

allan-meng · 2023-10-25T03:07:10Z

Running into the same issue. Do we have an ETA on when this fix will be released?

Lim3nius · 2023-10-25T12:55:26Z

This doesn't fix problem with #4241 as I have checked here, because problem there is in parseMember after value is unescaped from wire transmission, it is still checked, whether it's value is still compliant with W3C baggage specification (valueRe check), although it doesn't have to be.

In my opinion it would be best to address the bigger problem with baggage, which is demostrated in my example and best solution IMHO is described here

n-oden · 2023-10-25T15:45:18Z

@pellared those issues are all extremely long and most link to other issues. Respectfully, no: I am not going to trawl through all of the cases described across all of those tickets to determine if this PR applies. This PR fixes a specific bug (handling of + and / characters in baggage values) and adds unit tests to verify that the fix works as described and does not break any of the other tested cases. If that's not considered a useful contribution we are happy to just use our own fork internally until the larger issues are sorted out.

@Lim3nius I agree that the path forward described by @yurishkuro would be the correct long-term plan. But he admits up front that this would be a breaking change and thus presumably fodder for a v2.0 release. I would like to not wait until then to fix this particular bug, which is actively breaking tracing in our organization.

MikeGoldsmith · 2023-10-26T12:09:08Z

I agree here with @n-oden. This PR helps solve the immediate bug in correctly handling specific characters. The longer-term suggested fix from #3685 is larger, breaking change and hasn't been addressed in 6+ months.

This PR provides a simpler, non-breaking solution that at least makes the root cause a little easier to deal with.

Are there any specific concerns, or questions around this being reviewed?

pellared · 2023-10-26T14:17:48Z

Are there any specific concerns, or questions around this being reviewed?

None that I am aware of. I will ask during SIG meeting to review it.
@MikeGoldsmith, feel free to review it as well 😉

MikeGoldsmith

Other than updating the PR number in the changelog, this looks good to me.

I confirmed this is the same behaviour other SDKs use to encode baggage values.
For example: Java SDK (uses custom parser, but does path escaping)

CHANGELOG.md

pellared

Initially, I was hesitant to approve this PR as I find it bug-prone and improper to use url.PathUnescape for decoding and url.QueryEscape for encoding.

However, I was playing with different unit test cases on your PR for about an half an hour and it produces better results than the current implementation.

Ideally we should have a dedicated decoding and encoding logic. But I am OK approving it as it is, because perfect is the enemy of good.

Please make sure to update the changelog and keep the PR up to date (or give the maintainers permission to update the branch).

CHANGELOG.md

address comments Co-authored-by: Robert Pająk <pellared@hotmail.com>

n-oden · 2023-10-27T15:04:54Z

@pellared @MikeGoldsmith Updated the changelog and brought this up to date with the main branch -- should be good to go, thank you!

n-oden · 2023-10-27T21:33:43Z

So.... who merges this and what would be the expected ETA to getting it into a full release?

pellared · 2023-10-27T22:00:20Z

So.... who merges this and what would be the expected ETA to getting it into a full release?

This requires at least a second approval from an approver or maintainer. Please make sure to keep the PR up to date.

Release would be probably in a matter of few weeks, but I cannot promise anything.

n-oden · 2023-10-27T22:03:51Z

@MikeGoldsmith can I trouble you for a second approval? :)

pellared · 2023-10-30T13:47:15Z

@open-telemetry/go-approvers PTAL

n-oden · 2023-10-30T14:37:26Z

n.b. the failing coverage test appears to be an infra issue:

[2023-10-30T13:46:50.133Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
[2023-10-30T13:46:50.134Z] ['verbose'] The error stack is: Error: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
    at main (/snapshot/repo/dist/src/index.js)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
[2023-10-30T13:46:50.134Z] ['verbose'] End of uploader: 1201 milliseconds
Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3.1.4/dist/codecov' failed with exit code 255

robbkidd

I'm not an OTel Go approver, but I work with @MikeGoldsmith (who is on holiday until next week). He and I reviewed this PR together and I agree with the update for whatever a grey checkmark approval is worth!

n-oden requested review from MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared, hanyuancheung and dmathieu as code owners October 24, 2023 19:58

n-oden mentioned this pull request Oct 24, 2023

Error while parsing Baggage header with percent-encoded whitespace #3601

Closed

Merge branch 'main' into main

9aad30c

MikeGoldsmith approved these changes Oct 27, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pellared approved these changes Oct 27, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

n-oden and others added 2 commits October 27, 2023 11:03

Update CHANGELOG.md

6f203db

address comments Co-authored-by: Robert Pająk <pellared@hotmail.com>

Merge branch 'main' into main

0efc2bc

n-oden requested review from MikeGoldsmith and pellared October 27, 2023 15:05

pellared approved these changes Oct 27, 2023

View reviewed changes

Merge branch 'main' into main

aab183d

Merge branch 'main' into main

fb46309

n-oden added 2 commits October 30, 2023 15:25

Merge branch 'main' into main

4110835

Merge branch 'main' into main

59af688

robbkidd approved these changes Oct 31, 2023

View reviewed changes

dmathieu approved these changes Oct 31, 2023

View reviewed changes

MrAlias added this to the v1.20.0 milestone Oct 31, 2023

MrAlias merged commit bdb9322 into open-telemetry:main Oct 31, 2023
25 checks passed

pellared mentioned this pull request Nov 8, 2023

Use url.PathUnescape rather than url.QueryUnescape when parsing OTLP headers and resource attributes env vars #4698

Closed

mikeblum mentioned this pull request Nov 8, 2023

Use url.PathUnescape when parsing OTLP headers and resource attributes env vars #4699

Merged

pellared mentioned this pull request Nov 10, 2023

Release v1.20.0/v0.43.0 #4705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use url.PathUnescape rather than url.QueryUnescape in baggage parsing #4667

Use url.PathUnescape rather than url.QueryUnescape in baggage parsing #4667

n-oden commented Oct 24, 2023

codecov bot commented Oct 24, 2023 •

edited

pellared commented Oct 24, 2023

allan-meng commented Oct 25, 2023

Lim3nius commented Oct 25, 2023

n-oden commented Oct 25, 2023 •

edited

MikeGoldsmith commented Oct 26, 2023

pellared commented Oct 26, 2023

MikeGoldsmith left a comment

pellared left a comment •

edited

n-oden commented Oct 27, 2023 •

edited

n-oden commented Oct 27, 2023

pellared commented Oct 27, 2023 •

edited

n-oden commented Oct 27, 2023

pellared commented Oct 30, 2023 •

edited

n-oden commented Oct 30, 2023

robbkidd left a comment

Use url.PathUnescape rather than url.QueryUnescape in baggage parsing #4667

Use url.PathUnescape rather than url.QueryUnescape in baggage parsing #4667

Conversation

n-oden commented Oct 24, 2023

codecov bot commented Oct 24, 2023 • edited

Codecov Report

pellared commented Oct 24, 2023

allan-meng commented Oct 25, 2023

Lim3nius commented Oct 25, 2023

n-oden commented Oct 25, 2023 • edited

MikeGoldsmith commented Oct 26, 2023

pellared commented Oct 26, 2023

MikeGoldsmith left a comment

Choose a reason for hiding this comment

pellared left a comment • edited

Choose a reason for hiding this comment

n-oden commented Oct 27, 2023 • edited

n-oden commented Oct 27, 2023

pellared commented Oct 27, 2023 • edited

n-oden commented Oct 27, 2023

pellared commented Oct 30, 2023 • edited

n-oden commented Oct 30, 2023

robbkidd left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2023 •

edited

n-oden commented Oct 25, 2023 •

edited

pellared left a comment •

edited

n-oden commented Oct 27, 2023 •

edited

pellared commented Oct 27, 2023 •

edited

pellared commented Oct 30, 2023 •

edited