diff --git a/cmd/containers-storage/main.go b/cmd/containers-storage/main.go index 37e221010e..9b1547cd72 100644 --- a/cmd/containers-storage/main.go +++ b/cmd/containers-storage/main.go @@ -73,7 +73,7 @@ func main() { os.Exit(1) } if options.GraphRoot == "" && options.RunRoot == "" && options.GraphDriverName == "" && len(options.GraphDriverOptions) == 0 { - options, _ = types.DefaultStoreOptionsAutoDetectUID() + options, _ = types.DefaultStoreOptions() } args := flags.Args() if len(args) < 1 { diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index 8493a2c19a..fe216a2fff 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -254,7 +254,7 @@ func convertTarToZstdChunked(destDirectory string, blobSize int64, iss ImageSour // GetDiffer returns a differ than can be used with ApplyDiffWithDiffer. func GetDiffer(ctx context.Context, store storage.Store, blobSize int64, annotations map[string]string, iss ImageSourceSeekable) (graphdriver.Differ, error) { - storeOpts, err := types.DefaultStoreOptionsAutoDetectUID() + storeOpts, err := types.DefaultStoreOptions() if err != nil { return nil, err } diff --git a/store.go b/store.go index 6753b296ff..2c97015b36 100644 --- a/store.go +++ b/store.go @@ -3545,8 +3545,8 @@ func SetDefaultConfigFilePath(path string) { } // DefaultConfigFile returns the path to the storage config file used -func DefaultConfigFile(rootless bool) (string, error) { - return types.DefaultConfigFile(rootless) +func DefaultConfigFile() (string, error) { + return types.DefaultConfigFile() } // ReloadConfigurationFile parses the specified configuration file and overrides diff --git a/types/options.go b/types/options.go index 72d56a332e..ad0bfa43a6 100644 --- a/types/options.go +++ b/types/options.go @@ -89,7 +89,7 @@ func loadDefaultStoreOptions() { _, err := os.Stat(defaultOverrideConfigFile) if err == nil { - // The DefaultConfigFile(rootless) function returns the path + // The DefaultConfigFile() function returns the path // of the used storage.conf file, by returning defaultConfigFile // If override exists containers/storage uses it by default. defaultConfigFile = defaultOverrideConfigFile @@ -111,21 +111,41 @@ func loadDefaultStoreOptions() { setDefaults() } -// defaultStoreOptionsIsolated is an internal implementation detail of DefaultStoreOptions to allow testing. -// Everyone but the tests this is intended for should only call DefaultStoreOptions, never this function. -func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf string) (StoreOptions, error) { +// loadStoreOptions returns the default storage ops for containers +func loadStoreOptions() (StoreOptions, error) { + storageConf, err := DefaultConfigFile() + if err != nil { + return defaultStoreOptions, err + } + return loadStoreOptionsFromConfFile(storageConf) +} + +// usePerUserStorage returns whether the user private storage must be used. +// We cannot simply use the unshare.IsRootless() condition, because +// that checks only if the current process needs a user namespace to +// work and it would break cases where the process is already created +// in a user namespace (e.g. nested Podman/Buildah) and the desired +// behavior is to use system paths instead of user private paths. +func usePerUserStorage() bool { + return unshare.IsRootless() && unshare.GetRootlessUID() != 0 +} + +// loadStoreOptionsFromConfFile is an internal implementation detail of DefaultStoreOptions to allow testing. +// Everyone but the tests this is intended for should only call loadStoreOptions, never this function. +func loadStoreOptionsFromConfFile(storageConf string) (StoreOptions, error) { var ( defaultRootlessRunRoot string defaultRootlessGraphRoot string err error ) + defaultStoreOptionsOnce.Do(loadDefaultStoreOptions) if loadDefaultStoreOptionsErr != nil { return StoreOptions{}, loadDefaultStoreOptionsErr } storageOpts := defaultStoreOptions - if rootless && rootlessUID != 0 { - storageOpts, err = getRootlessStorageOpts(rootlessUID, storageOpts) + if usePerUserStorage() { + storageOpts, err = getRootlessStorageOpts(storageOpts) if err != nil { return storageOpts, err } @@ -139,7 +159,7 @@ func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf str defaultRootlessGraphRoot = storageOpts.GraphRoot storageOpts = StoreOptions{} reloadConfigurationFileIfNeeded(storageConf, &storageOpts) - if rootless && rootlessUID != 0 { + if usePerUserStorage() { // If the file did not specify a graphroot or runroot, // set sane defaults so we don't try and use root-owned // directories @@ -158,6 +178,7 @@ func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf str if storageOpts.RunRoot == "" { return storageOpts, fmt.Errorf("runroot must be set") } + rootlessUID := unshare.GetRootlessUID() runRoot, err := expandEnvPath(storageOpts.RunRoot, rootlessUID) if err != nil { return storageOpts, err @@ -188,26 +209,17 @@ func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf str return storageOpts, nil } -// loadStoreOptions returns the default storage ops for containers -func loadStoreOptions(rootless bool, rootlessUID int) (StoreOptions, error) { - storageConf, err := DefaultConfigFile(rootless && rootlessUID != 0) - if err != nil { - return defaultStoreOptions, err - } - return defaultStoreOptionsIsolated(rootless, rootlessUID, storageConf) -} - // UpdateOptions should be called iff container engine received a SIGHUP, // otherwise use DefaultStoreOptions -func UpdateStoreOptions(rootless bool, rootlessUID int) (StoreOptions, error) { - storeOptions, storeError = loadStoreOptions(rootless, rootlessUID) +func UpdateStoreOptions() (StoreOptions, error) { + storeOptions, storeError = loadStoreOptions() return storeOptions, storeError } // DefaultStoreOptions returns the default storage ops for containers -func DefaultStoreOptions(rootless bool, rootlessUID int) (StoreOptions, error) { +func DefaultStoreOptions() (StoreOptions, error) { once.Do(func() { - storeOptions, storeError = loadStoreOptions(rootless, rootlessUID) + storeOptions, storeError = loadStoreOptions() }) return storeOptions, storeError } @@ -272,9 +284,11 @@ func isRootlessDriver(driver string) bool { } // getRootlessStorageOpts returns the storage opts for containers running as non root -func getRootlessStorageOpts(rootlessUID int, systemOpts StoreOptions) (StoreOptions, error) { +func getRootlessStorageOpts(systemOpts StoreOptions) (StoreOptions, error) { var opts StoreOptions + rootlessUID := unshare.GetRootlessUID() + dataDir, err := homedir.GetDataHome() if err != nil { return opts, err @@ -355,12 +369,6 @@ func getRootlessStorageOpts(rootlessUID int, systemOpts StoreOptions) (StoreOpti return opts, nil } -// DefaultStoreOptionsAutoDetectUID returns the default storage ops for containers -func DefaultStoreOptionsAutoDetectUID() (StoreOptions, error) { - uid := unshare.GetRootlessUID() - return DefaultStoreOptions(uid != 0, uid) -} - var prevReloadConfig = struct { storeOptions *StoreOptions mod time.Time @@ -530,8 +538,8 @@ func Options() (StoreOptions, error) { } // Save overwrites the tomlConfig in storage.conf with the given conf -func Save(conf TomlConfig, rootless bool) error { - configFile, err := DefaultConfigFile(rootless) +func Save(conf TomlConfig) error { + configFile, err := DefaultConfigFile() if err != nil { return err } @@ -549,10 +557,10 @@ func Save(conf TomlConfig, rootless bool) error { } // StorageConfig is used to retrieve the storage.conf toml in order to overwrite it -func StorageConfig(rootless bool) (*TomlConfig, error) { +func StorageConfig() (*TomlConfig, error) { config := new(TomlConfig) - configFile, err := DefaultConfigFile(rootless) + configFile, err := DefaultConfigFile() if err != nil { return nil, err } diff --git a/types/options_test.go b/types/options_test.go index 5201936749..bd2d26564a 100644 --- a/types/options_test.go +++ b/types/options_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/containers/storage/pkg/idtools" + "github.com/containers/storage/pkg/unshare" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "gotest.tools/assert" @@ -32,7 +33,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { systemOpts.GraphRoot = home systemOpts.RunRoot = runhome - storageOpts, err := getRootlessStorageOpts(os.Geteuid(), systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) expectedDriver := vfsDriver @@ -45,7 +46,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=btrfs", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = "btrfs" - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, "btrfs") }) @@ -53,7 +54,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=overlay", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = overlayDriver - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, overlayDriver) }) @@ -61,7 +62,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=overlay2", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = "overlay2" - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, overlayDriver) }) @@ -69,7 +70,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=vfs", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = vfsDriver - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, vfsDriver) }) @@ -77,7 +78,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=aufs", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = "aufs" - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Assert(t, storageOpts.GraphDriverName == overlayDriver || storageOpts.GraphDriverName == vfsDriver, fmt.Sprintf("The rootless driver should be set to 'overlay' or 'vfs' not '%v'", storageOpts.GraphDriverName)) }) @@ -85,7 +86,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=devmapper", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = "devmapper" - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Assert(t, storageOpts.GraphDriverName == overlayDriver || storageOpts.GraphDriverName == vfsDriver, fmt.Sprintf("The rootless driver should be set to 'overlay' or 'vfs' not '%v'", storageOpts.GraphDriverName)) }) @@ -93,7 +94,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Run("systemDriver=zfs", func(t *testing.T) { systemOpts := StoreOptions{} systemOpts.GraphDriverName = "zfs" - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Assert(t, storageOpts.GraphDriverName == overlayDriver || storageOpts.GraphDriverName == vfsDriver, fmt.Sprintf("The rootless driver should be set to 'overlay' or 'vfs' not '%v'", storageOpts.GraphDriverName)) }) @@ -102,7 +103,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Setenv("STORAGE_DRIVER", "btrfs") systemOpts := StoreOptions{} systemOpts.GraphDriverName = vfsDriver - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, "btrfs") }) @@ -111,7 +112,7 @@ func TestGetRootlessStorageOpts(t *testing.T) { t.Setenv("STORAGE_DRIVER", "zfs") systemOpts := StoreOptions{} systemOpts.GraphDriverName = vfsDriver - storageOpts, err := getRootlessStorageOpts(1000, systemOpts) + storageOpts, err := getRootlessStorageOpts(systemOpts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphDriverName, "zfs") }) @@ -127,8 +128,8 @@ func TestGetRootlessStorageOpts2(t *testing.T) { opts := StoreOptions{ RootlessStoragePath: "/$HOME/$UID/containers/storage", } - expectedPath := filepath.Join(os.Getenv("HOME"), "2000", "containers/storage") - storageOpts, err := getRootlessStorageOpts(2000, opts) + expectedPath := filepath.Join(os.Getenv("HOME"), fmt.Sprintf("%d", unshare.GetRootlessUID()), "containers/storage") + storageOpts, err := getRootlessStorageOpts(opts) assert.NilError(t, err) assert.Equal(t, storageOpts.GraphRoot, expectedPath) } diff --git a/types/utils.go b/types/utils.go index 8dfc107f3c..5b4b31b80a 100644 --- a/types/utils.go +++ b/types/utils.go @@ -22,7 +22,7 @@ func expandEnvPath(path string, rootlessUID int) (string, error) { return newpath, nil } -func DefaultConfigFile(rootless bool) (string, error) { +func DefaultConfigFile() (string, error) { if defaultConfigFileSet { return defaultConfigFile, nil } @@ -30,7 +30,7 @@ func DefaultConfigFile(rootless bool) (string, error) { if path, ok := os.LookupEnv(storageConfEnv); ok { return path, nil } - if !rootless { + if !usePerUserStorage() { if _, err := os.Stat(defaultOverrideConfigFile); err == nil { return defaultOverrideConfigFile, nil } diff --git a/types/utils_test.go b/types/utils_test.go index 62acd54608..124ad877b7 100644 --- a/types/utils_test.go +++ b/types/utils_test.go @@ -1,17 +1,21 @@ package types import ( + "fmt" "os" "path/filepath" "testing" + "github.com/containers/storage/pkg/unshare" "gotest.tools/assert" ) func TestDefaultStoreOpts(t *testing.T) { - storageOpts, err := defaultStoreOptionsIsolated(true, 1000, "./storage_test.conf") - - expectedPath := filepath.Join(os.Getenv("HOME"), "1000", "containers/storage") + if !usePerUserStorage() { + t.Skip() + } + storageOpts, err := loadStoreOptionsFromConfFile("./storage_test.conf") + expectedPath := filepath.Join(os.Getenv("HOME"), fmt.Sprintf("%d", unshare.GetRootlessUID()), "containers/storage") assert.NilError(t, err) assert.Equal(t, storageOpts.RunRoot, expectedPath) @@ -21,7 +25,7 @@ func TestDefaultStoreOpts(t *testing.T) { func TestStorageConfOverrideEnvironmentDefaultConfigFileRootless(t *testing.T) { t.Setenv("CONTAINERS_STORAGE_CONF", "default_override_test.conf") - defaultFile, err := DefaultConfigFile(true) + defaultFile, err := DefaultConfigFile() expectedPath := "default_override_test.conf" @@ -31,7 +35,7 @@ func TestStorageConfOverrideEnvironmentDefaultConfigFileRootless(t *testing.T) { func TestStorageConfOverrideEnvironmentDefaultConfigFileRoot(t *testing.T) { t.Setenv("CONTAINERS_STORAGE_CONF", "default_override_test.conf") - defaultFile, err := DefaultConfigFile(false) + defaultFile, err := DefaultConfigFile() expectedPath := "default_override_test.conf" diff --git a/utils.go b/utils.go index 6e842c5480..5bade6ffe3 100644 --- a/utils.go +++ b/utils.go @@ -11,14 +11,9 @@ func ParseIDMapping(UIDMapSlice, GIDMapSlice []string, subUIDMap, subGIDMap stri return types.ParseIDMapping(UIDMapSlice, GIDMapSlice, subUIDMap, subGIDMap) } -// DefaultStoreOptionsAutoDetectUID returns the default storage options for containers -func DefaultStoreOptionsAutoDetectUID() (types.StoreOptions, error) { - return types.DefaultStoreOptionsAutoDetectUID() -} - // DefaultStoreOptions returns the default storage options for containers -func DefaultStoreOptions(rootless bool, rootlessUID int) (types.StoreOptions, error) { - return types.DefaultStoreOptions(rootless, rootlessUID) +func DefaultStoreOptions() (types.StoreOptions, error) { + return types.DefaultStoreOptions() } func validateMountOptions(mountOptions []string) error {