fix: address review feedback for OpenTelemetry config

- Add TOML variable expansion for tracing fields (endpoint, traceId,
  spanId, headers) via expandTracingVariables(), matching stdin JSON
  path behavior. Comments now accurately reflect expansion support.
- Reject all-zero traceId/spanId per W3C Trace Context specification.
- Make TestInitProvider_WithHeaders deterministic using a channel
  instead of conditional assertion on captured auth header.
- Remove unused varExprPatternSimple regex.
- Add tests: variable expansion, undefined var error, all-zero
  traceId/spanId rejection, unexpanded var expression rejection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: lpcox

internal/config/config_core.go

@@ -360,6 +360,10 @@ func LoadFromFile(path string) (*Config, error) {
 	if cfg.Gateway.Opentelemetry != nil {
 		cfg.Gateway.Tracing = cfg.Gateway.Opentelemetry
 		cfg.Gateway.Opentelemetry = nil
+		// Expand ${VAR} expressions in tracing fields before validation.
+		if err := expandTracingVariables(cfg.Gateway.Tracing); err != nil {
+			return nil, err
+		}
 		// Validate HTTPS endpoint requirement for the opentelemetry section
 		if err := validateOpenTelemetryConfig(cfg.Gateway.Tracing, true); err != nil {
 			return nil, err

internal/config/config_tracing.go

@@ -32,17 +32,19 @@ type TracingConfig struct {
 	Endpoint string `toml:"endpoint" json:"endpoint,omitempty"`
 
 	// Headers are HTTP headers sent with every OTLP export request (e.g. auth tokens).
-	// Values support ${VAR} variable expansion.
+	// Header values support ${VAR} variable expansion (expanded at config load time).
 	Headers map[string]string `toml:"headers" json:"headers,omitempty"`
 
 	// TraceID is an optional W3C trace ID (32-char lowercase hex) used to construct the
 	// parent traceparent header, linking gateway spans into a pre-existing trace.
-	// Supports ${VAR} variable expansion. Must be 32 lowercase hex characters.
+	// Supports ${VAR} variable expansion (expanded at config load time).
+	// Must be 32 lowercase hex characters and must not be all zeros.
 	TraceID string `toml:"trace_id" json:"traceId,omitempty"`
 
 	// SpanID is an optional W3C span ID (16-char lowercase hex) paired with TraceID
 	// to construct the parent traceparent header. Ignored when TraceID is absent.
-	// Supports ${VAR} variable expansion. Must be 16 lowercase hex characters.
+	// Supports ${VAR} variable expansion (expanded at config load time).
+	// Must be 16 lowercase hex characters and must not be all zeros.
 	SpanID string `toml:"span_id" json:"spanId,omitempty"`
 
 	// ServiceName is the service name reported in traces.

internal/config/config_tracing_test.go

@@ -180,17 +180,81 @@ func TestOTEL010_ServiceNameDefaults(t *testing.T) {
 		"T-OTEL-010: default serviceName must be 'mcp-gateway' after applyDefaults")
 }
 
-// TestValidateOpenTelemetryConfig_VarExpressions verifies that ${VAR} expressions
-// are accepted in traceId/spanId fields (they pass the regex check as whole-string var refs).
-func TestValidateOpenTelemetryConfig_VarExpressions(t *testing.T) {
+// TestValidateOpenTelemetryConfig_UnexpandedVarExpressions verifies that unexpanded
+// ${VAR} expressions are rejected by validation. In practice, expandTracingVariables
+// (TOML path) or ExpandRawJSONVariables (stdin JSON path) expand vars before validation,
+// so unexpanded expressions should never reach the validator in normal flow.
+func TestValidateOpenTelemetryConfig_UnexpandedVarExpressions(t *testing.T) {
 	cfg := &TracingConfig{
 		Endpoint: "https://otel-collector.example.com",
 		TraceID:  "${TRACE_ID}",
-		SpanID:   "${SPAN_ID}",
 	}
-	// Variable expressions should not be validated as hex (they haven't been expanded yet)
 	err := validateOpenTelemetryConfig(cfg, true)
-	require.NoError(t, err, "Variable expressions in traceId/spanId must pass validation before expansion")
+	require.Error(t, err, "Unexpanded variable expressions must fail hex validation")
+	assert.Contains(t, err.Error(), "traceId")
+}
+
+// TestExpandTracingVariables verifies that ${VAR} expressions in tracing config
+// fields are expanded from environment variables.
+func TestExpandTracingVariables(t *testing.T) {
+	t.Setenv("TEST_OTEL_ENDPOINT", "https://otel.example.com")
+	t.Setenv("TEST_TRACE_ID", "4bf92f3577b34da6a3ce929d0e0e4736")
+	t.Setenv("TEST_SPAN_ID", "00f067aa0ba902b7")
+	t.Setenv("TEST_AUTH_TOKEN", "Bearer secret-token")
+
+	cfg := &TracingConfig{
+		Endpoint: "${TEST_OTEL_ENDPOINT}",
+		TraceID:  "${TEST_TRACE_ID}",
+		SpanID:   "${TEST_SPAN_ID}",
+		Headers:  map[string]string{"Authorization": "${TEST_AUTH_TOKEN}"},
+	}
+
+	err := expandTracingVariables(cfg)
+	require.NoError(t, err)
+
+	assert.Equal(t, "https://otel.example.com", cfg.Endpoint)
+	assert.Equal(t, "4bf92f3577b34da6a3ce929d0e0e4736", cfg.TraceID)
+	assert.Equal(t, "00f067aa0ba902b7", cfg.SpanID)
+	assert.Equal(t, "Bearer secret-token", cfg.Headers["Authorization"])
+
+	// After expansion, validation should pass
+	err = validateOpenTelemetryConfig(cfg, true)
+	require.NoError(t, err, "Expanded config should pass validation")
+}
+
+// TestExpandTracingVariables_UndefinedVar verifies that an undefined variable
+// in tracing config causes an error during expansion.
+func TestExpandTracingVariables_UndefinedVar(t *testing.T) {
+	cfg := &TracingConfig{
+		Endpoint: "${UNDEFINED_OTEL_ENDPOINT_XYZZY}",
+	}
+	err := expandTracingVariables(cfg)
+	require.Error(t, err, "Undefined variable must cause expansion error")
+}
+
+// TestValidateOpenTelemetryConfig_AllZeroTraceID verifies that an all-zero traceId
+// is rejected per W3C Trace Context specification.
+func TestValidateOpenTelemetryConfig_AllZeroTraceID(t *testing.T) {
+	cfg := &TracingConfig{
+		Endpoint: "https://otel-collector.example.com",
+		TraceID:  "00000000000000000000000000000000",
+	}
+	err := validateOpenTelemetryConfig(cfg, true)
+	require.Error(t, err, "All-zero traceId must be rejected per W3C Trace Context")
+	assert.Contains(t, err.Error(), "all zeros")
+}
+
+// TestValidateOpenTelemetryConfig_AllZeroSpanID verifies that an all-zero spanId
+// is rejected per W3C Trace Context specification.
+func TestValidateOpenTelemetryConfig_AllZeroSpanID(t *testing.T) {
+	cfg := &TracingConfig{
+		Endpoint: "https://otel-collector.example.com",
+		TraceID:  "4bf92f3577b34da6a3ce929d0e0e4736",
+		SpanID:   "0000000000000000",
+	}
+	err := validateOpenTelemetryConfig(cfg, true)
+	require.Error(t, err, "All-zero spanId must be rejected per W3C Trace Context")
+	assert.Contains(t, err.Error(), "all zeros")
 }
 
 // TestGetSampleRate_NewFields verifies that the new fields don't affect GetSampleRate.

internal/config/validation.go

@@ -22,8 +22,9 @@ var varExprPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`)
 var (
 	traceIDPattern = regexp.MustCompile(`^[0-9a-f]{32}$`)
 	spanIDPattern  = regexp.MustCompile(`^[0-9a-f]{16}$`)
-	// varExprPatternSimple matches a whole-string variable expression like ${VAR}
-	varExprPatternSimple = regexp.MustCompile(`^\$\{[A-Za-z_][A-Za-z0-9_]*\}$`)
+	// W3C Trace Context forbids all-zero trace/span IDs.
+	allZeroTraceID = regexp.MustCompile(`^0{32}$`)
+	allZeroSpanID  = regexp.MustCompile(`^0{16}$`)
 )
 
 var logValidation = logger.New("config:validation")
@@ -116,6 +117,49 @@ func expandEnvVariables(env map[string]string, serverName string) (map[string]st
 	return result, nil
 }
 
+// expandTracingVariables expands ${VAR} expressions in TracingConfig fields.
+// This is called for TOML-loaded configs before validation, mirroring the
+// stdin JSON path where ExpandRawJSONVariables handles expansion.
+func expandTracingVariables(cfg *TracingConfig) error {
+	if cfg == nil {
+		return nil
+	}
+
+	if cfg.Endpoint != "" {
+		expanded, err := expandVariables(cfg.Endpoint, "gateway.opentelemetry.endpoint")
+		if err != nil {
+			return err
+		}
+		cfg.Endpoint = expanded
+	}
+
+	if cfg.TraceID != "" {
+		expanded, err := expandVariables(cfg.TraceID, "gateway.opentelemetry.traceId")
+		if err != nil {
+			return err
+		}
+		cfg.TraceID = expanded
+	}
+
+	if cfg.SpanID != "" {
+		expanded, err := expandVariables(cfg.SpanID, "gateway.opentelemetry.spanId")
+		if err != nil {
+			return err
+		}
+		cfg.SpanID = expanded
+	}
+
+	for key, value := range cfg.Headers {
+		expanded, err := expandVariables(value, fmt.Sprintf("gateway.opentelemetry.headers.%s", key))
+		if err != nil {
+			return err
+		}
+		cfg.Headers[key] = expanded
+	}
+
+	return nil
+}
+
 // validateMounts validates mount specifications using centralized rules
 func validateMounts(mounts []string, jsonPath string) error {
 	for i, mount := range mounts {
@@ -514,25 +558,39 @@ func validateOpenTelemetryConfig(cfg *TracingConfig, enforceHTTPS bool) error {
 		}
 	}
 
-	// Validate traceId: must be a 32-char lowercase hex string
+	// Validate traceId: must be a 32-char lowercase hex string, not all-zero
 	if cfg.TraceID != "" {
 		if !traceIDPattern.MatchString(cfg.TraceID) {
 			logValidation.Printf("Invalid traceId format: %s", cfg.TraceID)
 			return rules.InvalidValue("traceId",
 				fmt.Sprintf("traceId must be a 32-character lowercase hexadecimal string, got '%s'", cfg.TraceID),
 				"gateway.opentelemetry.traceId",
-				"Provide a valid W3C trace ID (32 lowercase hex chars, e.g., \"4bf92f3577b34da6a3ce929d0e0e4736\"); environment-variable expressions are not supported here")
+				"Provide a valid W3C trace ID (32 lowercase hex chars, e.g., \"4bf92f3577b34da6a3ce929d0e0e4736\")")
+		}
+		if allZeroTraceID.MatchString(cfg.TraceID) {
+			logValidation.Printf("All-zero traceId rejected per W3C Trace Context: %s", cfg.TraceID)
+			return rules.InvalidValue("traceId",
+				"traceId must not be all zeros (W3C Trace Context forbids an all-zero trace-id)",
+				"gateway.opentelemetry.traceId",
+				"Provide a non-zero W3C trace ID (e.g., \"4bf92f3577b34da6a3ce929d0e0e4736\")")
 		}
 	}
 
-	// Validate spanId: must be a 16-char lowercase hex string
+	// Validate spanId: must be a 16-char lowercase hex string, not all-zero
 	if cfg.SpanID != "" {
 		if !spanIDPattern.MatchString(cfg.SpanID) {
 			logValidation.Printf("Invalid spanId format: %s", cfg.SpanID)
 			return rules.InvalidValue("spanId",
 				fmt.Sprintf("spanId must be a 16-character lowercase hexadecimal string, got '%s'", cfg.SpanID),
 				"gateway.opentelemetry.spanId",
-				"Provide a valid W3C span ID (16 lowercase hex chars, e.g., \"00f067aa0ba902b7\"); environment-variable expressions are not supported here")
+				"Provide a valid W3C span ID (16 lowercase hex chars, e.g., \"00f067aa0ba902b7\")")
+		}
+		if allZeroSpanID.MatchString(cfg.SpanID) {
+			logValidation.Printf("All-zero spanId rejected per W3C Trace Context: %s", cfg.SpanID)
+			return rules.InvalidValue("spanId",
+				"spanId must not be all zeros (W3C Trace Context forbids an all-zero span-id)",
+				"gateway.opentelemetry.spanId",
+				"Provide a non-zero W3C span ID (e.g., \"00f067aa0ba902b7\")")
 		}
 	}
 

internal/tracing/provider_test.go

@@ -313,16 +313,19 @@ func TestWrapHTTPHandler_GeneratesRootSpan(t *testing.T) {
 	assert.False(t, capturedSpanCtx.IsRemote(), "span should not be marked remote — it is a local root span")
 }
 
-// TestInitProvider_WithHeaders verifies that OTLP export headers are accepted.
-// The headers are applied to the exporter but cannot easily be verified without
-// intercepting the network; this test confirms provider initialization succeeds.
+// TestInitProvider_WithHeaders verifies that OTLP export headers are forwarded
+// to the collector. A channel synchronises with the test HTTP server so the
+// assertion is deterministic rather than timing-dependent.
 func TestInitProvider_WithHeaders(t *testing.T) {
 	ctx := context.Background()
 
-	// Spin up a test server to receive OTLP export requests and capture headers.
-	var capturedAuth string
+	// Channel signals when the test server receives an export request.
+	received := make(chan string, 1)
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		capturedAuth = r.Header.Get("Authorization")
+		select {
+		case received <- r.Header.Get("Authorization"):
+		default:
+		}
 		w.WriteHeader(http.StatusOK)
 	}))
 	defer ts.Close()
@@ -341,16 +344,18 @@ func TestInitProvider_WithHeaders(t *testing.T) {
 	_, span := tr.Start(ctx, "header-test-span")
 	span.End()
 
-	// Shutdown with a brief timeout to flush pending spans.
-	shutdownCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond)
+	// Shutdown flushes the batch processor, ensuring the export is sent.
+	shutdownCtx, cancel := context.WithTimeout(ctx, 2*time.Second)
 	defer cancel()
 	_ = provider.Shutdown(shutdownCtx)
 
-	// The test server may or may not have received the export within the timeout.
-	// If it did, verify the Authorization header was forwarded correctly.
-	if capturedAuth != "" {
-		assert.Equal(t, "Bearer test-token", capturedAuth,
+	// Wait for the export request with a timeout.
+	select {
+	case auth := <-received:
+		assert.Equal(t, "Bearer test-token", auth,
 			"Authorization header must be forwarded to the OTLP collector")
+	case <-time.After(3 * time.Second):
+		t.Fatal("timed out waiting for OTLP export request — headers test is non-deterministic")
 	}
 }