Skip to content

Commit e92f34a

Browse files
authored
fix: data race on DCAClient.clusterAgentAPIClient in initHTTPClient (#54638)
### What does this PR do? Fixes a data race on `DCAClient.clusterAgentAPIClient`: an unlocked bootstrap check raced with the field's other, locked accesses. Also guards against `startReconnectHandler()` being launched more than once concurrently. ### Motivation Fix a race, found via a race-detector-enabled build in staging. ### Describe how you validated your changes Added a concurrent test on `initHTTPClient()`; fails under `-race` before this fix, passes after. Co-authored-by: pierre.gimalac <pierre.gimalac@datadoghq.com>
1 parent f1c9911 commit e92f34a

2 files changed

Lines changed: 63 additions & 2 deletions

File tree

pkg/util/clusteragent/clusteragent.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ type DCAClient struct {
104104
clusterAgentVersion version.Version // Version of the cluster-agent we're connected to
105105
clusterAgentAPIClient *http.Client
106106
leaderClient *leaderClient
107+
108+
// reconnectHandlerOnce ensures startReconnectHandler is only ever started once for
109+
// this DCAClient, even if init() runs concurrently (e.g. via overlapping retries).
110+
reconnectHandlerOnce sync.Once
107111
}
108112

109113
// resetGlobalClusterAgentClient is a helper to remove the current DCAClient global
@@ -153,8 +157,12 @@ func (c *DCAClient) init() error {
153157
return err
154158
}
155159

156-
// Run DCA connection refresh
157-
c.startReconnectHandler(time.Duration(pkgconfigsetup.Datadog().GetInt64("cluster_agent.client_reconnect_period_seconds")) * time.Second)
160+
// Run DCA connection refresh. Guarded by a sync.Once so that concurrent calls to
161+
// init() (e.g. overlapping retries from GetClusterAgentClient) don't spawn multiple
162+
// reconnect-handler goroutines racing on this DCAClient's state.
163+
c.reconnectHandlerOnce.Do(func() {
164+
c.startReconnectHandler(time.Duration(pkgconfigsetup.Datadog().GetInt64("cluster_agent.client_reconnect_period_seconds")) * time.Second)
165+
})
158166

159167
log.Infof("Successfully connected to the Datadog Cluster Agent %s", c.clusterAgentVersion.String())
160168
return nil
@@ -208,9 +216,11 @@ func (c *DCAClient) initHTTPClient() error {
208216
}
209217

210218
// We need to have a client to perform `GetVersion`, only happens during the first call
219+
c.clusterAgentClientLock.Lock()
211220
if c.clusterAgentAPIClient == nil {
212221
c.clusterAgentAPIClient = clusterAgentAPIClient
213222
}
223+
c.clusterAgentClientLock.Unlock()
214224

215225
// Validate the cluster-agent client by checking the version
216226
clusterAgentVersion, err := c.getVersion()

pkg/util/clusteragent/clusteragent_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -893,3 +893,54 @@ func (suite *clusterAgentSuite) TestDCAClientCertificateVerification() {
893893
})
894894
}
895895
}
896+
897+
// TestInitHTTPClientConcurrent calls initHTTPClient() concurrently to catch data races on DCAClient state.
898+
func (suite *clusterAgentSuite) TestInitHTTPClientConcurrent() {
899+
defer pkgapiutil.TestOnlyResetCrossNodeClientTLSConfig()
900+
pkgapiutil.TestOnlyResetCrossNodeClientTLSConfig()
901+
902+
dca, err := newDummyClusterAgent(suite.config)
903+
require.Nil(suite.T(), err, fmt.Sprintf("%v", err))
904+
905+
ts, p, err := dca.StartTLS()
906+
require.Nil(suite.T(), err, fmt.Sprintf("%v", err))
907+
defer ts.Close()
908+
909+
pkgapiutil.SetCrossNodeClientTLSConfig(&tls.Config{
910+
InsecureSkipVerify: true,
911+
})
912+
913+
c := &DCAClient{
914+
clusterAgentAPIEndpoint: fmt.Sprintf("https://127.0.0.1:%d", p),
915+
}
916+
c.clusterAgentAPIRequestHeaders = http.Header{}
917+
c.clusterAgentAPIRequestHeaders.Set(authorizationHeaderKey, "Bearer "+clusterAgentTokenValue)
918+
c.clusterAgentAPIRequestHeaders.Set(RealIPHeader, clcRunnerIP)
919+
920+
// dummyClusterAgent.requests is a bounded channel that nothing else drains here;
921+
// keep it flowing so ServeHTTP never blocks on a full channel and slows the test down.
922+
stopDrain := make(chan struct{})
923+
defer close(stopDrain)
924+
go func() {
925+
for {
926+
select {
927+
case <-dca.requests:
928+
case <-stopDrain:
929+
return
930+
}
931+
}
932+
}()
933+
934+
const goroutines = 10
935+
const iterations = 10
936+
937+
var wg sync.WaitGroup
938+
for i := 0; i < goroutines; i++ {
939+
wg.Go(func() {
940+
for j := 0; j < iterations; j++ {
941+
_ = c.initHTTPClient()
942+
}
943+
})
944+
}
945+
wg.Wait()
946+
}

0 commit comments

Comments
 (0)