Fix racy bundle client tests (spiffe#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: spiffe#2840,spiffe#3401

Signed-off-by: Andrew Harding <aharding@vmware.com>

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{

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 {

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())

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()),
 		),
 	})