diff --git a/cmd/cloudflared/generic_service.go b/cmd/cloudflared/generic_service.go index bd49d2b4bf4..c25758cb6fc 100644 --- a/cmd/cloudflared/generic_service.go +++ b/cmd/cloudflared/generic_service.go @@ -4,7 +4,6 @@ package main import ( "fmt" - "os" cli "github.com/urfave/cli/v2" @@ -28,7 +27,7 @@ func runApp(app *cli.App, graceShutdownC chan struct{}) { }, }, }) - app.Run(os.Args) + runAppAndExit(app) } func installGenericService(c *cli.Context) error { diff --git a/cmd/cloudflared/linux_service.go b/cmd/cloudflared/linux_service.go index 067d5b404b1..710a339a8bf 100644 --- a/cmd/cloudflared/linux_service.go +++ b/cmd/cloudflared/linux_service.go @@ -51,7 +51,7 @@ out if no configuration file with credentials was found).`, }, }, }) - _ = app.Run(os.Args) + runAppAndExit(app) } // The directory and files that are used by the service. diff --git a/cmd/cloudflared/macos_service.go b/cmd/cloudflared/macos_service.go index d38b715d143..137e5e813d6 100644 --- a/cmd/cloudflared/macos_service.go +++ b/cmd/cloudflared/macos_service.go @@ -50,7 +50,7 @@ causing it to look for credentials in a configuration file upon startup.`, }, }, }) - _ = app.Run(os.Args) + runAppAndExit(app) } func newLaunchdTemplate(installPath, stdoutPath, stderrPath string) *ServiceTemplate { diff --git a/cmd/cloudflared/main_test.go b/cmd/cloudflared/main_test.go new file mode 100644 index 00000000000..06f051daac2 --- /dev/null +++ b/cmd/cloudflared/main_test.go @@ -0,0 +1,40 @@ +package main + +import ( + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMain doubles as the helper-process entry point: when +// GO_WANT_HELPER_PROCESS is set, the test binary re-executes main() so tests +// can assert on the process exit code, which plain unit tests cannot. +func TestMain(m *testing.M) { + if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { + main() + os.Exit(0) + } + os.Exit(m.Run()) +} + +// Regression test for https://github.com/cloudflare/cloudflared/issues/1606: +// an invalid TUNNEL_GRACE_PERIOD value used to make cloudflared exit 0 with no +// output, because runApp discarded the error from app.Run. It must now print +// the parse error and exit non-zero. +func TestRunAppInvalidGracePeriodEnvExitsNonZero(t *testing.T) { + //nolint:gosec // G204: re-executes this test binary (os.Args[0]) with a fixed command line to assert the exit code + cmd := exec.Command(os.Args[0], "tunnel", "run") + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + "TUNNEL_GRACE_PERIOD=10", + ) + out, err := cmd.CombinedOutput() + + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr, "expected a non-zero exit code, got: %s", out) + assert.NotZero(t, exitErr.ExitCode()) + assert.Contains(t, string(out), "grace-period") +} diff --git a/cmd/cloudflared/run_app.go b/cmd/cloudflared/run_app.go new file mode 100644 index 00000000000..0762c267b86 --- /dev/null +++ b/cmd/cloudflared/run_app.go @@ -0,0 +1,20 @@ +package main + +import ( + "fmt" + "os" + + "github.com/urfave/cli/v2" +) + +// runAppAndExit runs the CLI app and exits with a non-zero status when it +// fails. Flag and env-var parse errors (e.g. an invalid TUNNEL_GRACE_PERIOD) +// are returned by app.Run without being printed, so ignoring the error used +// to make cloudflared exit 0 with no output (see #1606). Print the error to +// stderr and exit 1 so scripts and service managers can detect the failure. +func runAppAndExit(app *cli.App) { + if err := app.Run(os.Args); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} diff --git a/cmd/cloudflared/windows_service.go b/cmd/cloudflared/windows_service.go index 95451e3f7a8..b6178e13be8 100644 --- a/cmd/cloudflared/windows_service.go +++ b/cmd/cloudflared/windows_service.go @@ -94,7 +94,7 @@ causing it to look for credentials in a configuration file upon startup.`, log.Fatal().Err(err).Msg("failed to determine if we are running in an interactive session") } if isIntSess { - app.Run(os.Args) + runAppAndExit(app) return } @@ -106,7 +106,7 @@ causing it to look for credentials in a configuration file upon startup.`, if errno, ok := err.(syscall.Errno); ok && int(errno) == serviceControllerConnectionFailure { // Hack: assume this is a false negative from the IsAnInteractiveSession() check above. // Run the app in "interactive" mode anyway. - app.Run(os.Args) + runAppAndExit(app) return } log.Fatal().Err(err).Msgf("%s service failed", windowsServiceName)