IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2024-11-22 08:56:43 -06:00
Code Issues Packages Projects Releases Wiki Activity

Chore: Fix data race within tests and enable a few parallel tests in ssosettingsimpl (#81837)

Browse Source 
* Chore: Fix data race within tests of SSO Setting implementation

* Chore: fix data race within tests to allow parallel testing

* Chore: rollback changes runtime code to test a different approach

* Chore: Fix data race in SSO Setting implementation Upsert method

* Chore: fix typo in comment
This commit is contained in:
Diego Augusto Molina 2024-02-05 16:41:38 -03:00 committed by GitHub
parent b6373249d3
commit a6342fa576
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 86 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
pkg/services/ssosettings/ssosettingsimpl/service.go
View File

@ -196,9 +196,11 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings) erro
return err
}
settings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets)
// make a copy of current settings for reload operation and apply overrides
reloadSettings := *settings
reloadSettings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets)
go s.reload(social, settings.Provider, *settings)
go s.reload(social, settings.Provider, reloadSettings)
return nil
}

83
pkg/services/ssosettings/ssosettingsimpl/service_test.go
View File

@ -26,6 +26,8 @@ import (
)
func TestService_GetForProvider(t *testing.T) {
t.Parallel()
testCases := []struct {
name string
setup func(env testEnv)
@ -185,7 +187,13 @@ func TestService_GetForProvider(t *testing.T) {
}
for _, tc := range testCases {
// create a local copy of "tc" to allow concurrent access within tests to the different items of testCases,
// otherwise it would be like a moving pointer while tests run in parallel
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
if tc.setup != nil {
tc.setup(env)
@ -207,6 +215,8 @@ func TestService_GetForProvider(t *testing.T) {
}
func TestService_GetForProviderWithRedactedSecrets(t *testing.T) {
t.Parallel()
testCases := []struct {
name string
setup func(env testEnv)
@ -286,7 +296,13 @@ func TestService_GetForProviderWithRedactedSecrets(t *testing.T) {
}
for _, tc := range testCases {
// create a local copy of "tc" to allow concurrent access within tests to the different items of testCases,
// otherwise it would be like a moving pointer while tests run in parallel
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
if tc.setup != nil {
tc.setup(env)
@ -306,6 +322,8 @@ func TestService_GetForProviderWithRedactedSecrets(t *testing.T) {
}
func TestService_List(t *testing.T) {
t.Parallel()
testCases := []struct {
name string
setup func(env testEnv)
@ -429,7 +447,13 @@ func TestService_List(t *testing.T) {
},
}
for _, tc := range testCases {
// create a local copy of "tc" to allow concurrent access within tests to the different items of testCases,
// otherwise it would be like a moving pointer while tests run in parallel
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
if tc.setup != nil {
tc.setup(env)
@ -449,6 +473,8 @@ func TestService_List(t *testing.T) {
}
func TestService_ListWithRedactedSecrets(t *testing.T) {
t.Parallel()
testCases := []struct {
name string
setup func(env testEnv)
@ -723,7 +749,13 @@ func TestService_ListWithRedactedSecrets(t *testing.T) {
},
}
for _, tc := range testCases {
// create a local copy of "tc" to allow concurrent access within tests to the different items of testCases,
// otherwise it would be like a moving pointer while tests run in parallel
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
if tc.setup != nil {
tc.setup(env)
@ -743,7 +775,11 @@ func TestService_ListWithRedactedSecrets(t *testing.T) {
}
func TestService_Upsert(t *testing.T) {
t.Parallel()
t.Run("successfully upsert SSO settings", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -762,7 +798,7 @@ func TestService_Upsert(t *testing.T) {
reloadable := ssosettingstests.NewMockReloadable(t)
reloadable.On("Validate", mock.Anything, settings).Return(nil)
reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool {
wg.Done()
defer wg.Done()
return settings.Provider == provider &&
settings.ID == "someid" &&
maps.Equal(settings.Settings, map[string]any{
@ -805,6 +841,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if provider is not configurable", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.GrafanaComProviderName
@ -826,6 +864,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if provider was not found in reloadables", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -848,6 +888,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if validation fails", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -870,6 +912,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if a fallback strategy is not available for the provider", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
settings := &models.SSOSettings{
@ -889,6 +933,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if secrets encryption failed", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.OktaProviderName
@ -912,6 +958,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("should not update the current secret if the secret has not been updated", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -947,6 +995,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("returns error if store failed to upsert settings", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -977,6 +1027,8 @@ func TestService_Upsert(t *testing.T) {
})
t.Run("successfully upsert SSO settings if reload fails", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -1005,7 +1057,11 @@ func TestService_Upsert(t *testing.T) {
}
func TestService_Delete(t *testing.T) {
t.Parallel()
t.Run("successfully delete SSO settings", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
var wg sync.WaitGroup
@ -1042,6 +1098,8 @@ func TestService_Delete(t *testing.T) {
})
t.Run("return error if SSO setting was not found for the specified provider", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -1051,10 +1109,13 @@ func TestService_Delete(t *testing.T) {
err := env.service.Delete(context.Background(), provider)
require.Error(t, err)
require.ErrorIs(t, err, ssosettings.ErrNotFound)
})
t.Run("should not delete the SSO settings if the provider is not configurable", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
env.cfg.SSOSettingsConfigurableProviders = map[string]bool{social.AzureADProviderName: true}
@ -1066,6 +1127,8 @@ func TestService_Delete(t *testing.T) {
})
t.Run("return error when store fails to delete the SSO settings for the specified provider", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -1077,6 +1140,8 @@ func TestService_Delete(t *testing.T) {
})
t.Run("return successfully when the deletion was successful but reloading the settings fail", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := social.AzureADProviderName
@ -1094,7 +1159,11 @@ func TestService_Delete(t *testing.T) {
}
func TestService_DoReload(t *testing.T) {
t.Parallel()
t.Run("successfully reload settings", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
settingsList := []*models.SSOSettings{
@ -1133,6 +1202,8 @@ func TestService_DoReload(t *testing.T) {
})
t.Run("failed fetching the SSO settings", func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
provider := "github"
@ -1147,6 +1218,8 @@ func TestService_DoReload(t *testing.T) {
}
func TestService_decryptSecrets(t *testing.T) {
t.Parallel()
testCases := []struct {
name string
setup func(env testEnv)
@ -1219,7 +1292,13 @@ func TestService_decryptSecrets(t *testing.T) {
}
for _, tc := range testCases {
// create a local copy of "tc" to allow concurrent access within tests to the different items of testCases,
// otherwise it would be like a moving pointer while tests run in parallel
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
env := setupTestEnv(t)
if tc.setup != nil {
@ -1242,6 +1321,8 @@ func TestService_decryptSecrets(t *testing.T) {
}
func setupTestEnv(t *testing.T) testEnv {
t.Helper()
store := ssosettingstests.NewFakeStore()
fallbackStrategy := ssosettingstests.NewFakeFallbackStrategy()
secrets := secretsFakes.NewMockService(t)