From b02f0b926afb25b5eb895fb8527ed2dfbf3426a0 Mon Sep 17 00:00:00 2001 From: Diego Augusto Molina Date: Mon, 5 Feb 2024 12:25:54 -0300 Subject: [PATCH] Settings: Fix data race when dynamically overriding settings with environment variables (#81667) Chore: Fix data race when dynamically overriding settings with environment variables --- pkg/setting/setting.go | 19 ++++++++------ pkg/setting/setting_test.go | 46 ++++++++++++++++++++++++++++++++++ pkg/util/osutil/osutil.go | 42 +++++++++++++++++++++++++++++++ pkg/util/osutil/osutil_test.go | 35 ++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 pkg/util/osutil/osutil.go create mode 100644 pkg/util/osutil/osutil_test.go diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 7cb19b47cfd..3731ae08721 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -32,6 +32,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/osutil" ) type Scheme string @@ -822,6 +823,9 @@ func (cfg *Cfg) loadSpecifiedConfigFile(configFile string, masterFile *ini.File) return fmt.Errorf("failed to parse %q: %w", configFile, err) } + // micro-optimization since we don't need to share this ini file. In + // general, prefer to leave this flag as true as it is by default to prevent + // data races userConfig.BlockMode = false for _, section := range userConfig.Sections() { @@ -865,8 +869,6 @@ func (cfg *Cfg) loadConfiguration(args CommandLineArgs) (*ini.File, error) { return nil, err } - parsedFile.BlockMode = false - // command line props commandLineProps := cfg.getCommandLineProperties(args.Args) // load default overrides @@ -987,8 +989,6 @@ func NewCfgFromBytes(bytes []byte) (*Cfg, error) { return nil, fmt.Errorf("failed to parse bytes as INI file: %w", err) } - parsedFile.BlockMode = false - return NewCfgFromINIFile(parsedFile) } @@ -1398,13 +1398,14 @@ func (cfg *Cfg) LogConfigSources() { type DynamicSection struct { section *ini.Section Logger log.Logger + env osutil.Env } // Key dynamically overrides keys with environment variables. // As a side effect, the value of the setting key will be updated if an environment variable is present. func (s *DynamicSection) Key(k string) *ini.Key { envKey := EnvKey(s.section.Name(), k) - envValue := os.Getenv(envKey) + envValue := s.env.Getenv(envKey) key := s.section.Key(k) if len(envValue) == 0 { @@ -1421,7 +1422,7 @@ func (s *DynamicSection) KeysHash() map[string]string { hash := s.section.KeysHash() for k := range hash { envKey := EnvKey(s.section.Name(), k) - envValue := os.Getenv(envKey) + envValue := s.env.Getenv(envKey) if len(envValue) > 0 { hash[k] = envValue } @@ -1432,7 +1433,11 @@ func (s *DynamicSection) KeysHash() map[string]string { // SectionWithEnvOverrides dynamically overrides keys with environment variables. // As a side effect, the value of the setting key will be updated if an environment variable is present. func (cfg *Cfg) SectionWithEnvOverrides(s string) *DynamicSection { - return &DynamicSection{cfg.Raw.Section(s), cfg.Logger} + return &DynamicSection{ + section: cfg.Raw.Section(s), + Logger: cfg.Logger, + env: osutil.RealEnv{}, + } } func readSecuritySettings(iniFile *ini.File, cfg *Cfg) error { diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index a139160329c..184e5023b94 100644 --- a/pkg/setting/setting_test.go +++ b/pkg/setting/setting_test.go @@ -9,9 +9,12 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "time" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/util/osutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/ini.v1" @@ -952,3 +955,46 @@ func TestNewCfgFromINIFile(t *testing.T) { require.Equal(t, Prod, cfg.Env) require.Equal(t, "test.com", cfg.Domain) } + +func TestDynamicSection(t *testing.T) { + t.Parallel() + + t.Run("repro #44509 - panic on concurrent map write", func(t *testing.T) { + t.Parallel() + + const ( + goroutines = 10 + attempts = 1000 + section = "DEFAULT" + key = "TestDynamicSection_repro_44509" + value = "theval" + ) + + cfg, err := NewCfgFromBytes([]byte(``)) + require.NoError(t, err) + + ds := &DynamicSection{ + section: cfg.Raw.Section(section), + Logger: log.NewNopLogger(), + env: osutil.MapEnv{}, + } + osVar := EnvKey(section, key) + err = ds.env.Setenv(osVar, value) + require.NoError(t, err) + + var wg sync.WaitGroup + for i := 0; i < goroutines; i++ { + wg.Add(1) + go require.NotPanics(t, func() { + for i := 0; i < attempts; i++ { + ds.section.Key(key).SetValue("") + ds.Key(key) + } + wg.Done() + }) + } + wg.Wait() + + assert.Equal(t, value, ds.section.Key(key).String()) + }) +} diff --git a/pkg/util/osutil/osutil.go b/pkg/util/osutil/osutil.go new file mode 100644 index 00000000000..605a8812a0f --- /dev/null +++ b/pkg/util/osutil/osutil.go @@ -0,0 +1,42 @@ +package osutil + +import ( + "os" +) + +// Env collects global functions from standard package "os" that are related to +// environment variables. This allows abstracting code and provides a way to +// concurrently test code that needs access to these shared resources. +type Env interface { + Setenv(key, value string) error + Getenv(key string) string +} + +// RealEnv implements Env interface by calling the actual global functions in +// package "os". This should be used by default anywhere that an Env is +// expected, and use MapEnv instead in your unit tests. +type RealEnv struct{} + +func (RealEnv) Setenv(key, value string) error { + return os.Setenv(key, value) +} + +func (RealEnv) Getenv(key string) string { + return os.Getenv(key) +} + +// MapEnv is a fake implementing Env interface. It is purposefully not +// concurrency-safe, so if your tests using it panic due to concurrent map +// access, then you need to fix a data race in your code. This is +// because environment variables are globals to a process, so you should be +// properly synchronizing access to them (e.g. with a mutex). +type MapEnv map[string]string + +func (m MapEnv) Setenv(key, value string) error { + m[key] = value + return nil +} + +func (m MapEnv) Getenv(key string) string { + return m[key] +} diff --git a/pkg/util/osutil/osutil_test.go b/pkg/util/osutil/osutil_test.go new file mode 100644 index 00000000000..5a9dcc41b5f --- /dev/null +++ b/pkg/util/osutil/osutil_test.go @@ -0,0 +1,35 @@ +package osutil + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRealEnv(t *testing.T) { + // testing here is obviously not parallel since we do need to access real + // environment variables from the os + + const key = "MEREKETENGUE" + const value = "IS ALIVE" + + assert.Equal(t, os.Getenv(key), RealEnv{}.Getenv(key)) + assert.NoError(t, RealEnv{}.Setenv(key, value)) + assert.Equal(t, value, RealEnv{}.Getenv(key)) + assert.Equal(t, value, os.Getenv(key)) +} + +func TestMapEnv(t *testing.T) { + t.Parallel() + + const key = "THE_THING" + const value = "IS ALIVE" + + e := MapEnv{} + assert.Empty(t, e.Getenv(key)) + assert.Len(t, e, 0) + assert.NoError(t, e.Setenv(key, value)) + assert.Equal(t, value, e.Getenv(key)) + assert.Len(t, e, 1) +}