Skip to content

Commit 9ac5b16

Browse files
committed
[review-fix 2] Keep RC filters standard-only
- Supersede the prior provenance change by restoring standard-only RC attribution and evaluating baseline candidates through an immutable built-in/local filter. - Verify RC includes cannot admit and RC excludes cannot suppress baseline candidates. - Document the mode boundary inline; the PR description carries the same contract. Source: review feedback and maintainer clarification Validation: bazel test --nocache_test_results //comp/networkpath/npcollector/impl:impl_test
1 parent adfc8be commit 9ac5b16

3 files changed

Lines changed: 44 additions & 32 deletions

File tree

comp/networkpath/npcollector/impl/baseline_test.go

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ import (
1515

1616
model "github.com/DataDog/agent-payload/v5/process"
1717
"github.com/DataDog/datadog-agent/comp/networkpath/npcollector/impl/common"
18-
"github.com/DataDog/datadog-agent/comp/networkpath/npcollector/impl/connfilter"
1918
npmodel "github.com/DataDog/datadog-agent/comp/networkpath/npcollector/model"
2019
"github.com/DataDog/datadog-agent/pkg/networkpath/payload"
20+
"github.com/DataDog/datadog-agent/pkg/remoteconfig/state"
2121
"github.com/DataDog/datadog-agent/pkg/trace/teststatsd"
2222
"github.com/stretchr/testify/assert"
2323
"github.com/stretchr/testify/require"
@@ -150,32 +150,28 @@ func TestBaselineKeepsStrongestObservationPerPath(t *testing.T) {
150150
assert.Equal(t, []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}, scheduledBaselineHosts(t, collector))
151151
}
152152

153-
func TestBaselinePreservesWinningRCProvenance(t *testing.T) {
153+
func TestBaselineIgnoresDynamicRemoteConfigFilters(t *testing.T) {
154154
_, collector := newTestNpCollector(t, map[string]any{
155155
"network_path.connections_monitoring.baseline_tests_enabled": true,
156156
"network_path.collector.monitor_ip_without_domain": true,
157+
"network_path.collector.filters": []map[string]any{{
158+
"type": "exclude",
159+
"match_ip": "10.0.0.1",
160+
}},
157161
}, &teststatsd.Client{}, nil)
158-
filter, errs := connfilter.NewConnFilter([]connfilter.Config{
159-
{Type: connfilter.FilterTypeExclude, MatchIP: "10.0.0.1"},
160-
{
161-
Type: connfilter.FilterTypeInclude,
162-
MatchIP: "10.0.0.1",
163-
TestConfigID: "dynamic-a",
164-
Tags: []string{"team:payments"},
165-
},
166-
}, "", false)
167-
require.Empty(t, errs)
168-
collector.filter = filter
162+
collector.UpdateRemoteConfig(map[string]state.RawConfig{
163+
"dynamic": {Config: dynamicConfig("dynamic-a", `[
164+
{"type":"include","match_ip":"10.0.0.1"},
165+
{"type":"exclude","match_ip":"10.0.0.2"}
166+
]`)},
167+
}, func(string, state.ApplyStatus) {})
169168

170169
collector.ScheduleNetworkPathTests(slices.Values([]npmodel.NetworkPathConnection{
171170
baselineConn("10.0.0.1", 1),
171+
baselineConn("10.0.0.2", 2),
172172
}))
173173

174-
pathtest := <-collector.pathtestInputChan
175-
assert.Equal(t, payload.DynamicTestProfileBaseline, pathtest.DynamicTestProfile)
176-
assert.Equal(t, "dynamic-a", pathtest.TestConfigID)
177-
assert.Equal(t, payload.TestConfigSourceRemote, pathtest.TestConfigSource)
178-
assert.Equal(t, []string{"team:payments"}, pathtest.Tags)
174+
assert.Equal(t, []string{"10.0.0.2"}, scheduledBaselineHosts(t, collector), "RC filters must neither admit nor exclude baseline candidates")
179175
}
180176

181177
func TestBaselineEmptySnapshotsSelectNothing(t *testing.T) {

comp/networkpath/npcollector/impl/npcollector.go

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ type npCollectorImpl struct {
8484
networkDevicesNamespace string
8585
filterMutex sync.RWMutex
8686
filter *connfilter.ConnFilter
87+
localFilter *connfilter.ConnFilter
8788
localIPs *localIPCache
8889
remoteConfigState dynamicRemoteConfigState
8990
}
@@ -133,7 +134,8 @@ func newNpCollectorImpl(epForwarder eventplatform.Forwarder, collectorConfigs *c
133134
flushLoopDone: make(chan struct{}),
134135
workersDone: make(chan struct{}),
135136

136-
filter: filter,
137+
filter: filter,
138+
localFilter: filter,
137139
}
138140
}
139141

@@ -212,7 +214,7 @@ type pathEvaluation struct {
212214
tags []string
213215
}
214216

215-
func (s *npCollectorImpl) evaluateNetworkPathForConn(conn npmodel.NetworkPathConnection, origin payload.PathOrigin, vpcSubnets []netip.Prefix) pathEvaluation {
217+
func (s *npCollectorImpl) evaluateNetworkPathForConn(conn npmodel.NetworkPathConnection, origin payload.PathOrigin, vpcSubnets []netip.Prefix, baselineMode bool) pathEvaluation {
216218
if conn.IntraHost {
217219
_ = s.statsdClient.Incr(netpathConnsSkippedMetricName, []string{"reason:skip_intra_host"}, 1)
218220
return pathEvaluation{}
@@ -241,9 +243,18 @@ func (s *npCollectorImpl) evaluateNetworkPathForConn(conn npmodel.NetworkPathCon
241243
return pathEvaluation{}
242244
}
243245

244-
s.filterMutex.RLock()
245-
included, testConfigID, tags := s.filter.EvaluateWithTags(conn.Domain, conn.Dest.Addr())
246-
s.filterMutex.RUnlock()
246+
var included bool
247+
var testConfigID string
248+
var tags []string
249+
if baselineMode {
250+
// Dynamic Remote Configuration admits standard tests only. Baseline
251+
// selection must remain governed by built-in and local filters.
252+
included, testConfigID, tags = s.localFilter.EvaluateWithTags(conn.Domain, conn.Dest.Addr())
253+
} else {
254+
s.filterMutex.RLock()
255+
included, testConfigID, tags = s.filter.EvaluateWithTags(conn.Domain, conn.Dest.Addr())
256+
s.filterMutex.RUnlock()
257+
}
247258
if !included {
248259
_ = s.statsdClient.Incr(netpathConnsSkippedMetricName, []string{"reason:skip_not_matched_by_filters"}, 1)
249260
return pathEvaluation{}
@@ -323,23 +334,18 @@ func (s *npCollectorImpl) scheduleNetworkPathTests(origin payload.PathOrigin, co
323334
}
324335
for conn := range conns {
325336
connCount++
326-
evaluation := s.evaluateNetworkPathForConn(conn, origin, vpcSubnets)
337+
evaluation := s.evaluateNetworkPathForConn(conn, origin, vpcSubnets, baselineMode)
327338
if !evaluation.shouldSchedule {
328339
s.logger.Tracef("Skipped connection: addr=%s, protocol=%s", conn.Dest, conn.Type)
329340
continue
330341
}
331342
pathtest := s.makePathtest(conn, origin)
332-
pathtest.TestConfigID = evaluation.testConfigID
333-
pathtest.Tags = evaluation.tags
334-
if evaluation.testConfigID != "" {
335-
pathtest.TestConfigSource = payload.TestConfigSourceRemote
336-
}
337343
if baselineMode {
338344
selectedBaselineCandidates = addBaselinePath(selectedBaselineCandidates, pathtest, conn.Signals)
339345
continue
340346
}
341347

342-
if err := s.scheduleOne(&pathtest); err != nil {
348+
if err := s.scheduleStandardNetworkPathTest(pathtest, evaluation); err != nil {
343349
s.logger.Errorf("Error scheduling pathtests: %s", err)
344350
}
345351
}
@@ -349,6 +355,16 @@ func (s *npCollectorImpl) scheduleNetworkPathTests(origin payload.PathOrigin, co
349355
_ = s.statsdClient.Count(common.NetworkPathCollectorMetricPrefix+"schedule.conns_received", int64(connCount), []string{}, 1)
350356
_ = s.statsdClient.Gauge(common.NetworkPathCollectorMetricPrefix+"schedule.duration", s.TimeNowFn().Sub(startTime).Seconds(), nil, 1)
351357
}
358+
359+
func (s *npCollectorImpl) scheduleStandardNetworkPathTest(pathtest common.Pathtest, evaluation pathEvaluation) error {
360+
pathtest.TestConfigID = evaluation.testConfigID
361+
pathtest.Tags = evaluation.tags
362+
if evaluation.testConfigID != "" {
363+
pathtest.TestConfigSource = payload.TestConfigSourceRemote
364+
}
365+
return s.scheduleOne(&pathtest)
366+
}
367+
352368
func (s *npCollectorImpl) scheduleBaselinePaths(selected []baselineCandidate) {
353369
for i := range selected {
354370
if err := s.scheduleOne(&selected[i].path); err != nil {

comp/networkpath/npcollector/impl/npcollector_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2358,7 +2358,7 @@ network_path:
23582358
stats := &teststatsd.Client{}
23592359
_, npCollector := newTestNpCollector(t, agentConfigs, stats, nil)
23602360

2361-
require.Equal(t, tt.shouldSchedule, npCollector.evaluateNetworkPathForConn(tt.conn, payload.PathOriginNetworkTraffic, tt.vpcSubnets).shouldSchedule)
2361+
require.Equal(t, tt.shouldSchedule, npCollector.evaluateNetworkPathForConn(tt.conn, payload.PathOriginNetworkTraffic, tt.vpcSubnets, false).shouldSchedule)
23622362

23632363
if tt.subnetSkipped {
23642364
require.Contains(t, stats.CountCalls, subnetSkippedStat)
@@ -2424,7 +2424,7 @@ func Test_npCollectorImpl_evaluateNetworkPathForConn_subnets(t *testing.T) {
24242424
stats := &teststatsd.Client{}
24252425
_, npCollector := newTestNpCollector(t, agentConfigs, stats, nil)
24262426

2427-
assert.Equal(t, tt.shouldSchedule, npCollector.evaluateNetworkPathForConn(tt.conn, payload.PathOriginNetworkTraffic, nil).shouldSchedule)
2427+
assert.Equal(t, tt.shouldSchedule, npCollector.evaluateNetworkPathForConn(tt.conn, payload.PathOriginNetworkTraffic, nil, false).shouldSchedule)
24282428

24292429
if tt.subnetSkipped {
24302430
require.Contains(t, stats.CountCalls, subnetSkippedStat)

0 commit comments

Comments
 (0)