From da49e8e16be0a63c90294886b6e44feb03a180da Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Mon, 27 Jan 2025 22:23:16 +0000 Subject: [PATCH 1/7] Fixing various linting failures for GitLab Workhorse --- workhorse/cmd/gitlab-workhorse/main.go | 50 +++++++++++++++------ workhorse/cmd/gitlab-workhorse/main_test.go | 7 ++- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index ecc5c5d5d7a4f0..8f9b13a7631a70 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -6,7 +6,7 @@ import ( "fmt" "net" "net/http" - _ "net/http/pprof" + _ "net/http/pprof" // nolint:gosec "os" "os/signal" "syscall" @@ -76,8 +76,8 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error cfg.Version = Version fset := flag.NewFlagSet(arg0, flag.ContinueOnError) fset.Usage = func() { - fmt.Fprintf(fset.Output(), "Usage of %s:\n", arg0) - fmt.Fprintf(fset.Output(), "\n %s [OPTIONS]\n\nOptions:\n", arg0) + _, _ = fmt.Fprintf(fset.Output(), "Usage of %s:\n", arg0) + _, _ = fmt.Fprintf(fset.Output(), "\n %s [OPTIONS]\n\nOptions:\n", arg0) fset.PrintDefaults() } @@ -164,6 +164,31 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error return boot, cfg, nil } +func initializePprof(listenerAddress string, errors chan error) (*http.Server, error) { + if listenerAddress != "" { + return nil, nil + } + + l, err := net.Listen("tcp", listenerAddress) + if err != nil { + return nil, fmt.Errorf("pprofListenAddr: %v", err) + } + + server := &http.Server{ + Addr: listenerAddress, + Handler: nil, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 10 * time.Second, + } + + go func() { + errors <- server.Serve(l) + }() + + return server, nil +} + // run() lets us use normal Go error handling; there is no log.Fatal in run(). func run(boot bootConfig, cfg config.Config) error { closer, err := startLogging(boot.logFile, boot.logFormat) @@ -189,17 +214,13 @@ func run(boot bootConfig, cfg config.Config) error { // requests can only reach the profiler if we start a listener. So by // having no profiler HTTP listener by default, the profiler is // effectively disabled by default. - var l net.Listener - - if boot.pprofListenAddr != "" { - l, err = net.Listen("tcp", boot.pprofListenAddr) - if err != nil { - return fmt.Errorf("pprofListenAddr: %v", err) - } - - go func() { finalErrors <- http.Serve(l, nil) }() + _, err = initializePprof(boot.pprofListenAddr, finalErrors) + if err != nil { + return err } + var l net.Listener + monitoringOpts := []monitoring.Option{monitoring.WithBuildInformation(Version, BuildTime)} if cfg.MetricsListener != nil { l, err = newListener("metrics", *cfg.MetricsListener) @@ -265,7 +286,10 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) - srv := &http.Server{Handler: up} + srv := &http.Server{ + Handler: up, + ReadHeaderTimeout: time.Second, + } for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) } diff --git a/workhorse/cmd/gitlab-workhorse/main_test.go b/workhorse/cmd/gitlab-workhorse/main_test.go index f61413fc85b9b7..832e7436cdde3e 100644 --- a/workhorse/cmd/gitlab-workhorse/main_test.go +++ b/workhorse/cmd/gitlab-workhorse/main_test.go @@ -54,10 +54,13 @@ func TestMain(m *testing.M) { log.WithError(err).Fatal() } - defer gitaly.CloseConnections() gitaly.InitializeSidechannelRegistry(logrus.StandardLogger()) - os.Exit(m.Run()) + code := m.Run() + + gitaly.CloseConnections() + + os.Exit(code) } func TestDeniedClone(t *testing.T) { -- GitLab From 2b633afa46b13d7f827f2c743d851f4686e23e79 Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Mon, 27 Jan 2025 22:27:47 +0000 Subject: [PATCH 2/7] Update last known lint issues --- workhorse/_support/lint_last_known_acceptable.txt | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/workhorse/_support/lint_last_known_acceptable.txt b/workhorse/_support/lint_last_known_acceptable.txt index a8e25ca00c55e6..88f59c40f8de15 100644 --- a/workhorse/_support/lint_last_known_acceptable.txt +++ b/workhorse/_support/lint_last_known_acceptable.txt @@ -1,13 +1,7 @@ cmd/gitlab-workhorse/config_test.go:191: cmd/gitlab-workhorse/config_test.go:191: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO this is meant to be 50*time.Second ..." (godox) cmd/gitlab-workhorse/listener.go:26:16: G402: TLS MinVersion too low. (gosec) -cmd/gitlab-workhorse/main.go:9:2: G108: Profiling endpoint is automatically exposed on /debug/pprof (gosec) cmd/gitlab-workhorse/main.go:73: Function 'buildConfig' has too many statements (63 > 40) (funlen) -cmd/gitlab-workhorse/main.go:79:14: Error return value of `fmt.Fprintf` is not checked (errcheck) -cmd/gitlab-workhorse/main.go:80:14: Error return value of `fmt.Fprintf` is not checked (errcheck) -cmd/gitlab-workhorse/main.go:168: Function 'run' has too many statements (62 > 40) (funlen) -cmd/gitlab-workhorse/main.go:200:30: G114: Use of net/http serve function that has no support for setting timeouts (gosec) -cmd/gitlab-workhorse/main.go:268:10: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec) -cmd/gitlab-workhorse/main_test.go:60:2: exitAfterDefer: os.Exit will exit, and `defer gitaly.CloseConnections()` will not run (gocritic) +cmd/gitlab-workhorse/main.go:193: Function 'run' has too many statements (59 > 40) (funlen) internal/api/channel_settings.go:57:28: G402: TLS MinVersion too low. (gosec) internal/channel/channel.go:128:31: response body must be closed (bodyclose) internal/config/config.go:247:18: G204: Subprocess launched with variable (gosec) -- GitLab From f6abf33f8e7174f2f20b952b5f6d9895222f32d1 Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Tue, 28 Jan 2025 16:12:15 +0000 Subject: [PATCH 3/7] reverting timeout change and handling error --- workhorse/cmd/gitlab-workhorse/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index 8f9b13a7631a70..6462bcf8f78837 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -51,7 +51,7 @@ func main() { } if err != nil { if _, alreadyPrinted := err.(alreadyPrintedError); !alreadyPrinted { - fmt.Fprintln(os.Stderr, err) + _, _ = fmt.Fprintln(os.Stderr, err) } os.Exit(2) } @@ -286,9 +286,9 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) + // nolint:gosec // Accepting until more is known about timeout requirements srv := &http.Server{ - Handler: up, - ReadHeaderTimeout: time.Second, + Handler: up, } for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) -- GitLab From 7b38a3c3a1c4dd6314f39fe6da7363733c2c3ff3 Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Wed, 29 Jan 2025 08:06:58 +0000 Subject: [PATCH 4/7] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Ash McKenzie --- workhorse/cmd/gitlab-workhorse/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index 6462bcf8f78837..e7b3c677a916f9 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -286,9 +286,9 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) - // nolint:gosec // Accepting until more is known about timeout requirements srv := &http.Server{ - Handler: up, + Handler: up, + ReadHeaderTimeout: 90 * time.Second, } for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) -- GitLab From e8485702e428bcd8eea311b135b64ee8c93da651 Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Wed, 29 Jan 2025 08:28:58 +0000 Subject: [PATCH 5/7] fix indentation --- workhorse/cmd/gitlab-workhorse/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index e7b3c677a916f9..8a8c5aef3ace97 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -288,7 +288,7 @@ func run(boot bootConfig, cfg config.Config) error { srv := &http.Server{ Handler: up, - ReadHeaderTimeout: 90 * time.Second, + ReadHeaderTimeout: 90 * time.Second, } for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) -- GitLab From 4a59ee2fc797915ed4445c9edfa2812de8bf6d04 Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Wed, 29 Jan 2025 08:37:17 +0000 Subject: [PATCH 6/7] use tab for indentation --- workhorse/cmd/gitlab-workhorse/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index 8a8c5aef3ace97..9b8931e5c83c27 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -288,7 +288,7 @@ func run(boot bootConfig, cfg config.Config) error { srv := &http.Server{ Handler: up, - ReadHeaderTimeout: 90 * time.Second, + ReadHeaderTimeout: 90 * time.Second, } for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) -- GitLab From fdfd554a33b5942fe8201645473023dee264b47a Mon Sep 17 00:00:00 2001 From: Alex Marston Date: Wed, 29 Jan 2025 23:16:45 +0000 Subject: [PATCH 7/7] do not set timeouts for http server --- workhorse/_support/lint_last_known_acceptable.txt | 1 + workhorse/cmd/gitlab-workhorse/main.go | 6 ++---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/workhorse/_support/lint_last_known_acceptable.txt b/workhorse/_support/lint_last_known_acceptable.txt index 88f59c40f8de15..36d9ef59575fb3 100644 --- a/workhorse/_support/lint_last_known_acceptable.txt +++ b/workhorse/_support/lint_last_known_acceptable.txt @@ -2,6 +2,7 @@ cmd/gitlab-workhorse/config_test.go:191: cmd/gitlab-workhorse/config_test.go:191 cmd/gitlab-workhorse/listener.go:26:16: G402: TLS MinVersion too low. (gosec) cmd/gitlab-workhorse/main.go:73: Function 'buildConfig' has too many statements (63 > 40) (funlen) cmd/gitlab-workhorse/main.go:193: Function 'run' has too many statements (59 > 40) (funlen) +cmd/gitlab-workhorse/main.go:289:10: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec) internal/api/channel_settings.go:57:28: G402: TLS MinVersion too low. (gosec) internal/channel/channel.go:128:31: response body must be closed (bodyclose) internal/config/config.go:247:18: G204: Subprocess launched with variable (gosec) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index 9b8931e5c83c27..c69710d813fa18 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -286,10 +286,8 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) - srv := &http.Server{ - Handler: up, - ReadHeaderTimeout: 90 * time.Second, - } + srv := &http.Server{Handler: up} + for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) } -- GitLab