From d98577f7eb8862a660a91d3a8560bb4bbca7b083 Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Fri, 4 Nov 2022 16:28:29 -0600 Subject: [PATCH] Fix racy bundle client tests (#3575) This change fixes test failures in the bundle client package. The failures were caused by non-goroutine safe manipulation of a map of configurations used as a config source and also an errant assertion that didn't account for production code behavior. To fix the non-goroutine safe config source, a new type was introduced that protected the underlying config map with a RW mutex. The errant assertion assumed that only one bundle refresh would be performed for a newly discovered trust domain. However, since the manual refresh operation ends up kicking off a goroutine that will also periodically refresh the bundle, under certain timing conditions, the bundle is refreshed twice. The assertion was updated to ensure that the bundle is updated at least once. Fixes: #2840,#3401 Signed-off-by: Andrew Harding --- pkg/server/bundle/client/manager_test.go | 48 ++++++++++++------------ pkg/server/bundle/client/sources.go | 42 +++++++++++++++++++-- pkg/server/bundle/client/sources_test.go | 12 +++--- pkg/server/server.go | 2 +- 4 files changed, 71 insertions(+), 33 deletions(-) diff --git a/pkg/server/bundle/client/manager_test.go b/pkg/server/bundle/client/manager_test.go index 6d1d390aed..38ccc6ca93 100644 --- a/pkg/server/bundle/client/manager_test.go +++ b/pkg/server/bundle/client/manager_test.go @@ -13,7 +13,6 @@ import ( "github.com/spiffe/spire/pkg/common/telemetry" "github.com/spiffe/spire/test/clock" "github.com/spiffe/spire/test/fakes/fakedatastore" - "github.com/spiffe/spire/test/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/zeebo/errs" @@ -27,12 +26,12 @@ func TestManagerPeriodicBundleRefresh(t *testing.T) { endpointBundle := bundleutil.BundleFromRootCA(trustDomain, createCACertificate(t, "endpoint")) endpointBundle.SetRefreshHint(time.Hour * 2) - source := TrustDomainConfigMap{ + source := NewTrustDomainConfigSet(TrustDomainConfigMap{ trustDomain: TrustDomainConfig{ EndpointURL: "https://example.org/bundle", EndpointProfile: HTTPSWebProfile{}, }, - } + }) testCases := []struct { name string @@ -85,13 +84,9 @@ func TestManagerPeriodicBundleRefresh(t *testing.T) { } func TestManagerOnDemandBundleRefresh(t *testing.T) { - util.SkipFlakyTestUnderRaceDetectorWithFiledIssue( - t, - "https://github.com/spiffe/spire/issues/2840", - ) - trustDomainConfigs := make(TrustDomainConfigMap) + configSet := NewTrustDomainConfigSet(nil) - test := newManagerTest(t, trustDomainConfigs, nil, nil) + test := newManagerTest(t, configSet, nil, nil) // Wait for the config to be refreshed test.WaitForConfigRefresh() @@ -104,15 +99,19 @@ func TestManagerOnDemandBundleRefresh(t *testing.T) { // Now, add the trust domain configuration to the source and assert // that refreshing the bundle reloads configs from the source. - trustDomainConfigs[trustDomain] = TrustDomainConfig{ + configSet.Set(trustDomain, TrustDomainConfig{ EndpointURL: "https://some-domain.test/bundle", EndpointProfile: HTTPSWebProfile{}, - } + }) has, err = test.RefreshBundleFor(trustDomain) assert.True(t, has, "manager should know about the trust domain") assert.EqualError(t, err, "OHNO") - assert.Equal(t, 1, test.UpdateCount(trustDomain)) + + // The update count may be more than 1, since RefreshBundle will update the + // bundle, but also, since the trust domain is newly managed, kick off a + // goroutine that will refresh it as well. + assert.Greater(t, test.UpdateCount(trustDomain), 0) } func TestManagerConfigPeriodicRefresh(t *testing.T) { @@ -141,11 +140,12 @@ func TestManagerConfigPeriodicRefresh(t *testing.T) { }, } - trustDomainConfigs := make(TrustDomainConfigMap) - trustDomainConfigs[td1] = configSPIFFEA - trustDomainConfigs[td2] = configWebA + configSet := NewTrustDomainConfigSet(TrustDomainConfigMap{ + td1: configSPIFFEA, + td2: configWebA, + }) - test := newManagerTest(t, trustDomainConfigs, nil, nil) + test := newManagerTest(t, configSet, nil, nil) // Wait until the config is refreshed and a bundle refresh happens test.WaitForConfigRefresh() @@ -166,9 +166,10 @@ func TestManagerConfigPeriodicRefresh(t *testing.T) { // Now adjust the configuration to drop td1, change td2, and introduce td3. // Both td2 and td3 should have an extra update count. td1 update count will // remain the same. - delete(trustDomainConfigs, td1) - trustDomainConfigs[td2] = configSPIFFEB - trustDomainConfigs[td3] = configWebB + configSet.SetAll(TrustDomainConfigMap{ + td2: configSPIFFEB, + td3: configWebB, + }) // Wait until the config is refreshed and a bundle refresh happens test.AdvanceTime(bundleutil.MinimumRefreshHint + time.Millisecond) @@ -198,10 +199,11 @@ func TestManagerConfigManualRefresh(t *testing.T) { EndpointProfile: HTTPSWebProfile{}, } - trustDomainConfigs := make(TrustDomainConfigMap) - trustDomainConfigs[td1] = config1 + configSet := NewTrustDomainConfigSet(TrustDomainConfigMap{ + td1: config1, + }) - test := newManagerTest(t, trustDomainConfigs, nil, nil) + test := newManagerTest(t, configSet, nil, nil) // Wait for the original config to be loaded test.WaitForConfigRefresh() @@ -210,7 +212,7 @@ func TestManagerConfigManualRefresh(t *testing.T) { }, test.GetTrustDomainConfigs()) // Update config and trigger the reload - trustDomainConfigs[td2] = config2 + configSet.Set(td2, config2) test.manager.TriggerConfigReload() test.WaitForConfigRefresh() require.Equal(t, map[spiffeid.TrustDomain]TrustDomainConfig{ diff --git a/pkg/server/bundle/client/sources.go b/pkg/server/bundle/client/sources.go index f30223f566..b66ff56fc8 100644 --- a/pkg/server/bundle/client/sources.go +++ b/pkg/server/bundle/client/sources.go @@ -2,6 +2,7 @@ package client import ( "context" + "sync" "github.com/sirupsen/logrus" "github.com/spiffe/go-spiffe/v2/spiffeid" @@ -19,10 +20,45 @@ func (fn TrustDomainConfigSourceFunc) GetTrustDomainConfigs(ctx context.Context) return fn(ctx) } -type TrustDomainConfigMap map[spiffeid.TrustDomain]TrustDomainConfig +type TrustDomainConfigMap = map[spiffeid.TrustDomain]TrustDomainConfig -func (m TrustDomainConfigMap) GetTrustDomainConfigs(ctx context.Context) (map[spiffeid.TrustDomain]TrustDomainConfig, error) { - return m, nil +type TrustDomainConfigSet struct { + mtx sync.RWMutex + configMap TrustDomainConfigMap +} + +func NewTrustDomainConfigSet(configs TrustDomainConfigMap) *TrustDomainConfigSet { + s := &TrustDomainConfigSet{} + s.SetAll(configs) + return s +} + +func (s *TrustDomainConfigSet) Set(td spiffeid.TrustDomain, config TrustDomainConfig) { + s.mtx.Lock() + defer s.mtx.Unlock() + s.configMap[td] = config +} + +func (s *TrustDomainConfigSet) SetAll(configMap TrustDomainConfigMap) { + configMap = duplicateTrustDomainConfigMap(configMap) + + s.mtx.Lock() + defer s.mtx.Unlock() + s.configMap = configMap +} + +func (s *TrustDomainConfigSet) GetTrustDomainConfigs(ctx context.Context) (map[spiffeid.TrustDomain]TrustDomainConfig, error) { + s.mtx.RLock() + defer s.mtx.RUnlock() + return s.configMap, nil +} + +func duplicateTrustDomainConfigMap(in TrustDomainConfigMap) TrustDomainConfigMap { + out := make(TrustDomainConfigMap, len(in)) + for td, config := range in { + out[td] = config + } + return out } func MergeTrustDomainConfigSources(sources ...TrustDomainConfigSource) TrustDomainConfigSource { diff --git a/pkg/server/bundle/client/sources_test.go b/pkg/server/bundle/client/sources_test.go index 0dbcead875..ffa498eb45 100644 --- a/pkg/server/bundle/client/sources_test.go +++ b/pkg/server/bundle/client/sources_test.go @@ -21,15 +21,15 @@ var ( ) func TestMergedTrustDomainConfigSource(t *testing.T) { - sourceA := client.TrustDomainConfigMap{ + sourceA := client.NewTrustDomainConfigSet(client.TrustDomainConfigMap{ domain1: client.TrustDomainConfig{EndpointURL: "A"}, - } - sourceB := client.TrustDomainConfigMap{ + }) + sourceB := client.NewTrustDomainConfigSet(client.TrustDomainConfigMap{ domain1: client.TrustDomainConfig{EndpointURL: "B"}, - } - sourceC := client.TrustDomainConfigMap{ + }) + sourceC := client.NewTrustDomainConfigSet(client.TrustDomainConfigMap{ domain2: client.TrustDomainConfig{EndpointURL: "A"}, - } + }) t.Run("context is passed through and error returned", func(t *testing.T) { expectedCtx, cancel := context.WithCancel(context.Background()) diff --git a/pkg/server/server.go b/pkg/server/server.go index e700831e5b..214f550d2a 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -353,7 +353,7 @@ func (s *Server) newBundleManager(cat catalog.Catalog, metrics telemetry.Metrics Metrics: metrics, DataStore: cat.GetDataStore(), Source: bundle_client.MergeTrustDomainConfigSources( - bundle_client.TrustDomainConfigMap(s.config.Federation.FederatesWith), + bundle_client.NewTrustDomainConfigSet(s.config.Federation.FederatesWith), bundle_client.DataStoreTrustDomainConfigSource(log, cat.GetDataStore()), ), })