Comply with W3C Baggage specification limits#7880
Comply with W3C Baggage specification limits#7880pellared merged 23 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7880 +/- ##
=====================================
Coverage 81.7% 81.7%
=====================================
Files 304 304
Lines 23287 23331 +44
=====================================
+ Hits 19037 19077 +40
- Misses 3863 3866 +3
- Partials 387 388 +1
🚀 New features to boost your workflow:
|
I don't think dropping all baggage is the correct interpretation of this requirement. We cannot propagate any "partial list-members", but it doesn't mean we cannot propagate complete list-members. Infact, the specification specifically states:
While dropping all may indeed be a valid "selection" of list-members, I think we can do better and just take the first, or last |
Yeah. And, this will need to refactor |
MrAlias
left a comment
There was a problem hiding this comment.
Overall this looks good. It addresses the critical security issue, looks W3C specification compliant, and is well tested. The error handling of Parse can be improved, and the issue in extractMultiBaggage, where it silently drops headers, needs to be fixed.
There was a problem hiding this comment.
Pull request overview
Updates OpenTelemetry Go baggage parsing and propagation to enforce W3C Baggage size/member limits (64 members, 8192 bytes) and adjusts tests/changelog accordingly.
Changes:
- Reduce max baggage members to 64 and remove the per-member byte limit.
- Apply byte/member limit enforcement while parsing baggage strings and when extracting from multiple
baggageheaders. - Add/adjust unit tests and document the change in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
propagation/baggage.go |
Adds combined multi-header limit handling for member count and total bytes. |
propagation/baggage_test.go |
Adds helper generators and new multi-header limit/behavior test cases. |
baggage/baggage.go |
Updates constants and changes Parse behavior to truncate to W3C limits instead of erroring. |
baggage/baggage_test.go |
Updates tests for new limit behavior and removed per-member limit. |
CHANGELOG.md |
Notes W3C baggage limit compliance changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8858ccc to
1b59ccf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
baggage/baggage_test.go:293
Parsenow truncates and returns a partialBaggagewhen the 8192-byte limit is exceeded, but the test cases here no longer cover that behavior (the “too large” case now fails earlier aserrInvalidMember). Add a test where the input exceedsmaxBytesPerBaggageStringafter one or more valid members, and assertErrorIs(err, errBaggageBytes)and that the returned baggage contains the expected prefix members within the byte limit.
func TestBaggageParse(t *testing.T) {
tooLarge := key(maxBytesPerBaggageString + 1)
m := make([]string, maxMembers+1)
for i := range m {
m[i] = fmt.Sprintf("a%d=", i)
}
tooManyMembers := strings.Join(m, listDelimiter)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@open-telemetry/go-approvers PTAL |
bboreham
left a comment
There was a problem hiding this comment.
High-level looks fine to me; I wasn't aware of all the details of the limits so maybe try to over-communicate when this goes out.
This release is the last to support [Go 1.24]. The next release will require at least [Go 1.25]. ### Added - Support testing of [Go 1.26]. (#7902) ### Fixed - Update `Baggage` in `go.opentelemetry.io/otel/propagation` and `Parse` and `New` in `go.opentelemetry.io/otel/baggage` to comply with W3C Baggage specification limits. `New` and `Parse` now return partial baggage along with an error when limits are exceeded. Errors from baggage extraction are reported to the global error handler. (#7880) [Go 1.26]: https://go.dev/doc/go1.26 [Go 1.25]: https://go.dev/doc/go1.25 [Go 1.24]: https://go.dev/doc/go1.24
Updates the baggage implementation to comply with https://www.w3.org/TR/baggage/#limits:
Changed maxMembers from 180 to 64 (the W3C compliance requirement)
Removed maxBytesPerMembers (4096) - this per-member limit was not part of the W3C spec
Added limit checking in extractMultiBaggage for multiple baggage headers:
This PR uses non-deterministic truncation when handling baggage limits.