From be5064a38704694ed75171eb8ff3e11b9f25cd44 Mon Sep 17 00:00:00 2001 From: Michael Blum Date: Thu, 9 Nov 2023 12:51:59 -0600 Subject: [PATCH] Use url.PathUnescape rather than url.QueryUnescape when parsing OTLP headers and resource attributes env vars (#4698) (#4699) --- CHANGELOG.md | 7 ++++- .../internal/envconfig/envconfig.go | 4 +-- .../internal/envconfig/envconfig_test.go | 17 ++++++++++- .../internal/envconfig/envconfig.go | 4 +-- .../internal/envconfig/envconfig_test.go | 17 ++++++++++- .../internal/envconfig/envconfig.go | 4 +-- .../internal/envconfig/envconfig_test.go | 17 ++++++++++- .../internal/envconfig/envconfig.go | 4 +-- .../internal/envconfig/envconfig_test.go | 17 ++++++++++- .../shared/otlp/envconfig/envconfig.go.tmpl | 4 +-- .../otlp/envconfig/envconfig_test.go.tmpl | 17 ++++++++++- sdk/resource/env.go | 2 +- sdk/resource/env_test.go | 30 +++++++++++++++++++ 13 files changed, 127 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bebd78266fa..83c61ef2579 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +59,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- Fix improper parsing of characters such us `+`, `\` by `Parse` in `go.opentelemetry.io/otel/baggage` as they were rendered as a whitespace. (#4667) +- Fix improper parsing of characters such us `+`, `/` by `Parse` in `go.opentelemetry.io/otel/baggage` as they were rendered as a whitespace. (#4667) +- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_RESOURCE_ATTRIBUTES` in `go.opentelemetry.io/otel/sdk/resource` as they were rendered as a whitespace. (#4699) +- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_METRICS_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` as they were rendered as a whitespace. (#4699) +- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_METRICS_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` as they were rendered as a whitespace. (#4699) +- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_TRACES_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlptracegrpc` as they were rendered as a whitespace. (#4699) +- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_TRACES_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlptracehttp` as they were rendered as a whitespace. (#4699) - In `go.opentelemetry.op/otel/exporters/prometheus`, the exporter no longer `Collect`s metrics after `Shutdown` is invoked. (#4648) - Fix documentation for `WithCompressor` in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`. (#4695) - Fix documentation for `WithCompressor` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`. (#4695) diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go index 1d571294695..17951ceb451 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go @@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.QueryUnescape(n) + name, err := url.PathUnescape(n) if err != nil { global.Error(err, "escape header key", "key", n) continue } trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(v) + value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) continue diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go index cec506208d5..6cbe0c7ab11 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go @@ -427,7 +427,12 @@ func TestStringToHeader(t *testing.T) { want: map[string]string{"userId": "alice"}, }, { - name: "multiples headers encoded", + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + }, + { + name: "multiple headers encoded", value: "userId=alice,serverNode=DF%3A28,isProduction=false", want: map[string]string{ "userId": "alice", @@ -435,6 +440,16 @@ func TestStringToHeader(t *testing.T) { "isProduction": "false", }, }, + { + name: "multiple headers encoded per RFC 3986 spec", + value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test", + want: map[string]string{ + "userId": "alice+test", + "serverNode": "DF:28", + "isProduction": "false", + "namespace": "localhost/test", + }, + }, { name: "invalid headers format", value: "userId:alice", diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go index 38859fb9342..9dfb55c41bb 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go @@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.QueryUnescape(n) + name, err := url.PathUnescape(n) if err != nil { global.Error(err, "escape header key", "key", n) continue } trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(v) + value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) continue diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go index cec506208d5..6cbe0c7ab11 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go @@ -427,7 +427,12 @@ func TestStringToHeader(t *testing.T) { want: map[string]string{"userId": "alice"}, }, { - name: "multiples headers encoded", + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + }, + { + name: "multiple headers encoded", value: "userId=alice,serverNode=DF%3A28,isProduction=false", want: map[string]string{ "userId": "alice", @@ -435,6 +440,16 @@ func TestStringToHeader(t *testing.T) { "isProduction": "false", }, }, + { + name: "multiple headers encoded per RFC 3986 spec", + value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test", + want: map[string]string{ + "userId": "alice+test", + "serverNode": "DF:28", + "isProduction": "false", + "namespace": "localhost/test", + }, + }, { name: "invalid headers format", value: "userId:alice", diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go index becb1f0fbbe..5530119e4cf 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go @@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.QueryUnescape(n) + name, err := url.PathUnescape(n) if err != nil { global.Error(err, "escape header key", "key", n) continue } trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(v) + value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) continue diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go index cec506208d5..6cbe0c7ab11 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go @@ -427,7 +427,12 @@ func TestStringToHeader(t *testing.T) { want: map[string]string{"userId": "alice"}, }, { - name: "multiples headers encoded", + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + }, + { + name: "multiple headers encoded", value: "userId=alice,serverNode=DF%3A28,isProduction=false", want: map[string]string{ "userId": "alice", @@ -435,6 +440,16 @@ func TestStringToHeader(t *testing.T) { "isProduction": "false", }, }, + { + name: "multiple headers encoded per RFC 3986 spec", + value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test", + want: map[string]string{ + "userId": "alice+test", + "serverNode": "DF:28", + "isProduction": "false", + "namespace": "localhost/test", + }, + }, { name: "invalid headers format", value: "userId:alice", diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go index 5e9e8185d15..8016b7a0b88 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go @@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.QueryUnescape(n) + name, err := url.PathUnescape(n) if err != nil { global.Error(err, "escape header key", "key", n) continue } trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(v) + value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) continue diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go index cec506208d5..6cbe0c7ab11 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go @@ -427,7 +427,12 @@ func TestStringToHeader(t *testing.T) { want: map[string]string{"userId": "alice"}, }, { - name: "multiples headers encoded", + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + }, + { + name: "multiple headers encoded", value: "userId=alice,serverNode=DF%3A28,isProduction=false", want: map[string]string{ "userId": "alice", @@ -435,6 +440,16 @@ func TestStringToHeader(t *testing.T) { "isProduction": "false", }, }, + { + name: "multiple headers encoded per RFC 3986 spec", + value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test", + want: map[string]string{ + "userId": "alice+test", + "serverNode": "DF:28", + "isProduction": "false", + "namespace": "localhost/test", + }, + }, { name: "invalid headers format", value: "userId:alice", diff --git a/internal/shared/otlp/envconfig/envconfig.go.tmpl b/internal/shared/otlp/envconfig/envconfig.go.tmpl index 480f5f3cfd0..b516e9ca2c3 100644 --- a/internal/shared/otlp/envconfig/envconfig.go.tmpl +++ b/internal/shared/otlp/envconfig/envconfig.go.tmpl @@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.QueryUnescape(n) + name, err := url.PathUnescape(n) if err != nil { global.Error(err, "escape header key", "key", n) continue } trimmedName := strings.TrimSpace(name) - value, err := url.QueryUnescape(v) + value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) continue diff --git a/internal/shared/otlp/envconfig/envconfig_test.go.tmpl b/internal/shared/otlp/envconfig/envconfig_test.go.tmpl index cec506208d5..6cbe0c7ab11 100644 --- a/internal/shared/otlp/envconfig/envconfig_test.go.tmpl +++ b/internal/shared/otlp/envconfig/envconfig_test.go.tmpl @@ -427,7 +427,12 @@ func TestStringToHeader(t *testing.T) { want: map[string]string{"userId": "alice"}, }, { - name: "multiples headers encoded", + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + }, + { + name: "multiple headers encoded", value: "userId=alice,serverNode=DF%3A28,isProduction=false", want: map[string]string{ "userId": "alice", @@ -435,6 +440,16 @@ func TestStringToHeader(t *testing.T) { "isProduction": "false", }, }, + { + name: "multiple headers encoded per RFC 3986 spec", + value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test", + want: map[string]string{ + "userId": "alice+test", + "serverNode": "DF:28", + "isProduction": "false", + "namespace": "localhost/test", + }, + }, { name: "invalid headers format", value: "userId:alice", diff --git a/sdk/resource/env.go b/sdk/resource/env.go index 7e49ed58116..e29ae563a69 100644 --- a/sdk/resource/env.go +++ b/sdk/resource/env.go @@ -89,7 +89,7 @@ func constructOTResources(s string) (*Resource, error) { continue } key := strings.TrimSpace(k) - val, err := url.QueryUnescape(strings.TrimSpace(v)) + val, err := url.PathUnescape(strings.TrimSpace(v)) if err != nil { // Retain original value if decoding fails, otherwise it will be // an empty string. diff --git a/sdk/resource/env_test.go b/sdk/resource/env_test.go index e47aaa5babd..a6754f74bc6 100644 --- a/sdk/resource/env_test.go +++ b/sdk/resource/env_test.go @@ -40,6 +40,19 @@ func TestDetectOnePair(t *testing.T) { assert.Equal(t, NewSchemaless(attribute.String("key", "value")), res) } +func TestDetectURIEncodingOnePair(t *testing.T) { + store, err := ottest.SetEnvVariables(map[string]string{ + resourceAttrKey: "key=x+y+z?q=123", + }) + require.NoError(t, err) + defer func() { require.NoError(t, store.Restore()) }() + + detector := &fromEnv{} + res, err := detector.Detect(context.Background()) + require.NoError(t, err) + assert.Equal(t, NewSchemaless(attribute.String("key", "x+y+z?q=123")), res) +} + func TestDetectMultiPairs(t *testing.T) { store, err := ottest.SetEnvVariables(map[string]string{ "x": "1", @@ -60,6 +73,23 @@ func TestDetectMultiPairs(t *testing.T) { ), res) } +func TestDetectURIEncodingMultiPairs(t *testing.T) { + store, err := ottest.SetEnvVariables(map[string]string{ + "x": "1", + resourceAttrKey: "key=x+y+z,namespace=localhost/test&verify", + }) + require.NoError(t, err) + defer func() { require.NoError(t, store.Restore()) }() + + detector := &fromEnv{} + res, err := detector.Detect(context.Background()) + require.NoError(t, err) + assert.Equal(t, NewSchemaless( + attribute.String("key", "x+y+z"), + attribute.String("namespace", "localhost/test&verify"), + ), res) +} + func TestEmpty(t *testing.T) { store, err := ottest.SetEnvVariables(map[string]string{ resourceAttrKey: " ",