From b7b9f8d0cfcf2ec5eaa35d5a09c7527739b74683 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 17 Feb 2020 09:58:40 +0100 Subject: libpod/config: use built-in TOML instead of manually merging Instead of manually merging the configs, use the built-in features of TOMP to merge/extend the fields of a data type when encoding a file. This erases the need for the merge code in libpod/config and also addresses issues when merging booleans. Signed-off-by: Valentin Rothberg --- libpod/config/config.go | 166 ++++++++++++++++++++++----------------- libpod/config/config_test.go | 16 ++-- libpod/config/merge.go | 183 ------------------------------------------- libpod/config/merge_test.go | 157 ------------------------------------- 4 files changed, 99 insertions(+), 423 deletions(-) delete mode 100644 libpod/config/merge.go delete mode 100644 libpod/config/merge_test.go diff --git a/libpod/config/config.go b/libpod/config/config.go index 13c128688..c72a0efc7 100644 --- a/libpod/config/config.go +++ b/libpod/config/config.go @@ -2,7 +2,7 @@ package config import ( "bytes" - "io/ioutil" + "fmt" "os" "os/exec" "path/filepath" @@ -287,18 +287,16 @@ type DBConfig struct { } // readConfigFromFile reads the specified config file at `path` and attempts to -// unmarshal its content into a Config. -func readConfigFromFile(path string) (*Config, error) { - var config Config - - configBytes, err := ioutil.ReadFile(path) +// unmarshal its content into a Config. The config param specifies the previous +// default config. If the path, only specifies a few fields in the Toml file +// the defaults from the config parameter will be used for all other fields. +func readConfigFromFile(path string, config *Config) (*Config, error) { + logrus.Debugf("Reading configuration file %q", path) + _, err := toml.DecodeFile(path, config) if err != nil { - return nil, err + return nil, fmt.Errorf("unable to decode configuration %v: %v", path, err) } - logrus.Debugf("Reading configuration file %q", path) - err = toml.Unmarshal(configBytes, &config) - // For the sake of backwards compat we need to check if the config fields // with *Set suffix are set in the config. Note that the storage-related // fields are NOT set in the config here but in the storage.conf OR directly @@ -313,7 +311,7 @@ func readConfigFromFile(path string) (*Config, error) { config.TmpDirSet = true } - return &config, err + return config, err } // Write decodes the config as TOML and writes it to the specified path. @@ -439,15 +437,11 @@ func probeConmon(conmonBinary string) error { // with cgroupsv2. Other OCI runtimes are not yet supporting cgroupsv2. This // might change in the future. func NewConfig(userConfigPath string) (*Config, error) { - config := &Config{} // start with an empty config - - // First, try to read the user-specified config - if userConfigPath != "" { - var err error - config, err = readConfigFromFile(userConfigPath) - if err != nil { - return nil, errors.Wrapf(err, "error reading user config %q", userConfigPath) - } + // Start with the default config and interatively merge fields in the system + // configs. + config, err := defaultConfigFromMemory() + if err != nil { + return nil, err } // Now, check if the user can access system configs and merge them if needed. @@ -456,44 +450,45 @@ func NewConfig(userConfigPath string) (*Config, error) { return nil, errors.Wrapf(err, "error finding config on system") } - migrated := false for _, path := range configs { - systemConfig, err := readConfigFromFile(path) + config, err = readConfigFromFile(path, config) if err != nil { return nil, errors.Wrapf(err, "error reading system config %q", path) } - // Handle CGroups v2 configuration migration. - // Migrate only the first config, and do it before - // merging. - if !migrated { - if err := cgroupV2Check(path, systemConfig); err != nil { - return nil, errors.Wrapf(err, "error rewriting configuration file %s", userConfigPath) - } - migrated = true - } - // Merge the it into the config. Any unset field in config will be - // over-written by the systemConfig. - if err := config.mergeConfig(systemConfig); err != nil { - return nil, errors.Wrapf(err, "error merging system config") - } - logrus.Debugf("Merged system config %q: %v", path, config) } - // Finally, create a default config from memory and forcefully merge it into - // the config. This way we try to make sure that all fields are properly set - // and that user AND system config can partially set. - defaultConfig, err := defaultConfigFromMemory() - if err != nil { - return nil, errors.Wrapf(err, "error generating default config from memory") + // First, try to read the user-specified config + if userConfigPath != "" { + var err error + config, err = readConfigFromFile(userConfigPath, config) + if err != nil { + return nil, errors.Wrapf(err, "error reading user config %q", userConfigPath) + } } - // Check if we need to switch to cgroupfs and logger=file on rootless. - defaultConfig.checkCgroupsAndLogger() - - if err := config.mergeConfig(defaultConfig); err != nil { - return nil, errors.Wrapf(err, "error merging default config from memory") + // Since runc does not currently support cgroupV2 + // Change to default crun on first running of libpod.conf + // TODO Once runc has support for cgroups, this function should be removed. + if !config.CgroupCheck && rootless.IsRootless() { + cgroupsV2, err := cgroups.IsCgroup2UnifiedMode() + if err != nil { + return nil, err + } + if cgroupsV2 { + path, err := exec.LookPath("crun") + if err != nil { + // Can't find crun path so do nothing + logrus.Warnf("Can not find crun package on the host, containers might fail to run on cgroup V2 systems without crun: %q", err) + } else { + config.CgroupCheck = true + config.OCIRuntime = path + } + } } + // If we need to, switch to cgroupfs and logger=file on rootless. + config.checkCgroupsAndLogger() + // Relative paths can cause nasty bugs, because core paths we use could // shift between runs (or even parts of the program - the OCI runtime // uses a different working directory than we do, for example. @@ -532,12 +527,12 @@ func systemConfigs() ([]string, error) { } configs := []string{} - if _, err := os.Stat(_rootOverrideConfigPath); err == nil { - configs = append(configs, _rootOverrideConfigPath) - } if _, err := os.Stat(_rootConfigPath); err == nil { configs = append(configs, _rootConfigPath) } + if _, err := os.Stat(_rootOverrideConfigPath); err == nil { + configs = append(configs, _rootOverrideConfigPath) + } return configs, nil } @@ -568,29 +563,56 @@ func (c *Config) checkCgroupsAndLogger() { } } -// Since runc does not currently support cgroupV2 -// Change to default crun on first running of libpod.conf -// TODO Once runc has support for cgroups, this function should be removed. -func cgroupV2Check(configPath string, tmpConfig *Config) error { - if !tmpConfig.CgroupCheck && rootless.IsRootless() { - logrus.Debugf("Rewriting %s for CGroup v2 upgrade", configPath) - cgroupsV2, err := cgroups.IsCgroup2UnifiedMode() - if err != nil { - return err +// MergeDBConfig merges the configuration from the database. +func (c *Config) MergeDBConfig(dbConfig *DBConfig) error { + + if !c.StorageConfigRunRootSet && dbConfig.StorageTmp != "" { + if c.StorageConfig.RunRoot != dbConfig.StorageTmp && + c.StorageConfig.RunRoot != "" { + logrus.Debugf("Overriding run root %q with %q from database", + c.StorageConfig.RunRoot, dbConfig.StorageTmp) } - if cgroupsV2 { - path, err := exec.LookPath("crun") - if err != nil { - logrus.Warnf("Can not find crun package on the host, containers might fail to run on cgroup V2 systems without crun: %q", err) - // Can't find crun path so do nothing - return nil - } - tmpConfig.CgroupCheck = true - tmpConfig.OCIRuntime = path - if err := tmpConfig.Write(configPath); err != nil { - return err - } + c.StorageConfig.RunRoot = dbConfig.StorageTmp + } + + if !c.StorageConfigGraphRootSet && dbConfig.StorageRoot != "" { + if c.StorageConfig.GraphRoot != dbConfig.StorageRoot && + c.StorageConfig.GraphRoot != "" { + logrus.Debugf("Overriding graph root %q with %q from database", + c.StorageConfig.GraphRoot, dbConfig.StorageRoot) + } + c.StorageConfig.GraphRoot = dbConfig.StorageRoot + } + + if !c.StorageConfigGraphDriverNameSet && dbConfig.GraphDriver != "" { + if c.StorageConfig.GraphDriverName != dbConfig.GraphDriver && + c.StorageConfig.GraphDriverName != "" { + logrus.Errorf("User-selected graph driver %q overwritten by graph driver %q from database - delete libpod local files to resolve", + c.StorageConfig.GraphDriverName, dbConfig.GraphDriver) + } + c.StorageConfig.GraphDriverName = dbConfig.GraphDriver + } + + if !c.StaticDirSet && dbConfig.LibpodRoot != "" { + if c.StaticDir != dbConfig.LibpodRoot && c.StaticDir != "" { + logrus.Debugf("Overriding static dir %q with %q from database", c.StaticDir, dbConfig.LibpodRoot) + } + c.StaticDir = dbConfig.LibpodRoot + } + + if !c.TmpDirSet && dbConfig.LibpodTmp != "" { + if c.TmpDir != dbConfig.LibpodTmp && c.TmpDir != "" { + logrus.Debugf("Overriding tmp dir %q with %q from database", c.TmpDir, dbConfig.LibpodTmp) + } + c.TmpDir = dbConfig.LibpodTmp + c.EventsLogFilePath = filepath.Join(dbConfig.LibpodTmp, "events", "events.log") + } + + if !c.VolumePathSet && dbConfig.VolumePath != "" { + if c.VolumePath != dbConfig.VolumePath && c.VolumePath != "" { + logrus.Debugf("Overriding volume path %q with %q from database", c.VolumePath, dbConfig.VolumePath) } + c.VolumePath = dbConfig.VolumePath } return nil } diff --git a/libpod/config/config_test.go b/libpod/config/config_test.go index 47c092440..24620ce0e 100644 --- a/libpod/config/config_test.go +++ b/libpod/config/config_test.go @@ -11,14 +11,14 @@ import ( func TestEmptyConfig(t *testing.T) { // Make sure that we can read empty configs - config, err := readConfigFromFile("testdata/empty.conf") + config, err := readConfigFromFile("testdata/empty.conf", &Config{}) assert.NotNil(t, config) assert.Nil(t, err) } func TestDefaultLibpodConf(t *testing.T) { // Make sure that we can read the default libpod.conf - config, err := readConfigFromFile("testdata/libpod.conf") + config, err := readConfigFromFile("testdata/libpod.conf", &Config{}) assert.NotNil(t, config) assert.Nil(t, err) } @@ -32,13 +32,10 @@ func TestMergeEmptyAndDefaultMemoryConfig(t *testing.T) { defaultConfig.StateType = define.InvalidStateStore defaultConfig.StorageConfig = storage.StoreOptions{} - emptyConfig, err := readConfigFromFile("testdata/empty.conf") + emptyConfig, err := readConfigFromFile("testdata/empty.conf", defaultConfig) assert.NotNil(t, emptyConfig) assert.Nil(t, err) - err = emptyConfig.mergeConfig(defaultConfig) - assert.Nil(t, err) - equal := reflect.DeepEqual(emptyConfig, defaultConfig) assert.True(t, equal) } @@ -46,19 +43,16 @@ func TestMergeEmptyAndDefaultMemoryConfig(t *testing.T) { func TestMergeEmptyAndLibpodConfig(t *testing.T) { // Make sure that when we merge the default config into an empty one that we // effectively get the default config. - libpodConfig, err := readConfigFromFile("testdata/libpod.conf") + libpodConfig, err := readConfigFromFile("testdata/libpod.conf", &Config{}) assert.NotNil(t, libpodConfig) assert.Nil(t, err) libpodConfig.StateType = define.InvalidStateStore libpodConfig.StorageConfig = storage.StoreOptions{} - emptyConfig, err := readConfigFromFile("testdata/empty.conf") + emptyConfig, err := readConfigFromFile("testdata/empty.conf", libpodConfig) assert.NotNil(t, emptyConfig) assert.Nil(t, err) - err = emptyConfig.mergeConfig(libpodConfig) - assert.Nil(t, err) - equal := reflect.DeepEqual(emptyConfig, libpodConfig) assert.True(t, equal) } diff --git a/libpod/config/merge.go b/libpod/config/merge.go deleted file mode 100644 index 798a63da7..000000000 --- a/libpod/config/merge.go +++ /dev/null @@ -1,183 +0,0 @@ -package config - -import ( - "path/filepath" - - "github.com/containers/libpod/libpod/define" - "github.com/sirupsen/logrus" -) - -// Merge merges the other config into the current one. Note that a field of the -// other config is only merged when it's not already set in the current one. -// -// Note that the StateType and the StorageConfig will NOT be changed. -func (c *Config) mergeConfig(other *Config) error { - // strings - c.CgroupManager = mergeStrings(c.CgroupManager, other.CgroupManager) - c.CNIConfigDir = mergeStrings(c.CNIConfigDir, other.CNIConfigDir) - c.CNIDefaultNetwork = mergeStrings(c.CNIDefaultNetwork, other.CNIDefaultNetwork) - c.DefaultMountsFile = mergeStrings(c.DefaultMountsFile, other.DefaultMountsFile) - c.DetachKeys = mergeStrings(c.DetachKeys, other.DetachKeys) - c.EventsLogFilePath = mergeStrings(c.EventsLogFilePath, other.EventsLogFilePath) - c.EventsLogger = mergeStrings(c.EventsLogger, other.EventsLogger) - c.ImageDefaultTransport = mergeStrings(c.ImageDefaultTransport, other.ImageDefaultTransport) - c.InfraCommand = mergeStrings(c.InfraCommand, other.InfraCommand) - c.InfraImage = mergeStrings(c.InfraImage, other.InfraImage) - c.InitPath = mergeStrings(c.InitPath, other.InitPath) - c.LockType = mergeStrings(c.LockType, other.LockType) - c.Namespace = mergeStrings(c.Namespace, other.Namespace) - c.NetworkCmdPath = mergeStrings(c.NetworkCmdPath, other.NetworkCmdPath) - c.OCIRuntime = mergeStrings(c.OCIRuntime, other.OCIRuntime) - c.SignaturePolicyPath = mergeStrings(c.SignaturePolicyPath, other.SignaturePolicyPath) - c.StaticDir = mergeStrings(c.StaticDir, other.StaticDir) - c.TmpDir = mergeStrings(c.TmpDir, other.TmpDir) - c.VolumePath = mergeStrings(c.VolumePath, other.VolumePath) - - // string map of slices - c.OCIRuntimes = mergeStringMaps(c.OCIRuntimes, other.OCIRuntimes) - - // string slices - c.CNIPluginDir = mergeStringSlices(c.CNIPluginDir, other.CNIPluginDir) - c.ConmonEnvVars = mergeStringSlices(c.ConmonEnvVars, other.ConmonEnvVars) - c.ConmonPath = mergeStringSlices(c.ConmonPath, other.ConmonPath) - c.HooksDir = mergeStringSlices(c.HooksDir, other.HooksDir) - c.RuntimePath = mergeStringSlices(c.RuntimePath, other.RuntimePath) - c.RuntimeSupportsJSON = mergeStringSlices(c.RuntimeSupportsJSON, other.RuntimeSupportsJSON) - c.RuntimeSupportsNoCgroups = mergeStringSlices(c.RuntimeSupportsNoCgroups, other.RuntimeSupportsNoCgroups) - - // int64s - c.MaxLogSize = mergeInt64s(c.MaxLogSize, other.MaxLogSize) - - // uint32s - c.NumLocks = mergeUint32s(c.NumLocks, other.NumLocks) - - // bools - c.EnableLabeling = mergeBools(c.EnableLabeling, other.EnableLabeling) - c.EnablePortReservation = mergeBools(c.EnablePortReservation, other.EnablePortReservation) - c.NoPivotRoot = mergeBools(c.NoPivotRoot, other.NoPivotRoot) - c.SDNotify = mergeBools(c.SDNotify, other.SDNotify) - - // state type - if c.StateType == define.InvalidStateStore { - c.StateType = other.StateType - } - - // store options - need to check all fields since some configs might only - // set it partially - c.StorageConfig.RunRoot = mergeStrings(c.StorageConfig.RunRoot, other.StorageConfig.RunRoot) - c.StorageConfig.GraphRoot = mergeStrings(c.StorageConfig.GraphRoot, other.StorageConfig.GraphRoot) - c.StorageConfig.GraphDriverName = mergeStrings(c.StorageConfig.GraphDriverName, other.StorageConfig.GraphDriverName) - c.StorageConfig.GraphDriverOptions = mergeStringSlices(c.StorageConfig.GraphDriverOptions, other.StorageConfig.GraphDriverOptions) - if c.StorageConfig.UIDMap == nil { - c.StorageConfig.UIDMap = other.StorageConfig.UIDMap - } - if c.StorageConfig.GIDMap == nil { - c.StorageConfig.GIDMap = other.StorageConfig.GIDMap - } - - // backwards compat *Set fields - c.StorageConfigRunRootSet = mergeBools(c.StorageConfigRunRootSet, other.StorageConfigRunRootSet) - c.StorageConfigGraphRootSet = mergeBools(c.StorageConfigGraphRootSet, other.StorageConfigGraphRootSet) - c.StorageConfigGraphDriverNameSet = mergeBools(c.StorageConfigGraphDriverNameSet, other.StorageConfigGraphDriverNameSet) - c.VolumePathSet = mergeBools(c.VolumePathSet, other.VolumePathSet) - c.StaticDirSet = mergeBools(c.StaticDirSet, other.StaticDirSet) - c.TmpDirSet = mergeBools(c.TmpDirSet, other.TmpDirSet) - - return nil -} - -// MergeDBConfig merges the configuration from the database. -func (c *Config) MergeDBConfig(dbConfig *DBConfig) error { - - if !c.StorageConfigRunRootSet && dbConfig.StorageTmp != "" { - if c.StorageConfig.RunRoot != dbConfig.StorageTmp && - c.StorageConfig.RunRoot != "" { - logrus.Debugf("Overriding run root %q with %q from database", - c.StorageConfig.RunRoot, dbConfig.StorageTmp) - } - c.StorageConfig.RunRoot = dbConfig.StorageTmp - } - - if !c.StorageConfigGraphRootSet && dbConfig.StorageRoot != "" { - if c.StorageConfig.GraphRoot != dbConfig.StorageRoot && - c.StorageConfig.GraphRoot != "" { - logrus.Debugf("Overriding graph root %q with %q from database", - c.StorageConfig.GraphRoot, dbConfig.StorageRoot) - } - c.StorageConfig.GraphRoot = dbConfig.StorageRoot - } - - if !c.StorageConfigGraphDriverNameSet && dbConfig.GraphDriver != "" { - if c.StorageConfig.GraphDriverName != dbConfig.GraphDriver && - c.StorageConfig.GraphDriverName != "" { - logrus.Errorf("User-selected graph driver %q overwritten by graph driver %q from database - delete libpod local files to resolve", - c.StorageConfig.GraphDriverName, dbConfig.GraphDriver) - } - c.StorageConfig.GraphDriverName = dbConfig.GraphDriver - } - - if !c.StaticDirSet && dbConfig.LibpodRoot != "" { - if c.StaticDir != dbConfig.LibpodRoot && c.StaticDir != "" { - logrus.Debugf("Overriding static dir %q with %q from database", c.StaticDir, dbConfig.LibpodRoot) - } - c.StaticDir = dbConfig.LibpodRoot - } - - if !c.TmpDirSet && dbConfig.LibpodTmp != "" { - if c.TmpDir != dbConfig.LibpodTmp && c.TmpDir != "" { - logrus.Debugf("Overriding tmp dir %q with %q from database", c.TmpDir, dbConfig.LibpodTmp) - } - c.TmpDir = dbConfig.LibpodTmp - c.EventsLogFilePath = filepath.Join(dbConfig.LibpodTmp, "events", "events.log") - } - - if !c.VolumePathSet && dbConfig.VolumePath != "" { - if c.VolumePath != dbConfig.VolumePath && c.VolumePath != "" { - logrus.Debugf("Overriding volume path %q with %q from database", c.VolumePath, dbConfig.VolumePath) - } - c.VolumePath = dbConfig.VolumePath - } - return nil -} - -func mergeStrings(a, b string) string { - if a == "" { - return b - } - return a -} - -func mergeStringSlices(a, b []string) []string { - if len(a) == 0 && b != nil { - return b - } - return a -} - -func mergeStringMaps(a, b map[string][]string) map[string][]string { - if len(a) == 0 && b != nil { - return b - } - return a -} - -func mergeInt64s(a, b int64) int64 { - if a == 0 { - return b - } - return a -} - -func mergeUint32s(a, b uint32) uint32 { - if a == 0 { - return b - } - return a -} - -func mergeBools(a, b bool) bool { - if !a { - return b - } - return a -} diff --git a/libpod/config/merge_test.go b/libpod/config/merge_test.go deleted file mode 100644 index eb450b273..000000000 --- a/libpod/config/merge_test.go +++ /dev/null @@ -1,157 +0,0 @@ -package config - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestMergeStrings(t *testing.T) { - testData := []struct { - a string - b string - res string - }{ - {"", "", ""}, - {"a", "", "a"}, - {"a", "b", "a"}, - {"", "b", "b"}, - } - for _, data := range testData { - res := mergeStrings(data.a, data.b) - assert.Equal(t, data.res, res) - } -} - -func TestMergeStringSlices(t *testing.T) { - testData := []struct { - a []string - b []string - res []string - }{ - { - nil, nil, nil, - }, - { - nil, - []string{}, - []string{}, - }, - { - []string{}, - nil, - []string{}, - }, - { - []string{}, - []string{}, - []string{}, - }, - { - []string{"a"}, - []string{}, - []string{"a"}, - }, - { - []string{"a"}, - []string{"b"}, - []string{"a"}, - }, - { - []string{}, - []string{"b"}, - []string{"b"}, - }, - } - for _, data := range testData { - res := mergeStringSlices(data.a, data.b) - assert.Equal(t, data.res, res) - } -} - -func TestMergeStringMaps(t *testing.T) { - testData := []struct { - a map[string][]string - b map[string][]string - res map[string][]string - }{ - { - nil, nil, nil, - }, - { - nil, - map[string][]string{}, - map[string][]string{}}, - { - map[string][]string{"a": {"a"}}, - nil, - map[string][]string{"a": {"a"}}, - }, - { - nil, - map[string][]string{"b": {"b"}}, - map[string][]string{"b": {"b"}}, - }, - { - map[string][]string{"a": {"a"}}, - map[string][]string{"b": {"b"}}, - map[string][]string{"a": {"a"}}, - }, - } - for _, data := range testData { - res := mergeStringMaps(data.a, data.b) - assert.Equal(t, data.res, res) - } -} - -func TestMergeInts64(t *testing.T) { - testData := []struct { - a int64 - b int64 - res int64 - }{ - {int64(0), int64(0), int64(0)}, - {int64(1), int64(0), int64(1)}, - {int64(0), int64(1), int64(1)}, - {int64(2), int64(1), int64(2)}, - {int64(-1), int64(1), int64(-1)}, - {int64(0), int64(-1), int64(-1)}, - } - for _, data := range testData { - res := mergeInt64s(data.a, data.b) - assert.Equal(t, data.res, res) - } -} -func TestMergeUint32(t *testing.T) { - testData := []struct { - a uint32 - b uint32 - res uint32 - }{ - {uint32(0), uint32(0), uint32(0)}, - {uint32(1), uint32(0), uint32(1)}, - {uint32(0), uint32(1), uint32(1)}, - {uint32(2), uint32(1), uint32(2)}, - } - for _, data := range testData { - res := mergeUint32s(data.a, data.b) - assert.Equal(t, data.res, res) - } -} - -func TestMergeBools(t *testing.T) { - testData := []struct { - a bool - b bool - res bool - }{ - {false, false, false}, - {true, false, true}, - {false, true, true}, - {true, true, true}, - } - for _, data := range testData { - res := mergeBools(data.a, data.b) - assert.Equal(t, data.res, res) - } -} -- cgit v1.2.3-54-g00ecf