Skip to content
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

Fix admission webhook validation not denying invalid fallbacks and replica counts #5962

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vzdai
Copy link

@vzdai vzdai commented Jul 15, 2024

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

Fixes #5954

@vzdai vzdai requested a review from a team as a code owner July 15, 2024 00:44
@vzdai
Copy link
Author

vzdai commented Jul 15, 2024

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
Copy link
Member

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?

return k8sClient.Create(context.Background(), so)
}).Should(HaveOccurred())
err = k8sClient.Create(context.Background(), so)
Expect(err).To(HaveOccurred())
Copy link
Author

@vzdai vzdai Aug 13, 2024

Choose a reason for hiding this comment

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

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
Copy link
Member

zroubalik commented Sep 26, 2024

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

@JorTurFer
Copy link
Member

JorTurFer commented Oct 6, 2024

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

@vzdai
Copy link
Author

vzdai commented Oct 7, 2024

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

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.

ScaledObject still gets applied after failing validation due to using a fallback with a CPU trigger
4 participants