Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for drop-in config #1885

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+124 −30
Open

Add support for drop-in config #1885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion store.go
View file
Open in desktop
Expand Up @@ -3710,7 +3710,7 @@ func DefaultConfigFile() (string, error) {
// the configuration in storeOptions.
// Deprecated: Use types.ReloadConfigurationFile, which can return an error.
func ReloadConfigurationFile(configFile string, storeOptions *types.StoreOptions) {
_ = types.ReloadConfigurationFile(configFile, storeOptions)
_ = types.ReloadConfigurationFile(configFile, storeOptions, true)
}

// GetDefaultMountOptions returns the default mountoptions defined in container/storage
Expand Down
72 changes: 64 additions & 8 deletions types/options.go
View file
Open in desktop
Expand Up @@ -49,6 +49,12 @@ var (
defaultConfigFile = SystemConfigFile
// DefaultStoreOptions is a reasonable default set of options.
defaultStoreOptions StoreOptions

// defaultOverrideConfigFile path to override the default system wide storage.conf file
defaultOverrideConfigFile = "/etc/containers/storage.conf"

// defaultDropInConfigDir path to the folder containing drop in config files
defaultDropInConfigDir = defaultOverrideConfigFile + ".d"
)

func loadDefaultStoreOptions() {
Expand Down Expand Up @@ -114,11 +120,53 @@ func loadDefaultStoreOptions() {

// loadStoreOptions returns the default storage ops for containers
func loadStoreOptions() (StoreOptions, error) {
storageConf, err := DefaultConfigFile()
baseConf, err := DefaultConfigFile()
if err != nil {
return defaultStoreOptions, err
}

// Load the base config file
baseOptions, err := loadStoreOptionsFromConfFile(baseConf)
if err != nil {
return defaultStoreOptions, err
saschagrunert marked this conversation as resolved.
Show resolved Hide resolved
}
return loadStoreOptionsFromConfFile(storageConf)

if _, err := os.Stat(defaultDropInConfigDir); err == nil {
// The directory exists, so merge the configuration from this directory
err = mergeConfigFromDirectory(&baseOptions, defaultDropInConfigDir)
Copy link
Member

@giuseppe giuseppe Apr 24, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we should hardcode a specific directory, but instead try to load each file from the $STORAGE_CONF + ".d" directory, where $STORAGE_CONF is whatever file we decided to use

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @giuseppe, that's a good idea. I modified the code to set the drop-in config dir to storage conf path + .d

if err != nil {
return defaultStoreOptions, err
}
} else if !os.IsNotExist(err) {
// There was an error other than the directory not existing
return defaultStoreOptions, err
}

return baseOptions, nil
}

func mergeConfigFromDirectory(baseOptions *StoreOptions, configDir string) error {
return filepath.Walk(configDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice update. Should check for file extension here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a text file without a .conf or .toml can also contain a valid drop-in config. For any reason the content is not valid (irrespective of the extension) it will fail in ReloadConfigurationFile below while parsing the toml.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We support a file without an extension?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as the content of the file is valid, it shouldn't matter, right?

crio doesn't enforce the extension either - https://github.com/cri-o/cri-o/blob/256fda5ac98de1d67e26133d0b25df2a74e2ebd2/pkg/config/config.go#L776

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually there is a filter, but this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one risk is temporary files written by editor and the like. It mostly only is an issue if we're reloading automatically. If it's manually triggered (like here) then the user usually has written and edited the editor before the reload happens...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think requiring a .conf or .toml extension could be a nice change both here and cri-o. @sohankunkerkar added a filter for the kubelet's drop-in work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes require a .conf extension, that follows what we have done with other dropin files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rphillips @haircommander @rhatdan for this feedback. I have updated the code to select files with .conf extension only and also updated corresponding unit test to verify that.

// Only consider files with .conf extension
if filepath.Ext(path) != ".conf" {
return nil
}

// Load drop-in options from the current file
err = ReloadConfigurationFile(path, baseOptions, false)
if err != nil {
return err
}

return nil
})
}

// usePerUserStorage returns whether the user private storage must be used.
Expand Down Expand Up @@ -399,7 +447,7 @@ func ReloadConfigurationFileIfNeeded(configFile string, storeOptions *StoreOptio
return nil
}

if err := ReloadConfigurationFile(configFile, storeOptions); err != nil {
if err := ReloadConfigurationFile(configFile, storeOptions, true); err != nil {
return err
}

Expand All @@ -412,7 +460,7 @@ func ReloadConfigurationFileIfNeeded(configFile string, storeOptions *StoreOptio

// ReloadConfigurationFile parses the specified configuration file and overrides
// the configuration in storeOptions.
func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) error {
func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions, initializeOptions bool) error {
config := new(TomlConfig)

meta, err := toml.DecodeFile(configFile, &config)
Expand All @@ -428,8 +476,11 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) erro
}
}

// Clear storeOptions of previous settings
*storeOptions = StoreOptions{}
if initializeOptions {
// Clear storeOptions of previous settings
*storeOptions = StoreOptions{}
}

if config.Storage.Driver != "" {
storeOptions.GraphDriverName = config.Storage.Driver
}
Expand Down Expand Up @@ -519,8 +570,13 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) erro
storeOptions.PullOptions = config.Storage.Options.PullOptions
}

storeOptions.DisableVolatile = config.Storage.Options.DisableVolatile
storeOptions.TransientStore = config.Storage.TransientStore
if config.Storage.Options.DisableVolatile {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I set disableFolatile = false in the drop-in?

It seems to me that all of this is going to require some other approach, and probably very detailed testing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, alternatively, be very strict and very restrictive about which options are supported in drop-ins, support/test only those, and hard-fail if any others are present. Then we can add support for more options over time … assuming it’s sufficiently certain adding the others will actually be possible in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we need to call meta.Keys() to pull out the keys that are being set, then selectively set those values on a storeOptions instance.

Copy link
Author

@harche harche Apr 30, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rphillips. If I get the toml.DecodeFile(configFile, &config).Keys() and update the values only if the key is defined in a drop-in config then it seem to fix the issue.

keySet := make(map[string]bool)
for _, key := range meta.Keys() {
	keySet[key.String()] = true
}

if _, ok := keySet["storage.transient_store"]; ok {
	storeOptions.TransientStore = config.Storage.TransientStore
}

cc @mtrmac @haircommander @sohankunkerkar

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated snippet of the included test looks like this,

contents := []string{
		`[storage]
runroot = 'temp/run1'
graphroot = 'temp/graph1'
transient_store = false`,
		`[storage]
runroot = 'temp/run2'
graphroot = 'temp/graph2'`,
		`[storage]
runroot = 'should/ignore'
graphroot = 'should/ignore'`,
		`[storage]
runroot = 'temp/run3'`,
		`[storage]
runroot = 'temp/run4'
graphroot = 'temp/graph4'`,
	}
	for i, fileName := range fileNames {
		filePath := filepath.Join(tempDir, fileName)
		if err := os.WriteFile(filePath, []byte(contents[i]), 0o666); err != nil {
			t.Fatalf("Failed to write to temp file: %v", err)
		}
	}

	// Set base options
	baseOptions := StoreOptions{
		RunRoot:        "initial/run",
		GraphRoot:      "initial/graph",
		TransientStore: true,
	}

	// Expected results after merging configurations from only .conf files
	expectedOptions := StoreOptions{
		RunRoot:        "temp/run3", // Last .conf file (config3.conf) read overrides earlier values
		GraphRoot:      "temp/graph2",
		TransientStore: false,
	}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haircommander pointed me to how kubelet handles drop-in configs,

https://github.com/kubernetes/kubernetes/blob/02365ecec1cb1ddf993cf4ed12407737db950cea/cmd/kubelet/app/server.go#L322-L332

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more option is to use mergo to merge the StorageOptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sorta worry about mergo setting options to false. Did we get past this issue in MCO where values being set to false wouldn’t get set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the mergo option didn't work. It does set the option to false as you predicated.

code is at, harche@f0b92df#diff-5c0b7c9ae084e6c4613c438d00cede106862b604f184a331df1c3806cfbd11d2R164

So it seems like our only option is to use meta.Keys()?

BTW, which MCO issue are you talking about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://issues.redhat.com/browse/OCPBUGS-14399 I remember we had to hack around it.

storeOptions.DisableVolatile = config.Storage.Options.DisableVolatile
}

if config.Storage.TransientStore {
storeOptions.TransientStore = config.Storage.TransientStore
}

storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, cfg.GetGraphDriverOptions(storeOptions.GraphDriverName, config.Storage.Options)...)

Expand Down
2 changes: 0 additions & 2 deletions types/options_darwin.go
View file
Open in desktop
Expand Up @@ -8,8 +8,6 @@ const (
SystemConfigFile = "/usr/share/containers/storage.conf"
)

var defaultOverrideConfigFile = "/etc/containers/storage.conf"

// canUseRootlessOverlay returns true if the overlay driver can be used for rootless containers
func canUseRootlessOverlay(home, runhome string) bool {
return false
Expand Down
5 changes: 0 additions & 5 deletions types/options_freebsd.go
View file
Open in desktop
Expand Up @@ -8,11 +8,6 @@ const (
SystemConfigFile = "/usr/local/share/containers/storage.conf"
)

// defaultConfigFile path to the system wide storage.conf file
var (
defaultOverrideConfigFile = "/usr/local/etc/containers/storage.conf"
)

// canUseRootlessOverlay returns true if the overlay driver can be used for rootless containers
func canUseRootlessOverlay(home, runhome string) bool {
return false
Expand Down
5 changes: 0 additions & 5 deletions types/options_linux.go
View file
Open in desktop
Expand Up @@ -16,11 +16,6 @@ const (
SystemConfigFile = "/usr/share/containers/storage.conf"
)

// defaultConfigFile path to the system wide storage.conf file
var (
defaultOverrideConfigFile = "/etc/containers/storage.conf"
)

// canUseRootlessOverlay returns true if the overlay driver can be used for rootless containers
func canUseRootlessOverlay(home, runhome string) bool {
// we check first for fuse-overlayfs since it is cheaper.
Expand Down
61 changes: 58 additions & 3 deletions types/options_test.go
View file
Open in desktop
Expand Up @@ -143,7 +143,7 @@ func TestSetRemapUIDsGIDsOpts(t *testing.T) {
},
}

err := ReloadConfigurationFile("./storage_test.conf", &remapOpts)
err := ReloadConfigurationFile("./storage_test.conf", &remapOpts, true)
require.NoError(t, err)
if !reflect.DeepEqual(uidmap, remapOpts.UIDMap) {
t.Errorf("Failed to set UIDMap: Expected %v Actual %v", uidmap, remapOpts.UIDMap)
Expand Down Expand Up @@ -185,7 +185,7 @@ remap-group = "%s"

mappings, err := idtools.NewIDMappings(user, user)
require.NoError(t, err)
err = ReloadConfigurationFile(configPath, &remapOpts)
err = ReloadConfigurationFile(configPath, &remapOpts, true)
require.NoError(t, err)
if !reflect.DeepEqual(mappings.UIDs(), remapOpts.UIDMap) {
t.Errorf("Failed to set UIDMap: Expected %v Actual %v", mappings.UIDs(), remapOpts.UIDMap)
Expand All @@ -199,10 +199,65 @@ func TestReloadConfigurationFile(t *testing.T) {
content := bytes.NewBufferString("")
logrus.SetOutput(content)
var storageOpts StoreOptions
err := ReloadConfigurationFile("./storage_broken.conf", &storageOpts)
err := ReloadConfigurationFile("./storage_broken.conf", &storageOpts, true)
require.NoError(t, err)
assert.Equal(t, storageOpts.RunRoot, "/run/containers/test")
logrus.SetOutput(os.Stderr)

assert.Equal(t, strings.Contains(content.String(), "Failed to decode the keys [\\\"foo\\\" \\\"storage.options.graphroot\\\"] from \\\"./storage_broken.conf\\\"\""), true)
}

func TestMergeConfigFromDirectory(t *testing.T) {
tempDir, err := os.MkdirTemp("", "testConfigDir")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)

// Creating a mix of files with .txt and .conf extensions
fileNames := []string{"config1.conf", "config2.conf", "ignore.txt", "config3.conf", "config4.txt"}
contents := []string{
`[storage]
runroot = 'temp/run1'
graphroot = 'temp/graph1'`,
`[storage]
runroot = 'temp/run2'
graphroot = 'temp/graph2'`,
`[storage]
runroot = 'should/ignore'
graphroot = 'should/ignore'`,
`[storage]
runroot = 'temp/run3'`,
`[storage]
runroot = 'temp/run4'
graphroot = 'temp/graph4'`,
}
for i, fileName := range fileNames {
filePath := filepath.Join(tempDir, fileName)
if err := os.WriteFile(filePath, []byte(contents[i]), 0o666); err != nil {
t.Fatalf("Failed to write to temp file: %v", err)
}
}

// Set base options
baseOptions := StoreOptions{
RunRoot: "initial/run",
GraphRoot: "initial/graph",
TransientStore: true,
}

// Expected results after merging configurations from only .conf files
expectedOptions := StoreOptions{
RunRoot: "temp/run3", // Last .conf file (config3.conf) read overrides earlier values
GraphRoot: "temp/graph2",
TransientStore: true,
}

// Run the merging function
err = mergeConfigFromDirectory(&baseOptions, tempDir)
if err != nil {
t.Fatalf("Error merging config from directory: %v", err)
}

assert.DeepEqual(t, expectedOptions, baseOptions)
}
5 changes: 0 additions & 5 deletions types/options_windows.go
View file
Open in desktop
Expand Up @@ -8,11 +8,6 @@ const (
SystemConfigFile = "/usr/share/containers/storage.conf"
)

// defaultConfigFile path to the system wide storage.conf file
var (
defaultOverrideConfigFile = "/etc/containers/storage.conf"
)

// canUseRootlessOverlay returns true if the overlay driver can be used for rootless containers
func canUseRootlessOverlay(home, runhome string) bool {
return false
Expand Down
2 changes: 1 addition & 1 deletion types/utils.go
View file
Open in desktop
Expand Up @@ -66,7 +66,7 @@ func reloadConfigurationFileIfNeeded(configFile string, storeOptions *StoreOptio
return
}

ReloadConfigurationFile(configFile, storeOptions)
ReloadConfigurationFile(configFile, storeOptions, true)

prevReloadConfig.storeOptions = storeOptions
prevReloadConfig.mod = mtime
Expand Down