Merge pull request #8907 from dokku/8875-0-38-migration-plugins-ordered-before-config-silently-fail-to-migrate-their-dokku-config-vars

fix: migrate env files before reading deprecated vars

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