From 6743055201bb93258bb7add907f25935f978c726 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 17:04:55 +1000 Subject: [PATCH 1/8] Don't return the current process' cwd as the hook's after wd --- hook/scriptwrapper.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index 15b6b0ca7c..e61fe54a19 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -35,7 +35,6 @@ type ScriptWrapper struct { scriptFile *os.File beforeEnvFile *os.File afterEnvFile *os.File - beforeWd string } type HookScriptChanges struct { @@ -97,11 +96,6 @@ func CreateScriptWrapper(hookPath string) (*ScriptWrapper, error) { return nil, fmt.Errorf("Failed to find absolute path to \"%s\" (%s)", wrap.hookPath, err) } - wrap.beforeWd, err = os.Getwd() - if err != nil { - return nil, err - } - // Create the hook runner code var script string if isWindows && !isBashHook && !isPwshHook { @@ -182,7 +176,7 @@ func (wrap *ScriptWrapper) Changes() (HookScriptChanges, error) { diff := afterEnv.Diff(beforeEnv) - wd := wrap.getAfterWd(diff) + wd, _ := wrap.getAfterWd(diff) diff.Remove(hookExitStatusEnv) diff.Remove(hookWorkingDirEnv) @@ -193,16 +187,16 @@ func (wrap *ScriptWrapper) Changes() (HookScriptChanges, error) { return HookScriptChanges{Diff: diff, Dir: wd}, nil } -func (wrap *ScriptWrapper) getAfterWd(diff env.Diff) string { +func (wrap *ScriptWrapper) getAfterWd(diff env.Diff) (string, err) { addedVar, ok := diff.Added[hookWorkingDirEnv] if ok { - return addedVar + return addedVar, nil } changedVar, ok := diff.Changed[hookWorkingDirEnv] if ok { - return changedVar.New + return changedVar.New, nil } - return wrap.beforeWd + return "", fmt.Errorf("%q was not present in the hook after environment", hookWorkingDirEnv) } From 6779abc29b3054641e84fe796649fbc73dbe3f71 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 17:30:13 +1000 Subject: [PATCH 2/8] Make a genuine and then handled error out of the early exit case --- bootstrap/bootstrap.go | 25 +++++++++++++++++----- hook/scriptwrapper.go | 47 +++++++++++++++++++++--------------------- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index c3e4599368..a9f754e3cd 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -411,17 +411,32 @@ func (b *Bootstrap) executeHook(ctx context.Context, scope string, name string, // Get changed environment changes, err := script.Changes() if err != nil { - return errors.Wrapf(err, "Failed to get environment") + // Could not compute the changes in environment or working directory + // for some reason... + + switch err.(type) { + case *hook.HookExitError: + // ...because the hook called exit(), tsk we ignore any changes + // since we can't discern them but continue on with the job + break; + default: + // ...because something else happened, report it and stop the job + return errors.Wrapf(err, "Failed to get environment") + } + } else { + // Hook exited successfully (and not early!) We have an environment and + // wd change we can apply to our subsequent phases + b.applyEnvironmentChanges(changes, redactors) } - // Finally, apply changes to the current shell and config - b.applyEnvironmentChanges(changes, redactors) return nil } func (b *Bootstrap) applyEnvironmentChanges(changes hook.HookScriptChanges, redactors RedactorMux) { - if changes.Dir != b.shell.Getwd() { - _ = b.shell.Chdir(changes.Dir) + if afterWd, err := changes.GetAfterWd(); err == nil { + if afterWd != b.shell.Getwd() { + _ = b.shell.Chdir(afterWd) + } } // Do we even have any environment variables to change? diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index e61fe54a19..385b94570f 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -39,7 +39,26 @@ type ScriptWrapper struct { type HookScriptChanges struct { Diff env.Diff - Dir string +} + +func (changes *HookScriptChanges) GetAfterWd() (string, error) { + if addedVar, ok := changes.Diff.Added[hookWorkingDirEnv]; ok { + return addedVar, nil + } + + if changedVar, ok := changes.Diff.Changed[hookWorkingDirEnv]; ok { + return changedVar.New, nil + } + + return "", fmt.Errorf("%q was not present in the hook after environment", hookWorkingDirEnv) +} + +type HookExitError struct { + hookPath string +} + +func (e *HookExitError) Error() string { + return fmt.Sprintf("Hook %q early exited, could not record after environment or working directory") } // CreateScriptWrapper creates and configures a ScriptWrapper. @@ -164,39 +183,19 @@ func (wrap *ScriptWrapper) Changes() (HookScriptChanges, error) { } beforeEnv := env.FromExport(string(beforeEnvContents)) - afterEnv := env.FromExport(string(afterEnvContents)) + if afterEnv.Length() == 0 { - // If the after env is completely empty it is likely because the hook - // ran exit(). Instead of falling over and breaking any subsequent - // commands, lets fall back to the original before env since we can’t - // extract a meaningful diff. - afterEnv = beforeEnv + return HookScriptChanges{}, &HookExitError{hookPath: wrap.hookPath} } diff := afterEnv.Diff(beforeEnv) - wd, _ := wrap.getAfterWd(diff) - diff.Remove(hookExitStatusEnv) diff.Remove(hookWorkingDirEnv) // Bash sets this, but we don't care about it diff.Remove("_") - return HookScriptChanges{Diff: diff, Dir: wd}, nil -} - -func (wrap *ScriptWrapper) getAfterWd(diff env.Diff) (string, err) { - addedVar, ok := diff.Added[hookWorkingDirEnv] - if ok { - return addedVar, nil - } - - changedVar, ok := diff.Changed[hookWorkingDirEnv] - if ok { - return changedVar.New, nil - } - - return "", fmt.Errorf("%q was not present in the hook after environment", hookWorkingDirEnv) + return HookScriptChanges{Diff: diff}, nil } From a18cf8a77c04ddc24b0209f2d6a26f8d797bbe81 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 17:30:25 +1000 Subject: [PATCH 3/8] Fix tests --- hook/scriptwrapper_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hook/scriptwrapper_test.go b/hook/scriptwrapper_test.go index 4ae65a1174..b182750113 100644 --- a/hook/scriptwrapper_test.go +++ b/hook/scriptwrapper_test.go @@ -120,7 +120,12 @@ func TestRunningHookDetectsChangedWorkingDirectory(t *testing.T) { t.Fatal(err) } - changesDir, err := filepath.EvalSymlinks(changes.Dir) + afterWd, err := changes.GetAfterWd() + if err != nil { + t.Fatal(err) + } + + changesDir, err := filepath.EvalSymlinks(afterWd) if err != nil { t.Fatal(err) } From f7d198504a3ccccd89669c20fdce43db88f0aec8 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 17:55:04 +1000 Subject: [PATCH 4/8] Fix missing fmt arg --- hook/scriptwrapper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index 385b94570f..ad2030f991 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -58,7 +58,7 @@ type HookExitError struct { } func (e *HookExitError) Error() string { - return fmt.Sprintf("Hook %q early exited, could not record after environment or working directory") + return fmt.Sprintf("Hook %q early exited, could not record after environment or working directory", e.hookPath) } // CreateScriptWrapper creates and configures a ScriptWrapper. From 006bd6ee8a2aa04aefadab4d974d822d5b97d2c4 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 17:57:36 +1000 Subject: [PATCH 5/8] Fix test panics --- bootstrap/integration/hooks_integration_test.go | 10 ++++++---- bootstrap/integration/plugin_integration_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index df5d1f0989..9adca49a35 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -58,8 +58,9 @@ func TestEnvironmentVariablesPassBetweenHooks(t *testing.T) { if err := bintest.ExpectEnv(t, c.Env, `MY_CUSTOM_ENV=1`, `LLAMAS_ROCK=absolutely`); err != nil { fmt.Fprintf(c.Stderr, "%v\n", err) c.Exit(1) + } else { + c.Exit(0) } - c.Exit(0) }) tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") @@ -93,8 +94,9 @@ func TestDirectoryPassesBetweenHooks(t *testing.T) { if c.GetEnv("MY_CUSTOM_SUBDIR") != c.Dir { fmt.Fprintf(c.Stderr, "Expected current dir to be %q, got %q\n", c.GetEnv("MY_CUSTOM_SUBDIR"), c.Dir) c.Exit(1) + } else { + c.Exit(0) } - c.Exit(0) }) tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") @@ -145,9 +147,9 @@ func TestReplacingCheckoutHook(t *testing.T) { fmt.Fprint(c.Stderr, out) if err != nil { c.Exit(1) - return + } else { + c.Exit(0) } - c.Exit(0) }) tester.ExpectGlobalHook("pre-checkout").Once() diff --git a/bootstrap/integration/plugin_integration_test.go b/bootstrap/integration/plugin_integration_test.go index 649469b030..bf88bb4dd4 100644 --- a/bootstrap/integration/plugin_integration_test.go +++ b/bootstrap/integration/plugin_integration_test.go @@ -59,16 +59,18 @@ func TestRunningPlugins(t *testing.T) { if err := bintest.ExpectEnv(t, c.Env, `MY_CUSTOM_ENV=1`, `LLAMAS_ROCK=absolutely`); err != nil { fmt.Fprintf(c.Stderr, "%v\n", err) c.Exit(1) + } else { + c.Exit(0) } - c.Exit(0) }) tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { if err := bintest.ExpectEnv(t, c.Env, `MY_CUSTOM_ENV=1`, `LLAMAS_ROCK=absolutely`); err != nil { fmt.Fprintf(c.Stderr, "%v\n", err) c.Exit(1) + } else { + c.Exit(0) } - c.Exit(0) }) tester.RunAndCheck(t, env...) From 7469f9b201ef37fd5b55722024232631838d0c8c Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 18:10:46 +1000 Subject: [PATCH 6/8] Pluck the afterWd value from the map before removing it --- hook/scriptwrapper.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index ad2030f991..7ef923d77d 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -39,18 +39,15 @@ type ScriptWrapper struct { type HookScriptChanges struct { Diff env.Diff + afterWd string } func (changes *HookScriptChanges) GetAfterWd() (string, error) { - if addedVar, ok := changes.Diff.Added[hookWorkingDirEnv]; ok { - return addedVar, nil + if changes.afterWd == "" { + return "", fmt.Errorf("%q was not present in the hook after environment", hookWorkingDirEnv) } - if changedVar, ok := changes.Diff.Changed[hookWorkingDirEnv]; ok { - return changedVar.New, nil - } - - return "", fmt.Errorf("%q was not present in the hook after environment", hookWorkingDirEnv) + return changes.afterWd, nil } type HookExitError struct { @@ -191,11 +188,22 @@ func (wrap *ScriptWrapper) Changes() (HookScriptChanges, error) { diff := afterEnv.Diff(beforeEnv) + // Pluck the after wd from the diff before removing the key from the diff + afterWd := "" + if afterWd == "" { + afterWd, _ = diff.Added[hookWorkingDirEnv] + } + if afterWd == "" { + if change, ok := diff.Changed[hookWorkingDirEnv]; ok { + afterWd = change.New + } + } + diff.Remove(hookExitStatusEnv) diff.Remove(hookWorkingDirEnv) // Bash sets this, but we don't care about it diff.Remove("_") - return HookScriptChanges{Diff: diff}, nil + return HookScriptChanges{Diff: diff, afterWd: afterWd}, nil } From 4c0272b8d1ad8800a9a61439d68682797013a2fd Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 18:25:03 +1000 Subject: [PATCH 7/8] Add test that hook early exit dir changes are ignored Mostly this tests that when the hook early exits we don't munge the left over data from it into a completely different dir change --- .../integration/hooks_integration_test.go | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index 9adca49a35..a9f6735f40 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -102,6 +102,43 @@ func TestDirectoryPassesBetweenHooks(t *testing.T) { tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") } +func TestDirectoryPassesBetweenHooksIgnoredUnderExit(t *testing.T) { + t.Parallel() + + tester, err := NewBootstrapTester() + if err != nil { + t.Fatal(err) + } + defer tester.Close() + + if runtime.GOOS == "windows" { + t.Skip("Not implemented for windows yet") + } + + var script = []string{ + "#!/bin/bash", + "mkdir -p ./mysubdir", + "export MY_CUSTOM_SUBDIR=$(cd mysubdir; pwd)", + "cd ./mysubdir", + "exit 0", + } + + if err := ioutil.WriteFile(filepath.Join(tester.HooksDir, "pre-command"), []byte(strings.Join(script, "\n")), 0700); err != nil { + t.Fatal(err) + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if c.GetEnv("BUILDKITE_BUILD_CHECKOUT_PATH") != c.Dir { + fmt.Fprintf(c.Stderr, "Expected current dir to be %q, got %q\n", c.GetEnv("BUILDKITE_BUILD_CHECKOUT_PATH"), c.Dir) + c.Exit(1) + } else { + c.Exit(0) + } + }) + + tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") +} + func TestCheckingOutFiresCorrectHooks(t *testing.T) { t.Parallel() From ae7cd041ac6f38dfd64464ad8481a37ad8a40c20 Mon Sep 17 00:00:00 2001 From: Keith Duncan Date: Wed, 29 Sep 2021 18:37:28 +1000 Subject: [PATCH 8/8] Add unset environment variables test while we're here --- .../integration/hooks_integration_test.go | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index a9f6735f40..3b8559f21e 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -66,6 +66,74 @@ func TestEnvironmentVariablesPassBetweenHooks(t *testing.T) { tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") } +func TestHooksCanUnsetEnvironmentVariables(t *testing.T) { + t.Parallel() + + tester, err := NewBootstrapTester() + if err != nil { + t.Fatal(err) + } + defer tester.Close() + + if runtime.GOOS == "windows" { + var preCommand = []string{ + "@echo off", + "set LLAMAS_ROCK=absolutely", + } + if err := ioutil.WriteFile(filepath.Join(tester.HooksDir, "pre-command.bat"), + []byte(strings.Join(preCommand, "\r\n")), 0700); err != nil { + t.Fatal(err) + } + + var postCommand = []string{ + "@echo off", + "set LLAMAS_ROCK=", + } + if err := ioutil.WriteFile(filepath.Join(tester.HooksDir, "post-command.bat"), + []byte(strings.Join(postCommand, "\n")), 0700); err != nil { + t.Fatal(err) + } + } else { + var preCommand = []string{ + "#!/bin/bash", + "export LLAMAS_ROCK=absolutely", + } + if err := ioutil.WriteFile(filepath.Join(tester.HooksDir, "pre-command"), + []byte(strings.Join(preCommand, "\n")), 0700); err != nil { + t.Fatal(err) + } + + var postCommand = []string{ + "#!/bin/bash", + "unset LLAMAS_ROCK", + } + if err := ioutil.WriteFile(filepath.Join(tester.HooksDir, "post-command"), + []byte(strings.Join(postCommand, "\n")), 0700); err != nil { + t.Fatal(err) + } + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if c.GetEnv("LLAMAS_ROCK") != "absolutely" { + fmt.Fprintf(c.Stderr, "Expected command hook to have environment variable LLAMAS_ROCK be %q, got %q\n", "absolutely", c.GetEnv("LLAMAS_ROCK")) + c.Exit(1) + } else { + c.Exit(0) + } + }) + + tester.ExpectGlobalHook("pre-exit").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if c.GetEnv("LLAMAS_ROCK") != "" { + fmt.Fprintf(c.Stderr, "Expected pre-exit hook to have environment variable LLAMAS_ROCK be empty, got %q\n", c.GetEnv("LLAMAS_ROCK")) + c.Exit(1) + } else { + c.Exit(0) + } + }) + + tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") +} + func TestDirectoryPassesBetweenHooks(t *testing.T) { t.Parallel()