From 0c3ae47299ba21fd99a097fd8437fb41f750009c Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Fri, 18 Sep 2026 00:36:38 -0400 Subject: [PATCH] fix: report config write failures instead of exiting 0 `config:set`, `config:unset` and `config:clear` discarded the result of writing the app's environment, so a write that never landed still printed `Setting config vars`, still fired the `post-config-update` trigger, still restarted the app and still exited 0. The `config-set` and `config-unset` plugin triggers behaved the same way, which meant an `app.json` `env` block that could not be persisted deployed anyway with the variables missing. The environment is now staged in a temporary file and renamed into place, so a write that fails partway leaves the previous environment intact, and that file is created at `0600` rather than being created at `0644` and chmodded afterwards. `apps:clone` and `apps:rename` never applied that chmod at all and left the new app's environment world-readable. --- docs/appendices/0.38.0-migration-guide.md | 1 + docs/configuration/environment-variables.md | 2 + docs/development/plugin-triggers.md | 17 +++- plugins/config/config.go | 24 ++--- plugins/config/config_test.go | 67 ++++++++++++- plugins/config/environment.go | 69 ++++++++++++- plugins/config/environment_test.go | 92 +++++++++++++++++ plugins/config/migrate.go | 7 -- plugins/config/migrate_test.go | 27 ++--- plugins/config/triggers.go | 3 +- tests/unit/config.bats | 103 ++++++++++++++++++++ 11 files changed, 369 insertions(+), 43 deletions(-) diff --git a/docs/appendices/0.38.0-migration-guide.md b/docs/appendices/0.38.0-migration-guide.md index 835a38def..8226bdfba 100644 --- a/docs/appendices/0.38.0-migration-guide.md +++ b/docs/appendices/0.38.0-migration-guide.md @@ -30,6 +30,7 @@ - **New in 0.38.26:** Docker options drained out of the pre-0.38.0 `DOCKER_OPTIONS_` files were copied verbatim rather than being re-serialized the way `docker-options:add` stores them, so `docker-options:remove` compared the canonical string it builds against a value that could never match it and exited successfully without removing anything. Removal now matches stored options by shell word instead of by exact string, so those entries can be removed with the value as originally written. Stored options are also rewritten into the canonical form the first time `dokku` runs after the upgrade: values whose shell metacharacters were left unquoted are quoted, and an entry that carried several flags on one line becomes one entry per flag, which additionally fixes `ps:report` reading a restart policy off such a line and the `k3s` scheduler translating only the first of several `--cap-add`/`--sysctl` flags. Options added through `docker-options:add` are already canonical and are left untouched. - **New in 0.38.26:** Deprecated `DOKKU_*` config variables belonging to plugins whose install step runs before the `config` plugin's were not migrated to their plugin property on the upgrade run. Install steps fire in alphabetical order, so `apps`, `builder`, and `checks` read an app's environment before the `config` plugin had relocated the `ENV` file, found nothing, and moved nothing - without reporting anything. `dokku config:show` kept listing the variable while the plugin behaved as though it were unset, which for `DOKKU_CHECKS_SKIPPED` meant a process type silently regained a health check it was meant to skip. The relocation is now performed before any deprecated variable is read, regardless of install order. Affected installs recover on their next `dokku plugin:install --core`, which the upgrade already runs; the variables listed below can also be re-applied by hand using their replacement command. - **New in 0.38.27:** The 0.38.26 upgrade merged the leftover pre-0.38.0 `ENV` file back over the current config. Releases 0.38.0 through 0.38.25 recorded the migration and left that file on disk on purpose, and 0.38.26 read it as a hand-edit, so the environment as it stood at the 0.38.x upgrade won over every `dokku config:set` and `dokku config:unset` made since - including reinstating variables and secrets that had been deliberately unset. Nothing appeared broken, because running containers keep the environment they were created with; the rewound values would have reached containers on the next deploy. The 0.38.26 run consumed the leftover file, so this cannot happen a second time, but it also cannot be undone automatically. To check an app that was upgraded to 0.38.26 and has a container still running from before that upgrade, compare each key in `dokku config:show ` against `docker exec printenv ` - `GIT_REV` is expected to differ after a deploy, anything else that differs was rewound. Repair with `dokku config:set --no-restart KEY=VALUE` for values that regressed and `dokku config:unset --no-restart KEY` for variables that came back. +- **New in 0.38.29:** `dokku config:set`, `config:unset` and `config:clear` discarded the result of writing the app's environment to disk, so a write that never landed still printed `Setting config vars`, still fired the `post-config-update` trigger, still restarted the app and still exited 0. The same applied to the `config-set` and `config-unset` plugin triggers, which means an `app.json` `env` block that could not be persisted deployed anyway with the variables missing. These commands now report the failure and exit non-zero without restarting the app. The environment is also staged in a temporary file and renamed into place, so a write that fails partway - on a full disk, for instance - no longer truncates the existing environment, and that file is created at `0600` rather than being created at `0644` and chmodded afterwards. `dokku apps:clone` and `dokku apps:rename` never applied that chmod at all and left the new app's environment file world-readable until the next `config:set`; they no longer do. - The storage plugin now treats persistent volumes as named, scheduler-aware first-class resources via `storage:create`, `storage:mount`, `storage:set`, and `storage:destroy`. The legacy `storage:mount :` colon form continues to work on docker-local apps but is deprecated; on k3s apps it is rejected. Existing colon-form mounts are migrated automatically the first time the new storage plugin runs (during the install trigger) - they appear as `legacy-` entries in `storage:list-entries`. The migration is idempotent and tied to a per-app flag file at `$DOKKU_LIB_ROOT/config/storage/.migrated/`; deleting that file forces a re-scan on the next install. The `storage:ensure-directory` command keeps working but now emits a deprecation warning - prefer `storage:create []` (the path defaults to the same `$DOKKU_LIB_ROOT/data/storage/` location). Storage entry names must now be DNS-1123 labels of 45 characters or less so they can be used verbatim as Helm release and Kubernetes resource names; underscores and uppercase characters that the older `ensure-directory` validator accepted are rejected for new names. The migration synthesizer always uses lowercase hex hashes so existing data is never locked out. ### TLS handshake behavior change 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 247239ed8..e52d63e66 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..5f4f72485 100644 --- a/plugins/config/config_test.go +++ b/plugins/config/config_test.go @@ -164,7 +164,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") @@ -201,7 +201,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") @@ -231,7 +231,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") @@ -284,6 +284,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..f63f70ac5 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) { +// init against the real dokku directories, so a test using this helper must not +// use them. +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"