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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca8dd42
Fix admission webhook validation not denying invalid fallbacks and re…
vzdai Jul 15, 2024
32ff796
Fix tests for scaledobject webhook validating fallback and replica count
vzdai Aug 13, 2024
10b37cc
Fix changelog sort order
vzdai Oct 7, 2024
480573b
Merge branch 'main' into fallback-validation
JorTurFer Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ Here is an overview of all new **experimental** features:

### Fixes

- **General**: Fix admission webhook validation not denying invalid fallbacks and replica counts ([#5954](https://github.com/kedacore/keda/issues/5954))
- **AWS Secret Manager**: Pod identity overrides are honored ([#6195](https://github.com/kedacore/keda/issues/6195))
- **Azure Event Hub Scaler**: Checkpointer errors are correctly handled ([#6084](https://github.com/kedacore/keda/issues/6084))
- **Metrics API Scaler**: Prometheus metrics can have multiple labels ([#6077](https://github.com/kedacore/keda/issues/6077))
Expand Down
2 changes: 1 addition & 1 deletion apis/keda/v1alpha1/scaledobject_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func CheckFallbackValid(scaledObject *ScaledObject) error {

for _, trigger := range scaledObject.Spec.Triggers {
if trigger.Type == cpuString || trigger.Type == memoryString {
return fmt.Errorf("type is %s , but fallback it is not supported by the CPU & memory scalers", trigger.Type)
return fmt.Errorf("type is %s, but fallback it is not supported by the CPU & memory scalers", trigger.Type)
}
if trigger.MetricType != autoscalingv2.AverageValueMetricType {
return fmt.Errorf("MetricType=%s, but Fallback can only be enabled for triggers with metric of type AverageValue", trigger.MetricType)
Expand Down
2 changes: 2 additions & 0 deletions apis/keda/v1alpha1/scaledobject_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ func verifyReplicaCount(incomingSo *ScaledObject, action string, _ bool) error {
if err != nil {
scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-replicas")
return err
}
return nil
}
Expand All @@ -192,6 +193,7 @@ func verifyFallback(incomingSo *ScaledObject, action string, _ bool) error {
if err != nil {
scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-fallback")
return err
}
return nil
}
Expand Down
15 changes: 6 additions & 9 deletions apis/keda/v1alpha1/scaledobject_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,8 @@ var _ = It("shouldn't validate the so creation when the replica counts are wrong
err := k8sClient.Create(context.Background(), namespace)
Expect(err).ToNot(HaveOccurred())

Eventually(func() error {
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 •
edited
Loading

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.

})

var _ = It("shouldn't validate the so creation when the fallback is wrong", func() {
Expand All @@ -171,9 +170,8 @@ var _ = It("shouldn't validate the so creation when the fallback is wrong", func
err := k8sClient.Create(context.Background(), namespace)
Expect(err).ToNot(HaveOccurred())

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

var _ = It("shouldn't validate the so creation When the fallback are configured and the scaler is either CPU or memory.", func() {
Expand All @@ -191,9 +189,8 @@ var _ = It("shouldn't validate the so creation When the fallback are configured
err = k8sClient.Create(context.Background(), workload)
Expect(err).ToNot(HaveOccurred())

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

var _ = It("shouldn't validate the so creation when there is another unmanaged hpa and so has transfer-hpa-ownership activated", func() {
Expand Down
Loading