From ee850f5642e46f9d3daf776dc18f91fa53f2833d Mon Sep 17 00:00:00 2001 From: Ankur Shrivastava Date: Thu, 9 Apr 2026 13:22:18 +0800 Subject: [PATCH 1/3] feat: improve config validation, wire DebugLogInterceptor Config validation (4.1): - TLS file existence check via os.Stat - OTLP endpoint host:port format validation - Log level validation against known levels - Timeout vs shutdown duration sanity check DebugLogInterceptor wiring: - Add DisableDebugLogInterceptor and DebugLogHeaderName config fields - Wire to interceptors in processConfig - Update getCustomHeaderMatcher to variadic headers for HTTP support Also adds goleak.VerifyTestMain for goroutine leak detection. --- config/config.go | 39 +++++++++++++++++ config/config_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++ core.go | 35 +++++++++++----- go.mod | 1 + goleak_test.go | 27 ++++++++++++ 5 files changed, 189 insertions(+), 11 deletions(-) create mode 100644 goleak_test.go diff --git a/config/config.go b/config/config.go index 1e4b39e..d43a1a0 100644 --- a/config/config.go +++ b/config/config.go @@ -1,5 +1,11 @@ package config +import ( + "net" + "os" + "strings" +) + // Config is the configuration for the Coldbrew server // It is populated from environment variables and has sensible defaults for all fields so that you can just use it as is without any configuration // The following environment variables are supported and can be used to override the defaults for the fields @@ -106,6 +112,14 @@ type Config struct { // DisableProtoValidate disables the protovalidate interceptor in the default // interceptor chain. When disabled, proto validation annotations are ignored. DisableProtoValidate bool `envconfig:"DISABLE_PROTO_VALIDATE" default:"false"` + // DisableDebugLogInterceptor disables the DebugLogInterceptor in the default + // interceptor chain. When disabled, proto debug fields and metadata headers + // will not trigger per-request debug logging. + DisableDebugLogInterceptor bool `envconfig:"DISABLE_DEBUG_LOG_INTERCEPTOR" default:"false"` + // DebugLogHeaderName is the gRPC metadata / HTTP header name that triggers + // per-request debug logging. The header value should be a valid log level + // (e.g., "debug"). Default: "x-debug-log-level". + DebugLogHeaderName string `envconfig:"DEBUG_LOG_HEADER_NAME" default:"x-debug-log-level"` // DisableVTProtobuf disables the use of the vtprotobuf marshaller and unmarshaller for GRPC // https://github.com/planetscale/vtprotobuf DisableVTProtobuf bool `envconfig:"DISABLE_VT_PROTOBUF" default:"false"` @@ -210,6 +224,31 @@ func (c Config) Validate() []string { if c.GRPCServerDefaultTimeoutInSeconds < 0 { warnings = append(warnings, "GRPCServerDefaultTimeoutInSeconds is negative; use 0 to disable the timeout interceptor") } + if c.GRPCTLSCertFile != "" && c.GRPCTLSKeyFile != "" { + if _, err := os.Stat(c.GRPCTLSCertFile); err != nil { + warnings = append(warnings, "GRPCTLSCertFile not found: "+c.GRPCTLSCertFile) + } + if _, err := os.Stat(c.GRPCTLSKeyFile); err != nil { + warnings = append(warnings, "GRPCTLSKeyFile not found: "+c.GRPCTLSKeyFile) + } + } + if c.OTLPEndpoint != "" { + if _, _, err := net.SplitHostPort(c.OTLPEndpoint); err != nil { + warnings = append(warnings, "OTLPEndpoint should be in host:port format") + } + } + if c.LogLevel != "" { + switch strings.ToLower(c.LogLevel) { + case "error", "warn", "warning", "info", "debug": + // valid + default: + warnings = append(warnings, "LogLevel is not a recognized level: "+c.LogLevel) + } + } + if c.GRPCServerDefaultTimeoutInSeconds > 0 && c.ShutdownDurationInSeconds > 0 && + c.GRPCServerDefaultTimeoutInSeconds > c.ShutdownDurationInSeconds { + warnings = append(warnings, "GRPCServerDefaultTimeoutInSeconds exceeds ShutdownDurationInSeconds; in-flight RPCs may be killed before timeout") + } return warnings } diff --git a/config/config_test.go b/config/config_test.go index 6edc2a0..409bdf1 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -106,3 +106,101 @@ func TestValidateShutdownTiming(t *testing.T) { t.Error("healthcheck duration >= shutdown duration should produce a warning") } } + +func TestValidateTLSFileNotFound(t *testing.T) { + c := Config{ + GRPCPort: 9090, + HTTPPort: 9091, + GRPCTLSCertFile: "/nonexistent/cert.pem", + GRPCTLSKeyFile: "/nonexistent/key.pem", + } + warnings := c.Validate() + foundCert := false + foundKey := false + for _, w := range warnings { + if strings.Contains(w, "GRPCTLSCertFile not found") { + foundCert = true + } + if strings.Contains(w, "GRPCTLSKeyFile not found") { + foundKey = true + } + } + if !foundCert || !foundKey { + t.Errorf("non-existent TLS files should produce warnings, got: %v", warnings) + } +} + +func TestValidateOTLPEndpointFormat(t *testing.T) { + // Invalid endpoint + c := Config{ + GRPCPort: 9090, + HTTPPort: 9091, + OTLPEndpoint: "not-a-host-port", + } + warnings := c.Validate() + found := false + for _, w := range warnings { + if strings.Contains(w, "host:port") { + found = true + } + } + if !found { + t.Error("invalid OTLP endpoint should produce a warning") + } + + // Valid endpoint should not warn + c.OTLPEndpoint = "localhost:4317" + warnings = c.Validate() + for _, w := range warnings { + if strings.Contains(w, "host:port") { + t.Errorf("valid OTLP endpoint should not produce a warning, got: %s", w) + } + } +} + +func TestValidateLogLevel(t *testing.T) { + // Invalid level + c := Config{ + GRPCPort: 9090, + HTTPPort: 9091, + LogLevel: "trace", + } + warnings := c.Validate() + found := false + for _, w := range warnings { + if strings.Contains(w, "not a recognized level") { + found = true + } + } + if !found { + t.Error("invalid log level should produce a warning") + } + + // Valid level should not warn + c.LogLevel = "debug" + warnings = c.Validate() + for _, w := range warnings { + if strings.Contains(w, "not a recognized level") { + t.Errorf("valid log level should not produce a warning, got: %s", w) + } + } +} + +func TestValidateTimeoutExceedsShutdown(t *testing.T) { + c := Config{ + GRPCPort: 9090, + HTTPPort: 9091, + GRPCServerDefaultTimeoutInSeconds: 120, + ShutdownDurationInSeconds: 15, + } + warnings := c.Validate() + found := false + for _, w := range warnings { + if strings.Contains(w, "exceeds ShutdownDurationInSeconds") { + found = true + } + } + if !found { + t.Error("timeout exceeding shutdown duration should produce a warning") + } +} diff --git a/core.go b/core.go index b5650b5..92bee57 100644 --- a/core.go +++ b/core.go @@ -142,6 +142,12 @@ func (c *cb) processConfig() { if c.config.DisableProtoValidate { interceptors.SetDisableProtoValidate(true) } + if c.config.DisableDebugLogInterceptor { + interceptors.SetDisableDebugLogInterceptor(true) + } + if c.config.DebugLogHeaderName != "" { + interceptors.SetDebugLogHeaderName(c.config.DebugLogHeaderName) + } if c.config.EnablePrometheusGRPCHistogram { if len(c.config.PrometheusGRPCHistogramBuckets) > 0 { interceptors.SetServerMetricsOptions( @@ -390,19 +396,26 @@ func spanRouteMiddleware(next runtime.HandlerFunc) runtime.HandlerFunc { } } -// getCustomHeaderMatcher returns a matcher that matches the given header and prefix -func getCustomHeaderMatcher(prefixes []string, header string) func(string) (string, bool) { - header = strings.ToLower(header) +// getCustomHeaderMatcher returns a matcher that matches the given exact headers and prefixes. +// Exact-match headers (e.g., trace header, debug log header) are forwarded from HTTP to gRPC metadata. +func getCustomHeaderMatcher(prefixes []string, headers ...string) func(string) (string, bool) { + lowerHeaders := make([]string, 0, len(headers)) + for _, h := range headers { + if h != "" { + lowerHeaders = append(lowerHeaders, strings.ToLower(h)) + } + } return func(key string) (string, bool) { key = strings.ToLower(key) - if key == header { - return key, true - } else if len(prefixes) > 0 { - for _, prefix := range prefixes { - if len(prefix) > 0 && strings.HasPrefix(key, strings.ToLower(prefix)) { - return key, true - } + for _, h := range lowerHeaders { + if key == h { + return key, true + } + } + for _, prefix := range prefixes { + if len(prefix) > 0 && strings.HasPrefix(key, strings.ToLower(prefix)) { + return key, true } } @@ -425,7 +438,7 @@ func (c *cb) initHTTP(ctx context.Context) (*http.Server, error) { muxOpts := []runtime.ServeMuxOption{ runtime.WithIncomingHeaderMatcher( - getCustomHeaderMatcher(allowedHttpHeaderPrefixes, c.config.TraceHeaderName), + getCustomHeaderMatcher(allowedHttpHeaderPrefixes, c.config.TraceHeaderName, c.config.DebugLogHeaderName), ), runtime.WithMarshalerOption("application/proto", pMar), runtime.WithMarshalerOption("application/protobuf", pMar), diff --git a/go.mod b/go.mod index 0890f5e..07155f4 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.43.0 go.opentelemetry.io/otel/trace v1.43.0 go.uber.org/automaxprocs v1.6.0 + go.uber.org/goleak v1.3.0 golang.org/x/sync v0.20.0 google.golang.org/grpc v1.79.3 google.golang.org/protobuf v1.36.11 diff --git a/goleak_test.go b/goleak_test.go new file mode 100644 index 0000000..e027189 --- /dev/null +++ b/goleak_test.go @@ -0,0 +1,27 @@ +package core + +import ( + "testing" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m, + // gRPC callback serializers are started by resolver and balancer + // components during dial/server setup. They are cleaned up when the + // connection/server is closed, but some tests don't fully tear down. + goleak.IgnoreTopFunction("google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run"), + // OTEL batch span processor runs a background queue for exporting + // spans. Started by TracerProvider in tests that configure OTLP. + goleak.IgnoreTopFunction("go.opentelemetry.io/otel/sdk/trace.(*batchSpanProcessor).processQueue"), + // sentry-go starts batch processor and HTTP transport worker goroutines + // when sentry.Init() is called in tests that set SentryDSN. + goleak.IgnoreTopFunction("github.com/getsentry/sentry-go.(*batchProcessor[...]).run"), + goleak.IgnoreTopFunction("github.com/getsentry/sentry-go.(*HTTPTransport).worker"), + // rollbar-go creates a global async client at package init time + // (rollbar.go:39: std = NewAsync(...)), starting a background goroutine + // unconditionally when the package is imported. Cannot be avoided. + goleak.IgnoreTopFunction("github.com/rollbar/rollbar-go.NewAsyncTransport.func1"), + ) +} From 7ce60c4adc6c16478ab1208a5f71b33cf6199a94 Mon Sep 17 00:00:00 2001 From: Ankur Shrivastava Date: Thu, 9 Apr 2026 14:14:19 +0800 Subject: [PATCH 2/3] fix: differentiate TLS not-found vs access errors, add multi-header test Address review: use os.IsNotExist for "not found" vs general "could not be accessed" with error details. Add TestGetCustomHeaderMatcher_MultipleHeaders for variadic header matching. --- config/config.go | 12 ++++++++++-- core_test.go | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index d43a1a0..2ff3ed1 100644 --- a/config/config.go +++ b/config/config.go @@ -226,10 +226,18 @@ func (c Config) Validate() []string { } if c.GRPCTLSCertFile != "" && c.GRPCTLSKeyFile != "" { if _, err := os.Stat(c.GRPCTLSCertFile); err != nil { - warnings = append(warnings, "GRPCTLSCertFile not found: "+c.GRPCTLSCertFile) + if os.IsNotExist(err) { + warnings = append(warnings, "GRPCTLSCertFile not found: "+c.GRPCTLSCertFile) + } else { + warnings = append(warnings, "GRPCTLSCertFile could not be accessed: "+c.GRPCTLSCertFile+": "+err.Error()) + } } if _, err := os.Stat(c.GRPCTLSKeyFile); err != nil { - warnings = append(warnings, "GRPCTLSKeyFile not found: "+c.GRPCTLSKeyFile) + if os.IsNotExist(err) { + warnings = append(warnings, "GRPCTLSKeyFile not found: "+c.GRPCTLSKeyFile) + } else { + warnings = append(warnings, "GRPCTLSKeyFile could not be accessed: "+c.GRPCTLSKeyFile+": "+err.Error()) + } } } if c.OTLPEndpoint != "" { diff --git a/core_test.go b/core_test.go index babc8b5..f688282 100644 --- a/core_test.go +++ b/core_test.go @@ -164,3 +164,21 @@ func TestGetCustomHeaderMatcher(t *testing.T) { } }) } + +func TestGetCustomHeaderMatcher_MultipleHeaders(t *testing.T) { + // removed t.Parallel() — core tests mutate package-level globals + matcher := getCustomHeaderMatcher(nil, "X-Trace-Id", "X-Debug-Log-Level") + + _, traceMatched := matcher("X-Trace-Id") + if !traceMatched { + t.Fatal("expected trace header to match") + } + _, debugMatched := matcher("X-Debug-Log-Level") + if !debugMatched { + t.Fatal("expected debug log header to match") + } + _, unknownMatched := matcher("X-Random") + if unknownMatched { + t.Fatal("expected unknown header to not match") + } +} From 01f2c6aa152d1eb04cd4e71bc179933ba8f360ed Mon Sep 17 00:00:00 2001 From: Ankur Shrivastava Date: Thu, 9 Apr 2026 15:30:29 +0800 Subject: [PATCH 3/3] chore: bump interceptors v0.1.23, errors v0.2.14, log v0.3.2, tracing v0.2.2 --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index 07155f4..d483b2e 100644 --- a/go.mod +++ b/go.mod @@ -4,12 +4,12 @@ go 1.25.9 require ( github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5 - github.com/go-coldbrew/errors v0.2.13 + github.com/go-coldbrew/errors v0.2.14 github.com/go-coldbrew/hystrixprometheus v0.1.2 - github.com/go-coldbrew/interceptors v0.1.22 - github.com/go-coldbrew/log v0.3.1 + github.com/go-coldbrew/interceptors v0.1.23 + github.com/go-coldbrew/log v0.3.2 github.com/go-coldbrew/options v0.3.0 - github.com/go-coldbrew/tracing v0.2.0 + github.com/go-coldbrew/tracing v0.2.2 github.com/golang/protobuf v1.5.4 github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus v1.1.0 github.com/grpc-ecosystem/grpc-gateway/v2 v2.28.0 diff --git a/go.sum b/go.sum index 0e8a955..b59c621 100644 --- a/go.sum +++ b/go.sum @@ -193,18 +193,18 @@ github.com/ghostiam/protogetter v0.3.20 h1:oW7OPFit2FxZOpmMRPP9FffU4uUpfeE/rEdE1 github.com/ghostiam/protogetter v0.3.20/go.mod h1:FjIu5Yfs6FT391m+Fjp3fbAYJ6rkL/J6ySpZBfnODuI= github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= -github.com/go-coldbrew/errors v0.2.13 h1:OUOEWLml6Mstt0Sskc94VVPhZ/rMfONeNOlHqycaq1g= -github.com/go-coldbrew/errors v0.2.13/go.mod h1:eFLqeTPhgGyvsVVRXcdKGUxEnm+chrpddhL8lfogonk= +github.com/go-coldbrew/errors v0.2.14 h1:SQcV9Kw+hNfNGXjvu4fWl5uXw5NRD6lW+rkHgtzAYZE= +github.com/go-coldbrew/errors v0.2.14/go.mod h1:f9eGGKKF9KmyCpSWZRSqqV4HRWqbzmh1E9lyL8jyL+Y= github.com/go-coldbrew/hystrixprometheus v0.1.2 h1:WSt4FtYr8xNDKgdGWYpMfXGFIK7zdDSBwDSbpuPhBHI= github.com/go-coldbrew/hystrixprometheus v0.1.2/go.mod h1:OrNRHHxZagpmQXNp//oHKOemGSU0ScOqEcJgeKbJ+wg= -github.com/go-coldbrew/interceptors v0.1.22 h1:gglnX6iFl1unC4CoJRauwEmw+ldr5ZsLdxmqhE38Kk8= -github.com/go-coldbrew/interceptors v0.1.22/go.mod h1:qR1CzRxSemxlo+5UhSBYmNbfycRRIWwTO7zLgf3E6GE= -github.com/go-coldbrew/log v0.3.1 h1:Cyx6KWBW3wZE8dSru6mIDFtUnJ1R2h6C44ZDo5bOqAo= -github.com/go-coldbrew/log v0.3.1/go.mod h1:xxZGHBfni5eXc6Azg+g8UPTmqTJLAf9sX46gAT8o39Y= +github.com/go-coldbrew/interceptors v0.1.23 h1:mbrAx4ztOMei2DeLyxjXKttUvGAFeU5heDVMPhTOctw= +github.com/go-coldbrew/interceptors v0.1.23/go.mod h1:CjH6gc6GrdTNJnw+d7JigYtreWW5PcaGc5QH7zpsJPk= +github.com/go-coldbrew/log v0.3.2 h1:CoHa0PGX7a7o/Cv/ke7PdQfq4LKtbPVypUf3uXcRLMs= +github.com/go-coldbrew/log v0.3.2/go.mod h1:tumRNCmLWRep5wnhS/vzDQ7UMinF6OZ7WW8K/qlXAzc= github.com/go-coldbrew/options v0.3.0 h1:JwyVntb9bzBeFdaHFK6yGVVz30G3aVlqJJ6uVyYQfCc= github.com/go-coldbrew/options v0.3.0/go.mod h1:8JlmgVJXFoY1KiDLsyMmR//q1U1aBItCexvTrVT2Y60= -github.com/go-coldbrew/tracing v0.2.0 h1:WGfdp5PNunOGfjTZGXPFaip3G5qOOMP622JFYA90ML4= -github.com/go-coldbrew/tracing v0.2.0/go.mod h1:phF8WDsadDKK20lgB0Zv2/ocVIrCbVziMd3MMxqr+aU= +github.com/go-coldbrew/tracing v0.2.2 h1:pvRMSwla5txZgtQOi18OqsuJhtsqPbfeC1arH9tJMys= +github.com/go-coldbrew/tracing v0.2.2/go.mod h1:mMYoCOqxFN28fEPMFbufwK0w/6rCiSjGftSr0KrZ0T0= github.com/go-critic/go-critic v0.14.3 h1:5R1qH2iFeo4I/RJU8vTezdqs08Egi4u5p6vOESA0pog= github.com/go-critic/go-critic v0.14.3/go.mod h1:xwntfW6SYAd7h1OqDzmN6hBX/JxsEKl5up/Y2bsxgVQ= github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA=