Merge pull request #8558 from dokku/8557-storage-mount-migration-in-0-38-0-uses-the-wrong-owner-for-the-legacy-files-causing-ps-rebuild-failures

plugins/storage/entry.go

@@ -148,8 +148,11 @@ func SaveEntry(entry *Entry) error {
 		return fmt.Errorf("unable to encode storage entry %q: %w", entry.Name, err)
 	}
 
-	path := entryPath(entry.Name)
-	if err := os.WriteFile(path, data, 0600); err != nil {
+	if err := common.WriteBytesToFile(common.WriteBytesToFileInput{
+		Bytes:    data,
+		Filename: entryPath(entry.Name),
+		Mode:     0600,
+	}); err != nil {
 		return fmt.Errorf("unable to write storage entry %q: %w", entry.Name, err)
 	}
 	return nil

plugins/storage/entry_test.go

@@ -1,17 +1,36 @@
 package storage
 
 import (
+	"os"
+	"os/user"
 	"path/filepath"
+	"strconv"
 	"strings"
+	"syscall"
 	"testing"
 
 	. "github.com/onsi/gomega"
 )
 
+// withTempLibRoot points DOKKU_LIB_ROOT at a temp dir and overrides the
+// permission helpers' target user/group to the current process user so
+// SetPermissions is a no-op chown rather than a hard failure on dev and
+// CI machines that lack a dokku system user.
 func withTempLibRoot(t *testing.T) string {
 	t.Helper()
 	dir := t.TempDir()
 	t.Setenv("DOKKU_LIB_ROOT", dir)
+
+	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 dir
 }
 
@@ -203,3 +222,26 @@ func TestLegacyMountToEntry(t *testing.T) {
 	Expect(abs.HostPath).To(Equal("/host/path"))
 	Expect(abs.Validate()).To(Succeed())
 }
+
+// TestSaveEntryChownsToSystemUser is a regression test for #8557: the
+// install trigger runs as root, so SaveEntry must chown the file to the
+// dokku user before ps:rebuild can read it.
+func TestSaveEntryChownsToSystemUser(t *testing.T) {
+	RegisterTestingT(t)
+	withTempLibRoot(t)
+
+	Expect(SaveEntry(&Entry{
+		Name:      "demo-data",
+		Scheduler: SchedulerDockerLocal,
+		HostPath:  "/data/demo",
+	})).To(Succeed())
+
+	info, err := os.Stat(entryPath("demo-data"))
+	Expect(err).NotTo(HaveOccurred())
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	Expect(ok).To(BeTrue())
+
+	current, err := user.Current()
+	Expect(err).NotTo(HaveOccurred())
+	Expect(strconv.Itoa(int(stat.Uid))).To(Equal(current.Uid))
+}

plugins/storage/migrate.go

@@ -168,11 +168,7 @@ func migrateMount(appName string, mount string, phases []string) error {
 }
 
 func touchMigrationFlag(appName string) error {
-	f, err := os.Create(migrationFlagFile(appName))
-	if err != nil {
-		return err
-	}
-	return f.Close()
+	return common.TouchFile(migrationFlagFile(appName))
 }
 
 func filterMountLines(lines []string) []string {

plugins/storage/migrate_test.go

@@ -5,36 +5,26 @@ import (
 	"os/user"
 	"path/filepath"
 	"sort"
+	"strconv"
+	"syscall"
 	"testing"
 
 	"github.com/dokku/dokku/plugins/common"
 	dockeroptions "github.com/dokku/dokku/plugins/docker-options"
 )
 
-// setupMigrationEnv points DOKKU_LIB_ROOT and DOKKU_ROOT at temp dirs and
-// tells the permission helpers to chown to the current user (a no-op),
-// so the test works without root and without the dokku system user.
-// Mirrors the helper in docker-options/migration_test.go.
+// setupMigrationEnv reuses withTempLibRoot for DOKKU_LIB_ROOT plus the
+// permission-helper user/group overrides, then layers DOKKU_ROOT and
+// PLUGIN_PATH on top so DokkuApps can find the staged apps. Mirrors the
+// docker-options migration test fixture.
 func setupMigrationEnv(t *testing.T) (libRoot string, dokkuRoot string) {
 	t.Helper()
-	libRoot = t.TempDir()
+	libRoot = withTempLibRoot(t)
 	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)
-
 	// MigrateLegacyMounts creates the flag dir before iterating apps;
 	// migrateApp (the per-app helper) assumes it already exists. Pre-
 	// create it so tests that call migrateApp directly don't trip.
@@ -300,6 +290,40 @@ func TestMigrateAppRefusesEntryNameCollision(t *testing.T) {
 	}
 }
 
+// TestMigrateAppChownsLegacyEntryFiles is the regression for #8557:
+// migrating from the install trigger (which runs as root) must produce
+// dokku-owned legacy-*.json files. setupMigrationEnv overrides the
+// system user/group to the current process user, so we assert against
+// that uid rather than the literal dokku one.
+func TestMigrateAppChownsLegacyEntryFiles(t *testing.T) {
+	_, dokkuRoot := setupMigrationEnv(t)
+	stageApp(t, dokkuRoot, "alpha", map[string][]string{
+		"deploy": {"-v /var/log:/log"},
+	})
+
+	if err := migrateApp("alpha"); err != nil {
+		t.Fatalf("migrateApp: %v", err)
+	}
+
+	entry := LegacyMountToEntry("/var/log:/log")
+	info, err := os.Stat(entryPath(entry.Name))
+	if err != nil {
+		t.Fatalf("stat entry: %v", err)
+	}
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		t.Fatalf("stat.Sys is not *syscall.Stat_t")
+	}
+
+	current, err := user.Current()
+	if err != nil {
+		t.Fatalf("user.Current: %v", err)
+	}
+	if got := strconv.Itoa(int(stat.Uid)); got != current.Uid {
+		t.Errorf("entry file uid = %s, want %s", got, current.Uid)
+	}
+}
+
 func equalSorted(a, b []string) bool {
 	if len(a) != len(b) {
 		return false

plugins/storage/triggers.go

@@ -50,6 +50,10 @@ func TriggerInstall() error {
 		}
 	}
 
+	if err := repairRegistryOwnership(); err != nil {
+		return fmt.Errorf("Unable to repair storage registry ownership: %s", err.Error())
+	}
+
 	if err := MigrateLegacyMounts(); err != nil {
 		return fmt.Errorf("storage migration failed: %w", err)
 	}
@@ -73,6 +77,32 @@ func TriggerInstall() error {
 	return nil
 }
 
+// repairRegistryOwnership rewrites ownership on every regular file under
+// the registry tree so the dokku user can read entry / migration-flag
+// files. Earlier 0.38 builds wrote those files as root because SaveEntry
+// and touchMigrationFlag bypassed the permission helpers; on an upgrade
+// the per-app migration flag short-circuits MigrateLegacyMounts so a
+// plain SaveEntry fix would leave the broken files in place. The walk is
+// idempotent: calling SetPermissions on a file already owned by the
+// target user produces a no-op chown.
+func repairRegistryOwnership() error {
+	return filepath.Walk(RegistryDirectory(), func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			if os.IsNotExist(err) {
+				return nil
+			}
+			return err
+		}
+		if info.IsDir() {
+			return nil
+		}
+		return common.SetPermissions(common.SetPermissionInput{
+			Filename: path,
+			Mode:     info.Mode().Perm(),
+		})
+	})
+}
+
 // TriggerPostDelete removes the attachment store for an app that's being
 // destroyed. Entries are global and survive app deletion. The whole
 // per-app property folder goes away rather than being rewritten as

plugins/storage/triggers_test.go

@@ -0,0 +1,40 @@
+package storage
+
+import (
+	"os"
+	"os/user"
+	"strconv"
+	"syscall"
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+// TestRepairRegistryOwnership covers the install-time repair that exists
+// to fix #8557 on systems that already ran the buggy 0.38.0 install. The
+// repair must rewrite ownership without rewriting the file mode, and
+// must tolerate a missing registry tree (clean install).
+func TestRepairRegistryOwnership(t *testing.T) {
+	RegisterTestingT(t)
+	withTempLibRoot(t)
+
+	Expect(EnsureEntriesDirectory()).To(Succeed())
+	path := entryPath("legacy-deadbeef")
+	Expect(os.WriteFile(path, []byte(`{"name":"legacy-deadbeef"}`), 0640)).To(Succeed())
+
+	Expect(repairRegistryOwnership()).To(Succeed())
+
+	info, err := os.Stat(path)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(info.Mode().Perm()).To(Equal(os.FileMode(0640)))
+
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	Expect(ok).To(BeTrue())
+
+	current, err := user.Current()
+	Expect(err).NotTo(HaveOccurred())
+	Expect(strconv.Itoa(int(stat.Uid))).To(Equal(current.Uid))
+
+	Expect(os.RemoveAll(RegistryDirectory())).To(Succeed())
+	Expect(repairRegistryOwnership()).To(Succeed())
+}

tests/unit/storage.bats

@@ -359,6 +359,15 @@ teardown() {
   echo "status: $status"
   assert_success
 
+  # Regression for #8557: the legacy-*.json file must be readable by the
+  # dokku user. The migration runs as root via the install trigger, so
+  # without an explicit chown the file lands as root:root and ps:rebuild
+  # fails with permission denied.
+  legacy_entry_name=$(dokku storage:list-entries --format json | jq -r '.[] | select(.name | startswith("legacy-")) | .name' | head -1)
+  run /bin/bash -c "stat -c '%U:%G' /var/lib/dokku/data/storage-registry/entries/${legacy_entry_name}.json"
+  assert_success
+  assert_output "dokku:dokku"
+
   # docker-options no longer holds the -v line on either phase.
   run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-deploy"
   assert_success