Refactor and fix metrics-api scaler#6940
Conversation
Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com>
9484e89 to
1dc6321
Compare
Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com>
|
/run-e2e metrics_api |
pkg/scalers/metrics_api_scaler.go
Outdated
| AuthMode string `keda:"name=authMode,order=triggerMetadata;authParams,optional"` | ||
|
|
||
| // apiKeyAuth | ||
| enableAPIKeyAuth bool | ||
| method string // way of providing auth key, either "header" (default) or "query" | ||
| Method string `keda:"name=method,order=triggerMetadata;authParams,default=header,enum=header;query"` | ||
| // keyParamName is either header key or query param used for passing apikey | ||
| // default header is "X-API-KEY", default query param is "api_key" | ||
| keyParamName string | ||
| apiKey string | ||
| KeyParamName string `keda:"name=keyParamName,order=triggerMetadata;authParams,optional"` | ||
| APIKey string `keda:"name=apiKey,order=authParams,optional"` | ||
|
|
||
| // base auth | ||
| enableBaseAuth bool | ||
| username string | ||
| password string // +optional | ||
| Username string `keda:"name=username,order=authParams,optional"` | ||
| Password string `keda:"name=password,order=authParams,optional"` // +optional | ||
|
|
||
| // client certification | ||
| enableTLS bool | ||
| cert string | ||
| key string | ||
| ca string | ||
| Cert string `keda:"name=cert,order=authParams,optional"` | ||
| Key string `keda:"name=key,order=authParams,optional"` | ||
| CA string `keda:"name=ca,order=authParams,optional"` | ||
|
|
||
| // bearer | ||
| enableBearerAuth bool | ||
| bearerToken string | ||
| BearerToken string `keda:"name=token,order=authParams,optional"` |
There was a problem hiding this comment.
curious if this can be further simplified by using authentication.Config like in #6969? So far each scaler had its own quirky behavior and expectations regarding auth params so if it's too problematic, we can merge as is and work on further improvements later
There was a problem hiding this comment.
Is it too much work to change this as part of this PR? probably it's a task that we will forget xD
I approve it just in case, but I'd prefer to change it as part of this PR
zroubalik
left a comment
There was a problem hiding this comment.
What is the status here please?
Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com>
pkg/scalers/metrics_api_scaler.go
Outdated
| AuthMode string `keda:"name=authMode,order=triggerMetadata;authParams,optional"` | ||
|
|
||
| // apiKeyAuth | ||
| enableAPIKeyAuth bool | ||
| method string // way of providing auth key, either "header" (default) or "query" | ||
| Method string `keda:"name=method,order=triggerMetadata;authParams,default=header,enum=header;query"` | ||
| // keyParamName is either header key or query param used for passing apikey | ||
| // default header is "X-API-KEY", default query param is "api_key" | ||
| keyParamName string | ||
| apiKey string | ||
| KeyParamName string `keda:"name=keyParamName,order=triggerMetadata;authParams,optional"` | ||
| APIKey string `keda:"name=apiKey,order=authParams,optional"` | ||
|
|
||
| // base auth | ||
| enableBaseAuth bool | ||
| username string | ||
| password string // +optional | ||
| Username string `keda:"name=username,order=authParams,optional"` | ||
| Password string `keda:"name=password,order=authParams,optional"` // +optional | ||
|
|
||
| // client certification | ||
| enableTLS bool | ||
| cert string | ||
| key string | ||
| ca string | ||
| Cert string `keda:"name=cert,order=authParams,optional"` | ||
| Key string `keda:"name=key,order=authParams,optional"` | ||
| CA string `keda:"name=ca,order=authParams,optional"` | ||
|
|
||
| // bearer | ||
| enableBearerAuth bool | ||
| bearerToken string | ||
| BearerToken string `keda:"name=token,order=authParams,optional"` |
There was a problem hiding this comment.
Is it too much work to change this as part of this PR? probably it's a task that we will forget xD
I approve it just in case, but I'd prefer to change it as part of this PR
|
I refactored the auth logic, but I'm considering how to make it more generic. I found that a new type of authentication needs to be added, but its parameters are not generic. Currently, the auth parameters—APIKey, Method, and KeyParamName—are only suitable for metrics APIs, such as the current CustomAuth used for the Prometheus scaler. This means that if other scalers need to refactor their authentication, the number of auth parameters will continue to increase. Do you have any suggestions? Or should we just keep adding new parameters for now, since not many scalers have their own authentication yet? @wozniakjan @JorTurFer |
I'd move the scaler parameters to the generic configuration. It's true that we must avoid breaking changes, so maybe we have to deprecate the old values and move them to a generic configuration and we can use for any scaler. I agree with generalize and making things common for all the scaler |
|
/run-e2e metrics_api |
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
* one commit on spiritzhou/refactormetricapi Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update pkg/scalers/metrics_api_scaler.go Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> --------- Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: zhenghanzhou <zhenghanzhou@microsoft.com> Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
* one commit on spiritzhou/refactormetricapi Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update pkg/scalers/metrics_api_scaler.go Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> --------- Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: zhenghanzhou <zhenghanzhou@microsoft.com> Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com> Signed-off-by: Dmitriy Altuhov <altuhovd@gmail.com>
* one commit on spiritzhou/refactormetricapi Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update pkg/scalers/metrics_api_scaler.go Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> --------- Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: zhenghanzhou <zhenghanzhou@microsoft.com> Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
* one commit on spiritzhou/refactormetricapi Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update pkg/scalers/metrics_api_scaler.go Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> * Update Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> --------- Signed-off-by: zhenghanzhou <zhenghanzhou@microsoft.com> Signed-off-by: Zhenghan Zhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: zhenghanzhou <zhenghanzhou@microsoft.com> Co-authored-by: Rick Brouwer <rickbrouwer@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Refactor and fix metrics-api scaler
Checklist
Fixes #
Relates to # #6939