fix: address code review feedback on telemetry PR
- Use reqLog (with trace_id/span_id) for request completion log - Add SetMetrics doc comment: must be called before Start() - Add explicit histogram buckets for deploy duration (1-300s) - Trim trailing slash in parseEndpoint basePath - Add comment clarifying TraceSampleRate==0 semantics - Guard NewMetrics() with telemetry.Enabled check - Move restart metric after successful StartContainer - Record errors on proxy span via named returns + defer - Extract resolveExternalRoute and injectTelemetryMetrics helpers (gocyclo) - Fix docs: code block language, relative link path
- commit
- 4555ccb34904a327d9253266a8eb524c316902bd
- author
- brice · · 2026-02-10 07:30 UTC
- parent
- febe319
- signature
- unsigned
9 files · +96 -71
M docs/config/telemetry.md +1 -1
| @@ -49,7 +49,7 @@ The `endpoint` field accepts a full URL. Gordon parses it to extract: | |||
| 49 | 49 | ||
| 50 | 50 | For OpenObserve, the endpoint typically includes the organization path: | |
| 51 | 51 | ||
| 52 | - | ``` | |
| 52 | + | ```text | |
| 53 | 53 | http://localhost:5080/api/default | |
| 54 | 54 | ``` | |
| 55 | 55 | ||
| @@ -49,7 +49,7 @@ The `endpoint` field accepts a full URL. Gordon parses it to extract: | |||||
| 49 | 49 | ||||
| 50 | For OpenObserve, the endpoint typically includes the organization path: | 50 | For OpenObserve, the endpoint typically includes the organization path: | ||
| 51 | 51 | ||||
| 52 | - | ``` | 52 | + | ```text |
| 53 | http://localhost:5080/api/default | 53 | http://localhost:5080/api/default | ||
| 54 | ``` | 54 | ``` | ||
| 55 | 55 | ||||
M docs/reference/index.md +1 -1
| @@ -6,7 +6,7 @@ Technical reference documentation for Gordon. | |||
| 6 | 6 | ||
| 7 | 7 | - [Docker Labels](./docker-labels.md) - Container and image labels used by Gordon | |
| 8 | 8 | - [Environment Variables](./env-variables.md) - Environment variable syntax and interpolation | |
| 9 | - | - [Telemetry & Metrics](/docs/config/telemetry.md) - OpenTelemetry configuration, custom metrics, and trace spans | |
| 9 | + | - [Telemetry & Metrics](../config/telemetry.md) - OpenTelemetry configuration, custom metrics, and trace spans | |
| 10 | 10 | - [Troubleshooting](./troubleshooting.md) - Common issues and solutions | |
| 11 | 11 | ||
| 12 | 12 | ## Related | |
| @@ -6,7 +6,7 @@ Technical reference documentation for Gordon. | |||||
| 6 | 6 | ||||
| 7 | - [Docker Labels](./docker-labels.md) - Container and image labels used by Gordon | 7 | - [Docker Labels](./docker-labels.md) - Container and image labels used by Gordon | ||
| 8 | - [Environment Variables](./env-variables.md) - Environment variable syntax and interpolation | 8 | - [Environment Variables](./env-variables.md) - Environment variable syntax and interpolation | ||
| 9 | - | - [Telemetry & Metrics](/docs/config/telemetry.md) - OpenTelemetry configuration, custom metrics, and trace spans | 9 | + | - [Telemetry & Metrics](../config/telemetry.md) - OpenTelemetry configuration, custom metrics, and trace spans |
| 10 | - [Troubleshooting](./troubleshooting.md) - Common issues and solutions | 10 | - [Troubleshooting](./troubleshooting.md) - Common issues and solutions | ||
| 11 | 11 | ||||
| 12 | ## Related | 12 | ## Related | ||
M internal/adapters/in/http/middleware/logging.go +2 -2
| @@ -116,8 +116,8 @@ func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.H | |||
| 116 | 116 | // Calculate duration | |
| 117 | 117 | duration := time.Since(start) | |
| 118 | 118 | ||
| 119 | - | // Log the request | |
| 120 | - | log.Info(). | |
| 119 | + | // Log the request (use reqLog to include trace_id/span_id) | |
| 120 | + | reqLog.Info(). | |
| 121 | 121 | Str(zerowrap.FieldLayer, "adapter"). | |
| 122 | 122 | Str(zerowrap.FieldAdapter, "http"). | |
| 123 | 123 | Str("request_id", requestID). | |
| @@ -116,8 +116,8 @@ func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.H | |||||
| 116 | // Calculate duration | 116 | // Calculate duration | ||
| 117 | duration := time.Since(start) | 117 | duration := time.Since(start) | ||
| 118 | 118 | ||||
| 119 | - | // Log the request | 119 | + | // Log the request (use reqLog to include trace_id/span_id) |
| 120 | - | log.Info(). | 120 | + | reqLog.Info(). |
| 121 | Str(zerowrap.FieldLayer, "adapter"). | 121 | Str(zerowrap.FieldLayer, "adapter"). | ||
| 122 | Str(zerowrap.FieldAdapter, "http"). | 122 | Str(zerowrap.FieldAdapter, "http"). | ||
| 123 | Str("request_id", requestID). | 123 | Str("request_id", requestID). | ||
M internal/adapters/out/eventbus/inmemory.go +1 -0
| @@ -31,6 +31,7 @@ type InMemory struct { | |||
| 31 | 31 | } | |
| 32 | 32 | ||
| 33 | 33 | // SetMetrics sets the telemetry metrics for the event bus. | |
| 34 | + | // Must be called before Start() to avoid data races on bus.metrics reads. | |
| 34 | 35 | func (bus *InMemory) SetMetrics(m *telemetry.Metrics) { | |
| 35 | 36 | bus.mu.Lock() | |
| 36 | 37 | bus.metrics = m | |
| @@ -31,6 +31,7 @@ type InMemory struct { | |||||
| 31 | } | 31 | } | ||
| 32 | 32 | ||||
| 33 | // SetMetrics sets the telemetry metrics for the event bus. | 33 | // SetMetrics sets the telemetry metrics for the event bus. | ||
| 34 | + | // Must be called before Start() to avoid data races on bus.metrics reads. | |||
| 34 | func (bus *InMemory) SetMetrics(m *telemetry.Metrics) { | 35 | func (bus *InMemory) SetMetrics(m *telemetry.Metrics) { | ||
| 35 | bus.mu.Lock() | 36 | bus.mu.Lock() | ||
| 36 | bus.metrics = m | 37 | bus.metrics = m | ||
M internal/adapters/out/telemetry/metrics.go +2 -1
| @@ -40,7 +40,8 @@ func NewMetrics() (*Metrics, error) { | |||
| 40 | 40 | } | |
| 41 | 41 | if m.DeployDuration, err = meter.Float64Histogram("gordon.deploy.duration_seconds", | |
| 42 | 42 | metric.WithDescription("Deploy duration in seconds"), | |
| 43 | - | metric.WithUnit("s")); err != nil { | |
| 43 | + | metric.WithUnit("s"), | |
| 44 | + | metric.WithExplicitBucketBoundaries(1, 5, 10, 30, 60, 120, 300)); err != nil { | |
| 44 | 45 | return nil, err | |
| 45 | 46 | } | |
| 46 | 47 | if m.DeployErrors, err = meter.Int64Counter("gordon.deploy.errors", | |
| @@ -40,7 +40,8 @@ func NewMetrics() (*Metrics, error) { | |||||
| 40 | } | 40 | } | ||
| 41 | if m.DeployDuration, err = meter.Float64Histogram("gordon.deploy.duration_seconds", | 41 | if m.DeployDuration, err = meter.Float64Histogram("gordon.deploy.duration_seconds", | ||
| 42 | metric.WithDescription("Deploy duration in seconds"), | 42 | metric.WithDescription("Deploy duration in seconds"), | ||
| 43 | - | metric.WithUnit("s")); err != nil { | 43 | + | metric.WithUnit("s"), |
| 44 | + | metric.WithExplicitBucketBoundaries(1, 5, 10, 30, 60, 120, 300)); err != nil { | |||
| 44 | return nil, err | 45 | return nil, err | ||
| 45 | } | 46 | } | ||
| 46 | if m.DeployErrors, err = meter.Int64Counter("gordon.deploy.errors", | 47 | if m.DeployErrors, err = meter.Int64Counter("gordon.deploy.errors", | ||
M internal/adapters/out/telemetry/provider.go +3 -1
| @@ -6,6 +6,7 @@ import ( | |||
| 6 | 6 | "context" | |
| 7 | 7 | "fmt" | |
| 8 | 8 | "net/url" | |
| 9 | + | "strings" | |
| 9 | 10 | ||
| 10 | 11 | "go.opentelemetry.io/otel" | |
| 11 | 12 | "go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp" | |
| @@ -115,7 +116,7 @@ func parseEndpoint(cfg Config) (*endpointConfig, error) { | |||
| 115 | 116 | ||
| 116 | 117 | return &endpointConfig{ | |
| 117 | 118 | host: parsedURL.Host, | |
| 118 | - | basePath: parsedURL.Path, | |
| 119 | + | basePath: strings.TrimSuffix(parsedURL.Path, "/"), | |
| 119 | 120 | insecure: parsedURL.Scheme == "http", | |
| 120 | 121 | headers: headers, | |
| 121 | 122 | }, nil | |
| @@ -139,6 +140,7 @@ func (p *Provider) setupTracing(ctx context.Context, ep *endpointConfig, cfg Con | |||
| 139 | 140 | return fmt.Errorf("create trace exporter: %w", err) | |
| 140 | 141 | } | |
| 141 | 142 | ||
| 143 | + | // 0 = never sample (disabled), (0,1) = ratio-based, >= 1 = always sample. | |
| 142 | 144 | sampler := trace.AlwaysSample() | |
| 143 | 145 | if cfg.TraceSampleRate == 0 { | |
| 144 | 146 | sampler = trace.NeverSample() | |
| @@ -6,6 +6,7 @@ import ( | |||||
| 6 | "context" | 6 | "context" | ||
| 7 | "fmt" | 7 | "fmt" | ||
| 8 | "net/url" | 8 | "net/url" | ||
| 9 | + | "strings" | |||
| 9 | 10 | ||||
| 10 | "go.opentelemetry.io/otel" | 11 | "go.opentelemetry.io/otel" | ||
| 11 | "go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp" | 12 | "go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp" | ||
| @@ -115,7 +116,7 @@ func parseEndpoint(cfg Config) (*endpointConfig, error) { | |||||
| 115 | 116 | ||||
| 116 | return &endpointConfig{ | 117 | return &endpointConfig{ | ||
| 117 | host: parsedURL.Host, | 118 | host: parsedURL.Host, | ||
| 118 | - | basePath: parsedURL.Path, | 119 | + | basePath: strings.TrimSuffix(parsedURL.Path, "/"), |
| 119 | insecure: parsedURL.Scheme == "http", | 120 | insecure: parsedURL.Scheme == "http", | ||
| 120 | headers: headers, | 121 | headers: headers, | ||
| 121 | }, nil | 122 | }, nil | ||
| @@ -139,6 +140,7 @@ func (p *Provider) setupTracing(ctx context.Context, ep *endpointConfig, cfg Con | |||||
| 139 | return fmt.Errorf("create trace exporter: %w", err) | 140 | return fmt.Errorf("create trace exporter: %w", err) | ||
| 140 | } | 141 | } | ||
| 141 | 142 | ||||
| 143 | + | // 0 = never sample (disabled), (0,1) = ratio-based, >= 1 = always sample. | |||
| 142 | sampler := trace.AlwaysSample() | 144 | sampler := trace.AlwaysSample() | ||
| 143 | if cfg.TraceSampleRate == 0 { | 145 | if cfg.TraceSampleRate == 0 { | ||
| 144 | sampler = trace.NeverSample() | 146 | sampler = trace.NeverSample() | ||
M internal/app/run.go +17 -9
| @@ -495,15 +495,7 @@ func createServices(ctx context.Context, v *viper.Viper, cfg Config, log zerowra | |||
| 495 | 495 | svc.registrySvc = registrySvc.NewService(svc.blobStorage, svc.manifestStorage, svc.eventBus) | |
| 496 | 496 | svc.imageSvc = images.NewService(svc.runtime, svc.manifestStorage, svc.blobStorage, log) | |
| 497 | 497 | ||
| 498 | - | // Initialize and inject telemetry metrics into services | |
| 499 | - | gordonMetrics, err := telemetry.NewMetrics() | |
| 500 | - | if err != nil { | |
| 501 | - | log.Warn().Err(err).Msg("failed to create telemetry metrics, continuing without metrics") | |
| 502 | - | } else { | |
| 503 | - | svc.containerSvc.SetMetrics(gordonMetrics) | |
| 504 | - | svc.registrySvc.SetMetrics(gordonMetrics) | |
| 505 | - | svc.eventBus.SetMetrics(gordonMetrics) | |
| 506 | - | } | |
| 498 | + | injectTelemetryMetrics(cfg, svc, log) | |
| 507 | 499 | ||
| 508 | 500 | proxyCfg, err := buildProxyConfig(cfg, log) | |
| 509 | 501 | if err != nil { | |
| @@ -551,6 +543,22 @@ func createServices(ctx context.Context, v *viper.Viper, cfg Config, log zerowra | |||
| 551 | 543 | return svc, nil | |
| 552 | 544 | } | |
| 553 | 545 | ||
| 546 | + | // injectTelemetryMetrics creates and injects OTel metrics into services when | |
| 547 | + | // telemetry is enabled. Skipped otherwise to avoid unnecessary allocations. | |
| 548 | + | func injectTelemetryMetrics(cfg Config, svc *services, log zerowrap.Logger) { | |
| 549 | + | if !cfg.Telemetry.Enabled || !cfg.Telemetry.Metrics { | |
| 550 | + | return | |
| 551 | + | } | |
| 552 | + | gordonMetrics, err := telemetry.NewMetrics() | |
| 553 | + | if err != nil { | |
| 554 | + | log.Warn().Err(err).Msg("failed to create telemetry metrics, continuing without metrics") | |
| 555 | + | return | |
| 556 | + | } | |
| 557 | + | svc.containerSvc.SetMetrics(gordonMetrics) | |
| 558 | + | svc.registrySvc.SetMetrics(gordonMetrics) | |
| 559 | + | svc.eventBus.SetMetrics(gordonMetrics) | |
| 560 | + | } | |
| 561 | + | ||
| 554 | 562 | func setupInternalRegistryAuth(svc *services, log zerowrap.Logger) error { | |
| 555 | 563 | var err error | |
| 556 | 564 | svc.internalRegUser, svc.internalRegPass, err = generateInternalRegistryAuth() | |
| @@ -495,15 +495,7 @@ func createServices(ctx context.Context, v *viper.Viper, cfg Config, log zerowra | |||||
| 495 | svc.registrySvc = registrySvc.NewService(svc.blobStorage, svc.manifestStorage, svc.eventBus) | 495 | svc.registrySvc = registrySvc.NewService(svc.blobStorage, svc.manifestStorage, svc.eventBus) | ||
| 496 | svc.imageSvc = images.NewService(svc.runtime, svc.manifestStorage, svc.blobStorage, log) | 496 | svc.imageSvc = images.NewService(svc.runtime, svc.manifestStorage, svc.blobStorage, log) | ||
| 497 | 497 | ||||
| 498 | - | // Initialize and inject telemetry metrics into services | 498 | + | injectTelemetryMetrics(cfg, svc, log) |
| 499 | - | gordonMetrics, err := telemetry.NewMetrics() | |||
| 500 | - | if err != nil { | |||
| 501 | - | log.Warn().Err(err).Msg("failed to create telemetry metrics, continuing without metrics") | |||
| 502 | - | } else { | |||
| 503 | - | svc.containerSvc.SetMetrics(gordonMetrics) | |||
| 504 | - | svc.registrySvc.SetMetrics(gordonMetrics) | |||
| 505 | - | svc.eventBus.SetMetrics(gordonMetrics) | |||
| 506 | - | } | |||
| 507 | 499 | ||||
| 508 | proxyCfg, err := buildProxyConfig(cfg, log) | 500 | proxyCfg, err := buildProxyConfig(cfg, log) | ||
| 509 | if err != nil { | 501 | if err != nil { | ||
| @@ -551,6 +543,22 @@ func createServices(ctx context.Context, v *viper.Viper, cfg Config, log zerowra | |||||
| 551 | return svc, nil | 543 | return svc, nil | ||
| 552 | } | 544 | } | ||
| 553 | 545 | ||||
| 546 | + | // injectTelemetryMetrics creates and injects OTel metrics into services when | |||
| 547 | + | // telemetry is enabled. Skipped otherwise to avoid unnecessary allocations. | |||
| 548 | + | func injectTelemetryMetrics(cfg Config, svc *services, log zerowrap.Logger) { | |||
| 549 | + | if !cfg.Telemetry.Enabled || !cfg.Telemetry.Metrics { | |||
| 550 | + | return | |||
| 551 | + | } | |||
| 552 | + | gordonMetrics, err := telemetry.NewMetrics() | |||
| 553 | + | if err != nil { | |||
| 554 | + | log.Warn().Err(err).Msg("failed to create telemetry metrics, continuing without metrics") | |||
| 555 | + | return | |||
| 556 | + | } | |||
| 557 | + | svc.containerSvc.SetMetrics(gordonMetrics) | |||
| 558 | + | svc.registrySvc.SetMetrics(gordonMetrics) | |||
| 559 | + | svc.eventBus.SetMetrics(gordonMetrics) | |||
| 560 | + | } | |||
| 561 | + | ||||
| 554 | func setupInternalRegistryAuth(svc *services, log zerowrap.Logger) error { | 562 | func setupInternalRegistryAuth(svc *services, log zerowrap.Logger) error { | ||
| 555 | var err error | 563 | var err error | ||
| 556 | svc.internalRegUser, svc.internalRegPass, err = generateInternalRegistryAuth() | 564 | svc.internalRegUser, svc.internalRegPass, err = generateInternalRegistryAuth() | ||
M internal/usecase/container/monitor.go +6 -5
| @@ -224,7 +224,12 @@ func (m *Monitor) handleCrash(ctx context.Context, log zerowrap.Logger, domainNa | |||
| 224 | 224 | Int("exit_code", exitCode). | |
| 225 | 225 | Msg("monitor: restarting crashed container") | |
| 226 | 226 | ||
| 227 | - | // Record restart metric | |
| 227 | + | if err := m.service.runtime.StartContainer(ctx, containerID); err != nil { | |
| 228 | + | log.Warn().Err(err).Str("domain", domainName).Msg("monitor: failed to restart container") | |
| 229 | + | return | |
| 230 | + | } | |
| 231 | + | ||
| 232 | + | // Record restart metric only on successful restart | |
| 228 | 233 | if m.service.metrics != nil { | |
| 229 | 234 | attrs := metric.WithAttributes( | |
| 230 | 235 | attribute.String("domain", domainName), | |
| @@ -232,10 +237,6 @@ func (m *Monitor) handleCrash(ctx context.Context, log zerowrap.Logger, domainNa | |||
| 232 | 237 | ) | |
| 233 | 238 | m.service.metrics.ContainerRestarts.Add(ctx, 1, attrs) | |
| 234 | 239 | } | |
| 235 | - | ||
| 236 | - | if err := m.service.runtime.StartContainer(ctx, containerID); err != nil { | |
| 237 | - | log.Warn().Err(err).Str("domain", domainName).Msg("monitor: failed to restart container") | |
| 238 | - | } | |
| 239 | 240 | } | |
| 240 | 241 | ||
| 241 | 242 | func (m *Monitor) computeBackoff(consecutive int) time.Duration { | |
| @@ -224,7 +224,12 @@ func (m *Monitor) handleCrash(ctx context.Context, log zerowrap.Logger, domainNa | |||||
| 224 | Int("exit_code", exitCode). | 224 | Int("exit_code", exitCode). | ||
| 225 | Msg("monitor: restarting crashed container") | 225 | Msg("monitor: restarting crashed container") | ||
| 226 | 226 | ||||
| 227 | - | // Record restart metric | 227 | + | if err := m.service.runtime.StartContainer(ctx, containerID); err != nil { |
| 228 | + | log.Warn().Err(err).Str("domain", domainName).Msg("monitor: failed to restart container") | |||
| 229 | + | return | |||
| 230 | + | } | |||
| 231 | + | ||||
| 232 | + | // Record restart metric only on successful restart | |||
| 228 | if m.service.metrics != nil { | 233 | if m.service.metrics != nil { | ||
| 229 | attrs := metric.WithAttributes( | 234 | attrs := metric.WithAttributes( | ||
| 230 | attribute.String("domain", domainName), | 235 | attribute.String("domain", domainName), | ||
| @@ -232,10 +237,6 @@ func (m *Monitor) handleCrash(ctx context.Context, log zerowrap.Logger, domainNa | |||||
| 232 | ) | 237 | ) | ||
| 233 | m.service.metrics.ContainerRestarts.Add(ctx, 1, attrs) | 238 | m.service.metrics.ContainerRestarts.Add(ctx, 1, attrs) | ||
| 234 | } | 239 | } | ||
| 235 | - | ||||
| 236 | - | if err := m.service.runtime.StartContainer(ctx, containerID); err != nil { | |||
| 237 | - | log.Warn().Err(err).Str("domain", domainName).Msg("monitor: failed to restart container") | |||
| 238 | - | } | |||
| 239 | } | 240 | } | ||
| 240 | 241 | ||||
| 241 | func (m *Monitor) computeBackoff(consecutive int) time.Duration { | 242 | func (m *Monitor) computeBackoff(consecutive int) time.Duration { | ||
M internal/usecase/proxy/service.go +63 -51
| @@ -20,6 +20,7 @@ import ( | |||
| 20 | 20 | "github.com/bnema/zerowrap" | |
| 21 | 21 | "go.opentelemetry.io/otel" | |
| 22 | 22 | "go.opentelemetry.io/otel/attribute" | |
| 23 | + | "go.opentelemetry.io/otel/codes" | |
| 23 | 24 | "go.opentelemetry.io/otel/trace" | |
| 24 | 25 | ||
| 25 | 26 | "github.com/bnema/gordon/internal/boundaries/in" | |
| @@ -160,10 +161,16 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| 160 | 161 | } | |
| 161 | 162 | ||
| 162 | 163 | // GetTarget returns the proxy target for a given domain. | |
| 163 | - | func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.ProxyTarget, error) { | |
| 164 | + | func (s *Service) GetTarget(ctx context.Context, domainName string) (target *domain.ProxyTarget, retErr error) { | |
| 164 | 165 | ctx, span := proxyTracer.Start(ctx, "proxy.get_target", | |
| 165 | 166 | trace.WithAttributes(attribute.String("domain", domainName))) | |
| 166 | - | defer span.End() | |
| 167 | + | defer func() { | |
| 168 | + | if retErr != nil { | |
| 169 | + | span.RecordError(retErr) | |
| 170 | + | span.SetStatus(codes.Error, retErr.Error()) | |
| 171 | + | } | |
| 172 | + | span.End() | |
| 173 | + | }() | |
| 167 | 174 | ||
| 168 | 175 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |
| 169 | 176 | zerowrap.FieldLayer: "usecase", | |
| @@ -188,54 +195,7 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||
| 188 | 195 | // Check if this is an external route | |
| 189 | 196 | externalRoutes := s.configSvc.GetExternalRoutes() | |
| 190 | 197 | if targetAddr, ok := externalRoutes[domainName]; ok { | |
| 191 | - | host, portStr, err := net.SplitHostPort(targetAddr) | |
| 192 | - | if err != nil { | |
| 193 | - | return nil, log.WrapErrWithFields(err, "invalid external route target", map[string]any{"target": targetAddr}) | |
| 194 | - | } | |
| 195 | - | port, err := strconv.Atoi(portStr) | |
| 196 | - | if err != nil { | |
| 197 | - | return nil, log.WrapErrWithFields(err, "invalid port in external route", map[string]any{"target": targetAddr}) | |
| 198 | - | } | |
| 199 | - | ||
| 200 | - | // SECURITY: Resolve DNS and validate that the target is not an internal/blocked | |
| 201 | - | // network. We use the resolved IP as the proxy target to prevent DNS rebinding | |
| 202 | - | // (TOCTOU) attacks where a hostname resolves to a public IP during validation | |
| 203 | - | // but to a private IP when the proxy connects. | |
| 204 | - | resolvedIP, err := ResolveAndValidateHost(host) | |
| 205 | - | if err != nil { | |
| 206 | - | log.Warn(). | |
| 207 | - | Err(err). | |
| 208 | - | Str("host", host). | |
| 209 | - | Str("domain", domainName). | |
| 210 | - | Msg("SSRF protection: blocked external route to internal network") | |
| 211 | - | return nil, err | |
| 212 | - | } | |
| 213 | - | ||
| 214 | - | // Preserve the original hostname for the Host header so virtual-hosted | |
| 215 | - | // upstreams work correctly. The resolved IP is used for dialing only. | |
| 216 | - | var originalHost string | |
| 217 | - | if resolvedIP != host { | |
| 218 | - | originalHost = host | |
| 219 | - | } | |
| 220 | - | ||
| 221 | - | target := &domain.ProxyTarget{ | |
| 222 | - | Host: resolvedIP, | |
| 223 | - | Port: port, | |
| 224 | - | ContainerID: "", // Not a container | |
| 225 | - | Scheme: "http", | |
| 226 | - | OriginalHost: originalHost, | |
| 227 | - | } | |
| 228 | - | ||
| 229 | - | // Cache external route target | |
| 230 | - | s.mu.Lock() | |
| 231 | - | s.targets[domainName] = target | |
| 232 | - | s.mu.Unlock() | |
| 233 | - | ||
| 234 | - | log.Debug(). | |
| 235 | - | Str("host", host). | |
| 236 | - | Int("port", port). | |
| 237 | - | Msg("using external route target") | |
| 238 | - | return target, nil | |
| 198 | + | return s.resolveExternalRoute(ctx, domainName, targetAddr, log) | |
| 239 | 199 | } | |
| 240 | 200 | ||
| 241 | 201 | // Get container for this domain | |
| @@ -247,7 +207,6 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||
| 247 | 207 | log.Debug().Str("container_id", container.ID).Str("image", container.Image).Msg("found container for domain") | |
| 248 | 208 | ||
| 249 | 209 | // Build target based on runtime mode | |
| 250 | - | var target *domain.ProxyTarget | |
| 251 | 210 | ||
| 252 | 211 | if s.isRunningInContainer() { | |
| 253 | 212 | // Gordon is in a container - use container network | |
| @@ -304,6 +263,59 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||
| 304 | 263 | return target, nil | |
| 305 | 264 | } | |
| 306 | 265 | ||
| 266 | + | // resolveExternalRoute resolves an external route target address into a ProxyTarget, | |
| 267 | + | // performing DNS validation and SSRF protection. | |
| 268 | + | func (s *Service) resolveExternalRoute(_ context.Context, domainName, targetAddr string, log zerowrap.Logger) (*domain.ProxyTarget, error) { | |
| 269 | + | host, portStr, err := net.SplitHostPort(targetAddr) | |
| 270 | + | if err != nil { | |
| 271 | + | return nil, log.WrapErrWithFields(err, "invalid external route target", map[string]any{"target": targetAddr}) | |
| 272 | + | } | |
| 273 | + | port, err := strconv.Atoi(portStr) | |
| 274 | + | if err != nil { | |
| 275 | + | return nil, log.WrapErrWithFields(err, "invalid port in external route", map[string]any{"target": targetAddr}) | |
| 276 | + | } | |
| 277 | + | ||
| 278 | + | // SECURITY: Resolve DNS and validate that the target is not an internal/blocked | |
| 279 | + | // network. We use the resolved IP as the proxy target to prevent DNS rebinding | |
| 280 | + | // (TOCTOU) attacks where a hostname resolves to a public IP during validation | |
| 281 | + | // but to a private IP when the proxy connects. | |
| 282 | + | resolvedIP, err := ResolveAndValidateHost(host) | |
| 283 | + | if err != nil { | |
| 284 | + | log.Warn(). | |
| 285 | + | Err(err). | |
| 286 | + | Str("host", host). | |
| 287 | + | Str("domain", domainName). | |
| 288 | + | Msg("SSRF protection: blocked external route to internal network") | |
| 289 | + | return nil, err | |
| 290 | + | } | |
| 291 | + | ||
| 292 | + | // Preserve the original hostname for the Host header so virtual-hosted | |
| 293 | + | // upstreams work correctly. The resolved IP is used for dialing only. | |
| 294 | + | var originalHost string | |
| 295 | + | if resolvedIP != host { | |
| 296 | + | originalHost = host | |
| 297 | + | } | |
| 298 | + | ||
| 299 | + | t := &domain.ProxyTarget{ | |
| 300 | + | Host: resolvedIP, | |
| 301 | + | Port: port, | |
| 302 | + | ContainerID: "", // Not a container | |
| 303 | + | Scheme: "http", | |
| 304 | + | OriginalHost: originalHost, | |
| 305 | + | } | |
| 306 | + | ||
| 307 | + | // Cache external route target | |
| 308 | + | s.mu.Lock() | |
| 309 | + | s.targets[domainName] = t | |
| 310 | + | s.mu.Unlock() | |
| 311 | + | ||
| 312 | + | log.Debug(). | |
| 313 | + | Str("host", host). | |
| 314 | + | Int("port", port). | |
| 315 | + | Msg("using external route target") | |
| 316 | + | return t, nil | |
| 317 | + | } | |
| 318 | + | ||
| 307 | 319 | // RegisterTarget registers a new proxy target for a domain. | |
| 308 | 320 | func (s *Service) RegisterTarget(_ context.Context, domainName string, target *domain.ProxyTarget) error { | |
| 309 | 321 | s.mu.Lock() | |
| @@ -20,6 +20,7 @@ import ( | |||||
| 20 | "github.com/bnema/zerowrap" | 20 | "github.com/bnema/zerowrap" | ||
| 21 | "go.opentelemetry.io/otel" | 21 | "go.opentelemetry.io/otel" | ||
| 22 | "go.opentelemetry.io/otel/attribute" | 22 | "go.opentelemetry.io/otel/attribute" | ||
| 23 | + | "go.opentelemetry.io/otel/codes" | |||
| 23 | "go.opentelemetry.io/otel/trace" | 24 | "go.opentelemetry.io/otel/trace" | ||
| 24 | 25 | ||||
| 25 | "github.com/bnema/gordon/internal/boundaries/in" | 26 | "github.com/bnema/gordon/internal/boundaries/in" | ||
| @@ -160,10 +161,16 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||||
| 160 | } | 161 | } | ||
| 161 | 162 | ||||
| 162 | // GetTarget returns the proxy target for a given domain. | 163 | // GetTarget returns the proxy target for a given domain. | ||
| 163 | - | func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.ProxyTarget, error) { | 164 | + | func (s *Service) GetTarget(ctx context.Context, domainName string) (target *domain.ProxyTarget, retErr error) { |
| 164 | ctx, span := proxyTracer.Start(ctx, "proxy.get_target", | 165 | ctx, span := proxyTracer.Start(ctx, "proxy.get_target", | ||
| 165 | trace.WithAttributes(attribute.String("domain", domainName))) | 166 | trace.WithAttributes(attribute.String("domain", domainName))) | ||
| 166 | - | defer span.End() | 167 | + | defer func() { |
| 168 | + | if retErr != nil { | |||
| 169 | + | span.RecordError(retErr) | |||
| 170 | + | span.SetStatus(codes.Error, retErr.Error()) | |||
| 171 | + | } | |||
| 172 | + | span.End() | |||
| 173 | + | }() | |||
| 167 | 174 | ||||
| 168 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | 175 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | ||
| 169 | zerowrap.FieldLayer: "usecase", | 176 | zerowrap.FieldLayer: "usecase", | ||
| @@ -188,54 +195,7 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||||
| 188 | // Check if this is an external route | 195 | // Check if this is an external route | ||
| 189 | externalRoutes := s.configSvc.GetExternalRoutes() | 196 | externalRoutes := s.configSvc.GetExternalRoutes() | ||
| 190 | if targetAddr, ok := externalRoutes[domainName]; ok { | 197 | if targetAddr, ok := externalRoutes[domainName]; ok { | ||
| 191 | - | host, portStr, err := net.SplitHostPort(targetAddr) | 198 | + | return s.resolveExternalRoute(ctx, domainName, targetAddr, log) |
| 192 | - | if err != nil { | |||
| 193 | - | return nil, log.WrapErrWithFields(err, "invalid external route target", map[string]any{"target": targetAddr}) | |||
| 194 | - | } | |||
| 195 | - | port, err := strconv.Atoi(portStr) | |||
| 196 | - | if err != nil { | |||
| 197 | - | return nil, log.WrapErrWithFields(err, "invalid port in external route", map[string]any{"target": targetAddr}) | |||
| 198 | - | } | |||
| 199 | - | ||||
| 200 | - | // SECURITY: Resolve DNS and validate that the target is not an internal/blocked | |||
| 201 | - | // network. We use the resolved IP as the proxy target to prevent DNS rebinding | |||
| 202 | - | // (TOCTOU) attacks where a hostname resolves to a public IP during validation | |||
| 203 | - | // but to a private IP when the proxy connects. | |||
| 204 | - | resolvedIP, err := ResolveAndValidateHost(host) | |||
| 205 | - | if err != nil { | |||
| 206 | - | log.Warn(). | |||
| 207 | - | Err(err). | |||
| 208 | - | Str("host", host). | |||
| 209 | - | Str("domain", domainName). | |||
| 210 | - | Msg("SSRF protection: blocked external route to internal network") | |||
| 211 | - | return nil, err | |||
| 212 | - | } | |||
| 213 | - | ||||
| 214 | - | // Preserve the original hostname for the Host header so virtual-hosted | |||
| 215 | - | // upstreams work correctly. The resolved IP is used for dialing only. | |||
| 216 | - | var originalHost string | |||
| 217 | - | if resolvedIP != host { | |||
| 218 | - | originalHost = host | |||
| 219 | - | } | |||
| 220 | - | ||||
| 221 | - | target := &domain.ProxyTarget{ | |||
| 222 | - | Host: resolvedIP, | |||
| 223 | - | Port: port, | |||
| 224 | - | ContainerID: "", // Not a container | |||
| 225 | - | Scheme: "http", | |||
| 226 | - | OriginalHost: originalHost, | |||
| 227 | - | } | |||
| 228 | - | ||||
| 229 | - | // Cache external route target | |||
| 230 | - | s.mu.Lock() | |||
| 231 | - | s.targets[domainName] = target | |||
| 232 | - | s.mu.Unlock() | |||
| 233 | - | ||||
| 234 | - | log.Debug(). | |||
| 235 | - | Str("host", host). | |||
| 236 | - | Int("port", port). | |||
| 237 | - | Msg("using external route target") | |||
| 238 | - | return target, nil | |||
| 239 | } | 199 | } | ||
| 240 | 200 | ||||
| 241 | // Get container for this domain | 201 | // Get container for this domain | ||
| @@ -247,7 +207,6 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||||
| 247 | log.Debug().Str("container_id", container.ID).Str("image", container.Image).Msg("found container for domain") | 207 | log.Debug().Str("container_id", container.ID).Str("image", container.Image).Msg("found container for domain") | ||
| 248 | 208 | ||||
| 249 | // Build target based on runtime mode | 209 | // Build target based on runtime mode | ||
| 250 | - | var target *domain.ProxyTarget | |||
| 251 | 210 | ||||
| 252 | if s.isRunningInContainer() { | 211 | if s.isRunningInContainer() { | ||
| 253 | // Gordon is in a container - use container network | 212 | // Gordon is in a container - use container network | ||
| @@ -304,6 +263,59 @@ func (s *Service) GetTarget(ctx context.Context, domainName string) (*domain.Pro | |||||
| 304 | return target, nil | 263 | return target, nil | ||
| 305 | } | 264 | } | ||
| 306 | 265 | ||||
| 266 | + | // resolveExternalRoute resolves an external route target address into a ProxyTarget, | |||
| 267 | + | // performing DNS validation and SSRF protection. | |||
| 268 | + | func (s *Service) resolveExternalRoute(_ context.Context, domainName, targetAddr string, log zerowrap.Logger) (*domain.ProxyTarget, error) { | |||
| 269 | + | host, portStr, err := net.SplitHostPort(targetAddr) | |||
| 270 | + | if err != nil { | |||
| 271 | + | return nil, log.WrapErrWithFields(err, "invalid external route target", map[string]any{"target": targetAddr}) | |||
| 272 | + | } | |||
| 273 | + | port, err := strconv.Atoi(portStr) | |||
| 274 | + | if err != nil { | |||
| 275 | + | return nil, log.WrapErrWithFields(err, "invalid port in external route", map[string]any{"target": targetAddr}) | |||
| 276 | + | } | |||
| 277 | + | ||||
| 278 | + | // SECURITY: Resolve DNS and validate that the target is not an internal/blocked | |||
| 279 | + | // network. We use the resolved IP as the proxy target to prevent DNS rebinding | |||
| 280 | + | // (TOCTOU) attacks where a hostname resolves to a public IP during validation | |||
| 281 | + | // but to a private IP when the proxy connects. | |||
| 282 | + | resolvedIP, err := ResolveAndValidateHost(host) | |||
| 283 | + | if err != nil { | |||
| 284 | + | log.Warn(). | |||
| 285 | + | Err(err). | |||
| 286 | + | Str("host", host). | |||
| 287 | + | Str("domain", domainName). | |||
| 288 | + | Msg("SSRF protection: blocked external route to internal network") | |||
| 289 | + | return nil, err | |||
| 290 | + | } | |||
| 291 | + | ||||
| 292 | + | // Preserve the original hostname for the Host header so virtual-hosted | |||
| 293 | + | // upstreams work correctly. The resolved IP is used for dialing only. | |||
| 294 | + | var originalHost string | |||
| 295 | + | if resolvedIP != host { | |||
| 296 | + | originalHost = host | |||
| 297 | + | } | |||
| 298 | + | ||||
| 299 | + | t := &domain.ProxyTarget{ | |||
| 300 | + | Host: resolvedIP, | |||
| 301 | + | Port: port, | |||
| 302 | + | ContainerID: "", // Not a container | |||
| 303 | + | Scheme: "http", | |||
| 304 | + | OriginalHost: originalHost, | |||
| 305 | + | } | |||
| 306 | + | ||||
| 307 | + | // Cache external route target | |||
| 308 | + | s.mu.Lock() | |||
| 309 | + | s.targets[domainName] = t | |||
| 310 | + | s.mu.Unlock() | |||
| 311 | + | ||||
| 312 | + | log.Debug(). | |||
| 313 | + | Str("host", host). | |||
| 314 | + | Int("port", port). | |||
| 315 | + | Msg("using external route target") | |||
| 316 | + | return t, nil | |||
| 317 | + | } | |||
| 318 | + | ||||
| 307 | // RegisterTarget registers a new proxy target for a domain. | 319 | // RegisterTarget registers a new proxy target for a domain. | ||
| 308 | func (s *Service) RegisterTarget(_ context.Context, domainName string, target *domain.ProxyTarget) error { | 320 | func (s *Service) RegisterTarget(_ context.Context, domainName string, target *domain.ProxyTarget) error { | ||
| 309 | s.mu.Lock() | 321 | s.mu.Lock() | ||