Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -382,14 +382,15 @@ func (w *AutoscalerWatcher) processHPAv2Reference(addAutoscalerReference addAuto
func (w *AutoscalerWatcher) extractAutoscalerReference(
externalMetricName string,
externalMetricSelector *metav1.LabelSelector,
autoscalerNamespace string,
allowAutogen bool,
) (
ddMetricID string,
metricName string,
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
}),
Expand All @@ -180,34 +180,34 @@ 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",
},
}),
}

ddm := model.DatadogMetricInternal{
ID: "default/dd-metric-0",
ID: "ns0/dd-metric-0",
Active: false,
Valid: true,
Value: 10.0,
UpdateTime: updateTime,
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,
UpdateTime: updateTime,
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",
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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",
Expand All @@ -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@<namespace>:<name>` 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()
Expand Down Expand Up @@ -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",
},
},
}),
Expand Down Expand Up @@ -571,15 +627,15 @@ 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,
UpdateTime: time.Now(),
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")
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/clusteragent/autoscaling/externalmetrics/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
56 changes: 53 additions & 3 deletions pkg/clusteragent/autoscaling/externalmetrics/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ func TestGetExternalMetrics(t *testing.T) {
},
},
queryMetricName: "datadogmetric@ns:metric0",
queryNamespace: "ns",
expectedExternalMetrics: []external_metrics.ExternalMetricValue{
{
MetricName: "datadogmetric@ns:metric0",
Expand All @@ -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{
Expand All @@ -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)),
},
Expand All @@ -138,6 +165,7 @@ func TestGetExternalMetrics(t *testing.T) {
},
},
queryMetricName: "datadogmetric@ns:metric0",
queryNamespace: "ns",
expectedExternalMetrics: nil,
expectedError: errors.New("Some error"),
},
Expand All @@ -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"),
},
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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",
Expand Down
9 changes: 5 additions & 4 deletions pkg/clusteragent/autoscaling/externalmetrics/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading
Loading