From 767a0e0f0416c5e0d8ed4d5966c1ca63657219f6 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Tue, 21 Mar 2023 22:50:17 +0100 Subject: [PATCH 1/3] fix: Allow to remove the finalizer even if the ScaledObject isn't valid Signed-off-by: Jorge Turrado --- BUILD.md | 3 ++- CHANGELOG.md | 1 + apis/keda/v1alpha1/scaledobject_webhook.go | 17 ++++++++++++ .../v1alpha1/scaledobject_webhook_test.go | 26 +++++++++++++++++++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/BUILD.md b/BUILD.md index e417655c7a6..28f798891d8 100644 --- a/BUILD.md +++ b/BUILD.md @@ -287,7 +287,8 @@ Follow these instructions if you want to debug the KEDA webhook using VS Code. clientConfig: url: "https://${YOUR_URL}/validate-keda-sh-v1alpha1-scaledobject" ``` - **Note:** You could need to define also the key `caBundle` with the CA bundle encoded in base64 if the cluster can get it during the manifest apply (this happens with localtunnel for instance) + **Note:** You need to define also the key `caBundle` with the CA bundle encoded in base64. This `caBundle` is the pem file from the CA used to sign the certificate. Remember to disable the `caBundle` inyection to avoid unintended rewrites of your `caBundle` (by KEDA operator or by any other 3rd party) + 4. Deploy CRDs and KEDA into `keda` namespace ```bash diff --git a/CHANGELOG.md b/CHANGELOG.md index ed5eb5b591f..97325913159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio ### Fixes +- **General**: Allow to remove the finalizer even if the ScaledObject isn't valid ([#4396](https://github.com/kedacore/keda/issue/4396)) - **AWS SQS Scaler**: Respect `scaleOnInFlight` value ([#4276](https://github.com/kedacore/keda/issue/4276)) ### Deprecations diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index d81160a23d1..77570c38ba3 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -65,6 +65,12 @@ func (so *ScaledObject) ValidateCreate() error { func (so *ScaledObject) ValidateUpdate(old runtime.Object) error { val, _ := json.MarshalIndent(so, "", " ") scaledobjectlog.V(1).Info(fmt.Sprintf("validating scaledobject update for %s", string(val))) + + if isRemovingFinalizer(so, old) { + scaledobjectlog.V(1).Info("finalizer removal, skipping validation") + return nil + } + return validateWorkload(so, "update") } @@ -72,6 +78,17 @@ func (so *ScaledObject) ValidateDelete() error { return nil } +func isRemovingFinalizer(so *ScaledObject, old runtime.Object) bool { + oldSo := old.(*ScaledObject) + + soSpec, _ := json.MarshalIndent(so.Spec, "", " ") + oldSoSpec, _ := json.MarshalIndent(oldSo.Spec, "", " ") + soSpecString := string(soSpec) + oldSoSpecString := string(oldSoSpec) + + return len(so.ObjectMeta.Finalizers) == 0 && len(oldSo.ObjectMeta.Finalizers) == 1 && soSpecString == oldSoSpecString +} + func validateWorkload(so *ScaledObject, action string) error { prommetrics.RecordScaledObjectValidatingTotal(so.Namespace, action) err := verifyCPUMemoryScalers(so, action) diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index f5ea8f17af5..aa7160a9623 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/apis/keda/v1alpha1/scaledobject_webhook_test.go @@ -449,6 +449,32 @@ var _ = It("should validate so creation when min replicas is > 0 with only cpu s }) +var _ = It("should validate the so update if it's removing the finalizer even if it's invalid", func() { + + namespaceName := "removing-finalizers" + namespace := createNamespace(namespaceName) + workload := createDeployment(namespaceName, true, true) + so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", true) + so.ObjectMeta.Finalizers = append(so.ObjectMeta.Finalizers, "finalizer") + + err := k8sClient.Create(context.Background(), namespace) + Expect(err).ToNot(HaveOccurred()) + + err = k8sClient.Create(context.Background(), workload) + Expect(err).ToNot(HaveOccurred()) + + err = k8sClient.Create(context.Background(), so) + Expect(err).ToNot(HaveOccurred()) + + workload.Spec.Template.Spec.Containers[0].Resources.Requests = nil + err = k8sClient.Update(context.Background(), workload) + Expect(err).ToNot(HaveOccurred()) + + so.ObjectMeta.Finalizers = []string{} + err = k8sClient.Update(context.Background(), so) + Expect(err).ToNot(HaveOccurred()) +}) + var _ = AfterSuite(func() { cancel() By("tearing down the test environment") From 0a85d274060b8c5251d555c203656016009c7933 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Ferrero Date: Wed, 22 Mar 2023 09:28:51 +0100 Subject: [PATCH 2/3] Update CHANGELOG.md Co-authored-by: Tom Kerkhove Signed-off-by: Jorge Turrado Ferrero --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97325913159..419ba137ec6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio ### Fixes -- **General**: Allow to remove the finalizer even if the ScaledObject isn't valid ([#4396](https://github.com/kedacore/keda/issue/4396)) +- **Admission Webhooks**: Allow to remove the finalizer even if the ScaledObject isn't valid ([#4396](https://github.com/kedacore/keda/issue/4396)) - **AWS SQS Scaler**: Respect `scaleOnInFlight` value ([#4276](https://github.com/kedacore/keda/issue/4276)) ### Deprecations From fe21152f5a2c21afef0ce4154ff0f63b470ded2f Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Wed, 22 Mar 2023 12:08:35 +0100 Subject: [PATCH 3/3] use Eventually for unit tests Signed-off-by: Jorge Turrado --- .../v1alpha1/scaledobject_webhook_test.go | 106 +++++++++++------- 1 file changed, 63 insertions(+), 43 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index aa7160a9623..d2e3b369243 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/apis/keda/v1alpha1/scaledobject_webhook_test.go @@ -154,8 +154,9 @@ var _ = It("should validate the so creation when there isn't any hpa", func() { err := k8sClient.Create(context.Background(), namespace) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("should validate the so creation when there are other SO for other workloads", func() { @@ -171,8 +172,9 @@ var _ = It("should validate the so creation when there are other SO for other wo err = k8sClient.Create(context.Background(), so1) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so2) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so2) + }).ShouldNot(HaveOccurred()) }) var _ = It("should validate the so creation when there are other HPA for other workloads", func() { @@ -188,8 +190,9 @@ var _ = It("should validate the so creation when there are other HPA for other w err = k8sClient.Create(context.Background(), hpa) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("should validate the so creation when it's own hpa is already generated", func() { @@ -206,8 +209,9 @@ var _ = It("should validate the so creation when it's own hpa is already generat err = k8sClient.Create(context.Background(), hpa) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("should validate the so update when it's own hpa is already generated", func() { @@ -228,8 +232,9 @@ var _ = It("should validate the so update when it's own hpa is already generated Expect(err).ToNot(HaveOccurred()) so.Spec.MaxReplicaCount = pointer.Int32(7) - err = k8sClient.Update(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Update(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("shouldn't validate the so creation when there is another unmanaged hpa", func() { @@ -246,8 +251,9 @@ var _ = It("shouldn't validate the so creation when there is another unmanaged h err = k8sClient.Create(context.Background(), hpa) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("shouldn't validate the so creation when there is another so", func() { @@ -264,8 +270,9 @@ var _ = It("shouldn't validate the so creation when there is another so", func() err = k8sClient.Create(context.Background(), so2) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("shouldn't validate the so creation when there is another hpa with custom apis", func() { @@ -282,8 +289,9 @@ var _ = It("shouldn't validate the so creation when there is another hpa with cu err = k8sClient.Create(context.Background(), hpa) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("should validate the so creation with cpu and memory when deployment has requests", func() { @@ -299,8 +307,9 @@ var _ = It("should validate the so creation with cpu and memory when deployment err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("shouldn't validate the so creation with cpu and memory when deployment hasn't got memory request", func() { @@ -316,8 +325,9 @@ var _ = It("shouldn't validate the so creation with cpu and memory when deployme err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("shouldn't validate the so creation with cpu and memory when deployment hasn't got cpu request", func() { @@ -333,8 +343,9 @@ var _ = It("shouldn't validate the so creation with cpu and memory when deployme err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("should validate the so creation with cpu and memory when statefulset has requests", func() { @@ -350,8 +361,9 @@ var _ = It("should validate the so creation with cpu and memory when statefulset err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("shouldn't validate the so creation with cpu and memory when statefulset hasn't got memory request", func() { @@ -367,8 +379,9 @@ var _ = It("shouldn't validate the so creation with cpu and memory when stateful err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("shouldn't validate the so creation with cpu and memory when statefulset hasn't got cpu request", func() { @@ -384,8 +397,9 @@ var _ = It("shouldn't validate the so creation with cpu and memory when stateful err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("should validate the so creation without cpu and memory when custom resources", func() { @@ -397,8 +411,9 @@ var _ = It("should validate the so creation without cpu and memory when custom r err := k8sClient.Create(context.Background(), namespace) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("should validate so creation when all requirements are met for scaling to zero with cpu scaler", func() { @@ -406,7 +421,7 @@ var _ = It("should validate so creation when all requirements are met for scalin namespace := createNamespace(namespaceName) workload := createDeployment(namespaceName, true, false) - scaledobject := createScaledObjectSTZ(soName, namespaceName, workloadName, 0, 5, true) + so := createScaledObjectSTZ(soName, namespaceName, workloadName, 0, 5, true) err := k8sClient.Create(context.Background(), namespace) Expect(err).ToNot(HaveOccurred()) @@ -414,8 +429,9 @@ var _ = It("should validate so creation when all requirements are met for scalin err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), scaledobject) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = It("shouldn't validate so creation with cpu scaler requirements not being met for scaling to 0", func() { @@ -423,14 +439,15 @@ var _ = It("shouldn't validate so creation with cpu scaler requirements not bein namespace := createNamespace(namespaceName) workload := createDeployment(namespaceName, true, false) - scaledobject := createScaledObjectSTZ(soName, namespaceName, workloadName, 0, 5, false) + so := createScaledObjectSTZ(soName, namespaceName, workloadName, 0, 5, false) err := k8sClient.Create(context.Background(), namespace) Expect(err).ToNot(HaveOccurred()) err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), scaledobject) - Expect(err).To(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).Should(HaveOccurred()) }) var _ = It("should validate so creation when min replicas is > 0 with only cpu scaler given", func() { @@ -438,14 +455,15 @@ var _ = It("should validate so creation when min replicas is > 0 with only cpu s namespace := createNamespace(namespaceName) workload := createDeployment(namespaceName, true, false) - scaledobject := createScaledObjectSTZ(soName, namespaceName, workloadName, 1, 5, false) + so := createScaledObjectSTZ(soName, namespaceName, workloadName, 1, 5, false) err := k8sClient.Create(context.Background(), namespace) Expect(err).ToNot(HaveOccurred()) err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), scaledobject) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) @@ -463,16 +481,18 @@ var _ = It("should validate the so update if it's removing the finalizer even if err = k8sClient.Create(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) - err = k8sClient.Create(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) workload.Spec.Template.Spec.Containers[0].Resources.Requests = nil err = k8sClient.Update(context.Background(), workload) Expect(err).ToNot(HaveOccurred()) so.ObjectMeta.Finalizers = []string{} - err = k8sClient.Update(context.Background(), so) - Expect(err).ToNot(HaveOccurred()) + Eventually(func() error { + return k8sClient.Update(context.Background(), so) + }).ShouldNot(HaveOccurred()) }) var _ = AfterSuite(func() {