Experiment: Polyglot hooks #2040

moskyb · 2023-04-03T04:00:43Z

This PR: Removes the requirement that hooks for the buildkite-agent be written in a shell scripting language a la bash. With the use of this branch, hooks can also either be compiled executable binaries (ie those produced by languages like Golang or Rust), or any interpreted scripting language (ie python, ruby, lua), so long as an appropriate shebang is specified at the top of the file.

note that neither binary hooks nor non-shell script hooks have environment variable changes captured as bash script hooks do; they must use the Job API to modify the environment of the job.

moskyb · 2023-04-03T04:04:07Z

bootstrap/shell/shell.go

+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,
+	})
+}
+


this function is just the Run method and the RunWithPrompt methods garglemeshed together with an env-y thing going on in the middle. earlier on, i tried to make a nice refactor so that you could compose bits and bobs of the different Run* methods together, but it wasn't super important to make this change. i'd like to clean it up at some point though

moskyb · 2023-04-03T04:04:37Z

env/environment.go

+	if e == nil {
+		return New()
+	}
+


gotta catch 'em all ('em being nil pointer dereference panics)

I sort of expect a copy of nil to be nil, but it's probably more pragmatic for it to be New().

moskyb · 2023-04-03T04:06:51Z

hook/scriptwrapper.go

-	if shebang != "" && !shellscript.IsPOSIXShell(shebang) {
-		return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang)
-	}
+	// if shebang != "" && !shellscript.IsPOSIXShell(shebang) {
+	// 	return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang)
+	// }


this doesn't technically have to be commented out anymore - it's still an error if we try to scriptwrap a non-shell hook, but we've kind of lifted this logic out to here. Maybe it makes sense to keep this in as a defensive programming thing?

We can also defend ourselves with a test. I think that's more robust, so would prefer to delete the comments.

oh totally, if this gets out of WIP i'd either de-comment it or remove these lines entirely.

when you say we can defend ourselves with a test, do you mean adding a test that asserts that if we try to wrap a non-shell-script, it complains?

Yeah, exactly. I'd imagine, we would want to test a whole table of binaries and scripts and verify that they run. We should be able to tease out a test that fails when the scriptwrapper is run on the wrong type of binary from this.

You might have to think a bit about making an interface. I think that's a good thing to do, though.

yep, that sounds like a good idea. when we get to properly testing this, i'll do that.

Have we gotten to properly testing this?

very good call! when i said:

oh totally, if this gets out of WIP i'd either de-comment it or remove these lines entirely.

apparently i was lying :P

i'll bang out some tests for this now

moskyb · 2023-04-03T04:07:42Z

shellscript/shellscript.go

 )

+// Magic numbers for various types of executable files
+var (


i'd like to thank the peanut gallery in the #serious-engineering channel for helping me with all of this binary nonsense

shellscript/shellscript.go

bootstrap/bootstrap.go

pda · 2023-04-03T07:19:03Z

shellscript/shellscript.go

+	ELFMagic = []byte{0x7F, 0x45, 0x4C, 0x46}
+
+	// Windows uses MS-DOS MZ-style executables. This includes PE32 and PE32+ (which is 64-bit 🙃) binaries,
+	// which a are what modern windowses tend to use.


It hates the windowses, precious.

shellscript/shellscript.go

bootstrap/bootstrap.go

shellscript/shellscript.go

lox · 2023-04-07T23:16:27Z

Love this!

note that neither binary hooks nor non-shell script hooks have environment variable changes captured as bash script hooks do; they must use the Job API to modify the environment of the job.

Yeah, this is my only concern. I wonder if this will be an unexpected sharp edge. I guess that folks assume that bash scripts are simply sourced (vs the magic of wrapped hooks) and they won't expect that behaviour for non-shell scripts.

I assume the future is all Job API and no more magical environment capture?

moskyb · 2023-04-17T00:47:39Z

@lox

I assume the future is all Job API and no more magical environment capture?

in an "as the limit tends to infinity" sense, i think so. i doubt that we'd ever fully remove the environment functionality anytime in the forseeable future; it's just too useful and easy to set up.

i suspect that if we merged this, we'd try to make non-bash hooks the golden path, and try to build some nicer tooling around them, and encourage it that way, but keep env capture around as a simple mode? i'm not 100% sure, i may or may or may not have thought that far ahead 😅

DrJosh9000

Looking pretty good 🥞

experiments/experiments.go

hook/binary.go

bootstrap/integration/hooks_integration_test.go

hook/type_test.go

triarius

Thank you for your service @moskyb 🫡

I think my only blocking comment is for the scriptwrapper tests. It may no longer be necessary to write them, though because of some combination of other tests. If so, I think you should delete the commented code. If not, you should still delete it. We are all pretty good at git archaeology.

triarius · 2023-05-19T03:48:51Z

bootstrap/bootstrap.go

+			// 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.


💯% reasonable to cut this from scope.

experiments/experiments.go

triarius · 2023-05-19T04:03:34Z

env/environment.go

+	if e == nil {
+		return New()
+	}
+


I sort of expect a copy of nil to be nil, but it's probably more pragmatic for it to be New().

triarius · 2023-05-19T04:05:55Z

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

+		Env: map[string]string{
+			"OCEAN":    "Pacífico",
+			"MOUNTAIN": "chimborazo",
+		},


Good thinking to test the jobapi as well 🧠

triarius · 2023-05-19T04:06:58Z

bootstrap/shell/shell.go

+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,
+	})
+}
+


scripts/generate-test-binaries.sh

hook/type.go

triarius · 2023-05-19T04:28:19Z

hook/scriptwrapper.go

-	if shebang != "" && !shellscript.IsPOSIXShell(shebang) {
-		return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang)
-	}
+	// if shebang != "" && !shellscript.IsPOSIXShell(shebang) {
+	// 	return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang)
+	// }


Have we gotten to properly testing this?

The server needs to be able to handle JSON nulls as values, so it should use a map[string]*string, but the client library should just not allow anyone to send nulls, so it should use a map[string]string. This has the side-benefit of making the DX of actually using the library much better

…shell script

bootstrap/bootstrap.go

bootstrap/integration/hooks_integration_test.go

DrJosh9000

Awesome! Ship it 🚢

moskyb requested a review from a team April 3, 2023 04:00

moskyb commented Apr 3, 2023

View reviewed changes

DrJosh9000 reviewed Apr 3, 2023

View reviewed changes

shellscript/shellscript.go Outdated Show resolved Hide resolved

DrJosh9000 reviewed Apr 3, 2023

View reviewed changes

bootstrap/bootstrap.go Outdated Show resolved Hide resolved

pda reviewed Apr 3, 2023

View reviewed changes

triarius reviewed Apr 3, 2023

View reviewed changes

shellscript/shellscript.go Outdated Show resolved Hide resolved

moskyb force-pushed the polyglot-hooks branch from d6e6659 to 7243817 Compare April 4, 2023 01:37

moskyb force-pushed the polyglot-hooks branch 2 times, most recently from 2527108 to 81976a2 Compare May 4, 2023 00:22

DrJosh9000 reviewed May 4, 2023

View reviewed changes

moskyb force-pushed the polyglot-hooks branch from 80cedde to ca6c2f8 Compare May 5, 2023 04:08

moskyb force-pushed the polyglot-hooks branch 13 times, most recently from 95775d0 to 25e8192 Compare May 16, 2023 06:34

moskyb mentioned this pull request May 17, 2023

fix: 🪶 experiment normalised-upload-paths was left out of the list of available experiments #2076

Merged

moskyb force-pushed the polyglot-hooks branch 4 times, most recently from 2b791bc to 2f7e950 Compare May 18, 2023 02:57

moskyb marked this pull request as ready for review May 18, 2023 03:45

moskyb requested review from pda, DrJosh9000 and triarius May 18, 2023 03:45

WIP support polyglot hooks

ace3fbb

moskyb force-pushed the polyglot-hooks branch 2 times, most recently from a73416c to 6b534b1 Compare May 18, 2023 04:35

triarius reviewed May 19, 2023

View reviewed changes

moskyb added 8 commits May 19, 2023 16:09

Add tests for hook type detection

d8b5c36

Add WithUndo to experiments package

8879ad5

Add polyglot hook integration tests

fa98c76

Wrap polyglot hooks in an experiment

775cb16

Disable script hooks on windows

6634945

Executable hooks on windows probably have to be .exe, ay?

aee1191

Ensure that the scriptwrapper won't try to run anything that's not a …

1a5f050

…shell script

moskyb force-pushed the polyglot-hooks branch from 6b534b1 to 1a5f050 Compare May 19, 2023 06:12

Put hook type detection test binaries in the temp dir

251b72f

triarius approved these changes May 22, 2023

View reviewed changes

DrJosh9000 requested changes May 22, 2023

View reviewed changes

Minor edits for clarity

3971f6d

moskyb force-pushed the polyglot-hooks branch from 5526ee7 to 3971f6d Compare May 22, 2023 03:04

DrJosh9000 approved these changes May 22, 2023

View reviewed changes

moskyb merged commit 01542e0 into main May 22, 2023

moskyb deleted the polyglot-hooks branch May 22, 2023 03:36

DrJosh9000 mentioned this pull request Oct 31, 2024

Promote polyglot-hooks experiment to default #3063

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment: Polyglot hooks #2040

Experiment: Polyglot hooks #2040

moskyb commented Apr 3, 2023 •

edited

Loading

moskyb Apr 3, 2023

triarius May 19, 2023

moskyb Apr 3, 2023

triarius May 19, 2023

moskyb Apr 3, 2023

triarius Apr 3, 2023

moskyb Apr 3, 2023

triarius Apr 3, 2023 •

edited

Loading

moskyb Apr 3, 2023

triarius May 19, 2023

moskyb May 19, 2023

moskyb Apr 3, 2023

pda Apr 3, 2023 •

edited

Loading

lox commented Apr 7, 2023

moskyb commented Apr 17, 2023

DrJosh9000 left a comment

triarius left a comment

triarius May 19, 2023

triarius May 19, 2023

triarius May 19, 2023

triarius May 19, 2023

triarius May 19, 2023

DrJosh9000 left a comment

Experiment: Polyglot hooks #2040

Experiment: Polyglot hooks #2040

Conversation

moskyb commented Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pda Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

lox commented Apr 7, 2023

moskyb commented Apr 17, 2023

DrJosh9000 left a comment

Choose a reason for hiding this comment

triarius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

moskyb commented Apr 3, 2023 •

edited

Loading

triarius Apr 3, 2023 •

edited

Loading

pda Apr 3, 2023 •

edited

Loading