[baggage] validate key and values during baggage injection/extraction

[lachmatt]

Fixes #5479 Design discussion issue #

Changes

• Baggage item key is no longer encoded and decoded - it is validated against token requirement from the spec instead
• Baggage item value is validated during extraction
• Only invalid baggage items are rejected during injection
• All of the baggage items are rejected during extraction if any of them is invalid (more strict than injection)

NOTE: Amount of allocations (e.g in extract part) can be reduced, I'd prefer to address it in a separate PR.

Merge requirement checklist

• [x] CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• [x] Unit tests added/updated
• [x] Appropriate CHANGELOG.md files updated for non-trivial changes
• [ ] Changes in public API reviewed (if applicable)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.01%. Comparing base (6250307) to head (6b88989). Report is 251 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5647      +/-   ##
==========================================
+ Coverage   83.38%   86.01%   +2.63%     
==========================================
  Files         297      254      -43     
  Lines       12531    11114    -1417     
==========================================
- Hits        10449     9560     -889     
+ Misses       2082     1554     -528     

Flag	Coverage Δ
unittests	?
unittests-Project-Experimental	86.01% <94.28%> (?)
unittests-Project-Stable	86.01% <94.28%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	83.33% <100.00%> (+2.08%)	:arrow_up:
...metry.Api/Context/Propagation/BaggagePropagator.cs	88.98% <93.93%> (+3.49%)	:arrow_up:

... and 128 files with indirect coverage changes

[vishweshbankwar]

@lachmatt - Could you please clarify where these requirements are coming from?

• Only invalid baggage items are rejected during injection
• All of the baggage items are rejected during extraction if any of them is invalid (more strict than injection)

[lachmatt]

@lachmatt - Could you please clarify where these requirements are coming from?

• Only invalid baggage items are rejected during injection
• All of the baggage items are rejected during extraction if any of them is invalid (more strict than injection)

This is not a requirement, but a choice made during implementation (I'm open for suggestions how to improve it).

For the injection part , Baggage API allows arbitrary strings as names. This is less restrictive than key requirements from the W3C Baggage specification. What would be the preferred behavior for situation when one of the names previously accepted in Baggage API Set operation doesn't meet token requirement from the W3C Baggage specification? I assumed it might be beneficial to propagate valid items in that case.

For the extraction part, current behavior is consistent with e.g. how invalid items are handled during tracestate extraction.

As far as I know, invalid values are handled differently by different language implementations, e.g.:

• in Java, only invalid baggage items are ignored, during both injection and extraction (valid items are propagated)
• in Go, all baggage items are ignored during extraction if invalid item is recognized

[vishweshbankwar]

@lachmatt - Could you please clarify where these requirements are coming from?

• Only invalid baggage items are rejected during injection
• All of the baggage items are rejected during extraction if any of them is invalid (more strict than injection)

This is not a requirement, but a choice made during implementation (I'm open for suggestions how to improve it).

For the injection part , Baggage API allows arbitrary strings as names. This is less restrictive than key requirements from the W3C Baggage specification. What would be the preferred behavior for situation when one of the names previously accepted in Baggage API Set operation doesn't meet token requirement from the W3C Baggage specification? I assumed it might be beneficial to propagate valid items in that case.

For the extraction part, current behavior is consistent with e.g. how invalid items are handled during tracestate extraction.

As far as I know, invalid values are handled differently by different language implementations, e.g.:

• in Java, only invalid baggage items are ignored, during both injection and extraction (valid items are propagated)
• in Go, all baggage items are ignored during extraction if invalid item is recognized

@lachmatt - Shouldn't this be coming from spec?

Looking at the spec

However, the specific Propagators that are used to transmit baggage entries across component boundaries may impose their own restrictions on baggage names. For example, the W3C Baggage specification restricts the baggage keys to strings that satisfy the token definition from RFC7230, Section 3.2.6

This seems to suggest it is not a MUST?

Also, looking at this https://www.w3.org/TR/baggage/#mutating-baggage. Looks like there is already a suggested approach for values.

If a system determines that the value of a baggage entry is not in the format defined in this specification, it MAY remove that entry before propagating the baggage header as part of outgoing requests.

But a different approach is proposed here?

[lachmatt]

@vishweshbankwar I created an issue in the spec

[xiang17]

@vishweshbankwar I created an issue in the spec

I think the spec says to remove that (baggage) entry if conditions are met, rather than dropping the entire baggage.

If a system determines that the value of a baggage entry is not in the format defined in this specification, it MAY remove that entry before propagating the baggage header as part of outgoing requests.