postgres scaler uses full connection string in metric name

[ycabrer]

The metric name is exposing the full connection string as the metric name. Ideally, we wouldn't have a metric name with the credentials and URL to hit postgres.

Expected Behavior

A metric name that does not expose DB credentials

Actual Behavior

A metric name that exposes DB credentials

Steps to Reproduce the Problem

1. Create a postgres scaler with s.metadata.connection defined
2. Observe full connection string in External Metric Names status

keda/pkg/scalers/postgresql_scaler.go

Line 186 in 934d309

metricName = kedautil.NormalizeString(fmt.Sprintf("%s-%s", metricName, s.metadata.connection))

Specifications

• KEDA Version: V2 Beta
• Platform & Version: EKS
• Kubernetes Version: 1.17.x
• Scaler(s): Postgres

[zroubalik]

@ycabrer we are doing this, because we need a way how to generate unique Metric Name for each Trigger in the ScaledObject. Do you have any idea how to modify that part to not contain full connection string, but the resulting Metric Name is unique (and always the same for the same Trigger?).

[ycabrer]

@zroubalik That makes sense, I don't think it will work though if multiple triggers use the same connection string. Two triggers with separate queries and the same connection will end up with the same name.

What would be truly unique is somehow a combination of the query being used, database host, and name. A hash value? That's not very human friendly. Ideally, the metric name would be easy to identify and correlate with the trigger. Maybe this isn't a problem Keda should be trying to solve programmatically.

The Prometheus scaler allows you to define a metric name. Like Postgres, Prometheus can potentially have many triggers associated with the same host. I think the same approach should be taken here and have it documented that it must be unique.

[zroubalik]

@ycabrer yeah, that's true. We were thinking about this and the benefit is, that user don't have to care about this setting (as it is KEDA internal only) and another point is, that other scalers solve this programatically as well, so we have the same experience for all scalers.

Looking at the code, you are right about the the connection setting and uniqueness, we should fix that first, so it is a combination of both, so it is not broken and than we can try to find a solution for this particular issue (maybe a combination of hashed connection string + query?)

[ycabrer]

@zroubalik

Issue 1 (Password in Metric Name):
We can remove the password from the connection string by parsing it as a URL, grabbing the password, and doing a string replacement. That's how the net/http package seems to handle masking credentials.

Issue 2 (Uniqueness of Metric Name):
I like the idea of basing it off of the connection string (without the password) and the query. My only concern is that it may potentially be a very long metric name. I'm not sure if Kubernetes has a character limit for this but it does affect the readability of the created HPA.

I can't think of a better way without adding a metric name field to Postgres metadata struct like the Prometheus scaler. It is kind of a pain though to make a change like that after v2 Keda scaler has already been defined.

[ycabrer]

Let me know your thoughts. I can't seem to get out of my thought loop. There might be a better way.

[zroubalik]

@ycabrer sorry for the delay. It seems like you are right, for Postres scaler it would make sense to add a metric name field the same way it has been added to Prometheus, as both scalers are based on a long query.

And we cannot assure uniqueness from the other fields, am I right?

To mitigate the problems with changing the scaler API, we could do:

1. add metric name to the spec as an optional field
2. if the metric name is not set, try to generate the name programatically (without exposing the password)
3. add a general check to the main Reconciler loop, to check that metric names in one ScaledObject are unique enough, which should be done anyway )no matter which scaler is being used). This check would inform user via error message/scaledobject status etc that he should modify the metric name.

wdyt?

[ycabrer]

@zroubalik That's a good idea!

I will open a PR for the metric name issue.

I will also look into the reconciler loop check.

[ycabrer]

@zroubalik Is this what you were thinking of for the metric name uniqueness check?

It only checks for uniqueness within the same ScaledObject.

ycabrer@8427b62

[zroubalik]

@ycabrer yeah that's exactly what I was thinking about :)

[zroubalik]

@ycabrer just checking the status. Are you planning to open another PR with the check, please? Or should we track it in some issue as something that's needs to be done. Thanks :)

[ycabrer]

@zroubalik Yes! Let me add a couple of tests for it first and update the changelog/docs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed due to inactivity.