fix: do not import an already-migrated ENV file

Releases 0.38.0 through 0.38.25 recorded the ENV file migration and deliberately left the file at the old path, so 0.38.26 treating a leftover file as a hand-edit replayed the environment as it stood at that upgrade over every `config:set` and `config:unset` made since, reinstating variables and secrets that had been deliberately unset. A file found at the old path once its migration is on record is no longer imported: it is removed when it agrees with the current config, and is otherwise moved aside to `ENV.migrated` with the keys it disagrees on named in a warning. That warning now reaches the operator during an upgrade rather than being swallowed, which is why the overwrite went unreported.

plugins/common/properties.go

@@ -706,9 +706,15 @@ func MigrateConfigToProperties(pluginName string, entries []MigrateConfigEntry)
 // 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.
+//
+// The trigger's stderr is streamed rather than captured. This is the first call
+// to reach the migration on an upgrade, so anything it has to report about the
+// files it found is only ever said once, and a captured warning would be
+// discarded with the response.
 func migrateLegacyEnvFiles() {
 	if _, err := CallPlugnTrigger(PlugnTriggerInput{
-		Trigger: "config-migrate-env",
+		Trigger:      "config-migrate-env",
+		StreamStderr: true,
 	}); err != nil {
 		LogWarn(fmt.Sprintf("Unable to migrate legacy env files: %s", err.Error()))
 	}

plugins/config/migrate.go

@@ -86,9 +86,11 @@ func migrateAppEnv(appName string) error {
 // 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.
+// The merge happens once and only once. A legacy file that is still there when
+// the migration is already on record is handed to preserveStaleEnvFile instead:
+// releases 0.38.0 through 0.38.25 recorded the migration and left the file
+// behind on purpose, so importing it would replay the environment as it stood
+// at that upgrade over every config:set and config:unset made since.
 func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 	migrated := common.PropertyGetDefault("config", name, envMigratedProperty, "") == "true"
 
@@ -99,15 +101,15 @@ func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 		return writeEnvMigrated(name)
 	}
 
+	if migrated {
+		return preserveStaleEnvFile(name, oldEnvFile, env)
+	}
+
 	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())
@@ -131,6 +133,51 @@ func drainLegacyEnvFile(name string, oldEnvFile string, env *Env) error {
 	return nil
 }
 
+// preserveStaleEnvFile clears a legacy ENV file that outlived its own migration
+// out of the way without importing any of it. A file whose values all match the
+// current environment has nothing left to give and is removed outright;
+// otherwise it is moved alongside itself with a .migrated suffix and the keys it
+// disagrees on are named, so an operator who did write it by hand can still
+// recover the values through `dokku config:set`.
+func preserveStaleEnvFile(name string, oldEnvFile string, env *Env) error {
+	staleEnv, err := loadFromFile(name, oldEnvFile)
+	if err != nil {
+		return fmt.Errorf("Unable to load stale environment: %s", err.Error())
+	}
+
+	diverged := divergedKeys(staleEnv, env)
+	if len(diverged) == 0 {
+		if err := os.Remove(oldEnvFile); err != nil {
+			return fmt.Errorf("Unable to remove stale file %s: %s", oldEnvFile, err.Error())
+		}
+		return nil
+	}
+
+	preservedFile := oldEnvFile + ".migrated"
+	common.LogWarn(fmt.Sprintf("Not importing already-migrated ENV file %s: the current config takes precedence for %s", oldEnvFile, strings.Join(diverged, " ")))
+	common.LogWarn(fmt.Sprintf("Preserved at %s; apply any values still needed with dokku config:set", preservedFile))
+
+	if err := os.Rename(oldEnvFile, preservedFile); err != nil {
+		return fmt.Errorf("Unable to preserve stale file %s: %s", oldEnvFile, err.Error())
+	}
+
+	return nil
+}
+
+// divergedKeys returns the keys stale holds that env either does not have at all
+// or holds a different value for.
+func divergedKeys(stale *Env, env *Env) []string {
+	diverged := []string{}
+	for _, key := range stale.Keys() {
+		staleValue, _ := stale.Get(key)
+		if value, ok := env.Get(key); !ok || value != staleValue {
+			diverged = append(diverged, key)
+		}
+	}
+
+	return diverged
+}
+
 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())

plugins/config/migrate_test.go

@@ -49,6 +49,24 @@ func writeLegacyAppEnv(t *testing.T, dokkuRoot, appName, contents string) string
 	return path
 }
 
+// expectPreservedKey asserts the stale ENV file moved aside at path still holds
+// the given key, so the values it was not allowed to import remain recoverable.
+func expectPreservedKey(t *testing.T, path, key, want string) {
+	t.Helper()
+
+	preserved, err := loadFromFile("preserved", path)
+	if err != nil {
+		t.Fatalf("loadFromFile(%s): %v", path, err)
+	}
+	got, ok := preserved.Get(key)
+	if !ok {
+		t.Fatalf("expected %s to hold %s", path, key)
+	}
+	if got != want {
+		t.Errorf("%s in %s = %q, want %q", key, path, got, want)
+	}
+}
+
 func expectEnvValue(t *testing.T, appName, key, want string) {
 	t.Helper()
 	got, ok := Get(appName, key)
@@ -102,11 +120,10 @@ func TestMigrateEnvFiles_DrainsAndRemovesGlobalFile(t *testing.T) {
 	}
 }
 
-// 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) {
+// TestMigrateEnvFiles_PreservesStaleFileWithoutImporting covers a legacy file
+// 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)
 
 	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\n")
@@ -114,21 +131,129 @@ func TestMigrateEnvFiles_ImportsHandEditedFile(t *testing.T) {
 		t.Fatalf("first MigrateEnvFiles: %v", err)
 	}
 
-	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export SECOND=two\n")
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export FIRST=one\nexport 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 _, ok := Get("alpha", "SECOND"); ok {
+		t.Errorf("did not expect SECOND to be imported from a stale legacy file")
+	}
+
 	if _, err := os.Stat(legacy); !os.IsNotExist(err) {
-		t.Errorf("expected %s to be removed, got err=%v", legacy, err)
+		t.Errorf("expected %s to be cleared out, got err=%v", legacy, err)
+	}
+	expectPreservedKey(t, legacy+".migrated", "SECOND", "two")
+}
+
+// TestMigrateEnvFiles_KeepsConfigSetAfterMigration covers the 0.38.26 regression
+// reported in #8929: releases 0.38.0 through 0.38.25 recorded the migration and
+// 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)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DATABASE_URL=value-A\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := SetMany("alpha", map[string]string{"DATABASE_URL": "value-B"}, false, false); err != nil {
+		t.Fatalf("SetMany: %v", err)
+	}
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DATABASE_URL=value-A\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "alpha", "DATABASE_URL", "value-B")
+	expectPreservedKey(t, legacy+".migrated", "DATABASE_URL", "value-A")
+}
+
+// TestMigrateEnvFiles_DoesNotResurrectUnsetKeys covers the other half of #8929: a
+// 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)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_PROXY_PORT=80\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := UnsetMany("alpha", []string{"DOKKU_PROXY_PORT"}, false); err != nil {
+		t.Fatalf("UnsetMany: %v", err)
+	}
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export DOKKU_PROXY_PORT=80\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	if _, ok := Get("alpha", "DOKKU_PROXY_PORT"); ok {
+		t.Errorf("did not expect DOKKU_PROXY_PORT to be resurrected from a stale legacy file")
 	}
 }
 
-// TestMigrateEnvFiles_LegacyValueWins documents the merge precedence the
-// hand-edit path depends on: the legacy file overwrites the value already held
-// at the config path.
+// TestMigrateEnvFiles_RemovesStaleFileMatchingCurrentConfig covers the common
+// 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)
+
+	writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("first MigrateEnvFiles: %v", err)
+	}
+
+	legacy := writeLegacyAppEnv(t, dokkuRoot, "alpha", "export MY_VAR=value\n")
+	if err := MigrateEnvFiles(); err != nil {
+		t.Fatalf("second MigrateEnvFiles: %v", err)
+	}
+
+	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 _, err := os.Stat(legacy + ".migrated"); !os.IsNotExist(err) {
+		t.Errorf("did not expect %s.migrated to be left behind, got err=%v", legacy, err)
+	}
+}
+
+// TestMigrateEnvFiles_PreservesStaleGlobalFile covers the global file, which
+// 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)
+
+	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("first MigrateEnvFiles: %v", err)
+	}
+
+	if err := SetMany("--global", map[string]string{"DOKKU_WAIT_TO_RETIRE": "60"}, false, false); err != nil {
+		t.Fatalf("SetMany: %v", err)
+	}
+
+	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("second MigrateEnvFiles: %v", err)
+	}
+
+	expectEnvValue(t, "--global", "DOKKU_WAIT_TO_RETIRE", "60")
+	expectPreservedKey(t, legacy+".migrated", "DOKKU_WAIT_TO_RETIRE", "30")
+}
+
+// TestMigrateEnvFiles_LegacyValueWins documents the precedence of the one drain
+// 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)