Fix admission webhook validation not denying invalid fallbacks and replica counts

[vzdai]

Provide a description of what has been changed

When a ScaledObject with an invalid fallback (using fallback with CPU/Memory scalers) or an invalid replica count (minReplicaCount > maxReplicaCount) was applied, the admission webhook logged an error but the ScaledObject was still updated.

After this fix, attempting to create an invalid ScaledObject will get rejected with the respective error messages:

Error from server (Forbidden): error when creating "scaledobject.yaml": admission webhook "vscaledobject.kb.io" denied the request: type is cpu, but fallback it is not supported by the CPU & memory scalers

Error from server (Forbidden): error when creating "scaledobject.yaml": admission webhook "vscaledobject.kb.io" denied the request: MinReplicaCount=3 must be less than MaxReplicaCount=2

Note: Since invalid ScaledObjects applies are being allowed through right now, users may encounter errors when trying to update the same ScaledObject after this change. I'm unsure if this should be considered a breaking change or if it needs an additional warning somewhere.

Checklist

• When introducing a new scaler, I agree with the scaling governance policy
• 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)
• A PR is opened to update the documentation on (repo) (if applicable)
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #5954

[vzdai]

Regarding tests:

The test case for the webhook rejecting invalid fallback is done here. However, this was falsely passing before due to the section:

	Eventually(func() error {
		return k8sClient.Create(context.Background(), so)
	}).Should(HaveOccurred())

According to Gomega docs, Eventually blocks when called and attempts an assertion periodically until it passes or a timeout occurs. In this test case, we are repeatedly trying to create a new ScaledObject, and previously the first create would pass but the second create would fail with resourceVersion should not be set on objects to be created, since the first create made an in-place update of so and added resourceVersion metadata.

Is there a reason we are wrapping this assertion in an Eventually block? I don't see why this needs to poll or be run asynchronously.

The whole scaledobject_webhook_test.go file uses Eventually in most of its tests, but I believe they can be simplified into just

	err = k8sClient.Create(context.Background(), so)
	Expect(err).ToNot(HaveOccurred())

[JorTurFer]

Awesome catch! In theory, the addmission webhook must block invalid SO, so I think that the fix isn't a breaking change (@tomkerkhove @zroubalik ?) Could you update the test to ensure that the behaviour is correctly covered by tests?

[vzdai]

I've removed the use of Eventually in these tests according to #5962 (comment).

I also set up vzdai#1 to remove Eventually from all other tests where it's not needed. That branch is based off of this branch (since without the changes here, some tests would fail), and once this PR gets merged, I can remake that PR in kedacore:main.

[zroubalik]

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

[JorTurFer]

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

[vzdai]

Rebased onto main due to a merge conflict with the changelog.

[JorTurFer]

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

[JorTurFer]

PTAL @wozniakjan

[zroubalik]

What if there is some scaler that supports fallback (eg. Prometheus) and also CPU/Memory scaler?

[wozniakjan]

What if there is some scaler that supports fallback (eg. Prometheus) and also CPU/Memory scaler?

imho ideally KEDA should allow this, looks like this PR would make it not possible while in previous versions it just logged as an error?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed due to inactivity.