beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

daemon/config: Reload(): add TODO for config reload logic

Browse source 
The Reload logic is problematic and needs a rewrite.

Currently, config.Reload() is validating newConfig before the reload callback
is executed. At that point, newConfig may be a partial configuration, yet to be
merged with the existing configuration (in the "reload()" callback). Validating
this config before it's merged can result in incorrect validation errors.

However, the current "reload()" callback we use is DaemonCli.reloadConfig(),
which includes a call to Daemon.Reload(), which both performs "merging" and
validation, as well as actually updating the daemon configuration. Calling
DaemonCli.reloadConfig() *before* validation, could thus lead to a failure in
that function (making the reload non-atomic).

While *some* errors could always occur when applying/updating the config, we
should make it more atomic, and;

1. get (a copy of) the active configuration
2. get the new configuration
3. apply the (reloadable) options from the new configuration
4. validate the merged results
5. apply the new configuration.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This commit is contained in:
Sebastiaan van Stijn 2022-04-24 16:18:20 +02:00
parent 9a54dadc44
commit c46e2e85ee
No known key found for this signature in database
GPG key ID: 76698F39D527CE8C
1 changed files with 20 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
daemon/config/config.go
View file

@ -350,6 +350,26 @@ func Reload(configFile string, flags *pflag.FlagSet, reload func(*Config)) error
}
newConfig.Labels = newLabels
// TODO(thaJeztah) This logic is problematic and needs a rewrite;
// This is validating newConfig before the "reload()" callback is executed.
// At this point, newConfig may be a partial configuration, to be merged
// with the existing configuration in the "reload()" callback. Validating
// this config before it's merged can result in incorrect validation errors.
//
// However, the current "reload()" callback we use is DaemonCli.reloadConfig(),
// which includes a call to Daemon.Reload(), which both performs "merging"
// and validation, as well as actually updating the daemon configuration.
// Calling DaemonCli.reloadConfig() *before* validation, could thus lead to
// a failure in that function (making the reload non-atomic).
//
// While *some* errors could always occur when applying/updating the config,
// we should make it more atomic, and;
//
// 1. get (a copy of) the active configuration
// 2. get the new configuration
// 3. apply the (reloadable) options from the new configuration
// 4. validate the merged results
// 5. apply the new configuration.
if err := Validate(newConfig); err != nil {
return errors.Wrap(err, "file configuration validation failed")
}