fix: ensure prefixed commands do not clobber legacy default values (#190)

Author: johnstcn

README.md

@@ -306,8 +306,8 @@ On MacOS or Windows systems, we recommend either using a VM or the provided `.de
 | Flag | Environment variable | Default | Description |
 | - | - | - | - |
 | `--setup-script` | `ENVBUILDER_SETUP_SCRIPT` |  | The script to run before the init script. It runs as the root user regardless of the user specified in the devcontainer.json file. SetupScript is ran as the root user prior to the init script. It is used to configure envbuilder dynamically during the runtime. e.g. specifying whether to start systemd or tiny init for PID 1. |
-| `--init-script` | `ENVBUILDER_INIT_SCRIPT` | `sleep infinity` | The script to run to initialize the workspace. |
-| `--init-command` | `ENVBUILDER_INIT_COMMAND` | `/bin/sh` | The command to run to initialize the workspace. |
+| `--init-script` | `ENVBUILDER_INIT_SCRIPT` |  | The script to run to initialize the workspace. Default: `sleep infinity`. |
+| `--init-command` | `ENVBUILDER_INIT_COMMAND` |  | The command to run to initialize the workspace. Default: `/bin/sh`. |
 | `--init-args` | `ENVBUILDER_INIT_ARGS` |  | The arguments to pass to the init command. They are split according to /bin/sh rules with https://github.com/kballard/go-shellquote. |
 | `--cache-repo` | `ENVBUILDER_CACHE_REPO` |  | The name of the container registry to push the cache image to. If this is empty, the cache will not be pushed. |
 | `--base-image-cache-dir` | `ENVBUILDER_BASE_IMAGE_CACHE_DIR` |  | The path to a directory where the base image can be found. This should be a read-only directory solely mounted for the purpose of caching the base image. |

envbuilder.go

@@ -86,6 +86,19 @@ type DockerConfig configfile.ConfigFile
 // Filesystem is the filesystem to use for all operations.
 // Defaults to the host filesystem.
 func Run(ctx context.Context, options Options) error {
+	// Temporarily removed these from the default settings to prevent conflicts
+	// between current and legacy environment variables that add default values.
+	// Once the legacy environment variables are phased out, this can be
+	// reinstated to the previous default values.
+	if len(options.IgnorePaths) == 0 {
+		options.IgnorePaths = []string{"/var/run"}
+	}
+	if options.InitScript == "" {
+		options.InitScript = "sleep infinity"
+	}
+	if options.InitCommand == "" {
+		options.InitCommand = "/bin/sh"
+	}
 	// Default to the shell!
 	initArgs := []string{"-c", options.InitScript}
 	if options.InitArgs != "" {
@@ -377,13 +390,6 @@ func Run(ctx context.Context, options Options) error {
 		options.CacheRepo = fmt.Sprintf("localhost:%d/local/cache", tcpAddr.Port)
 	}
 
-	// Temporarily removed this from the default settings to prevent conflicts
-	// between current and legacy environment variables that add default values.
-	// Once the legacy environment variables are phased out, this can be
-	// reinstated to the IGNORE_PATHS default.
-	if len(options.IgnorePaths) == 0 {
-		options.IgnorePaths = []string{"/var/run"}
-	}
 	// IgnorePaths in the Kaniko options doesn't properly ignore paths.
 	// So we add them to the default ignore list. See:
 	// https://github.com/GoogleContainerTools/kaniko/blob/63be4990ca5a60bdf06ddc4d10aa4eca0c0bc714/cmd/executor/cmd/root.go#L136

integration/integration_test.go

@@ -19,6 +19,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/coder/envbuilder"
 	"github.com/coder/envbuilder/devcontainer/features"
@@ -44,6 +45,54 @@ const (
 	testImageUbuntu    = "localhost:5000/envbuilder-test-ubuntu:latest"
 )
 
+func TestInitScriptInitCommand(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+
+	// Init script will hit the below handler to signify INIT_SCRIPT works.
+	initCalled := make(chan struct{})
+	initSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		initCalled <- struct{}{}
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	srv := createGitServer(t, gitServerOptions{
+		files: map[string]string{
+			// Let's say /bin/sh is not available and we can only use /bin/ash
+			"Dockerfile": fmt.Sprintf("FROM %s\nRUN unlink /bin/sh", testImageAlpine),
+		},
+	})
+	_, err := runEnvbuilder(t, options{env: []string{
+		envbuilderEnv("GIT_URL", srv.URL),
+		envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
+		envbuilderEnv("INIT_SCRIPT", fmt.Sprintf(`wget -O - %q`, initSrv.URL)),
+		envbuilderEnv("INIT_COMMAND", "/bin/ash"),
+	}})
+	require.NoError(t, err)
+
+	select {
+	case <-initCalled:
+	case <-ctx.Done():
+	}
+	require.NoError(t, ctx.Err(), "init script did not execute for prefixed env vars")
+
+	_, err = runEnvbuilder(t, options{env: []string{
+		envbuilderEnv("GIT_URL", srv.URL),
+		envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
+		fmt.Sprintf(`INIT_SCRIPT=wget -O - %q`, initSrv.URL),
+		`INIT_COMMAND=/bin/ash`,
+	}})
+	require.NoError(t, err)
+
+	select {
+	case <-initCalled:
+	case <-ctx.Done():
+	}
+	require.NoError(t, ctx.Err(), "init script did not execute for legacy env vars")
+}
+
 func TestForceSafe(t *testing.T) {
 	t.Parallel()
 

options.go

@@ -156,18 +156,18 @@ func (o *Options) CLI() serpent.OptionSet {
 				"specifying whether to start systemd or tiny init for PID 1.",
 		},
 		{
-			Flag:        "init-script",
-			Env:         WithEnvPrefix("INIT_SCRIPT"),
-			Default:     "sleep infinity",
+			Flag: "init-script",
+			Env:  WithEnvPrefix("INIT_SCRIPT"),
+			// Default:     "sleep infinity", // TODO: reinstate once legacy opts are removed.
 			Value:       serpent.StringOf(&o.InitScript),
-			Description: "The script to run to initialize the workspace.",
+			Description: "The script to run to initialize the workspace. Default: `sleep infinity`.",
 		},
 		{
-			Flag:        "init-command",
-			Env:         WithEnvPrefix("INIT_COMMAND"),
-			Default:     "/bin/sh",
+			Flag: "init-command",
+			Env:  WithEnvPrefix("INIT_COMMAND"),
+			// Default:     "/bin/sh", // TODO: reinstate once legacy opts are removed.
 			Value:       serpent.StringOf(&o.InitCommand),
-			Description: "The command to run to initialize the workspace.",
+			Description: "The command to run to initialize the workspace. Default: `/bin/sh`.",
 		},
 		{
 			Flag:  "init-args",

options_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/coder/envbuilder"
 	"github.com/coder/serpent"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -68,8 +69,8 @@ func TestEnvOptionParsing(t *testing.T) {
 func TestLegacyEnvVars(t *testing.T) {
 	legacyEnvs := map[string]string{
 		"SETUP_SCRIPT":             "./setup-legacy-script.sh",
-		"INIT_SCRIPT":              "sleep infinity",
-		"INIT_COMMAND":             "/bin/sh",
+		"INIT_SCRIPT":              "./init-legacy-script.sh",
+		"INIT_COMMAND":             "/bin/zsh",
 		"INIT_ARGS":                "arg1 arg2",
 		"CACHE_REPO":               "example-cache-repo",
 		"BASE_IMAGE_CACHE_DIR":     "/path/to/base/image/cache",
@@ -104,36 +105,36 @@ func TestLegacyEnvVars(t *testing.T) {
 
 	o := runCLI()
 
-	require.Equal(t, o.SetupScript, legacyEnvs["SETUP_SCRIPT"])
-	require.Equal(t, o.InitScript, legacyEnvs["INIT_SCRIPT"])
-	require.Equal(t, o.InitCommand, legacyEnvs["INIT_COMMAND"])
-	require.Equal(t, o.InitArgs, legacyEnvs["INIT_ARGS"])
-	require.Equal(t, o.CacheRepo, legacyEnvs["CACHE_REPO"])
-	require.Equal(t, o.BaseImageCacheDir, legacyEnvs["BASE_IMAGE_CACHE_DIR"])
-	require.Equal(t, o.LayerCacheDir, legacyEnvs["LAYER_CACHE_DIR"])
-	require.Equal(t, o.DevcontainerDir, legacyEnvs["DEVCONTAINER_DIR"])
-	require.Equal(t, o.DevcontainerJSONPath, legacyEnvs["DEVCONTAINER_JSON_PATH"])
-	require.Equal(t, o.DockerfilePath, legacyEnvs["DOCKERFILE_PATH"])
-	require.Equal(t, o.BuildContextPath, legacyEnvs["BUILD_CONTEXT_PATH"])
-	require.Equal(t, o.CacheTTLDays, int64(7))
-	require.Equal(t, o.DockerConfigBase64, legacyEnvs["DOCKER_CONFIG_BASE64"])
-	require.Equal(t, o.FallbackImage, legacyEnvs["FALLBACK_IMAGE"])
-	require.Equal(t, o.ExitOnBuildFailure, true)
-	require.Equal(t, o.ForceSafe, true)
-	require.Equal(t, o.Insecure, true)
-	require.Equal(t, o.IgnorePaths, []string{"/var/run", "/tmp"})
-	require.Equal(t, o.SkipRebuild, true)
-	require.Equal(t, o.GitURL, legacyEnvs["GIT_URL"])
-	require.Equal(t, o.GitCloneDepth, int64(1))
-	require.Equal(t, o.GitCloneSingleBranch, true)
-	require.Equal(t, o.GitUsername, legacyEnvs["GIT_USERNAME"])
-	require.Equal(t, o.GitPassword, legacyEnvs["GIT_PASSWORD"])
-	require.Equal(t, o.GitSSHPrivateKeyPath, legacyEnvs["GIT_SSH_PRIVATE_KEY_PATH"])
-	require.Equal(t, o.GitHTTPProxyURL, legacyEnvs["GIT_HTTP_PROXY_URL"])
-	require.Equal(t, o.WorkspaceFolder, legacyEnvs["WORKSPACE_FOLDER"])
-	require.Equal(t, o.SSLCertBase64, legacyEnvs["SSL_CERT_BASE64"])
-	require.Equal(t, o.ExportEnvFile, legacyEnvs["EXPORT_ENV_FILE"])
-	require.Equal(t, o.PostStartScriptPath, legacyEnvs["POST_START_SCRIPT_PATH"])
+	assert.Equal(t, legacyEnvs["SETUP_SCRIPT"], o.SetupScript)
+	assert.Equal(t, legacyEnvs["INIT_SCRIPT"], o.InitScript)
+	assert.Equal(t, legacyEnvs["INIT_COMMAND"], o.InitCommand)
+	assert.Equal(t, legacyEnvs["INIT_ARGS"], o.InitArgs)
+	assert.Equal(t, legacyEnvs["CACHE_REPO"], o.CacheRepo)
+	assert.Equal(t, legacyEnvs["BASE_IMAGE_CACHE_DIR"], o.BaseImageCacheDir)
+	assert.Equal(t, legacyEnvs["LAYER_CACHE_DIR"], o.LayerCacheDir)
+	assert.Equal(t, legacyEnvs["DEVCONTAINER_DIR"], o.DevcontainerDir)
+	assert.Equal(t, legacyEnvs["DEVCONTAINER_JSON_PATH"], o.DevcontainerJSONPath)
+	assert.Equal(t, legacyEnvs["DOCKERFILE_PATH"], o.DockerfilePath)
+	assert.Equal(t, legacyEnvs["BUILD_CONTEXT_PATH"], o.BuildContextPath)
+	assert.Equal(t, int64(7), o.CacheTTLDays)
+	assert.Equal(t, legacyEnvs["DOCKER_CONFIG_BASE64"], o.DockerConfigBase64)
+	assert.Equal(t, legacyEnvs["FALLBACK_IMAGE"], o.FallbackImage)
+	assert.Equal(t, true, o.ExitOnBuildFailure)
+	assert.Equal(t, true, o.ForceSafe)
+	assert.Equal(t, true, o.Insecure)
+	assert.Equal(t, []string{"/var/run", "/tmp"}, o.IgnorePaths)
+	assert.Equal(t, true, o.SkipRebuild)
+	assert.Equal(t, legacyEnvs["GIT_URL"], o.GitURL)
+	assert.Equal(t, int64(1), o.GitCloneDepth)
+	assert.Equal(t, true, o.GitCloneSingleBranch)
+	assert.Equal(t, legacyEnvs["GIT_USERNAME"], o.GitUsername)
+	assert.Equal(t, legacyEnvs["GIT_PASSWORD"], o.GitPassword)
+	assert.Equal(t, legacyEnvs["GIT_SSH_PRIVATE_KEY_PATH"], o.GitSSHPrivateKeyPath)
+	assert.Equal(t, legacyEnvs["GIT_HTTP_PROXY_URL"], o.GitHTTPProxyURL)
+	assert.Equal(t, legacyEnvs["WORKSPACE_FOLDER"], o.WorkspaceFolder)
+	assert.Equal(t, legacyEnvs["SSL_CERT_BASE64"], o.SSLCertBase64)
+	assert.Equal(t, legacyEnvs["EXPORT_ENV_FILE"], o.ExportEnvFile)
+	assert.Equal(t, legacyEnvs["POST_START_SCRIPT_PATH"], o.PostStartScriptPath)
 }
 
 // UpdateGoldenFiles indicates golden files should be updated.

testdata/options.golden

@@ -107,11 +107,12 @@ OPTIONS:
           The arguments to pass to the init command. They are split according to
           /bin/sh rules with https://github.com/kballard/go-shellquote.
 
-      --init-command string, $ENVBUILDER_INIT_COMMAND (default: /bin/sh)
-          The command to run to initialize the workspace.
+      --init-command string, $ENVBUILDER_INIT_COMMAND
+          The command to run to initialize the workspace. Default: `/bin/sh`.
 
-      --init-script string, $ENVBUILDER_INIT_SCRIPT (default: sleep infinity)
-          The script to run to initialize the workspace.
+      --init-script string, $ENVBUILDER_INIT_SCRIPT
+          The script to run to initialize the workspace. Default: `sleep
+          infinity`.
 
       --insecure bool, $ENVBUILDER_INSECURE
           Bypass TLS verification when cloning and pulling from container