Use real types for build instructions

Summary:
Refactored build instructions to use concrete types and
`build.Instruction` as an interface instead of relying on a simple enum
and arbitrary string arguments. The formal types result in:

 1. Clearer internal data structures
 2. Partial compilation and proper argument quoting for all instructions
    moved into the common `build` package
 3. Higher order instructions like `build.RunAll` that easily reduce to
    compiler specific output

Test Plan: Run `arc unit` or `go test ./...`

Reviewers: thcipriani, mmodell, #release-engineering-team

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

Differential Revision: https://phabricator.wikimedia.org/D741

10 files changed, 264 insertions, 114 deletions

diff --git a/build/instructions.go b/build/instructions.go
index e807b5b..5c12796 100644
--- a/build/instructions.go
+++ b/build/instructions.go
@@ -1,14 +1,96 @@
 package build
 
-type InstructionType int
-
-const (
-	Run InstructionType = iota
-	Copy
-	Env
+import (
+	"fmt"
+	"sort"
+	"strconv"
+	"strings"
 )
 
-type Instruction struct {
-	Type      InstructionType
+type Instruction interface {
+	Compile() []string
+}
+
+type Run struct {
+	Command   string
 	Arguments []string
 }
+
+func (run Run) Compile() []string {
+	numInnerArgs := strings.Count(run.Command, `%`) - strings.Count(run.Command, `%%`)
+	command := sprintf(run.Command, run.Arguments[0:numInnerArgs])
+
+	if len(run.Arguments) > numInnerArgs {
+		command += " " + strings.Join(quoteAll(run.Arguments[numInnerArgs:]), " ")
+	}
+
+	return []string{command}
+}
+
+type RunAll struct {
+	Runs []Run
+}
+
+func (runAll RunAll) Compile() []string {
+	commands := make([]string, len(runAll.Runs))
+
+	for i, run := range runAll.Runs {
+		commands[i] = run.Compile()[0]
+	}
+
+	return []string{strings.Join(commands, " && ")}
+}
+
+type Copy struct {
+	Sources     []string
+	Destination string
+}
+
+func (copy Copy) Compile() []string {
+	return append(quoteAll(copy.Sources), quote(copy.Destination))
+}
+
+type Env struct {
+	Definitions map[string]string
+}
+
+func (env Env) Compile() []string {
+	defs := make([]string, 0, len(env.Definitions))
+	names := make([]string, 0, len(env.Definitions))
+
+	for name := range env.Definitions {
+		names = append(names, name)
+	}
+
+	sort.Strings(names)
+
+	for _, name := range names {
+		defs = append(defs, name+"="+quote(env.Definitions[name]))
+	}
+
+	return defs
+}
+
+func quote(arg string) string {
+	return strconv.Quote(arg)
+}
+
+func quoteAll(arguments []string) []string {
+	quoted := make([]string, len(arguments))
+
+	for i, arg := range arguments {
+		quoted[i] = quote(arg)
+	}
+
+	return quoted
+}
+
+func sprintf(format string, arguments []string) string {
+	args := make([]interface{}, len(arguments))
+
+	for i, v := range arguments {
+		args[i] = quote(v)
+	}
+
+	return fmt.Sprintf(format, args...)
+}
diff --git a/build/instructions_test.go b/build/instructions_test.go
new file mode 100644
index 0000000..dbab4b5
--- /dev/null
+++ b/build/instructions_test.go
@@ -0,0 +1,51 @@
+package build_test
+
+import (
+	"testing"
+
+	"gopkg.in/stretchr/testify.v1/assert"
+
+	"phabricator.wikimedia.org/source/blubber.git/build"
+)
+
+func TestRun(t *testing.T) {
+	i := build.Run{"echo %s %s", []string{"foo bar", "baz"}}
+
+	assert.Equal(t, []string{`echo "foo bar" "baz"`}, i.Compile())
+}
+
+func TestRunWithInnerAndOuterArguments(t *testing.T) {
+	i := build.Run{"useradd -d %s -u %s", []string{"/foo", "666", "bar"}}
+
+	assert.Equal(t, []string{`useradd -d "/foo" -u "666" "bar"`}, i.Compile())
+}
+
+func TestRunAll(t *testing.T) {
+	i := build.RunAll{[]build.Run{
+		{"echo %s", []string{"foo"}},
+		{"cat %s", []string{"/bar"}},
+		{"baz", []string{}},
+	}}
+
+	assert.Equal(t, []string{`echo "foo" && cat "/bar" && baz`}, i.Compile())
+}
+
+func TestCopy(t *testing.T) {
+	i := build.Copy{[]string{"source1", "source2"}, "dest"}
+
+	assert.Equal(t, []string{`"source1"`, `"source2"`, `"dest"`}, i.Compile())
+}
+
+func TestEnv(t *testing.T) {
+	i := build.Env{map[string]string{
+		"fooname": "foovalue",
+		"barname": "barvalue",
+		"quxname": "quxvalue",
+	}}
+
+	assert.Equal(t, []string{
+		`barname="barvalue"`,
+		`fooname="foovalue"`,
+		`quxname="quxvalue"`,
+	}, i.Compile())
+}
diff --git a/config/apt.go b/config/apt.go
index 2382bbf..37a5c3e 100644
--- a/config/apt.go
+++ b/config/apt.go
@@ -1,8 +1,6 @@
 package config
 
 import (
-	"strings"
-
 	"phabricator.wikimedia.org/source/blubber.git/build"
 )
 
@@ -19,10 +17,10 @@ func (apt AptConfig) InstructionsForPhase(phase build.Phase) []build.Instruction
 		switch phase {
 		case build.PhasePrivileged:
 			return []build.Instruction{
-				{build.Run, []string{
-					"apt-get update && apt-get install -y ",
-					strings.Join(apt.Packages, " "),
-					" && rm -rf /var/lib/apt/lists/*",
+				build.RunAll{[]build.Run{
+					{"apt-get update", []string{}},
+					{"apt-get install -y", apt.Packages},
+					{"rm -rf /var/lib/apt/lists/*", []string{}},
 				}},
 			}
 		}
diff --git a/config/apt_test.go b/config/apt_test.go
index 3801231..9e0e08c 100644
--- a/config/apt_test.go
+++ b/config/apt_test.go
@@ -38,10 +38,10 @@ func TestAptConfigInstructions(t *testing.T) {
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{
-					"apt-get update && apt-get install -y ",
-					"libfoo libbar",
-					" && rm -rf /var/lib/apt/lists/*",
+				build.RunAll{[]build.Run{
+					{"apt-get update", []string{}},
+					{"apt-get install -y", []string{"libfoo", "libbar"}},
+					{"rm -rf /var/lib/apt/lists/*", []string{}},
 				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePrivileged),
diff --git a/config/npm.go b/config/npm.go
index f263d24..26fd0ad 100644
--- a/config/npm.go
+++ b/config/npm.go
@@ -24,20 +24,26 @@ func (npm NpmConfig) InstructionsForPhase(phase build.Phase) []build.Instruction
 	if npm.Install.True {
 		switch phase {
 		case build.PhasePreInstall:
-			npmCmd := "npm install"
+			npmInstall := build.RunAll{[]build.Run{
+				{"cd", []string{tempNpmInstallDir}},
+				{"npm install", []string{}},
+			}}
 
 			if npm.Env == "production" {
-				npmCmd += " --production && npm dedupe"
+				npmInstall.Runs[1].Arguments = []string{"--production"}
+				npmInstall.Runs = append(npmInstall.Runs,
+					build.Run{"npm dedupe", []string{}},
+				)
 			}
 
 			return []build.Instruction{
-				{build.Run, []string{"mkdir -p " + tempNpmInstallDir}},
-				{build.Copy, []string{"package.json", tempNpmInstallDir}},
-				{build.Run, []string{"cd " + tempNpmInstallDir + " && " + npmCmd}},
+				build.Run{"mkdir -p", []string{tempNpmInstallDir}},
+				build.Copy{[]string{"package.json"}, tempNpmInstallDir},
+				npmInstall,
 			}
 		case build.PhasePostInstall:
 			return []build.Instruction{
-				{build.Run, []string{"mv " + path.Join(tempNpmInstallDir, "node_modules") + " ./"}},
+				build.Run{"mv", []string{path.Join(tempNpmInstallDir, "node_modules"), "./"}},
 			}
 		}
 	}
diff --git a/config/npm_test.go b/config/npm_test.go
index 69799a9..d73eb75 100644
--- a/config/npm_test.go
+++ b/config/npm_test.go
@@ -65,9 +65,12 @@ func TestNpmConfigInstructionsNonProduction(t *testing.T) {
 	t.Run("PhasePreInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mkdir -p /tmp/node-deps/"}},
-				{build.Copy, []string{"package.json", "/tmp/node-deps/"}},
-				{build.Run, []string{"cd /tmp/node-deps/ && npm install"}},
+				build.Run{"mkdir -p", []string{"/tmp/node-deps/"}},
+				build.Copy{[]string{"package.json"}, "/tmp/node-deps/"},
+				build.RunAll{[]build.Run{
+					{"cd", []string{"/tmp/node-deps/"}},
+					{"npm install", []string{}},
+				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePreInstall),
 		)
@@ -76,7 +79,7 @@ func TestNpmConfigInstructionsNonProduction(t *testing.T) {
 	t.Run("PhasePostInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mv /tmp/node-deps/node_modules ./"}},
+				build.Run{"mv", []string{"/tmp/node-deps/node_modules", "./"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePostInstall),
 		)
@@ -97,9 +100,13 @@ func TestNpmConfigInstructionsProduction(t *testing.T) {
 	t.Run("PhasePreInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mkdir -p /tmp/node-deps/"}},
-				{build.Copy, []string{"package.json", "/tmp/node-deps/"}},
-				{build.Run, []string{"cd /tmp/node-deps/ && npm install --production && npm dedupe"}},
+				build.Run{"mkdir -p", []string{"/tmp/node-deps/"}},
+				build.Copy{[]string{"package.json"}, "/tmp/node-deps/"},
+				build.RunAll{[]build.Run{
+					{"cd", []string{"/tmp/node-deps/"}},
+					{"npm install", []string{"--production"}},
+					{"npm dedupe", []string{}},
+				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePreInstall),
 		)
@@ -108,7 +115,7 @@ func TestNpmConfigInstructionsProduction(t *testing.T) {
 	t.Run("PhasePostInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mv /tmp/node-deps/node_modules ./"}},
+				build.Run{"mv", []string{"/tmp/node-deps/node_modules", "./"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePostInstall),
 		)
diff --git a/config/runs.go b/config/runs.go
index b2b6623..12184bf 100644
--- a/config/runs.go
+++ b/config/runs.go
@@ -1,8 +1,6 @@
 package config
 
 import (
-	"fmt"
-	"sort"
 	"strconv"
 
 	"phabricator.wikimedia.org/source/blubber.git/build"
@@ -47,53 +45,43 @@ func (run RunsConfig) Home() string {
 	}
 }
 
-func (run RunsConfig) EnvironmentDefinitions() []string {
-	defs := make([]string, 0, len(run.Environment))
-	names := make([]string, 0, len(run.Environment))
-
-	for name := range run.Environment {
-		names = append(names, name)
-	}
-
-	sort.Strings(names)
-
-	for _, name := range names {
-		defs = append(defs, name+"="+strconv.Quote(run.Environment[name]))
-	}
-
-	return defs
-}
-
 func (run RunsConfig) InstructionsForPhase(phase build.Phase) []build.Instruction {
 	ins := []build.Instruction{}
 
 	switch phase {
 	case build.PhasePrivileged:
+		runAll := build.RunAll{}
+
 		if run.In != "" {
-			ins = append(ins, build.Instruction{build.Run, []string{
-				fmt.Sprintf("mkdir -p %q", run.In),
-			}})
+			runAll.Runs = append(runAll.Runs,
+				build.Run{"mkdir -p", []string{run.In}},
+			)
 		}
 
 		if run.As != "" {
-			ins = append(ins, build.Instruction{build.Run, []string{
-				fmt.Sprintf("groupadd -o -g %d -r %q", run.Gid, run.As) + " && " +
-					fmt.Sprintf("useradd -o -m -d %q -r -g %q -u %d %q", run.Home(), run.As, run.Uid, run.As),
-			}})
+			runAll.Runs = append(runAll.Runs,
+				build.Run{"groupadd -o -g %s -r",
+					[]string{strconv.Itoa(run.Gid), run.As}},
+				build.Run{"useradd -o -m -d %s -r -g %s -u %s",
+					[]string{run.Home(), run.As, strconv.Itoa(run.Uid), run.As}},
+			)
 
 			if run.In != "" {
-				ins = append(ins, build.Instruction{build.Run, []string{
-					fmt.Sprintf("chown %q:%q %q", run.As, run.As, run.In),
-				}})
+				runAll.Runs = append(runAll.Runs,
+					build.Run{"chown %s:%s",
+						[]string{run.As, run.As, run.In}},
+				)
 			}
 		}
+
+		if len(runAll.Runs) > 0 {
+			ins = append(ins, runAll)
+		}
 	case build.PhasePrivilegeDropped:
-		ins = append(ins, build.Instruction{build.Env, []string{
-			"HOME=" + strconv.Quote(run.Home()),
-		}})
+		ins = append(ins, build.Env{map[string]string{"HOME": run.Home()}})
 
-		if definitions := run.EnvironmentDefinitions(); len(definitions) > 0 {
-			ins = append(ins, build.Instruction{build.Env, definitions})
+		if len(run.Environment) > 0 {
+			ins = append(ins, build.Env{run.Environment})
 		}
 	}
 
diff --git a/config/runs_test.go b/config/runs_test.go
index cef740c..cb5e1df 100644
--- a/config/runs_test.go
+++ b/config/runs_test.go
@@ -45,22 +45,6 @@ func TestRunsHomeWithoutUser(t *testing.T) {
 	assert.Equal(t, "/root", runs.Home())
 }
 
-func TestEnvironmentDefinitionsIsSortedAndQuoted(t *testing.T) {
-	runs := config.RunsConfig{
-		Environment: map[string]string{
-			"fooname": "foovalue",
-			"barname": "barvalue",
-			"quxname": "quxvalue",
-		},
-	}
-
-	assert.Equal(t, []string{
-		`barname="barvalue"`,
-		`fooname="foovalue"`,
-		`quxname="quxvalue"`,
-	}, runs.EnvironmentDefinitions())
-}
-
 func TestRunsConfigInstructions(t *testing.T) {
 	cfg := config.RunsConfig{
 		As:  "someuser",
@@ -75,14 +59,12 @@ func TestRunsConfigInstructions(t *testing.T) {
 
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Equal(t,
-			[]build.Instruction{
-				{build.Run, []string{`mkdir -p "/some/directory"`}},
-				{build.Run, []string{
-					`groupadd -o -g 777 -r "someuser" && ` +
-						`useradd -o -m -d "/home/someuser" -r -g "someuser" -u 666 "someuser"`,
-				}},
-				{build.Run, []string{`chown "someuser":"someuser" "/some/directory"`}},
-			},
+			[]build.Instruction{build.RunAll{[]build.Run{
+				{"mkdir -p", []string{"/some/directory"}},
+				{"groupadd -o -g %s -r", []string{"777", "someuser"}},
+				{"useradd -o -m -d %s -r -g %s -u %s", []string{"/home/someuser", "someuser", "666", "someuser"}},
+				{"chown %s:%s", []string{"someuser", "someuser", "/some/directory"}},
+			}}},
 			cfg.InstructionsForPhase(build.PhasePrivileged),
 		)
 	})
@@ -90,8 +72,8 @@ func TestRunsConfigInstructions(t *testing.T) {
 	t.Run("PhasePrivilegeDropped", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Env, []string{`HOME="/home/someuser"`}},
-				{build.Env, []string{`barname="barvalue"`, `fooname="foovalue"`}},
+				build.Env{map[string]string{"HOME": "/home/someuser"}},
+				build.Env{map[string]string{"barname": "barvalue", "fooname": "foovalue"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
 		)
@@ -101,7 +83,7 @@ func TestRunsConfigInstructions(t *testing.T) {
 
 			assert.Equal(t,
 				[]build.Instruction{
-					{build.Env, []string{`HOME="/home/someuser"`}},
+					build.Env{map[string]string{"HOME": "/home/someuser"}},
 				},
 				cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
 			)
diff --git a/docker/instructions.go b/docker/instructions.go
index 39400b3..a93ad11 100644
--- a/docker/instructions.go
+++ b/docker/instructions.go
@@ -3,25 +3,24 @@ package docker
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
 
 	"phabricator.wikimedia.org/source/blubber.git/build"
 )
 
 func NewDockerInstruction(instruction build.Instruction) (DockerInstruction, error) {
-	switch instruction.Type {
-	case build.Run:
+	switch instruction.(type) {
+	case build.Run, build.RunAll:
 		var dockerInstruction DockerRun
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	case build.Copy:
 		var dockerInstruction DockerCopy
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	case build.Env:
 		var dockerInstruction DockerEnv
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	}
 	return nil, errors.New("Unable to create DockerInstruction")
@@ -52,9 +51,8 @@ type DockerCopy struct{ abstractDockerInstruction }
 
 func (dc DockerCopy) Compile() string {
 	return fmt.Sprintf(
-		"COPY [%s, %s]\n",
-		removeNewlines(strconv.Quote(dc.arguments[0])),
-		removeNewlines(strconv.Quote(dc.arguments[1])))
+		"COPY [%s]\n",
+		removeNewlines(strings.Join(dc.arguments, ", ")))
 }
 
 type DockerEnv struct{ abstractDockerInstruction }
diff --git a/docker/instructions_test.go b/docker/instructions_test.go
index 66dac61..c03d48b 100644
--- a/docker/instructions_test.go
+++ b/docker/instructions_test.go
@@ -8,35 +8,73 @@ import (
 	"phabricator.wikimedia.org/source/blubber.git/docker"
 )
 
-func TestFactory(t *testing.T) {
-	i := build.Instruction{build.Run, []string{"echo hello"}}
-	dr, _ := docker.NewDockerInstruction(i)
+func TestRun(t *testing.T) {
+	i := build.Run{"echo", []string{"hello"}}
+	di, err := docker.NewDockerInstruction(i)
 
 	var dockerRun docker.DockerRun
 
-	assert.IsType(t, dockerRun, dr)
-	assert.Equal(t, dr.Arguments(), i.Arguments)
-	assert.NotEmpty(t, dr.Compile())
-	assert.Equal(t, "RUN echo hello\n", dr.Compile())
+	assert.Nil(t, err)
+	assert.IsType(t, dockerRun, di)
+	assert.Equal(t, "RUN echo \"hello\"\n", di.Compile())
+}
+
+func TestRunAll(t *testing.T) {
+	i := build.RunAll{[]build.Run{
+		{"echo", []string{"hello"}},
+		{"echo", []string{"yo"}},
+	}}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerRun docker.DockerRun
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerRun, di)
+	assert.Equal(t, "RUN echo \"hello\" && echo \"yo\"\n", di.Compile())
+}
+
+func TestCopy(t *testing.T) {
+	i := build.Copy{[]string{"foo1", "foo2"}, "bar"}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerCopy docker.DockerCopy
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerCopy, di)
+	assert.Equal(t, "COPY [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+}
+
+func TestEnv(t *testing.T) {
+	i := build.Env{map[string]string{"foo": "bar", "bar": "foo"}}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerEnv docker.DockerEnv
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerEnv, di)
+	assert.Equal(t, "ENV bar=\"foo\" foo=\"bar\"\n", di.Compile())
 }
 
 func TestEscapeRun(t *testing.T) {
-	i := build.Instruction{build.Run, []string{"/bin/true\nRUN echo HACKED!"}}
+	i := build.Run{"/bin/true\nRUN echo HACKED!", []string{}}
 	dr, _ := docker.NewDockerInstruction(i)
 
 	assert.Equal(t, "RUN /bin/true\\nRUN echo HACKED!\n", dr.Compile())
 }
 
 func TestEscapeCopy(t *testing.T) {
-	i := build.Instruction{build.Copy, []string{"file.a", "file.b"}}
+	i := build.Copy{[]string{"file.a", "file.b"}, "dest"}
 	dr, _ := docker.NewDockerInstruction(i)
 
-	assert.Equal(t, "COPY [\"file.a\", \"file.b\"]\n", dr.Compile())
+	assert.Equal(t, "COPY [\"file.a\", \"file.b\", \"dest\"]\n", dr.Compile())
 }
 
 func TestEscapeEnv(t *testing.T) {
-	i := build.Instruction{build.Env, []string{"a=b\nRUN echo HACKED!"}}
+	i := build.Env{map[string]string{"a": "b\nRUN echo HACKED!"}}
 	dr, _ := docker.NewDockerInstruction(i)
 
-	assert.Equal(t, "ENV a=b\\nRUN echo HACKED!\n", dr.Compile())
+	assert.Equal(t, "ENV a=\"b\\nRUN echo HACKED!\"\n", dr.Compile())
 }