From 68f63f8eee4f3ecf3128cca20ebfc97985471c4f Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Wed, 23 May 2018 14:31:49 +0200 Subject: [PATCH 1/9] Check error from terraformCmd.Wait() Error is silently ignored at the moment, which causes the process to continue even if the Terraform executable could not be moved to /usr/local/bin. --- bootstrap/bootstrap.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 7f85b90819..a75d0a31bf 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -110,7 +110,10 @@ Follow these instructions to create a token (we don't store any tokens): if err != nil { return errors.Wrapf(err, "moving terraform binary into /usr/local/bin") } - terraformCmd.Wait() + err = terraformCmd.Wait() + if err != nil { + return errors.Wrapf(err, "moving terraform binary into /usr/local/bin") + } colorstring.Println("[green]=> installed terraform successfully at /usr/local/bin") } else { colorstring.Println("[green]=> terraform found in $PATH!") From 304171b718124dbf3b8cdab0af6498e1581cc3f7 Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Wed, 23 May 2018 18:46:38 +0200 Subject: [PATCH 2/9] Add missing newline --- bootstrap/bootstrap.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index a75d0a31bf..fe003ad1a8 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -220,7 +220,7 @@ tunnels: time.Sleep(2 * time.Second) _, err = executeCmd("open", []string{pullRequestURL}) if err != nil { - colorstring.Printf("[red]=> opening pull request failed. please go to: %s on the browser", pullRequestURL) + colorstring.Printf("[red]=> opening pull request failed. please go to: %s on the browser\n", pullRequestURL) } s.Stop() From daa246f71dd600da0f985b6f0d78cb56a0e3a469 Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Wed, 23 May 2018 18:46:45 +0200 Subject: [PATCH 3/9] Improve error handling in bootstrap Separate quick operations (copy a file) from long-running, background operations (run ngrok). Use context to cancel background tasks. --- bootstrap/bootstrap.go | 58 ++++++++++++++++++++++-------------------- bootstrap/utils.go | 27 +++++++++++++++----- 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index fe003ad1a8..f3628aa94e 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -105,12 +105,7 @@ Follow these instructions to create a token (we don't store any tokens): colorstring.Println("[green]=> downloaded terraform successfully!") s.Stop() - var terraformCmd *exec.Cmd - terraformCmd, err = executeCmd("mv", []string{"/tmp/terraform", "/usr/local/bin/"}) - if err != nil { - return errors.Wrapf(err, "moving terraform binary into /usr/local/bin") - } - err = terraformCmd.Wait() + err = executeCmd("mv", []string{"/tmp/terraform", "/usr/local/bin/"}) if err != nil { return errors.Wrapf(err, "moving terraform binary into /usr/local/bin") } @@ -155,17 +150,16 @@ tunnels: return errors.Wrap(err, "writing ngrok config file") } - ngrokCmd, err := executeCmd("/tmp/ngrok", []string{"start", "atlantis", "--config", ngrokConfigFile.Name()}) - if err != nil { - return errors.Wrapf(err, "creating ngrok tunnel") + cancelNgrok, ngrokErrors := executeBackgroundCmd("/tmp/ngrok", []string{"start", "atlantis", "--config", ngrokConfigFile.Name()}) + // Check if we got a fast error. Move on if we haven't (the command is still running). + select { + case err := <-ngrokErrors: + return errors.Wrap(err, "creating ngrok tunnel") + default: } - ngrokErrChan := make(chan error, 10) - go func() { - ngrokErrChan <- ngrokCmd.Wait() - }() // When this function returns, ngrok tunnel should be stopped. - defer ngrokCmd.Process.Kill() + defer cancelNgrok() // Wait for the tunnel to be up. time.Sleep(2 * time.Second) @@ -180,17 +174,17 @@ tunnels: // Start atlantis server. colorstring.Println("[white]=> starting atlantis server") s.Start() - atlantisCmd, err := executeCmd(os.Args[0], []string{"server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)}) - if err != nil { - return errors.Wrapf(err, "creating atlantis server") - } + cancelAtlantis, atlantisErrors := executeBackgroundCmd(os.Args[0], []string{"server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)}) - atlantisErrChan := make(chan error, 10) - go func() { - atlantisErrChan <- atlantisCmd.Wait() - }() + // Check if we got a fast error. Move on if we haven't (the command is still running). + select { + case err := <-atlantisErrors: + return errors.Wrap(err, "creating atlantis server") + default: + } // When this function returns atlantis server should be stopped. - defer atlantisCmd.Process.Kill() + defer cancelAtlantis() + colorstring.Printf("[green]=> atlantis server is now securely exposed at [bold][underline]%s\n", tunnelURL) fmt.Println("") @@ -218,7 +212,7 @@ tunnels: colorstring.Println("[white]=> opening pull request") s.Start() time.Sleep(2 * time.Second) - _, err = executeCmd("open", []string{pullRequestURL}) + err = executeCmd("open", []string{pullRequestURL}) if err != nil { colorstring.Printf("[red]=> opening pull request failed. please go to: %s on the browser\n", pullRequestURL) } @@ -233,8 +227,16 @@ tunnels: // bootstrap process and want's to stop. signalChan := make(chan os.Signal, 1) signal.Notify(signalChan, syscall.SIGINT, syscall.SIGTERM) - <-signalChan - colorstring.Println("\n[red]shutdown signal received, exiting....") - colorstring.Println("\n[green]Thank you for using atlantis :) \n[white]For more information about how to use atlantis in production go to: https://github.com/runatlantis/atlantis") - return nil + + // Keep checking for errors from ngrok or atlantis server. Exit normally on shutdown signal. + select { + case <-signalChan: + colorstring.Println("\n[red]shutdown signal received, exiting....") + colorstring.Println("\n[green]Thank you for using atlantis :) \n[white]For more information about how to use atlantis in production go to: https://github.com/runatlantis/atlantis") + return nil + case err := <-ngrokErrors: + return errors.Wrap(err, "ngrok tunnel") + case err := <-atlantisErrors: + return errors.Wrap(err, "atlantis server") + } } diff --git a/bootstrap/utils.go b/bootstrap/utils.go index e9bc42792a..232756b0a5 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -15,6 +15,7 @@ package bootstrap import ( "archive/zip" + "context" "encoding/json" "fmt" "io" @@ -134,11 +135,23 @@ func downloadAndUnzip(url string, path string, target string) error { return unzip(path, target) } -func executeCmd(cmd string, args []string) (*exec.Cmd, error) { - command := exec.Command(cmd, args...) // #nosec - err := command.Start() - if err != nil { - return nil, err - } - return command, nil +// Executes a command, waits for it to finish and returns any errors. +func executeCmd(cmd string, args []string) error { + command := exec.Command(cmd, args...) + return command.Run() +} + +// Executes a command in the background. Returns a context so that the caller may cancel the +// command prematurely if necessary, as well as an errors channel. +func executeBackgroundCmd(cmd string, args []string) (context.CancelFunc, <-chan error) { + ctx, cancel := context.WithCancel(context.Background()) + command := exec.CommandContext(ctx, cmd, args...) // #nosec + + errChan := make(chan error, 1) + go func() { + err := command.Run() + errChan <- err + }() + + return cancel, errChan } From b8998fc35c622552178b5b8449ed5fbd3d2a27c3 Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Wed, 23 May 2018 22:51:44 +0200 Subject: [PATCH 4/9] Fix error re-declaration --- bootstrap/bootstrap.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index f3628aa94e..90249dcaa2 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -153,7 +153,7 @@ tunnels: cancelNgrok, ngrokErrors := executeBackgroundCmd("/tmp/ngrok", []string{"start", "atlantis", "--config", ngrokConfigFile.Name()}) // Check if we got a fast error. Move on if we haven't (the command is still running). select { - case err := <-ngrokErrors: + case err = <-ngrokErrors: return errors.Wrap(err, "creating ngrok tunnel") default: } @@ -178,7 +178,7 @@ tunnels: // Check if we got a fast error. Move on if we haven't (the command is still running). select { - case err := <-atlantisErrors: + case err = <-atlantisErrors: return errors.Wrap(err, "creating atlantis server") default: } From e80120ebf4a8f29131b6bbf2b298b4e69ea39735 Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Wed, 23 May 2018 22:55:11 +0200 Subject: [PATCH 5/9] Mark exec.Command with nosec --- bootstrap/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bootstrap/utils.go b/bootstrap/utils.go index 232756b0a5..4a405bbf9a 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -137,7 +137,7 @@ func downloadAndUnzip(url string, path string, target string) error { // Executes a command, waits for it to finish and returns any errors. func executeCmd(cmd string, args []string) error { - command := exec.Command(cmd, args...) + command := exec.Command(cmd, args...) // #nosec return command.Run() } From a2de603379cee0fae47507caddc8cae1a8c95faf Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Fri, 25 May 2018 00:01:31 +0200 Subject: [PATCH 6/9] Begin doc comments with function's name --- bootstrap/utils.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bootstrap/utils.go b/bootstrap/utils.go index 4a405bbf9a..747976f9ab 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -135,14 +135,14 @@ func downloadAndUnzip(url string, path string, target string) error { return unzip(path, target) } -// Executes a command, waits for it to finish and returns any errors. +// executeCmd executes a command, waits for it to finish and returns any errors. func executeCmd(cmd string, args []string) error { command := exec.Command(cmd, args...) // #nosec return command.Run() } -// Executes a command in the background. Returns a context so that the caller may cancel the -// command prematurely if necessary, as well as an errors channel. +// executeBackgroundCmd executes a command in the background. The function returns a context so +// that the caller may cancel the command prematurely if necessary, as well as an errors channel. func executeBackgroundCmd(cmd string, args []string) (context.CancelFunc, <-chan error) { ctx, cancel := context.WithCancel(context.Background()) command := exec.CommandContext(ctx, cmd, args...) // #nosec From daf0270480b47c82cd402672775e334400b9b8fa Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Fri, 25 May 2018 18:13:50 +0200 Subject: [PATCH 7/9] Include command output in error --- bootstrap/utils.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bootstrap/utils.go b/bootstrap/utils.go index 747976f9ab..1cf468366f 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -138,7 +138,11 @@ func downloadAndUnzip(url string, path string, target string) error { // executeCmd executes a command, waits for it to finish and returns any errors. func executeCmd(cmd string, args []string) error { command := exec.Command(cmd, args...) // #nosec - return command.Run() + bytes, err := command.CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %s", err, bytes) + } + return nil } // executeBackgroundCmd executes a command in the background. The function returns a context so From 7bb8a5b1e302f886c37522a2ef98aba3961db11a Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Sat, 26 May 2018 15:06:58 +0200 Subject: [PATCH 8/9] Terminate child processes properly - Wait for cancel functions to complete before main function returns. - Make executeCmd and executeBackgroundCmd variadic. --- bootstrap/bootstrap.go | 13 +++++++++---- bootstrap/utils.go | 7 +++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 90249dcaa2..b546445e95 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -24,6 +24,7 @@ import ( "os/signal" "runtime" "strings" + "sync" "syscall" "time" @@ -105,7 +106,7 @@ Follow these instructions to create a token (we don't store any tokens): colorstring.Println("[green]=> downloaded terraform successfully!") s.Stop() - err = executeCmd("mv", []string{"/tmp/terraform", "/usr/local/bin/"}) + err = executeCmd("mv", "/tmp/terraform", "/usr/local/bin/") if err != nil { return errors.Wrapf(err, "moving terraform binary into /usr/local/bin") } @@ -150,7 +151,11 @@ tunnels: return errors.Wrap(err, "writing ngrok config file") } - cancelNgrok, ngrokErrors := executeBackgroundCmd("/tmp/ngrok", []string{"start", "atlantis", "--config", ngrokConfigFile.Name()}) + // Used to ensure proper termination of all background commands. + var wg sync.WaitGroup + defer wg.Wait() + + cancelNgrok, ngrokErrors := executeBackgroundCmd(&wg, "/tmp/ngrok", "start", "atlantis", "--config", ngrokConfigFile.Name()) // Check if we got a fast error. Move on if we haven't (the command is still running). select { case err = <-ngrokErrors: @@ -174,7 +179,7 @@ tunnels: // Start atlantis server. colorstring.Println("[white]=> starting atlantis server") s.Start() - cancelAtlantis, atlantisErrors := executeBackgroundCmd(os.Args[0], []string{"server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)}) + cancelAtlantis, atlantisErrors := executeBackgroundCmd(&wg, os.Args[0], "server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)) // Check if we got a fast error. Move on if we haven't (the command is still running). select { @@ -212,7 +217,7 @@ tunnels: colorstring.Println("[white]=> opening pull request") s.Start() time.Sleep(2 * time.Second) - err = executeCmd("open", []string{pullRequestURL}) + err = executeCmd("open", pullRequestURL) if err != nil { colorstring.Printf("[red]=> opening pull request failed. please go to: %s on the browser\n", pullRequestURL) } diff --git a/bootstrap/utils.go b/bootstrap/utils.go index 1cf468366f..fcb86cfebb 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -23,6 +23,7 @@ import ( "os" "os/exec" "path/filepath" + "sync" "syscall" "github.com/pkg/errors" @@ -136,7 +137,7 @@ func downloadAndUnzip(url string, path string, target string) error { } // executeCmd executes a command, waits for it to finish and returns any errors. -func executeCmd(cmd string, args []string) error { +func executeCmd(cmd string, args ...string) error { command := exec.Command(cmd, args...) // #nosec bytes, err := command.CombinedOutput() if err != nil { @@ -147,12 +148,14 @@ func executeCmd(cmd string, args []string) error { // executeBackgroundCmd executes a command in the background. The function returns a context so // that the caller may cancel the command prematurely if necessary, as well as an errors channel. -func executeBackgroundCmd(cmd string, args []string) (context.CancelFunc, <-chan error) { +func executeBackgroundCmd(wg *sync.WaitGroup, cmd string, args ...string) (context.CancelFunc, <-chan error) { ctx, cancel := context.WithCancel(context.Background()) command := exec.CommandContext(ctx, cmd, args...) // #nosec errChan := make(chan error, 1) + wg.Add(1) go func() { + defer wg.Done() err := command.Run() errChan <- err }() From 27f9b3a112223a8d51df4cbd4e715fd67b85e3df Mon Sep 17 00:00:00 2001 From: Johanan Liebermann Date: Sat, 26 May 2018 16:18:19 +0200 Subject: [PATCH 9/9] Handle fast errors in background tasks --- bootstrap/bootstrap.go | 14 ++++---------- bootstrap/utils.go | 19 ++++++++++++++----- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index b546445e95..f2e0ad1443 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -155,14 +155,11 @@ tunnels: var wg sync.WaitGroup defer wg.Wait() - cancelNgrok, ngrokErrors := executeBackgroundCmd(&wg, "/tmp/ngrok", "start", "atlantis", "--config", ngrokConfigFile.Name()) + cancelNgrok, ngrokErrors, err := executeBackgroundCmd(&wg, "/tmp/ngrok", "start", "atlantis", "--config", ngrokConfigFile.Name()) // Check if we got a fast error. Move on if we haven't (the command is still running). - select { - case err = <-ngrokErrors: + if err != nil { return errors.Wrap(err, "creating ngrok tunnel") - default: } - // When this function returns, ngrok tunnel should be stopped. defer cancelNgrok() @@ -179,13 +176,10 @@ tunnels: // Start atlantis server. colorstring.Println("[white]=> starting atlantis server") s.Start() - cancelAtlantis, atlantisErrors := executeBackgroundCmd(&wg, os.Args[0], "server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)) - + cancelAtlantis, atlantisErrors, err := executeBackgroundCmd(&wg, os.Args[0], "server", "--gh-user", githubUsername, "--gh-token", githubToken, "--data-dir", "/tmp/atlantis/data", "--atlantis-url", tunnelURL, "--repo-whitelist", fmt.Sprintf("github.com/%s/%s", githubUsername, terraformExampleRepo)) // Check if we got a fast error. Move on if we haven't (the command is still running). - select { - case err = <-atlantisErrors: + if err != nil { return errors.Wrap(err, "creating atlantis server") - default: } // When this function returns atlantis server should be stopped. defer cancelAtlantis() diff --git a/bootstrap/utils.go b/bootstrap/utils.go index fcb86cfebb..e227cac376 100644 --- a/bootstrap/utils.go +++ b/bootstrap/utils.go @@ -146,19 +146,28 @@ func executeCmd(cmd string, args ...string) error { return nil } -// executeBackgroundCmd executes a command in the background. The function returns a context so -// that the caller may cancel the command prematurely if necessary, as well as an errors channel. -func executeBackgroundCmd(wg *sync.WaitGroup, cmd string, args ...string) (context.CancelFunc, <-chan error) { +// executeBackgroundCmd executes a long-running command in the background. The function returns a +// context so that the caller may cancel the command prematurely if necessary, as well as an errors +// channel. +// +// The function returns an error if the command could not start successfully. +func executeBackgroundCmd(wg *sync.WaitGroup, cmd string, args ...string) (context.CancelFunc, <-chan error, error) { ctx, cancel := context.WithCancel(context.Background()) command := exec.CommandContext(ctx, cmd, args...) // #nosec errChan := make(chan error, 1) + + err := command.Start() + if err != nil { + return cancel, errChan, fmt.Errorf("starting command: %v", err) + } + wg.Add(1) go func() { defer wg.Done() - err := command.Run() + err := command.Wait() errChan <- err }() - return cancel, errChan + return cancel, errChan, nil }