fix: Allow to remove the finalizer even if the ScaledObject isn't val…

…id (kedacore#4397)

Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>

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

CHANGELOG.md

@@ -53,6 +53,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio
 
 ### Fixes
 
+- **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))
 - **Azure Pipelines**: Fix for disallowing `$top` on query when using `meta.parentID` method ([#4397])
 - **Azure Pipelines**: Respect all required demands ([#4404](https://github.com/kedacore/keda/issues/4404))

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -62,13 +62,30 @@ 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")
 }
 
 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)

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,88 @@ 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)
+	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() {
+	namespaceName := "scale-to-zero-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, true, false)
+
+	so := createScaledObjectSTZ(soName, namespaceName, workloadName, 0, 5, true)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	err = k8sClient.Create(context.Background(), workload)
+	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() {
+	namespaceName := "scale-to-zero-min-replicas-bad"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, true, 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())
+	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() {
+	namespaceName := "scale-to-zero-no-external-trigger-good"
+	namespace := createNamespace(namespaceName)
+	workload := createDeployment(namespaceName, true, 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())
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+
+})
+
+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())
+
+	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{}
+	Eventually(func() error {
+		return k8sClient.Update(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
 })
 
 var _ = AfterSuite(func() {
@@ -595,3 +689,44 @@ func createStatefulSet(namespace string, hasCPU, hasMemory bool) *appsv1.Statefu
 		},
 	}
 }
+
+func createScaledObjectSTZ(name string, namespace string, targetName string, minReplicas int32, maxReplicas int32, hasExternalTrigger bool) *ScaledObject {
+	triggers := []ScaleTriggers{
+		{
+			Type: "cpu",
+			Metadata: map[string]string{
+				"value": "10",
+			},
+		},
+	}
+	if hasExternalTrigger {
+		kubeWorkloadTrigger := ScaleTriggers{
+			Type: "kubernetes-workload",
+			Metadata: map[string]string{
+				"podSelector": "pod=workload-test",
+				"value":       "1",
+			},
+		}
+		triggers = append(triggers, kubeWorkloadTrigger)
+	}
+	return &ScaledObject{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+			UID:       types.UID(name),
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "ScaledObject",
+			APIVersion: "keda.sh",
+		},
+		Spec: ScaledObjectSpec{
+			ScaleTargetRef: &ScaleTarget{
+				Name: targetName,
+			},
+			MinReplicaCount: pointer.Int32(minReplicas),
+			MaxReplicaCount: pointer.Int32(maxReplicas),
+			CooldownPeriod:  pointer.Int32(1),
+			Triggers:        triggers,
+		},
+	}
+}