diff --git a/docs/configuration/environment-variables.md b/docs/configuration/environment-variables.md index f294098d3..afbc7861c 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -55,6 +55,8 @@ For multi-line values from a file (such as an SSH private key), `cat` and `base6 dokku config:set --encoded node-js-app KEY="$(cat ~/.ssh/id_rsa | base64 -w 0)" ``` +If the environment cannot be written to disk, `config:set`, `config:unset` and `config:clear` report the failure and exit non-zero without restarting the application. The previous environment is left intact, so the command can be retried once the underlying problem - a full disk or the wrong permissions on `$DOKKU_LIB_ROOT/config/` being the usual causes - has been dealt with. + When setting or unsetting environment variables, you may wish to avoid an application restart. This is useful when developing plugins or when setting multiple environment variables in a scripted manner. To do so, use the `--no-restart` flag: ```shell diff --git a/docs/development/plugin-triggers.md b/docs/development/plugin-triggers.md index 1a15fdee1..365461f31 100644 --- a/docs/development/plugin-triggers.md +++ b/docs/development/plugin-triggers.md @@ -598,7 +598,7 @@ plugn trigger config-migrate-env ### `config-set` -- Description: Sets one or more config values for an app without restarting (when --no-restart flag is specified) +- Description: Sets one or more config values for an app without restarting (when --no-restart flag is specified). Exits non-zero without restarting the app when the config cannot be persisted. - Invoked by: app-json plugin - Arguments: `[--no-restart] $APP $KEY1=VALUE1 [$KEY2=VALUE2 ...]` - Example: @@ -611,6 +611,21 @@ set -eo pipefail; [[ $DOKKU_TRACE ]] && set -x # TODO ``` +### `config-unset` + +- Description: Unsets a single config value for an app. Exits non-zero without restarting the app when the config cannot be persisted. +- Invoked by: `common` when migrating deprecated config vars to plugin properties +- Arguments: `$APP $KEY $RESTART`, or `--global $KEY $RESTART` when the `--global` flag is given +- Example: + +```shell +#!/usr/bin/env bash + +set -eo pipefail; [[ $DOKKU_TRACE ]] && set -x + +plugn trigger config-unset node-js-app DEPRECATED_VAR false +``` + ### `core-post-deploy` > To avoid issues with community plugins, this plugin trigger should be used _only_ for core plugins. Please avoid using this trigger in your own plugins. diff --git a/plugins/config/config.go b/plugins/config/config.go index 5b898370d..83878ab67 100644 --- a/plugins/config/config.go +++ b/plugins/config/config.go @@ -55,11 +55,9 @@ func SetMany(appName string, entries map[string]string, replace bool, restart bo if os.Getenv("DOKKU_QUIET_OUTPUT") == "" { fmt.Println(prettyPrintEnvEntries(" ", entries)) } - env.Write() - common.SetPermissions(common.SetPermissionInput{ - Filename: env.Filename(), - Mode: os.FileMode(0600), - }) + if err := env.Write(); err != nil { + return fmt.Errorf("Unable to write config vars: %s", err.Error()) + } triggerUpdate(appName, "set", keys) } if !global && restart && shouldRestart(appName) { @@ -91,11 +89,9 @@ func UnsetMany(appName string, keys []string, restart bool) (err error) { } } if changed { - env.Write() - common.SetPermissions(common.SetPermissionInput{ - Filename: env.Filename(), - Mode: os.FileMode(0600), - }) + if err := env.Write(); err != nil { + return fmt.Errorf("Unable to write config vars: %s", err.Error()) + } triggerUpdate(appName, "unset", keys) } if !global && restart && shouldRestart(appName) { @@ -118,11 +114,9 @@ func UnsetAll(appName string, restart bool) (err error) { changed = true } if changed { - env.Write() - common.SetPermissions(common.SetPermissionInput{ - Filename: env.Filename(), - Mode: os.FileMode(0600), - }) + if err := env.Write(); err != nil { + return fmt.Errorf("Unable to write config vars: %s", err.Error()) + } triggerUpdate(appName, "clear", []string{}) } if !global && restart && shouldRestart(appName) { diff --git a/plugins/config/config_test.go b/plugins/config/config_test.go index ef8b48d29..b82a00ff7 100644 --- a/plugins/config/config_test.go +++ b/plugins/config/config_test.go @@ -6,68 +6,47 @@ import ( "strings" "testing" - "github.com/dokku/dokku/plugins/common" - . "github.com/onsi/gomega" ) -var ( - testAppName = "test-app-1" - dokkuRoot = common.MustGetEnv("DOKKU_ROOT") - dokkuLibRoot = common.MustGetEnv("DOKKU_LIB_ROOT") - testAppRoot = filepath.Join(dokkuRoot, testAppName) - testAppDir = filepath.Join(dokkuLibRoot, "config", testAppName) - globalConfigFile = filepath.Join(dokkuLibRoot, "config", "--global", "ENV") -) +const testAppName = "test-app-1" -func setupTests() (err error) { - if err := os.Setenv("PLUGIN_PATH", "/var/lib/dokku/plugins"); err != nil { - return err - } +// setupTestApp points the dokku environment at temporary directories and seeds +// the app and global config the tests read, returning the app's config +// directory. Nothing is written to a real dokku installation and no dokku user +// needs to exist, which matters now that a failure to apply ownership is +// reported rather than discarded. The directories are removed with the test. +func setupTestApp(t *testing.T) (testAppDir string) { + t.Helper() - if err := os.Setenv("PLUGIN_ENABLED_PATH", "/var/lib/dokku/plugins/enabled"); err != nil { - return err - } + dokkuRoot, libRoot := setupIsolatedEnv(t) + t.Setenv("PLUGIN_ENABLED_PATH", filepath.Join(libRoot, "plugins", "enabled")) + t.Setenv("PLUGIN_CORE_AVAILABLE_PATH", filepath.Join(libRoot, "core-plugins", "available")) - return os.Setenv("PLUGIN_CORE_AVAILABLE_PATH", "/var/lib/dokku/core-plugins/available") -} + Expect(os.MkdirAll(filepath.Join(dokkuRoot, testAppName), 0755)).To(Succeed()) -func setupTestApp() (err error) { - Expect(os.MkdirAll(testAppRoot, 0766)).To(Succeed()) - Expect(os.MkdirAll(testAppDir, 0766)).To(Succeed()) - b := []byte("export testKey=TESTING\n") - if err = os.WriteFile(strings.Join([]string{testAppDir, "/ENV"}, ""), b, 0644); err != nil { - return - } + testAppDir = filepath.Join(libRoot, "config", testAppName) + Expect(os.MkdirAll(testAppDir, 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(testAppDir, "ENV"), []byte("export testKey=TESTING\n"), 0600)).To(Succeed()) - Expect(os.MkdirAll(filepath.Dir(globalConfigFile), 0766)).To(Succeed()) - b = []byte("export testKey=GLOBAL_TESTING\nexport globalKey=GLOBAL_VALUE") - if err = os.WriteFile(globalConfigFile, b, 0644); err != nil { - return - } - return -} + globalConfigFile := filepath.Join(libRoot, "config", "--global", "ENV") + Expect(os.MkdirAll(filepath.Dir(globalConfigFile), 0755)).To(Succeed()) + Expect(os.WriteFile(globalConfigFile, []byte("export testKey=GLOBAL_TESTING\nexport globalKey=GLOBAL_VALUE"), 0600)).To(Succeed()) -func teardownTestApp() { - os.RemoveAll(testAppRoot) - os.RemoveAll(testAppDir) + return testAppDir } func TestConfigGetWithDefault(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) + setupTestApp(t) Expect(GetWithDefault(testAppName, "unknownKey", "UNKNOWN")).To(Equal("UNKNOWN")) Expect(GetWithDefault(testAppName, "testKey", "testKey")).To(Equal("TESTING")) Expect(GetWithDefault(testAppName+"-nonexistent", "testKey", "default")).To(Equal("default")) - teardownTestApp() } func TestConfigGet(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) expectValue(testAppName, "testKey", "TESTING") expectValue("", "testKey", "GLOBAL_TESTING") @@ -78,9 +57,7 @@ func TestConfigGet(t *testing.T) { func TestConfigSetMany(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) expectValue(testAppName, "testKey", "TESTING") @@ -102,9 +79,7 @@ func TestConfigSetMany(t *testing.T) { func TestConfigUnsetAll(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) expectValue(testAppName, "testKey", "TESTING") expectValue("", "testKey", "GLOBAL_TESTING") @@ -119,9 +94,7 @@ func TestConfigUnsetAll(t *testing.T) { func TestConfigUnsetMany(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) expectValue(testAppName, "testKey", "TESTING") expectValue("", "testKey", "GLOBAL_TESTING") @@ -140,9 +113,7 @@ func TestConfigUnsetMany(t *testing.T) { func TestConfigImport(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) tempFile, err := os.CreateTemp("", "test-config-import-*.env") Expect(err).To(Succeed()) @@ -164,7 +135,7 @@ testKey2=TESTING-updated2 env.Set("testKey", "TESTING-original1") env.Set("testKey2", "TESTING-original2") env.Set("testKey3", "TESTING-original3") - env.Write() + Expect(env.Write()).To(Succeed()) Expect(CommandImport(testAppName, false, false, true, "envfile", tempFile.Name())).To(Succeed()) expectValue(testAppName, "testKey", "TESTING-updated1") @@ -179,9 +150,7 @@ testKey2=TESTING-updated2 func TestConfigImportJSON(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) tempFile, err := os.CreateTemp("", "test-config-import-*.json") Expect(err).To(Succeed()) @@ -201,7 +170,7 @@ func TestConfigImportJSON(t *testing.T) { env.Set("testKey", "TESTING-original1") env.Set("testKey2", "TESTING-original2") env.Set("testKey3", "TESTING-original3") - env.Write() + Expect(env.Write()).To(Succeed()) Expect(CommandImport(testAppName, false, false, true, "json", tempFile.Name())).To(Succeed()) expectValue(testAppName, "testKey", "TESTING-updated1") @@ -216,9 +185,7 @@ func TestConfigImportJSON(t *testing.T) { func TestEnvironmentLoading(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) env, err := LoadMergedAppEnv(testAppName) Expect(err).To(Succeed()) @@ -231,7 +198,7 @@ func TestEnvironmentLoading(t *testing.T) { env, err = LoadAppEnv(testAppName) env.Set("testKey", "TESTING-updated") env.Set("testKey2", "TESTING-'\n'-updated") - env.Write() + Expect(env.Write()).To(Succeed()) expectValue(testAppName, "testKey", "TESTING-updated") expectValue(testAppName, "testKey2", "TESTING-'\n'-updated") @@ -241,9 +208,7 @@ func TestEnvironmentLoading(t *testing.T) { func TestInvalidKeys(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + setupTestApp(t) invalidKeys := []string{"0invalidKey", "invalid:key", "invalid=Key", "!invalidKey"} for _, key := range invalidKeys { @@ -259,11 +224,9 @@ func TestInvalidKeys(t *testing.T) { func TestInvalidEnvOnDisk(t *testing.T) { RegisterTestingT(t) - Expect(setupTests()).To(Succeed()) - Expect(setupTestApp()).To(Succeed()) - defer teardownTestApp() + testAppDir := setupTestApp(t) - appConfigFile := strings.Join([]string{testAppDir, "/ENV"}, "") + appConfigFile := filepath.Join(testAppDir, "ENV") b := []byte("export --invalid-key=TESTING\nexport valid_key=value\n") if err := os.WriteFile(appConfigFile, b, 0644); err != nil { return @@ -284,6 +247,67 @@ func TestInvalidEnvOnDisk(t *testing.T) { } +// TestConfigSetManyReportsWriteFailure covers the reported bug: a write that +// never lands used to print "Setting config vars", fire the update trigger and +// exit successfully. The app's config directory path is occupied by a regular +// file so the write fails with ENOTDIR, which root cannot bypass; the read +// treats the same path as an empty environment and succeeds. +func TestConfigSetManyReportsWriteFailure(t *testing.T) { + RegisterTestingT(t) + _, libRoot := setupIsolatedEnv(t) + + Expect(os.MkdirAll(filepath.Join(libRoot, "config"), 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(libRoot, "config", "alpha"), []byte(""), 0600)).To(Succeed()) + + Expect(SetMany("alpha", map[string]string{"testKey": "value"}, false, false)).ToNot(Succeed()) + expectNoValue("alpha", "testKey") +} + +// TestConfigUnsetManyReportsWriteFailure and its UnsetAll counterpart cover the +// same bug for the removal paths. No filesystem state lets the read load a +// non-empty environment and still fail the write for root, because both go +// through one path resolved once at load, so the permission step inside Write +// is made to fail instead. That step runs against the staged temporary file, so +// the environment on disk is left untouched. +func TestConfigUnsetManyReportsWriteFailure(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + setupIsolatedApp(t, "alpha") + + t.Setenv("DOKKU_SYSTEM_USER", "dokku-no-such-user") + Expect(UnsetMany("alpha", []string{"testKey"}, false)).ToNot(Succeed()) + expectValue("alpha", "testKey", "TESTING") +} + +func TestConfigUnsetAllReportsWriteFailure(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + setupIsolatedApp(t, "alpha") + + t.Setenv("DOKKU_SYSTEM_USER", "dokku-no-such-user") + Expect(UnsetAll("alpha", false)).ToNot(Succeed()) + expectValue("alpha", "testKey", "TESTING") +} + +// TestTriggerConfigUnsetReportsFailure covers the config-unset plugin trigger, +// which discarded the error it was handed and always exited successfully. An +// invalid key fails validation before anything is read or written. +func TestTriggerConfigUnsetReportsFailure(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + + Expect(TriggerConfigUnset("alpha", "invalid-key", false)).ToNot(Succeed()) +} + +// setupIsolatedApp creates the config directory for an app inside an isolated +// environment and seeds it with a value to remove +func setupIsolatedApp(t *testing.T, appName string) { + t.Helper() + + Expect(setupAppConfigDir(appName)).To(Succeed()) + Expect(SetMany(appName, map[string]string{"testKey": "TESTING"}, false, false)).To(Succeed()) +} + func expectValue(appName string, key string, expected string) { v, ok := Get(appName, key) Expect(ok).To(Equal(true)) diff --git a/plugins/config/environment.go b/plugins/config/environment.go index 3e9d5313d..0f6131250 100644 --- a/plugins/config/environment.go +++ b/plugins/config/environment.go @@ -164,12 +164,77 @@ func (e *Env) Merge(other *Env) { } } -// Write an Env back to the file it was read from as an exportfile +// Write an Env back to the file it was read from as an exportfile. The contents +// are written to a temporary file in the same directory and renamed into place, +// so a failed write leaves the previous environment intact rather than +// truncating it. The temporary file is created at 0600, so the values are never +// on disk in a mode other users can read. func (e *Env) Write() error { if e.filename == "" { return errors.New("this Env was created unbound to a file") } - return godotenv.Write(e.Map(), e.filename) + + contents, err := godotenv.Marshal(e.Map()) + if err != nil { + return fmt.Errorf("Unable to serialize environment: %s", err.Error()) + } + + target := resolveEnvPath(e.filename) + file, err := os.CreateTemp(filepath.Dir(target), ".ENV-*") + if err != nil { + return fmt.Errorf("Unable to create temporary environment file: %s", err.Error()) + } + defer os.Remove(file.Name()) + + if err := writeEnvFile(file, contents); err != nil { + return err + } + + if err := common.SetPermissions(common.SetPermissionInput{ + Filename: file.Name(), + Mode: os.FileMode(0600), + }); err != nil { + return fmt.Errorf("Unable to set permissions on environment file: %s", err.Error()) + } + + if err := os.Rename(file.Name(), target); err != nil { + return fmt.Errorf("Unable to move environment file into place: %s", err.Error()) + } + + return nil +} + +// resolveEnvPath follows any symlink at filename so the environment is written +// through the symlink rather than replacing it with a regular file, and so the +// temporary file is created on the same filesystem as the real target. A path +// that cannot be resolved - a file that does not exist yet being the common +// case - is returned unchanged. +func resolveEnvPath(filename string) string { + resolved, err := filepath.EvalSymlinks(filename) + if err != nil { + return filename + } + + return resolved +} + +// writeEnvFile writes contents to file, flushes it to disk and closes it +func writeEnvFile(file *os.File, contents string) error { + if _, err := file.WriteString(contents + "\n"); err != nil { + file.Close() + return fmt.Errorf("Unable to write environment file: %s", err.Error()) + } + + if err := file.Sync(); err != nil { + file.Close() + return fmt.Errorf("Unable to flush environment file: %s", err.Error()) + } + + if err := file.Close(); err != nil { + return fmt.Errorf("Unable to close environment file: %s", err.Error()) + } + + return nil } // Export the Env in the given format diff --git a/plugins/config/environment_test.go b/plugins/config/environment_test.go index dbe0bf744..61f55bf85 100644 --- a/plugins/config/environment_test.go +++ b/plugins/config/environment_test.go @@ -1,6 +1,8 @@ package config import ( + "os" + "path/filepath" "testing" . "github.com/onsi/gomega" @@ -91,6 +93,96 @@ func TestGet(t *testing.T) { Expect(b).To(Equal(true)) //anything but "0" is true } +// TestWriteSetsRestrictivePermissions covers an Env written over a file left +// behind at a mode other users can read, which is what every app ENV file +// created before this was fixed looks like on disk. +func TestWriteSetsRestrictivePermissions(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + + filename := filepath.Join(t.TempDir(), "ENV") + Expect(os.WriteFile(filename, []byte("export testKey=TESTING\n"), 0644)).To(Succeed()) + + env, err := loadFromFile("alpha", filename) + Expect(err).To(Succeed()) + env.Set("testKey", "updated") + Expect(env.Write()).To(Succeed()) + + info, err := os.Stat(filename) + Expect(err).To(Succeed()) + Expect(info.Mode().Perm()).To(Equal(os.FileMode(0600))) + expectFileValue(filename, "testKey", "updated") +} + +func TestWriteFailsWhenDirectoryIsMissing(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + + env, err := loadFromFile("alpha", filepath.Join(t.TempDir(), "missing", "ENV")) + Expect(err).To(Succeed()) + env.Set("testKey", "value") + Expect(env.Write()).ToNot(Succeed()) +} + +// TestWriteLeavesFileIntactOnFailure covers the guarantee that makes a reported +// failure safe to act on: the environment is staged in a temporary file and +// renamed into place, so a failure never leaves a truncated ENV file behind. +// The permission step is what is made to fail here, because it is the last +// thing that happens before the rename. +func TestWriteLeavesFileIntactOnFailure(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + + filename := filepath.Join(t.TempDir(), "ENV") + Expect(os.WriteFile(filename, []byte("export testKey=TESTING\n"), 0600)).To(Succeed()) + original, err := os.ReadFile(filename) + Expect(err).To(Succeed()) + + env, err := loadFromFile("alpha", filename) + Expect(err).To(Succeed()) + env.Set("testKey", "updated") + + t.Setenv("DOKKU_SYSTEM_USER", "dokku-no-such-user") + Expect(env.Write()).ToNot(Succeed()) + + Expect(os.ReadFile(filename)).To(Equal(original)) +} + +// TestWriteFollowsSymlinks covers an ENV file an operator has pointed at +// storage of their own. Renaming the staged file over the symlink would replace +// it with a regular file and orphan the original. +func TestWriteFollowsSymlinks(t *testing.T) { + RegisterTestingT(t) + setupIsolatedEnv(t) + + dir := t.TempDir() + target := filepath.Join(dir, "elsewhere", "ENV") + Expect(os.MkdirAll(filepath.Dir(target), 0755)).To(Succeed()) + Expect(os.WriteFile(target, []byte("export testKey=TESTING\n"), 0600)).To(Succeed()) + + filename := filepath.Join(dir, "ENV") + Expect(os.Symlink(target, filename)).To(Succeed()) + + env, err := loadFromFile("alpha", filename) + Expect(err).To(Succeed()) + env.Set("testKey", "updated") + Expect(env.Write()).To(Succeed()) + + info, err := os.Lstat(filename) + Expect(err).To(Succeed()) + Expect(info.Mode() & os.ModeSymlink).ToNot(Equal(os.FileMode(0))) + expectFileValue(target, "testKey", "updated") +} + +// expectFileValue asserts the ENV file at filename holds the given value +func expectFileValue(filename string, key string, expected string) { + env, err := loadFromFile("expected", filename) + Expect(err).To(Succeed()) + value, ok := env.Get(key) + Expect(ok).To(Equal(true)) + Expect(value).To(Equal(expected)) +} + func pairs(vars ...string) map[string]string { res := map[string]string{} var i = 0 diff --git a/plugins/config/migrate.go b/plugins/config/migrate.go index 588f8761b..1e46340a1 100644 --- a/plugins/config/migrate.go +++ b/plugins/config/migrate.go @@ -115,13 +115,6 @@ func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error { return fmt.Errorf("Unable to write environment: %s", err.Error()) } - if err := common.SetPermissions(common.SetPermissionInput{ - Filename: env.Filename(), - Mode: os.FileMode(0600), - }); err != nil { - return fmt.Errorf("Unable to set permissions on environment: %s", err.Error()) - } - if err := writeEnvMigrated(name); err != nil { return err } diff --git a/plugins/config/migrate_test.go b/plugins/config/migrate_test.go index cf0bb202b..163b4581e 100644 --- a/plugins/config/migrate_test.go +++ b/plugins/config/migrate_test.go @@ -9,11 +9,12 @@ import ( "github.com/dokku/dokku/plugins/common" ) -// setupMigrateEnv points the dokku env at temporary directories and tells the +// setupIsolatedEnv points the dokku env at temporary directories and tells the // permission helpers to chown files to the current user (a no-op) so the test -// works without root. The package-level paths in config_test.go are captured at -// init against the real dokku directories, so these tests must not use them. -func setupMigrateEnv(t *testing.T) (dokkuRoot string, libRoot string) { +// works without root and without a dokku user existing. Every test in this +// package goes through it, directly or via setupTestApp, so none of them touch +// a real dokku installation. +func setupIsolatedEnv(t *testing.T) (dokkuRoot string, libRoot string) { t.Helper() libRoot = t.TempDir() @@ -79,7 +80,7 @@ func expectEnvValue(t *testing.T, appName, key, want string) { } func TestMigrateEnvFiles_DrainsAndRemovesAppFile(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_CHECKS_SKIPPED=worker\nexport MY_VAR=value\n") @@ -99,7 +100,7 @@ func TestMigrateEnvFiles_DrainsAndRemovesAppFile(t *testing.T) { } func TestMigrateEnvFiles_DrainsAndRemovesGlobalFile(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) legacy := filepath.Join(dokkuRoot, "ENV") if err := os.WriteFile(legacy, []byte("export DOKKU_WAIT_TO_RETIRE=30\n"), 0600); err != nil { @@ -124,7 +125,7 @@ func TestMigrateEnvFiles_DrainsAndRemovesGlobalFile(t *testing.T) { // that is still there once the migration has been recorded. Nothing it holds is // imported, and it is moved aside so the values remain recoverable. func TestMigrateEnvFiles_PreservesStaleFileWithoutImporting(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\n") if err := MigrateEnvFiles(); err != nil { @@ -152,7 +153,7 @@ func TestMigrateEnvFiles_PreservesStaleFileWithoutImporting(t *testing.T) { // left the legacy file in place, so draining it a second time replayed the // environment as it stood at that upgrade over every config:set made since. func TestMigrateEnvFiles_KeepsConfigSetAfterMigration(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DATABASE_URL=value-A\n") if err := MigrateEnvFiles(); err != nil { @@ -176,7 +177,7 @@ func TestMigrateEnvFiles_KeepsConfigSetAfterMigration(t *testing.T) { // key unset after the migration was recorded came back when the legacy file was // drained again, which for a rotated secret meant putting it back into service. func TestMigrateEnvFiles_DoesNotResurrectUnsetKeys(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_PROXY_PORT=80\n") if err := MigrateEnvFiles(); err != nil { @@ -201,7 +202,7 @@ func TestMigrateEnvFiles_DoesNotResurrectUnsetKeys(t *testing.T) { // upgrade: the leftover file still agrees with the current config, so there is // nothing to preserve and no reason to say anything about it. func TestMigrateEnvFiles_RemovesStaleFileMatchingCurrentConfig(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n") if err := MigrateEnvFiles(); err != nil { @@ -226,7 +227,7 @@ func TestMigrateEnvFiles_RemovesStaleFileMatchingCurrentConfig(t *testing.T) { // releases 0.38.0 through 0.38.25 never removed at all, so every host upgraded // through that range still has one. func TestMigrateEnvFiles_PreservesStaleGlobalFile(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) legacy := filepath.Join(dokkuRoot, "ENV") if err := os.WriteFile(legacy, []byte("export DOKKU_WAIT_TO_RETIRE=30\n"), 0600); err != nil { @@ -255,7 +256,7 @@ func TestMigrateEnvFiles_PreservesStaleGlobalFile(t *testing.T) { // that does import: before the migration is recorded the legacy file is the // source of truth, so it overwrites the value already held at the config path. func TestMigrateEnvFiles_LegacyValueWins(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil { t.Fatalf("MkdirAll: %v", err) @@ -279,7 +280,7 @@ func TestMigrateEnvFiles_LegacyValueWins(t *testing.T) { } func TestMigrateEnvFiles_MarksAppsWithoutLegacyFile(t *testing.T) { - dokkuRoot, _ := setupMigrateEnv(t) + dokkuRoot, _ := setupIsolatedEnv(t) if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil { t.Fatalf("MkdirAll: %v", err) @@ -297,7 +298,7 @@ func TestMigrateEnvFiles_MarksAppsWithoutLegacyFile(t *testing.T) { // TestMigrateEnvFiles_KeepsLegacyFileWhenWriteFails verifies the legacy file // survives a failure to write the merged environment, so nothing is lost. func TestMigrateEnvFiles_KeepsLegacyFileWhenWriteFails(t *testing.T) { - dokkuRoot, libRoot := setupMigrateEnv(t) + dokkuRoot, libRoot := setupIsolatedEnv(t) legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n") diff --git a/plugins/config/triggers.go b/plugins/config/triggers.go index 98ac0c434..1bedc1f39 100644 --- a/plugins/config/triggers.go +++ b/plugins/config/triggers.go @@ -56,8 +56,7 @@ func TriggerConfigSet(appName string, noRestart bool, pairs ...string) error { // TriggerConfigUnset unsets an app config value by key func TriggerConfigUnset(appName string, key string, restart bool) error { - UnsetMany(appName, []string{key}, restart) - return nil + return UnsetMany(appName, []string{key}, restart) } func setupAppConfigDir(appName string) error { diff --git a/tests/unit/config.bats b/tests/unit/config.bats index 1f7d9c164..24108fdbe 100644 --- a/tests/unit/config.bats +++ b/tests/unit/config.bats @@ -11,6 +11,7 @@ setup() { } teardown() { + restore_app_config_dir destroy_app if [[ -f ${DOKKU_LIB_ROOT}/config/--global/ENV.bak ]]; then mv -f ${DOKKU_LIB_ROOT}/config/--global/ENV.bak ${DOKKU_LIB_ROOT}/config/--global/ENV @@ -704,6 +705,108 @@ teardown() { assert_success } +@test "(config:set) fails when the config file cannot be written" { + run /bin/bash -c "dokku config:set --no-restart $TEST_APP first_key=first_value" + echo "output: $output" + echo "status: $status" + assert_success + + block_app_config_dir + + run /bin/bash -c "dokku config:set --no-restart $TEST_APP second_key=second_value" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Unable to write config vars" + + restore_app_config_dir + + run /bin/bash -c "dokku config:get $TEST_APP first_key" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "first_value" + + run /bin/bash -c "dokku config:get $TEST_APP second_key" + echo "output: $output" + echo "status: $status" + assert_failure +} + +@test "(config:unset) fails when the config file cannot be written" { + run /bin/bash -c "dokku config:set --no-restart $TEST_APP first_key=first_value" + echo "output: $output" + echo "status: $status" + assert_success + + block_app_config_dir + + run /bin/bash -c "dokku config:unset --no-restart $TEST_APP first_key" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Unable to write config vars" + + restore_app_config_dir + + run /bin/bash -c "dokku config:get $TEST_APP first_key" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "first_value" +} + +@test "(config:clear) fails when the config file cannot be written" { + run /bin/bash -c "dokku config:set --no-restart $TEST_APP first_key=first_value" + echo "output: $output" + echo "status: $status" + assert_success + + block_app_config_dir + + run /bin/bash -c "dokku config:clear --no-restart $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Unable to write config vars" + + restore_app_config_dir + + run /bin/bash -c "dokku config:get $TEST_APP first_key" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "first_value" +} + +@test "(config) config-unset plugin trigger reports a failure" { + run_plugn_trigger config-unset "$TEST_APP" invalid-key false + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid key name" +} + +block_app_config_dir() { + declare desc="makes the app config directory unwritable for the dokku user" + + run /bin/bash -c "sudo chown root:root ${DOKKU_LIB_ROOT}/config/$TEST_APP && sudo chmod 0555 ${DOKKU_LIB_ROOT}/config/$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success +} + +restore_app_config_dir() { + declare desc="hands the app config directory back to the dokku user" + + if [[ ! -d "${DOKKU_LIB_ROOT}/config/$TEST_APP" ]]; then + return 0 + fi + + sudo chmod 0755 "${DOKKU_LIB_ROOT}/config/$TEST_APP" + sudo chown -R dokku:dokku "${DOKKU_LIB_ROOT}/config/$TEST_APP" +} + stage_stale_env() { declare desc="writes a legacy ENV file at the pre-0.38 path for the test app"