From f0babc7c87af36bd8c53b0c2bbe9b383bb8fd509 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Tue, 17 Jan 2023 16:25:31 -0500 Subject: [PATCH] Add CLI flags for controlling HTTP server timeouts (#4167) ## Which problem is this PR solving? - Allows to control collector's HTTP server timeout settings: idle, read, and read-header - Resolves #4166 ## Short description of the changes - add 3 options to http-server CLI flags Signed-off-by: Yuri Shkuro Signed-off-by: shubbham1215 --- cmd/collector/app/flags/flags.go | 16 ++++++++++++++++ cmd/collector/app/flags/flags_test.go | 6 ++++++ cmd/collector/app/server/http.go | 11 ++++++++++- cmd/collector/app/server/http_test.go | 24 +++++++++++++++++++++++- 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/cmd/collector/app/flags/flags.go b/cmd/collector/app/flags/flags.go index e9a2daf3d2c5..b95e3d583675 100644 --- a/cmd/collector/app/flags/flags.go +++ b/cmd/collector/app/flags/flags.go @@ -38,6 +38,10 @@ const ( flagSuffixHostPort = "host-port" + flagSuffixHTTPReadTimeout = "read-timeout" + flagSuffixHTTPReadHeaderTimeout = "read-header-timeout" + flagSuffixHTTPIdleTimeout = "idle-timeout" + flagSuffixGRPCMaxReceiveMessageLength = "max-message-size" flagSuffixGRPCMaxConnectionAge = "max-connection-age" flagSuffixGRPCMaxConnectionAgeGrace = "max-connection-age-grace" @@ -140,6 +144,12 @@ type HTTPOptions struct { HostPort string // TLS configures secure transport for HTTP endpoint TLS tlscfg.Options + // ReadTimeout sets the respective parameter of http.Server + ReadTimeout time.Duration + // ReadHeaderTimeout sets the respective parameter of http.Server + ReadHeaderTimeout time.Duration + // IdleTimeout sets the respective parameter of http.Server + IdleTimeout time.Duration } // GRPCOptions defines options for a gRPC server @@ -185,6 +195,9 @@ func AddFlags(flags *flag.FlagSet) { func addHTTPFlags(flags *flag.FlagSet, cfg serverFlagsConfig, defaultHostPort string) { flags.String(cfg.prefix+"."+flagSuffixHostPort, defaultHostPort, "The host:port (e.g. 127.0.0.1:12345 or :12345) of the collector's HTTP server") + flags.Duration(cfg.prefix+"."+flagSuffixHTTPIdleTimeout, 0, "See https://pkg.go.dev/net/http#Server") + flags.Duration(cfg.prefix+"."+flagSuffixHTTPReadTimeout, 0, "See https://pkg.go.dev/net/http#Server") + flags.Duration(cfg.prefix+"."+flagSuffixHTTPReadHeaderTimeout, 2*time.Second, "See https://pkg.go.dev/net/http#Server") cfg.tls.AddFlags(flags) } @@ -210,6 +223,9 @@ func addGRPCFlags(flags *flag.FlagSet, cfg serverFlagsConfig, defaultHostPort st func (opts *HTTPOptions) initFromViper(v *viper.Viper, logger *zap.Logger, cfg serverFlagsConfig) error { opts.HostPort = ports.FormatHostPort(v.GetString(cfg.prefix + "." + flagSuffixHostPort)) + opts.IdleTimeout = v.GetDuration(cfg.prefix + "." + flagSuffixHTTPIdleTimeout) + opts.ReadTimeout = v.GetDuration(cfg.prefix + "." + flagSuffixHTTPReadTimeout) + opts.ReadHeaderTimeout = v.GetDuration(cfg.prefix + "." + flagSuffixHTTPReadHeaderTimeout) if tlsOpts, err := cfg.tls.InitFromViper(v); err == nil { opts.TLS = tlsOpts } else { diff --git a/cmd/collector/app/flags/flags_test.go b/cmd/collector/app/flags/flags_test.go index 527f7ea7a52e..43628cbb5795 100644 --- a/cmd/collector/app/flags/flags_test.go +++ b/cmd/collector/app/flags/flags_test.go @@ -99,12 +99,18 @@ func TestCollectorOptionsWithFlags_CheckMaxConnectionAge(t *testing.T) { command.ParseFlags([]string{ "--collector.grpc-server.max-connection-age=5m", "--collector.grpc-server.max-connection-age-grace=1m", + "--collector.http-server.idle-timeout=5m", + "--collector.http-server.read-timeout=6m", + "--collector.http-server.read-header-timeout=5s", }) _, err := c.InitFromViper(v, zap.NewNop()) require.NoError(t, err) assert.Equal(t, 5*time.Minute, c.GRPC.MaxConnectionAge) assert.Equal(t, time.Minute, c.GRPC.MaxConnectionAgeGrace) + assert.Equal(t, 5*time.Minute, c.HTTP.IdleTimeout) + assert.Equal(t, 6*time.Minute, c.HTTP.ReadTimeout) + assert.Equal(t, 5*time.Second, c.HTTP.ReadHeaderTimeout) } func TestCollectorOptionsWithFlags_CheckNoTenancy(t *testing.T) { diff --git a/cmd/collector/app/server/http.go b/cmd/collector/app/server/http.go index 6461ace05b24..f903c048f101 100644 --- a/cmd/collector/app/server/http.go +++ b/cmd/collector/app/server/http.go @@ -42,6 +42,13 @@ type HTTPServerParams struct { MetricsFactory metrics.Factory HealthCheck *healthcheck.HealthCheck Logger *zap.Logger + + // ReadTimeout sets the respective parameter of http.Server + ReadTimeout time.Duration + // ReadHeaderTimeout sets the respective parameter of http.Server + ReadHeaderTimeout time.Duration + // IdleTimeout sets the respective parameter of http.Server + IdleTimeout time.Duration } // StartHTTPServer based on the given parameters @@ -51,8 +58,10 @@ func StartHTTPServer(params *HTTPServerParams) (*http.Server, error) { errorLog, _ := zap.NewStdLogAt(params.Logger, zapcore.ErrorLevel) server := &http.Server{ Addr: params.HostPort, + ReadTimeout: params.ReadTimeout, + ReadHeaderTimeout: params.ReadHeaderTimeout, + IdleTimeout: params.IdleTimeout, ErrorLog: errorLog, - ReadHeaderTimeout: 2 * time.Second, } if params.TLSConfig.Enabled { tlsCfg, err := params.TLSConfig.Config(params.Logger) // This checks if the certificates are correctly provided diff --git a/cmd/collector/app/server/http_test.go b/cmd/collector/app/server/http_test.go index 5e9b571d49c9..bbff5f554fb4 100644 --- a/cmd/collector/app/server/http_test.go +++ b/cmd/collector/app/server/http_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zaptest" "github.com/jaegertracing/jaeger/cmd/collector/app/handler" "github.com/jaegertracing/jaeger/internal/metricstest" @@ -201,7 +202,7 @@ func TestSpanCollectorHTTPS(t *testing.T) { for _, test := range testCases { t.Run(test.name, func(t *testing.T) { - logger, _ := zap.NewDevelopment() + logger := zaptest.NewLogger(t) params := &HTTPServerParams{ HostPort: fmt.Sprintf(":%d", ports.CollectorHTTP), Handler: handler.NewJaegerSpanHandler(logger, &mockSpanProcessor{}), @@ -257,3 +258,24 @@ func TestSpanCollectorHTTPS(t *testing.T) { }) } } + +func TestStartHTTPServerParams(t *testing.T) { + logger := zaptest.NewLogger(t) + params := &HTTPServerParams{ + HostPort: fmt.Sprintf(":%d", ports.CollectorHTTP), + Handler: handler.NewJaegerSpanHandler(logger, &mockSpanProcessor{}), + SamplingStore: &mockSamplingStore{}, + MetricsFactory: metricstest.NewFactory(time.Hour), + HealthCheck: healthcheck.New(), + Logger: logger, + IdleTimeout: 5 * time.Minute, + ReadTimeout: 6 * time.Minute, + ReadHeaderTimeout: 7 * time.Second, + } + + server, err := StartHTTPServer(params) + require.NoError(t, err) + assert.Equal(t, 5*time.Minute, server.IdleTimeout) + assert.Equal(t, 6*time.Minute, server.ReadTimeout) + assert.Equal(t, 7*time.Second, server.ReadHeaderTimeout) +}