gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 17:11:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
This reverts commit 576742dfeb.
This commit is contained in:
Julian Sommer
2026-08-04 14:58:06 +10:00
parent d9907fbf13
commit 317f714355
3 changed files with 2 additions and 190 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/migrate/predastoretopology/004_predastore_topology.go
+2 -30
View File
@@ -261,13 +261,6 @@ 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.
@@ -275,13 +268,6 @@ 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
@@ -306,23 +292,9 @@ 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)
}
// 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
if err := os.MkdirAll(filepath.Dir(m.To), 0750); err != nil {
return false, fmt.Errorf("create %s: %w", filepath.Dir(m.To), 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)
}
spinifex/migrate/predastoretopology/ownership.go
-66
View File
@@ -1,66 +0,0 @@
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)
})
}
spinifex/migrate/predastoretopology/ownership_test.go
-94
View File
@@ -1,94 +0,0 @@
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)
}
}