From c6f2979986d87d7236b132c687c8887c92248dd8 Mon Sep 17 00:00:00 2001 From: WeidiDeng Date: Wed, 16 Oct 2024 09:28:20 +0800 Subject: caddyhttp: Close http3 server gracefully (#6213) * close http3 server gracefully * update server field * update from upstream --------- Co-authored-by: Matt Holt --- modules/caddyhttp/app.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) (limited to 'modules/caddyhttp/app.go') diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 7dc2bee72..cd32e72d7 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -689,16 +689,7 @@ func (app *App) Stop() error { return } - // First close h3server then close listeners unlike stdlib for several reasons: - // 1, udp has only a single socket, once closed, no more data can be read and - // written. In contrast, closing tcp listeners won't affect established connections. - // This have something to do with graceful shutdown when upstream implements it. - // 2, h3server will only close listeners it's registered (quic listeners). Closing - // listener first and these listeners maybe unregistered thus won't be closed. caddy - // distinguishes quic-listener and underlying datagram sockets. - - // TODO: CloseGracefully, once implemented upstream (see https://github.com/quic-go/quic-go/issues/2103) - if err := server.h3server.Close(); err != nil { + if err := server.h3server.Shutdown(ctx); err != nil { app.logger.Error("HTTP/3 server shutdown", zap.Error(err), zap.Strings("addresses", server.Listen)) -- cgit v1.2.3 From 388c7e898c6cbcddd2c59e8a902238a0b4f06857 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Fri, 18 Oct 2024 18:54:21 +0300 Subject: metrics: move `metrics` up, outside `servers` (#6606) * metrics: move `metrics` up, outside `servers` This change moves the metrics configuration from per-server level to a single config knob within the `http` app. Enabling `metrics` in any of the configured servers inside `http` enables metrics for all servers. Fix #6604 Signed-off-by: Mohammed Al Sahaf * normalize domain name --------- Signed-off-by: Mohammed Al Sahaf --- caddyconfig/httpcaddyfile/httptype.go | 14 ++++++++ caddyconfig/httpcaddyfile/options.go | 20 +++++++++++ caddyconfig/httpcaddyfile/serveroptions.go | 1 + .../metrics_merge_options.caddyfiletest | 39 ++++++++++++++++++++++ .../caddyfile_adapt/metrics_perhost.caddyfiletest | 8 ++--- modules/caddyhttp/app.go | 27 +++++++++++---- modules/caddyhttp/metrics.go | 5 +-- modules/caddyhttp/server.go | 1 + 8 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest (limited to 'modules/caddyhttp/app.go') diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index 4dacd9058..a238a33b4 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/caddyconfig/httpcaddyfile/httptype.go @@ -15,6 +15,7 @@ package httpcaddyfile import ( + "cmp" "encoding/json" "fmt" "net" @@ -186,12 +187,25 @@ func (st ServerType) Setup( return nil, warnings, err } + // hoist the metrics config from per-server to global + metrics, _ := options["metrics"].(*caddyhttp.Metrics) + for _, s := range servers { + if s.Metrics != nil { + metrics = cmp.Or[*caddyhttp.Metrics](metrics, &caddyhttp.Metrics{}) + metrics = &caddyhttp.Metrics{ + PerHost: metrics.PerHost || s.Metrics.PerHost, + } + s.Metrics = nil // we don't need it anymore + } + } + // now that each server is configured, make the HTTP app httpApp := caddyhttp.App{ HTTPPort: tryInt(options["http_port"], &warnings), HTTPSPort: tryInt(options["https_port"], &warnings), GracePeriod: tryDuration(options["grace_period"], &warnings), ShutdownDelay: tryDuration(options["shutdown_delay"], &warnings), + Metrics: metrics, Servers: servers, } diff --git a/caddyconfig/httpcaddyfile/options.go b/caddyconfig/httpcaddyfile/options.go index 9bae760c0..03b9ba230 100644 --- a/caddyconfig/httpcaddyfile/options.go +++ b/caddyconfig/httpcaddyfile/options.go @@ -24,6 +24,7 @@ import ( "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddyconfig" "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" + "github.com/caddyserver/caddy/v2/modules/caddyhttp" "github.com/caddyserver/caddy/v2/modules/caddytls" ) @@ -53,6 +54,7 @@ func init() { RegisterGlobalOption("local_certs", parseOptTrue) RegisterGlobalOption("key_type", parseOptSingleString) RegisterGlobalOption("auto_https", parseOptAutoHTTPS) + RegisterGlobalOption("metrics", parseMetricsOptions) RegisterGlobalOption("servers", parseServerOptions) RegisterGlobalOption("ocsp_stapling", parseOCSPStaplingOptions) RegisterGlobalOption("cert_lifetime", parseOptDuration) @@ -446,6 +448,24 @@ func parseOptAutoHTTPS(d *caddyfile.Dispenser, _ any) (any, error) { return val, nil } +func unmarshalCaddyfileMetricsOptions(d *caddyfile.Dispenser) (any, error) { + d.Next() // consume option name + metrics := new(caddyhttp.Metrics) + for d.NextBlock(0) { + switch d.Val() { + case "per_host": + metrics.PerHost = true + default: + return nil, d.Errf("unrecognized servers option '%s'", d.Val()) + } + } + return metrics, nil +} + +func parseMetricsOptions(d *caddyfile.Dispenser, _ any) (any, error) { + return unmarshalCaddyfileMetricsOptions(d) +} + func parseServerOptions(d *caddyfile.Dispenser, _ any) (any, error) { return unmarshalCaddyfileServerOptions(d) } diff --git a/caddyconfig/httpcaddyfile/serveroptions.go b/caddyconfig/httpcaddyfile/serveroptions.go index b05af47f5..40a8af209 100644 --- a/caddyconfig/httpcaddyfile/serveroptions.go +++ b/caddyconfig/httpcaddyfile/serveroptions.go @@ -240,6 +240,7 @@ func unmarshalCaddyfileServerOptions(d *caddyfile.Dispenser) (any, error) { } case "metrics": + caddy.Log().Warn("The nested 'metrics' option inside `servers` is deprecated and will be removed in the next major version. Use the global 'metrics' option instead.") serverOpts.Metrics = new(caddyhttp.Metrics) for nesting := d.Nesting(); d.NextBlock(nesting); { switch d.Val() { diff --git a/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest b/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest new file mode 100644 index 000000000..946b3d0c8 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/metrics_merge_options.caddyfiletest @@ -0,0 +1,39 @@ +{ + metrics + servers :80 { + metrics { + per_host + } + } +} +:80 { + respond "Hello" +} + +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":80" + ], + "routes": [ + { + "handle": [ + { + "body": "Hello", + "handler": "static_response" + } + ] + } + ] + } + }, + "metrics": { + "per_host": true + } + } + } +} \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest b/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest index 499215515..e362cecc9 100644 --- a/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/metrics_perhost.caddyfiletest @@ -26,11 +26,11 @@ } ] } - ], - "metrics": { - "per_host": true - } + ] } + }, + "metrics": { + "per_host": true } } } diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index cd32e72d7..2d221265f 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -15,6 +15,7 @@ package caddyhttp import ( + "cmp" "context" "crypto/tls" "fmt" @@ -142,6 +143,10 @@ type App struct { // affect functionality. Servers map[string]*Server `json:"servers,omitempty"` + // If set, metrics observations will be enabled. + // This setting is EXPERIMENTAL and subject to change. + Metrics *Metrics `json:"metrics,omitempty"` + ctx caddy.Context logger *zap.Logger tlsApp *caddytls.TLS @@ -184,6 +189,10 @@ func (app *App) Provision(ctx caddy.Context) error { return err } + if app.Metrics != nil { + app.Metrics.init = sync.Once{} + app.Metrics.httpMetrics = &httpMetrics{} + } // prepare each server oldContext := ctx.Context for srvName, srv := range app.Servers { @@ -196,6 +205,15 @@ func (app *App) Provision(ctx caddy.Context) error { srv.errorLogger = app.logger.Named("log.error") srv.shutdownAtMu = new(sync.RWMutex) + if srv.Metrics != nil { + srv.logger.Warn("per-server 'metrics' is deprecated; use 'metrics' in the root 'http' app instead") + app.Metrics = cmp.Or[*Metrics](app.Metrics, &Metrics{ + init: sync.Once{}, + httpMetrics: &httpMetrics{}, + }) + app.Metrics.PerHost = app.Metrics.PerHost || srv.Metrics.PerHost + } + // only enable access logs if configured if srv.Logs != nil { srv.accessLogger = app.logger.Named("log.access") @@ -342,16 +360,11 @@ func (app *App) Provision(ctx caddy.Context) error { srv.listenerWrappers = append([]caddy.ListenerWrapper{new(tlsPlaceholderWrapper)}, srv.listenerWrappers...) } } - // pre-compile the primary handler chain, and be sure to wrap it in our // route handler so that important security checks are done, etc. primaryRoute := emptyHandler if srv.Routes != nil { - if srv.Metrics != nil { - srv.Metrics.init = sync.Once{} - srv.Metrics.httpMetrics = &httpMetrics{} - } - err := srv.Routes.ProvisionHandlers(ctx, srv.Metrics) + err := srv.Routes.ProvisionHandlers(ctx, app.Metrics) if err != nil { return fmt.Errorf("server %s: setting up route handlers: %v", srvName, err) } @@ -370,7 +383,7 @@ func (app *App) Provision(ctx caddy.Context) error { // provision the named routes (they get compiled at runtime) for name, route := range srv.NamedRoutes { - err := route.Provision(ctx, srv.Metrics) + err := route.Provision(ctx, app.Metrics) if err != nil { return fmt.Errorf("server %s: setting up named route '%s' handlers: %v", name, srvName, err) } diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index 947721429..9bb97e0b4 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "strings" "sync" "time" @@ -133,8 +134,8 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": method, "code": ""} if h.metrics.PerHost { - labels["host"] = r.Host - statusLabels["host"] = r.Host + labels["host"] = strings.ToLower(r.Host) + statusLabels["host"] = strings.ToLower(r.Host) } inFlight := h.metrics.httpMetrics.requestInFlight.With(labels) diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 96001c6f9..24fecfd88 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -226,6 +226,7 @@ type Server struct { // If set, metrics observations will be enabled. // This setting is EXPERIMENTAL and subject to change. + // DEPRECATED: Use the app-level `metrics` field. Metrics *Metrics `json:"metrics,omitempty"` name string -- cgit v1.2.3 From 197c564f2032becba14aeec0152fe5eeb639d6c1 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Tue, 19 Nov 2024 11:24:12 -0700 Subject: caddyhttp: Set default ReadHeaderTimeout (1 min) Ref. #6663 --- modules/caddyhttp/app.go | 22 +++++++++++++++++----- modules/caddyhttp/server.go | 1 + 2 files changed, 18 insertions(+), 5 deletions(-) (limited to 'modules/caddyhttp/app.go') diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 2d221265f..850d3aa8f 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -401,6 +401,9 @@ func (app *App) Provision(ctx caddy.Context) error { if srv.IdleTimeout == 0 { srv.IdleTimeout = defaultIdleTimeout } + if srv.ReadHeaderTimeout == 0 { + srv.ReadHeaderTimeout = defaultReadHeaderTimeout // see #6663 + } } ctx.Context = oldContext return nil @@ -770,11 +773,20 @@ func (app *App) httpsPort() int { return app.HTTPSPort } -// defaultIdleTimeout is the default HTTP server timeout -// for closing idle connections; useful to avoid resource -// exhaustion behind hungry CDNs, for example (we've had -// several complaints without this). -const defaultIdleTimeout = caddy.Duration(5 * time.Minute) +const ( + // defaultIdleTimeout is the default HTTP server timeout + // for closing idle connections; useful to avoid resource + // exhaustion behind hungry CDNs, for example (we've had + // several complaints without this). + defaultIdleTimeout = caddy.Duration(5 * time.Minute) + + // defaultReadHeaderTimeout is the default timeout for + // reading HTTP headers from clients. Headers are generally + // small, often less than 1 KB, so it shouldn't take a + // long time even on legitimately slow connections or + // busy servers to read it. + defaultReadHeaderTimeout = caddy.Duration(time.Minute) +) // Interface guards var ( diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 24fecfd88..12c032dee 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -61,6 +61,7 @@ type Server struct { ReadTimeout caddy.Duration `json:"read_timeout,omitempty"` // ReadHeaderTimeout is like ReadTimeout but for request headers. + // Default is 1 minute. ReadHeaderTimeout caddy.Duration `json:"read_header_timeout,omitempty"` // WriteTimeout is how long to allow a write to a client. Note -- cgit v1.2.3