Add CLI flags for controlling HTTP server timeouts (jaegertracing#4167)

## Which problem is this PR solving? - Allows to control collector's HTTP server timeout settings: idle, read, and read-header - Resolves jaegertracing#4166 ## Short description of the changes - add 3 options to http-server CLI flags Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: shubbham1215 <[email protected]>
shubbham1215 · Mar 5, 2023 · f0babc7 · f0babc7
1 parent 0193164
commit f0babc7
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)
+}