fix(migrate): keep predastore's data dir owned by its service user

The v4 to v5 topology migration runs as root under `spx admin upgrade`,
but predastore runs as spinifex-storage with an empty capability
bounding set. Two paths left it locked out of its own state.

The migration created the cluster data dir as root:root 0750, so
spinifex-storage could not traverse into the node dirs the rename had
placed under it, and clusterrun could not create new ones. Separately,
the raft recovery that follows each state replica move reopens badger
and writes a fresh snapshot as root, leaving root-owned files inside a
node dir that the rename had otherwise left correctly owned.

Both now take their ownership from the data being moved rather than
from a service user name, so a dev install running as the invoking user
needs no special case. The data dir chown is unconditional so a re-run
repairs a dir left behind by an earlier failed run, and the tree chown
runs even when there was nothing to recover, since that path still
opens the stores.
This commit is contained in:
brynmailer-mdc
2026-08-03 12:44:10 +10:00
committed by Bryn Mailer
parent 42c7b9b663
commit 576742dfeb
3 changed files with 190 additions and 2 deletions
@@ -261,6 +261,13 @@ func relocatePredastoreData(ctx migrate.ConfigContext, topo *predastoreTopology)
continue
}
// The rename preserved this directory's ownership, which is what
// recovery must leave behind on everything it writes into it.
uid, gid, err := dirOwner(m.To)
if err != nil {
return err
}
// The replica set is renumbered, so the configuration persisted here
// names servers that no longer exist. Raft will not bootstrap over
// existing state to correct that; it has to be rewritten.
@@ -268,6 +275,13 @@ func relocatePredastoreData(ctx migrate.ConfigContext, topo *predastoreTopology)
if err != nil {
return fmt.Errorf("recover raft configuration for state replica %d in %s: %w", m.NodeID, m.To, err)
}
// Recovery reopened badger and wrote a fresh snapshot as root, both of
// which create files. Runs even when there was nothing to recover,
// which still opens the stores.
if err := chownTree(m.To, uid, gid); err != nil {
return err
}
ctx.Logger.Info("Migrated predastore state replica", "nodeID", m.NodeID, "dataDir", m.To, "raftRecovered", recovered)
}
return nil
@@ -292,9 +306,23 @@ func movePredastoreNodeDir(ctx migrate.ConfigContext, m predastoreMove) (bool, e
return false, fmt.Errorf("cannot move %s to %s: destination already exists", m.From, m.To)
}
if err := os.MkdirAll(filepath.Dir(m.To), 0750); err != nil {
return false, fmt.Errorf("create %s: %w", filepath.Dir(m.To), err)
// Taken before the rename, which carries it to the destination; the data
// dir created below has to match or predastore cannot traverse into it.
uid, gid, err := dirOwner(m.From)
if err != nil {
return false, err
}
dataDir := filepath.Dir(m.To)
if err := os.MkdirAll(dataDir, 0750); err != nil {
return false, fmt.Errorf("create %s: %w", dataDir, err)
}
// Unconditional, so a re-run also repairs a data dir left root-owned by an
// earlier run that failed after creating it.
if err := chown(dataDir, uid, gid); err != nil {
return false, err
}
if err := os.Rename(m.From, m.To); err != nil {
return false, fmt.Errorf("move %s to %s: %w", m.From, m.To, err)
}
@@ -0,0 +1,66 @@
package predastoretopology
import (
"fmt"
"io/fs"
"os"
"path/filepath"
"syscall"
)
// The migration runs as root under `spx admin upgrade`, but predastore runs as
// its own service user. Anything root creates along the way is root-owned and
// unreachable to it, so the migration has to hand each path back.
// dirOwner reports the uid and gid owning path. The migration takes the
// ownership it applies from the data it is moving rather than from a service
// user name, so a dev install running as the invoking user needs no special
// case.
func dirOwner(path string) (int, int, error) {
info, err := os.Stat(path)
if err != nil {
return 0, 0, fmt.Errorf("stat %s: %w", path, err)
}
st, ok := info.Sys().(*syscall.Stat_t)
if !ok {
return 0, 0, fmt.Errorf("cannot read ownership of %s", path)
}
return int(st.Uid), int(st.Gid), nil
}
// chown gives path to uid:gid.
//
// Only root can hand a file to another user, and only root ever creates one
// owned by the wrong user, so an unprivileged run has nothing to correct.
func chown(path string, uid, gid int) error {
if os.Geteuid() != 0 {
return nil
}
if err := os.Lchown(path, uid, gid); err != nil {
return fmt.Errorf("chown %s to %d:%d: %w", path, uid, gid, err)
}
return nil
}
// chownTree gives path and everything beneath it to uid:gid.
//
// os.Root scopes the walk to the tree, so a symlink planted inside it cannot
// redirect a chown running as root.
func chownTree(path string, uid, gid int) error {
if os.Geteuid() != 0 {
return nil
}
root, err := os.OpenRoot(path)
if err != nil {
return fmt.Errorf("open %s: %w", path, err)
}
defer root.Close()
return fs.WalkDir(root.FS(), ".", func(p string, _ fs.DirEntry, err error) error {
if err != nil {
return fmt.Errorf("walk %s: %w", filepath.Join(path, p), err)
}
return chown(filepath.Join(path, p), uid, gid)
})
}
@@ -0,0 +1,94 @@
package predastoretopology
import (
"fmt"
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// nobodyUID is an unprivileged id the root-only assertions move data to, so
// "inherited the owner" cannot be confused with "happened to already match".
const nobodyUID, nobodyGID = 65534, 65534
// TestPredastoreDataDirInheritsNodeOwnership pins the ownership of the data
// dir the migration creates. Predastore runs as its own service user while the
// migration runs as root, and cannot traverse a root-owned parent.
func TestPredastoreDataDirInheritsNodeOwnership(t *testing.T) {
nodeDirs := []string{
"distributed/db/node-1", "distributed/db/node-2", "distributed/db/node-3",
"distributed/nodes/node-1", "distributed/nodes/node-2", "distributed/nodes/node-3",
}
configDir, dataDir := predastoreFixture(t, "predastore_v4_singlenode.toml", nodeDirs)
wantUID, wantGID := os.Geteuid(), os.Getegid()
if os.Geteuid() == 0 {
// Stand the fixture up as an unprivileged user owns it, which is the
// shape of a real install the root migration has to preserve.
for _, dir := range nodeDirs {
require.NoError(t, chownTree(filepath.Join(dataDir, "predastore", dir), nobodyUID, nobodyGID))
}
wantUID, wantGID = nobodyUID, nobodyGID
}
runPredastoreMigration(t, configDir, dataDir)
cluster := filepath.Join(dataDir, "predastore", "cluster")
uid, gid, err := dirOwner(cluster)
require.NoError(t, err)
assert.Equal(t, wantUID, uid, "data dir owner")
assert.Equal(t, wantGID, gid, "data dir group")
// The node dirs arrive by rename, so they keep their owner rather than
// gaining one; assert it anyway, since recovery writes into them.
for id := 1; id <= 6; id++ {
uid, gid, err := dirOwner(filepath.Join(cluster, fmt.Sprintf("node-%d", id)))
require.NoError(t, err)
assert.Equal(t, wantUID, uid, "node-%d owner", id)
assert.Equal(t, wantGID, gid, "node-%d group", id)
}
}
// TestChownIsNoopWhenUnprivileged covers the guard that lets the migration run
// as an ordinary user, where there is no ownership to correct and every chown
// would fail.
func TestChownIsNoopWhenUnprivileged(t *testing.T) {
if os.Geteuid() == 0 {
t.Skip("asserts the unprivileged path")
}
dir := t.TempDir()
require.NoError(t, os.WriteFile(filepath.Join(dir, "file"), []byte("x"), 0640))
require.NoError(t, chown(dir, nobodyUID, nobodyGID))
require.NoError(t, chownTree(dir, nobodyUID, nobodyGID))
uid, _, err := dirOwner(dir)
require.NoError(t, err)
assert.Equal(t, os.Geteuid(), uid, "ownership must be left alone")
}
// TestChownTreeCoversNestedContents guards the recursive case: raft recovery
// writes a snapshot directory and badger segments below the node dir.
func TestChownTreeCoversNestedContents(t *testing.T) {
if os.Geteuid() != 0 {
t.Skip("chown to another user requires root")
}
dir := t.TempDir()
nested := filepath.Join(dir, "snapshots", "1-2-3")
require.NoError(t, os.MkdirAll(nested, 0750))
require.NoError(t, os.WriteFile(filepath.Join(nested, "state.bin"), []byte("x"), 0640))
require.NoError(t, chownTree(dir, nobodyUID, nobodyGID))
for _, path := range []string{dir, nested, filepath.Join(nested, "state.bin")} {
uid, gid, err := dirOwner(path)
require.NoError(t, err)
assert.Equal(t, nobodyUID, uid, path)
assert.Equal(t, nobodyGID, gid, path)
}
}