prevent HPA deletion via webhook

api/autoscaling/v2/horizontalpodautoscaler_webhook.go

@@ -27,9 +27,11 @@ package autoscalingv2
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	v2 "k8s.io/api/autoscaling/v2"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
@@ -77,7 +79,7 @@ func (h *HPAWebhook) Default(ctx context.Context, obj runtime.Object) error {
 	}
 
 	if t.Status.Targets.HorizontalPodAutoscaler != hpa.Name {
-		// not managed by tortoise
+		// should not reach here
 		return nil
 	}
 
@@ -95,3 +97,64 @@ func (h *HPAWebhook) Default(ctx context.Context, obj runtime.Object) error {
 
 	return nil
 }
+
+//+kubebuilder:webhook:path=/validate-autoscaling-v2-horizontalpodautoscaler,mutating=false,failurePolicy=fail,sideEffects=None,groups=autoscaling,resources=horizontalpodautoscalers,verbs=delete,versions=v2,name=mhorizontalpodautoscaler.kb.io,admissionReviewVersions=v1
+
+var _ admission.CustomValidator = &HPAWebhook{}
+
+// ValidateCreate validates the object on creation.
+// The optional warnings will be added to the response as warning messages.
+// Return an error if the object is invalid.
+func (*HPAWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (warnings admission.Warnings, err error) {
+	return nil, nil
+}
+
+// ValidateUpdate validates the object on update.
+// The optional warnings will be added to the response as warning messages.
+// Return an error if the object is invalid.
+func (*HPAWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (warnings admission.Warnings, err error) {
+	return nil, nil
+}
+
+// ValidateDelete validates the object on deletion.
+// The optional warnings will be added to the response as warning messages.
+// Return an error if the object is invalid.
+func (h *HPAWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (warnings admission.Warnings, err error) {
+	hpa := obj.(*v2.HorizontalPodAutoscaler)
+	tortoiseName, ok := hpa.GetAnnotations()[annotation.TortoiseNameAnnotation]
+	if !ok {
+		// not managed by tortoise
+		return nil, nil
+	}
+
+	t, err := h.tortoiseService.GetTortoise(ctx, types.NamespacedName{
+		Namespace: hpa.Namespace,
+		Name:      tortoiseName,
+	})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			// expected scenario - tortoise is deleted before HPA is deleted.
+			return nil, nil
+		}
+		// unknown error
+		return nil, fmt.Errorf("failed to get tortoise(%s) for mutating webhook of HPA(%s/%s): %w", tortoiseName, hpa.Namespace, hpa.Name, err)
+	}
+	if !t.ObjectMeta.DeletionTimestamp.IsZero() {
+		// expected scenario - tortoise is being deleted before HPA is deleted.
+		return nil, nil
+	}
+
+	if t.Status.Targets.HorizontalPodAutoscaler != hpa.Name {
+		// should not reach here
+		return nil, nil
+	}
+
+	message := fmt.Sprintf("HPA(%s/%s) is being deleted while Tortoise(%s) is running. Please delete Tortoise first", hpa.Namespace, hpa.Name, tortoiseName)
+
+	if t.Spec.UpdateMode == v1beta2.UpdateModeOff {
+		// DryRun - don't block the deletion of HPA, but emit warning.
+		return []string{message}, nil
+	}
+
+	return nil, fmt.Errorf(message)
+}

api/autoscaling/v2/horizontalpodautoscaler_webhook_test.go

@@ -28,34 +28,33 @@ package autoscalingv2
 import (
 	"bytes"
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
 
 	v2 "k8s.io/api/autoscaling/v2"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/yaml"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/mercari/tortoise/api/v1beta2"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
-func mutateTest(before, after, torotise string) {
+func mutateTest(before, after, tortoise string) {
 	ctx := context.Background()
 
-	y, err := os.ReadFile(torotise)
+	y, err := os.ReadFile(tortoise)
 	Expect(err).NotTo(HaveOccurred())
 	tor := &v1beta2.Tortoise{}
 	err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(tor)
 	status := tor.Status
 	Expect(err).NotTo(HaveOccurred())
 	err = k8sClient.Create(ctx, tor.DeepCopy())
 	Expect(err).NotTo(HaveOccurred())
-	defer func() {
-		err = k8sClient.Delete(ctx, tor)
-		Expect(err).NotTo(HaveOccurred())
-	}()
 
 	err = k8sClient.Get(ctx, types.NamespacedName{Name: tor.GetName(), Namespace: tor.GetNamespace()}, tor)
 	Expect(err).NotTo(HaveOccurred())
@@ -71,6 +70,9 @@ func mutateTest(before, after, torotise string) {
 	err = k8sClient.Create(ctx, beforehpa)
 	Expect(err).NotTo(HaveOccurred())
 	defer func() {
+		// cleanup
+		err = k8sClient.Delete(ctx, tor)
+		Expect(err).NotTo(HaveOccurred())
 		err = k8sClient.Delete(ctx, beforehpa)
 		Expect(err).NotTo(HaveOccurred())
 	}()
@@ -88,6 +90,63 @@ func mutateTest(before, after, torotise string) {
 	Expect(ret.Spec).Should(Equal(afterhpa.Spec))
 }
 
+func validateDeletionTest(hpa, tortoise string, valid bool) {
+	ctx := context.Background()
+
+	var tor *v1beta2.Tortoise
+	if tortoise != "" {
+		y, err := os.ReadFile(tortoise)
+		Expect(err).NotTo(HaveOccurred())
+		tor = &v1beta2.Tortoise{}
+		err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(tor)
+		status := tor.Status
+		Expect(err).NotTo(HaveOccurred())
+		err = k8sClient.Create(ctx, tor.DeepCopy())
+		Expect(err).NotTo(HaveOccurred())
+
+		err = k8sClient.Get(ctx, types.NamespacedName{Name: tor.GetName(), Namespace: tor.GetNamespace()}, tor)
+		Expect(err).NotTo(HaveOccurred())
+		tor.Status = status
+		err = k8sClient.Status().Update(ctx, tor)
+		Expect(err).NotTo(HaveOccurred())
+	}
+
+	y, err := os.ReadFile(hpa)
+	Expect(err).NotTo(HaveOccurred())
+	beforehpa := &v2.HorizontalPodAutoscaler{}
+	err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(beforehpa)
+	Expect(err).NotTo(HaveOccurred())
+	err = k8sClient.Create(ctx, beforehpa)
+	Expect(err).NotTo(HaveOccurred())
+
+	defer func() {
+		// cleanup
+		if tortoise != "" {
+			err = k8sClient.Delete(ctx, tor)
+			Expect(err).NotTo(HaveOccurred())
+		}
+		if !valid { // if valid, HPA is already deleted
+			err = k8sClient.Delete(ctx, beforehpa)
+			Expect(err).NotTo(HaveOccurred())
+		}
+	}()
+
+	ret := &v2.HorizontalPodAutoscaler{}
+	err = k8sClient.Get(ctx, types.NamespacedName{Name: beforehpa.GetName(), Namespace: beforehpa.GetNamespace()}, ret)
+	Expect(err).NotTo(HaveOccurred())
+
+	err = k8sClient.Delete(ctx, beforehpa)
+	if valid {
+		Expect(err).NotTo(HaveOccurred(), "Tortoise: %v", beforehpa)
+	} else {
+		Expect(err).To(HaveOccurred(), "Tortoise: %v", beforehpa)
+		statusErr := &apierrors.StatusError{}
+		Expect(errors.As(err, &statusErr)).To(BeTrue())
+		expected := beforehpa.Annotations["message"]
+		Expect(statusErr.ErrStatus.Message).To(ContainSubstring(expected))
+	}
+}
+
 var _ = Describe("v2.HPA Webhook", func() {
 	Context("mutating", func() {
 		It("HPA is mutated based on the recommendation", func() {
@@ -101,4 +160,58 @@ var _ = Describe("v2.HPA Webhook", func() {
 			mutateTest(filepath.Join("testdata", "mutating", "has-annotation-but-invalid2", "before.yaml"), filepath.Join("testdata", "mutating", "has-annotation-but-invalid2", "after.yaml"), filepath.Join("testdata", "mutating", "has-annotation-but-invalid2", "tortoise.yaml"))
 		})
 	})
+	Context("validating", func() {
+		It("valid: HPA can be deleted when Tortoise (Off) exists", func() {
+			validateDeletionTest(filepath.Join("testdata", "validating", "hpa-with-off", "hpa.yaml"), filepath.Join("testdata", "validating", "hpa-with-off", "tortoise.yaml"), true)
+		})
+		It("valid: HPA can be deleted when Tortoise (Auto) is deleted", func() {
+			validateDeletionTest(filepath.Join("testdata", "validating", "hpa-with-auto-deleted", "hpa.yaml"), "", true)
+		})
+		It("invalid: HPA cannot be deleted when Tortoise (Auto) exists", func() {
+			validateDeletionTest(filepath.Join("testdata", "validating", "hpa-with-auto-existing", "hpa.yaml"), filepath.Join("testdata", "validating", "hpa-with-auto-existing", "tortoise.yaml"), false)
+		})
+		It("valid: HPA can be deleted when Tortoise (Auto) is being deleted", func() {
+			// create tortoise
+			y, err := os.ReadFile(filepath.Join("testdata", "validating", "hpa-with-auto-being-deleted", "tortoise.yaml"))
+			Expect(err).NotTo(HaveOccurred())
+			tor := &v1beta2.Tortoise{}
+			err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(tor)
+			// add finalizer
+			controllerutil.AddFinalizer(tor, tortoiseFinalizer)
+			status := tor.Status
+			Expect(err).NotTo(HaveOccurred())
+			err = k8sClient.Create(ctx, tor.DeepCopy())
+			Expect(err).NotTo(HaveOccurred())
+
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: tor.GetName(), Namespace: tor.GetNamespace()}, tor)
+			Expect(err).NotTo(HaveOccurred())
+			tor.Status = status
+			err = k8sClient.Status().Update(ctx, tor)
+			Expect(err).NotTo(HaveOccurred())
+
+			// delete tortoise, but it shouldn't be deleted because of the finalizer
+			err = k8sClient.Delete(ctx, tor)
+			Expect(err).NotTo(HaveOccurred())
+
+			// create HPA
+			y, err = os.ReadFile(filepath.Join("testdata", "validating", "hpa-with-auto-being-deleted", "hpa.yaml"))
+			Expect(err).NotTo(HaveOccurred())
+			beforehpa := &v2.HorizontalPodAutoscaler{}
+			err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(beforehpa)
+			Expect(err).NotTo(HaveOccurred())
+			err = k8sClient.Create(ctx, beforehpa)
+			Expect(err).NotTo(HaveOccurred())
+
+			// try to delete HPA
+			err = k8sClient.Delete(ctx, beforehpa)
+			Expect(err).NotTo(HaveOccurred(), "Tortoise: %v", beforehpa)
+
+			// remove finalizer and remove tortoise (cleanup)
+			controllerutil.RemoveFinalizer(tor, tortoiseFinalizer)
+			err = k8sClient.Delete(ctx, tor)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
 })
+
+const tortoiseFinalizer = "tortoise.autoscaling.mercari.com/finalizer"

api/autoscaling/v2/testdata/validating/hpa-with-auto-being-deleted/hpa.yaml

@@ -0,0 +1,29 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: sample
+  namespace: default
+  annotations:
+    tortoises.autoscaling.mercari.com/tortoise-name: tortoise-sample
+spec:
+  maxReplicas: 12
+  metrics:
+    - type: ContainerResource
+      containerResource:
+        name: cpu
+        container: nginx
+        target:
+          type: Utilization
+          averageUtilization: 30
+    - type: ContainerResource
+      containerResource:
+        name: cpu
+        container: istio-proxy
+        target:
+          type: Utilization
+          averageUtilization: 30
+  minReplicas: 3
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: sample

api/autoscaling/v2/testdata/validating/hpa-with-auto-being-deleted/tortoise.yaml

@@ -0,0 +1,92 @@
+apiVersion: autoscaling.mercari.com/v1beta2
+kind: Tortoise
+metadata:
+  name: tortoise-sample
+  namespace: default
+  deletionTimestamp: "2023-10-04T15:45:16Z"
+spec:
+  updateMode: "Auto"
+  deletionPolicy: "DeleteAll"
+  targetRefs:
+    horizontalPodAutoscalerName: sample
+    scaleTargetRef:
+      kind: Deployment
+      name: sample
+  resourcePolicy:
+    - containerName: istio-proxy
+      autoscalingPolicy:
+        cpu: Horizontal
+        memory: Vertical
+    - containerName: nginx
+      autoscalingPolicy:
+        cpu: Horizontal
+        memory: Vertical
+status:
+  tortoisePhase: Working
+  containerResourcePhases:
+    - containerName: "nginx"
+      resourcePhases:
+        cpu: 
+          phase: Working 
+        memory:
+          phase: Working 
+    - containerName: "istio-proxy"
+      resourcePhases:
+        cpu: 
+          phase: Working 
+        memory:
+          phase: Working 
+  targets:
+    scaleTargetRef:
+      kind: Deployment
+      name: sample
+    horizontalPodAutoscaler: sample
+    verticalPodAutoscalers: 
+    - name: tortoise-monitor-sample
+      role: Monitor
+    - name: tortoise-updater-sample
+      role: Updater
+  conditions:
+    containerRecommendationFromVPA:
+    - containerName: echo
+      maxRecommendation:
+        cpu:
+          quantity: 6m
+          updatedAt: "2023-10-04T15:45:16Z"
+        memory:
+          quantity: "56623104"
+          updatedAt: "2023-10-04T15:45:16Z"
+      recommendation:
+        cpu:
+          quantity: 6m
+          updatedAt: "2023-10-04T15:45:16Z"
+        memory:
+          quantity: "56623104"
+          updatedAt: "2023-10-04T15:45:16Z"
+  recommendations:
+      horizontal:
+        targetUtilizations:
+        - containerName: "nginx"
+          targetUtilization:
+            cpu: 30
+        - containerName: "istio-proxy"
+          targetUtilization:
+            cpu: 30
+        maxReplicas:
+        - from: 0
+          timezone: Asia/Tokyo
+          to: 24
+          updatedAt: "2023-10-04T15:45:16Z"
+          value: 12
+        minReplicas:
+        - from: 0
+          timezone: Asia/Tokyo
+          to: 1
+          updatedAt: "2023-10-04T15:45:16Z"
+          value: 3
+      vertical:
+        containerResourceRecommendation:
+        - RecommendedResource:
+            cpu: 6m
+            memory: "56623104"
+          containerName: nginx