-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Conversation
Regarding tests: The test case for the webhook rejecting invalid fallback is done here. However, this was falsely passing before due to the section:
According to Gomega docs, Is there a reason we are wrapping this assertion in an The whole
|
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? |
4003f16
to
9864f70
Compare
return k8sClient.Create(context.Background(), so) | ||
}).Should(HaveOccurred()) | ||
err = k8sClient.Create(context.Background(), so) | ||
Expect(err).To(HaveOccurred()) |
There was a problem hiding this comment.
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
.
/run-e2e |
/run-e2e |
…plica counts Signed-off-by: Vivian Dai <viviandai12@gmail.com>
Signed-off-by: Vivian Dai <viviandai12@gmail.com>
ce73bea
to
32ff796
Compare
Rebased onto |
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:
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