From ab1bab5ca26f3b8144b2af6f410fc05dfea34cde Mon Sep 17 00:00:00 2001 From: Mostafa Moradian Date: Sun, 24 Sep 2023 22:42:06 +0200 Subject: [PATCH 1/3] Fix bug in writing headers twice when serving metrics --- cmd/run.go | 9 ++++++++- metrics/utils.go | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 metrics/utils.go diff --git a/cmd/run.go b/cmd/run.go index ea0bfaf3..810373f3 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -352,7 +352,14 @@ var runCmd = &cobra.Command{ span.RecordError(err) sentry.CaptureException(err) } - next.ServeHTTP(responseWriter, request) + // The WriteHeader method intentionally does nothing, to prevent a bug + // in the merging metrics that causes the headers to be written twice, + // which results in an error: "http: superfluous response.WriteHeader call". + next.ServeHTTP( + &metrics.HeaderBypassResponseWriter{ + ResponseWriter: responseWriter, + }, + request) } return http.HandlerFunc(handler) } diff --git a/metrics/utils.go b/metrics/utils.go new file mode 100644 index 00000000..8227c35b --- /dev/null +++ b/metrics/utils.go @@ -0,0 +1,19 @@ +package metrics + +import "net/http" + +// HeaderBypassResponseWriter implements the http.ResponseWriter interface +// and allows us to bypass the response header when writing to the response. +// This is useful for merging metrics from multiple sources. +type HeaderBypassResponseWriter struct { + http.ResponseWriter +} + +// WriteHeader intentionally does nothing, but is required to +// implement the http.ResponseWriter. +func (w *HeaderBypassResponseWriter) WriteHeader(code int) {} + +// Write writes the data to the response. +func (w *HeaderBypassResponseWriter) Write(data []byte) (int, error) { + return w.ResponseWriter.Write(data) +} From 6563ceebaa611cee515ba0af0d950ece05aa3c44 Mon Sep 17 00:00:00 2001 From: Mostafa Moradian Date: Sun, 24 Sep 2023 22:43:22 +0200 Subject: [PATCH 2/3] Create and shutdown HTTP server (for metrics) gracefully --- cmd/run.go | 26 +++++++++++++++++++++++--- cmd/run_test.go | 2 ++ metrics/utils.go | 4 ++-- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/cmd/run.go b/cmd/run.go index 810373f3..5a5fbc7d 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -3,6 +3,7 @@ package cmd import ( "context" "crypto/tls" + "errors" "fmt" "log" "net/http" @@ -54,6 +55,7 @@ var ( globalConfigFile string conf *config.Config pluginRegistry *plugin.Registry + metricsServer *http.Server UsageReportURL = "localhost:59091" @@ -72,6 +74,7 @@ func StopGracefully( pluginTimeoutCtx context.Context, sig os.Signal, metricsMerger *metrics.Merger, + metricsServer *http.Server, pluginRegistry *plugin.Registry, logger zerolog.Logger, servers map[string]*network.Server, @@ -110,6 +113,16 @@ func StopGracefully( logger.Info().Msg("Stopped metrics merger") span.AddEvent("Stopped metrics merger") } + if metricsServer != nil { + //nolint:contextcheck + if err := metricsServer.Shutdown(context.Background()); err != nil { + logger.Error().Err(err).Msg("Failed to stop metrics server") + span.RecordError(err) + } else { + logger.Info().Msg("Stopped metrics server") + span.AddEvent("Stopped metrics server") + } + } for name, server := range servers { logger.Info().Str("name", name).Msg("Stopping server") server.Shutdown() //nolint:contextcheck @@ -378,6 +391,7 @@ var runCmd = &cobra.Command{ if conf.Plugin.EnableMetricsMerger && metricsMerger != nil { handler = mergedMetricsHandler(handler) } + // Check if the metrics server is already running before registering the handler. if _, err = http.Get(address); err != nil { //nolint:gosec http.Handle(metricsConfig.Path, gziphandler.GzipHandler(handler)) @@ -386,9 +400,14 @@ var runCmd = &cobra.Command{ span.RecordError(err) } - //nolint:gosec - if err = http.ListenAndServe( - metricsConfig.Address, nil); err != nil { + // Create a new metrics server. + metricsServer = &http.Server{ + Addr: metricsConfig.Address, + Handler: handler, + } + + // Start the metrics server. + if err = metricsServer.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) { logger.Error().Err(err).Msg("Failed to start metrics server") span.RecordError(err) } @@ -730,6 +749,7 @@ var runCmd = &cobra.Command{ pluginTimeoutCtx, sig, metricsMerger, + metricsServer, pluginRegistry, logger, servers, diff --git a/cmd/run_test.go b/cmd/run_test.go index fd7ef8f0..643ac2f2 100644 --- a/cmd/run_test.go +++ b/cmd/run_test.go @@ -34,6 +34,7 @@ func Test_runCmd(t *testing.T) { nil, nil, nil, + nil, loggers[config.Default], servers, stopChan, @@ -115,6 +116,7 @@ func Test_runCmdWithCachePlugin(t *testing.T) { nil, nil, nil, + nil, loggers[config.Default], servers, stopChan, diff --git a/metrics/utils.go b/metrics/utils.go index 8227c35b..1049c8d5 100644 --- a/metrics/utils.go +++ b/metrics/utils.go @@ -11,9 +11,9 @@ type HeaderBypassResponseWriter struct { // WriteHeader intentionally does nothing, but is required to // implement the http.ResponseWriter. -func (w *HeaderBypassResponseWriter) WriteHeader(code int) {} +func (w *HeaderBypassResponseWriter) WriteHeader(int) {} // Write writes the data to the response. func (w *HeaderBypassResponseWriter) Write(data []byte) (int, error) { - return w.ResponseWriter.Write(data) + return w.ResponseWriter.Write(data) //nolint:wrapcheck } From 2b6f50326bf67dc3d135f2aa25b0eca70daed074 Mon Sep 17 00:00:00 2001 From: Mostafa Moradian Date: Sun, 24 Sep 2023 23:06:49 +0200 Subject: [PATCH 3/3] Add ReadHeaderTimeout to prevent Slowloris attacks --- cmd/run.go | 6 +++--- config/config.go | 7 ++++--- config/constants.go | 5 +++-- config/getters.go | 7 +++++++ config/getters_test.go | 6 ++++++ config/types.go | 7 ++++--- gatewayd.yaml | 1 + 7 files changed, 28 insertions(+), 11 deletions(-) diff --git a/cmd/run.go b/cmd/run.go index 5a5fbc7d..7dc4fcce 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -402,8 +402,9 @@ var runCmd = &cobra.Command{ // Create a new metrics server. metricsServer = &http.Server{ - Addr: metricsConfig.Address, - Handler: handler, + Addr: metricsConfig.Address, + Handler: handler, + ReadHeaderTimeout: metricsConfig.GetReadHeaderTimeout(), } // Start the metrics server. @@ -414,7 +415,6 @@ var runCmd = &cobra.Command{ }(conf.Global.Metrics[config.Default], logger) // This is a notification hook, so we don't care about the result. - // TODO: Use a context with a timeout if data, ok := conf.GlobalKoanf.Get("loggers").(map[string]interface{}); ok { _, err = pluginRegistry.Run( pluginTimeoutCtx, data, v1.HookName_HOOK_NAME_ON_NEW_LOGGER) diff --git a/config/config.go b/config/config.go index c1abc8d1..e2d38f9b 100644 --- a/config/config.go +++ b/config/config.go @@ -101,9 +101,10 @@ func (c *Config) LoadDefaults(ctx context.Context) { } defaultMetric := Metrics{ - Enabled: true, - Address: DefaultMetricsAddress, - Path: DefaultMetricsPath, + Enabled: true, + Address: DefaultMetricsAddress, + Path: DefaultMetricsPath, + ReadHeaderTimeout: DefaultReadHeaderTimeout, } defaultClient := Client{ diff --git a/config/constants.go b/config/constants.go index 6c6709b7..bb7445c7 100644 --- a/config/constants.go +++ b/config/constants.go @@ -123,8 +123,9 @@ const ( ChecksumBufferSize = 65536 // Metrics constants. - DefaultMetricsAddress = "localhost:9090" - DefaultMetricsPath = "/metrics" + DefaultMetricsAddress = "localhost:9090" + DefaultMetricsPath = "/metrics" + DefaultReadHeaderTimeout = 10 * time.Second // Sentry constants. DefaultTraceSampleRate = 0.2 diff --git a/config/getters.go b/config/getters.go index 2b2f6950..47900ff1 100644 --- a/config/getters.go +++ b/config/getters.go @@ -269,3 +269,10 @@ func GetDefaultConfigFilePath(filename string) string { // The fallback is the current directory. return filepath.Join("./", filename) } + +func (m Metrics) GetReadHeaderTimeout() time.Duration { + if m.ReadHeaderTimeout <= 0 { + return DefaultReadHeaderTimeout + } + return m.ReadHeaderTimeout +} diff --git a/config/getters_test.go b/config/getters_test.go index 3ec6bf09..2d485c39 100644 --- a/config/getters_test.go +++ b/config/getters_test.go @@ -128,3 +128,9 @@ func TestGetPlugins(t *testing.T) { func TestGetDefaultConfigFilePath(t *testing.T) { assert.Equal(t, GlobalConfigFilename, GetDefaultConfigFilePath(GlobalConfigFilename)) } + +// TestGetReadTimeout tests the GetReadTimeout function. +func TestGetReadHeaderTimeout(t *testing.T) { + metrics := Metrics{} + assert.Equal(t, DefaultReadHeaderTimeout, metrics.GetReadHeaderTimeout()) +} diff --git a/config/types.go b/config/types.go index a4785e37..17bb1ae7 100644 --- a/config/types.go +++ b/config/types.go @@ -68,9 +68,10 @@ type Logger struct { } type Metrics struct { - Enabled bool `json:"enabled"` - Address string `json:"address"` - Path string `json:"path"` + Enabled bool `json:"enabled"` + Address string `json:"address"` + Path string `json:"path"` + ReadHeaderTimeout time.Duration `json:"readHeaderTimeout" jsonschema:"oneof_type=string;integer"` } type Pool struct { diff --git a/gatewayd.yaml b/gatewayd.yaml index 174343c2..77d5eb76 100644 --- a/gatewayd.yaml +++ b/gatewayd.yaml @@ -24,6 +24,7 @@ metrics: enabled: True address: localhost:9090 path: /metrics + readHeaderTimeout: 10s # duration, prevents Slowloris attacks clients: default: