Skip to content

fix: Admission Webhook blocks ScaledObject without metricType with fallback #6702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 15, 2025

Conversation

rickbrouwer
Copy link
Contributor

@rickbrouwer rickbrouwer commented Apr 9, 2025

In scaler.go there is a function GetMetricTargetType which helps get the metric target type of the scaler. If no metric type is provided then the AverageValue is used.

In v2.17 is a check in the webhook admission that if at least one trigger is of the type AverageValue, then having fallback is valid. A metricType is optional and often not supplied. As a result, many scaledObjects now give errors when supplying a fallback without metricType.

Adding an extra check if the trigger.MetricType is empty will fix this issue. This check is also in the scaler.go.

Any feedback regarding the chosen solution is okay.

Checklist

Fixes #6696

@rickbrouwer rickbrouwer requested a review from a team as a code owner April 9, 2025 11:34
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

could you please also add an e2e test for this case so future KEDA releases are a bit safer? Don't feel pressured, if it's not in your capacity at the moment, I'm happy to approve this as is and add the e2e test coverage later this week.

@wozniakjan
Copy link
Member

wozniakjan commented Apr 9, 2025

/run-e2e fallback|scaling_modifiers
Update: You can check the progress here

Signed-off-by: Rick Brouwer <[email protected]>
@rickbrouwer
Copy link
Contributor Author

thank you!

could you please also add an e2e test for this case so future KEDA releases are a bit safer? Don't feel pressured, if it's not in your capacity at the moment, I'm happy to approve this as is and add the e2e test coverage later this week.

I'll add a unit test first. That also does a good check. I saw that scaledobject_types doesn't have this anyway, so here it is :)

I will create a test case in the e2e of fallback later.

Signed-off-by: rickbrouwer <[email protected]>
@rickbrouwer
Copy link
Contributor Author

Also added an e2e test

@wozniakjan
Copy link
Member

wozniakjan commented Apr 9, 2025

/run-e2e fallback|scaling_modifiers
Update: You can check the progress here

@rickbrouwer
Copy link
Contributor Author

Added more unit tests for the remaining functions of scaledobject_types

@wozniakjan
Copy link
Member

wozniakjan commented Apr 10, 2025

/run-e2e fallback|scaling_modifiers
Update: You can check the progress here

Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
@wozniakjan
Copy link
Member

wozniakjan commented Apr 10, 2025

/run-e2e fallback|scaling_modifiers
Update: You can check the progress here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@wozniakjan wozniakjan merged commit 476717e into kedacore:main Apr 15, 2025
21 checks passed
@rickbrouwer rickbrouwer deleted the issue-6696 branch April 15, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admission Webhook blocks ScaledObject with fallback (using AverageValue) 2.17
3 participants