Skip to content

Commit 337a320

Browse files
tongosstongoss
committed
1. Add more unit tests
2. Fix the code in order to parse valid header keys only 3. Add global error messages
1 parent 52d8272 commit 337a320

File tree

4 files changed

+248
-8
lines changed

4 files changed

+248
-8
lines changed

‎exporters/otlp/otlplog/otlploggrpc/config.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,7 @@ func convHeaders(s string) (map[string]string, error) {
439439
for _, header := range strings.Split(s, ",") {
440440
rawKey, rawVal, found := strings.Cut(header, "=")
441441
if !found {
442+
global.Error(errors.New("missing '="), "parse headers", "input", header)
442443
err = errors.Join(err, fmt.Errorf("invalid header: %s", header))
443444
continue
444445
}
@@ -447,20 +448,26 @@ func convHeaders(s string) (map[string]string, error) {
447448

448449
// Validate the key.
449450
if !isValidHeaderKey(key) {
451+
global.Error(errors.New("invalid header key"), "parse headers", "key", key)
450452
err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey))
451453
continue
452454
}
453455

454456
// Only decode the value.
455457
escVal, e := url.PathUnescape(rawVal)
456458
if e != nil {
459+
global.Error(err, "escape header value", "value", rawVal)
457460
err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal))
458461
continue
459462
}
460463
val := strings.TrimSpace(escVal)
461464

462465
out[key] = val
463466
}
467+
// If at least one valid header exists, reset err to nil
468+
if len(out) > 0 {
469+
err = nil
470+
}
464471
return out, err
465472
}
466473

‎exporters/otlp/otlplog/otlploggrpc/config_test.go

Lines changed: 116 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ func TestNewConfig(t *testing.T) {
8080
require.NoError(t, err, "testing TLS config")
8181

8282
headers := map[string]string{"a": "A"}
83+
percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"}
84+
validHeaders := map[string]string{"valid-key": "value"}
8385
rc := retry.Config{}
8486

8587
dialOptions := []grpc.DialOption{grpc.WithUserAgent("test-agent")}
@@ -338,7 +340,7 @@ func TestNewConfig(t *testing.T) {
338340
name: "InvalidEnvironmentVariables",
339341
envars: map[string]string{
340342
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "%invalid",
341-
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "a,%ZZ=valid,key=%ZZ",
343+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "invalid key=value",
342344
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "xz",
343345
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "100 seconds",
344346
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "invalid_cert",
@@ -355,9 +357,7 @@ func TestNewConfig(t *testing.T) {
355357
`failed to load TLS:`,
356358
`certificate not added`,
357359
`tls: failed to find any PEM data in certificate input`,
358-
`invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`,
359-
`invalid header: a`,
360-
`invalid header value: %ZZ`,
360+
`invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value invalid key=value: invalid header key: invalid key`,
361361
`invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`,
362362
`invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`,
363363
},
@@ -439,6 +439,48 @@ func TestNewConfig(t *testing.T) {
439439
timeout: newSetting(defaultTimeout),
440440
},
441441
},
442+
{
443+
name: "with percent-encoded headers",
444+
envars: map[string]string{
445+
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix",
446+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "user%2Did=42,user%20name=alice%20smith",
447+
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip",
448+
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000",
449+
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path",
450+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path",
451+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path",
452+
},
453+
want: config{
454+
endpoint: newSetting("env.endpoint:8080"),
455+
insecure: newSetting(false),
456+
tlsCfg: newSetting(tlsCfg),
457+
headers: newSetting(percentDecodedHeaders),
458+
compression: newSetting(GzipCompression),
459+
timeout: newSetting(15 * time.Second),
460+
retryCfg: newSetting(defaultRetryCfg),
461+
},
462+
},
463+
{
464+
name: "with invalid header key",
465+
envars: map[string]string{
466+
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix",
467+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "valid-key=value,invalid key=value",
468+
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip",
469+
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000",
470+
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path",
471+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path",
472+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path",
473+
},
474+
want: config{
475+
endpoint: newSetting("env.endpoint:8080"),
476+
insecure: newSetting(false),
477+
tlsCfg: newSetting(tlsCfg),
478+
headers: newSetting(validHeaders),
479+
compression: newSetting(GzipCompression),
480+
timeout: newSetting(15 * time.Second),
481+
retryCfg: newSetting(defaultRetryCfg),
482+
},
483+
},
442484
}
443485

444486
for _, tc := range testcases {
@@ -493,3 +535,73 @@ func assertTLSConfig(t *testing.T, want, got setting[*tls.Config]) {
493535
}
494536
assert.Equal(t, want.Value.Certificates, got.Value.Certificates, "Certificates")
495537
}
538+
539+
func TestConvHeaders(t *testing.T) {
540+
tests := []struct {
541+
name string
542+
value string
543+
want map[string]string
544+
}{
545+
{
546+
name: "simple test",
547+
value: "userId=alice",
548+
want: map[string]string{"userId": "alice"},
549+
},
550+
{
551+
name: "simple test with spaces",
552+
value: " userId = alice ",
553+
want: map[string]string{"userId": "alice"},
554+
},
555+
{
556+
name: "simple header conforms to RFC 3986 spec",
557+
value: " userId = alice+test ",
558+
want: map[string]string{"userId": "alice+test"},
559+
},
560+
{
561+
name: "multiple headers encoded",
562+
value: "userId=alice,serverNode=DF%3A28,isProduction=false",
563+
want: map[string]string{
564+
"userId": "alice",
565+
"serverNode": "DF:28",
566+
"isProduction": "false",
567+
},
568+
},
569+
{
570+
name: "multiple headers encoded per RFC 3986 spec",
571+
value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
572+
want: map[string]string{
573+
"userId": "alice+test",
574+
"serverNode": "DF:28",
575+
"isProduction": "false",
576+
"namespace": "localhost/test",
577+
},
578+
},
579+
{
580+
name: "invalid headers format",
581+
value: "userId:alice",
582+
want: map[string]string{},
583+
},
584+
{
585+
name: "invalid key",
586+
value: "%XX=missing,userId=alice",
587+
want: map[string]string{
588+
"%XX": "missing",
589+
"userId": "alice",
590+
},
591+
},
592+
{
593+
name: "invalid value",
594+
value: "missing=%XX,userId=alice",
595+
want: map[string]string{
596+
"userId": "alice",
597+
},
598+
},
599+
}
600+
601+
for _, tt := range tests {
602+
t.Run(tt.name, func(t *testing.T) {
603+
keyValues, _ := convHeaders(tt.value)
604+
assert.Equal(t, tt.want, keyValues)
605+
})
606+
}
607+
}

‎exporters/otlp/otlplog/otlploghttp/config.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,7 @@ func convHeaders(s string) (map[string]string, error) {
541541
for _, header := range strings.Split(s, ",") {
542542
rawKey, rawVal, found := strings.Cut(header, "=")
543543
if !found {
544+
global.Error(errors.New("missing '="), "parse headers", "input", header)
544545
err = errors.Join(err, fmt.Errorf("invalid header: %s", header))
545546
continue
546547
}
@@ -549,20 +550,26 @@ func convHeaders(s string) (map[string]string, error) {
549550

550551
// Validate the key.
551552
if !isValidHeaderKey(key) {
553+
global.Error(errors.New("invalid header key"), "parse headers", "key", key)
552554
err = errors.Join(err, fmt.Errorf("invalid header key: %s", rawKey))
553555
continue
554556
}
555557

556558
// Only decode the value.
557559
escVal, e := url.PathUnescape(rawVal)
558560
if e != nil {
561+
global.Error(err, "escape header value", "value", rawVal)
559562
err = errors.Join(err, fmt.Errorf("invalid header value: %s", rawVal))
560563
continue
561564
}
562565
val := strings.TrimSpace(escVal)
563566

564567
out[key] = val
565568
}
569+
// If at least one valid header exists, reset err to nil
570+
if len(out) > 0 {
571+
err = nil
572+
}
566573
return out, err
567574
}
568575

‎exporters/otlp/otlplog/otlploghttp/config_test.go

Lines changed: 118 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ func TestNewConfig(t *testing.T) {
8080
require.NoError(t, err, "testing TLS config")
8181

8282
headers := map[string]string{"a": "A"}
83+
percentDecodedHeaders := map[string]string{"user%2Did": "42", "user%20name": "alice smith"}
84+
validHeaders := map[string]string{"valid-key": "value"}
8385
rc := retry.Config{}
8486

8587
testcases := []struct {
@@ -344,7 +346,7 @@ func TestNewConfig(t *testing.T) {
344346
name: "InvalidEnvironmentVariables",
345347
envars: map[string]string{
346348
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "%invalid",
347-
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "a,%ZZ=valid,key=%ZZ",
349+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "invalid key=value",
348350
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "xz",
349351
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "100 seconds",
350352
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "invalid_cert",
@@ -362,13 +364,55 @@ func TestNewConfig(t *testing.T) {
362364
`failed to load TLS:`,
363365
`certificate not added`,
364366
`tls: failed to find any PEM data in certificate input`,
365-
`invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value a,%ZZ=valid,key=%ZZ:`,
366-
`invalid header: a`,
367-
`invalid header value: %ZZ`,
367+
`invalid OTEL_EXPORTER_OTLP_LOGS_HEADERS value invalid key=value: invalid header key: invalid key`,
368368
`invalid OTEL_EXPORTER_OTLP_LOGS_COMPRESSION value xz: unknown compression: xz`,
369369
`invalid OTEL_EXPORTER_OTLP_LOGS_TIMEOUT value 100 seconds: strconv.Atoi: parsing "100 seconds": invalid syntax`,
370370
},
371371
},
372+
{
373+
name: "with percent-encoded headers",
374+
envars: map[string]string{
375+
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix",
376+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "user%2Did=42,user%20name=alice%20smith",
377+
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip",
378+
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000",
379+
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path",
380+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path",
381+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path",
382+
},
383+
want: config{
384+
endpoint: newSetting("env.endpoint:8080"),
385+
path: newSetting("/prefix"),
386+
insecure: newSetting(false),
387+
tlsCfg: newSetting(tlsCfg),
388+
headers: newSetting(percentDecodedHeaders),
389+
compression: newSetting(GzipCompression),
390+
timeout: newSetting(15 * time.Second),
391+
retryCfg: newSetting(defaultRetryCfg),
392+
},
393+
},
394+
{
395+
name: "with invalid header key",
396+
envars: map[string]string{
397+
"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT": "https://env.endpoint:8080/prefix",
398+
"OTEL_EXPORTER_OTLP_LOGS_HEADERS": "valid-key=value,invalid key=value",
399+
"OTEL_EXPORTER_OTLP_LOGS_COMPRESSION": "gzip",
400+
"OTEL_EXPORTER_OTLP_LOGS_TIMEOUT": "15000",
401+
"OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE": "cert_path",
402+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE": "cert_path",
403+
"OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY": "key_path",
404+
},
405+
want: config{
406+
endpoint: newSetting("env.endpoint:8080"),
407+
path: newSetting("/prefix"),
408+
insecure: newSetting(false),
409+
tlsCfg: newSetting(tlsCfg),
410+
headers: newSetting(validHeaders),
411+
compression: newSetting(GzipCompression),
412+
timeout: newSetting(15 * time.Second),
413+
retryCfg: newSetting(defaultRetryCfg),
414+
},
415+
},
372416
}
373417

374418
for _, tc := range testcases {
@@ -435,3 +479,73 @@ func TestWithProxy(t *testing.T) {
435479
assert.True(t, c.proxy.Set)
436480
assert.NotNil(t, c.proxy.Value)
437481
}
482+
483+
func TestConvHeaders(t *testing.T) {
484+
tests := []struct {
485+
name string
486+
value string
487+
want map[string]string
488+
}{
489+
{
490+
name: "simple test",
491+
value: "userId=alice",
492+
want: map[string]string{"userId": "alice"},
493+
},
494+
{
495+
name: "simple test with spaces",
496+
value: " userId = alice ",
497+
want: map[string]string{"userId": "alice"},
498+
},
499+
{
500+
name: "simple header conforms to RFC 3986 spec",
501+
value: " userId = alice+test ",
502+
want: map[string]string{"userId": "alice+test"},
503+
},
504+
{
505+
name: "multiple headers encoded",
506+
value: "userId=alice,serverNode=DF%3A28,isProduction=false",
507+
want: map[string]string{
508+
"userId": "alice",
509+
"serverNode": "DF:28",
510+
"isProduction": "false",
511+
},
512+
},
513+
{
514+
name: "multiple headers encoded per RFC 3986 spec",
515+
value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
516+
want: map[string]string{
517+
"userId": "alice+test",
518+
"serverNode": "DF:28",
519+
"isProduction": "false",
520+
"namespace": "localhost/test",
521+
},
522+
},
523+
{
524+
name: "invalid headers format",
525+
value: "userId:alice",
526+
want: map[string]string{},
527+
},
528+
{
529+
name: "invalid key",
530+
value: "%XX=missing,userId=alice",
531+
want: map[string]string{
532+
"%XX": "missing",
533+
"userId": "alice",
534+
},
535+
},
536+
{
537+
name: "invalid value",
538+
value: "missing=%XX,userId=alice",
539+
want: map[string]string{
540+
"userId": "alice",
541+
},
542+
},
543+
}
544+
545+
for _, tt := range tests {
546+
t.Run(tt.name, func(t *testing.T) {
547+
keyValues, _ := convHeaders(tt.value)
548+
assert.Equal(t, tt.want, keyValues)
549+
})
550+
}
551+
}

0 commit comments

Comments
 (0)