diff --git a/audit/pkg/_CROSS-CUTTING.audit.md b/audit/pkg/_CROSS-CUTTING.audit.md index c7ba220..040bfcf 100644 --- a/audit/pkg/_CROSS-CUTTING.audit.md +++ b/audit/pkg/_CROSS-CUTTING.audit.md @@ -68,7 +68,7 @@ every one of them on the first run: | `pkg/config` | `*viper.Viper` has no internal lock; `configInstance` singleton | `config.audit.md` findings 1, 2 | | `pkg/logger` | `Logger`, `errorTracker` globals | `logger.audit.md` finding 1 | | `pkg/modelregistry` | `defaultRegistry` read by 6 functions without the lock | `modelregistry.audit.md` findings 2, 8 *(fixed 2026-09-30)* | -| `pkg/tracing` | `tracer` global | `tracing.audit.md` finding 5 | +| `pkg/tracing` | `tracer` global | `tracing.audit.md` finding 5 *(fixed 2026-09-30)* | | `pkg/errortracking` | `sentry.Init` mutates process globals | `errortracking.audit.md` finding 2 | **Failure scenario.** `pkg/config` finding 1 is the sharpest illustration. A @@ -162,7 +162,7 @@ The test bodies that exist but are never executed by CI: | `logger` | 0 | 0 | — | | `modelregistry` | 1 | ~150 | yes (`-race`) *(added 2026-09-30)* | | `testmodels` | 0 | 0 | — | -| `tracing` | 0 | 0 | — | +| `tracing` | 1 | ~90 | yes *(added 2026-09-30)* | **Failure scenario.** `pkg/security` has 6 359 lines of tests — the largest test body in the repository — and **not one of them runs in CI**. A change that breaks @@ -307,7 +307,7 @@ package-level variables, and most guard it with nothing: | `pkg/logger` | `Logger *zap.SugaredLogger` (`logger.go:15`), `errorTracker` (`:16`) | **no** — and `Logger` is exported | | `pkg/cache` | `defaultCache *Cache` (`cache.go:10`) | **no** | | `pkg/config` | `configInstance *Manager` (`manager.go:15`) | **no** | -| `pkg/tracing` | `tracer` (`tracing.go:19`) | **no** | +| `pkg/tracing` | `tracer` | **yes** *(fixed 2026-09-30)* — `atomic.Pointer` | | `pkg/modelregistry` | `defaultRegistry` | **yes** *(fixed 2026-09-30)* — guarded by `registriesMutex`; all access via `GetDefaultRegistry()` | | `pkg/metrics` | `globalProvider` (`interfaces.go:50-51`) | **yes** — `globalProviderMu sync.RWMutex` | @@ -369,7 +369,7 @@ to configure otherwise: | Component | Default | Configurable? | Reference | |---|---|---|---| | PostgreSQL | `sslmode: disable` (`config/manager.go:242`) | yes, via config | `config.audit.md` finding 3 | -| OTLP traces | `otlptracegrpc.WithInsecure()` hardcoded (`tracing/tracing.go:41`) | **no** — `Config` has no field for it | `tracing.audit.md` finding 1 | +| OTLP traces | `otlptracegrpc.WithInsecure()` hardcoded (`tracing/tracing.go:41`) *(fixed 2026-09-30: TLS default, `Insecure` opt-in)* | **yes** | `tracing.audit.md` finding 1 | | Redis (cache) | no `TLSConfig` set | **no** — `RedisConfig` has no TLS field | `cache.audit.md` finding 15 | | Memcache | no TLS | **no** | `cache.audit.md` finding 15 | | CORS | `allowed_origins: ["*"]`, `allowed_headers: ["*"]` (`config/manager.go:214-216`) | yes | `config.audit.md` finding 3 | diff --git a/audit/pkg/tracing.audit.md b/audit/pkg/tracing.audit.md index 58cf181..788d7fc 100644 --- a/audit/pkg/tracing.audit.md +++ b/audit/pkg/tracing.audit.md @@ -36,6 +36,27 @@ by configuration alone. | 9 | Low | Maintenance | `semconv/v1.4.0` (2021) — deprecated attribute names modern collectors no longer index | | 10 | Low | Security | `SetAttributes`/`AddEvent` pass caller data through with no size or cardinality limit | +## Resolution (2026-09-30) + +Fixed in `pkg/tracing/tracing.go`, `pkg/config` (`TracingConfig`, defaults), the package README, and new +`pkg/tracing/tracing_test.go` (passes). + +| # | Status | What changed | +|---|--------|--------------| +| 1 | **Fixed** | TLS is the default. `Config` gains `Insecure`, `TLSConfig` and `Headers` (OTLP auth). `tracing.insecure` added to `pkg/config`. **Breaking:** plaintext collectors now need `Insecure: true`. | +| 2 | **Fixed** | Query string and `Host` are no longer exported; attributes are method, `url.path`, scheme, `http.route`, status. `TLSConfig` has no config-file key (code only). | +| 3 | **Fixed** | `ParentBased(TraceIDRatioBased(rate))`; `SampleRate` defaults to 0.1, validated to [0,1]; `tracing.sample_rate` added to `pkg/config`. | +| 4 | **Fixed** | Span name is `METHOD ` from `Request.Pattern`, `` otherwise. `MiddlewareWithRoute(fn)` supports other routers. | +| 5 | **Fixed** | `tracer` is an `atomic.Pointer`; a second `InitTracer` returns an error; the shutdown func resets state. | +| 6 | **Fixed** | Response writer wrapped; `http.response.status_code` recorded, 5xx sets Error status. Preserves `Flush`/`Unwrap`. | +| 7 | **Fixed** | Panics are recorded (`RecordError`, Error status) and re-raised so the panic middleware still responds. Must be installed inside the panic middleware; actual order in `pkg/server` not verified. | +| 8 | **Fixed** | `InitTracerContext(ctx, cfg)` with `InitTimeout` (default 10s); `InitTracer` retained as a wrapper. Exporter is shut down if resource creation fails. | +| 9 | **Fixed** | Moved to `semconv/v1.26.0`. | +| 10 | **Fixed** | `AttributeValueLengthLimit` set via `WithRawSpanLimits` (`AttributeValueLimit`, default 1024). | + +Tests added: query redaction and route naming, unmatched route, 5xx status, panic recorded and re-raised, +double-init and invalid sample rate. + --- ## Findings diff --git a/go.mod b/go.mod index cdbd76e..8c72e95 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,9 @@ require ( go.uber.org/zap v1.28.0 golang.org/x/crypto v0.55.0 golang.org/x/oauth2 v0.36.0 + golang.org/x/sys v0.47.0 golang.org/x/time v0.15.0 + google.golang.org/grpc v1.83.2 gorm.io/driver/postgres v1.6.0 gorm.io/driver/sqlite v1.6.0 gorm.io/driver/sqlserver v1.6.3 @@ -147,11 +149,9 @@ require ( golang.org/x/mod v0.38.0 // indirect golang.org/x/net v0.58.0 // indirect golang.org/x/sync v0.22.0 // indirect - golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.41.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect - google.golang.org/grpc v1.83.2 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect modernc.org/libc v1.72.3 // indirect diff --git a/pkg/config/config.go b/pkg/config/config.go index 884e269..da230c6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -91,6 +91,12 @@ type TracingConfig struct { ServiceName string `mapstructure:"service_name"` ServiceVersion string `mapstructure:"service_version"` Endpoint string `mapstructure:"endpoint"` + // Insecure exports traces over plaintext gRPC (default false: TLS). + Insecure bool `mapstructure:"insecure"` + // SampleRate is the fraction of root traces sampled; 0 selects the default (0.1). + SampleRate float64 `mapstructure:"sample_rate"` + // Headers are sent with every OTLP export request (e.g. auth tokens). + Headers map[string]string `mapstructure:"headers"` } // CacheConfig holds cache provider configuration diff --git a/pkg/config/manager.go b/pkg/config/manager.go index 20889a4..57614f6 100644 --- a/pkg/config/manager.go +++ b/pkg/config/manager.go @@ -250,6 +250,8 @@ func setDefaults(v *viper.Viper) { v.SetDefault("tracing.service_name", "resolvespec") v.SetDefault("tracing.service_version", "1.0.0") v.SetDefault("tracing.endpoint", "") + v.SetDefault("tracing.insecure", false) + v.SetDefault("tracing.sample_rate", 0.1) // Cache defaults v.SetDefault("cache.provider", "memory") diff --git a/pkg/tracing/README.md b/pkg/tracing/README.md index 080f0c3..e3ab9bb 100644 --- a/pkg/tracing/README.md +++ b/pkg/tracing/README.md @@ -33,6 +33,10 @@ type Config struct { ServiceVersion string // Version for tracking deployments Endpoint string // OTLP collector endpoint (e.g., "localhost:4317") Enabled bool // Enable/disable tracing + Insecure bool // Plaintext gRPC export (default: TLS) + TLSConfig *tls.Config // Optional TLS customisation + Headers map[string]string // OTLP auth headers + SampleRate float64 // Root-trace sampling fraction (default 0.1) } ``` @@ -429,8 +433,10 @@ if err != nil { For high-traffic services, configure sampling: ```go -// In production: sample 10% of traces -// Currently using AlwaysSample() - update in tracing.go if needed +// Default is ParentBased(TraceIDRatioBased(0.1)); set Config.SampleRate (0-1) to change. +// Query strings are never exported; span names use the matched route pattern +// (Request.Pattern, or MiddlewareWithRoute for other routers). Install the +// middleware inside the panic-recovery middleware. ``` ### 5. Context Propagation diff --git a/pkg/tracing/tracing.go b/pkg/tracing/tracing.go index 9102078..1086e20 100644 --- a/pkg/tracing/tracing.go +++ b/pkg/tracing/tracing.go @@ -2,21 +2,43 @@ package tracing import ( "context" + "crypto/tls" + "errors" "fmt" "net/http" + "sync" + "sync/atomic" + "time" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/exporters/otlp/otlptrace" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/sdk/resource" sdktrace "go.opentelemetry.io/otel/sdk/trace" - semconv "go.opentelemetry.io/otel/semconv/v1.4.0" + semconv "go.opentelemetry.io/otel/semconv/v1.26.0" "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc/credentials" ) -var tracer trace.Tracer +const ( + // DefaultSampleRate is used when Config.SampleRate is zero. + DefaultSampleRate = 0.1 + // DefaultAttributeValueLimit caps the length of any span attribute value. + DefaultAttributeValueLimit = 1024 + // DefaultInitTimeout bounds exporter and resource creation. + DefaultInitTimeout = 10 * time.Second + + unmatchedRoute = "" +) + +var ( + tracer atomic.Pointer[trace.Tracer] + initMu sync.Mutex + initialized bool +) // Config holds tracing configuration type Config struct { @@ -24,24 +46,77 @@ type Config struct { ServiceVersion string Endpoint string // OTLP endpoint (e.g., "localhost:4317") Enabled bool + + // Insecure exports traces over plaintext gRPC. TLS is used unless this is set. + Insecure bool + // TLSConfig customises TLS for the exporter. Nil uses the system roots. Ignored when Insecure. + TLSConfig *tls.Config + // Headers are sent with every export request (e.g. an authorization token). + Headers map[string]string + + // SampleRate is the fraction of new root traces sampled (0 < rate <= 1). + // Zero selects DefaultSampleRate. Upstream sampling decisions are respected. + SampleRate float64 + // AttributeValueLimit caps span attribute value length. Zero selects DefaultAttributeValueLimit. + AttributeValueLimit int + // InitTimeout bounds exporter/resource creation. Zero selects DefaultInitTimeout. + InitTimeout time.Duration } -// InitTracer initializes the OpenTelemetry tracer +// InitTracer initializes the OpenTelemetry tracer using a background context. func InitTracer(config Config) (func(context.Context) error, error) { + return InitTracerContext(context.Background(), config) +} + +// InitTracerContext initializes the OpenTelemetry tracer. ctx bounds startup work. +// Calling it again while a tracer is active returns an error; call the returned +// shutdown function first. +func InitTracerContext(ctx context.Context, config Config) (func(context.Context) error, error) { if !config.Enabled { // Return no-op shutdown function return func(context.Context) error { return nil }, nil } - ctx := context.Background() + initMu.Lock() + defer initMu.Unlock() + if initialized { + return nil, errors.New("tracing already initialized; shut down the previous tracer first") + } + + if config.SampleRate < 0 || config.SampleRate > 1 { + return nil, fmt.Errorf("invalid tracing sample rate %v: must be within [0, 1]", config.SampleRate) + } + rate := config.SampleRate + if rate == 0 { + rate = DefaultSampleRate + } + valueLimit := config.AttributeValueLimit + if valueLimit <= 0 { + valueLimit = DefaultAttributeValueLimit + } + timeout := config.InitTimeout + if timeout <= 0 { + timeout = DefaultInitTimeout + } + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() // Create OTLP exporter - client := otlptracegrpc.NewClient( - otlptracegrpc.WithEndpoint(config.Endpoint), - otlptracegrpc.WithInsecure(), // Use WithTLSCredentials in production - ) + opts := []otlptracegrpc.Option{otlptracegrpc.WithEndpoint(config.Endpoint)} + if config.Insecure { + opts = append(opts, otlptracegrpc.WithInsecure()) + } else { + tlsCfg := config.TLSConfig + if tlsCfg == nil { + tlsCfg = &tls.Config{MinVersion: tls.VersionTLS12} + } + opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsCfg))) + } + if len(config.Headers) > 0 { + opts = append(opts, otlptracegrpc.WithHeaders(config.Headers)) + } - exporter, err := otlptrace.New(ctx, client) + exporter, err := otlptrace.New(ctx, otlptracegrpc.NewClient(opts...)) if err != nil { return nil, fmt.Errorf("failed to create OTLP exporter: %w", err) } @@ -54,14 +129,18 @@ func InitTracer(config Config) (func(context.Context) error, error) { ), ) if err != nil { + _ = exporter.Shutdown(ctx) return nil, fmt.Errorf("failed to create resource: %w", err) } // Create trace provider + limits := sdktrace.NewSpanLimits() + limits.AttributeValueLengthLimit = valueLimit tp := sdktrace.NewTracerProvider( sdktrace.WithBatcher(exporter), sdktrace.WithResource(res), - sdktrace.WithSampler(sdktrace.AlwaysSample()), + sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(rate))), + sdktrace.WithRawSpanLimits(limits), ) // Set global trace provider @@ -74,49 +153,145 @@ func InitTracer(config Config) (func(context.Context) error, error) { )) // Get tracer - tracer = tp.Tracer(config.ServiceName) + t := tp.Tracer(config.ServiceName) + tracer.Store(&t) + initialized = true // Return shutdown function - return tp.Shutdown, nil + return func(ctx context.Context) error { + initMu.Lock() + tracer.Store(nil) + initialized = false + initMu.Unlock() + return tp.Shutdown(ctx) + }, nil } -// Middleware returns an HTTP middleware that creates spans for requests +func currentTracer() trace.Tracer { + if p := tracer.Load(); p != nil { + return *p + } + return nil +} + +// statusRecorder captures the response status code. +type statusRecorder struct { + http.ResponseWriter + status int +} + +func (s *statusRecorder) WriteHeader(code int) { + if s.status == 0 { + s.status = code + } + s.ResponseWriter.WriteHeader(code) +} + +func (s *statusRecorder) Write(b []byte) (int, error) { + if s.status == 0 { + s.status = http.StatusOK + } + return s.ResponseWriter.Write(b) +} + +// Unwrap lets http.ResponseController reach the underlying writer. +func (s *statusRecorder) Unwrap() http.ResponseWriter { return s.ResponseWriter } + +// Flush preserves streaming support. +func (s *statusRecorder) Flush() { + if f, ok := s.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +// Middleware returns an HTTP middleware that creates spans for requests. The span +// name uses the matched route pattern (http.ServeMux's Request.Pattern); use +// MiddlewareWithRoute for other routers. It should be installed inside the panic +// recovery middleware: panics are recorded on the span and re-raised. func Middleware(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if tracer == nil { - next.ServeHTTP(w, r) - return - } + return MiddlewareWithRoute(nil)(next) +} - // Extract context from request headers - ctx := otel.GetTextMapPropagator().Extract(r.Context(), propagation.HeaderCarrier(r.Header)) +// MiddlewareWithRoute is like Middleware but resolves the low-cardinality route +// template with fn, called after the handler ran (so the router has matched). +// A nil fn falls back to Request.Pattern. +func MiddlewareWithRoute(fn func(*http.Request) string) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t := currentTracer() + if t == nil { + next.ServeHTTP(w, r) + return + } - // Start span - ctx, span := tracer.Start(ctx, r.Method+" "+r.URL.Path, - trace.WithSpanKind(trace.SpanKindServer), - trace.WithAttributes( - semconv.HTTPMethodKey.String(r.Method), - semconv.HTTPURLKey.String(r.URL.String()), - semconv.HTTPTargetKey.String(r.URL.Path), - semconv.HTTPSchemeKey.String(r.URL.Scheme), - semconv.NetHostNameKey.String(r.Host), - ), - ) - defer span.End() + // Extract context from request headers + ctx := otel.GetTextMapPropagator().Extract(r.Context(), propagation.HeaderCarrier(r.Header)) - // Create new request with updated context - r = r.WithContext(ctx) + // Start span; the query string is deliberately not exported and the + // name is refined to the route template once routing has happened. + ctx, span := t.Start(ctx, r.Method, + trace.WithSpanKind(trace.SpanKindServer), + trace.WithAttributes( + semconv.HTTPRequestMethodKey.String(r.Method), + semconv.URLPath(r.URL.Path), + semconv.URLScheme(requestScheme(r)), + ), + ) + rec := &statusRecorder{ResponseWriter: w} + // Create new request with updated context + r = r.WithContext(ctx) - next.ServeHTTP(w, r) - }) + defer func() { + route := unmatchedRoute + if fn != nil { + if v := fn(r); v != "" { + route = v + } + } else if r.Pattern != "" { + route = r.Pattern + } + if p := recover(); p != nil { + span.RecordError(fmt.Errorf("panic: %v", p)) + span.SetStatus(codes.Error, "panic") + span.SetAttributes(semconv.HTTPResponseStatusCode(http.StatusInternalServerError)) + span.SetName(r.Method + " " + route) + span.End() + panic(p) + } + status := rec.status + if status == 0 { + status = http.StatusOK + } + span.SetName(r.Method + " " + route) + span.SetAttributes(semconv.HTTPRoute(route), semconv.HTTPResponseStatusCode(status)) + if status >= 500 { + span.SetStatus(codes.Error, http.StatusText(status)) + } + span.End() + }() + + next.ServeHTTP(rec, r) + }) + } +} + +func requestScheme(r *http.Request) string { + if r.URL.Scheme != "" { + return r.URL.Scheme + } + if r.TLS != nil { + return "https" + } + return "http" } // StartSpan starts a new span with the given name func StartSpan(ctx context.Context, name string, attrs ...attribute.KeyValue) (context.Context, trace.Span) { - if tracer == nil { + t := currentTracer() + if t == nil { return ctx, trace.SpanFromContext(ctx) } - return tracer.Start(ctx, name, trace.WithAttributes(attrs...)) + return t.Start(ctx, name, trace.WithAttributes(attrs...)) } // SpanFromContext returns the current span from the context diff --git a/pkg/tracing/tracing_test.go b/pkg/tracing/tracing_test.go new file mode 100644 index 0000000..a2e2501 --- /dev/null +++ b/pkg/tracing/tracing_test.go @@ -0,0 +1,100 @@ +package tracing + +import ( + "net/http" + "net/http/httptest" + "testing" + + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + "go.opentelemetry.io/otel/trace" +) + +func setup(t *testing.T) *tracetest.SpanRecorder { + t.Helper() + sr := tracetest.NewSpanRecorder() + tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) + tr := tp.Tracer("test") + tracer.Store(&tr) + t.Cleanup(func() { tracer.Store(nil) }) + return sr +} + +func attrs(s sdktrace.ReadOnlySpan) map[string]string { + m := map[string]string{} + for _, kv := range s.Attributes() { + m[string(kv.Key)] = kv.Value.Emit() + } + return m +} + +func TestMiddlewareRedactsQueryAndUsesRoute(t *testing.T) { + sr := setup(t) + mux := http.NewServeMux() + mux.HandleFunc("GET /api/{id}", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) }) + h := Middleware(mux) + + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/api/42?token=secret", nil)) + + spans := sr.Ended() + if len(spans) != 1 { + t.Fatalf("got %d spans", len(spans)) + } + s := spans[0] + if s.Name() != "GET GET /api/{id}" { + t.Errorf("name = %q", s.Name()) + } + for k, v := range attrs(s) { + if v == "secret" || k == "http.url" || k == "url.full" || k == "url.query" { + t.Errorf("leaked %s=%s", k, v) + } + } + if a := attrs(s); a["http.response.status_code"] != "500" { + t.Errorf("status attr = %q", a["http.response.status_code"]) + } + if s.Status().Code.String() != "Error" { + t.Errorf("status = %v", s.Status().Code) + } +} + +func TestMiddlewareUnmatchedRoute(t *testing.T) { + sr := setup(t) + h := Middleware(http.NotFoundHandler()) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/random/7f3c", nil)) + if n := sr.Ended()[0].Name(); n != "GET " { + t.Errorf("name = %q", n) + } +} + +func TestMiddlewareRecordsPanicAndRepanics(t *testing.T) { + sr := setup(t) + h := Middleware(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { panic("boom") })) + func() { + defer func() { + if recover() == nil { + t.Error("panic was swallowed") + } + }() + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/x", nil)) + }() + s := sr.Ended()[0] + if s.Status().Code.String() != "Error" || len(s.Events()) == 0 { + t.Errorf("panic not recorded: %v %d", s.Status(), len(s.Events())) + } + var _ trace.Span +} + +func TestInitTwiceErrors(t *testing.T) { + cfg := Config{Enabled: true, ServiceName: "t", Endpoint: "localhost:4317", Insecure: true} + shutdown, err := InitTracer(cfg) + if err != nil { + t.Fatal(err) + } + if _, err := InitTracer(cfg); err == nil { + t.Error("expected error on second init") + } + _ = shutdown(t.Context()) + if _, err := InitTracer(Config{Enabled: true, SampleRate: 2}); err == nil { + t.Error("expected invalid sample rate error") + } +}