Refactor scalers parsing to unify them

[JorTurFer]

Proposal

Currently, each scaler parses the configuration using its own way, some of them use helpers for reading envs, other don't, some use one helper, others use their own helpers...
There is a lot of fragmentation in scalers code and we should unify it

Use-Case

No response

Is this a feature you are interested in implementing yourself?

Maybe

Anything else?

No response

[dttung2905]

I take a look at some use cases inside parse${SCALER_NAME}Metadata method and its true that the way we parse the config varies from scaler to scaler. I also noticed we have a helper method getParameterFromConfig in azure_log_analytics_scaler.go that does exactly this thing i.e. get the string value from either AuthParams or TriggerMetadata and throw out error if we can't find anything.

keda/pkg/scalers/azure_log_analytics_scaler.go

Lines 246 to 255 in faf8c9a

	func getParameterFromConfig(config *ScalerConfig, parameter string, checkAuthParams bool) (string, error) {
	if val, ok := config.AuthParams[parameter]; checkAuthParams && ok && val != "" {
	return val, nil
	} else if val, ok := config.TriggerMetadata[parameter]; ok && val != "" {
	return val, nil
	} else if val, ok := config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]; ok && val != "" {
	return config.ResolvedEnv[config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]], nil
	}
	return "", fmt.Errorf("error parsing metadata. Details: %s was not found in metadata. Check your ScaledObject configuration", parameter)
	}

My proposal is to move this helper function to something like pkg/scalers/helper.go ( not sure about the exact path though but it should be near to scalers. After we get the string value, whatever transformation can be done after that. What do you think about this ?

[zroubalik]

There are also additional things that needs to be considered.

• It should be able to distinguish between optional and mandatory fields
• Some settings have dependencies - eg. if setting1 = foo then setting2 must be set etc. I don't have the specific example here, but I this is needed for some settings a setting around tls/auth

[dttung2905]

Some settings have dependencies - eg. if setting1 = foo then setting2 must be set etc. I don't have the specific example here, but I this is needed for some settings a setting around tls/auth

So for this case we are just going to throw out an error right?
Assuming its throwing out an error, I'm thinking of modifying the function getParameterFromConfig to add in an extra arg isOptional of type bool. For case where dependencies are not met, this field should be set as isOptional=false

// getParameterFromConfig gets the parameter from the configs, if checkAuthParams is true
// then AuthParams is also check for the parameter
 func getParameterFromConfig(config *ScalerConfig, parameter string, checkAuthParams bool, isOptional bool) (string, error) {
	if val, ok := config.AuthParams[parameter]; checkAuthParams && ok && val != "" {
		return val, nil
	} else if val, ok := config.TriggerMetadata[parameter]; ok && val != "" {
		return val, nil
	} else if val, ok := config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]; ok && val != "" {
		return config.ResolvedEnv[config.TriggerMetadata[fmt.Sprintf("%sFromEnv", parameter)]], nil
	}
        // if its an optional field, we can throw out something here, just put empty string as an arbitrary value for now
	if isOptional {
		return "", nil
	}
	return "", fmt.Errorf("error parsing metadata. Details: %s was not found in metadata. Check your ScaledObject configuration", parameter)

wdyt?

[JorTurFer]

I think that this can be useful and I agree with the approach of having a single function like getParameterFromConfig with some config parameters, but I'd be more explicit, something like:

• useMetadata
• useAuthentication
• useResolvedEnv
• isOptional
• default

There are some parameters that we must read only from secure sources 😄
If possible, I'd use generics here to return directly the expected value with the correct type, instead of having to parse outside, so maybe we have to add another parameter for the type