Skip to content

Commit 68e4d34

Browse files
hmahmoodhmahmood
authored
VULN-92045 resolve DatadogMetric references in the referencing namespace (#54218)
<!-- dd-meta {"pullId":"00000000-0000-0000-0000-000000000000","source":"chat","resourceId":"c4ad7c08-91ff-417c-ba4c-46e80bea665a","workflowId":"8c98a1cb-9292-475c-92c3-1ad5cdb2fd04","codeChangeId":"8c98a1cb-9292-475c-92c3-1ad5cdb2fd04","sourceType":"chat"} --> <!--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? Makes a `datadogmetric@<namespace>:<name>` external metric reference always resolve in the namespace of the object doing the referencing, instead of the namespace embedded in the metric name. - `metricNameToDatadogMetricID` now takes the referencing namespace and builds the DatadogMetric ID from it. The `<namespace>:` part of the reference becomes an optional group in the parser and is discarded at read time. - `datadogMetricProvider.getExternalMetric` passes the request namespace, so a request from namespace `A` for `datadogmetric@B:name` looks up `A/name` and returns the usual not-found error. - `AutoscalerWatcher.extractAutoscalerReference` passes the HPA/WPA namespace, so a cross-namespace reference can no longer mark a foreign DatadogMetric active or make `metrics_retriever` refresh it. - `datadogmetric@<name>` (no namespace) is now a valid reference and resolves to the referencing namespace, so the namespace is only ever redundant, never meaningful. - Adds coverage for a tenant namespace attempting to read or activate a DatadogMetric owned by another namespace, and for the namespace-less reference form. ### Motivation Addresses VULN-92045. The external metrics provider resolved a DatadogMetric using only the namespace embedded in the metric name, with no comparison against the namespace the request was authorized for. In a multi-tenant cluster, tenant `A` could read tenant `B`'s DatadogMetric values, and — through the autoscaler watcher — mark `B`'s DatadogMetrics active, mutating their status and forcing their Datadog queries to run against shared API quota. Cross-namespace sharing was the original intent of the `<namespace>:` syntax, but it has no authorization model behind it. Rather than adding one, this PR drops the capability and keeps the syntax accepted-but-ignored so existing same-namespace manifests keep working unchanged. ### Describe how you validated your changes - `dda inv test --targets=./pkg/clusteragent/autoscaling/externalmetrics --build-include=kubeapiserver,test` — 110 tests pass. (The `test` build tag is required, otherwise `datadogmetric_controller_test.go` fails to build against `autoscaling.FilterInformerActions`.) - `dda inv linter.go --targets=./pkg/clusteragent/autoscaling/externalmetrics --build-tags=kubeapiserver,test` — 0 issues. ### Additional Notes Behavior change for anyone relying on cross-namespace references: an HPA in namespace `A` pointing at `datadogmetric@B:name` now resolves to `A/name` and fails as not-found rather than reading `B/name`. There is no way to opt back in. `datadogmetric@:<name>` (empty namespace) does not parse, matching the behavior before this PR. Created by Auto-JIRA (https://github.com/DataDog/datadog-agent/blob/main/.agents/skills/auto-jira/SKILL.md). --- PR by Bits - [View session in Datadog](https://app.datadoghq.com/code/c4ad7c08-91ff-417c-ba4c-46e80bea665a) Comment @DataDog to request changes Co-authored-by: hasan.mahmood <hasan.mahmood@datadoghq.com>
1 parent 8cc30d2 commit 68e4d34

7 files changed

Lines changed: 192 additions & 28 deletions

File tree

‎pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ func (w *AutoscalerWatcher) getAutoscalerReferences() (map[string]*externalMetri
300300

301301
external := metric.External
302302
ref := buildAutoscalerReference(autoscalerWPAKindKey, wpa.ObjectMeta)
303-
ddMetricID, metricName, metricLabels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, allowAutogen)
303+
ddMetricID, metricName, metricLabels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, wpa.Namespace, allowAutogen)
304304
if ok {
305305
addAutoscalerReference(ddMetricID, ref, metricName, metricLabels)
306306
}
@@ -340,7 +340,7 @@ func (w *AutoscalerWatcher) processHPAv2beta1Reference(addAutoscalerReference ad
340340
}
341341

342342
external := metric.External
343-
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, allowAutogen)
343+
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, hpa.Namespace, allowAutogen)
344344
if ok {
345345
addAutoscalerReference(ddMetricID, ref, metricName, labels)
346346
}
@@ -356,7 +356,7 @@ func (w *AutoscalerWatcher) processHPAv2beta2Reference(addAutoscalerReference ad
356356
}
357357

358358
external := metric.External
359-
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, allowAutogen)
359+
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, hpa.Namespace, allowAutogen)
360360
if ok {
361361
addAutoscalerReference(ddMetricID, ref, metricName, labels)
362362
}
@@ -372,7 +372,7 @@ func (w *AutoscalerWatcher) processHPAv2Reference(addAutoscalerReference addAuto
372372
}
373373

374374
external := metric.External
375-
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, allowAutogen)
375+
ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, hpa.Namespace, allowAutogen)
376376
if ok {
377377
addAutoscalerReference(ddMetricID, ref, metricName, labels)
378378
}
@@ -382,14 +382,15 @@ func (w *AutoscalerWatcher) processHPAv2Reference(addAutoscalerReference addAuto
382382
func (w *AutoscalerWatcher) extractAutoscalerReference(
383383
externalMetricName string,
384384
externalMetricSelector *metav1.LabelSelector,
385+
autoscalerNamespace string,
385386
allowAutogen bool,
386387
) (
387388
ddMetricID string,
388389
metricName string,
389390
labels map[string]string,
390391
ok bool,
391392
) {
392-
ddMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(externalMetricName)
393+
ddMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(externalMetricName, autoscalerNamespace)
393394
if parsed {
394395
// datadogmetric@ references are always tracked regardless of hpaLabelSelector — the selector controls autogen only.
395396
return ddMetricID, "", nil, true

‎pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher_test.go‎

Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
165165
{
166166
Type: autoscaler.ExternalMetricSourceType,
167167
External: &autoscaler.ExternalMetricSource{
168-
MetricName: "datadogmetric@default:dd-metric-0",
168+
MetricName: "datadogmetric@dd-metric-0",
169169
},
170170
},
171171
}),
@@ -180,34 +180,34 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
180180
newFakeWatermarkPodAutoscaler("ns0", "wpa0", []interface{}{
181181
map[string]interface{}{
182182
"external": map[string]interface{}{
183-
"metricName": "datadogmetric@default:dd-metric-1",
183+
"metricName": "datadogmetric@dd-metric-1",
184184
},
185185
"type": "External",
186186
},
187187
}),
188188
}
189189

190190
ddm := model.DatadogMetricInternal{
191-
ID: "default/dd-metric-0",
191+
ID: "ns0/dd-metric-0",
192192
Active: false,
193193
Valid: true,
194194
Value: 10.0,
195195
UpdateTime: updateTime,
196196
Error: nil,
197197
}
198198
ddm.SetQueries("metric query0")
199-
f.store.Set("default/dd-metric-0", ddm, "utest")
199+
f.store.Set("ns0/dd-metric-0", ddm, "utest")
200200

201201
ddm = model.DatadogMetricInternal{
202-
ID: "default/dd-metric-1",
202+
ID: "ns0/dd-metric-1",
203203
Active: true,
204204
Valid: true,
205205
Value: 11.0,
206206
UpdateTime: updateTime,
207207
Error: nil,
208208
}
209209
ddm.SetQueries("metric query1")
210-
f.store.Set("default/dd-metric-1", ddm, "utest")
210+
f.store.Set("ns0/dd-metric-1", ddm, "utest")
211211

212212
ddm = model.DatadogMetricInternal{
213213
ID: "default/dd-metric-2",
@@ -226,7 +226,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
226226
// Check internal store content
227227
assert.Equal(t, 3, f.store.Count())
228228
ddm = model.DatadogMetricInternal{
229-
ID: "default/dd-metric-0",
229+
ID: "ns0/dd-metric-0",
230230
Active: true,
231231
Valid: true,
232232
Value: 10.0,
@@ -235,10 +235,10 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
235235
AutoscalerReferences: "hpa:ns0/hpa0",
236236
}
237237
ddm.SetQueries("metric query0")
238-
compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-0"))
238+
compareDatadogMetricInternal(t, &ddm, f.store.Get("ns0/dd-metric-0"))
239239

240240
ddm = model.DatadogMetricInternal{
241-
ID: "default/dd-metric-1",
241+
ID: "ns0/dd-metric-1",
242242
Active: true,
243243
Valid: true,
244244
Value: 11.0,
@@ -247,7 +247,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
247247
AutoscalerReferences: "wpa:ns0/wpa0",
248248
}
249249
ddm.SetQueries("metric query1")
250-
compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-1"))
250+
compareDatadogMetricInternal(t, &ddm, f.store.Get("ns0/dd-metric-1"))
251251

252252
ddm = model.DatadogMetricInternal{
253253
ID: "default/dd-metric-2",
@@ -262,6 +262,62 @@ func TestUpdateAutoscalerReferences(t *testing.T) {
262262
compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-2"))
263263
}
264264

265+
// The namespace part of a `datadogmetric@<namespace>:<name>` reference is ignored, the DatadogMetric
266+
// is always resolved in the namespace of the autoscaler, so a reference to another namespace never
267+
// activates the DatadogMetric it points to.
268+
func TestAutoscalerWatcherIgnoresDatadogMetricReferenceNamespace(t *testing.T) {
269+
f := newAutoscalerFixture(t)
270+
updateTime := time.Now()
271+
272+
f.hpaLister = []*autoscaler.HorizontalPodAutoscaler{
273+
newFakeHorizontalPodAutoscaler("tenant-a", "hpa0", []autoscaler.MetricSpec{
274+
{
275+
Type: autoscaler.ExternalMetricSourceType,
276+
External: &autoscaler.ExternalMetricSource{
277+
MetricName: "datadogmetric@shared:dd-metric-0",
278+
},
279+
},
280+
}),
281+
}
282+
283+
f.wpaLister = []*unstructured.Unstructured{
284+
newFakeWatermarkPodAutoscaler("tenant-b", "wpa0", []interface{}{
285+
map[string]interface{}{
286+
"external": map[string]interface{}{
287+
"metricName": "datadogmetric@shared:dd-metric-0",
288+
},
289+
"type": "External",
290+
},
291+
}),
292+
}
293+
294+
ddm := model.DatadogMetricInternal{
295+
ID: "shared/dd-metric-0",
296+
Active: false,
297+
Valid: true,
298+
Value: 10.0,
299+
UpdateTime: updateTime,
300+
Error: nil,
301+
}
302+
ddm.SetQueries("metric query0")
303+
f.store.Set("shared/dd-metric-0", ddm, "utest")
304+
305+
f.runWatcherUpdate()
306+
307+
assert.Equal(t, 1, f.store.Count())
308+
ddm = model.DatadogMetricInternal{
309+
ID: "shared/dd-metric-0",
310+
Active: false,
311+
Valid: true,
312+
Value: 10.0,
313+
UpdateTime: updateTime,
314+
Error: nil,
315+
AutoscalerReferences: "",
316+
}
317+
ddm.SetQueries("metric query0")
318+
compareDatadogMetricInternal(t, &ddm, f.store.Get("shared/dd-metric-0"))
319+
}
320+
265321
func TestCreateAutogenDatadogMetrics(t *testing.T) {
266322
f := newAutoscalerFixture(t)
267323
updateTime := time.Now()
@@ -523,7 +579,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) {
523579
{
524580
Type: autoscaler.ExternalMetricSourceType,
525581
External: &autoscaler.ExternalMetricSource{
526-
MetricName: "datadogmetric@default:dd-metric-ref",
582+
MetricName: "datadogmetric@ns0:dd-metric-ref",
527583
},
528584
},
529585
}),
@@ -571,15 +627,15 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) {
571627
}
572628

573629
ddm := model.DatadogMetricInternal{
574-
ID: "default/dd-metric-ref",
630+
ID: "ns0/dd-metric-ref",
575631
Active: false,
576632
Valid: true,
577633
Value: 20.0,
578634
UpdateTime: time.Now(),
579635
Error: nil,
580636
}
581637
ddm.SetQueries("metric query ref")
582-
f.store.Set("default/dd-metric-ref", ddm, "utest")
638+
f.store.Set("ns0/dd-metric-ref", ddm, "utest")
583639

584640
// Parse a selector that excludes autoscalers with app.kubernetes.io/managed-by=keda-operator
585641
selector, err := labels.Parse("app.kubernetes.io/managed-by!=keda-operator")
@@ -617,7 +673,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) {
617673
assert.True(t, foundHpa0Ref, "hpa0 should be included (matches label selector)")
618674

619675
// hpa1 has datadogmetric@ reference — dd-metric-ref should be active despite failing label selector
620-
refMetric := f.store.Get("default/dd-metric-ref")
676+
refMetric := f.store.Get("ns0/dd-metric-ref")
621677
assert.NotNil(t, refMetric)
622678
assert.True(t, refMetric.Active)
623679
assert.Equal(t, "hpa:ns0/hpa1", refMetric.AutoscalerReferences)

‎pkg/clusteragent/autoscaling/externalmetrics/provider.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func (p *datadogMetricProvider) getExternalMetric(namespace string, metricSelect
157157
info.Metric = strings.ToLower(info.Metric)
158158

159159
// If the metric name is already prefixed, we can directly look up metrics in store
160-
datadogMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(info.Metric)
160+
datadogMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(info.Metric, namespace)
161161
if !hasPrefix {
162162
datadogMetricID = p.autogenNamespace + kubernetesNamespaceSep + getAutogenDatadogMetricNameFromSelector(info.Metric, metricSelector)
163163
parsed = true

‎pkg/clusteragent/autoscaling/externalmetrics/provider_test.go‎

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ func TestGetExternalMetrics(t *testing.T) {
9696
},
9797
},
9898
queryMetricName: "datadogmetric@ns:metric0",
99+
queryNamespace: "ns",
99100
expectedExternalMetrics: []external_metrics.ExternalMetricValue{
100101
{
101102
MetricName: "datadogmetric@ns:metric0",
@@ -105,6 +106,31 @@ func TestGetExternalMetrics(t *testing.T) {
105106
},
106107
},
107108
},
109+
{
110+
desc: "Test nominal case - DatadogMetric reference without namespace",
111+
storeContent: []ddmWithQuery{
112+
{
113+
ddm: model.DatadogMetricInternal{
114+
ID: "ns/metric0",
115+
DataTime: defaultUpdateTime,
116+
Valid: true,
117+
Error: nil,
118+
Value: 42.0,
119+
},
120+
query: "query-metric0",
121+
},
122+
},
123+
queryMetricName: "datadogmetric@metric0",
124+
queryNamespace: "ns",
125+
expectedExternalMetrics: []external_metrics.ExternalMetricValue{
126+
{
127+
MetricName: "datadogmetric@metric0",
128+
MetricLabels: nil,
129+
Timestamp: defaultMetaUpdateTime,
130+
Value: resource.MustParse(fmt.Sprintf("%v", 42.0)),
131+
},
132+
},
133+
},
108134
{
109135
desc: "Test DatadogMetric is valid but old returns error",
110136
storeContent: []ddmWithQuery{
@@ -120,6 +146,7 @@ func TestGetExternalMetrics(t *testing.T) {
120146
},
121147
},
122148
queryMetricName: "datadogmetric@ns:metric0",
149+
queryNamespace: "ns",
123150
expectedExternalMetrics: nil,
124151
expectedError: fmt.Errorf("DatadogMetric is stale, last updated: %v. Check datadog-cluster-agent logs for errors", defaultUpdateTime.Add(-time.Hour)),
125152
},
@@ -138,6 +165,7 @@ func TestGetExternalMetrics(t *testing.T) {
138165
},
139166
},
140167
queryMetricName: "datadogmetric@ns:metric0",
168+
queryNamespace: "ns",
141169
expectedExternalMetrics: nil,
142170
expectedError: errors.New("Some error"),
143171
},
@@ -155,6 +183,7 @@ func TestGetExternalMetrics(t *testing.T) {
155183
},
156184
},
157185
queryMetricName: "datadogmetric@ns:metric0",
186+
queryNamespace: "ns",
158187
expectedExternalMetrics: nil,
159188
expectedError: errors.New("DatadogMetric is invalid, missing error details"),
160189
},
@@ -173,11 +202,12 @@ func TestGetExternalMetrics(t *testing.T) {
173202
},
174203
},
175204
queryMetricName: "datadogmetric@ns:metric1",
205+
queryNamespace: "ns",
176206
expectedExternalMetrics: nil,
177207
expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric1, datadogmetricid: ns/metric1"),
178208
},
179209
{
180-
desc: "Test DatadogMetric not found",
210+
desc: "Test DatadogMetric not found in request namespace",
181211
storeContent: []ddmWithQuery{
182212
{
183213
ddm: model.DatadogMetricInternal{
@@ -191,9 +221,29 @@ func TestGetExternalMetrics(t *testing.T) {
191221
},
192222
},
193223
queryMetricName: "datadogmetric@ns:metric1",
224+
queryNamespace: "ns",
194225
expectedExternalMetrics: nil,
195226
expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric1, datadogmetricid: ns/metric1"),
196227
},
228+
{
229+
desc: "Test DatadogMetric reference namespace is ignored, DatadogMetric is not readable from another namespace",
230+
storeContent: []ddmWithQuery{
231+
{
232+
ddm: model.DatadogMetricInternal{
233+
ID: "ns/metric0",
234+
DataTime: defaultUpdateTime,
235+
Valid: true,
236+
Error: nil,
237+
Value: 42.0,
238+
},
239+
query: "query-metric0",
240+
},
241+
},
242+
queryMetricName: "datadogmetric@ns:metric0",
243+
queryNamespace: "tenant-b",
244+
expectedExternalMetrics: nil,
245+
expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric0, datadogmetricid: tenant-b/metric0"),
246+
},
197247
{
198248
desc: "Test ExternalMetric use wrong DatadogMetric format",
199249
storeContent: []ddmWithQuery{
@@ -208,9 +258,9 @@ func TestGetExternalMetrics(t *testing.T) {
208258
query: "query-metric0",
209259
},
210260
},
211-
queryMetricName: "datadogmetric@metric1",
261+
queryMetricName: "datadogmetric@metric_1",
212262
expectedExternalMetrics: nil,
213-
expectedError: errors.New("ExternalMetric does not follow DatadogMetric format: datadogmetric@metric1"),
263+
expectedError: errors.New("ExternalMetric does not follow DatadogMetric format: datadogmetric@metric_1"),
214264
},
215265
{
216266
desc: "Test ExternalMetric does not use DatadogMetric format",

‎pkg/clusteragent/autoscaling/externalmetrics/utils.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,18 @@ const (
3131
)
3232

3333
var (
34-
datadogMetricFormat = *regexp.MustCompile("^" + datadogMetricRefPrefix + kubernetesNameFormat + datadogMetricRefSep + kubernetesNameFormat + "$")
34+
// The namespace part of the reference is optional and ignored, DatadogMetric objects are always resolved in the namespace of the referencing object.
35+
datadogMetricFormat = *regexp.MustCompile("^" + datadogMetricRefPrefix + "(?:" + kubernetesNameFormat + datadogMetricRefSep + ")?" + kubernetesNameFormat + "$")
3536
// These values are set by the provider when starting, here are default values for unit tests
3637
queryConfigAggregator = "avg"
3738
queryConfigRollup = 30
3839
)
3940

40-
// datadogMetric.ID is namespace/name
41-
func metricNameToDatadogMetricID(metricName string) (id string, parsed bool, hasPrefix bool) {
41+
// datadogMetric.ID is namespace/name, the namespace being the one of the object referencing the DatadogMetric.
42+
func metricNameToDatadogMetricID(metricName, namespace string) (id string, parsed bool, hasPrefix bool) {
4243
metricName = strings.ToLower(metricName)
4344
if matches := datadogMetricFormat.FindStringSubmatch(metricName); matches != nil {
44-
return matches[1] + kubernetesNamespaceSep + matches[2], true, true
45+
return namespace + kubernetesNamespaceSep + matches[2], true, true
4546
}
4647

4748
return "", false, strings.HasPrefix(metricName, datadogMetricRefPrefix)

0 commit comments

Comments
 (0)