Merge pull request #9049 from dokku/9043-config-set-reports-success-even-when-writing-the-env-file-fails

Report config write failures instead of exiting 0

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/<app>` 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

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.

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) {

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))

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

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

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
 	}

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")
 

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 {

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"