Skip to content

Commit 9525b84

Browse files
authored
fix: publish-before-populate data race in compliance reportCheckEvents (#54661)
<!--Please give us some feedback on your experience writing this PR ! https://app.datadoghq.com/forms/43db4c02-6837-400c-8083-692e141b1b88 !--> ### What does this PR do? Fixes a data race in `pkg/compliance`'s `reportCheckEvents()`: it mutated `event.Container.Image*` and `event.K8SManaged` after `updateEvent()` had already published the event into `a.statuses`, where the expvar/status endpoint reads it without a lock. Reorders those mutations to happen before publish, and makes `updateEvent()`/`getChecksStatus()` store/return copies so nothing outside the lock can mutate what a concurrent reader sees. ### Motivation Found via a race-detector-enabled build in staging, which reported isolated `WARNING: DATA RACE` occurrences in system-probe whenever the status endpoint was scraped while a compliance check event was being reported. ### Describe how you validated your changes Added `TestReportCheckEventsConcurrentStatusRead`, which races `reportCheckEvents()` against concurrent reads of the checks status. Confirmed it fails under `-race` before this fix and passes cleanly after; full `pkg/compliance` suite (137 tests) also passes under `-race`. ### Additional Notes No behavior change — events are published fully-formed instead of partially, same final content. Co-authored-by: pierre.gimalac <pierre.gimalac@datadoghq.com>
1 parent e92f34a commit 9525b84

3 files changed

Lines changed: 122 additions & 10 deletions

File tree

pkg/compliance/BUILD.bazel

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ go_library(
191191
dd_agent_go_test(
192192
name = "compliance_test",
193193
srcs = [
194+
"agent_race_test.go",
194195
"agent_test.go",
195196
"cri_test.go",
196197
"k8s_reflectors_test.go",
@@ -213,10 +214,17 @@ dd_agent_go_test(
213214
],
214215
],
215216
deps = [
217+
"//comp/core/config",
218+
"//comp/core/log/mock",
216219
"//comp/core/secrets/noop-impl",
220+
"//comp/core/workloadmeta/def",
221+
"//comp/core/workloadmeta/impl",
222+
"//comp/def",
217223
"//comp/logs-library/client",
218224
"//comp/logs/agent/config",
219225
"//comp/serializer/logscompression/fx-mock",
226+
"//pkg/logs/message",
227+
"//pkg/logs/sources",
220228
"@com_github_stretchr_testify//assert",
221229
"@com_github_stretchr_testify//require",
222230
"@io_k8s_api//rbac/v1:rbac",

pkg/compliance/agent.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -575,18 +575,21 @@ func (a *Agent) reportCheckEvents(eventsTTL time.Duration, events ...*CheckEvent
575575
eventsExpireAt := time.Now().Add(2 * eventsTTL).Truncate(1 * time.Second)
576576
for _, event := range events {
577577
event.ExpireAt = &eventsExpireAt
578+
// Mutate event fully before updateEvent() publishes it into a.statuses.
579+
if event.Result != CheckSkipped {
580+
if a.wmeta != nil && event.Container != nil {
581+
if ctnr, _ := a.wmeta.GetContainer(event.Container.ContainerID); ctnr != nil {
582+
event.Container.ImageID = ctnr.Image.ID
583+
event.Container.ImageName = ctnr.Image.Name
584+
event.Container.ImageTag = ctnr.Image.Tag
585+
}
586+
}
587+
event.K8SManaged = a.k8sManaged
588+
}
578589
a.updateEvent(event)
579590
if event.Result == CheckSkipped {
580591
continue
581592
}
582-
if a.wmeta != nil && event.Container != nil {
583-
if ctnr, _ := a.wmeta.GetContainer(event.Container.ContainerID); ctnr != nil {
584-
event.Container.ImageID = ctnr.Image.ID
585-
event.Container.ImageName = ctnr.Image.Name
586-
event.Container.ImageTag = ctnr.Image.Tag
587-
}
588-
}
589-
event.K8SManaged = a.k8sManaged
590593
a.opts.Reporter.ReportEvent(event)
591594
}
592595
}
@@ -613,7 +616,9 @@ func (a *Agent) getChecksStatus() []*CheckStatus {
613616
defer a.statusesMu.RUnlock()
614617
statuses := make([]*CheckStatus, 0, len(a.statuses))
615618
for _, status := range a.statuses {
616-
statuses = append(statuses, status)
619+
// Copy under the lock: callers marshal the result without holding it.
620+
statusCopy := *status
621+
statuses = append(statuses, &statusCopy)
617622
}
618623
return statuses
619624
}
@@ -657,7 +662,9 @@ func (a *Agent) updateEvent(event *CheckEvent) {
657662
if !ok || status == nil {
658663
log.Errorf("check for rule=%s was not registered in checks monitor statuses", event.RuleID)
659664
} else {
660-
status.LastEvent = event
665+
// Publish a copy: callers must not be able to mutate it afterwards.
666+
eventCopy := *event
667+
status.LastEvent = &eventCopy
661668
}
662669
}
663670

pkg/compliance/agent_race_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed
2+
// under the Apache License Version 2.0.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
// Copyright 2016-present Datadog, Inc.
5+
6+
package compliance
7+
8+
import (
9+
"expvar"
10+
"sync"
11+
"testing"
12+
"time"
13+
14+
"github.com/DataDog/datadog-agent/comp/core/config"
15+
logmock "github.com/DataDog/datadog-agent/comp/core/log/mock"
16+
workloadmeta "github.com/DataDog/datadog-agent/comp/core/workloadmeta/def"
17+
workloadmetaimpl "github.com/DataDog/datadog-agent/comp/core/workloadmeta/impl"
18+
compdef "github.com/DataDog/datadog-agent/comp/def"
19+
logsconfig "github.com/DataDog/datadog-agent/comp/logs/agent/config"
20+
"github.com/DataDog/datadog-agent/pkg/logs/message"
21+
"github.com/DataDog/datadog-agent/pkg/logs/sources"
22+
)
23+
24+
// TestReportCheckEventsConcurrentStatusRead races reportCheckEvents() against
25+
// concurrent status reads, mirroring the expvar/status endpoint. Run with -race.
26+
func TestReportCheckEventsConcurrentStatusRead(t *testing.T) {
27+
const containerID = "abc123"
28+
29+
wmetaMock := workloadmetaimpl.NewWorkloadMetaMock(workloadmetaimpl.Dependencies{
30+
Lc: compdef.NewTestLifecycle(t),
31+
Log: logmock.New(t),
32+
Config: config.NewMock(t),
33+
Params: workloadmeta.NewParams(),
34+
})
35+
wmetaMock.Set(&workloadmeta.Container{
36+
EntityID: workloadmeta.EntityID{Kind: workloadmeta.KindContainer, ID: containerID},
37+
Image: workloadmeta.ContainerImage{
38+
ID: "sha256:deadbeef",
39+
Name: "redis",
40+
Tag: "latest",
41+
},
42+
})
43+
44+
// LogReporter is built directly (same package) with a drained, buffered
45+
// channel instead of a real network pipeline, so ReportEvent never blocks.
46+
logChan := make(chan *message.Message, 100)
47+
go func() {
48+
for msg := range logChan {
49+
_ = msg
50+
}
51+
}()
52+
reporter := &LogReporter{
53+
hostname: "test-host",
54+
logSource: sources.NewLogSource("test", &logsconfig.LogsConfig{Type: "test", Source: "test"}),
55+
logChan: logChan,
56+
endpoints: &logsconfig.Endpoints{},
57+
}
58+
59+
managedEnv := "test-managed-env"
60+
a := &Agent{
61+
wmeta: wmetaMock,
62+
opts: AgentOptions{Reporter: reporter},
63+
statuses: map[string]*CheckStatus{
64+
"rule-1": {RuleID: "rule-1"},
65+
},
66+
k8sManaged: &managedEnv,
67+
}
68+
69+
statusFn := expvar.Func(func() interface{} { return a.getChecksStatus() })
70+
71+
stop := make(chan struct{})
72+
var wg sync.WaitGroup
73+
wg.Go(func() {
74+
for {
75+
select {
76+
case <-stop:
77+
return
78+
default:
79+
_ = statusFn.String()
80+
}
81+
}
82+
})
83+
84+
for i := 0; i < 2000; i++ {
85+
event := &CheckEvent{
86+
RuleID: "rule-1",
87+
Result: CheckPassed,
88+
Container: &CheckContainerMeta{
89+
ContainerID: containerID,
90+
},
91+
}
92+
a.reportCheckEvents(time.Minute, event)
93+
}
94+
95+
close(stop)
96+
wg.Wait()
97+
}

0 commit comments

Comments
 (0)