diff --git a/weed/security/tls.go b/weed/security/tls.go index 29b0d3c14..4bdf9e89e 100644 --- a/weed/security/tls.go +++ b/weed/security/tls.go @@ -161,7 +161,7 @@ func LoadClientTLSFromFile(configFile string, component string) (grpc.DialOption v.Set(key, filepath.Join(configDir, p)) } } - return LoadClientTLS(&util.ViperProxy{Viper: v}, component), nil + return LoadClientTLS(util.NewViperProxy(v), component), nil } func LoadClientTLS(config *util.ViperProxy, component string) grpc.DialOption { diff --git a/weed/security/tls_client_cert_test.go b/weed/security/tls_client_cert_test.go index 8161a9ee2..7147f2e32 100644 --- a/weed/security/tls_client_cert_test.go +++ b/weed/security/tls_client_cert_test.go @@ -134,7 +134,7 @@ func TestLoadClientTLSPrefersClientCert(t *testing.T) { serverCert, serverKey := ca.issue(t, dir, "server", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) clientCert, clientKey := ca.issue(t, dir, "client", []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", serverCert) v.Set("grpc.master.key", serverKey) @@ -153,7 +153,7 @@ func TestLoadClientTLSFallsBackToServingCert(t *testing.T) { ca := newTestCA(t, dir) dualCert, dualKey := ca.issue(t, dir, "dual", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", dualCert) v.Set("grpc.master.key", dualKey) @@ -171,7 +171,7 @@ func TestLoadClientTLSServerOnlyEkuRejected(t *testing.T) { ca := newTestCA(t, dir) serverCert, serverKey := ca.issue(t, dir, "server", []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) - v := &util.ViperProxy{Viper: viper.New()} + v := util.NewViperProxy(viper.New()) v.Set("grpc.ca", ca.file) v.Set("grpc.master.cert", serverCert) v.Set("grpc.master.key", serverKey) diff --git a/weed/util/config.go b/weed/util/config.go index 181b5efa9..533d7134f 100644 --- a/weed/util/config.go +++ b/weed/util/config.go @@ -3,6 +3,7 @@ package util import ( "strings" "sync" + "time" "github.com/spf13/viper" @@ -39,6 +40,10 @@ func LoadSecurityConfiguration() { } func LoadConfiguration(configFileName string, required bool) (loaded bool) { + // MergeInConfig mutates the shared viper that ViperProxy serializes; + // take the same lock so a merge cannot race a reader or SetDefault. + vp.Lock() + defer vp.Unlock() // find a filer store viper.SetConfigName(configFileName) // name of config file (without extension) @@ -72,8 +77,12 @@ func LoadConfiguration(configFileName string, required bool) (loaded bool) { return true } +// ViperProxy serializes access to the global viper. The wrapped Viper is a +// named field, not embedded, so every method must be declared here under the +// mutex — a promoted method would take no lock and race, e.g. GetStringMap +// against a concurrent SetDefault during `weed server` startup. type ViperProxy struct { - *viper.Viper + v *viper.Viper sync.Mutex } @@ -84,42 +93,123 @@ var ( func (vp *ViperProxy) SetDefault(key string, value interface{}) { vp.Lock() defer vp.Unlock() - vp.Viper.SetDefault(key, value) + vp.v.SetDefault(key, value) +} + +func (vp *ViperProxy) AutomaticEnv() { + vp.Lock() + defer vp.Unlock() + vp.v.AutomaticEnv() } func (vp *ViperProxy) GetString(key string) string { vp.Lock() defer vp.Unlock() - return vp.Viper.GetString(key) + return vp.v.GetString(key) } func (vp *ViperProxy) GetBool(key string) bool { vp.Lock() defer vp.Unlock() - return vp.Viper.GetBool(key) + return vp.v.GetBool(key) } func (vp *ViperProxy) GetInt(key string) int { vp.Lock() defer vp.Unlock() - return vp.Viper.GetInt(key) + return vp.v.GetInt(key) } func (vp *ViperProxy) GetStringSlice(key string) []string { vp.Lock() defer vp.Unlock() - return vp.Viper.GetStringSlice(key) + // a []string value comes back uncast — the same shared subtree issue + // GetStringMap has, so hand the caller a copy + return append([]string(nil), vp.v.GetStringSlice(key)...) +} + +func (vp *ViperProxy) GetStringMap(key string) map[string]interface{} { + vp.Lock() + defer vp.Unlock() + // viper hands back its internal subtree, so a caller iterating it after + // the lock is released would race the next SetDefault — copy it out. + return deepCopyStringMap(vp.v.GetStringMap(key)) +} + +func deepCopyStringMap(m map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(m)) + for k, v := range m { + out[k] = deepCopyValue(v) + } + return out +} + +func deepCopyValue(v interface{}) interface{} { + switch nested := v.(type) { + case map[string]interface{}: + return deepCopyStringMap(nested) + case []interface{}: + out := make([]interface{}, len(nested)) + for i, item := range nested { + out[i] = deepCopyValue(item) + } + return out + default: + return v + } +} + +func (vp *ViperProxy) GetUint32(key string) uint32 { + vp.Lock() + defer vp.Unlock() + return vp.v.GetUint32(key) +} + +func (vp *ViperProxy) GetFloat64(key string) float64 { + vp.Lock() + defer vp.Unlock() + return vp.v.GetFloat64(key) +} + +func (vp *ViperProxy) GetDuration(key string) time.Duration { + vp.Lock() + defer vp.Unlock() + return vp.v.GetDuration(key) +} + +func (vp *ViperProxy) Set(key string, value interface{}) { + vp.Lock() + defer vp.Unlock() + vp.v.Set(key, value) +} + +func (vp *ViperProxy) IsSet(key string) bool { + vp.Lock() + defer vp.Unlock() + return vp.v.IsSet(key) +} + +func (vp *ViperProxy) AllKeys() []string { + vp.Lock() + defer vp.Unlock() + return vp.v.AllKeys() +} + +// NewViperProxy wraps a specific viper — for configuration loaded from a +// source other than the shared instance GetViper returns. +func NewViperProxy(v *viper.Viper) *ViperProxy { + return &ViperProxy{v: v} } func GetViper() *ViperProxy { vp.Lock() defer vp.Unlock() - if vp.Viper == nil { - vp.Viper = viper.GetViper() - vp.AutomaticEnv() - vp.SetEnvPrefix("weed") - vp.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + if vp.v == nil { + vp.v = viper.GetViper() + vp.v.AutomaticEnv() + vp.v.SetEnvPrefix("weed") + vp.v.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) } return vp diff --git a/weed/util/config_test.go b/weed/util/config_test.go new file mode 100644 index 000000000..86a0cd4d0 --- /dev/null +++ b/weed/util/config_test.go @@ -0,0 +1,53 @@ +package util + +import ( + "fmt" + "sync" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" +) + +// `weed server` starts the volume server (SetDefault writers) and the master +// (GetStringMap readers) back to back. Reads and writes must share the proxy +// lock — a race-enabled build flags the interleaving the issue reported. +func TestViperProxySerializesMapReadsAgainstWrites(t *testing.T) { + p := NewViperProxy(viper.New()) + p.SetDefault("storage.backend.a.enabled", true) + + var wg sync.WaitGroup + for i := 0; i < 4; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + for j := 0; j < 200; j++ { + p.SetDefault(fmt.Sprintf("storage.backend.d%d.enabled", j), i) + } + }(i) + } + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + if got := p.GetStringMap("storage.backend"); len(got) == 0 { + t.Error("GetStringMap returned no keys") + return + } + } + }() + wg.Wait() + + got := p.GetStringMap("storage.backend") + assert.Contains(t, got, "a") +} + +// A proxy over its own viper serves the wrapped instance, for configuration +// loaded from a source other than the shared one. +func TestNewViperProxyWrapsGivenViper(t *testing.T) { + v := viper.New() + v.Set("x.y", "z") + p := NewViperProxy(v) + assert.Equal(t, "z", p.GetString("x.y")) + assert.Equal(t, map[string]interface{}{"y": "z"}, p.GetStringMap("x")) +}