fix: migrate env files before reading deprecated vars

Install steps run in alphabetical order of the enabled plugin directory, so `apps`, `builder`, and `checks` read an app's environment before the `config` plugin had moved the `ENV` file to its new location. The read came back empty, so their deprecated `DOKKU_*` variables were never migrated to the matching plugin property and were never unset, with nothing reported either way: `dokku config:show` kept listing the variable while the plugin behaved as though it were unset. The relocation now runs before any deprecated variable is read, whatever the install order, and each old file is removed as soon as it has been drained rather than on a later install, which also covers the global file that was never removed at all. A file that reappears at the old path can only have been written by hand, so it is merged in with a warning naming its keys instead of being discarded.

plugins/checks/install

@@ -60,6 +60,11 @@ trigger-checks-install() {
   declare trigger="install"
 
   fn-plugin-property-setup "checks"
+
+  # the checks plugin installs before the config plugin, so the pre-0.38 ENV
+  # files have to be drained before any deprecated config var is read
+  plugn trigger config-migrate-env
+
   migrate_checks_vars_0_5_0 "$@"
   migrate_checks_vars_0_6_0 "$@"
 

plugins/common/properties.go

@@ -668,6 +668,8 @@ type MigrateConfigEntry struct {
 // across all apps and optionally globally. It is idempotent: if the property already
 // exists, the migration is skipped for that app/entry.
 func MigrateConfigToProperties(pluginName string, entries []MigrateConfigEntry) error {
+	migrateLegacyEnvFiles()
+
 	apps, err := UnfilteredDokkuApps()
 	if err != nil && !errors.Is(err, NoAppsExist) {
 		return nil
@@ -694,6 +696,24 @@ func MigrateConfigToProperties(pluginName string, entries []MigrateConfigEntry)
 	return nil
 }
 
+// migrateLegacyEnvFiles drains the pre-0.38 ENV files into the config property
+// path before any config var is read.
+//
+// Install triggers fire in lexicographic order of the enabled-plugin directory
+// names, so plugins sorting before "config" - apps, builder and checks among
+// them - would otherwise read an environment the config plugin has not
+// relocated yet, find it empty, and migrate nothing without reporting anything.
+// A failure here is not fatal: the migration is retried on the next install, so
+// an unavailable trigger degrades to the previous behavior rather than aborting
+// every plugin's install.
+func migrateLegacyEnvFiles() {
+	if _, err := CallPlugnTrigger(PlugnTriggerInput{
+		Trigger: "config-migrate-env",
+	}); err != nil {
+		LogWarn(fmt.Sprintf("Unable to migrate legacy env files: %s", err.Error()))
+	}
+}
+
 // migrateConfigEntry migrates a single config variable to a property for a given app
 func migrateConfigEntry(pluginName string, appName string, configVar string, entry MigrateConfigEntry) error {
 	if entry.ListProperty {

plugins/config/Makefile

@@ -1,6 +1,6 @@
 GOARCH ?= amd64
 SUBCOMMANDS = subcommands/bundle subcommands/clear subcommands/export subcommands/get subcommands/import subcommands/keys subcommands/show subcommands/set subcommands/unset
-TRIGGERS = triggers/config-export triggers/config-get triggers/config-get-global triggers/install triggers/config-set triggers/config-unset triggers/post-app-clone-setup triggers/post-app-rename-setup triggers/post-create triggers/post-delete
+TRIGGERS = triggers/config-export triggers/config-get triggers/config-get-global triggers/config-migrate-env triggers/install triggers/config-set triggers/config-unset triggers/post-app-clone-setup triggers/post-app-rename-setup triggers/post-create triggers/post-delete
 BUILD = commands config_sub subcommands triggers
 PLUGIN_NAME = config
 

plugins/config/migrate.go

@@ -0,0 +1,139 @@
+package config
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/dokku/dokku/plugins/common"
+)
+
+// envMigratedProperty records that an ENV file has been drained out of the
+// pre-0.38 location into the config property path.
+const envMigratedProperty = "env-migrated"
+
+// MigrateEnvFiles drains the pre-0.38 ENV files - $DOKKU_ROOT/ENV and
+// $DOKKU_ROOT/<app>/ENV - into the config property path, removing each file as
+// soon as it has been drained.
+//
+// It is safe to call from any plugin's install trigger, and does so via the
+// config-migrate-env plugin trigger. Plugins whose enabled-directory name sorts
+// before "config" - apps, builder and checks among them - run their deprecated
+// DOKKU_* config var to property migrations before the config plugin's own
+// install trigger would have relocated these files, and would otherwise read an
+// empty environment and silently migrate nothing. Nothing here may assume the
+// config install trigger has already run.
+func MigrateEnvFiles() error {
+	if err := common.PropertySetup("config"); err != nil {
+		return fmt.Errorf("Unable to setup config properties: %s", err.Error())
+	}
+
+	if err := migrateGlobalEnv(); err != nil {
+		return fmt.Errorf("Unable to migrate global environment: %s", err.Error())
+	}
+
+	apps, err := common.UnfilteredDokkuApps()
+	if err != nil {
+		return nil
+	}
+
+	for _, appName := range apps {
+		if err := migrateAppEnv(appName); err != nil {
+			return fmt.Errorf("Unable to migrate environment for %s: %s", appName, err.Error())
+		}
+	}
+
+	return nil
+}
+
+// migrateGlobalEnv drains $DOKKU_ROOT/ENV into the global config property path.
+func migrateGlobalEnv() error {
+	if err := common.PropertySetup("--global"); err != nil {
+		return fmt.Errorf("Unable to setup global environment: %s", err.Error())
+	}
+
+	oldGlobalEnvFile := filepath.Join(common.MustGetEnv("DOKKU_ROOT"), "ENV")
+	globalEnv, err := LoadGlobalEnv()
+	if err != nil {
+		return fmt.Errorf("Unable to load global environment: %s", err.Error())
+	}
+
+	return drainLegacyEnvFile("--global", oldGlobalEnvFile, globalEnv)
+}
+
+// migrateAppEnv drains $DOKKU_ROOT/<app>/ENV into the app's config property path.
+func migrateAppEnv(appName string) error {
+	if err := common.PropertySetupApp("config", appName); err != nil {
+		return fmt.Errorf("Unable to setup app environment: %s", err.Error())
+	}
+
+	if err := setupAppConfigDir(appName); err != nil {
+		return fmt.Errorf("Unable to setup app config directory: %s", err.Error())
+	}
+
+	env, err := LoadAppEnv(appName)
+	if err != nil {
+		return fmt.Errorf("Unable to load app environment: %s", err.Error())
+	}
+
+	oldEnvFile := filepath.Join(common.AppRoot(appName), "ENV")
+	return drainLegacyEnvFile(appName, oldEnvFile, env)
+}
+
+// drainLegacyEnvFile merges the legacy ENV file at oldEnvFile into env, records
+// the migration against name, and removes the legacy file. The file is only
+// removed once the merged environment has been written successfully, so a parse
+// or write failure leaves the original in place.
+//
+// A legacy file that turns up after the migration has already been recorded was
+// written by hand rather than through `dokku config:*`, so its contents are
+// merged in and named in a warning rather than being discarded silently.
+func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
+	migrated := common.PropertyGetDefault("config", name, envMigratedProperty, "") == "true"
+
+	if !common.FileExists(oldEnvFile) {
+		if migrated {
+			return nil
+		}
+		return writeEnvMigrated(name)
+	}
+
+	oldEnv, err := loadFromFile(name, oldEnvFile)
+	if err != nil {
+		return fmt.Errorf("Unable to load old environment: %s", err.Error())
+	}
+
+	if migrated && oldEnv.Len() > 0 {
+		common.LogWarn(fmt.Sprintf("Importing %s written outside of dokku config: %s", oldEnvFile, strings.Join(oldEnv.Keys(), " ")))
+	}
+
+	env.Merge(oldEnv)
+	if err := env.Write(); err != nil {
+		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
+	}
+
+	if err := os.Remove(oldEnvFile); err != nil {
+		return fmt.Errorf("Unable to remove migrated file %s: %s", oldEnvFile, err.Error())
+	}
+
+	return nil
+}
+
+func writeEnvMigrated(name string) error {
+	if err := common.PropertyWrite("config", name, envMigratedProperty, "true"); err != nil {
+		return fmt.Errorf("Unable to set %s property: %s", envMigratedProperty, err.Error())
+	}
+	return nil
+}

plugins/config/migrate_test.go

@@ -0,0 +1,198 @@
+package config
+
+import (
+	"os"
+	"os/user"
+	"path/filepath"
+	"testing"
+
+	"github.com/dokku/dokku/plugins/common"
+)
+
+// setupMigrateEnv 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) {
+	t.Helper()
+
+	libRoot = t.TempDir()
+	dokkuRoot = t.TempDir()
+
+	t.Setenv("DOKKU_LIB_ROOT", libRoot)
+	t.Setenv("DOKKU_ROOT", dokkuRoot)
+	t.Setenv("PLUGIN_PATH", filepath.Join(libRoot, "plugins"))
+
+	current, err := user.Current()
+	if err != nil {
+		t.Fatalf("user.Current: %v", err)
+	}
+	group, err := user.LookupGroupId(current.Gid)
+	if err != nil {
+		t.Fatalf("user.LookupGroupId: %v", err)
+	}
+	t.Setenv("DOKKU_SYSTEM_USER", current.Username)
+	t.Setenv("DOKKU_SYSTEM_GROUP", group.Name)
+
+	return dokkuRoot, libRoot
+}
+
+func writeLegacyAppEnv(t *testing.T, dokkuRoot, appName, contents string) string {
+	t.Helper()
+	if err := os.MkdirAll(filepath.Join(dokkuRoot, appName), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+	path := filepath.Join(dokkuRoot, appName, "ENV")
+	if err := os.WriteFile(path, []byte(contents), 0600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+	return path
+}
+
+func expectEnvValue(t *testing.T, appName, key, want string) {
+	t.Helper()
+	got, ok := Get(appName, key)
+	if !ok {
+		t.Fatalf("expected %s to be set for %s", key, appName)
+	}
+	if got != want {
+		t.Errorf("%s for %s = %q, want %q", key, appName, got, want)
+	}
+}
+
+func TestMigrateEnvFiles_DrainsAndRemovesAppFile(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_CHECKS_SKIPPED=worker\nexport MY_VAR=value\n")
+
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "DOKKU_CHECKS_SKIPPED", "worker")
+	expectEnvValue(t, "alpha", "MY_VAR", "value")
+
+	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
+		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+	}
+	if common.PropertyGet("config", "alpha", envMigratedProperty) != "true" {
+		t.Errorf("expected %s property to be set for alpha", envMigratedProperty)
+	}
+}
+
+func TestMigrateEnvFiles_DrainsAndRemovesGlobalFile(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	legacy := filepath.Join(dokkuRoot, "ENV")
+	if err := os.WriteFile(legacy, []byte("export DOKKU_WAIT_TO_RETIRE=30\n"), 0600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "--global", "DOKKU_WAIT_TO_RETIRE", "30")
+
+	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
+		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+	}
+	if common.PropertyGet("config", "--global", envMigratedProperty) != "true" {
+		t.Errorf("expected %s property to be set globally", envMigratedProperty)
+	}
+}
+
+// TestMigrateEnvFiles_ImportsHandEditedFile covers a legacy file that turns up
+// after the migration was already recorded, which means it was written by hand
+// rather than through `dokku config:*`. Its contents are merged in rather than
+// discarded.
+func TestMigrateEnvFiles_ImportsHandEditedFile(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export SECOND=two\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "FIRST", "one")
+	expectEnvValue(t, "alpha", "SECOND", "two")
+	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
+		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+	}
+}
+
+// TestMigrateEnvFiles_LegacyValueWins documents the merge precedence the
+// hand-edit path depends on: the legacy file overwrites the value already held
+// at the config path.
+func TestMigrateEnvFiles_LegacyValueWins(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+	if err := common.PropertySetupApp("config", "alpha"); err != nil {
+		t.Fatalf("PropertySetupApp: %v", err)
+	}
+	if err := setupAppConfigDir("alpha"); err != nil {
+		t.Fatalf("setupAppConfigDir: %v", err)
+	}
+	if err := SetMany("alpha", map[string]string{"SHARED": "new"}, false, false); err != nil {
+		t.Fatalf("SetMany: %v", err)
+	}
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export SHARED=legacy\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "SHARED", "legacy")
+}
+
+func TestMigrateEnvFiles_MarksAppsWithoutLegacyFile(t *testing.T) {
+	dokkuRoot, _ := setupMigrateEnv(t)
+
+	if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("MigrateEnvFiles: %v", err)
+	}
+
+	if common.PropertyGet("config", "alpha", envMigratedProperty) != "true" {
+		t.Errorf("expected %s property to be set for an app with no legacy file", envMigratedProperty)
+	}
+}
+
+// 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)
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n")
+
+	// Occupy the app's config directory path with a regular file so
+	// MkdirAll cannot create it, even when the tests run as root.
+	if err := os.MkdirAll(filepath.Join(libRoot, "config"), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(libRoot, "config", "alpha"), []byte(""), 0600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	if err := MigrateEnvFiles(); err == nil {
+		t.Fatalf("expected MigrateEnvFiles to fail")
+	}
+
+	if _, err := os.Stat(legacy); err != nil {
+		t.Errorf("expected %s to survive a failed migration, got err=%v", legacy, err)
+	}
+	if common.PropertyGet("config", "alpha", envMigratedProperty) == "true" {
+		t.Errorf("did not expect %s property to be set after a failed migration", envMigratedProperty)
+	}
+}

plugins/config/src/triggers/triggers.go

@@ -37,6 +37,8 @@ func main() {
 	case "config-get-global":
 		key := flag.Arg(0)
 		err = config.TriggerConfigGetGlobal(key)
+	case "config-migrate-env":
+		err = config.TriggerConfigMigrateEnv()
 	case "config-set":
 		appName := flag.Arg(0)
 		pairs := flag.Args()[1:]

plugins/config/triggers.go

@@ -77,44 +77,11 @@ func setupAppConfigDir(appName string) error {
 	})
 }
 
-func migrateGlobalEnv() error {
-	if err := common.PropertySetup("--global"); err != nil {
-		return fmt.Errorf("Unable to setup global environment: %s", err.Error())
-	}
-
-	oldGlobalEnvFile := filepath.Join(common.MustGetEnv("DOKKU_ROOT"), "ENV")
-	isGlobalMigrated := common.PropertyGetDefault("config", "--global", "env-migrated", "")
-	if isGlobalMigrated == "true" {
-		return nil
-	}
-
-	oldGlobalEnv, err := loadFromFile("--global", oldGlobalEnvFile)
-	if err != nil {
-		return fmt.Errorf("Unable to load old global environment: %s", err.Error())
-	}
-
-	globalEnv, err := LoadGlobalEnv()
-	if err != nil {
-		return fmt.Errorf("Unable to load global environment: %s", err.Error())
-	}
-
-	globalEnv.Merge(oldGlobalEnv)
-	if err := globalEnv.Write(); err != nil {
-		return fmt.Errorf("Unable to write global environment: %s", err.Error())
-	}
-
-	if err := common.SetPermissions(common.SetPermissionInput{
-		Filename: globalEnv.Filename(),
-		Mode:     os.FileMode(0600),
-	}); err != nil {
-		return fmt.Errorf("Unable to set permissions on global environment: %s", err.Error())
-	}
-
-	if err := common.PropertyWrite("config", "--global", "env-migrated", "true"); err != nil {
-		return fmt.Errorf("Unable to set env-migrated property: %s", err.Error())
-	}
-
-	return nil
+// TriggerConfigMigrateEnv drains the pre-0.38 ENV files into the config
+// property path. Exposed as a trigger so plugins that install before config can
+// force the migration before reading a deprecated config var.
+func TriggerConfigMigrateEnv() error {
+	return MigrateEnvFiles()
 }
 
 // TriggerInstall runs the install step for the config plugin
@@ -123,65 +90,7 @@ func TriggerInstall() error {
 		return fmt.Errorf("Unable to install the config plugin: %s", err.Error())
 	}
 
-	if err := migrateGlobalEnv(); err != nil {
-		return fmt.Errorf("Unable to migrate global environment: %s", err.Error())
-	}
-
-	apps, err := common.UnfilteredDokkuApps()
-	if err != nil {
-		return nil
-	}
-
-	// migrate all app ENV files to config path
-	for _, appName := range apps {
-		if err := common.PropertySetupApp("config", appName); err != nil {
-			return fmt.Errorf("Unable to setup app environment: %s", err.Error())
-		}
-
-		if err := setupAppConfigDir(appName); err != nil {
-			return fmt.Errorf("Unable to setup app config directory: %s", err.Error())
-		}
-
-		oldEnvFile := filepath.Join(common.AppRoot(appName), "ENV")
-		isMigrated := common.PropertyGetDefault("config", appName, "env-migrated", "")
-		// delete the old file on the next install
-		if isMigrated == "true" {
-			if err := os.RemoveAll(oldEnvFile); err != nil {
-				return fmt.Errorf("Unable to remove old ENV file: %s", err.Error())
-			}
-			continue
-		}
-
-		// skip if the file doesn't exist
-		if _, err := os.Stat(oldEnvFile); err != nil {
-			if err := common.PropertyWrite("config", appName, "env-migrated", "true"); err != nil {
-				return fmt.Errorf("Unable to set env-migrated property: %s", err.Error())
-			}
-			continue
-		}
-
-		// merge in the old env into the new env
-		oldEnv, err := loadFromFile(appName, oldEnvFile)
-		if err != nil {
-			return fmt.Errorf("Unable to load old environment: %s", err.Error())
-		}
-
-		env, err := LoadAppEnv(appName)
-		if err != nil {
-			return fmt.Errorf("Unable to load app environment: %s", err.Error())
-		}
-
-		env.Merge(oldEnv)
-		if err := env.Write(); err != nil {
-			return fmt.Errorf("Unable to write app environment: %s", err.Error())
-		}
-
-		if err := common.PropertyWrite("config", appName, "env-migrated", "true"); err != nil {
-			return fmt.Errorf("Unable to set env-migrated property: %s", err.Error())
-		}
-	}
-
-	return nil
+	return MigrateEnvFiles()
 }
 
 // TriggerPostAppCloneSetup creates new buildpacks files