ScaledObject with multiple triggers with non unique name should be rejected at creation

[nonoswz]

Report

Hi,

We just upgraded from Keda 2.5 to 2.10.0. We have ScaledObject with multiple triggers with non-unique name. This change was introduced to ensure names are unique amongst all triggers in a ScaledObject.

We were not aware about this change until we saw the following error logs on our end:

2023-06-07T20:40:11Z	ERROR	Reconciler error	{"controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": {"name":"<service-name>","namespace":"<service-name>"}, "namespace": "<service-name>", "name": "<service-name>", "reconcileID": "76264394-339a-4440-9ced-bd3faa280a1b", "error": "triggerName=cron is defined multiple times in the ScaledObject, but it must be unique"}

After investigation we found out few of our ScaledObject indeed have multiple times the same name, such as the following one:

spec:
  maxReplicaCount: 220
  minReplicaCount: 100
  scaleTargetRef:
    apiVersion: apps/v1
    envSourceContainerName: <service-name>
    kind: Deployment
    name: <service-name>
  triggers:
  - metadata:
      type: Utilization
      value: "80"
    name: memory
    type: memory
  - metadata:
      type: Utilization
      value: "65"
    name: cpu
    type: cpu
  - metadata:
      desiredReplicas: "170"
      end: 0 23 * * MON-FRI
      start: 0 8 * * MON-FRI
      timezone: America/New_York
    name: cron
    type: cron
  - metadata:
      desiredReplicas: "135"
      end: 0 23 * * SAT
      start: 0 9 * * SAT
      timezone: America/New_York
    name: cron
    type: cron
  - metadata:
      desiredReplicas: "135"
      end: 0 23 * * SUN
      start: 0 9 * * SUN
      timezone: America/New_York
    name: cron
    type: cron

We understand the change, so we removed the name from the triggers as it's an optional field.

We do think, there is a bug as we are still able to create a ScaledObject with non unique trigger names with 2.10.0. This is actually pretty confusing as the object would be created properly, but the underlying HPA would never be created or reconciled. So it is misleading. Could you please ensure a ScaledObject with multiple triggers with non unique name is rejected at creation? Thanks.

Expected Behavior

A ScaledObject with non unique trigger name should be rejected at creation.

Actual Behavior

A ScaledObject with non unique trigger name can be created, then the underlying HPA is never created or reconciled.

Steps to Reproduce the Problem

1. Create a scaledObject with mutlitple triggers with the same name, such as the object in the description above.
2. Get the HPA, and see that it is never created
3. Check Keda logs and you will see the HPA cannot be reconciled.

Logs from KEDA operator

I replaced our service-name in the logs.

2023-06-07T20:40:11Z	ERROR	Reconciler error	{"controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": {"name":"<service-name>","namespace":"<service-name>"}, "namespace": "<service-name>", "name": "<service-name>", "reconcileID": "76264394-339a-4440-9ced-bd3faa280a1b", "error": "triggerName=cron is defined multiple times in the ScaledObject, but it must be unique"}
2023-06-07T20:40:11Z	ERROR	ScaledObject doesn't have correct triggers specification	{"controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": {"name":"<service-name>","namespace":"<service-name>"}, "namespace": "<service-name>", "name": "<service-name>", "reconcileID": "76264394-339a-4440-9ced-bd3faa280a1b", "error": "triggerName=cron is defined multiple times in the ScaledObject, but it must be unique"}

KEDA Version

2.10.0

Kubernetes Version

1.24

Platform

Amazon Web Services

Scaler Details

cpu/memory/cron

Anything else?

No response

[JorTurFer]

Hi,
Could you share KEDA operator logs? That name isn't used for anything internally, it's just a field to provide a name for observability proposals, but internally (in the HPA), that name isn't used, we generated a unique name:

keda/pkg/scalers/cron_scaler.go

Lines 130 to 140 in d993d61

	func (s *cronScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec {
	var specReplicas int64 = 1
	externalMetric := &v2.ExternalMetricSource{
	Metric: v2.MetricIdentifier{
	Name: GenerateMetricNameWithIndex(s.metadata.scalerIndex, kedautil.NormalizeString(fmt.Sprintf("cron-%s-%s-%s", s.metadata.timezone, parseCronTimeFormat(s.metadata.start), parseCronTimeFormat(s.metadata.end)))),
	},
	Target: GetMetricTarget(s.metricType, specReplicas),
	}
	metricSpec := v2.MetricSpec{External: externalMetric, Type: cronMetricType}
	return []v2.MetricSpec{metricSpec}
	}

[zroubalik]

@JorTurFer this is not a metric name, but trigger name (optional) used for prometheus metrics.

The issue is correct, we should add a validation check.

[JorTurFer]

oh f**k, got it.
We can add the check to the admission webhook, I think the best place

[zroubalik]

@JorTurFer I am already fixing this, so will assign to me.

[JorTurFer]

Lol, I have it almost done too xD
I was moving the func checkTriggers from ScaledObjectReconciler to the scaledobject itself to reuse it in the webhook and in the operator

[zroubalik]

Lol, I have it almost done too xD I was moving the func checkTriggers from ScaledObjectReconciler to the scaledobject itself to reuse it in the webhook and in the operator

great minds think alike 😄

[nonoswz]

Thanks for the fix!