Fix metric name passed to external scaler in GetMetrics call. (#2890)

CHANGELOG.md

@@ -82,6 +82,8 @@
 - **AWS DynamoDB:** Setup AWS DynamoDB test account ([#2803](https://github.com/kedacore/keda/issues/2803))
 - **AWS Kinesis Stream:** Adding e2e test ([#1526](https://github.com/kedacore/keda/issues/1526))
 - **AWS SQS Queue:** Adding e2e test ([#1527](https://github.com/kedacore/keda/issues/1527))
+- **External Scaler:** Adding e2e test. ([#2697](https://github.com/kedacore/keda/issues/2697))
+- **External Scaler:** Fix issue with internal KEDA core prefix being passed to external scaler. ([#2640](https://github.com/kedacore/keda/issues/2640))
 - **GCP Pubsub Scaler:** Adding e2e test ([#1528](https://github.com/kedacore/keda/issues/1528))
 - **Hashicorp Vault Secret Provider:** Adding e2e test ([#2842](https://github.com/kedacore/keda/issues/2842))
 - **Memory Scaler:** Adding e2e test ([#2220](https://github.com/kedacore/keda/issues/2220))

pkg/scalers/external_scaler.go

@@ -197,6 +197,12 @@ func (s *externalScaler) GetMetrics(ctx context.Context, metricName string, metr
 	}
 	defer done()
 
+	// Remove the sX- prefix as the external scaler shouldn't have to know about it
+	metricName, err = RemoveIndexFromMetricName(s.metadata.scalerIndex, metricName)
+	if err != nil {
+		return metrics, err
+	}
+
 	request := &pb.GetMetricsRequest{
 		MetricName:      metricName,
 		ScaledObjectRef: &s.scaledObjectRef,

pkg/scalers/scaler.go

@@ -19,6 +19,7 @@ package scalers
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"k8s.io/api/autoscaling/v2beta2"
@@ -110,6 +111,21 @@ func GenerateMetricNameWithIndex(scalerIndex int, metricName string) string {
 	return fmt.Sprintf("s%d-%s", scalerIndex, metricName)
 }
 
+// RemoveIndexFromMetricName removes the index prefix from the metric name
+func RemoveIndexFromMetricName(scalerIndex int, metricName string) (string, error) {
+	metricNameSplit := strings.SplitN(metricName, "-", 2)
+	if len(metricNameSplit) != 2 {
+		return "", fmt.Errorf("metric name without index prefix")
+	}
+
+	indexPrefix, metricNameWithoutIndex := metricNameSplit[0], metricNameSplit[1]
+	if indexPrefix != fmt.Sprintf("s%d", scalerIndex) {
+		return "", fmt.Errorf("metric name contains incorrect index prefix")
+	}
+
+	return metricNameWithoutIndex, nil
+}
+
 // GetMetricTargetType helps getting the metric target type of the scaler
 func GetMetricTargetType(config *ScalerConfig) (v2beta2.MetricTargetType, error) {
 	switch config.MetricType {

pkg/scalers/scaler_test.go

@@ -85,3 +85,40 @@ func TestGetMetricTarget(t *testing.T) {
 		})
 	}
 }
+
+func TestRemoveIndexFromMetricName(t *testing.T) {
+	cases := []struct {
+		scalerIndex                          int
+		metricName                           string
+		expectedMetricNameWithoutIndexPrefix string
+		isError                              bool
+	}{
+		// Proper input
+		{scalerIndex: 0, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "metricName", isError: false},
+		// Proper input with scalerIndex > 9
+		{scalerIndex: 123, metricName: "s123-metricName", expectedMetricNameWithoutIndexPrefix: "metricName", isError: false},
+		// Incorrect index prefix
+		{scalerIndex: 1, metricName: "s0-metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true},
+		// Incorrect index prefix
+		{scalerIndex: 0, metricName: "0-metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true},
+		// No index prefix
+		{scalerIndex: 0, metricName: "metricName", expectedMetricNameWithoutIndexPrefix: "", isError: true},
+	}
+
+	for _, testCase := range cases {
+		metricName, err := RemoveIndexFromMetricName(testCase.scalerIndex, testCase.metricName)
+		if err != nil && !testCase.isError {
+			t.Error("Expected success but got error", err)
+		}
+
+		if testCase.isError && err == nil {
+			t.Error("Expected error but got success")
+		}
+
+		if err == nil {
+			if metricName != testCase.expectedMetricNameWithoutIndexPrefix {
+				t.Errorf("Expected - %s, Got - %s", testCase.expectedMetricNameWithoutIndexPrefix, metricName)
+			}
+		}
+	}
+}

tests/scalers/external-scaler.test.ts

@@ -0,0 +1,192 @@
+import * as sh from "shelljs"
+import test from "ava"
+import { createNamespace, createYamlFile, waitForDeploymentReplicaCount } from "./helpers"
+
+const testName = "test-external-scaler"
+const testNamespace = `${testName}-ns`
+const scalerName = `${testName}-scaler`
+const serviceName = `${testName}-service`
+const deploymentName = `${testName}-deployment`
+const scaledObjectName = `${testName}-scaled-object`
+
+const idleReplicaCount = 0
+const minReplicaCount = 1
+const maxReplicaCount = 2
+const threshold = 10
+
+test.before(async t => {
+    sh.config.silent = true
+
+    // Create Kubernetes Namespace
+    createNamespace(testNamespace)
+
+    // Create external scaler deployment
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(scalerYaml)} -n ${testNamespace}`).code,
+        0,
+        "Createing a external scaler deployment should work"
+    )
+
+    // Create service
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(serviceYaml)} -n ${testNamespace}`).code,
+        0,
+        "Createing a service should work"
+    )
+
+    // Create deployment
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(deploymentYaml)} -n ${testNamespace}`).code,
+        0,
+        "Createing a deployment should work"
+    )
+
+    // Create scaled object
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", "0"))} -n ${testNamespace}`).code,
+        0,
+        "Creating a scaled object should work"
+    )
+
+    t.true(await waitForDeploymentReplicaCount(idleReplicaCount, deploymentName, testNamespace, 60, 1000),
+        `Replica count should be ${idleReplicaCount} after 1 minute`)
+})
+
+test.serial("Deployment should scale up to minReplicaCount", async t => {
+    // Modify scaled object's metricValue to induce scaling
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", `${threshold}`))} -n ${testNamespace}`).code,
+        0,
+        "Modifying scaled object should work"
+    )
+
+    t.true(await waitForDeploymentReplicaCount(minReplicaCount, deploymentName, testNamespace, 60, 1000),
+    `Replica count should be ${minReplicaCount} after 1 minute`)
+})
+
+test.serial("Deployment should scale up to maxReplicaCount", async t => {
+    // Modify scaled object's metricValue to induce scaling
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", `${threshold * 2}`))} -n ${testNamespace}`).code,
+        0,
+        "Modifying scaled object should work"
+    )
+
+    t.true(await waitForDeploymentReplicaCount(maxReplicaCount, deploymentName, testNamespace, 60, 1000),
+    `Replica count should be ${maxReplicaCount} after 1 minute`)
+})
+
+test.serial("Deployment should scale back down to idleReplicaCount", async t => {
+    // Modify scaled object's metricValue to induce scaling
+    t.is(
+        sh.exec(`kubectl apply -f ${createYamlFile(scaledObjectYaml.replace("{{VALUE}}", "0"))} -n ${testNamespace}`).code,
+        0,
+        "Modifying scaled object should work"
+    )
+
+    t.true(await waitForDeploymentReplicaCount(idleReplicaCount, deploymentName, testNamespace, 60, 1000),
+    `Replica count should be ${idleReplicaCount} after 1 minute`)
+})
+
+test.after.always("Clean up E2E K8s objects", async t => {
+    const resources = [
+        `scaledobject.keda.sh/${scaledObjectName}`,
+        `deployments.apps/${deploymentName}`,
+        `service/${serviceName}`,
+        `deployments.apps/${scalerName}`,
+    ]
+
+    for (const resource of resources) {
+        sh.exec(`kubectl delete ${resource} -n ${testNamespace}`)
+    }
+
+    sh.exec(`kubectl delete ns ${testNamespace}`)
+})
+
+// YAML Definitions for Kubernetes resources
+// External Scaler Deployment
+const scalerYaml =
+`
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: ${scalerName}
+  namespace: ${testNamespace}
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: ${scalerName}
+  template:
+    metadata:
+      labels:
+        app: ${scalerName}
+    spec:
+      containers:
+      - name: scaler
+        image: ghcr.io/kedacore/tests-external-scaler-e2e:latest
+        imagePullPolicy: Always
+        ports:
+        - containerPort: 6000
+`
+
+const serviceYaml =
+`
+apiVersion: v1
+kind: Service
+metadata:
+  name: ${serviceName}
+  namespace: ${testNamespace}
+spec:
+  ports:
+  - port: 6000
+    targetPort: 6000
+  selector:
+    app: ${scalerName}
+`
+
+// Deployment
+const deploymentYaml =
+`apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: ${deploymentName}
+  namespace: ${testNamespace}
+spec:
+  replicas: 0
+  selector:
+    matchLabels:
+      app: ${deploymentName}
+  template:
+    metadata:
+      labels:
+        app: ${deploymentName}
+    spec:
+      containers:
+      - name: nginx
+        image: nginx:1.16.1
+`
+
+// Scaled Object
+const scaledObjectYaml =
+`
+apiVersion: keda.sh/v1alpha1
+kind: ScaledObject
+metadata:
+  name: ${scaledObjectName}
+  namespace: ${testNamespace}
+spec:
+  scaleTargetRef:
+    name: ${deploymentName}
+  pollingInterval: 5
+  cooldownPeriod: 10
+  idleReplicaCount: ${idleReplicaCount}
+  minReplicaCount: ${minReplicaCount}
+  maxReplicaCount: ${maxReplicaCount}
+  triggers:
+  - type: external
+    metadata:
+      scalerAddress: ${serviceName}.${testNamespace}:6000
+      metricThreshold: "${threshold}"
+      metricValue: "{{VALUE}}"
+`

tests/scalers/helpers.ts

@@ -31,6 +31,12 @@ export async function createNamespace(namespace: string) {
     sh.exec(`kubectl apply -f ${namespaceFile.name}`)
 }
 
+export function createYamlFile(yaml: string) {
+    const tmpFile = tmp.fileSync()
+    fs.writeFileSync(tmpFile.name, yaml)
+    return tmpFile.name
+}
+
 const namespaceTemplate = `apiVersion: v1
 kind: Namespace
 metadata: