From c81a51e823f690efb4536e234d5365edb315a245 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 30 Mar 2024 13:18:33 -0700 Subject: [PATCH 01/20] Allow UTF-8 string in baggage key --- baggage/baggage.go | 31 ++++++++++++++++++++++++-- baggage/baggage_test.go | 48 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index f3e870f8a8e..42dfae95f7c 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -8,6 +8,7 @@ import ( "fmt" "net/url" "strings" + "unicode/utf8" "go.opentelemetry.io/otel/internal/baggage" ) @@ -241,7 +242,12 @@ func NewMember(key, value string, props ...Property) (Member, error) { // NewMemberRaw returns a new Member from the passed arguments. // -// The passed key must be compliant with W3C Baggage specification. +// The passed key and value must be valid UTF-8 string. +// However, the specific Propagators that are used to transmit baggage entries across +// component boundaries may impose their own restrictions on baggage key. +// For example, the W3C Baggage specification restricts the baggage keys to strings that +// satisfy the token definition from RFC7230, Section 3.2.6. +// For maximum compatibility, alpha-numeric value are strongly recommended to be used as baggage key. func NewMemberRaw(key, value string, props ...Property) (Member, error) { m := Member{ key: key, @@ -313,9 +319,12 @@ func (m Member) validate() error { return fmt.Errorf("%w: %q", errInvalidMember, m) } - if !validateKey(m.key) { + if !validateBaggageName(m.key) { return fmt.Errorf("%w: %q", errInvalidKey, m.key) } + if !validateBaggageValue(m.value) { + return fmt.Errorf("%w: %q", errInvalidValue, m.value) + } return m.properties.validate() } @@ -630,6 +639,23 @@ func skipSpace(s string, offset int) int { return i } +// validateBaggageName checks if the string is a valid OpenTelemetry Baggage name. +// Baggage name is a valid UTF-8 string. +func validateBaggageName(s string) bool { + if len(s) == 0 { + return false + } + + return utf8.ValidString(s) +} + +// validateBaggageValue checks if the string is a valid OpenTelemetry Baggage value. +// Baggage value is a valid UTF-8 strings. +func validateBaggageValue(s string) bool { + return utf8.ValidString(s) +} + +// validateKey checks if the string is a valid W3C Baggage key. func validateKey(s string) bool { if len(s) == 0 { return false @@ -658,6 +684,7 @@ func validateKeyChar(c int32) bool { c == 0x7e } +// validateValue checks if the string is a valid W3C Baggage value. func validateValue(s string) bool { for _, c := range s { if !validateValueChar(c) { diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 4ef5b334297..9a2b646bba9 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -140,6 +140,11 @@ func TestNewKeyValueProperty(t *testing.T) { assert.ErrorIs(t, err, errInvalidValue) assert.Equal(t, Property{}, p) + // wrong value with wrong decoding + p, err = NewKeyValueProperty("key", "%zzzzz") + assert.ErrorIs(t, err, errInvalidValue) + assert.Equal(t, Property{}, p) + p, err = NewKeyValueProperty("key", "value") assert.NoError(t, err) assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) @@ -409,6 +414,15 @@ func TestBaggageParse(t *testing.T) { "foo": {Value: "ąść"}, }, }, + { + name: "encoded UTF-8 string in key", + in: "a=b,%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87", + want: baggage.List{ + "a": {Value: "b"}, + // The percent-encoded key won't be decoded. + "%C4%85%C5%9B%C4%87": {Value: "ąść"}, + }, + }, { name: "invalid member: empty", in: "foo=,,bar=", @@ -861,6 +875,10 @@ func TestMemberValidation(t *testing.T) { m.hasData = true assert.ErrorIs(t, m.validate(), errInvalidKey) + // Invalid UTF-8 in value + m.key, m.value = "k", string([]byte{255}) + assert.ErrorIs(t, m.validate(), errInvalidValue) + m.key, m.value = "k", "\\" assert.NoError(t, m.validate()) } @@ -882,6 +900,11 @@ func TestNewMember(t *testing.T) { } assert.Equal(t, expected, m) + // wrong value with invalid token + val = ";" + _, err = NewMember(key, val, p) + assert.ErrorIs(t, err, errInvalidValue) + // wrong value with wrong decoding val = "%zzzzz" _, err = NewMember(key, val, p) @@ -926,6 +949,31 @@ func TestNewMemberRaw(t *testing.T) { assert.Equal(t, expected, m) } +func TestBaggageUTF8(t *testing.T) { + testCases := map[string]string{ + "ąść": "B% 💼", + + // Case sensitive + "a": "a", + "A": "A", + } + + var members []Member + for k, v := range testCases { + m, err := NewMemberRaw(k, v) + require.NoError(t, err) + + members = append(members, m) + } + + b, err := New(members...) + require.NoError(t, err) + + for k, v := range testCases { + assert.Equal(t, v, b.Member(k).Value()) + } +} + func TestPropertiesValidate(t *testing.T) { p := properties{{}} assert.ErrorIs(t, p.validate(), errInvalidKey) From dc96def48999b456de2fd958b06885664191b722 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Mon, 1 Apr 2024 22:52:15 -0700 Subject: [PATCH 02/20] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6a9a9c1aad..8a53d537e80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed - `SpanFromContext` and `SpanContextFromContext` in `go.opentelemetry.io/otel/trace` no longer make a heap allocation when the passed context has no span. (#5049) +- `NewMemberRaw` in `go.opentelemetry.io/otel/baggage` allows UTF-8 string. (#5132) ### Fixed From 9434b2f3375537081711ad6fbd273fc875ce9a49 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 20 Apr 2024 16:34:34 -0700 Subject: [PATCH 03/20] Allow UTF-8 in property key --- baggage/baggage.go | 56 +++++++++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index dbdd239f3f0..8eae5adf19e 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -46,7 +46,7 @@ type Property struct { // // If key is invalid, an error will be returned. func NewKeyProperty(key string) (Property, error) { - if !validateKey(key) { + if !validateBaggageName(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } @@ -56,12 +56,20 @@ func NewKeyProperty(key string) (Property, error) { // NewKeyValueProperty returns a new Property for key with value. // -// The passed key must be compliant with W3C Baggage specification. +// The passed key must be percent-encoded. // The passed value must be percent-encoded as defined in W3C Baggage specification. // // Notice: Consider using [NewKeyValuePropertyRaw] instead -// that does not require percent-encoding of the value. +// that does not require percent-encoding of the key and the value. func NewKeyValueProperty(key, value string) (Property, error) { + if !validateKey(key) { + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) + } + decodedKey, err := url.PathUnescape(value) + if err != nil { + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + if !validateValue(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -69,16 +77,24 @@ func NewKeyValueProperty(key, value string) (Property, error) { if err != nil { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } - return NewKeyValuePropertyRaw(key, decodedValue) + return NewKeyValuePropertyRaw(decodedKey, decodedValue) } // NewKeyValuePropertyRaw returns a new Property for key with value. // -// The passed key must be compliant with W3C Baggage specification. +// The passed key and value must be valid UTF-8 string. +// However, the specific Propagators that are used to transmit baggage entries across +// component boundaries may impose their own restrictions on Property key. +// For example, the W3C Baggage specification restricts the Property keys to strings that +// satisfy the token definition from RFC7230, Section 3.2.6. +// For maximum compatibility, alpha-numeric value are strongly recommended to be used as Property key. func NewKeyValuePropertyRaw(key, value string) (Property, error) { - if !validateKey(key) { + if !validateBaggageName(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } + if !validateValue(key) { + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) + } p := Property{ key: key, @@ -115,12 +131,15 @@ func (p Property) validate() error { return fmt.Errorf("invalid property: %w", err) } - if !validateKey(p.key) { + if !validateBaggageName(p.key) { return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key)) } if !p.hasValue && p.value != "" { return errFunc(errors.New("inconsistent value")) } + if !validateValue(p.value) { + return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) + } return nil } @@ -136,11 +155,10 @@ func (p Property) Value() (string, bool) { return p.value, p.hasValue } -// String encodes Property into a header string compliant with the W3C Baggage -// specification. +// String encodes Property into a header string. func (p Property) String() string { if p.hasValue { - return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value)) + return fmt.Sprintf("%s%s%v", valueEscape(p.key), keyValueDelimiter, valueEscape(p.value)) } return p.key } @@ -224,12 +242,20 @@ type Member struct { // NewMember returns a new Member from the passed arguments. // -// The passed key must be compliant with W3C Baggage specification. +// The passed key must be percent-encoded. // The passed value must be percent-encoded as defined in W3C Baggage specification. // // Notice: Consider using [NewMemberRaw] instead // that does not require percent-encoding of the value. func NewMember(key, value string, props ...Property) (Member, error) { + if !validateKey(key) { + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) + } + decodedKey, err := url.PathUnescape(key) + if err != nil { + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) + } + if !validateValue(value) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -237,7 +263,7 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err != nil { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } - return NewMemberRaw(key, decodedValue, props...) + return NewMemberRaw(decodedKey, decodedValue, props...) } // NewMemberRaw returns a new Member from the passed arguments. @@ -340,10 +366,8 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // 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 - // US-ASCII characters excluding CTLs, whitespace, - // DQUOTE, comma, semicolon, and backslash. - s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, valueEscape(m.value)) + // A key is can be a valid UTF-8 string. + s := fmt.Sprintf("%s%s%s", valueEscape(m.key), keyValueDelimiter, valueEscape(m.value)) if len(m.properties) > 0 { s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) } From 171b0dbce1f56beb07bf959b97abe4dee0dac908 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 20 Apr 2024 23:32:48 -0700 Subject: [PATCH 04/20] Fix tests --- baggage/baggage.go | 12 ++++++---- baggage/baggage_test.go | 51 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 8eae5adf19e..cd8dca646fd 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -65,9 +65,9 @@ func NewKeyValueProperty(key, value string) (Property, error) { if !validateKey(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - decodedKey, err := url.PathUnescape(value) + decodedKey, err := url.PathUnescape(key) if err != nil { - return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) + return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } if !validateValue(value) { @@ -92,7 +92,7 @@ func NewKeyValuePropertyRaw(key, value string) (Property, error) { if !validateBaggageName(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - if !validateValue(key) { + if !validateBaggageValue(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } @@ -137,7 +137,7 @@ func (p Property) validate() error { if !p.hasValue && p.value != "" { return errFunc(errors.New("inconsistent value")) } - if !validateValue(p.value) { + if p.hasValue && !validateBaggageValue(p.value) { return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value)) } return nil @@ -765,6 +765,10 @@ func validateBaggageName(s string) bool { // validateBaggageValue checks if the string is a valid OpenTelemetry Baggage value. // Baggage value is a valid UTF-8 strings. func validateBaggageValue(s string) bool { + if len(s) == 0 { + return false + } + return utf8.ValidString(s) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 3492a32371c..943ab3fa0c0 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -123,12 +123,22 @@ func TestParsePropertyError(t *testing.T) { func TestNewKeyProperty(t *testing.T) { p, err := NewKeyProperty(" ") - assert.ErrorIs(t, err, errInvalidKey) - assert.Equal(t, Property{}, p) + assert.NoError(t, err) + assert.Equal(t, Property{key: " "}, p) p, err = NewKeyProperty("key") assert.NoError(t, err) assert.Equal(t, Property{key: "key"}, p) + + // UTF-8 key + p, err = NewKeyProperty("B% 💼") + assert.NoError(t, err) + assert.Equal(t, Property{key: "B% 💼"}, p) + + // Invalid UTF-8 key + p, err = NewKeyProperty(string([]byte{255})) + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) } func TestNewKeyValueProperty(t *testing.T) { @@ -140,6 +150,11 @@ func TestNewKeyValueProperty(t *testing.T) { assert.ErrorIs(t, err, errInvalidValue) assert.Equal(t, Property{}, p) + // wrong key with wrong decoding + p, err = NewKeyValueProperty("%zzzzz", "value") + assert.ErrorIs(t, err, errInvalidKey) + assert.Equal(t, Property{}, p) + // wrong value with wrong decoding p, err = NewKeyValueProperty("key", "%zzzzz") assert.ErrorIs(t, err, errInvalidValue) @@ -148,16 +163,32 @@ func TestNewKeyValueProperty(t *testing.T) { p, err = NewKeyValueProperty("key", "value") assert.NoError(t, err) assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) + + // Percent-encoded key and value + p, err = NewKeyValueProperty("%C4%85%C5%9B%C4%87", "%C4%85%C5%9B%C4%87") + assert.NoError(t, err) + assert.Equal(t, Property{key: "ąść", value: "ąść", hasValue: true}, p) } func TestNewKeyValuePropertyRaw(t *testing.T) { - p, err := NewKeyValuePropertyRaw(" ", "") + // Empty key + p, err := NewKeyValuePropertyRaw("", " ") assert.ErrorIs(t, err, errInvalidKey) assert.Equal(t, Property{}, p) - p, err = NewKeyValuePropertyRaw("key", "Witaj Świecie!") + // Empty value + p, err = NewKeyValuePropertyRaw(" ", "") + assert.ErrorIs(t, err, errInvalidValue) + assert.Equal(t, Property{}, p) + + // Space value + p, err = NewKeyValuePropertyRaw(" ", " ") assert.NoError(t, err) - assert.Equal(t, Property{key: "key", value: "Witaj Świecie!", hasValue: true}, p) + assert.Equal(t, Property{key: " ", value: " ", hasValue: true}, p) + + p, err = NewKeyValuePropertyRaw("B% 💼", "Witaj Świecie!") + assert.NoError(t, err) + assert.Equal(t, Property{key: "B% 💼", value: "Witaj Świecie!", hasValue: true}, p) } func TestPropertyValidate(t *testing.T) { @@ -172,6 +203,10 @@ func TestPropertyValidate(t *testing.T) { p.hasValue = true assert.NoError(t, p.validate()) + + // Invalid value + p.value = string([]byte{255}) + assert.ErrorIs(t, p.validate(), errInvalidValue) } func TestNewEmptyBaggage(t *testing.T) { @@ -900,7 +935,13 @@ func TestNewMember(t *testing.T) { } assert.Equal(t, expected, m) + // wong key with wrong decoding + key = "%zzzzz" + _, err = NewMember(key, val, p) + assert.ErrorIs(t, err, errInvalidKey) + // wrong value with invalid token + key = "k" val = ";" _, err = NewMember(key, val, p) assert.ErrorIs(t, err, errInvalidValue) From ce67af3f65eeefb07ba915d2d2b66358d623c82a Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 20 Apr 2024 23:44:42 -0700 Subject: [PATCH 05/20] Allow UTF-8 in key to be parsed --- baggage/baggage.go | 22 ++++++++++++++++++---- baggage/baggage_test.go | 5 ++--- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index cd8dca646fd..812030e675a 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -330,12 +330,18 @@ func parseMember(member string) (Member, error) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v) } + // Decode a percent-encoded key. + unescapedKey, err := url.PathUnescape(key) + if err != nil { + return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidKey, err) + } + // Decode a percent-encoded value. - value, err := url.PathUnescape(val) + unescapedValue, err := url.PathUnescape(val) if err != nil { return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err) } - return Member{key: key, value: value, properties: props, hasData: true}, nil + return Member{key: unescapedKey, value: unescapedValue, properties: props, hasData: true}, nil } // validate ensures m conforms to the W3C Baggage specification. @@ -416,8 +422,10 @@ func New(members ...Member) (Baggage, error) { } // Parse attempts to decode a baggage-string from the passed string. It -// returns an error if the input is invalid according to the W3C Baggage +// returns an error if the input is invalid according to the OpenTelemetry Baggage // specification. +// The OpenTelemetry Baggage specification has less strict requirements than the +// W3C Baggage specification, as it allows UTF-8 characters in keys. // // If there are duplicate list-members contained in baggage, the last one // defined (reading left-to-right) will be the only one kept. This diverges @@ -638,6 +646,12 @@ func parsePropertyInternal(s string) (p Property, ok bool) { return } + // Decode a percent-encoded key. + key, err := url.PathUnescape(s[keyStart:keyEnd]) + if err != nil { + return + } + // Decode a percent-encoded value. value, err := url.PathUnescape(s[valueStart:valueEnd]) if err != nil { @@ -645,7 +659,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } ok = true - p.key = s[keyStart:keyEnd] + p.key = key p.hasValue = true p.value = value diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 943ab3fa0c0..6ee84982933 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -453,9 +453,8 @@ func TestBaggageParse(t *testing.T) { name: "encoded UTF-8 string in key", in: "a=b,%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87", want: baggage.List{ - "a": {Value: "b"}, - // The percent-encoded key won't be decoded. - "%C4%85%C5%9B%C4%87": {Value: "ąść"}, + "a": {Value: "b"}, + "ąść": {Value: "ąść"}, }, }, { From e8644293d7b298d2f0ad28954afb8da371038c58 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 20 Apr 2024 23:51:09 -0700 Subject: [PATCH 06/20] Add more tests to cover 100% of lines --- baggage/baggage_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 6ee84982933..a8875a56bf6 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -482,6 +482,11 @@ func TestBaggageParse(t *testing.T) { in: "foo=\\", err: errInvalidValue, }, + { + name: "invalid member: improper url encoded key", + in: "key%=val", + err: errInvalidKey, + }, { name: "invalid member: improper url encoded value", in: "key1=val%", @@ -497,11 +502,21 @@ func TestBaggageParse(t *testing.T) { in: "foo=1;key\\=v", err: errInvalidProperty, }, + { + name: "invalid property: improper url encoded key", + in: "foo=1;key%=v", + err: errInvalidProperty, + }, { name: "invalid property: invalid value", in: "foo=1;key=\\", err: errInvalidProperty, }, + { + name: "invalid property: improper url encoded value", + in: "foo=1;key=val%", + err: errInvalidProperty, + }, { name: "invalid baggage string: too large", in: tooLarge, From 097d8a2d0e612ac0463dcb345b4264fc32e66afb Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sat, 20 Apr 2024 23:58:29 -0700 Subject: [PATCH 07/20] Update CHANGELOG --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b5a8d60e4d..1718e8dfbbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed - `SpanFromContext` and `SpanContextFromContext` in `go.opentelemetry.io/otel/trace` no longer make a heap allocation when the passed context has no span. (#5049) -- `NewMemberRaw` in `go.opentelemetry.io/otel/baggage` allows UTF-8 string. (#5132) +- `NewMember` and `NewKeyValueProperty` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) +- `NewMemberRaw`, `NewKeyProperty` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage` allow UTF-8 string in key. (#5132) +- `Parse` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) - `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` now create a gRPC client in idle mode and with "dns" as the default resolver using [`grpc.NewClient`](https://pkg.go.dev/google.golang.org/grpc#NewClient). (#5151) Because of that `WithDialOption` ignores [`grpc.WithBlock`](https://pkg.go.dev/google.golang.org/grpc#WithBlock), [`grpc.WithTimeout`](https://pkg.go.dev/google.golang.org/grpc#WithTimeout), and [`grpc.WithReturnConnectionError`](https://pkg.go.dev/google.golang.org/grpc#WithReturnConnectionError). Notice that [`grpc.DialContext`](https://pkg.go.dev/google.golang.org/grpc#DialContext) which was used before is now deprecated. From d716cec7fb6c4cbaea68e3e7ca7272afb08854c6 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sun, 21 Apr 2024 00:28:39 -0700 Subject: [PATCH 08/20] Add more tests --- baggage/baggage.go | 2 +- baggage/baggage_test.go | 27 ++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 812030e675a..4da2e889928 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -160,7 +160,7 @@ func (p Property) String() string { if p.hasValue { return fmt.Sprintf("%s%s%v", valueEscape(p.key), keyValueDelimiter, valueEscape(p.value)) } - return p.key + return valueEscape(p.key) } type properties []Property diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index a8875a56bf6..80d8429f00b 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -457,6 +457,14 @@ func TestBaggageParse(t *testing.T) { "ąść": {Value: "ąść"}, }, }, + { + name: "encoded UTF-8 string in property", + in: "a=b,%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87;%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87", + want: baggage.List{ + "a": {Value: "b"}, + "ąść": {Value: "ąść", Properties: []baggage.Property{{Key: "ąść", HasValue: true, Value: "ąść"}}}, + }, + }, { name: "invalid member: empty", in: "foo=,,bar=", @@ -667,6 +675,23 @@ func TestBaggageString(t *testing.T) { }, }, }, + { + name: "utf-8 key and value", + out: "%C4%85%C5%9B%C4%872=B%25%20%F0%9F%92%BC-2;yellow,%C4%85%C5%9B%C4%87=B%25%20%F0%9F%92%BC;%C4%85%C5%9B%C4%87-1=B%25%20%F0%9F%92%BC-1;%C4%85%C5%9B%C4%87-2", + baggage: baggage.List{ + "ąść": { + Value: "B% 💼", + Properties: []baggage.Property{ + {Key: "ąść-1", Value: "B% 💼-1", HasValue: true}, + {Key: "ąść-2"}, + }, + }, + "ąść2": { + Value: "B% 💼-2", + Properties: []baggage.Property{{Key: "yellow"}}, + }, + }, + }, } orderer := func(s string) string { @@ -1096,7 +1121,7 @@ func BenchmarkString(b *testing.B) { addMember("key1", "val1") addMember("key2", " ;,%") - addMember("key3", "Witaj świecie!") + addMember("B% 💼", "Witaj świecie!") addMember("key4", strings.Repeat("Hello world!", 10)) bg, err := New(members...) From a07ffb49d2f5d0525411b790de04b250e7c60e9b Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Wed, 19 Jun 2024 14:26:38 -0700 Subject: [PATCH 09/20] Allow empty string as baggage value --- baggage/baggage.go | 6 ++---- baggage/baggage_test.go | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 547c4d8f9fe..ad8469ecbbe 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -768,6 +768,7 @@ var safeKeyCharset = [utf8.RuneSelf]bool{ // validateBaggageName checks if the string is a valid OpenTelemetry Baggage name. // Baggage name is a valid UTF-8 string. +// Empty string is also a valid UTF-8 string. func validateBaggageName(s string) bool { if len(s) == 0 { return false @@ -778,11 +779,8 @@ func validateBaggageName(s string) bool { // validateBaggageValue checks if the string is a valid OpenTelemetry Baggage value. // Baggage value is a valid UTF-8 strings. +// Empty string is also a valid UTF-8 string. func validateBaggageValue(s string) bool { - if len(s) == 0 { - return false - } - return utf8.ValidString(s) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 2a2c8fa711b..45529db8304 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -177,9 +177,10 @@ func TestNewKeyValuePropertyRaw(t *testing.T) { assert.Equal(t, Property{}, p) // Empty value + // Empty string is also a valid UTF-8 string. p, err = NewKeyValuePropertyRaw(" ", "") - assert.ErrorIs(t, err, errInvalidValue) - assert.Equal(t, Property{}, p) + assert.NoError(t, err) + assert.Equal(t, Property{key: " ", hasValue: true}, p) // Space value p, err = NewKeyValuePropertyRaw(" ", " ") From 6c289d940e51d834d78d305b209d00ff0f71c489 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Wed, 3 Jul 2024 12:03:02 -0700 Subject: [PATCH 10/20] Keep the behavior of W3C baggage propagator --- baggage/baggage.go | 39 ++++++++++++++++++++++++++++++--------- baggage/baggage_test.go | 14 ++++++++++---- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 931276666a1..8550e721909 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -155,12 +155,18 @@ func (p Property) Value() (string, bool) { return p.value, p.hasValue } -// String encodes Property into a header string. +// String encodes Property into a header string compliant with the W3C Baggage +// specification. func (p Property) String() string { + // W3C Baggage specification does not allow percent-encoded keys. + if !validateKey(p.key) { + return "" + } + if p.hasValue { - return fmt.Sprintf("%s%s%v", valueEscape(p.key), keyValueDelimiter, valueEscape(p.value)) + return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value)) } - return valueEscape(p.key) + return p.key } type properties []Property @@ -221,9 +227,14 @@ func (p properties) validate() error { // String encodes properties into a header string compliant with the W3C Baggage // specification. func (p properties) String() string { - props := make([]string, len(p)) - for i, prop := range p { - props[i] = prop.String() + props := make([]string, 0, len(p)) + for _, prop := range p { + s := prop.String() + + // Ignored empty properties. + if s != "" { + props = append(props, s) + } } return strings.Join(props, propertyDelimiter) } @@ -333,7 +344,7 @@ func parseMember(member string) (Member, error) { // Decode a percent-encoded key. unescapedKey, err := url.PathUnescape(key) if err != nil { - return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidKey, err) + return newInvalidMember(), fmt.Errorf("%w: %w", errInvalidKey, err) } // Decode a percent-encoded value. @@ -372,6 +383,11 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // String encodes Member into a header string compliant with the W3C Baggage // specification. func (m Member) String() string { + // W3C Baggage specification does not allow percent-encoded keys. + if !validateKey(m.key) { + return "" + } + // A key is can be a valid UTF-8 string. s := valueEscape(m.key) + keyValueDelimiter + valueEscape(m.value) if len(m.properties) > 0 { @@ -571,11 +587,16 @@ func (b Baggage) Len() int { func (b Baggage) String() string { members := make([]string, 0, len(b.list)) for k, v := range b.list { - members = append(members, Member{ + s := Member{ key: k, value: v.Value, properties: fromInternalProperties(v.Properties), - }.String()) + }.String() + + // Ignored empty members. + if s != "" { + members = append(members, s) + } } return strings.Join(members, listDelimiter) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 45529db8304..d27c6aadd9f 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -677,8 +677,10 @@ func TestBaggageString(t *testing.T) { }, }, { + // W3C does not allow percent-encoded keys. + // The baggage that has percent-encoded keys should be ignored. name: "utf-8 key and value", - out: "%C4%85%C5%9B%C4%872=B%25%20%F0%9F%92%BC-2;yellow,%C4%85%C5%9B%C4%87=B%25%20%F0%9F%92%BC;%C4%85%C5%9B%C4%87-1=B%25%20%F0%9F%92%BC-1;%C4%85%C5%9B%C4%87-2", + out: "foo=B%25%20%F0%9F%92%BC-2;foo-1=B%25%20%F0%9F%92%BC-4;foo-2", baggage: baggage.List{ "ąść": { Value: "B% 💼", @@ -687,9 +689,13 @@ func TestBaggageString(t *testing.T) { {Key: "ąść-2"}, }, }, - "ąść2": { - Value: "B% 💼-2", - Properties: []baggage.Property{{Key: "yellow"}}, + "foo": { + Value: "B% 💼-2", + Properties: []baggage.Property{ + {Key: "ąść", Value: "B% 💼-3", HasValue: true}, + {Key: "foo-1", Value: "B% 💼-4", HasValue: true}, + {Key: "foo-2"}, + }, }, }, }, From 6c014602fea365652cbedd6f58518ac197d29ca6 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Wed, 3 Jul 2024 13:46:15 -0700 Subject: [PATCH 11/20] Revert the behavior of Parse to only parse W3C Baggage --- baggage/baggage.go | 21 ++++----------------- baggage/baggage_test.go | 20 ++++++-------------- 2 files changed, 10 insertions(+), 31 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 8550e721909..407c160cd69 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -341,18 +341,12 @@ func parseMember(member string) (Member, error) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v) } - // Decode a percent-encoded key. - unescapedKey, err := url.PathUnescape(key) - if err != nil { - return newInvalidMember(), fmt.Errorf("%w: %w", errInvalidKey, err) - } - // Decode a percent-encoded value. unescapedValue, err := url.PathUnescape(val) if err != nil { return newInvalidMember(), fmt.Errorf("%w: %w", errInvalidValue, err) } - return Member{key: unescapedKey, value: unescapedValue, properties: props, hasData: true}, nil + return Member{key: key, value: unescapedValue, properties: props, hasData: true}, nil } // validate ensures m conforms to the W3C Baggage specification. @@ -438,10 +432,8 @@ func New(members ...Member) (Baggage, error) { } // Parse attempts to decode a baggage-string from the passed string. It -// returns an error if the input is invalid according to the OpenTelemetry Baggage +// returns an error if the input is invalid according to the W3C Baggage // specification. -// The OpenTelemetry Baggage specification has less strict requirements than the -// W3C Baggage specification, as it allows UTF-8 characters in keys. // // If there are duplicate list-members contained in baggage, the last one // defined (reading left-to-right) will be the only one kept. This diverges @@ -667,12 +659,6 @@ func parsePropertyInternal(s string) (p Property, ok bool) { return } - // Decode a percent-encoded key. - key, err := url.PathUnescape(s[keyStart:keyEnd]) - if err != nil { - return - } - // Decode a percent-encoded value. value, err := url.PathUnescape(s[valueStart:valueEnd]) if err != nil { @@ -680,7 +666,7 @@ func parsePropertyInternal(s string) (p Property, ok bool) { } ok = true - p.key = key + p.key = s[keyStart:keyEnd] p.hasValue = true p.value = value @@ -805,6 +791,7 @@ func validateBaggageValue(s string) bool { return utf8.ValidString(s) } +// validateValue checks if the string is a valid W3C Baggage key. func validateKey(s string) bool { if len(s) == 0 { return false diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index d27c6aadd9f..f2af720de32 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -454,16 +454,18 @@ func TestBaggageParse(t *testing.T) { name: "encoded UTF-8 string in key", in: "a=b,%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87", want: baggage.List{ - "a": {Value: "b"}, - "ąść": {Value: "ąść"}, + "a": {Value: "b"}, + "%C4%85%C5%9B%C4%87": {Value: "ąść"}, }, }, { name: "encoded UTF-8 string in property", in: "a=b,%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87;%C4%85%C5%9B%C4%87=%C4%85%C5%9B%C4%87", want: baggage.List{ - "a": {Value: "b"}, - "ąść": {Value: "ąść", Properties: []baggage.Property{{Key: "ąść", HasValue: true, Value: "ąść"}}}, + "a": {Value: "b"}, + "%C4%85%C5%9B%C4%87": {Value: "ąść", Properties: []baggage.Property{ + {Key: "%C4%85%C5%9B%C4%87", HasValue: true, Value: "ąść"}, + }}, }, }, { @@ -491,11 +493,6 @@ func TestBaggageParse(t *testing.T) { in: "foo=\\", err: errInvalidValue, }, - { - name: "invalid member: improper url encoded key", - in: "key%=val", - err: errInvalidKey, - }, { name: "invalid member: improper url encoded value", in: "key1=val%", @@ -511,11 +508,6 @@ func TestBaggageParse(t *testing.T) { in: "foo=1;key\\=v", err: errInvalidProperty, }, - { - name: "invalid property: improper url encoded key", - in: "foo=1;key%=v", - err: errInvalidProperty, - }, { name: "invalid property: invalid value", in: "foo=1;key=\\", From 1a88be716b823211a68747869ec7233fee1409cc Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Wed, 3 Jul 2024 13:47:36 -0700 Subject: [PATCH 12/20] Update CHANGELOG --- CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f07164eb4c..2d764d87dad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Changed + +- `NewMember` and `NewKeyValueProperty` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) +- `NewMemberRaw`, `NewKeyProperty` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage` allow UTF-8 string in key. (#5132) + ## [1.28.0/0.50.0/0.4.0] 2024-07-02 ### Added @@ -141,9 +146,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed - `SpanFromContext` and `SpanContextFromContext` in `go.opentelemetry.io/otel/trace` no longer make a heap allocation when the passed context has no span. (#5049) -- `NewMember` and `NewKeyValueProperty` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) -- `NewMemberRaw`, `NewKeyProperty` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage` allow UTF-8 string in key. (#5132) -- `Parse` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) - `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` now create a gRPC client in idle mode and with "dns" as the default resolver using [`grpc.NewClient`](https://pkg.go.dev/google.golang.org/grpc#NewClient). (#5151) Because of that `WithDialOption` ignores [`grpc.WithBlock`](https://pkg.go.dev/google.golang.org/grpc#WithBlock), [`grpc.WithTimeout`](https://pkg.go.dev/google.golang.org/grpc#WithTimeout), and [`grpc.WithReturnConnectionError`](https://pkg.go.dev/google.golang.org/grpc#WithReturnConnectionError). Notice that [`grpc.DialContext`](https://pkg.go.dev/google.golang.org/grpc#DialContext) which was used before is now deprecated. From 6eed4e0513fe43c64d2b3d78d43a8c396b04f7b5 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Tue, 16 Jul 2024 20:51:53 -0700 Subject: [PATCH 13/20] Update comments --- baggage/baggage.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index c568c0558da..c234d5d9e52 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -82,7 +82,8 @@ func NewKeyValueProperty(key, value string) (Property, error) { // NewKeyValuePropertyRaw returns a new Property for key with value. // -// The passed key and value must be valid UTF-8 string. +// The passed key must be valid, non-empty UTF-8 string. +// The passed value must be valid UTF-8 string. // However, the specific Propagators that are used to transmit baggage entries across // component boundaries may impose their own restrictions on Property key. // For example, the W3C Baggage specification restricts the Property keys to strings that @@ -253,7 +254,7 @@ type Member struct { // NewMember returns a new Member from the passed arguments. // -// The passed key must be percent-encoded. +// The passed key must be non-empty percent-encoded. // The passed value must be percent-encoded as defined in W3C Baggage specification. // // Notice: Consider using [NewMemberRaw] instead @@ -279,7 +280,8 @@ func NewMember(key, value string, props ...Property) (Member, error) { // NewMemberRaw returns a new Member from the passed arguments. // -// The passed key and value must be valid UTF-8 string. +// The passed key must be valid, non-empty UTF-8 string. +// The passed value must be valid UTF-8 string. // However, the specific Propagators that are used to transmit baggage entries across // component boundaries may impose their own restrictions on baggage key. // For example, the W3C Baggage specification restricts the baggage keys to strings that @@ -802,8 +804,7 @@ var safeKeyCharset = [utf8.RuneSelf]bool{ } // validateBaggageName checks if the string is a valid OpenTelemetry Baggage name. -// Baggage name is a valid UTF-8 string. -// Empty string is also a valid UTF-8 string. +// Baggage name is a valid, non-empty UTF-8 string. func validateBaggageName(s string) bool { if len(s) == 0 { return false From ef3973eae46b4a7f571861fa1b460a9ad45be699 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sun, 21 Jul 2024 15:24:10 -0700 Subject: [PATCH 14/20] Revert the behavior of NewMember --- baggage/baggage.go | 8 ++------ baggage/baggage_test.go | 15 +++++++++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index c234d5d9e52..802fd5f4969 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -254,7 +254,7 @@ type Member struct { // NewMember returns a new Member from the passed arguments. // -// The passed key must be non-empty percent-encoded. +// The passed key must be compliant with W3C Baggage specification. // The passed value must be percent-encoded as defined in W3C Baggage specification. // // Notice: Consider using [NewMemberRaw] instead @@ -263,10 +263,6 @@ func NewMember(key, value string, props ...Property) (Member, error) { if !validateKey(key) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) } - decodedKey, err := url.PathUnescape(key) - if err != nil { - return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key) - } if !validateValue(value) { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) @@ -275,7 +271,7 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err != nil { return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } - return NewMemberRaw(decodedKey, decodedValue, props...) + return NewMemberRaw(key, decodedValue, props...) } // NewMemberRaw returns a new Member from the passed arguments. diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 46f09cf0eba..8d0fd5343cb 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -1033,10 +1033,17 @@ func TestNewMember(t *testing.T) { } assert.Equal(t, expected, m) - // wong key with wrong decoding - key = "%zzzzz" - _, err = NewMember(key, val, p) - assert.ErrorIs(t, err, errInvalidKey) + // it won't use percent decoding for key + key = "%3B" + m, err = NewMember(key, val, p) + assert.NoError(t, err) + expected = Member{ + key: key, + value: val, + properties: properties{{key: "foo"}}, + hasData: true, + } + assert.Equal(t, expected, m) // wrong value with invalid token key = "k" From 99fcf534cbd5c339f4c329a0ace1d2fa1e8cc82b Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sun, 21 Jul 2024 15:29:31 -0700 Subject: [PATCH 15/20] Revert the behavior of NewKeyValueProperty --- baggage/baggage.go | 10 +++------- baggage/baggage_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 802fd5f4969..ab4f74477f8 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -56,19 +56,15 @@ func NewKeyProperty(key string) (Property, error) { // NewKeyValueProperty returns a new Property for key with value. // -// The passed key must be percent-encoded. +// The passed key must be compliant with W3C Baggage specification. // The passed value must be percent-encoded as defined in W3C Baggage specification. // // Notice: Consider using [NewKeyValuePropertyRaw] instead -// that does not require percent-encoding of the key and the value. +// that does not require percent-encoding of the value. func NewKeyValueProperty(key, value string) (Property, error) { if !validateKey(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) } - decodedKey, err := url.PathUnescape(key) - if err != nil { - return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) - } if !validateValue(value) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) @@ -77,7 +73,7 @@ func NewKeyValueProperty(key, value string) (Property, error) { if err != nil { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value) } - return NewKeyValuePropertyRaw(decodedKey, decodedValue) + return NewKeyValuePropertyRaw(key, decodedValue) } // NewKeyValuePropertyRaw returns a new Property for key with value. diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 8d0fd5343cb..7d164727128 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -151,10 +151,10 @@ func TestNewKeyValueProperty(t *testing.T) { assert.ErrorIs(t, err, errInvalidValue) assert.Equal(t, Property{}, p) - // wrong key with wrong decoding + // it won't use percent decoding for key p, err = NewKeyValueProperty("%zzzzz", "value") - assert.ErrorIs(t, err, errInvalidKey) - assert.Equal(t, Property{}, p) + assert.NoError(t, err) + assert.Equal(t, Property{key: "%zzzzz", value: "value", hasValue: true}, p) // wrong value with wrong decoding p, err = NewKeyValueProperty("key", "%zzzzz") @@ -165,10 +165,10 @@ func TestNewKeyValueProperty(t *testing.T) { assert.NoError(t, err) assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p) - // Percent-encoded key and value - p, err = NewKeyValueProperty("%C4%85%C5%9B%C4%87", "%C4%85%C5%9B%C4%87") + // Percent-encoded value + p, err = NewKeyValueProperty("key", "%C4%85%C5%9B%C4%87") assert.NoError(t, err) - assert.Equal(t, Property{key: "ąść", value: "ąść", hasValue: true}, p) + assert.Equal(t, Property{key: "key", value: "ąść", hasValue: true}, p) } func TestNewKeyValuePropertyRaw(t *testing.T) { From 8d694184d1846531518d417e25276f062eee2b30 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sun, 21 Jul 2024 15:30:30 -0700 Subject: [PATCH 16/20] Update CHANGELOG --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dd2d277a23..8ae88c89877 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed -- `NewMember` and `NewKeyValueProperty` in `go.opentelemetry.io/otel/baggage` allow percent-encoded UTF-8 string in key. (#5132) - `NewMemberRaw`, `NewKeyProperty` and `NewKeyValuePropertyRaw` in `go.opentelemetry.io/otel/baggage` allow UTF-8 string in key. (#5132) ### Fixed From 9a8541522012fe4f23eb031e09d53d25cc9111d8 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Sun, 21 Jul 2024 15:36:18 -0700 Subject: [PATCH 17/20] Update comments --- baggage/baggage.go | 1 + 1 file changed, 1 insertion(+) diff --git a/baggage/baggage.go b/baggage/baggage.go index ab4f74477f8..806da3c62c5 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -44,6 +44,7 @@ type Property struct { // NewKeyProperty returns a new Property for key. // +// The passed key must be valid, non-empty UTF-8 string. // If key is invalid, an error will be returned. func NewKeyProperty(key string) (Property, error) { if !validateBaggageName(key) { From a544f84dabb7b54c50c8cbdfdaf24e31c2db69e1 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Tue, 23 Jul 2024 17:29:18 -0700 Subject: [PATCH 18/20] Remove the key escaping in `String` method of `Member` --- baggage/baggage.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 806da3c62c5..33c28668f67 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -403,8 +403,7 @@ func (m Member) String() string { return "" } - // A key is can be a valid UTF-8 string. - s := valueEscape(m.key) + keyValueDelimiter + valueEscape(m.value) + s := m.key + keyValueDelimiter + valueEscape(m.value) if len(m.properties) > 0 { s += propertyDelimiter + m.properties.String() } From e8e07f3725ce8cb83f7e069e9dd1b5c1258c2c9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 24 Jul 2024 06:57:00 +0200 Subject: [PATCH 19/20] Update baggage/baggage.go --- baggage/baggage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 33c28668f67..6edba2941b5 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -812,7 +812,7 @@ func validateBaggageValue(s string) bool { return utf8.ValidString(s) } -// validateValue checks if the string is a valid W3C Baggage key. +// validateKey checks if the string is a valid W3C Baggage key. func validateKey(s string) bool { if len(s) == 0 { return false From 66d8cd1f54a7a29fb78d4e2621eea0a061ace1e8 Mon Sep 17 00:00:00 2001 From: Sam Xie Date: Thu, 1 Aug 2024 09:50:44 -0700 Subject: [PATCH 20/20] Add more comments --- baggage/baggage.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/baggage/baggage.go b/baggage/baggage.go index 531b0d996b3..b3569e95e5c 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -46,6 +46,11 @@ type Property struct { // // The passed key must be valid, non-empty UTF-8 string. // If key is invalid, an error will be returned. +// However, the specific Propagators that are used to transmit baggage entries across +// component boundaries may impose their own restrictions on Property key. +// For example, the W3C Baggage specification restricts the Property keys to strings that +// satisfy the token definition from RFC7230, Section 3.2.6. +// For maximum compatibility, alpha-numeric value are strongly recommended to be used as Property key. func NewKeyProperty(key string) (Property, error) { if !validateBaggageName(key) { return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key) @@ -155,6 +160,9 @@ func (p Property) Value() (string, bool) { // String encodes Property into a header string compliant with the W3C Baggage // specification. +// It would return empty string if the key is invalid with the W3C Baggage +// specification. This could happen for a UTF-8 key, as it may contain +// invalid characters. func (p Property) String() string { // W3C Baggage specification does not allow percent-encoded keys. if !validateKey(p.key) { @@ -397,6 +405,9 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // String encodes Member into a header string compliant with the W3C Baggage // specification. +// It would return empty string if the key is invalid with the W3C Baggage +// specification. This could happen for a UTF-8 key, as it may contain +// invalid characters. func (m Member) String() string { // W3C Baggage specification does not allow percent-encoded keys. if !validateKey(m.key) { @@ -596,6 +607,9 @@ func (b Baggage) Len() int { // String encodes Baggage into a header string compliant with the W3C Baggage // specification. +// It would ignore members where the member key is invalid with the W3C Baggage +// specification. This could happen for a UTF-8 key, as it may contain +// invalid characters. func (b Baggage) String() string { members := make([]string, 0, len(b.list)) for k, v := range b.list {