fix(ScaledObject): Check default limits from LimitRange for ScaledObject validations

[Bhargav-InfraCloud]

KEDA admission webhook rejects ScaledObject requests with CPU or memory triggers when the resource limits (CPU/memory based on triggers) are not set in the pod spec. This is expected behavior.

But if default limits are set in the LimitRange object in the same namespace, the admission webhook should allow the ScaledObject request, which currently doesn’t.

This change will check if there is a LimitRange with default limits (CPU/memory based on triggers) in the namespace that ScaledObject is in, and allows the request to proceed.

Also, added RBAC permissions for list & watch LimitRange.

Checklist

• I have verified that my change is according to the deprecations & breaking changes policy
• Tests have been added
• Changelog has been updated and is aligned with our changelog requirements
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified) feat(ClusterRole): Add RBAC rule to allow access to LimitRange charts#588
• A PR is opened to update the documentation on (repo) (if applicable) docs(scalers): Update cpu and memory scaler docs for validating requests/limits from LimitRange keda-docs#1296
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #5348

[github-actions]

Thank you for your contribution! 🙏 Let us know when you are ready for a review by publishing the PR.

[Bhargav-InfraCloud]

Minimal Steps to Test the Changes

1. Deploy from the current branch.
2. Run test scripts from keda-5348.zip.
   Only no-default-cpu-memory-limits.sh should throw the below-mentioned error, and no error for default-limits-in-limitrange.sh.

Error from server (Forbidden): error when creating "STDIN": admission webhook "vscaledobject.kb.io" denied the request: the scaledobject has a cpu trigger but the container test-app doesn't have the cpu request defined

[JorTurFer]

/run-e2e internal
Update: You can check the progress here

[JorTurFer]

/run-e2e internal
Update: You can check the progress here

[zroubalik]

And with that, I realized, 😅 I should probably add a note in the docs about the controller picking the default limits from LimitRange for validation.

@Bhargav-InfraCloud yeah, we should document this for sure :)

[Bhargav-InfraCloud]

Please review the PR for updating the docs. Thanks!
kedacore/keda-docs#1296

[zroubalik]

I haven't got time to review properly, but to be clear, we don't enforce requests, but only limits set on the target resource, is this respected in this PR?

#4802

[Bhargav-InfraCloud]

I haven't got time to review properly, but to be clear, we don't enforce requests, but only limits set on the target resource, is this respected in this PR?

#4802

Not exactly. The fix to that issue has only added the validation for limits and hasn't removed the existing validation on requests. It is ORed though. We should probably open a card for that and update the docs too. I see docs have examples with just requests.

keda/apis/keda/v1alpha1/scaledobject_webhook.go

Lines 462 to 464 in 4fd3ed5

	requests, requestsSet := rr.Requests[name]
	limits, limitsSet := rr.Limits[name]
	return (requestsSet || limitsSet) && (requests.AsApproximateFloat64() > 0 || limits.AsApproximateFloat64() > 0)

https://github.com/kedacore/keda-docs/blob/675cdd9b61728a210175075bd284359f27ebd0d6/content/docs/2.13/scalers/cpu.md?plain=1#L23-L30

[wozniakjan]

excellent PR, I think this can be merged, @zroubalik, although I don't know how to get to the failing e2e test logs

I haven't got time to review properly, but to be clear, we don't enforce requests, but only limits set on the target resource, is this respected in this PR?

isn't it the other way around in 2.12.1? from my quick testing requests are enforced, limits can be missing.
never mind 🤦‍♂️, I just realized #4803 is not part of 2.12.1 and will be released in 2.13.0...

[zroubalik]

Not exactly. The fix to that issue has only added the validation for limits and hasn't removed the existing validation on requests. It is ORed though. We should probably open a card for that and update the docs too. I see docs have examples with just requests.

@Bhargav-InfraCloud could you please incorporate this into your docs PR. Once the docs PR is done, we can proceed and merge this PR. Great job, thanks!

[zroubalik]

/run-e2e internal
Update: You can check the progress here

[tomkerkhove]

/run-e2e internal
Update: You can check the progress here

[Bhargav-InfraCloud]

Thanks, everyone! :-)