From 52d82725f3a3685cd85838035db312626cee45fd Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Sun, 2 Mar 2025 02:08:06 -0500 Subject: [PATCH 01/13] bug fix --- CHANGELOG.md | 4 +++ exporters/otlp/otlplog/otlploggrpc/config.go | 28 +++++++++++++++++-- .../otlp/otlplog/otlploggrpc/config_test.go | 1 - exporters/otlp/otlplog/otlploghttp/config.go | 28 +++++++++++++++++-- .../otlp/otlplog/otlploghttp/config_test.go | 1 - 5 files changed, 54 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f01d9796a5b..6ad5de6e1bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Drop support for [Go 1.22]. (#6381) +### Fixes + +- Stop percent encoding header environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#6392) + diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index cd33a168271..65c3eb750ea 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -13,6 +13,7 @@ import ( "strconv" "strings" "time" + "unicode" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -442,13 +443,15 @@ func convHeaders(s string) (map[string]string, error) { continue } - escKey, e := url.PathUnescape(rawKey) - if e != nil { + key := strings.TrimSpace(rawKey) + + // Validate the key. + if !isValidHeaderKey(key) { err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } - key := strings.TrimSpace(escKey) + // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) @@ -651,3 +654,22 @@ func fallback[T any](val T) resolver[T] { return s } } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlplog/otlploggrpc/config_test.go b/exporters/otlp/otlplog/otlploggrpc/config_test.go index cb21786b2c4..96de7f52dd1 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config_test.go +++ b/exporters/otlp/otlplog/otlploggrpc/config_test.go @@ -357,7 +357,6 @@ func TestNewConfig(t *testing.T) { `tls: failed to find any PEM data in certificate input`, `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`, `invalid header: a`, - `invalid header key: %ZZ`, `invalid header value: %ZZ`, `invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`, `invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`, diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index bfe768091e3..b8952272c13 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -14,6 +14,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp/internal/retry" @@ -544,13 +545,15 @@ func convHeaders(s string) (map[string]string, error) { continue } - escKey, e := url.PathUnescape(rawKey) - if e != nil { + key := strings.TrimSpace(rawKey) + + // Validate the key. + if !isValidHeaderKey(key) { err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } - key := strings.TrimSpace(escKey) + // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) @@ -600,3 +603,22 @@ func fallback[T any](val T) resolver[T] { return s } } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlplog/otlploghttp/config_test.go b/exporters/otlp/otlplog/otlploghttp/config_test.go index 1a7568921db..ff974b3783c 100644 --- a/exporters/otlp/otlplog/otlploghttp/config_test.go +++ b/exporters/otlp/otlplog/otlploghttp/config_test.go @@ -364,7 +364,6 @@ func TestNewConfig(t *testing.T) { `tls: failed to find any PEM data in certificate input`, `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`, `invalid header: a`, - `invalid header key: %ZZ`, `invalid header value: %ZZ`, `invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`, `invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`, From 337a320b8f6e4b0f8aad1e2f34d1208e2c3a2fc5 Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Mon, 3 Mar 2025 17:49:02 -0500 Subject: [PATCH 02/13] 1. Add more unit tests 2. Fix the code in order to parse valid header keys only 3. Add global error messages --- exporters/otlp/otlplog/otlploggrpc/config.go | 7 + .../otlp/otlplog/otlploggrpc/config_test.go | 120 ++++++++++++++++- exporters/otlp/otlplog/otlploghttp/config.go | 7 + .../otlp/otlplog/otlploghttp/config_test.go | 122 +++++++++++++++++- 4 files changed, 248 insertions(+), 8 deletions(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index 65c3eb750ea..2bca1595db0 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -439,6 +439,7 @@ func convHeaders(s string) (map[string]string, error) { for _, header := range strings.Split(s, ",") { rawKey, rawVal, found := strings.Cut(header, "=") if !found { + global.Error(errors.New("missing '="), "parse headers", "input", header) err = errors.Join(err, fmt.Errorf("invalid header: %s", header)) continue } @@ -447,6 +448,7 @@ func convHeaders(s string) (map[string]string, error) { // Validate the key. if !isValidHeaderKey(key) { + global.Error(errors.New("invalid header key"), "parse headers", "key", key) err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } @@ -454,6 +456,7 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { + global.Error(err, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } @@ -461,6 +464,10 @@ func convHeaders(s string) (map[string]string, error) { out[key] = val } + // If at least one valid header exists, reset err to nil + if len(out) > 0 { + err = nil + } return out, err } diff --git a/exporters/otlp/otlplog/otlploggrpc/config_test.go b/exporters/otlp/otlplog/otlploggrpc/config_test.go index 96de7f52dd1..59cee6fac12 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config_test.go +++ b/exporters/otlp/otlplog/otlploggrpc/config_test.go @@ -80,6 +80,8 @@ func TestNewConfig(t *testing.T) { require.NoError(t, err, "testing TLS config") headers := map[string]string{"a": "A"} + percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} + validHeaders := map[string]string{"valid-key": "value"} rc := retry.Config{} dialOptions := []grpc.DialOption{grpc.WithUserAgent("test-agent")} @@ -338,7 +340,7 @@ func TestNewConfig(t *testing.T) { name: "InvalidEnvironmentVariables", envars: map[string]string{ "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "%invalid", - "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "a,%ZZ=valid,key=%ZZ", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "invalid key=value", "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "xz", "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "100 seconds", "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "invalid_cert", @@ -355,9 +357,7 @@ func TestNewConfig(t *testing.T) { `failed to load TLS:`, `certificate not added`, `tls: failed to find any PEM data in certificate input`, - `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`, - `invalid header: a`, - `invalid header value: %ZZ`, + `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value invalid key=value: invalid header key: invalid key`, `invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`, `invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`, }, @@ -439,6 +439,48 @@ func TestNewConfig(t *testing.T) { timeout: newSetting(defaultTimeout), }, }, + { + name: "with percent-encoded headers", + envars: map[string]string{ + "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "user%2Did=42,user%20name=alice%20smith", + "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip", + "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000", + "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path", + }, + want: config{ + endpoint: newSetting("env.endpoint:8080"), + insecure: newSetting(false), + tlsCfg: newSetting(tlsCfg), + headers: newSetting(percentDecodedHeaders), + compression: newSetting(GzipCompression), + timeout: newSetting(15 * time.Second), + retryCfg: newSetting(defaultRetryCfg), + }, + }, + { + name: "with invalid header key", + envars: map[string]string{ + "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "valid-key=value,invalid key=value", + "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip", + "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000", + "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path", + }, + want: config{ + endpoint: newSetting("env.endpoint:8080"), + insecure: newSetting(false), + tlsCfg: newSetting(tlsCfg), + headers: newSetting(validHeaders), + compression: newSetting(GzipCompression), + timeout: newSetting(15 * time.Second), + retryCfg: newSetting(defaultRetryCfg), + }, + }, } for _, tc := range testcases { @@ -493,3 +535,73 @@ func assertTLSConfig(t *testing.T, want, got setting[*tls.Config]) { } assert.Equal(t, want.Value.Certificates, got.Value.Certificates, "Certificates") } + +func TestConvHeaders(t *testing.T) { + tests := []struct { + name string + value string + want map[string]string + }{ + { + name: "simple test", + value: "userId=alice", + want: map[string]string{"userId": "alice"}, + }, + { + name: "simple test with spaces", + value: " userId = alice ", + want: map[string]string{"userId": "alice"}, + }, + { + 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", + "serverNode": "DF:28", + "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", + want: map[string]string{}, + }, + { + name: "invalid key", + value: "%XX=missing,userId=alice", + want: map[string]string{ + "%XX": "missing", + "userId": "alice", + }, + }, + { + name: "invalid value", + value: "missing=%XX,userId=alice", + want: map[string]string{ + "userId": "alice", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + keyValues, _ := convHeaders(tt.value) + assert.Equal(t, tt.want, keyValues) + }) + } +} diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index b8952272c13..154730a3931 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -541,6 +541,7 @@ func convHeaders(s string) (map[string]string, error) { for _, header := range strings.Split(s, ",") { rawKey, rawVal, found := strings.Cut(header, "=") if !found { + global.Error(errors.New("missing '="), "parse headers", "input", header) err = errors.Join(err, fmt.Errorf("invalid header: %s", header)) continue } @@ -549,6 +550,7 @@ func convHeaders(s string) (map[string]string, error) { // Validate the key. if !isValidHeaderKey(key) { + global.Error(errors.New("invalid header key"), "parse headers", "key", key) err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } @@ -556,6 +558,7 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { + global.Error(err, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } @@ -563,6 +566,10 @@ func convHeaders(s string) (map[string]string, error) { out[key] = val } + // If at least one valid header exists, reset err to nil + if len(out) > 0 { + err = nil + } return out, err } diff --git a/exporters/otlp/otlplog/otlploghttp/config_test.go b/exporters/otlp/otlplog/otlploghttp/config_test.go index ff974b3783c..3fc6a048cd1 100644 --- a/exporters/otlp/otlplog/otlploghttp/config_test.go +++ b/exporters/otlp/otlplog/otlploghttp/config_test.go @@ -80,6 +80,8 @@ func TestNewConfig(t *testing.T) { require.NoError(t, err, "testing TLS config") headers := map[string]string{"a": "A"} + percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} + validHeaders := map[string]string{"valid-key": "value"} rc := retry.Config{} testcases := []struct { @@ -344,7 +346,7 @@ func TestNewConfig(t *testing.T) { name: "InvalidEnvironmentVariables", envars: map[string]string{ "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "%invalid", - "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "a,%ZZ=valid,key=%ZZ", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "invalid key=value", "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "xz", "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "100 seconds", "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "invalid_cert", @@ -362,13 +364,55 @@ func TestNewConfig(t *testing.T) { `failed to load TLS:`, `certificate not added`, `tls: failed to find any PEM data in certificate input`, - `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`, - `invalid header: a`, - `invalid header value: %ZZ`, + `invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value invalid key=value: invalid header key: invalid key`, `invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`, `invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`, }, }, + { + name: "with percent-encoded headers", + envars: map[string]string{ + "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "user%2Did=42,user%20name=alice%20smith", + "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip", + "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000", + "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path", + }, + want: config{ + endpoint: newSetting("env.endpoint:8080"), + path: newSetting("/prefix"), + insecure: newSetting(false), + tlsCfg: newSetting(tlsCfg), + headers: newSetting(percentDecodedHeaders), + compression: newSetting(GzipCompression), + timeout: newSetting(15 * time.Second), + retryCfg: newSetting(defaultRetryCfg), + }, + }, + { + name: "with invalid header key", + envars: map[string]string{ + "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix", + "OTEL_EXPORTER_OTLP_LOGS_HEADERS": "valid-key=value,invalid key=value", + "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip", + "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000", + "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path", + "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path", + }, + want: config{ + endpoint: newSetting("env.endpoint:8080"), + path: newSetting("/prefix"), + insecure: newSetting(false), + tlsCfg: newSetting(tlsCfg), + headers: newSetting(validHeaders), + compression: newSetting(GzipCompression), + timeout: newSetting(15 * time.Second), + retryCfg: newSetting(defaultRetryCfg), + }, + }, } for _, tc := range testcases { @@ -435,3 +479,73 @@ func TestWithProxy(t *testing.T) { assert.True(t, c.proxy.Set) assert.NotNil(t, c.proxy.Value) } + +func TestConvHeaders(t *testing.T) { + tests := []struct { + name string + value string + want map[string]string + }{ + { + name: "simple test", + value: "userId=alice", + want: map[string]string{"userId": "alice"}, + }, + { + name: "simple test with spaces", + value: " userId = alice ", + want: map[string]string{"userId": "alice"}, + }, + { + 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", + "serverNode": "DF:28", + "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", + want: map[string]string{}, + }, + { + name: "invalid key", + value: "%XX=missing,userId=alice", + want: map[string]string{ + "%XX": "missing", + "userId": "alice", + }, + }, + { + name: "invalid value", + value: "missing=%XX,userId=alice", + want: map[string]string{ + "userId": "alice", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + keyValues, _ := convHeaders(tt.value) + assert.Equal(t, tt.want, keyValues) + }) + } +} From 0a50e3ef1b5a56ae404305ad58dcc8897361c619 Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Mon, 3 Mar 2025 23:19:44 -0500 Subject: [PATCH 03/13] update global.Error message --- exporters/otlp/otlplog/otlploggrpc/config.go | 2 +- exporters/otlp/otlplog/otlploghttp/config.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index 2bca1595db0..229317f4e5b 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -456,7 +456,7 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { - global.Error(err, "escape header value", "value", rawVal) + global.Error(e, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index 154730a3931..d89a98d07f8 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -558,7 +558,7 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { - global.Error(err, "escape header value", "value", rawVal) + global.Error(e, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } From 0d956e55c1bd32121ea6dfc5be96dba5f14b0672 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:48:09 -0500 Subject: [PATCH 04/13] Update exporters/otlp/otlplog/otlploggrpc/config.go Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploggrpc/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index 229317f4e5b..d03967a7c41 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -439,7 +439,6 @@ func convHeaders(s string) (map[string]string, error) { for _, header := range strings.Split(s, ",") { rawKey, rawVal, found := strings.Cut(header, "=") if !found { - global.Error(errors.New("missing '="), "parse headers", "input", header) err = errors.Join(err, fmt.Errorf("invalid header: %s", header)) continue } From bcf5fecbc739841f2280f6173325ab74ac62ff00 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:48:43 -0500 Subject: [PATCH 05/13] Update exporters/otlp/otlplog/otlploggrpc/config.go Apply the suggested changes Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploggrpc/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index d03967a7c41..553433e16e4 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -447,7 +447,6 @@ func convHeaders(s string) (map[string]string, error) { // Validate the key. if !isValidHeaderKey(key) { - global.Error(errors.New("invalid header key"), "parse headers", "key", key) err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } From 28b03f4db550c295aa1537ad3edb471bbff2c326 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:48:58 -0500 Subject: [PATCH 06/13] Update exporters/otlp/otlplog/otlploggrpc/config.go Apply the suggested changes Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploggrpc/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index 553433e16e4..912acf05cf0 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -454,7 +454,6 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { - global.Error(e, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } From fb22d991ae1073f4a713ae0016be59cbc9886df7 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:49:15 -0500 Subject: [PATCH 07/13] Update exporters/otlp/otlplog/otlploghttp/config.go Apply the suggested changes Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploghttp/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index d89a98d07f8..99ccdf50e3b 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -541,7 +541,6 @@ func convHeaders(s string) (map[string]string, error) { for _, header := range strings.Split(s, ",") { rawKey, rawVal, found := strings.Cut(header, "=") if !found { - global.Error(errors.New("missing '="), "parse headers", "input", header) err = errors.Join(err, fmt.Errorf("invalid header: %s", header)) continue } From 9cbacc6bdb102cd32a4da6394cce6218325cda97 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:49:30 -0500 Subject: [PATCH 08/13] Update exporters/otlp/otlplog/otlploghttp/config.go Apply the suggested changes Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploghttp/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index 99ccdf50e3b..c23191ea29e 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -549,7 +549,6 @@ func convHeaders(s string) (map[string]string, error) { // Validate the key. if !isValidHeaderKey(key) { - global.Error(errors.New("invalid header key"), "parse headers", "key", key) err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey)) continue } From c6f0fa374eed690595298561a0acd8873b678e56 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:49:43 -0500 Subject: [PATCH 09/13] Update exporters/otlp/otlplog/otlploghttp/config.go Apply the suggested changes Co-authored-by: Damien Mathieu <42@dmathieu.com> --- exporters/otlp/otlplog/otlploghttp/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index c23191ea29e..69512a6cc22 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -556,7 +556,6 @@ func convHeaders(s string) (map[string]string, error) { // Only decode the value. escVal, e := url.PathUnescape(rawVal) if e != nil { - global.Error(e, "escape header value", "value", rawVal) err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal)) continue } From edcfc4376247ad4565c1aed332d3b7513ed841bb Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Wed, 5 Mar 2025 14:03:43 -0500 Subject: [PATCH 10/13] Add global warning message before nullify the error --- exporters/otlp/otlplog/otlploggrpc/config.go | 1 + exporters/otlp/otlplog/otlploghttp/config.go | 1 + 2 files changed, 2 insertions(+) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index 912acf05cf0..e5f4209b9ca 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -463,6 +463,7 @@ func convHeaders(s string) (map[string]string, error) { } // If at least one valid header exists, reset err to nil if len(out) > 0 { + global.Warn("Some headers were invalid but at least one valid header exists: %v", err) err = nil } return out, err diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index 69512a6cc22..4d8f0d7333a 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -565,6 +565,7 @@ func convHeaders(s string) (map[string]string, error) { } // If at least one valid header exists, reset err to nil if len(out) > 0 { + global.Warn("Some headers were invalid but at least one valid header exists: %v", err) err = nil } return out, err From 6889ded10fd3c16542f246b4e2e52544d4d5da70 Mon Sep 17 00:00:00 2001 From: Robert Wu <94589791+tongoss@users.noreply.github.com> Date: Thu, 6 Mar 2025 17:57:28 -0500 Subject: [PATCH 11/13] Update exporters/otlp/otlplog/otlploggrpc/config.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- exporters/otlp/otlplog/otlploggrpc/config.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config.go b/exporters/otlp/otlplog/otlploggrpc/config.go index e5f4209b9ca..65c3eb750ea 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config.go +++ b/exporters/otlp/otlplog/otlploggrpc/config.go @@ -461,11 +461,6 @@ func convHeaders(s string) (map[string]string, error) { out[key] = val } - // If at least one valid header exists, reset err to nil - if len(out) > 0 { - global.Warn("Some headers were invalid but at least one valid header exists: %v", err) - err = nil - } return out, err } From fe68066109f6a24d9560ac4096321d316167e1bd Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Thu, 6 Mar 2025 18:01:46 -0500 Subject: [PATCH 12/13] Remove the code for nullifying err. Update unit test: return none if both invalid and valid headers exist --- exporters/otlp/otlplog/otlploggrpc/config_test.go | 2 -- exporters/otlp/otlplog/otlploghttp/config.go | 5 ----- exporters/otlp/otlplog/otlploghttp/config_test.go | 2 -- 3 files changed, 9 deletions(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config_test.go b/exporters/otlp/otlplog/otlploggrpc/config_test.go index 59cee6fac12..86fcd9701f6 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config_test.go +++ b/exporters/otlp/otlplog/otlploggrpc/config_test.go @@ -81,7 +81,6 @@ func TestNewConfig(t *testing.T) { headers := map[string]string{"a": "A"} percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} - validHeaders := map[string]string{"valid-key": "value"} rc := retry.Config{} dialOptions := []grpc.DialOption{grpc.WithUserAgent("test-agent")} @@ -475,7 +474,6 @@ func TestNewConfig(t *testing.T) { endpoint: newSetting("env.endpoint:8080"), insecure: newSetting(false), tlsCfg: newSetting(tlsCfg), - headers: newSetting(validHeaders), compression: newSetting(GzipCompression), timeout: newSetting(15 * time.Second), retryCfg: newSetting(defaultRetryCfg), diff --git a/exporters/otlp/otlplog/otlploghttp/config.go b/exporters/otlp/otlplog/otlploghttp/config.go index 4d8f0d7333a..b8952272c13 100644 --- a/exporters/otlp/otlplog/otlploghttp/config.go +++ b/exporters/otlp/otlplog/otlploghttp/config.go @@ -563,11 +563,6 @@ func convHeaders(s string) (map[string]string, error) { out[key] = val } - // If at least one valid header exists, reset err to nil - if len(out) > 0 { - global.Warn("Some headers were invalid but at least one valid header exists: %v", err) - err = nil - } return out, err } diff --git a/exporters/otlp/otlplog/otlploghttp/config_test.go b/exporters/otlp/otlplog/otlploghttp/config_test.go index 3fc6a048cd1..f05fb59e078 100644 --- a/exporters/otlp/otlplog/otlploghttp/config_test.go +++ b/exporters/otlp/otlplog/otlploghttp/config_test.go @@ -81,7 +81,6 @@ func TestNewConfig(t *testing.T) { headers := map[string]string{"a": "A"} percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} - validHeaders := map[string]string{"valid-key": "value"} rc := retry.Config{} testcases := []struct { @@ -407,7 +406,6 @@ func TestNewConfig(t *testing.T) { path: newSetting("/prefix"), insecure: newSetting(false), tlsCfg: newSetting(tlsCfg), - headers: newSetting(validHeaders), compression: newSetting(GzipCompression), timeout: newSetting(15 * time.Second), retryCfg: newSetting(defaultRetryCfg), From 2d57f4c87e43d78bc74b2ee9cd325c8b360a9407 Mon Sep 17 00:00:00 2001 From: Robert Wu Date: Fri, 7 Mar 2025 17:40:45 -0500 Subject: [PATCH 13/13] Include TestConvHeaders for err assertion. Update percent-encoded headers test case --- .../otlp/otlplog/otlploggrpc/config_test.go | 50 ++++++++++++------- .../otlp/otlplog/otlploghttp/config_test.go | 50 ++++++++++++------- 2 files changed, 64 insertions(+), 36 deletions(-) diff --git a/exporters/otlp/otlplog/otlploggrpc/config_test.go b/exporters/otlp/otlplog/otlploggrpc/config_test.go index 86fcd9701f6..e7a0cf1c9b4 100644 --- a/exporters/otlp/otlplog/otlploggrpc/config_test.go +++ b/exporters/otlp/otlplog/otlploggrpc/config_test.go @@ -80,7 +80,6 @@ func TestNewConfig(t *testing.T) { require.NoError(t, err, "testing TLS config") headers := map[string]string{"a": "A"} - percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} rc := retry.Config{} dialOptions := []grpc.DialOption{grpc.WithUserAgent("test-agent")} @@ -453,7 +452,7 @@ func TestNewConfig(t *testing.T) { endpoint: newSetting("env.endpoint:8080"), insecure: newSetting(false), tlsCfg: newSetting(tlsCfg), - headers: newSetting(percentDecodedHeaders), + headers: newSetting(map[string]string{"user%2Did": "42", "user%20name": "alice smith"}), compression: newSetting(GzipCompression), timeout: newSetting(15 * time.Second), retryCfg: newSetting(defaultRetryCfg), @@ -536,24 +535,28 @@ func assertTLSConfig(t *testing.T, want, got setting[*tls.Config]) { func TestConvHeaders(t *testing.T) { tests := []struct { - name string - value string - want map[string]string + name string + value string + want map[string]string + wantErr bool }{ { - name: "simple test", - value: "userId=alice", - want: map[string]string{"userId": "alice"}, + name: "simple test", + value: "userId=alice", + want: map[string]string{"userId": "alice"}, + wantErr: false, }, { - name: "simple test with spaces", - value: " userId = alice ", - want: map[string]string{"userId": "alice"}, + name: "simple test with spaces", + value: " userId = alice ", + want: map[string]string{"userId": "alice"}, + wantErr: false, }, { - name: "simple header conforms to RFC 3986 spec", - value: " userId = alice+test ", - want: map[string]string{"userId": "alice+test"}, + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + wantErr: false, }, { name: "multiple headers encoded", @@ -563,6 +566,7 @@ func TestConvHeaders(t *testing.T) { "serverNode": "DF:28", "isProduction": "false", }, + wantErr: false, }, { name: "multiple headers encoded per RFC 3986 spec", @@ -573,11 +577,13 @@ func TestConvHeaders(t *testing.T) { "isProduction": "false", "namespace": "localhost/test", }, + wantErr: false, }, { - name: "invalid headers format", - value: "userId:alice", - want: map[string]string{}, + name: "invalid headers format", + value: "userId:alice", + want: map[string]string{}, + wantErr: true, }, { name: "invalid key", @@ -586,6 +592,7 @@ func TestConvHeaders(t *testing.T) { "%XX": "missing", "userId": "alice", }, + wantErr: false, }, { name: "invalid value", @@ -593,13 +600,20 @@ func TestConvHeaders(t *testing.T) { want: map[string]string{ "userId": "alice", }, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - keyValues, _ := convHeaders(tt.value) + keyValues, err := convHeaders(tt.value) assert.Equal(t, tt.want, keyValues) + + if tt.wantErr { + assert.Error(t, err, "expected an error but got nil") + } else { + assert.NoError(t, err, "expected no error but got one") + } }) } } diff --git a/exporters/otlp/otlplog/otlploghttp/config_test.go b/exporters/otlp/otlplog/otlploghttp/config_test.go index f05fb59e078..a6863e3805a 100644 --- a/exporters/otlp/otlplog/otlploghttp/config_test.go +++ b/exporters/otlp/otlplog/otlploghttp/config_test.go @@ -80,7 +80,6 @@ func TestNewConfig(t *testing.T) { require.NoError(t, err, "testing TLS config") headers := map[string]string{"a": "A"} - percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"} rc := retry.Config{} testcases := []struct { @@ -384,7 +383,7 @@ func TestNewConfig(t *testing.T) { path: newSetting("/prefix"), insecure: newSetting(false), tlsCfg: newSetting(tlsCfg), - headers: newSetting(percentDecodedHeaders), + headers: newSetting(map[string]string{"user%2Did": "42", "user%20name": "alice smith"}), compression: newSetting(GzipCompression), timeout: newSetting(15 * time.Second), retryCfg: newSetting(defaultRetryCfg), @@ -480,24 +479,28 @@ func TestWithProxy(t *testing.T) { func TestConvHeaders(t *testing.T) { tests := []struct { - name string - value string - want map[string]string + name string + value string + want map[string]string + wantErr bool }{ { - name: "simple test", - value: "userId=alice", - want: map[string]string{"userId": "alice"}, + name: "simple test", + value: "userId=alice", + want: map[string]string{"userId": "alice"}, + wantErr: false, }, { - name: "simple test with spaces", - value: " userId = alice ", - want: map[string]string{"userId": "alice"}, + name: "simple test with spaces", + value: " userId = alice ", + want: map[string]string{"userId": "alice"}, + wantErr: false, }, { - name: "simple header conforms to RFC 3986 spec", - value: " userId = alice+test ", - want: map[string]string{"userId": "alice+test"}, + name: "simple header conforms to RFC 3986 spec", + value: " userId = alice+test ", + want: map[string]string{"userId": "alice+test"}, + wantErr: false, }, { name: "multiple headers encoded", @@ -507,6 +510,7 @@ func TestConvHeaders(t *testing.T) { "serverNode": "DF:28", "isProduction": "false", }, + wantErr: false, }, { name: "multiple headers encoded per RFC 3986 spec", @@ -517,11 +521,13 @@ func TestConvHeaders(t *testing.T) { "isProduction": "false", "namespace": "localhost/test", }, + wantErr: false, }, { - name: "invalid headers format", - value: "userId:alice", - want: map[string]string{}, + name: "invalid headers format", + value: "userId:alice", + want: map[string]string{}, + wantErr: true, }, { name: "invalid key", @@ -530,6 +536,7 @@ func TestConvHeaders(t *testing.T) { "%XX": "missing", "userId": "alice", }, + wantErr: false, }, { name: "invalid value", @@ -537,13 +544,20 @@ func TestConvHeaders(t *testing.T) { want: map[string]string{ "userId": "alice", }, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - keyValues, _ := convHeaders(tt.value) + keyValues, err := convHeaders(tt.value) assert.Equal(t, tt.want, keyValues) + + if tt.wantErr { + assert.Error(t, err, "expected an error but got nil") + } else { + assert.NoError(t, err, "expected no error but got one") + } }) } }