From 259143a6628a9d0b344b4d6a0b704d1e94d69bb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 10 Jan 2024 14:02:57 +0100 Subject: [PATCH] baggage: Add NewMemberRaw and NewKeyValuePropertyRaw (#4804) --- CHANGELOG.md | 3 ++ baggage/baggage.go | 86 ++++++++++++++++++++----------- baggage/baggage_test.go | 83 ++++++++++++++++++++++------- bridge/opentracing/bridge.go | 2 +- bridge/opentracing/bridge_test.go | 24 +++++++-- bridge/opentracing/mix_test.go | 2 +- example/namedtracer/main.go | 4 +- propagation/baggage_test.go | 5 +- 8 files changed, 150 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d33d430d270..6147c177966 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add `WithResourceAsConstantLabels` option to apply resource attributes for every metric emitted by the Prometheus exporter. (#4733) - Experimental cardinality limiting is added to the metric SDK. See [metric documentation](./sdk/metric/EXPERIMENTAL.md#cardinality-limit) for more information about this feature and how to enable it. (#4457) +- Add `NewMemberRaw` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage`. (#4804) ### Changed @@ -31,12 +32,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Improve `go.opentelemetry.io/otel/baggage` performance. (#4743) - Improve performance of the `(*Set).Filter` method in `go.opentelemetry.io/otel/attribute` when the passed filter does not filter out any attributes from the set. (#4774) - `Member.String` in `go.opentelemetry.io/otel/baggage` percent-encodes only when necessary. (#4775) +- `Property.Value` in `go.opentelemetry.io/otel/baggage` now returns a raw string instead of a percent-encoded value. (#4804) ### Fixed - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755) - Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756) - Fix baggage item key so that it is not canonicalized in `go.opentelemetry.io/otel/bridge/opentracing`. (#4776) +- Fix `go.opentelemetry.io/otel/bridge/opentracing` to properly handle baggage values that requires escaping during propagation. (#4804) ## [1.21.0/0.44.0] 2023-11-16 diff --git a/baggage/baggage.go b/baggage/baggage.go index 89a0664201d..7d27cf77d5c 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -66,14 +66,29 @@ func NewKeyProperty(key string) (Property, error) { // NewKeyValueProperty returns a new Property for key with value. // -// If key or value are invalid, an error will be returned. +// The passed key must be compliant with W3C Baggage specification. +// The passed value must be precent-encoded as defined in W3C Baggage specification. +// +// Notice: Consider using [NewKeyValuePropertyRaw] instead +// that does not require precent-encoding of the value. func NewKeyValueProperty(key, value string) (Property, error) { - if !validateKey(key) { - return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) - } if !validateValue(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } + decodedValue, err := url.PathUnescape(value) + if err != nil { + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + return NewKeyValuePropertyRaw(key, decodedValue) +} + +// NewKeyValuePropertyRaw returns a new Property for key with value. +// +// The passed key must be compliant with W3C Baggage specification. +func NewKeyValuePropertyRaw(key, value string) (Property, error) { + if !validateKey(key) { + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) + } p := Property{ key: key, @@ -113,9 +128,6 @@ func (p Property) validate() error { if !validateKey(p.key) { return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } - if p.hasValue && !validateValue(p.value) { - return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) - } if !p.hasValue && p.value != "" { return errFunc(errors.New("inconsistent value")) } @@ -134,11 +146,11 @@ func (p Property) Value() (string, bool) { return p.value, p.hasValue } -// String encodes Property into a string compliant with the W3C Baggage +// String encodes Property into a header string compliant with the W3C Baggage // specification. func (p Property) String() string { if p.hasValue { - return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value) + return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value)) } return p.key } @@ -198,7 +210,7 @@ func (p properties) validate() error { return nil } -// String encodes properties into a string compliant with the W3C Baggage +// String encodes properties into a header string compliant with the W3C Baggage // specification. func (p properties) String() string { props := make([]string, len(p)) @@ -220,11 +232,28 @@ type Member struct { hasData bool } -// NewMember returns a new Member from the passed arguments. The key will be -// used directly while the value will be url decoded after validation. An error -// is returned if the created Member would be invalid according to the W3C -// Baggage specification. +// NewMemberRaw returns a new Member from the passed arguments. +// +// The passed key must be compliant with W3C Baggage specification. +// The passed value must be precent-encoded as defined in W3C Baggage specification. +// +// Notice: Consider using [NewMemberRaw] instead +// that does not require precent-encoding of the value. func NewMember(key, value string, props ...Property) (Member, error) { + if !validateValue(value) { + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + decodedValue, err := url.PathUnescape(value) + if err != nil { + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + return NewMemberRaw(key, decodedValue, props...) +} + +// NewMemberRaw returns a new Member from the passed arguments. +// +// The passed key must be compliant with W3C Baggage specification. +func NewMemberRaw(key, value string, props ...Property) (Member, error) { m := Member{ key: key, value: value, @@ -234,11 +263,6 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err := m.validate(); err != nil { return newInvalidMember(), err } - decodedValue, err := url.PathUnescape(value) - if err != nil { - return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) - } - m.value = decodedValue return m, nil } @@ -284,6 +308,8 @@ func parseMember(member string) (Member, error) { if !validateValue(val) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v) } + + // Decode a precent-encoded value. value, err := url.PathUnescape(val) if err != nil { return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err) @@ -292,8 +318,7 @@ func parseMember(member string) (Member, error) { } // validate ensures m conforms to the W3C Baggage specification. -// A key is just an ASCII string, but a value must be URL encoded UTF-8, -// returning an error otherwise. +// A key must be an ASCII string, returning an error otherwise. func (m Member) validate() error { if !m.hasData { return fmt.Errorf("%w: %q", errInvalidMember, m) @@ -302,9 +327,6 @@ func (m Member) validate() error { if !validateKey(m.key) { return fmt.Errorf("%w: %q", errInvalidKey, m.key) } - if !validateValue(m.value) { - return fmt.Errorf("%w: %q", errInvalidValue, m.value) - } return m.properties.validate() } @@ -317,7 +339,7 @@ func (m Member) Value() string { return m.value } // Properties returns a copy of the Member properties. func (m Member) Properties() []Property { return m.properties.Copy() } -// String encodes Member into a string compliant with the W3C Baggage +// String encodes Member into a header string compliant with the W3C Baggage // specification. func (m Member) String() string { // A key is just an ASCII string. A value is restricted to be @@ -514,9 +536,8 @@ func (b Baggage) Len() int { return len(b.list) } -// String encodes Baggage into a string compliant with the W3C Baggage -// specification. The returned string will be invalid if the Baggage contains -// any invalid list-members. +// String encodes Baggage into a header string compliant with the W3C Baggage +// specification. func (b Baggage) String() string { members := make([]string, 0, len(b.list)) for k, v := range b.list { @@ -595,10 +616,17 @@ func parsePropertyInternal(s string) (p Property, ok bool) { return } + // Decode a precent-encoded value. + value, err := url.PathUnescape(s[valueStart:valueEnd]) + if err != nil { + return + } + ok = true p.key = s[keyStart:keyEnd] p.hasValue = true - p.value = s[valueStart:valueEnd] + + p.value = value return } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index e6664f0e0af..4bacf1ea54b 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -143,7 +143,7 @@ func TestNewKeyProperty(t *testing.T) { } func TestNewKeyValueProperty(t *testing.T) { - p, err := NewKeyValueProperty(" ", "") + p, err := NewKeyValueProperty(" ", "value") assert.ErrorIs(t, err, errInvalidKey) assert.Equal(t, Property{}, p) @@ -156,6 +156,16 @@ func TestNewKeyValueProperty(t *testing.T) { assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) } +func TestNewKeyValuePropertyRaw(t *testing.T) { + p, err := NewKeyValuePropertyRaw(" ", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + + p, err = NewKeyValuePropertyRaw("key", "Witaj Świecie!") + assert.NoError(t, err) + assert.Equal(t, Property{key: "key", value: "Witaj Świecie!", hasValue: true}, p) +} + func TestPropertyValidate(t *testing.T) { p := Property{} assert.ErrorIs(t, p.validate(), errInvalidKey) @@ -163,13 +173,10 @@ func TestPropertyValidate(t *testing.T) { p.key = "k" assert.NoError(t, p.validate()) - p.value = ";" + p.value = "v" assert.EqualError(t, p.validate(), "invalid property: inconsistent value") p.hasValue = true - assert.ErrorIs(t, p.validate(), errInvalidValue) - - p.value = "v" assert.NoError(t, p.validate()) } @@ -397,6 +404,15 @@ func TestBaggageParse(t *testing.T) { "key1": {Value: "val%2,"}, }, }, + { + name: "encoded property", + in: "key1=;bar=val%252%2C", + want: baggage.List{ + "key1": { + Properties: []baggage.Property{{Key: "bar", HasValue: true, Value: "val%2,"}}, + }, + }, + }, { name: "encoded UTF-8 string", in: "foo=%C4%85%C5%9B%C4%87", @@ -528,6 +544,17 @@ func TestBaggageString(t *testing.T) { "foo": {Value: "ąść"}, }, }, + { + name: "Encoded property value", + out: "foo=;bar=%20", + baggage: baggage.List{ + "foo": { + Properties: []baggage.Property{ + {Key: "bar", Value: " ", HasValue: true}, + }, + }, + }, + }, { name: "plus", out: "foo=1+1", @@ -846,9 +873,6 @@ func TestMemberValidation(t *testing.T) { assert.ErrorIs(t, m.validate(), errInvalidKey) m.key, m.value = "k", "\\" - assert.ErrorIs(t, m.validate(), errInvalidValue) - - m.value = "v" assert.NoError(t, m.validate()) } @@ -891,6 +915,28 @@ func TestNewMember(t *testing.T) { assert.Equal(t, expected, m) } +func TestNewMemberRaw(t *testing.T) { + m, err := NewMemberRaw("", "") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Member{hasData: false}, m) + + key, val := "k", "v" + p := Property{key: "foo"} + m, err = NewMemberRaw(key, val, p) + assert.NoError(t, err) + expected := Member{ + key: key, + value: val, + properties: properties{{key: "foo"}}, + hasData: true, + } + assert.Equal(t, expected, m) + + // Ensure new member is immutable. + p.key = "bar" + assert.Equal(t, expected, m) +} + func TestPropertiesValidate(t *testing.T) { p := properties{{}} assert.ErrorIs(t, p.validate(), errInvalidKey) @@ -904,11 +950,12 @@ func TestPropertiesValidate(t *testing.T) { func TestMemberString(t *testing.T) { // normal key value pair - member, _ := NewMember("key", "value") + member, _ := NewMemberRaw("key", "value") memberStr := member.String() assert.Equal(t, memberStr, "key=value") - // encoded key - member, _ = NewMember("key", "%3B%20") + + // encoded value + member, _ = NewMemberRaw("key", "; ") memberStr = member.String() assert.Equal(t, memberStr, "key=%3B%20") } @@ -916,10 +963,10 @@ func TestMemberString(t *testing.T) { var benchBaggage Baggage func BenchmarkNew(b *testing.B) { - mem1, _ := NewMember("key1", "val1") - mem2, _ := NewMember("key2", "val2") - mem3, _ := NewMember("key3", "val3") - mem4, _ := NewMember("key4", "val4") + mem1, _ := NewMemberRaw("key1", "val1") + mem2, _ := NewMemberRaw("key2", "val2") + mem3, _ := NewMemberRaw("key3", "val3") + mem4, _ := NewMemberRaw("key4", "val4") b.ReportAllocs() b.ResetTimer() @@ -931,11 +978,11 @@ func BenchmarkNew(b *testing.B) { var benchMember Member -func BenchmarkNewMember(b *testing.B) { +func BenchmarkNewMemberRaw(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - benchMember, _ = NewMember("key", "value") + benchMember, _ = NewMemberRaw("key", "value") } } @@ -950,7 +997,7 @@ func BenchmarkParse(b *testing.B) { func BenchmarkString(b *testing.B) { var members []Member addMember := func(k, v string) { - m, err := NewMember(k, valueEscape(v)) + m, err := NewMemberRaw(k, valueEscape(v)) require.NoError(b, err) members = append(members, m) } diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index f61dc9eee00..6ecb3fc25bf 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -73,7 +73,7 @@ func (c *bridgeSpanContext) ForeachBaggageItem(handler func(k, v string) bool) { } func (c *bridgeSpanContext) setBaggageItem(restrictedKey, value string) { - m, err := baggage.NewMember(restrictedKey, value) + m, err := baggage.NewMemberRaw(restrictedKey, value) if err != nil { return } diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index 7a3e384424b..11c21d79496 100644 --- a/bridge/opentracing/bridge_test.go +++ b/bridge/opentracing/bridge_test.go @@ -578,17 +578,17 @@ func TestBridgeSpanContextPromotedMethods(t *testing.T) { func TestBridgeCarrierBaggagePropagation(t *testing.T) { carriers := []struct { name string - carrier interface{} + factory func() interface{} format ot.BuiltinFormat }{ { name: "TextMapCarrier", - carrier: ot.TextMapCarrier{}, + factory: func() interface{} { return ot.TextMapCarrier{} }, format: ot.TextMap, }, { name: "HTTPHeadersCarrier", - carrier: ot.HTTPHeadersCarrier{}, + factory: func() interface{} { return ot.HTTPHeadersCarrier{} }, format: ot.HTTPHeaders, }, } @@ -619,6 +619,19 @@ func TestBridgeCarrierBaggagePropagation(t *testing.T) { }, }, }, + { + name: "with characters escaped by baggage propagator", + baggageItems: []bipBaggage{ + { + key: "space", + value: "Hello world!", + }, + { + key: "utf8", + value: "Świat", + }, + }, + }, } for _, c := range carriers { @@ -638,10 +651,11 @@ func TestBridgeCarrierBaggagePropagation(t *testing.T) { } defer span.Finish() - err := b.Inject(span.Context(), c.format, c.carrier) + carrier := c.factory() + err := b.Inject(span.Context(), c.format, carrier) assert.NoError(t, err) - spanContext, err := b.Extract(c.format, c.carrier) + spanContext, err := b.Extract(c.format, carrier) assert.NoError(t, err) // Check baggage items. diff --git a/bridge/opentracing/mix_test.go b/bridge/opentracing/mix_test.go index b57f37e8ea7..7dc902545a6 100644 --- a/bridge/opentracing/mix_test.go +++ b/bridge/opentracing/mix_test.go @@ -515,7 +515,7 @@ func (bio *baggageInteroperationTest) addAndRecordBaggage(t *testing.T, ctx cont otSpan.SetBaggageItem(otKey, value) - m, err := baggage.NewMember(otelKey, value) + m, err := baggage.NewMemberRaw(otelKey, value) if err != nil { t.Error(err) return ctx diff --git a/example/namedtracer/main.go b/example/namedtracer/main.go index 68c51ce45c8..51caa20a3dd 100644 --- a/example/namedtracer/main.go +++ b/example/namedtracer/main.go @@ -67,8 +67,8 @@ func main() { ctx := context.Background() defer func() { _ = tp.Shutdown(ctx) }() - m0, _ := baggage.NewMember(string(fooKey), "foo1") - m1, _ := baggage.NewMember(string(barKey), "bar1") + m0, _ := baggage.NewMemberRaw(string(fooKey), "foo1") + m1, _ := baggage.NewMemberRaw(string(barKey), "bar1") b, _ := baggage.New(m0, m1) ctx = baggage.ContextWithBaggage(ctx, b) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 8149fbaf833..3919c3d1d72 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -17,7 +17,6 @@ package propagation_test import ( "context" "net/http" - "net/url" "strings" "testing" @@ -41,13 +40,13 @@ type member struct { func (m member) Member(t *testing.T) baggage.Member { props := make([]baggage.Property, 0, len(m.Properties)) for _, p := range m.Properties { - p, err := baggage.NewKeyValueProperty(p.Key, p.Value) + p, err := baggage.NewKeyValuePropertyRaw(p.Key, p.Value) if err != nil { t.Fatal(err) } props = append(props, p) } - bMember, err := baggage.NewMember(m.Key, url.PathEscape(m.Value), props...) + bMember, err := baggage.NewMemberRaw(m.Key, m.Value, props...) if err != nil { t.Fatal(err) }