Merge pull request #2040 from buildkite/polyglot-hooks

Experiment: Polyglot hooks

.buildkite/Dockerfile-compile

@@ -1,3 +1,6 @@
 FROM golang:1.20.4@sha256:6876eff5b20336c5c2896b0c3055f3258bdeba7aa38bbdabcb5a4abb5cdd39c7
 COPY build/ssh.conf /etc/ssh/ssh_config.d/
 RUN go install github.com/google/go-licenses@latest
+
+# Ruby used for polyglot hook integration tests
+RUN apt update && apt install -y ruby

bootstrap/bootstrap.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"runtime"
 	"strconv"
 	"strings"
 	"time"
@@ -297,11 +298,74 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error {
 
 	b.shell.Headerf("Running %s hook", hookName)
 
+	if !experiments.IsEnabled(experiments.PolyglotHooks) {
+		return b.runWrappedShellScriptHook(ctx, hookName, hookCfg)
+	}
+
+	hookType, err := hook.Type(hookCfg.Path)
+	if err != nil {
+		return fmt.Errorf("determining hook type for %q hook: %w", hookName, err)
+	}
+
+	switch hookType {
+	case hook.TypeScript:
+		if runtime.GOOS == "windows" {
+			// We use shebangs to figure out how to run scripts, and Windows has no way to interpret a shebang
+			// ie, on linux, we can just point the OS to a file of some sort and say "run that", and as part of that it will try to
+			// read a shebang, and run the script using the interpreter specified. Windows can't do this, so we can't run scripts
+			// directly on Windows
+
+			// Potentially there's something that we could do with file extensions, but that's a bit of a minefield, and would
+			// probably mean that we have to have a list of blessed hook runtimes on windows only... or something.
+
+			// Regardless: not supported right now, or potentially ever.
+			sheb, _ := shellscript.ShebangLine(hookCfg.Path) // we know this won't error because it must have a shebang to be a script
+
+			err := fmt.Errorf(`when trying to run the hook at %q, the agent found that it was a script with a shebang that isn't for a shellscripting language - in this case, %q.
+Hooks of this kind are unfortunately not supported on Windows, as we have no way of interpreting a shebang on Windows`, hookCfg.Path, sheb)
+			return err
+		}
+
+		// It's a script, and we can rely on the OS to figure out how to run it (because we're not on windows), so run it
+		// directly without wrapping
+		if err := b.runUnwrappedHook(ctx, hookName, hookCfg); err != nil {
+			return fmt.Errorf("running %q script hook: %w", hookName, err)
+		}
+
+		return nil
+	case hook.TypeBinary:
+		// It's a binary, so we'll just run it directly, no wrapping needed or possible
+		if err := b.runUnwrappedHook(ctx, hookName, hookCfg); err != nil {
+			return fmt.Errorf("running %q binary hook: %w", hookName, err)
+		}
+
+		return nil
+	case hook.TypeShell:
+		// It's definitely a shell script, wrap it so that we can snaffle the changed environment variables
+		if err := b.runWrappedShellScriptHook(ctx, hookName, hookCfg); err != nil {
+			return fmt.Errorf("running %q shell hook: %w", hookName, err)
+		}
+
+		return nil
+	default:
+		return fmt.Errorf("unknown hook type %q for %q hook", hookType, hookName)
+	}
+}
+
+func (b *Bootstrap) runUnwrappedHook(ctx context.Context, hookName string, hookCfg HookConfig) error {
+	environ := hookCfg.Env.Copy()
+
+	environ.Set("BUILDKITE_HOOK_PHASE", hookCfg.Name)
+	environ.Set("BUILDKITE_HOOK_PATH", hookCfg.Path)
+	environ.Set("BUILDKITE_HOOK_TRIGGERED_FROM", hookCfg.Scope)
+
+	return b.shell.RunWithEnv(ctx, environ, hookCfg.Path)
+}
+
+func (b *Bootstrap) runWrappedShellScriptHook(ctx context.Context, hookName string, hookCfg HookConfig) error {
 	redactors := b.setupRedactors()
 	defer redactors.Flush()
 
-	// We need a script to wrap the hook script so that we can snaffle the changed
-	// environment variables
 	script, err := hook.NewScriptWrapper(hook.WithHookPath(hookCfg.Path))
 	if err != nil {
 		b.shell.Errorf("Error creating hook script: %v", err)

bootstrap/integration/checkout_integration_test.go

@@ -26,21 +26,9 @@ var commitPattern = bintest.MatchPattern(`(?ms)\Acommit [0-9a-f]+\nabbrev-commit
 // We expect this arg multiple times, just define it once.
 var gitShowFormatArg = "--format=commit %H%nabbrev-commit %h%nAuthor: %an <%ae>%n%n%w(0,4,4)%B"
 
-// Enable an experiment, returning a function to restore the previous state.
-// Usage: defer experimentWithUndo("foo")()
-func experimentWithUndo(name string) func() {
-	prev := experiments.IsEnabled(name)
-	experiments.Enable(name)
-	return func() {
-		if !prev {
-			experiments.Disable(name)
-		}
-	}
-}
-
 func TestCheckingOutGitHubPullRequestsWithGitMirrorsExperiment(t *testing.T) {
 	// t.Parallel() cannot be used with experiments.Enable()
-	defer experimentWithUndo(experiments.GitMirrors)()
+	defer experiments.EnableWithUndo(experiments.GitMirrors)()
 
 	tester, err := NewBootstrapTester()
 	if err != nil {
@@ -81,7 +69,7 @@ func TestCheckingOutGitHubPullRequestsWithGitMirrorsExperiment(t *testing.T) {
 
 func TestWithResolvingCommitExperiment(t *testing.T) {
 	// t.Parallel() cannot be used with experiments.Enable()
-	defer experimentWithUndo(experiments.ResolveCommitAfterCheckout)()
+	defer experiments.EnableWithUndo(experiments.ResolveCommitAfterCheckout)()
 
 	tester, err := NewBootstrapTester()
 	if err != nil {
@@ -691,7 +679,7 @@ func TestRepositorylessCheckout(t *testing.T) {
 
 func TestGitMirrorEnv(t *testing.T) {
 	// t.Parallel() cannot test experiment flags in parallel
-	defer experimentWithUndo(experiments.GitMirrors)()
+	defer experiments.EnableWithUndo(experiments.GitMirrors)()
 
 	tester, err := NewBootstrapTester()
 	if err != nil {

bootstrap/integration/hooks_integration_test.go

@@ -3,6 +3,7 @@ package integration
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -11,6 +12,7 @@ import (
 	"time"
 
 	"github.com/buildkite/agent/v3/bootstrap/shell"
+	"github.com/buildkite/agent/v3/experiments"
 	"github.com/buildkite/bintest/v3"
 )
 
@@ -488,3 +490,85 @@ func TestPreExitHooksFireAfterCancel(t *testing.T) {
 
 	tester.CheckMocks(t)
 }
+
+func TestPolyglotScriptHooksCanBeRun(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("script hooks aren't supported on windows")
+	}
+
+	path, err := exec.LookPath("ruby")
+	if err != nil {
+		t.Fatalf("error finding path to ruby executable: %v", err)
+	}
+
+	if path == "" {
+		t.Fatal("ruby not found in $PATH. This test requires ruby to be installed on the host")
+	}
+
+	defer experiments.EnableWithUndo(experiments.PolyglotHooks)()
+
+	tester, err := NewBootstrapTester()
+	if err != nil {
+		t.Fatalf("NewBootstrapTester() error = %v", err)
+	}
+	defer tester.Close()
+
+	filename := "environment"
+	script := []string{
+		"#!/usr/bin/env ruby",
+		`puts "ohai, it's ruby!"`,
+	}
+
+	if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0755); err != nil {
+		t.Fatalf("os.WriteFile(%q, script, 0755) = %v", filename, err)
+	}
+
+	tester.RunAndCheck(t)
+
+	if !strings.Contains(tester.Output, "ohai, it's ruby!") {
+		t.Fatalf("tester.Output %q does not contain expected output: %q", tester.Output, "ohai, it's ruby!")
+	}
+}
+
+func TestPolyglotBinaryHooksCanBeRun(t *testing.T) {
+	defer experiments.EnableWithUndo(experiments.PolyglotHooks)()
+	defer experiments.EnableWithUndo(experiments.JobAPI)()
+
+	tester, err := NewBootstrapTester()
+	if err != nil {
+		t.Fatalf("NewBootstrapTester() error = %v", err)
+	}
+	defer tester.Close()
+
+	// We build a binary as part of this test so that we can produce a binary hook
+	// Forgive me for my sins, RSC, but it's better than the alternatives.
+	// The code that we're building is in ./test-binary-hook/main.go, it's pretty straightforward.
+
+	fmt.Println("Building test-binary-hook")
+	hookPath := filepath.Join(tester.HooksDir, "environment")
+
+	if runtime.GOOS == "windows" {
+		hookPath += ".exe"
+	}
+
+	output, err := exec.Command("go", "build", "-o", hookPath, "./test-binary-hook").CombinedOutput()
+	if err != nil {
+		t.Fatalf("Failed to build test-binary-hook: %v, output: %s", err, string(output))
+	}
+
+	tester.ExpectGlobalHook("post-command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) {
+		// Set via ./test-binary-hook/main.go
+		if c.GetEnv("OCEAN") != "Pacífico" {
+			fmt.Fprintf(c.Stderr, "Expected OCEAN to be Pacífico, got %q", c.GetEnv("OCEAN"))
+			c.Exit(1)
+		} else {
+			c.Exit(0)
+		}
+	})
+
+	tester.RunAndCheck(t)
+
+	if !strings.Contains(tester.Output, "hi there from golang 🌊") {
+		t.Fatalf("tester.Output %s does not contain expected output: %q", tester.Output, "hi there from golang 🌊")
+	}
+}

bootstrap/integration/job_api_integration_test.go

@@ -16,7 +16,7 @@ import (
 )
 
 func TestBootstrapRunsJobAPI(t *testing.T) {
-	defer experimentWithUndo(experiments.JobAPI)()
+	defer experiments.EnableWithUndo(experiments.JobAPI)()
 
 	tester, err := NewBootstrapTester()
 	if err != nil {
@@ -87,7 +87,7 @@ func TestBootstrapRunsJobAPI(t *testing.T) {
 		}
 
 		mtn := "chimborazo"
-		b, err := json.Marshal(jobapi.EnvUpdateRequest{Env: map[string]*string{"MOUNTAIN": &mtn}})
+		b, err := json.Marshal(jobapi.EnvUpdateRequest{Env: map[string]string{"MOUNTAIN": mtn}})
 		if err != nil {
 			t.Errorf("marshaling env update request: %v", err)
 			c.Exit(1)

bootstrap/integration/test-binary-hook/main.go

@@ -0,0 +1,31 @@
+package main
+
+import (
+	"context"
+	"fmt"
+	"log"
+
+	"github.com/buildkite/agent/v3/jobapi"
+)
+
+// This file gets built and commited to this repo, then used as part of the hooks integration test to ensure that the
+// bootstrap can run binary hooks
+func main() {
+	c, err := jobapi.NewDefaultClient(context.TODO())
+	if err != nil {
+		log.Fatalf("error: %v", fmt.Errorf("creating job api client: %w", err))
+	}
+
+	_, err = c.EnvUpdate(context.TODO(), &jobapi.EnvUpdateRequest{
+		Env: map[string]string{
+			"OCEAN":    "Pacífico",
+			"MOUNTAIN": "chimborazo",
+		},
+	})
+
+	if err != nil {
+		log.Fatalf("error: %v", fmt.Errorf("updating env: %w", err))
+	}
+
+	fmt.Println("hi there from golang 🌊")
+}

bootstrap/shell/shell.go

@@ -282,6 +282,30 @@ func (s *Shell) Run(ctx context.Context, command string, arg ...string) error {
 	return s.RunWithoutPrompt(ctx, command, arg...)
 }
 
+func (s *Shell) RunWithEnv(ctx context.Context, environ *env.Environment, command string, arg ...string) error {
+	formatted := process.FormatCommand(command, arg)
+	if s.stdin == nil {
+		s.Promptf("%s", formatted)
+	} else {
+		// bash-syntax-compatible indication that input is coming from somewhere
+		s.Promptf("%s < /dev/stdin", formatted)
+	}
+
+	cmd, err := s.buildCommand(command, arg...)
+	if err != nil {
+		s.Errorf("Error building command: %v", err)
+		return err
+	}
+
+	cmd.Env = append(cmd.Env, environ.ToSlice()...)
+
+	return s.executeCommand(ctx, cmd, s.Writer, executeFlags{
+		Stdout: true,
+		Stderr: true,
+		PTY:    s.PTY,
+	})
+}
+
 // RunWithoutPrompt runs a command, writes stdout and err to the logger,
 // and returns an error if it fails. It doesn't show a prompt.
 func (s *Shell) RunWithoutPrompt(ctx context.Context, command string, arg ...string) error {
@@ -379,6 +403,8 @@ func (s *Shell) RunScript(ctx context.Context, path string, extra *env.Environme
 	case !isWindows && isSh:
 		// If the script contains a shebang line, it can be run directly,
 		// with the shebang line choosing the interpreter.
+		// note that this means that it isn't necessarily a shell script in this case!
+		// #!/usr/bin/env python would be totally valid, and would execute as a python script
 		sb, err := shellscript.ShebangLine(path)
 		if err == nil && sb != "" {
 			command = path
@@ -397,7 +423,7 @@ func (s *Shell) RunScript(ctx context.Context, path string, extra *env.Environme
 			s.Warningf("Couldn't find bash (%v). Attempting to fall back to sh. This may cause issues for hooks and plugins that assume Bash features.", err)
 			shPath, err = s.AbsolutePath("sh")
 			if err != nil {
-				return fmt.Errorf("Error finding a shell, needed to run scripts: %v.", err)
+				return fmt.Errorf("error finding a shell, needed to run scripts: %v", err)
 			}
 		}
 		command = shPath

clicommand/env_set.go

@@ -72,7 +72,7 @@ func envSetAction(c *cli.Context) error {
 	}
 
 	req := &jobapi.EnvUpdateRequest{
-		Env: make(map[string]*string),
+		Env: make(map[string]string),
 	}
 
 	var parse func(string) error
@@ -84,7 +84,7 @@ func envSetAction(c *cli.Context) error {
 			if !ok {
 				return fmt.Errorf("%q is not in key=value format", input)
 			}
-			req.Env[e] = &v
+			req.Env[e] = v
 			return nil
 		}
 

env/environment.go

@@ -197,6 +197,10 @@ func (e *Environment) Apply(diff Diff) {
 
 // Copy returns a copy of the env
 func (e *Environment) Copy() *Environment {
+	if e == nil {
+		return New()
+	}
+
 	c := New()
 
 	e.underlying.Range(func(k, v string) bool {

experiments/experiments.go

@@ -5,6 +5,7 @@
 package experiments
 
 const (
+	PolyglotHooks              = "polyglot-hooks"
 	JobAPI                     = "job-api"
 	KubernetesExec             = "kubernetes-exec"
 	ANSITimestamps             = "ansi-timestamps"
@@ -19,6 +20,7 @@ const (
 
 var (
 	Available = map[string]struct{}{
+		PolyglotHooks:              {},
 		JobAPI:                     {},
 		KubernetesExec:             {},
 		ANSITimestamps:             {},
@@ -34,6 +36,11 @@ var (
 	experiments = make(map[string]bool, len(Available))
 )
 
+func EnableWithUndo(key string) func() {
+	Enable(key)
+	return func() { Disable(key) }
+}
+
 // Enable a particular experiment in the agent.
 func Enable(key string) (known bool) {
 	experiments[key] = true