From 40e22ba1defbc61cddba150aa6f07098588b01f3 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 4 Aug 2025 14:22:52 +0200 Subject: [PATCH 1/9] pkg/config: simplify addConfigs() There is no reason to use filepath.Walk() as we do not want to recurse at all as such this just makes the code harder to follow. We can just use os.ReadDir() which also sorts the result already so there is no need to sort ourselves again. Signed-off-by: Paul Holzinger --- pkg/config/new.go | 49 ++++++++++++++++------------------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/pkg/config/new.go b/pkg/config/new.go index 407a685df..40f6287de 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -6,7 +6,6 @@ import ( "io/fs" "os" "path/filepath" - "sort" "strings" "sync" @@ -186,38 +185,24 @@ func systemConfigs() (configs []string, finalErr error) { } // addConfigs will search one level in the config dirPath for config files -// If the dirPath does not exist, addConfigs will return nil +// If the dirPath does not exist, addConfigs will return configs, nil. func addConfigs(dirPath string, configs []string) ([]string, error) { - newConfigs := []string{} - - err := filepath.WalkDir(dirPath, - // WalkFunc to read additional configs - func(path string, d fs.DirEntry, err error) error { - switch { - case err != nil: - // return error (could be a permission problem) - return err - case d.IsDir(): - if path != dirPath { - // make sure to not recurse into sub-directories - return filepath.SkipDir - } - // ignore directories - return nil - default: - // only add *.conf files - if strings.HasSuffix(path, ".conf") { - newConfigs = append(newConfigs, path) - } - return nil - } - }, - ) - if errors.Is(err, os.ErrNotExist) { - err = nil - } - sort.Strings(newConfigs) - return append(configs, newConfigs...), err + // Note: ReadDir already sorts the result by filename so we do not have to sort again. + entries, err := os.ReadDir(dirPath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return configs, nil + } + return nil, err + } + newConfigs := make([]string, 0, len(entries)) + for _, entry := range entries { + if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".conf") { + newConfigs = append(newConfigs, filepath.Join(dirPath, entry.Name())) + } + } + + return append(configs, newConfigs...), nil } // readConfigFromFile reads the specified config file at `path` and attempts to From 37e9def0d9182136d9276ed7bb416126b9df1cd8 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 4 Aug 2025 14:54:14 +0200 Subject: [PATCH 2/9] pkg/config: unexport config paths They should be no need for external consumers to directly access these. Signed-off-by: Paul Holzinger --- pkg/config/config.go | 4 ++-- pkg/config/config_bsd.go | 8 ++++---- pkg/config/config_darwin.go | 8 ++++---- pkg/config/config_linux.go | 8 ++++---- pkg/config/config_unix.go | 4 ++-- pkg/config/config_windows.go | 4 ++-- pkg/config/new.go | 2 +- 7 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index f904eb2db..305e11636 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -21,8 +21,8 @@ import ( ) const ( - // UserOverrideContainersConfig holds the containers config path overridden by the rootless user - UserOverrideContainersConfig = ".config/" + _configPath + // userOverrideContainersConfig holds the containers config path overridden by the rootless user. + userOverrideContainersConfig = ".config/" + _configPath // Token prefix for looking for helper binary under $BINDIR bindirPrefix = "$BINDIR" ) diff --git a/pkg/config/config_bsd.go b/pkg/config/config_bsd.go index 9f3513a67..fdade6e4e 100644 --- a/pkg/config/config_bsd.go +++ b/pkg/config/config_bsd.go @@ -3,11 +3,11 @@ package config const ( - // OverrideContainersConfig holds the default config path overridden by the root user - OverrideContainersConfig = "/usr/local/etc/" + _configPath + // overrideContainersConfig holds the default config path overridden by the root user. + overrideContainersConfig = "/usr/local/etc/" + _configPath - // DefaultContainersConfig holds the default containers config path - DefaultContainersConfig = "/usr/local/share/" + _configPath + // defaultContainersConfig holds the default containers config path. + defaultContainersConfig = "/usr/local/share/" + _configPath // DefaultSignaturePolicyPath is the default value for the // policy.json file. diff --git a/pkg/config/config_darwin.go b/pkg/config/config_darwin.go index 9982d7995..f48f53536 100644 --- a/pkg/config/config_darwin.go +++ b/pkg/config/config_darwin.go @@ -1,11 +1,11 @@ package config const ( - // OverrideContainersConfig holds the default config path overridden by the root user - OverrideContainersConfig = "/etc/" + _configPath + // overrideContainersConfig holds the default config path overridden by the root user. + overrideContainersConfig = "/etc/" + _configPath - // DefaultContainersConfig holds the default containers config path - DefaultContainersConfig = "/usr/share/" + _configPath + // defaultContainersConfig holds the default containers config path. + defaultContainersConfig = "/usr/share/" + _configPath // DefaultSignaturePolicyPath is the default value for the // policy.json file. diff --git a/pkg/config/config_linux.go b/pkg/config/config_linux.go index d7a241de8..fc794cbd6 100644 --- a/pkg/config/config_linux.go +++ b/pkg/config/config_linux.go @@ -6,11 +6,11 @@ import ( ) const ( - // OverrideContainersConfig holds the default config path overridden by the root user - OverrideContainersConfig = "/etc/" + _configPath + // overrideContainersConfig holds the default config path overridden by the root user. + overrideContainersConfig = "/etc/" + _configPath - // DefaultContainersConfig holds the default containers config path - DefaultContainersConfig = "/usr/share/" + _configPath + // defaultContainersConfig holds the default containers config path. + defaultContainersConfig = "/usr/share/" + _configPath // DefaultSignaturePolicyPath is the default value for the // policy.json file. diff --git a/pkg/config/config_unix.go b/pkg/config/config_unix.go index 7de35062d..1481086ae 100644 --- a/pkg/config/config_unix.go +++ b/pkg/config/config_unix.go @@ -25,11 +25,11 @@ func userConfigPath() (string, error) { return "", err } - return filepath.Join(home, UserOverrideContainersConfig), nil + return filepath.Join(home, userOverrideContainersConfig), nil } // overrideContainersConfigPath returns the default config path overridden // by the root user func overrideContainersConfigPath() (string, error) { - return OverrideContainersConfig, nil + return overrideContainersConfig, nil } diff --git a/pkg/config/config_windows.go b/pkg/config/config_windows.go index b2cd751a1..bbaac5ad5 100644 --- a/pkg/config/config_windows.go +++ b/pkg/config/config_windows.go @@ -7,8 +7,8 @@ const ( // inside a given config directory. _configPath = "\\containers\\containers.conf" - // DefaultContainersConfig holds the default containers config path - DefaultContainersConfig = "" + // defaultContainersConfig holds the default containers config path + defaultContainersConfig = "" // DefaultSignaturePolicyPath is the default value for the // policy.json file. diff --git a/pkg/config/new.go b/pkg/config/new.go index 40f6287de..2af405005 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -158,7 +158,7 @@ func systemConfigs() (configs []string, finalErr error) { return append(configs, path), nil } - configs = append(configs, DefaultContainersConfig) + configs = append(configs, defaultContainersConfig) var err error path, err := overrideContainersConfigPath() From 85ac702e2a4f1811820e8c61c58c4af84dc7004e Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 4 Aug 2025 18:37:06 +0200 Subject: [PATCH 3/9] pkg/config: add systemd wide rootless search locations Add three new search location when running rootless for the config file: - /etc/containers/containers.rootless.conf - /etc/containers/containers.rootless.conf.d - /etc/containers/containers.rootless.conf.d// The idea is that this can be set by system administrators to define default config for all rootless users without setting the options for the rootful podman. Today /etc/containers/containers.conf is already used as rootless but its options are also read when podman is used as root which made it not useable for some users. Signed-off-by: Paul Holzinger --- docs/containers.conf.5.md | 4 +++- pkg/config/new.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/containers.conf.5.md b/docs/containers.conf.5.md index 4af6df59f..ecd14a0f4 100644 --- a/docs/containers.conf.5.md +++ b/docs/containers.conf.5.md @@ -11,7 +11,9 @@ a TOML format that can be easily modified and versioned. Container engines read the __/usr/share/containers/containers.conf__, __/etc/containers/containers.conf__, and __/etc/containers/containers.conf.d/\*.conf__ -for global configuration that effects all users. +for global configuration that affects all users. +For global configuration that only affects rootless users use __/etc/containers/containers.rootless.conf__, +__/etc/containers/containers.rootless.d/\*.conf__ and __/etc/containers/containers.rootless.d/\$UID/\*.conf__. The UID is the user's uid which podman runs under so it can be used to specify a certain config for only a single user without having to put the config into the user's home directory. For user specific configuration it reads __\$XDG_CONFIG_HOME/containers/containers.conf__ and __\$XDG_CONFIG_HOME/containers/containers.conf.d/\*.conf__ files. When `$XDG_CONFIG_HOME` is not set it falls back to using `$HOME/.config` instead. diff --git a/pkg/config/new.go b/pkg/config/new.go index 2af405005..1e8555704 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -6,11 +6,13 @@ import ( "io/fs" "os" "path/filepath" + "strconv" "strings" "sync" "github.com/BurntSushi/toml" "github.com/containers/storage/pkg/fileutils" + "github.com/containers/storage/pkg/unshare" "github.com/sirupsen/logrus" ) @@ -172,6 +174,25 @@ func systemConfigs() (configs []string, finalErr error) { return nil, err } + // add rootless specific file overwrites in a global system dir + // /etc/containers/containers.rootless.conf + // /etc/containers/containers.rootless.conf.d/ + // /etc/containers/containers.rootless.conf.d// + uid := unshare.GetRootlessUID() + if uid > 0 { + rootlessOverwritePath := filepath.Join(filepath.Dir(path), "containers.rootless.conf") + configs = append(configs, rootlessOverwritePath) + rootlessOverwritePathD := rootlessOverwritePath + ".d" + configs, err = addConfigs(rootlessOverwritePathD, configs) + if err != nil { + return nil, err + } + configs, err = addConfigs(filepath.Join(rootlessOverwritePathD, strconv.Itoa(uid)), configs) + if err != nil { + return nil, err + } + } + path, err = userConfigPath() if err != nil { return nil, err From a11d41e6bfb385e42ca3ca364f48953e16303767 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 14:51:35 +0200 Subject: [PATCH 4/9] pkg/config: make newLocked() unit testable Define a helper struct which contains the search paths. That way unit tests can mock the paths by setting them to something else so we don't actually end up reading the paths under /usr and /etc and $HOME. Signed-off-by: Paul Holzinger --- pkg/config/new.go | 71 ++++++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 25 deletions(-) diff --git a/pkg/config/new.go b/pkg/config/new.go index 1e8555704..215086a83 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -41,6 +41,31 @@ type Options struct { additionalConfigs []string } +// paths defines the search paths used for config reading. +type paths struct { + usr string + etc string + home string + uid int +} + +func defaultPaths() (*paths, error) { + etcPath, err := overrideContainersConfigPath() + if err != nil { + return nil, err + } + homePath, err := userConfigPath() + if err != nil { + return nil, err + } + return &paths{ + usr: defaultContainersConfig, + etc: etcPath, + home: homePath, + uid: unshare.GetRootlessUID(), + }, nil +} + // New returns a Config as described in the containers.conf(5) man page. func New(options *Options) (*Config, error) { if options == nil { @@ -49,7 +74,11 @@ func New(options *Options) (*Config, error) { cachedConfigMutex.Lock() defer cachedConfigMutex.Unlock() } - return newLocked(options) + paths, err := defaultPaths() + if err != nil { + return nil, err + } + return newLocked(options, paths) } // Default returns the default container config. If no default config has been @@ -62,13 +91,17 @@ func Default() (*Config, error) { if cachedConfig != nil || cachedConfigError != nil { return cachedConfig, cachedConfigError } - cachedConfig, cachedConfigError = newLocked(&Options{SetDefault: true}) + paths, err := defaultPaths() + if err != nil { + return nil, err + } + cachedConfig, cachedConfigError = newLocked(&Options{SetDefault: true}, paths) return cachedConfig, cachedConfigError } // A helper function for New() expecting the caller to hold the // cachedConfigMutex if options.SetDefault is set.. -func newLocked(options *Options) (*Config, error) { +func newLocked(options *Options, paths *paths) (*Config, error) { // Start with the built-in defaults config, err := defaultConfig() if err != nil { @@ -76,7 +109,7 @@ func newLocked(options *Options) (*Config, error) { } // Now, gather the system configs and merge them as needed. - configs, err := systemConfigs() + configs, err := systemConfigs(paths) if err != nil { return nil, fmt.Errorf("finding config on system: %w", err) } @@ -152,7 +185,7 @@ func NewConfig(userConfigPath string) (*Config, error) { // Returns the list of configuration files, if they exist in order of hierarchy. // The files are read in order and each new file can/will override previous // file settings. -func systemConfigs() (configs []string, finalErr error) { +func systemConfigs(paths *paths) (configs []string, finalErr error) { if path := os.Getenv(containersConfEnv); path != "" { if err := fileutils.Exists(path); err != nil { return nil, fmt.Errorf("%s file: %w", containersConfEnv, err) @@ -160,16 +193,10 @@ func systemConfigs() (configs []string, finalErr error) { return append(configs, path), nil } - configs = append(configs, defaultContainersConfig) - - var err error - path, err := overrideContainersConfigPath() - if err != nil { - return nil, err - } - configs = append(configs, path) + configs = append(configs, paths.usr) + configs = append(configs, paths.etc) - configs, err = addConfigs(path+".d", configs) + configs, err := addConfigs(paths.etc+".d", configs) if err != nil { return nil, err } @@ -178,27 +205,21 @@ func systemConfigs() (configs []string, finalErr error) { // /etc/containers/containers.rootless.conf // /etc/containers/containers.rootless.conf.d/ // /etc/containers/containers.rootless.conf.d// - uid := unshare.GetRootlessUID() - if uid > 0 { - rootlessOverwritePath := filepath.Join(filepath.Dir(path), "containers.rootless.conf") + if paths.uid > 0 { + rootlessOverwritePath := filepath.Join(filepath.Dir(paths.etc), "containers.rootless.conf") configs = append(configs, rootlessOverwritePath) rootlessOverwritePathD := rootlessOverwritePath + ".d" configs, err = addConfigs(rootlessOverwritePathD, configs) if err != nil { return nil, err } - configs, err = addConfigs(filepath.Join(rootlessOverwritePathD, strconv.Itoa(uid)), configs) + configs, err = addConfigs(filepath.Join(rootlessOverwritePathD, strconv.Itoa(paths.uid)), configs) if err != nil { return nil, err } } - - path, err = userConfigPath() - if err != nil { - return nil, err - } - configs = append(configs, path) - configs, err = addConfigs(path+".d", configs) + configs = append(configs, paths.home) + configs, err = addConfigs(paths.home+".d", configs) if err != nil { return nil, err } From 2aaff5f82d53d54403e50164b7b8debd51a7d479 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 15:34:25 +0200 Subject: [PATCH 5/9] pkg/config: use newLocked() for tests This allows us to fully control the test paths so it doesn't read global system paths which can break unit tests. Signed-off-by: Paul Holzinger --- pkg/config/config_local_test.go | 80 ++++++++++++++++---------------- pkg/config/config_remote_test.go | 2 +- pkg/config/config_test.go | 25 +++++----- pkg/config/connections_test.go | 8 ++-- pkg/config/modules_test.go | 14 +++--- 5 files changed, 63 insertions(+), 66 deletions(-) diff --git a/pkg/config/config_local_test.go b/pkg/config/config_local_test.go index fd31854d1..6dbcb7333 100644 --- a/pkg/config/config_local_test.go +++ b/pkg/config/config_local_test.go @@ -90,7 +90,7 @@ var _ = Describe("Config Local", func() { }) It("parse network subnet pool", func() { - config, err := NewConfig("testdata/containers_default.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) net1, _ := types.ParseCIDR("10.89.0.0/16") @@ -108,11 +108,11 @@ var _ = Describe("Config Local", func() { It("parse dns port", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Network.DNSBindPort).To(gomega.Equal(uint16(0))) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Network.DNSBindPort).To(gomega.Equal(uint16(1153))) @@ -120,11 +120,11 @@ var _ = Describe("Config Local", func() { It("test firewall", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Network.FirewallDriver).To(gomega.Equal(string(""))) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Network.FirewallDriver).To(gomega.Equal("none")) @@ -132,11 +132,11 @@ var _ = Describe("Config Local", func() { It("parse pasta_options", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Network.PastaOptions.Get()).To(gomega.BeEmpty()) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Network.PastaOptions.Get()).To(gomega.Equal([]string{"-t", "auto"})) @@ -144,11 +144,11 @@ var _ = Describe("Config Local", func() { It("parse default_rootless_network_cmd", func() { // Given - config, err := NewConfig("") + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Network.DefaultRootlessNetworkCmd).To(gomega.Equal("pasta")) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Network.DefaultRootlessNetworkCmd).To(gomega.Equal("slirp4netns")) @@ -338,7 +338,7 @@ var _ = Describe("Config Local", func() { // Given expectedEnv := []string{"super=duper", "foo=bar"} // When - config, err := NewConfig("testdata/containers_default.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.Env.Get()).To(gomega.BeEquivalentTo(expectedEnv)) @@ -348,25 +348,25 @@ var _ = Describe("Config Local", func() { It("should override cdi_spec_dirs if provided", func() { // Given - config1, err := New(nil) + config1, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config1.Engine.CdiSpecDirs.Get()).To(gomega.Equal([]string{"/etc/cdi", "/var/run/cdi"})) // Given default just get default - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Engine.CdiSpecDirs.Get()).To(gomega.Equal([]string{"/etc/cdi", "/var/run/cdi"})) // Given override just get override - config3, err := NewConfig("testdata/containers_override.conf") + config3, err := newLocked(&Options{}, &paths{etc: "testdata/containers_override.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config3.Engine.CdiSpecDirs.Get()).To(gomega.Equal([]string{"/somepath"})) // Given override just get override - config4, err := NewConfig("testdata/containers_override2.conf") + config4, err := newLocked(&Options{}, &paths{etc: "testdata/containers_override2.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config4.Engine.CdiSpecDirs.Get()).To(gomega.Equal([]string{"/somepath", "/some_other_path"})) @@ -375,7 +375,7 @@ var _ = Describe("Config Local", func() { It("Expect Remote to be False", func() { // Given // When - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.Remote).To(gomega.BeFalse()) @@ -390,7 +390,7 @@ var _ = Describe("Config Local", func() { t.Setenv(containersConfEnv, "/dev/null") // When - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) @@ -412,7 +412,7 @@ var _ = Describe("Config Local", func() { It("Default Umask", func() { // Given // When - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.Umask).To(gomega.Equal("0022")) @@ -420,7 +420,7 @@ var _ = Describe("Config Local", func() { It("Set Umask", func() { // Given // When - config, err := NewConfig("testdata/containers_default.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.Umask).To(gomega.Equal("0002")) @@ -442,11 +442,11 @@ var _ = Describe("Config Local", func() { It("default netns", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.NetNS).To(gomega.Equal("private")) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Containers.NetNS).To(gomega.Equal("bridge")) @@ -456,7 +456,7 @@ var _ = Describe("Config Local", func() { // Given path := "" // When - config, err := NewConfig(path) + config, err := newLocked(&Options{}, &paths{etc: path}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) // Then gomega.Expect(config.Secrets.Driver).To(gomega.Equal("file")) @@ -466,7 +466,7 @@ var _ = Describe("Config Local", func() { // Given path := "testdata/containers_override.conf" // When - config, err := NewConfig(path) + config, err := newLocked(&Options{}, &paths{etc: path}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) // Then gomega.Expect(config.Secrets.Driver).To(gomega.Equal("pass")) @@ -478,11 +478,11 @@ var _ = Describe("Config Local", func() { It("Set machine image path", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Machine.Image).To(gomega.Equal("")) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) path := "https://example.com/$OS/$ARCH/foobar.ami" @@ -493,11 +493,11 @@ var _ = Describe("Config Local", func() { It("CompatAPIEnforceDockerHub", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.CompatAPIEnforceDockerHub).To(gomega.BeTrue()) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Engine.CompatAPIEnforceDockerHub).To(gomega.BeFalse()) @@ -505,11 +505,11 @@ var _ = Describe("Config Local", func() { It("ComposeProviders", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.ComposeProviders.Get()).To(gomega.Equal(getDefaultComposeProviders())) // no hard-coding to work on all platforms // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Engine.ComposeProviders.Get()).To(gomega.Equal([]string{"/some/thing/else", "/than/before"})) @@ -517,11 +517,11 @@ var _ = Describe("Config Local", func() { It("AddCompression", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.AddCompression.Get()).To(gomega.BeEmpty()) // no hard-coding to work on all platforms // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Engine.AddCompression.Get()).To(gomega.Equal([]string{"zstd", "zstd:chunked"})) @@ -529,11 +529,11 @@ var _ = Describe("Config Local", func() { It("ComposeWarningLogs", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Engine.ComposeWarningLogs).To(gomega.BeTrue()) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Engine.ComposeWarningLogs).To(gomega.BeFalse()) @@ -541,11 +541,11 @@ var _ = Describe("Config Local", func() { It("Set machine disk", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Machine.DiskSize).To(gomega.Equal(uint64(100))) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Machine.DiskSize).To(gomega.Equal(uint64(20))) @@ -557,33 +557,33 @@ var _ = Describe("Config Local", func() { cpus = 1 } - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Machine.CPUs).To(gomega.Equal(uint64(cpus))) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Machine.CPUs).To(gomega.Equal(uint64(1))) }) It("Set machine memory", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Machine.Memory).To(gomega.Equal(uint64(2048))) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Machine.Memory).To(gomega.Equal(uint64(1024))) }) It("Get Rosetta value", func() { // Given - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Machine.Rosetta).To(gomega.BeTrue()) // When - config2, err := NewConfig("testdata/containers_default.conf") + config2, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config2.Machine.Rosetta).To(gomega.BeFalse()) diff --git a/pkg/config/config_remote_test.go b/pkg/config/config_remote_test.go index e8f02e616..f23eaf8c7 100644 --- a/pkg/config/config_remote_test.go +++ b/pkg/config/config_remote_test.go @@ -100,7 +100,7 @@ var _ = Describe("Config Remote", func() { It("Expect Remote to be true", func() { // Given // When - config, err := New(nil) + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).To(gomega.BeNil()) gomega.Expect(config.Engine.Remote).To(gomega.BeTrue()) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f39a38cdc..1a3ba378c 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -22,7 +22,7 @@ var _ = Describe("Config", func() { It("should succeed with default config", func() { // Given // When - defaultConfig, err := NewConfig("") + defaultConfig, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) @@ -130,7 +130,7 @@ var _ = Describe("Config", func() { }) It("Check SELinux settings", func() { - defaultConfig, _ := NewConfig("") + defaultConfig, _ := newLocked(&Options{}, &paths{}) // EnableLabeling should match whether or not SELinux is enabled on the host gomega.Expect(defaultConfig.Containers.EnableLabeling).To(gomega.Equal(selinux.GetEnabled())) gomega.Expect(defaultConfig.Containers.EnableLabeledUsers).To(gomega.BeFalse()) @@ -140,7 +140,7 @@ var _ = Describe("Config", func() { // Note: Podmansh.Timeout must be preferred over Engine.PodmanshTimeout // Given - defaultConfig, _ := NewConfig("") + defaultConfig, _ := newLocked(&Options{}, &paths{}) // When defaultConfig.Engine.PodmanshTimeout = 30 defaultConfig.Podmansh.Timeout = 0 @@ -190,7 +190,7 @@ image_copy_tmp_dir="storage"` // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) - config, _ := NewConfig(testFile) + config, _ := newLocked(&Options{}, &paths{etc: testFile}) path, err := config.ImageCopyTmpDir() gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(path).To(gomega.ContainSubstring("containers/storage/tmp")) @@ -446,7 +446,7 @@ image_copy_tmp_dir="storage"` // Given we do GinkgoT().Setenv(containersConfEnv, "/dev/null") // When - config, err := NewConfig("") + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal(apparmor.Profile)) @@ -484,7 +484,7 @@ image_copy_tmp_dir="storage"` It("should success with valid user file path", func() { // Given // When - config, err := NewConfig("testdata/containers_default.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_default.conf"}) // Then cgroupConf := []string{ "memory.high=1073741824", @@ -504,10 +504,7 @@ image_copy_tmp_dir="storage"` }) It("contents of passed-in file should override others", func() { - // Given we do - GinkgoT().Setenv(containersConfEnv, "containers.conf") - // When - config, err := NewConfig("testdata/containers_override.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_override.conf"}) crunWasm := "crun-wasm" PlatformToOCIRuntimeMap := map[string]string{ @@ -546,7 +543,7 @@ image_copy_tmp_dir="storage"` It("should fail with invalid value", func() { // Given // When - config, err := NewConfig("testdata/containers_invalid.conf") + config, err := newLocked(&Options{}, &paths{etc: "testdata/containers_invalid.conf"}) // Then gomega.Expect(err).To(gomega.HaveOccurred()) gomega.Expect(config).To(gomega.BeNil()) @@ -558,7 +555,7 @@ image_copy_tmp_dir="storage"` Skip(fmt.Sprintf("capabilities not supported on %s", runtime.GOOS)) } // When - config, err := NewConfig("") + config, err := newLocked(&Options{}, &paths{}) // Then gomega.Expect(err).ToNot(gomega.HaveOccurred()) var addcaps, dropcaps []string @@ -791,13 +788,13 @@ env=["foo=bar"]` It("CONTAINERS_CONF_OVERRIDE", func() { t := GinkgoT() t.Setenv("CONTAINERS_CONF_OVERRIDE", "testdata/containers_override.conf") - config, err := NewConfig("") + config, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal("overridden-default")) // Make sure that _OVERRIDE is loaded even when CONTAINERS_CONF is set. t.Setenv(containersConfEnv, "testdata/containers_default.conf") - config, err = NewConfig("") + config, err = newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal("overridden-default")) gomega.Expect(config.Containers.BaseHostsFile).To(gomega.Equal("/etc/hosts2")) diff --git a/pkg/config/connections_test.go b/pkg/config/connections_test.go index 1c4af7cf2..8021d2305 100644 --- a/pkg/config/connections_test.go +++ b/pkg/config/connections_test.go @@ -128,7 +128,7 @@ var _ = Describe("Connections conf", func() { }) It("GetConnection()", func() { - conf, err := New(nil) + conf, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) con, err := conf.GetConnection("", true) @@ -169,7 +169,7 @@ var _ = Describe("Connections conf", func() { }) It("GetAllConnections()", func() { - conf, err := New(nil) + conf, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) cons, err := conf.GetAllConnections() @@ -197,7 +197,7 @@ var _ = Describe("Connections conf", func() { }) It("GetFarmConnections()", func() { - conf, err := New(nil) + conf, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) name, cons, err := conf.GetDefaultFarmConnections() @@ -236,7 +236,7 @@ var _ = Describe("Connections conf", func() { }) It("GetAllFarms()", func() { - conf, err := New(nil) + conf, err := newLocked(&Options{}, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) farms, err := conf.GetAllFarms() diff --git a/pkg/config/modules_test.go b/pkg/config/modules_test.go index e9aafcbd1..4f1a0ef4c 100644 --- a/pkg/config/modules_test.go +++ b/pkg/config/modules_test.go @@ -104,18 +104,18 @@ var _ = Describe("Config Modules", func() { gomega.Expect(err).ToNot(gomega.HaveOccurred()) options := &Options{Modules: []string{"none.conf"}} - _, err = New(options) + _, err = newLocked(options, &paths{}) gomega.Expect(err).To(gomega.HaveOccurred()) // must error out options = &Options{} - c, err := New(options) + c, err := newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.BeEmpty()) // no module is getting loaded! gomega.Expect(c).NotTo(gomega.BeNil()) gomega.Expect(c.LoadedModules()).To(gomega.BeEmpty()) options = &Options{Modules: []string{"fourth.conf"}} - c, err = New(options) + c, err = newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) @@ -124,14 +124,14 @@ var _ = Describe("Config Modules", func() { gomega.Expect(c.LoadedModules()).To(gomega.Equal([]string{filepath.Join(wd, "testdata/modules/etc/containers/containers.conf.modules/fourth.conf")})) options = &Options{Modules: []string{"fourth.conf"}} - c, err = New(options) + c, err = newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) options = &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", "sub/etc-only.conf"}} - c, err = New(options) + c, err = newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(3)) // 3 modules are getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) @@ -140,7 +140,7 @@ var _ = Describe("Config Modules", func() { gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(3)) options = &Options{Modules: []string{"third.conf"}} - c, err = New(options) + c, err = newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) @@ -163,7 +163,7 @@ var _ = Describe("Config Modules", func() { absConf := filepath.Join(wd, "testdata/modules/home/.config/containers/containers.conf.modules/second.conf") options := &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", absConf}} - c, err := New(options) + c, err := newLocked(options, &paths{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(4)) // 2 modules + abs path + override conf are getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) From 5d22b5a27be8a049dc344dac6b3b005559d4d4e4 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 18:11:01 +0200 Subject: [PATCH 6/9] pkg/config: fix modules path lookup module tests are currently actually broken as rootless as the home dir is resolved only once and does not take the test override into account. Instead rewrite modules to use the new paths overwrite struct used by th emain containers.conf tests already. With that we can mock all paths and even root/rootless cases while running as the same user. Signed-off-by: Paul Holzinger --- pkg/config/modules.go | 44 +++++++------------ pkg/config/modules_test.go | 90 ++++++++++++++++++-------------------- pkg/config/new.go | 2 +- 3 files changed, 58 insertions(+), 78 deletions(-) diff --git a/pkg/config/modules.go b/pkg/config/modules.go index 4f23694b7..fae784ac7 100644 --- a/pkg/config/modules.go +++ b/pkg/config/modules.go @@ -5,21 +5,9 @@ import ( "path/filepath" "github.com/containers/storage/pkg/fileutils" - "github.com/containers/storage/pkg/homedir" - "github.com/containers/storage/pkg/unshare" "github.com/hashicorp/go-multierror" ) -// The subdirectory for looking up containers.conf modules. -const moduleSubdir = "containers/containers.conf.modules" - -// Moving the base paths into variables allows for overriding them in units -// tests. -var ( - moduleBaseEtc = "/etc/" - moduleBaseUsr = "/usr/share" -) - // LoadedModules returns absolute paths to loaded containers.conf modules. func (c *Config) LoadedModules() []string { // Required for conmon's callback to Podman's cleanup. @@ -29,15 +17,12 @@ func (c *Config) LoadedModules() []string { // Find the specified modules in the options. Return an error if a specific // module cannot be located on the host. -func (o *Options) modules() ([]string, error) { +func (o *Options) modules(paths *paths) ([]string, error) { if len(o.Modules) == 0 { return nil, nil } - dirs, err := ModuleDirectories() - if err != nil { - return nil, err - } + dirs := moduleDirectories(paths) configs := make([]string, 0, len(o.Modules)) for _, path := range o.Modules { @@ -56,21 +41,22 @@ func (o *Options) modules() ([]string, error) { // 2) /etc/ // 3) /usr/share func ModuleDirectories() ([]string, error) { // Public API for shell completions in Podman - modules := []string{ - filepath.Join(moduleBaseEtc, moduleSubdir), - filepath.Join(moduleBaseUsr, moduleSubdir), - } - - if !unshare.IsRootless() { - return modules, nil - } - - // Prepend the user modules dir. - configHome, err := homedir.GetConfigHome() + paths, err := defaultPaths() if err != nil { return nil, err } - return append([]string{filepath.Join(configHome, moduleSubdir)}, modules...), nil + return moduleDirectories(paths), nil +} + +func moduleDirectories(paths *paths) []string { + const moduleSuffix = ".modules" + modules := make([]string, 0, 3) + if paths.uid > 0 { + modules = append(modules, paths.home+moduleSuffix) + } + modules = append(modules, paths.etc+moduleSuffix) + modules = append(modules, paths.usr+moduleSuffix) + return modules } // Resolve the specified path to a module. diff --git a/pkg/config/modules_test.go b/pkg/config/modules_test.go index 4f1a0ef4c..bf3524216 100644 --- a/pkg/config/modules_test.go +++ b/pkg/config/modules_test.go @@ -10,27 +10,21 @@ import ( ) const ( - testBaseHome = "testdata/modules/home/.config" - testBaseEtc = "testdata/modules/etc" - testBaseUsr = "testdata/modules/usr/share" + testBaseHome = "testdata/modules/home/.config/containers/containers.conf" + testBaseEtc = "testdata/modules/etc/containers/containers.conf" + testBaseUsr = "testdata/modules/usr/share/containers/containers.conf" ) -func testSetModulePaths() { - t := GinkgoT() - +func testSetModulePaths() *paths { wd, err := os.Getwd() gomega.Expect(err).ToNot(gomega.HaveOccurred()) - t.Setenv("XDG_CONFIG_HOME", filepath.Join(wd, testBaseHome)) - - oldEtc := moduleBaseEtc - oldUsr := moduleBaseUsr - moduleBaseEtc = filepath.Join(wd, testBaseEtc) - moduleBaseUsr = filepath.Join(wd, testBaseUsr) - DeferCleanup(func() { - moduleBaseEtc = oldEtc - moduleBaseUsr = oldUsr - }) + return &paths{ + usr: filepath.Join(wd, testBaseUsr), + etc: filepath.Join(wd, testBaseEtc), + home: filepath.Join(wd, testBaseHome), + uid: 1000, + } } var _ = Describe("Config Modules", func() { @@ -49,21 +43,19 @@ var _ = Describe("Config Modules", func() { It("resolve modules", func() { // This test makes sure that the correct module is being // returned. - testSetModulePaths() + paths := testSetModulePaths() - dirs, err := ModuleDirectories() - gomega.Expect(err).ToNot(gomega.HaveOccurred()) + rootlessDirs := moduleDirectories(paths) + gomega.Expect(rootlessDirs).To(gomega.HaveLen(3)) + gomega.Expect(rootlessDirs[0]).To(gomega.ContainSubstring(testBaseHome)) + gomega.Expect(rootlessDirs[1]).To(gomega.ContainSubstring(testBaseEtc)) + gomega.Expect(rootlessDirs[2]).To(gomega.ContainSubstring(testBaseUsr)) - if unshare.IsRootless() { - gomega.Expect(dirs).To(gomega.HaveLen(3)) - gomega.Expect(dirs[0]).To(gomega.ContainSubstring(testBaseHome)) - gomega.Expect(dirs[1]).To(gomega.ContainSubstring(testBaseEtc)) - gomega.Expect(dirs[2]).To(gomega.ContainSubstring(testBaseUsr)) - } else { - gomega.Expect(dirs).To(gomega.HaveLen(2)) - gomega.Expect(dirs[0]).To(gomega.ContainSubstring(testBaseEtc)) - gomega.Expect(dirs[1]).To(gomega.ContainSubstring(testBaseUsr)) - } + paths.uid = 0 + rootfulDirs := moduleDirectories(paths) + gomega.Expect(rootfulDirs).To(gomega.HaveLen(2)) + gomega.Expect(rootfulDirs[0]).To(gomega.ContainSubstring(testBaseEtc)) + gomega.Expect(rootfulDirs[1]).To(gomega.ContainSubstring(testBaseUsr)) for _, test := range []struct { input string @@ -84,8 +76,9 @@ var _ = Describe("Config Modules", func() { {"sub/share-only.conf", testBaseUsr, false, false}, {"none.conf", "", true, false}, } { - if test.rootless && !unshare.IsRootless() { - continue + dirs := rootfulDirs + if test.rootless { + dirs = rootlessDirs } result, err := resolveModule(test.input, dirs) if test.mustFail { @@ -93,29 +86,29 @@ var _ = Describe("Config Modules", func() { continue } gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(result).To(gomega.HaveSuffix(filepath.Join(test.expectedDir, moduleSubdir, test.input))) + gomega.Expect(result).To(gomega.HaveSuffix(filepath.Join(test.expectedDir+".modules", test.input))) } }) It("new config with modules", func() { - testSetModulePaths() + paths := testSetModulePaths() wd, err := os.Getwd() gomega.Expect(err).ToNot(gomega.HaveOccurred()) options := &Options{Modules: []string{"none.conf"}} - _, err = newLocked(options, &paths{}) + _, err = newLocked(options, paths) gomega.Expect(err).To(gomega.HaveOccurred()) // must error out options = &Options{} - c, err := newLocked(options, &paths{}) + c, err := newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.BeEmpty()) // no module is getting loaded! gomega.Expect(c).NotTo(gomega.BeNil()) gomega.Expect(c.LoadedModules()).To(gomega.BeEmpty()) options = &Options{Modules: []string{"fourth.conf"}} - c, err = newLocked(options, &paths{}) + c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) @@ -124,14 +117,14 @@ var _ = Describe("Config Modules", func() { gomega.Expect(c.LoadedModules()).To(gomega.Equal([]string{filepath.Join(wd, "testdata/modules/etc/containers/containers.conf.modules/fourth.conf")})) options = &Options{Modules: []string{"fourth.conf"}} - c, err = newLocked(options, &paths{}) + c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) options = &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", "sub/etc-only.conf"}} - c, err = newLocked(options, &paths{}) + c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(3)) // 3 modules are getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) @@ -140,19 +133,20 @@ var _ = Describe("Config Modules", func() { gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(3)) options = &Options{Modules: []string{"third.conf"}} - c, err = newLocked(options, &paths{}) + c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! - gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) - if unshare.IsRootless() { - gomega.Expect(c.Network.DefaultNetwork).To(gomega.Equal("home third")) - } else { - gomega.Expect(c.Network.DefaultNetwork).To(gomega.Equal("etc third")) - } + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) // 1 module is getting loaded! + gomega.Expect(c.Network.DefaultNetwork).To(gomega.Equal("home third")) + + paths.uid = 0 + c, err = newLocked(options, paths) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) // 1 module is getting loaded! + gomega.Expect(c.Network.DefaultNetwork).To(gomega.Equal("etc third")) }) It("new config with modules and env variables", func() { - testSetModulePaths() + paths := testSetModulePaths() t := GinkgoT() t.Setenv(containersConfOverrideEnv, "testdata/modules/override.conf") @@ -163,7 +157,7 @@ var _ = Describe("Config Modules", func() { absConf := filepath.Join(wd, "testdata/modules/home/.config/containers/containers.conf.modules/second.conf") options := &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", absConf}} - c, err := newLocked(options, &paths{}) + c, err := newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(4)) // 2 modules + abs path + override conf are getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) diff --git a/pkg/config/new.go b/pkg/config/new.go index 215086a83..43abe0ca1 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -125,7 +125,7 @@ func newLocked(options *Options, paths *paths) (*Config, error) { logrus.Tracef("%+v", config) } - modules, err := options.modules() + modules, err := options.modules(paths) if err != nil { return nil, err } From 6c30f0c0792e770d0c9c11b6b1a64d9b183b9df6 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 15:51:02 +0200 Subject: [PATCH 7/9] pkg/config: remove NewConfig() Signed-off-by: Paul Holzinger --- pkg/config/new.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/config/new.go b/pkg/config/new.go index 43abe0ca1..d9de289c3 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -174,14 +174,6 @@ func newLocked(options *Options, paths *paths) (*Config, error) { return config, nil } -// NewConfig creates a new Config. It starts with an empty config and, if -// specified, merges the config at `userConfigPath` path. -// -// Deprecated: use new instead. -func NewConfig(userConfigPath string) (*Config, error) { - return New(&Options{additionalConfigs: []string{userConfigPath}}) -} - // Returns the list of configuration files, if they exist in order of hierarchy. // The files are read in order and each new file can/will override previous // file settings. From f0ed5caf2d87997770398de4ef7fe1d06e31ac7c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 17:12:40 +0200 Subject: [PATCH 8/9] pkg/config: remove additionalConfigs field It is mainly used for tetsing without adding much value. We can check the loaded modules via the LoadedModules() call on the Config. Signed-off-by: Paul Holzinger --- pkg/config/modules_test.go | 14 +++++--------- pkg/config/new.go | 31 ++++++++++++------------------- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/pkg/config/modules_test.go b/pkg/config/modules_test.go index bf3524216..af4c82b6d 100644 --- a/pkg/config/modules_test.go +++ b/pkg/config/modules_test.go @@ -103,34 +103,30 @@ var _ = Describe("Config Modules", func() { options = &Options{} c, err := newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.BeEmpty()) // no module is getting loaded! gomega.Expect(c).NotTo(gomega.BeNil()) - gomega.Expect(c.LoadedModules()).To(gomega.BeEmpty()) + gomega.Expect(c.LoadedModules()).To(gomega.BeEmpty()) // no module is getting loaded! options = &Options{Modules: []string{"fourth.conf"}} c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) - gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) // 1 module is getting loaded! // Make sure the returned module path is absolute. gomega.Expect(c.LoadedModules()).To(gomega.Equal([]string{filepath.Join(wd, "testdata/modules/etc/containers/containers.conf.modules/fourth.conf")})) options = &Options{Modules: []string{"fourth.conf"}} c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(1)) // 1 module is getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) - gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(1)) // 1 module is getting loaded! options = &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", "sub/etc-only.conf"}} c, err = newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(3)) // 3 modules are getting loaded! gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) gomega.Expect(c.Containers.Env.Get()).To(gomega.Equal([]string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", "usr share only"})) gomega.Expect(c.Network.DefaultNetwork).To(gomega.Equal("etc only conf")) - gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(3)) + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(3)) // 3 modules are getting loaded! options = &Options{Modules: []string{"third.conf"}} c, err = newLocked(options, paths) @@ -159,7 +155,7 @@ var _ = Describe("Config Modules", func() { options := &Options{Modules: []string{"fourth.conf", "sub/share-only.conf", absConf}} c, err := newLocked(options, paths) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(options.additionalConfigs).To(gomega.HaveLen(4)) // 2 modules + abs path + override conf are getting loaded! + gomega.Expect(c.LoadedModules()).To(gomega.HaveLen(3)) // 2 modules + abs path gomega.Expect(c.Containers.InitPath).To(gomega.Equal("etc four")) gomega.Expect(c.Containers.Env.Get()).To(gomega.Equal([]string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", "usr share only", "override conf always wins"})) gomega.Expect(c.Containers.Volumes.Get()).To(gomega.Equal([]string{"volume four", "home second"})) diff --git a/pkg/config/new.go b/pkg/config/new.go index d9de289c3..bc4948cbe 100644 --- a/pkg/config/new.go +++ b/pkg/config/new.go @@ -35,10 +35,6 @@ type Options struct { // Set the loaded config as the default one which can later on be // accessed via Default(). SetDefault bool - - // Additional configs to load. An internal only field to make the - // behavior observable and testable in unit tests. - additionalConfigs []string } // paths defines the search paths used for config reading. @@ -131,23 +127,9 @@ func newLocked(options *Options, paths *paths) (*Config, error) { } config.loadedModules = modules - options.additionalConfigs = append(options.additionalConfigs, modules...) - - // The _OVERRIDE variable _must_ always win. That's a contract we need - // to honor (for the Podman CI). - if path := os.Getenv(containersConfOverrideEnv); path != "" { - if err := fileutils.Exists(path); err != nil { - return nil, fmt.Errorf("%s file: %w", containersConfOverrideEnv, err) - } - options.additionalConfigs = append(options.additionalConfigs, path) - } - // If the caller specified a config path to use, then we read it to // override the system defaults. - for _, add := range options.additionalConfigs { - if add == "" { - continue - } + for _, add := range modules { // readConfigFromFile reads in container config in the specified // file and then merge changes with the current default. if err := readConfigFromFile(add, config, false); err != nil { @@ -156,6 +138,17 @@ func newLocked(options *Options, paths *paths) (*Config, error) { logrus.Debugf("Merged additional config %q", add) logrus.Tracef("%+v", config) } + + // The _OVERRIDE variable _must_ always win. That's a contract we need + // to honor (for the Podman CI). + if path := os.Getenv(containersConfOverrideEnv); path != "" { + if err := readConfigFromFile(path, config, true); err != nil { + return nil, fmt.Errorf("reading %s config %q: %w", containersConfOverrideEnv, path, err) + } + logrus.Debugf("Merged %s config %q", containersConfOverrideEnv, path) + logrus.Tracef("%+v", config) + } + config.addCAPPrefix() if err := config.Validate(); err != nil { From 527d27541880f9e2fb5be1732889ae432e139b13 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Aug 2025 20:10:51 +0200 Subject: [PATCH 9/9] pkg/config: add config file search order test Until now there didn't seem to exists a proper that that actually verifies the containers.conf loading order so fix that. The test got a bit complicated but I coud not find something better. Signed-off-by: Paul Holzinger --- pkg/config/search_order_test.go | 135 ++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 pkg/config/search_order_test.go diff --git a/pkg/config/search_order_test.go b/pkg/config/search_order_test.go new file mode 100644 index 000000000..3864cf94c --- /dev/null +++ b/pkg/config/search_order_test.go @@ -0,0 +1,135 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = Describe("search order", func() { + It("config file parsing order", func() { + // High level idea, we define 12 file lookup paths. Then we simulate parsing + // of that file tree for all three cases, uid 0 (root), uid 1000 (reads rootless + // specific config files) and uid 500 (also reads rootless config files but not + // the uid 1000 specific one) + // + // To know that each file is getting parsed we use 12 config values and 12 files, + // each file in the order is populated with one less config value. + // So the first value only exits in the /usr/ file and then the next file writes + // one less config value but instead increments the values by one so we can + // differentiate what value is in what file. + + configPaths := []string{ + "/usr/share/containers/containers.conf", + "/etc/containers/containers.conf", + "/etc/containers/containers.conf.d/1.conf", + "/etc/containers/containers.conf.d/2.conf", + "/etc/containers/containers.rootless.conf", + "/etc/containers/containers.rootless.conf.d/3.conf", + "/etc/containers/containers.rootless.conf.d/4.conf", + "/etc/containers/containers.rootless.conf.d/1000/5.conf", + "/etc/containers/containers.rootless.conf.d/1000/6.conf", + "/home/.config/containers/containers.conf", + "/home/.config/containers/containers.conf.d/7.conf", + "/home/.config/containers/containers.conf.d/8.conf", + } + + configFileValues := []string{ + `apparmor_profile = "apparmor%d"`, + `base_hosts_file = "base%d"`, + `cgroupns = "cgroupns%d"`, + `cgroups = "cgroups%d"`, + `host_containers_internal_ip = "host%d"`, + `init_path = "init%d"`, + `ipcns = "ipcns%d"`, + `log_driver = "logdriver%d"`, + `log_tag = "logtag%d"`, + `netns = "netns%d"`, + `pidns = "pidns%d"`, + `seccomp_profile = "seccomp%d"`, + } + + test := func(config *Config, uid int, testname string) { + gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal("apparmor1"), testname) + gomega.Expect(config.Containers.BaseHostsFile).To(gomega.Equal("base2"), testname) + gomega.Expect(config.Containers.CgroupNS).To(gomega.Equal("cgroupns3"), testname) + gomega.Expect(config.Containers.Cgroups).To(gomega.Equal("cgroups4"), testname) + + // This is a bit confusing but I did not find a way to make it better readable. + // Basically we have three cases here: + // - uid 0 (root): should not read containers.rootless.conf, values must stay at 4 + // - uid != 1000: should read containers.rootless.conf but not + // containers.rootless.conf.d/1000/*.conf, values for the 1000 files must be 7 + // - uid == 1000: all files are read so each value should be incremented by 1 + hostIP := "host5" + initPath := "init6" + ipcns := "ipcns7" + logDriver := "logdriver7" + logTag := "logtag7" + switch uid { + case 0: + hostIP = "host4" + initPath = "init4" + ipcns = "ipcns4" + logDriver = "logdriver4" + logTag = "logtag4" + case 1000: + logDriver = "logdriver8" + logTag = "logtag9" + } + gomega.Expect(config.Containers.HostContainersInternalIP).To(gomega.Equal(hostIP), testname) + gomega.Expect(config.Containers.InitPath).To(gomega.Equal(initPath), testname) + gomega.Expect(config.Containers.IPCNS).To(gomega.Equal(ipcns), testname) + gomega.Expect(config.Containers.LogDriver).To(gomega.Equal(logDriver), testname) + gomega.Expect(config.Containers.LogTag).To(gomega.Equal(logTag), testname) + + gomega.Expect(config.Containers.NetNS).To(gomega.Equal("netns10"), testname) + gomega.Expect(config.Containers.PidNS).To(gomega.Equal("pidns11"), testname) + gomega.Expect(config.Containers.SeccompProfile).To(gomega.Equal("seccomp12"), testname) + } + + // sanity check + gomega.Expect(configFileValues).To(gomega.HaveLen(len(configPaths))) + + tmpdir := GinkgoT().TempDir() + for i, path := range configPaths { + path := filepath.Join(tmpdir, path) + err := os.MkdirAll(filepath.Dir(path), 0o755) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + f, err := os.Create(path) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + _, err = f.WriteString("[containers]\n") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + for _, val := range configFileValues[i:] { + _, err = fmt.Fprintf(f, val+"\n", i+1) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + } + f.Close() + } + + paths := &paths{ + usr: filepath.Join(tmpdir, "/usr/share/containers/containers.conf"), + etc: filepath.Join(tmpdir, "/etc/containers/containers.conf"), + home: filepath.Join(tmpdir, "/home/.config/containers/containers.conf"), + uid: 1000, + } + + config, err := newLocked(&Options{}, paths) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + test(config, 1000, "lookup with uid 1000") + + paths.uid = 500 + config, err = newLocked(&Options{}, paths) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + test(config, 500, "lookup with uid 500") + + paths.uid = 0 + config, err = newLocked(&Options{}, paths) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + test(config, 0, "lookup with uid 0 (root)") + }) +})