From 46078d391fda82bc3099668049cd54cf7141f4d9 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 10 Mar 2021 14:13:01 +0000 Subject: [PATCH] config: make config file reads reload the config file if needed #4996 Before this change the config file needed to be explicitly reloaded. This coupled the config file implementation with the backends needlessly. This change stats the config file to see if it needs to be reloaded on every config file operation. This allows us to remove calls to - config.SaveConfig - config.GetFresh Which now makes the the only needed interface to the config file be that provided by configmap.Map when rclone is not being configured. This also adds tests for configfile --- backend/onedrive/onedrive.go | 1 - backend/seafile/seafile.go | 1 - fs/config/config.go | 34 +--- fs/config/config_test.go | 23 --- fs/config/configfile/configfile.go | 63 ++++++- fs/config/configfile/configfile_test.go | 215 ++++++++++++++++++++++++ lib/oauthutil/oauthutil.go | 18 +- 7 files changed, 278 insertions(+), 77 deletions(-) create mode 100644 fs/config/configfile/configfile_test.go diff --git a/backend/onedrive/onedrive.go b/backend/onedrive/onedrive.go index 06817fce5..2973ba8ce 100755 --- a/backend/onedrive/onedrive.go +++ b/backend/onedrive/onedrive.go @@ -302,7 +302,6 @@ func init() { m.Set(configDriveID, finalDriveID) m.Set(configDriveType, rootItem.ParentReference.DriveType) - config.SaveConfig() }, Options: append(oauthutil.SharedOptions, []fs.Option{{ Name: "region", diff --git a/backend/seafile/seafile.go b/backend/seafile/seafile.go index 794efd856..784beb4d8 100644 --- a/backend/seafile/seafile.go +++ b/backend/seafile/seafile.go @@ -372,7 +372,6 @@ func Config(ctx context.Context, name string, m configmap.Mapper) { m.Set(configAuthToken, token) // And delete any previous entry for password m.Set(configPassword, "") - config.SaveConfig() // And we're done here break } diff --git a/fs/config/config.go b/fs/config/config.go index b42be2e9d..df13e2195 100644 --- a/fs/config/config.go +++ b/fs/config/config.go @@ -230,7 +230,7 @@ func LoadConfig(ctx context.Context) { var ErrorConfigFileNotFound = errors.New("config file not found") // SaveConfig calling function which saves configuration file. -// if saveConfig returns error trying again after sleep. +// if SaveConfig returns error trying again after sleep. func SaveConfig() { ctx := context.Background() ci := fs.GetConfig(ctx) @@ -253,19 +253,6 @@ func SaveConfig() { func SetValueAndSave(name, key, value string) error { // Set the value in config in case we fail to reload it Data.SetValue(name, key, value) - - // Reload the config file - err := Data.Load() - if err == ErrorConfigFileNotFound { - // Config file not written yet so ignore reload - return nil - } else if err != nil { - return err - } - if !Data.HasSection(name) { - // Section doesn't exist yet so ignore reload - return nil - } // Save it again SaveConfig() return nil @@ -281,20 +268,6 @@ func getWithDefault(name, key, defaultValue string) string { return value } -// FileGetFresh reads the config key under section return the value or -// an error if the config file was not found or that value couldn't be -// read. -func FileGetFresh(section, key string) (value string, err error) { - if err := Data.Load(); err != nil { - return "", err - } - value, found := Data.GetValue(section, key) - if !found { - return "", errors.New("value not found") - } - return value, nil -} - // UpdateRemote adds the keyValues passed in to the remote of name. // keyValues should be key, value pairs. func UpdateRemote(ctx context.Context, name string, keyValues rc.Params, doObscure, noObscure bool) error { @@ -446,11 +419,6 @@ func FileDeleteKey(section, key string) bool { var matchEnv = regexp.MustCompile(`^RCLONE_CONFIG_(.*?)_TYPE=.*$`) -// FileRefresh ensures the latest configFile is loaded from disk -func FileRefresh() error { - return Data.Load() -} - // FileSections returns the sections in the config file // including any defined by environment variables. func FileSections() []string { diff --git a/fs/config/config_test.go b/fs/config/config_test.go index 486416451..2e597d9c3 100644 --- a/fs/config/config_test.go +++ b/fs/config/config_test.go @@ -3,16 +3,12 @@ package config_test import ( - "bytes" "context" - "io/ioutil" "testing" "github.com/rclone/rclone/fs/config" "github.com/rclone/rclone/fs/config/configfile" - "github.com/rclone/rclone/fs/rc" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestConfigLoad(t *testing.T) { @@ -31,22 +27,3 @@ func TestConfigLoad(t *testing.T) { expect = []string{"type", "nounc"} assert.Equal(t, expect, keys) } - -func TestFileRefresh(t *testing.T) { - ctx := context.Background() - defer testConfigFile(t, "refresh.conf")() - require.NoError(t, config.CreateRemote(ctx, "refresh_test", "config_test_remote", rc.Params{ - "bool": true, - }, false, false)) - b, err := ioutil.ReadFile(config.ConfigPath) - assert.NoError(t, err) - - b = bytes.Replace(b, []byte("refresh_test"), []byte("refreshed_test"), 1) - err = ioutil.WriteFile(config.ConfigPath, b, 0644) - assert.NoError(t, err) - - assert.NotEqual(t, []string{"refreshed_test"}, config.Data.GetSectionList()) - err = config.FileRefresh() - assert.NoError(t, err) - assert.Equal(t, []string{"refreshed_test"}, config.Data.GetSectionList()) -} diff --git a/fs/config/configfile/configfile.go b/fs/config/configfile/configfile.go index 8a87ecb0d..a889a3450 100644 --- a/fs/config/configfile/configfile.go +++ b/fs/config/configfile/configfile.go @@ -1,3 +1,4 @@ +// Package configfile implements a config file loader and saver package configfile import ( @@ -6,6 +7,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync" "github.com/Unknwon/goconfig" "github.com/pkg/errors" @@ -22,28 +24,54 @@ func LoadConfig(ctx context.Context) { // Storage implements config.Storage for saving and loading config // data in a simple INI based file. type Storage struct { - gc *goconfig.ConfigFile + gc *goconfig.ConfigFile // config file loaded - thread safe + mu sync.Mutex // to protect the following variables + fi os.FileInfo // stat of the file when last loaded } -// Load the config from permanent storage, decrypting if necessary -func (s *Storage) Load() (err error) { +// Check to see if we need to reload the config +func (s *Storage) check() { + s.mu.Lock() + defer s.mu.Unlock() + + // Check to see if config file has changed since it was last loaded + fi, err := os.Stat(config.ConfigPath) + if err == nil { + // check to see if config file has changed and if it has, reload it + if s.fi == nil || !fi.ModTime().Equal(s.fi.ModTime()) || fi.Size() != s.fi.Size() { + fs.Debugf(nil, "Config file has changed externaly - reloading") + err := s._load() + if err != nil { + fs.Errorf(nil, "Failed to read config file - using previous config: %v", err) + } + } + } +} + +// _load the config from permanent storage, decrypting if necessary +// +// mu must be held when calling this +func (s *Storage) _load() (err error) { // Make sure we have a sensible default even when we error defer func() { - if err != nil { + if s.gc == nil { s.gc, _ = goconfig.LoadFromReader(bytes.NewReader([]byte{})) } }() - b, err := os.Open(config.ConfigPath) + fd, err := os.Open(config.ConfigPath) if err != nil { if os.IsNotExist(err) { return config.ErrorConfigFileNotFound } return err } - defer fs.CheckClose(b, &err) + defer fs.CheckClose(fd, &err) - cryptReader, err := config.Decrypt(b) + // Update s.fi with the current file info + s.fi, _ = os.Stat(config.ConfigPath) + + cryptReader, err := config.Decrypt(fd) if err != nil { return err } @@ -57,8 +85,18 @@ func (s *Storage) Load() (err error) { return nil } +// Load the config from permanent storage, decrypting if necessary +func (s *Storage) Load() (err error) { + s.mu.Lock() + defer s.mu.Unlock() + return s._load() +} + // Save the config to permanent storage, encrypting if necessary func (s *Storage) Save() error { + s.mu.Lock() + defer s.mu.Unlock() + dir, name := filepath.Split(config.ConfigPath) err := os.MkdirAll(dir, os.ModePerm) if err != nil { @@ -116,11 +154,15 @@ func (s *Storage) Save() error { fs.Errorf(nil, "Failed to remove backup config file: %v", err) } + // Update s.fi with the newly written file + s.fi, _ = os.Stat(config.ConfigPath) + return nil } // Serialize the config into a string func (s *Storage) Serialize() (string, error) { + s.check() var buf bytes.Buffer if err := goconfig.SaveConfigData(s.gc, &buf); err != nil { return "", errors.Errorf("Failed to save config file: %v", err) @@ -131,6 +173,7 @@ func (s *Storage) Serialize() (string, error) { // HasSection returns true if section exists in the config file func (s *Storage) HasSection(section string) bool { + s.check() _, err := s.gc.GetSection(section) if err != nil { return false @@ -141,22 +184,26 @@ func (s *Storage) HasSection(section string) bool { // DeleteSection removes the named section and all config from the // config file func (s *Storage) DeleteSection(section string) { + s.check() s.gc.DeleteSection(section) } // GetSectionList returns a slice of strings with names for all the // sections func (s *Storage) GetSectionList() []string { + s.check() return s.gc.GetSectionList() } // GetKeyList returns the keys in this section func (s *Storage) GetKeyList(section string) []string { + s.check() return s.gc.GetKeyList(section) } // GetValue returns the key in section with a found flag func (s *Storage) GetValue(section string, key string) (value string, found bool) { + s.check() value, err := s.gc.GetValue(section, key) if err != nil { return "", false @@ -166,11 +213,13 @@ func (s *Storage) GetValue(section string, key string) (value string, found bool // SetValue sets the value under key in section func (s *Storage) SetValue(section string, key string, value string) { + s.check() s.gc.SetValue(section, key, value) } // DeleteKey removes the key under section func (s *Storage) DeleteKey(section string, key string) bool { + s.check() return s.gc.DeleteKey(section, key) } diff --git a/fs/config/configfile/configfile_test.go b/fs/config/configfile/configfile_test.go new file mode 100644 index 000000000..52e441988 --- /dev/null +++ b/fs/config/configfile/configfile_test.go @@ -0,0 +1,215 @@ +package configfile + +import ( + "fmt" + "io/ioutil" + "os" + "runtime" + "strings" + "testing" + + "github.com/rclone/rclone/fs/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var configData = `[one] +type = number1 +fruit = potato + +[two] +type = number2 +fruit = apple +topping = nuts + +[three] +type = number3 +fruit = banana + +` + +// Fill up a temporary config file with the testdata filename passed in +func setConfigFile(t *testing.T, data string) func() { + out, err := ioutil.TempFile("", "rclone-configfile-test") + require.NoError(t, err) + filePath := out.Name() + + _, err = out.Write([]byte(data)) + require.NoError(t, err) + + require.NoError(t, out.Close()) + + old := config.ConfigPath + config.ConfigPath = filePath + return func() { + config.ConfigPath = old + _ = os.Remove(filePath) + } +} + +// toUnix converts \r\n to \n in buf +func toUnix(buf string) string { + if runtime.GOOS == "windows" { + return strings.Replace(buf, "\r\n", "\n", -1) + } + return buf +} + +func TestConfigFile(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, data.Load()) + + t.Run("Read", func(t *testing.T) { + t.Run("Serialize", func(t *testing.T) { + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, configData, toUnix(buf)) + }) + t.Run("HasSection", func(t *testing.T) { + assert.True(t, data.HasSection("one")) + assert.False(t, data.HasSection("missing")) + }) + t.Run("GetSectionList", func(t *testing.T) { + assert.Equal(t, []string{ + "one", + "two", + "three", + }, data.GetSectionList()) + }) + t.Run("GetKeyList", func(t *testing.T) { + assert.Equal(t, []string{ + "type", + "fruit", + "topping", + }, data.GetKeyList("two")) + assert.Equal(t, []string(nil), data.GetKeyList("unicorn")) + }) + t.Run("GetValue", func(t *testing.T) { + value, ok := data.GetValue("one", "type") + assert.True(t, ok) + assert.Equal(t, "number1", value) + value, ok = data.GetValue("three", "fruit") + assert.True(t, ok) + assert.Equal(t, "banana", value) + value, ok = data.GetValue("one", "typeX") + assert.False(t, ok) + assert.Equal(t, "", value) + value, ok = data.GetValue("threeX", "fruit") + assert.False(t, ok) + assert.Equal(t, "", value) + }) + }) + + //defer setConfigFile(configData)() + + t.Run("Write", func(t *testing.T) { + t.Run("SetValue", func(t *testing.T) { + data.SetValue("one", "extra", "42") + data.SetValue("two", "fruit", "acorn") + + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +type = number1 +fruit = potato +extra = 42 + +[two] +type = number2 +fruit = acorn +topping = nuts + +[three] +type = number3 +fruit = banana + +`, toUnix(buf)) + t.Run("DeleteKey", func(t *testing.T) { + data.DeleteKey("one", "type") + data.DeleteKey("two", "missing") + data.DeleteKey("three", "fruit") + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[two] +type = number2 +fruit = acorn +topping = nuts + +[three] +type = number3 + +`, toUnix(buf)) + t.Run("DeleteSection", func(t *testing.T) { + data.DeleteSection("two") + data.DeleteSection("missing") + buf, err := data.Serialize() + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[three] +type = number3 + +`, toUnix(buf)) + t.Run("Save", func(t *testing.T) { + require.NoError(t, data.Save()) + buf, err := ioutil.ReadFile(config.ConfigPath) + require.NoError(t, err) + assert.Equal(t, `[one] +fruit = potato +extra = 42 + +[three] +type = number3 + +`, toUnix(string(buf))) + }) + }) + }) + }) + }) +} + +func TestConfigFileReload(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, data.Load()) + + value, ok := data.GetValue("three", "appended") + assert.False(t, ok) + assert.Equal(t, "", value) + + // Now write a new value on the end + out, err := os.OpenFile(config.ConfigPath, os.O_APPEND|os.O_WRONLY, 0777) + require.NoError(t, err) + fmt.Fprintln(out, "appended = what magic") + require.NoError(t, out.Close()) + + // And check we magically reloaded it + value, ok = data.GetValue("three", "appended") + assert.True(t, ok) + assert.Equal(t, "what magic", value) +} + +func TestConfigFileDoesNotExist(t *testing.T) { + defer setConfigFile(t, configData)() + data := &Storage{} + + require.NoError(t, os.Remove(config.ConfigPath)) + + err := data.Load() + require.Equal(t, config.ErrorConfigFileNotFound, err) + + // check that using data doesn't crash + value, ok := data.GetValue("three", "appended") + assert.False(t, ok) + assert.Equal(t, "", value) +} diff --git a/lib/oauthutil/oauthutil.go b/lib/oauthutil/oauthutil.go index f332d8af3..d04aed11f 100644 --- a/lib/oauthutil/oauthutil.go +++ b/lib/oauthutil/oauthutil.go @@ -148,14 +148,8 @@ func PutToken(name string, m configmap.Mapper, token *oauth2.Token, newSection b tokenString := string(tokenBytes) old, ok := m.Get(config.ConfigToken) if !ok || tokenString != old { - err = config.SetValueAndSave(name, config.ConfigToken, tokenString) - if newSection && err != nil { - fs.Debugf(name, "Added new token to config, still needs to be saved") - } else if err != nil { - fs.Errorf(nil, "Failed to save new token in config file: %v", err) - } else { - fs.Debugf(name, "Saved new token in config file") - } + m.Set(config.ConfigToken, tokenString) + fs.Debugf(name, "Saved new token in config file") } return nil } @@ -175,13 +169,13 @@ type TokenSource struct { // If token has expired then first try re-reading it from the config // file in case a concurrently running rclone has updated it already func (ts *TokenSource) reReadToken() bool { - tokenString, err := config.FileGetFresh(ts.name, config.ConfigToken) - if err != nil { - fs.Debugf(ts.name, "Failed to read token out of config file: %v", err) + tokenString, found := ts.m.Get(config.ConfigToken) + if !found || tokenString == "" { + fs.Debugf(ts.name, "Failed to read token out of config file") return false } newToken := new(oauth2.Token) - err = json.Unmarshal([]byte(tokenString), newToken) + err := json.Unmarshal([]byte(tokenString), newToken) if err != nil { fs.Debugf(ts.name, "Failed to parse token out of config file: %v", err) return false