diff --git a/CHANGELOG.md b/CHANGELOG.md index 11c86011494..6844ecadbfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed +- Use direct normalized-key lookups in `Carrier.Get` and `Carrier.Keys` in `go.opentelemetry.io/contrib/propagators/envcar`. (#9112) - Update log bridge conversions to use attribute key-values instead of the removed log key-values in `go.opentelemetry.io/contrib/bridges/otellogr`, `go.opentelemetry.io/contrib/bridges/otellogrus`, `go.opentelemetry.io/contrib/bridges/otelslog`, and `go.opentelemetry.io/contrib/bridges/otelzap`. (#9180) - The `Version()` function in `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux` has been replaced by `const Version`. (#9076) - Set `error.type` attribute instead of adding `exception` span events in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#8977) diff --git a/propagators/envcar/carrier.go b/propagators/envcar/carrier.go index 9e6206b3f43..9f3a0631978 100644 --- a/propagators/envcar/carrier.go +++ b/propagators/envcar/carrier.go @@ -6,61 +6,52 @@ package envcar // import "go.opentelemetry.io/contrib/propagators/envcar" import ( "os" "strings" - "sync" "go.opentelemetry.io/otel/propagation" ) -// Carrier is a TextMapCarrier that uses the environment variables as a -// storage medium for propagated key-value pairs. The keys are normalized -// before being used to access the environment variables. -// This is useful for propagating values that are set in the environment -// and need to be accessed by different processes or services. -// The keys are uppercased to avoid case sensitivity issues across different -// operating systems and environments. +// Carrier is a [propagation.TextMapCarrier] for environment variables. // -// If you do not set SetEnvFunc, [Carrier.Set] will do nothing. -// Using [os.Setenv] here is discouraged as the environment should -// be immutable: -// https://opentelemetry.io/docs/specs/otel/context/env-carriers/#environment-variable-immutability +// [Carrier.Get] and [Carrier.Keys] read from the current process environment. +// [Carrier.Get] normalizes the key before lookup. [Carrier.Keys] only lists +// environment variable names that are already normalized. +// +// [Carrier.Set] writes through [Carrier.SetEnvFunc] with a normalized key. If +// SetEnvFunc is nil, [Carrier.Set] does nothing. This lets injection target the +// environment of a child process without mutating the current process +// environment. Using [os.Setenv] as SetEnvFunc is discouraged because +// applications should not modify their own context-related environment +// variables. +// +// Key name normalization is defined by the [OpenTelemetry specification]. +// +// [OpenTelemetry specification]: https://opentelemetry.io/docs/specs/otel/context/env-carriers/#key-name-normalization type Carrier struct { - // SetEnvFunc is the function that sets the environment variable. - // Usually, you want to set the environment variables for processes - // that are spawned by the current process. + // SetEnvFunc sets an environment variable for injected context. + // [Carrier.Set] calls SetEnvFunc with a normalized key. + // + // Set this to update the environment that will be passed to a child + // process. Leave it nil for an extract-only carrier. SetEnvFunc func(key, value string) - values map[string]string - once sync.Once } // Compile time check that Carrier implements the TextMapCarrier. var _ propagation.TextMapCarrier = (*Carrier)(nil) -// fetch runs once on first access, and stores the environment in the -// carrier. -func (c *Carrier) fetch() { - c.once.Do(func() { - environ := os.Environ() - c.values = make(map[string]string, len(environ)) - for _, kv := range environ { - kvPair := strings.SplitN(kv, "=", 2) - key := normalize(kvPair[0]) - c.values[key] = kvPair[1] - } - }) -} - -// Get returns the value associated with the normalized passed key. -// The first call to [Carrier.Get] or [Carrier.Keys] for a -// given Carrier will read and store the values from the -// environment and all future reads will be from that store. -func (c *Carrier) Get(key string) string { - c.fetch() - return c.values[normalize(key)] +// Get normalizes key and returns the corresponding value from the current +// process environment. It returns an empty string if the normalized +// environment variable is unset or set to an empty value. +// +// On platforms with case-insensitive environment lookup, such as Windows, the +// lookup may match an environment variable whose name differs from the +// normalized key only by case. +func (*Carrier) Get(key string) string { + return os.Getenv(normalize(key)) } -// Set stores the key-value pair in the environment variable. -// The key is normalized before being used to set the -// environment variable. +// Set stores the key-value pair by calling [Carrier.SetEnvFunc]. +// The key is normalized before SetEnvFunc is called. +// // If SetEnvFunc is not set, this method does nothing. func (c *Carrier) Set(key, value string) { if c.SetEnvFunc == nil { @@ -70,16 +61,15 @@ func (c *Carrier) Set(key, value string) { c.SetEnvFunc(k, value) } -// Keys lists the keys stored in this carrier. -// This returns all the keys in the environment variables. -// The first call to [Carrier.Get] or [Carrier.Keys] for a -// given Carrier will read and store the values from the -// environment and all future reads will be from that store. -// The keys are returned in their normalized form. -func (c *Carrier) Keys() []string { - c.fetch() - keys := make([]string, 0, len(c.values)) - for key := range c.values { +// Keys returns normalized environment variable names from the current process. +func (*Carrier) Keys() []string { + environ := os.Environ() + keys := make([]string, 0, len(environ)) + for _, kv := range environ { + key, _, _ := strings.Cut(kv, "=") + if !normalized(key) { + continue + } keys = append(keys, key) } return keys diff --git a/propagators/envcar/carrier_example_test.go b/propagators/envcar/carrier_example_test.go index 799786faa92..14210f97446 100644 --- a/propagators/envcar/carrier_example_test.go +++ b/propagators/envcar/carrier_example_test.go @@ -15,11 +15,20 @@ import ( "go.opentelemetry.io/contrib/propagators/envcar" ) -// This example is a go program where the environment variables are carrying the -// trace information, and we're going to pick them up into our context. +// An example where the environment variables are carrying the trace +// information, and we're going to pick them up into our context. func ExampleCarrier_extractFromParent() { - // Simulate environment variables set by a parent process. - // In practice, these would already be set when this process starts. + // Simulate an environment variable set by a parent process. In practice, + // this would already be set when this process starts, and the application + // would extract context during initialization. + orig, ok := os.LookupEnv("TRACEPARENT") + defer func() { + if ok { + _ = os.Setenv("TRACEPARENT", orig) + return + } + _ = os.Unsetenv("TRACEPARENT") + }() _ = os.Setenv("TRACEPARENT", "00-0102030405060708090a0b0c0d0e0f10-0102030405060708-01") // Create a carrier to read trace context from environment variables. @@ -29,7 +38,8 @@ func ExampleCarrier_extractFromParent() { prop := propagation.TraceContext{} ctx := prop.Extract(context.Background(), &carrier) - // The context now contains the span context from the parent. + // The context now contains the span context from the parent. Keep and pass + // this context instead of repeatedly extracting from the environment. spanCtx := trace.SpanContextFromContext(ctx) fmt.Printf("Trace ID: %s\n", spanCtx.TraceID()) fmt.Printf("Span ID: %s\n", spanCtx.SpanID()) @@ -40,8 +50,8 @@ func ExampleCarrier_extractFromParent() { // Sampled: true } -// This example is a go program where we have a trace and we'd like to inject it -// into a command we're going to run. +// An example where we have a trace and we'd like to inject it into a command +// we're going to run. func ExampleCarrier_childProcess() { // Create a span context with a known trace ID. traceID := trace.TraceID{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10} @@ -54,6 +64,7 @@ func ExampleCarrier_childProcess() { ctx := trace.ContextWithSpanContext(context.Background(), spanCtx) // Prepare a command that prints the TRACEPARENT environment variable. + // Each child process gets its own copy of the environment. cmd := exec.CommandContext(context.Background(), "printenv", "TRACEPARENT") cmd.Env = os.Environ() @@ -65,7 +76,7 @@ func ExampleCarrier_childProcess() { }, } - // Inject trace context into the child's environment. + // Inject trace context into the child's environment copy. prop := propagation.TraceContext{} prop.Inject(ctx, &carrier) diff --git a/propagators/envcar/carrier_test.go b/propagators/envcar/carrier_test.go index c37e2a7519f..938314dd95a 100644 --- a/propagators/envcar/carrier_test.go +++ b/propagators/envcar/carrier_test.go @@ -6,7 +6,7 @@ package envcar_test import ( "os" "os/exec" - "slices" + "runtime" "strings" "sync" "testing" @@ -61,19 +61,6 @@ func TestExtractValidTraceContextEnvCarrier(t *testing.T) { Remote: true, }), }, - { - name: "lowercase env names", - envs: map[string]string{ - "traceparent": "00-000000000000007b00000000000001c8-000000000000007b-00", - "tracestate": stateStr, - }, - want: trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: traceID, - SpanID: spanID, - TraceState: state, - Remote: true, - }), - }, } for _, tc := range tests { @@ -88,6 +75,21 @@ func TestExtractValidTraceContextEnvCarrier(t *testing.T) { } } +func TestExtractIgnoresNonNormalizedEnvNames(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows environment variables are case-insensitive, so this test is not applicable.") + } + + unsetenv(t, "TRACEPARENT") + unsetenv(t, "TRACESTATE") + t.Setenv("traceparent", "00-000000000000007b00000000000001c8-000000000000007b-00") + t.Setenv("tracestate", "key1=value1,key2=value2") + + ctx := prop.Extract(t.Context(), &envcar.Carrier{}) + + assert.False(t, trace.SpanContextFromContext(ctx).IsValid()) +} + func TestInjectTraceContextEnvCarrier(t *testing.T) { stateStr := "key1=value1,key2=value2" state, err := trace.ParseTraceState(stateStr) @@ -147,13 +149,16 @@ func TestInjectTraceContextEnvCarrier(t *testing.T) { } func TestCarrierKeys(t *testing.T) { - t.Setenv("traceparent", "value") + t.Setenv("TRACEPARENT", "value") + unsetenv(t, "ENVCAR_NON_NORMALIZED_KEY") + t.Setenv("envcar-non-normalized-key", "ignored") c := envcar.Carrier{} keys := c.Keys() assert.Contains(t, keys, "TRACEPARENT") - assert.NotContains(t, keys, "traceparent") + assert.NotContains(t, keys, "envcar-non-normalized-key") + assert.NotContains(t, keys, "ENVCAR_NON_NORMALIZED_KEY") } func TestCarrierSetNilFunc(_ *testing.T) { @@ -162,13 +167,22 @@ func TestCarrierSetNilFunc(_ *testing.T) { } func TestCarrierGetNormalizesKey(t *testing.T) { - t.Setenv("traceparent", "myvalue") + t.Setenv("TRACEPARENT", "myvalue") c := envcar.Carrier{} assert.Equal(t, "myvalue", c.Get("traceparent")) assert.Equal(t, "myvalue", c.Get("TRACEPARENT")) } +func TestCarrierGetIgnoresNonNormalizedEnvNames(t *testing.T) { + unsetenv(t, "ENVCAR_GET_NON_NORMALIZED_KEY") + t.Setenv("envcar-get-non-normalized-key", "ignored") + + c := envcar.Carrier{} + assert.Empty(t, c.Get("envcar_get_non_normalized_key")) + assert.Empty(t, c.Get("ENVCAR_GET_NON_NORMALIZED_KEY")) +} + func TestCarrierSetUppercasesUnderscoresKey(t *testing.T) { var gotKey string var gotValue string @@ -192,6 +206,29 @@ func TestCarrierSetUppercasesUnderscoresKey(t *testing.T) { assert.Equal(t, "🧳", gotValue) } +func TestCarrierGetReadsDirectly(t *testing.T) { + t.Setenv("TRACEPARENT", "myvalue") + c := envcar.Carrier{} + require.Equal(t, "myvalue", c.Get("TRACEPARENT")) + + t.Setenv("TRACEPARENT", "bad") + t.Setenv("ENVCAR_GET_READS_DIRECTLY_NEW", "value") + + assert.Equal(t, "bad", c.Get("TRACEPARENT")) + assert.Contains(t, c.Keys(), "ENVCAR_GET_READS_DIRECTLY_NEW") +} + +func TestCarrierKeysReadsDirectly(t *testing.T) { + t.Setenv("TRACEPARENT", "myvalue") + c := envcar.Carrier{} + require.Contains(t, c.Keys(), "TRACEPARENT") + + t.Setenv("ENVCAR_KEYS_FETCH_ONCE_NEW", "value") + + assert.Contains(t, c.Keys(), "TRACEPARENT") + assert.Contains(t, c.Keys(), "ENVCAR_KEYS_FETCH_ONCE_NEW") +} + func TestConcurrentChildProcesses(t *testing.T) { // Test that concurrent goroutines can each spawn child processes // with their own unique trace context. @@ -261,16 +298,16 @@ func TestConcurrentChildProcesses(t *testing.T) { } } -// Ensure Get and Keys to fetch from cache. -func TestCarrierGetFetchOnce(t *testing.T) { - t.Setenv("TRACEPARENT", "myvalue") - c := envcar.Carrier{} - require.Equal(t, "myvalue", c.Get("TRACEPARENT")) - require.NotEqual(t, -1, slices.Index(c.Keys(), "TRACEPARENT")) - t.Setenv("TRACEPARENT", "bad") - t.Setenv("SOMETHINGNEW", "bad") - // Assert a carrier instance reads the env vars only once: - assert.Equal(t, "myvalue", c.Get("TRACEPARENT")) - assert.NotEqual(t, -1, slices.Index(c.Keys(), "TRACEPARENT")) - assert.Equal(t, -1, slices.Index(c.Keys(), "SOMETHINGNEW")) +func unsetenv(t *testing.T, key string) { + t.Helper() + + value, ok := os.LookupEnv(key) + require.NoError(t, os.Unsetenv(key)) + t.Cleanup(func() { + if ok { + require.NoError(t, os.Setenv(key, value)) //nolint:usetesting // t.Setenv cannot restore after an explicit unset. + return + } + require.NoError(t, os.Unsetenv(key)) + }) } diff --git a/propagators/envcar/doc.go b/propagators/envcar/doc.go index 73d0c22f42c..7c4ab2bf3f0 100644 --- a/propagators/envcar/doc.go +++ b/propagators/envcar/doc.go @@ -1,6 +1,22 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -// Package envcar implements the Environment Carrier specification as documented -// here https://opentelemetry.io/docs/specs/otel/context/env-carriers/ +// Package envcar implements the +// [Environment Variables as Context Propagation Carriers specification]. +// +// Environment variable propagation is intended for process boundaries where +// network protocols are not available, such as batch jobs, CI/CD, and command +// line tools. Treat context-related environment variables as process-startup +// input. +// +// Applications should extract context from the environment once during startup +// and then use the extracted context. Repeated extraction uses the process +// environment as a live parent-context source and may observe later environment +// variable changes. +// +// Note that environment variables can be visible to code in the same process +// and, on many systems, to other users or processes with sufficient +// permissions. Do not use this carrier for sensitive context. +// +// [Environment Variables as Context Propagation Carriers specification]: https://opentelemetry.io/docs/specs/otel/context/env-carriers/ package envcar // import "go.opentelemetry.io/contrib/propagators/envcar" diff --git a/propagators/envcar/normalize.go b/propagators/envcar/normalize.go index cdc90445b6e..c3d2aeeaa4a 100644 --- a/propagators/envcar/normalize.go +++ b/propagators/envcar/normalize.go @@ -9,13 +9,14 @@ import ( // normalize converts s to a valid POSIX environment variable name. // The conversion rules are: +// - Empty input is converted to _. // - A–Z, 0–9, and _ are kept as-is. // - a–z are uppercased. // - All other characters are replaced with _. // - If the result would start with a digit, an underscore is prepended. func normalize(s string) string { if s == "" { - return "" + return "_" } // Pre-allocate the exact output length. If the first byte is a digit, @@ -46,3 +47,26 @@ func normalize(s string) string { } return string(b) } + +// normalized reports whether s is already a non-empty normalized environment +// variable name. +func normalized(s string) bool { + if s == "" { + return false + } + // Normalized names cannot start with a digit; normalize would prepend '_'. + if s[0] >= '0' && s[0] <= '9' { + return false + } + + for _, r := range s { + switch { + // A-Z, 0-9, and _ are already normalized. + case r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '_': + default: + // Lowercase letters and all other characters would be rewritten. + return false + } + } + return true +} diff --git a/propagators/envcar/normalize_test.go b/propagators/envcar/normalize_test.go index 20965c67944..0b1f031763a 100644 --- a/propagators/envcar/normalize_test.go +++ b/propagators/envcar/normalize_test.go @@ -12,7 +12,7 @@ import ( var normalizeCases = []struct { in, want string }{ - {"", ""}, + {"", "_"}, {"ABC", "ABC"}, {"abc", "ABC"}, {"01239", "_01239"}, @@ -37,6 +37,14 @@ func TestNormalize(t *testing.T) { } } +func TestNormalized(t *testing.T) { + for _, tc := range normalizeCases { + t.Run(tc.in, func(t *testing.T) { + assert.Equal(t, tc.in == tc.want, normalized(tc.in)) + }) + } +} + func BenchmarkNormalize(b *testing.B) { for _, tc := range normalizeCases { b.Run(tc.in, func(b *testing.B) { @@ -47,3 +55,14 @@ func BenchmarkNormalize(b *testing.B) { }) } } + +func BenchmarkNormalized(b *testing.B) { + for _, tc := range normalizeCases { + b.Run(tc.in, func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + normalized(tc.in) + } + }) + } +}