diff --git a/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher.go b/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher.go index bdaec8e23c5a..6c3b813df806 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher.go @@ -300,7 +300,7 @@ func (w *AutoscalerWatcher) getAutoscalerReferences() (map[string]*externalMetri external := metric.External ref := buildAutoscalerReference(autoscalerWPAKindKey, wpa.ObjectMeta) - ddMetricID, metricName, metricLabels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, allowAutogen) + ddMetricID, metricName, metricLabels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, wpa.Namespace, allowAutogen) if ok { addAutoscalerReference(ddMetricID, ref, metricName, metricLabels) } @@ -340,7 +340,7 @@ func (w *AutoscalerWatcher) processHPAv2beta1Reference(addAutoscalerReference ad } external := metric.External - ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, allowAutogen) + ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.MetricName, external.MetricSelector, hpa.Namespace, allowAutogen) if ok { addAutoscalerReference(ddMetricID, ref, metricName, labels) } @@ -356,7 +356,7 @@ func (w *AutoscalerWatcher) processHPAv2beta2Reference(addAutoscalerReference ad } external := metric.External - ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, allowAutogen) + ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, hpa.Namespace, allowAutogen) if ok { addAutoscalerReference(ddMetricID, ref, metricName, labels) } @@ -372,7 +372,7 @@ func (w *AutoscalerWatcher) processHPAv2Reference(addAutoscalerReference addAuto } external := metric.External - ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, allowAutogen) + ddMetricID, metricName, labels, ok := w.extractAutoscalerReference(external.Metric.Name, external.Metric.Selector, hpa.Namespace, allowAutogen) if ok { addAutoscalerReference(ddMetricID, ref, metricName, labels) } @@ -382,6 +382,7 @@ func (w *AutoscalerWatcher) processHPAv2Reference(addAutoscalerReference addAuto func (w *AutoscalerWatcher) extractAutoscalerReference( externalMetricName string, externalMetricSelector *metav1.LabelSelector, + autoscalerNamespace string, allowAutogen bool, ) ( ddMetricID string, @@ -389,7 +390,7 @@ func (w *AutoscalerWatcher) extractAutoscalerReference( labels map[string]string, ok bool, ) { - ddMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(externalMetricName) + ddMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(externalMetricName, autoscalerNamespace) if parsed { // datadogmetric@ references are always tracked regardless of hpaLabelSelector — the selector controls autogen only. return ddMetricID, "", nil, true diff --git a/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher_test.go b/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher_test.go index 8a8a919874e2..7ed165e0a697 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher_test.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/autoscaler_watcher_test.go @@ -165,7 +165,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { { Type: autoscaler.ExternalMetricSourceType, External: &autoscaler.ExternalMetricSource{ - MetricName: "datadogmetric@default:dd-metric-0", + MetricName: "datadogmetric@dd-metric-0", }, }, }), @@ -180,7 +180,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { newFakeWatermarkPodAutoscaler("ns0", "wpa0", []interface{}{ map[string]interface{}{ "external": map[string]interface{}{ - "metricName": "datadogmetric@default:dd-metric-1", + "metricName": "datadogmetric@dd-metric-1", }, "type": "External", }, @@ -188,7 +188,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { } ddm := model.DatadogMetricInternal{ - ID: "default/dd-metric-0", + ID: "ns0/dd-metric-0", Active: false, Valid: true, Value: 10.0, @@ -196,10 +196,10 @@ func TestUpdateAutoscalerReferences(t *testing.T) { Error: nil, } ddm.SetQueries("metric query0") - f.store.Set("default/dd-metric-0", ddm, "utest") + f.store.Set("ns0/dd-metric-0", ddm, "utest") ddm = model.DatadogMetricInternal{ - ID: "default/dd-metric-1", + ID: "ns0/dd-metric-1", Active: true, Valid: true, Value: 11.0, @@ -207,7 +207,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { Error: nil, } ddm.SetQueries("metric query1") - f.store.Set("default/dd-metric-1", ddm, "utest") + f.store.Set("ns0/dd-metric-1", ddm, "utest") ddm = model.DatadogMetricInternal{ ID: "default/dd-metric-2", @@ -226,7 +226,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { // Check internal store content assert.Equal(t, 3, f.store.Count()) ddm = model.DatadogMetricInternal{ - ID: "default/dd-metric-0", + ID: "ns0/dd-metric-0", Active: true, Valid: true, Value: 10.0, @@ -235,10 +235,10 @@ func TestUpdateAutoscalerReferences(t *testing.T) { AutoscalerReferences: "hpa:ns0/hpa0", } ddm.SetQueries("metric query0") - compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-0")) + compareDatadogMetricInternal(t, &ddm, f.store.Get("ns0/dd-metric-0")) ddm = model.DatadogMetricInternal{ - ID: "default/dd-metric-1", + ID: "ns0/dd-metric-1", Active: true, Valid: true, Value: 11.0, @@ -247,7 +247,7 @@ func TestUpdateAutoscalerReferences(t *testing.T) { AutoscalerReferences: "wpa:ns0/wpa0", } ddm.SetQueries("metric query1") - compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-1")) + compareDatadogMetricInternal(t, &ddm, f.store.Get("ns0/dd-metric-1")) ddm = model.DatadogMetricInternal{ ID: "default/dd-metric-2", @@ -262,6 +262,62 @@ func TestUpdateAutoscalerReferences(t *testing.T) { compareDatadogMetricInternal(t, &ddm, f.store.Get("default/dd-metric-2")) } +// The namespace part of a `datadogmetric@:` reference is ignored, the DatadogMetric +// is always resolved in the namespace of the autoscaler, so a reference to another namespace never +// activates the DatadogMetric it points to. +func TestAutoscalerWatcherIgnoresDatadogMetricReferenceNamespace(t *testing.T) { + f := newAutoscalerFixture(t) + updateTime := time.Now() + + f.hpaLister = []*autoscaler.HorizontalPodAutoscaler{ + newFakeHorizontalPodAutoscaler("tenant-a", "hpa0", []autoscaler.MetricSpec{ + { + Type: autoscaler.ExternalMetricSourceType, + External: &autoscaler.ExternalMetricSource{ + MetricName: "datadogmetric@shared:dd-metric-0", + }, + }, + }), + } + + f.wpaLister = []*unstructured.Unstructured{ + newFakeWatermarkPodAutoscaler("tenant-b", "wpa0", []interface{}{ + map[string]interface{}{ + "external": map[string]interface{}{ + "metricName": "datadogmetric@shared:dd-metric-0", + }, + "type": "External", + }, + }), + } + + ddm := model.DatadogMetricInternal{ + ID: "shared/dd-metric-0", + Active: false, + Valid: true, + Value: 10.0, + UpdateTime: updateTime, + Error: nil, + } + ddm.SetQueries("metric query0") + f.store.Set("shared/dd-metric-0", ddm, "utest") + + f.runWatcherUpdate() + + assert.Equal(t, 1, f.store.Count()) + ddm = model.DatadogMetricInternal{ + ID: "shared/dd-metric-0", + Active: false, + Valid: true, + Value: 10.0, + UpdateTime: updateTime, + Error: nil, + AutoscalerReferences: "", + } + ddm.SetQueries("metric query0") + compareDatadogMetricInternal(t, &ddm, f.store.Get("shared/dd-metric-0")) +} + func TestCreateAutogenDatadogMetrics(t *testing.T) { f := newAutoscalerFixture(t) updateTime := time.Now() @@ -523,7 +579,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) { { Type: autoscaler.ExternalMetricSourceType, External: &autoscaler.ExternalMetricSource{ - MetricName: "datadogmetric@default:dd-metric-ref", + MetricName: "datadogmetric@ns0:dd-metric-ref", }, }, }), @@ -571,7 +627,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) { } ddm := model.DatadogMetricInternal{ - ID: "default/dd-metric-ref", + ID: "ns0/dd-metric-ref", Active: false, Valid: true, Value: 20.0, @@ -579,7 +635,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) { Error: nil, } ddm.SetQueries("metric query ref") - f.store.Set("default/dd-metric-ref", ddm, "utest") + f.store.Set("ns0/dd-metric-ref", ddm, "utest") // Parse a selector that excludes autoscalers with app.kubernetes.io/managed-by=keda-operator selector, err := labels.Parse("app.kubernetes.io/managed-by!=keda-operator") @@ -617,7 +673,7 @@ func TestAutoscalerAutogenLabelSelectorFiltering(t *testing.T) { assert.True(t, foundHpa0Ref, "hpa0 should be included (matches label selector)") // hpa1 has datadogmetric@ reference — dd-metric-ref should be active despite failing label selector - refMetric := f.store.Get("default/dd-metric-ref") + refMetric := f.store.Get("ns0/dd-metric-ref") assert.NotNil(t, refMetric) assert.True(t, refMetric.Active) assert.Equal(t, "hpa:ns0/hpa1", refMetric.AutoscalerReferences) diff --git a/pkg/clusteragent/autoscaling/externalmetrics/provider.go b/pkg/clusteragent/autoscaling/externalmetrics/provider.go index e2d77e527f52..8797c99ee394 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/provider.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/provider.go @@ -157,7 +157,7 @@ func (p *datadogMetricProvider) getExternalMetric(namespace string, metricSelect info.Metric = strings.ToLower(info.Metric) // If the metric name is already prefixed, we can directly look up metrics in store - datadogMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(info.Metric) + datadogMetricID, parsed, hasPrefix := metricNameToDatadogMetricID(info.Metric, namespace) if !hasPrefix { datadogMetricID = p.autogenNamespace + kubernetesNamespaceSep + getAutogenDatadogMetricNameFromSelector(info.Metric, metricSelector) parsed = true diff --git a/pkg/clusteragent/autoscaling/externalmetrics/provider_test.go b/pkg/clusteragent/autoscaling/externalmetrics/provider_test.go index bd67462563b5..35af94d2588e 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/provider_test.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/provider_test.go @@ -96,6 +96,7 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric0", + queryNamespace: "ns", expectedExternalMetrics: []external_metrics.ExternalMetricValue{ { MetricName: "datadogmetric@ns:metric0", @@ -105,6 +106,31 @@ func TestGetExternalMetrics(t *testing.T) { }, }, }, + { + desc: "Test nominal case - DatadogMetric reference without namespace", + storeContent: []ddmWithQuery{ + { + ddm: model.DatadogMetricInternal{ + ID: "ns/metric0", + DataTime: defaultUpdateTime, + Valid: true, + Error: nil, + Value: 42.0, + }, + query: "query-metric0", + }, + }, + queryMetricName: "datadogmetric@metric0", + queryNamespace: "ns", + expectedExternalMetrics: []external_metrics.ExternalMetricValue{ + { + MetricName: "datadogmetric@metric0", + MetricLabels: nil, + Timestamp: defaultMetaUpdateTime, + Value: resource.MustParse(fmt.Sprintf("%v", 42.0)), + }, + }, + }, { desc: "Test DatadogMetric is valid but old returns error", storeContent: []ddmWithQuery{ @@ -120,6 +146,7 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric0", + queryNamespace: "ns", expectedExternalMetrics: nil, expectedError: fmt.Errorf("DatadogMetric is stale, last updated: %v. Check datadog-cluster-agent logs for errors", defaultUpdateTime.Add(-time.Hour)), }, @@ -138,6 +165,7 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric0", + queryNamespace: "ns", expectedExternalMetrics: nil, expectedError: errors.New("Some error"), }, @@ -155,6 +183,7 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric0", + queryNamespace: "ns", expectedExternalMetrics: nil, expectedError: errors.New("DatadogMetric is invalid, missing error details"), }, @@ -173,11 +202,12 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric1", + queryNamespace: "ns", expectedExternalMetrics: nil, expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric1, datadogmetricid: ns/metric1"), }, { - desc: "Test DatadogMetric not found", + desc: "Test DatadogMetric not found in request namespace", storeContent: []ddmWithQuery{ { ddm: model.DatadogMetricInternal{ @@ -191,9 +221,29 @@ func TestGetExternalMetrics(t *testing.T) { }, }, queryMetricName: "datadogmetric@ns:metric1", + queryNamespace: "ns", expectedExternalMetrics: nil, expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric1, datadogmetricid: ns/metric1"), }, + { + desc: "Test DatadogMetric reference namespace is ignored, DatadogMetric is not readable from another namespace", + storeContent: []ddmWithQuery{ + { + ddm: model.DatadogMetricInternal{ + ID: "ns/metric0", + DataTime: defaultUpdateTime, + Valid: true, + Error: nil, + Value: 42.0, + }, + query: "query-metric0", + }, + }, + queryMetricName: "datadogmetric@ns:metric0", + queryNamespace: "tenant-b", + expectedExternalMetrics: nil, + expectedError: errors.New("DatadogMetric not found for metric name: datadogmetric@ns:metric0, datadogmetricid: tenant-b/metric0"), + }, { desc: "Test ExternalMetric use wrong DatadogMetric format", storeContent: []ddmWithQuery{ @@ -208,9 +258,9 @@ func TestGetExternalMetrics(t *testing.T) { query: "query-metric0", }, }, - queryMetricName: "datadogmetric@metric1", + queryMetricName: "datadogmetric@metric_1", expectedExternalMetrics: nil, - expectedError: errors.New("ExternalMetric does not follow DatadogMetric format: datadogmetric@metric1"), + expectedError: errors.New("ExternalMetric does not follow DatadogMetric format: datadogmetric@metric_1"), }, { desc: "Test ExternalMetric does not use DatadogMetric format", diff --git a/pkg/clusteragent/autoscaling/externalmetrics/utils.go b/pkg/clusteragent/autoscaling/externalmetrics/utils.go index f3c3319660f4..4ee852d33970 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/utils.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/utils.go @@ -31,17 +31,18 @@ const ( ) var ( - datadogMetricFormat = *regexp.MustCompile("^" + datadogMetricRefPrefix + kubernetesNameFormat + datadogMetricRefSep + kubernetesNameFormat + "$") + // The namespace part of the reference is optional and ignored, DatadogMetric objects are always resolved in the namespace of the referencing object. + datadogMetricFormat = *regexp.MustCompile("^" + datadogMetricRefPrefix + "(?:" + kubernetesNameFormat + datadogMetricRefSep + ")?" + kubernetesNameFormat + "$") // These values are set by the provider when starting, here are default values for unit tests queryConfigAggregator = "avg" queryConfigRollup = 30 ) -// datadogMetric.ID is namespace/name -func metricNameToDatadogMetricID(metricName string) (id string, parsed bool, hasPrefix bool) { +// datadogMetric.ID is namespace/name, the namespace being the one of the object referencing the DatadogMetric. +func metricNameToDatadogMetricID(metricName, namespace string) (id string, parsed bool, hasPrefix bool) { metricName = strings.ToLower(metricName) if matches := datadogMetricFormat.FindStringSubmatch(metricName); matches != nil { - return matches[1] + kubernetesNamespaceSep + matches[2], true, true + return namespace + kubernetesNamespaceSep + matches[2], true, true } return "", false, strings.HasPrefix(metricName, datadogMetricRefPrefix) diff --git a/pkg/clusteragent/autoscaling/externalmetrics/utils_test.go b/pkg/clusteragent/autoscaling/externalmetrics/utils_test.go index b68739b28e85..d0c1699927bc 100644 --- a/pkg/clusteragent/autoscaling/externalmetrics/utils_test.go +++ b/pkg/clusteragent/autoscaling/externalmetrics/utils_test.go @@ -18,24 +18,50 @@ import ( func TestMetricNameToDatadogMetricID(t *testing.T) { tests := []struct { metricName string + namespace string expID string expParsed bool expHasPrefix bool }{ { metricName: "datadogmetric@myns:name", + namespace: "myns", + expID: "myns/name", + expParsed: true, + expHasPrefix: true, + }, + { + // The namespace part of the reference is ignored, the referencing object namespace is used. + metricName: "datadogmetric@otherns:name", + namespace: "myns", expID: "myns/name", expParsed: true, expHasPrefix: true, }, { metricName: "datadogmetric@name", + namespace: "myns", + expID: "myns/name", + expParsed: true, + expHasPrefix: true, + }, + { + metricName: "datadogmetric@:name", + namespace: "myns", + expID: "", + expParsed: false, + expHasPrefix: true, + }, + { + metricName: "datadogmetric@bad_name", + namespace: "myns", expID: "", expParsed: false, expHasPrefix: true, }, { metricName: "nginx.responsetime", + namespace: "myns", expID: "", expParsed: false, expHasPrefix: false, @@ -44,7 +70,7 @@ func TestMetricNameToDatadogMetricID(t *testing.T) { for _, test := range tests { t.Run(test.metricName, func(t *testing.T) { - id, parsed, hasPrefix := metricNameToDatadogMetricID(test.metricName) + id, parsed, hasPrefix := metricNameToDatadogMetricID(test.metricName, test.namespace) assert.Equal(t, test.expID, id) assert.Equal(t, test.expParsed, parsed) assert.Equal(t, test.expHasPrefix, hasPrefix) diff --git a/releasenotes-dca/notes/datadogmetric-reference-namespace-0f65c15fb4ed2d92.yaml b/releasenotes-dca/notes/datadogmetric-reference-namespace-0f65c15fb4ed2d92.yaml new file mode 100644 index 000000000000..5103f4a325be --- /dev/null +++ b/releasenotes-dca/notes/datadogmetric-reference-namespace-0f65c15fb4ed2d92.yaml @@ -0,0 +1,30 @@ +--- +upgrade: + - | + The ```` part of a ``datadogmetric@:`` external metric + reference is now ignored. The referenced ``DatadogMetric`` is always looked up in the + namespace of the ``HorizontalPodAutoscaler`` or ``WatermarkPodAutoscaler`` that holds + the reference, so referencing a ``DatadogMetric`` owned by another namespace is no + longer supported. Such a reference now resolves to a ``DatadogMetric`` that does not + exist, which leaves the autoscaler without a metric value and unable to scale. + + To find out whether you are affected, list every external metric reference that carries + an explicit namespace: + + .. code-block:: shell + + kubectl get hpa --all-namespaces -o yaml | grep -E 'datadogmetric@[a-z0-9-]+:' + kubectl get wpa --all-namespaces -o yaml | grep -E 'datadogmetric@[a-z0-9-]+:' + + References whose namespace is the namespace of the autoscaler holding them keep working + unchanged. For every reference pointing at another namespace, create a ``DatadogMetric`` + with the same query in the autoscaler's own namespace and point the autoscaler at it. + The namespace can now be left out entirely, ``datadogmetric@`` is a valid + reference that resolves in the autoscaler's namespace. +security: + - | + The Cluster Agent external metrics provider now resolves ``datadogmetric@`` references + in the namespace of the requesting object rather than the namespace embedded in the + metric name. Previously, a workload could read the value of a ``DatadogMetric`` owned by + another namespace, and keep that ``DatadogMetric`` active so that its Datadog queries + kept running.