Keda: Highlight the difference between literal and resolved values in metadata

[turbaszek]

Original question from slack:

Hi all! I’ve a question regarding metadata values that are picked from resolvedEnv . Is there any information how this mechanism works and which params for each scaler are resolved from environment?
For example Redis scaler have a metadata password . But there’s no information that if I want to pass a password I should pass env variable instead of the exact password (what would be unsecure). I am just wondering if it would make sense to have some naming convention for such params ex. envPassword ?

Other option (and more reliable than a convention) is to have env section of metadata as suggested by @jeffhollan .

[tomkerkhove]

Adding an env section would leak the internal implementation which is something I'd try to avoid.

This feels more like a documentation issue rather than implementation, no? 🤔

[turbaszek]

This feels more like a documentation issue rather than implementation, no? 🤔

Improving documentation will help for sure. But I think making this distinction more explicit would help, especially those who are new to KEDA / k8s.

[tomkerkhove]

What would you suggest then?

[tomkerkhove]

In terms of your Redis scenario, that's why we've introduced TriggerAuthentication

[turbaszek]

What would you suggest then?

As mentioned in first comment I was thinking about something like envPassword to indicate the difference with a prefix.

I suppose it's only a problem when you start to play with KEDA. When you get to the TriggerAuthentication and check some code you will fully understand what's going on. So if it's only me then we can close the issue.

[tomkerkhove]

Personally that's not something I would do, we don't do that for other scalers as well and it should be well documented.

Adding the prefix couples it to our internal workings. The real issue here is that password should be via parameters giving you the opportunity to use an environment variable or secret.

Frankly, it's clearly documented:

Provide the password field if the redis server requires a password. Both the hostname and password fields need to be set to the names of the environment variables in the target deployment that contain the host name and password respectively.

And we do support TriggerAuthentication (sample) so for me everything seems to be working as expected.

Thoughts @jeffhollan?

[jeffhollan]

Yeah I think it's not quite leaking implementation. We don't read keys from config maps and secrets as an architecture choice, but specifically require some values to be a reference to a secret on config name rather than the actual value. In the spirit of making KEDA feel self-explanatory, I do think there is merit that if just looking at a scaled object spec you would have no idea if metadata values were something like:

metadata:
  hostLocation:
  hostConfig:
  someOtherValue:

Which one of those needed to be literal vs. references. TriggerAuthentication samples in docs definitely help here as makes very explicit. I do think the most actionable thing short term is make sure the docs are super clear in the examples and page themselves which metadata values need to be references (whether a TriggerAuthentication version or not).

The option I'd be open to so that you don't have to necessarily have docs open would be something like:

metadata:
  values:
    port: 
    hostLocation:
  references:
    connectionString:
    password:
    < -- OR -- >
    authenticationRef: 

So more or less the metdata is the same, but more clear which one are references or not. That said I'm not saying we should for sure do this. Would be a breaking change so have to be in a major version release, but I'm not opposed to it. I think we could keep this issue open and get some thoughts on if folks think making the naming or schema more obvious in the different ways these values resolve is worthwhile

[turbaszek]

Related to #590 ?