From 43f2ed90e216e966a7a11f9f8b0a8892256e9be3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 13:50:09 +0000 Subject: [PATCH 1/5] fix expected/actual order --- options_test.go | 61 +++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/options_test.go b/options_test.go index 6d0185b8..47f4a470 100644 --- a/options_test.go +++ b/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" ) @@ -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. From 7558d4a0faea49d61e039821d8e44c6b62f21e73 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 13:51:31 +0000 Subject: [PATCH 2/5] RED: update unit tests to repro issue --- integration/integration_test.go | 33 +++++++++++++++++++++++++++++++++ options_test.go | 4 ++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 0d1e0480..c3783cb8 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -44,6 +44,39 @@ const ( testImageUbuntu = "localhost:5000/envbuilder-test-ubuntu:latest" ) +func TestInitCommand(t *testing.T) { + t.Parallel() + t.Run("OK", func(t *testing.T) { + t.Parallel() + srv := createGitServer(t, gitServerOptions{ + files: map[string]string{ + "Dockerfile": "FROM " + testImageAlpine, + }, + }) + _, err := runEnvbuilder(t, options{env: []string{ + envbuilderEnv("GIT_URL", srv.URL), + envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"), + envbuilderEnv("INIT_SCRIPT", "exit 0"), + }}) + require.NoError(t, err) + }) + + t.Run("Legacy", func(t *testing.T) { + t.Parallel() + srv := createGitServer(t, gitServerOptions{ + files: map[string]string{ + "Dockerfile": "FROM " + testImageAlpine, + }, + }) + _, err := runEnvbuilder(t, options{env: []string{ + envbuilderEnv("GIT_URL", srv.URL), + envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"), + `INIT_SCRIPT="exit 0"`, + }}) + require.NoError(t, err) + }) +} + func TestForceSafe(t *testing.T) { t.Parallel() diff --git a/options_test.go b/options_test.go index 47f4a470..14dfd182 100644 --- a/options_test.go +++ b/options_test.go @@ -69,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", From 9bb43048ee94ec2ff385b5c0adcace4c1e46dd46 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 14:50:56 +0000 Subject: [PATCH 3/5] fixup! RED: update unit tests to repro issue --- integration/integration_test.go | 50 ++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index c3783cb8..c58444ff 100644 --- a/integration/integration_test.go +++ b/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,36 +45,71 @@ const ( testImageUbuntu = "localhost:5000/envbuilder-test-ubuntu:latest" ) -func TestInitCommand(t *testing.T) { +func TestInitScriptInitCommand(t *testing.T) { t.Parallel() + t.Run("OK", func(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) { + close(initCalled) + w.WriteHeader(http.StatusOK) + })) + srv := createGitServer(t, gitServerOptions{ files: map[string]string{ - "Dockerfile": "FROM " + testImageAlpine, + // 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", "exit 0"), + 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") }) t.Run("Legacy", func(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) { + close(initCalled) + w.WriteHeader(http.StatusOK) + })) + srv := createGitServer(t, gitServerOptions{ files: map[string]string{ - "Dockerfile": "FROM " + testImageAlpine, + // 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"), - `INIT_SCRIPT="exit 0"`, + 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") }) } From 1655b4c9435b4ed031046934351b9a4146595928 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 14:54:53 +0000 Subject: [PATCH 4/5] GREEN: temporarily remove default value for INIT_SCRIPT and INIT_COMMAND --- README.md | 4 ++-- envbuilder.go | 20 +++++++++++++------- options.go | 16 ++++++++-------- testdata/options.golden | 9 +++++---- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index ec5d8a27..7b09215b 100644 --- a/README.md +++ b/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. | diff --git a/envbuilder.go b/envbuilder.go index 0c2bd2a0..fb6b69d0 100644 --- a/envbuilder.go +++ b/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 diff --git a/options.go b/options.go index 6ac9802a..9e4d38ef 100644 --- a/options.go +++ b/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", diff --git a/testdata/options.golden b/testdata/options.golden index da242ec4..38c1ec4f 100644 --- a/testdata/options.golden +++ b/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 From 564f863cde6cae4f5543af4b9c2ad419486f5df3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 14 May 2024 15:04:15 +0000 Subject: [PATCH 5/5] squish them into one test --- integration/integration_test.go | 96 +++++++++++++-------------------- 1 file changed, 38 insertions(+), 58 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index c58444ff..fdfb4571 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -48,69 +48,49 @@ const ( func TestInitScriptInitCommand(t *testing.T) { t.Parallel() - t.Run("OK", func(t *testing.T) { - 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) { - close(initCalled) - 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) + 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) + })) - select { - case <-initCalled: - case <-ctx.Done(): - } - require.NoError(t, ctx.Err(), "init script did not execute") + 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) - t.Run("Legacy", func(t *testing.T) { - 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) { - close(initCalled) - w.WriteHeader(http.StatusOK) - })) + select { + case <-initCalled: + case <-ctx.Done(): + } + require.NoError(t, ctx.Err(), "init script did not execute for prefixed env vars") - 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"), - fmt.Sprintf(`INIT_SCRIPT=wget -O - %q`, initSrv.URL), - `INIT_COMMAND=/bin/ash`, - }}) - require.NoError(t, err) + _, 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") - }) + 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) {