From 9ab5b47a6e5dae5392f565f90527a185549ef214 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Tue, 10 Oct 2023 20:21:02 +0900 Subject: [PATCH] add the test cases for updating validation (#161) --- .../before-tortoise.yaml | 22 +++++ .../no-horizontal-with-hpa/deployment.yaml | 26 ++++++ .../no-horizontal-with-hpa/hpa.yaml | 27 ++++++ .../updating-tortoise.yaml | 22 +++++ .../before-tortoise.yaml | 21 +++++ .../deployment.yaml | 26 ++++++ .../no-horizontal-with-no-deletion/hpa.yaml | 27 ++++++ .../updating-tortoise.yaml | 21 +++++ .../before-tortoise.yaml | 22 +++++ .../deployment.yaml | 26 ++++++ .../success-remove-all-horizontal/hpa.yaml | 27 ++++++ .../updating-tortoise.yaml | 21 +++++ api/v1beta1/tortoise_webhook.go | 18 ++-- api/v1beta1/tortoise_webhook_test.go | 86 +++++++++++++++++-- 14 files changed, 375 insertions(+), 17 deletions(-) create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-hpa/before-tortoise.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-hpa/deployment.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-hpa/hpa.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-hpa/updating-tortoise.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/before-tortoise.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/deployment.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/hpa.yaml create mode 100644 api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/updating-tortoise.yaml create mode 100644 api/v1beta1/testdata/validating/success-remove-all-horizontal/before-tortoise.yaml create mode 100644 api/v1beta1/testdata/validating/success-remove-all-horizontal/deployment.yaml create mode 100644 api/v1beta1/testdata/validating/success-remove-all-horizontal/hpa.yaml create mode 100644 api/v1beta1/testdata/validating/success-remove-all-horizontal/updating-tortoise.yaml diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-hpa/before-tortoise.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/before-tortoise.yaml new file mode 100644 index 00000000..2b9ce752 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/before-tortoise.yaml @@ -0,0 +1,22 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + 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 \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-hpa/deployment.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/deployment.yaml new file mode 100644 index 00000000..d431d7f5 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/deployment.yaml @@ -0,0 +1,26 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: sample + namespace: default + labels: + app: nginx +spec: + replicas: 3 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: istio-proxy + image: istio-proxy:1.0.0 + ports: + - containerPort: 81 + - name: nginx + image: nginx:1.14.2 + ports: + - containerPort: 80 \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-hpa/hpa.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/hpa.yaml new file mode 100644 index 00000000..fa1875c6 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/hpa.yaml @@ -0,0 +1,27 @@ +apiVersion: autoscaling/v2 +kind: HorizontalPodAutoscaler +metadata: + name: sample + namespace: default +spec: + maxReplicas: 10 + metrics: + - type: ContainerResource + containerResource: + name: cpu + container: nginx + target: + type: Utilization + averageUtilization: 60 + - type: ContainerResource + containerResource: + name: cpu + container: istio-proxy + target: + type: Utilization + averageUtilization: 60 + minReplicas: 3 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: sample \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-hpa/updating-tortoise.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/updating-tortoise.yaml new file mode 100644 index 00000000..6426ad71 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-hpa/updating-tortoise.yaml @@ -0,0 +1,22 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + deletionPolicy: "DeleteAll" + targetRefs: + horizontalPodAutoscalerName: "sample" + scaleTargetRef: + kind: Deployment + name: sample + resourcePolicy: + - containerName: istio-proxy + autoscalingPolicy: + cpu: Vertical + memory: Vertical + - containerName: nginx + autoscalingPolicy: + cpu: Vertical + memory: Vertical \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/before-tortoise.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/before-tortoise.yaml new file mode 100644 index 00000000..91ab691c --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/before-tortoise.yaml @@ -0,0 +1,21 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + deletionPolicy: "NoDelete" + targetRefs: + scaleTargetRef: + kind: Deployment + name: sample + resourcePolicy: + - containerName: istio-proxy + autoscalingPolicy: + cpu: Horizontal + memory: Vertical + - containerName: nginx + autoscalingPolicy: + cpu: Horizontal + memory: Vertical \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/deployment.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/deployment.yaml new file mode 100644 index 00000000..d431d7f5 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/deployment.yaml @@ -0,0 +1,26 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: sample + namespace: default + labels: + app: nginx +spec: + replicas: 3 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: istio-proxy + image: istio-proxy:1.0.0 + ports: + - containerPort: 81 + - name: nginx + image: nginx:1.14.2 + ports: + - containerPort: 80 \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/hpa.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/hpa.yaml new file mode 100644 index 00000000..fa1875c6 --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/hpa.yaml @@ -0,0 +1,27 @@ +apiVersion: autoscaling/v2 +kind: HorizontalPodAutoscaler +metadata: + name: sample + namespace: default +spec: + maxReplicas: 10 + metrics: + - type: ContainerResource + containerResource: + name: cpu + container: nginx + target: + type: Utilization + averageUtilization: 60 + - type: ContainerResource + containerResource: + name: cpu + container: istio-proxy + target: + type: Utilization + averageUtilization: 60 + minReplicas: 3 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: sample \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/updating-tortoise.yaml b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/updating-tortoise.yaml new file mode 100644 index 00000000..5fca8cbe --- /dev/null +++ b/api/v1beta1/testdata/validating/no-horizontal-with-no-deletion/updating-tortoise.yaml @@ -0,0 +1,21 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + deletionPolicy: "NoDelete" + targetRefs: + scaleTargetRef: + kind: Deployment + name: sample + resourcePolicy: + - containerName: istio-proxy + autoscalingPolicy: + cpu: Vertical + memory: Vertical + - containerName: nginx + autoscalingPolicy: + cpu: Vertical + memory: Vertical \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/success-remove-all-horizontal/before-tortoise.yaml b/api/v1beta1/testdata/validating/success-remove-all-horizontal/before-tortoise.yaml new file mode 100644 index 00000000..a609d33a --- /dev/null +++ b/api/v1beta1/testdata/validating/success-remove-all-horizontal/before-tortoise.yaml @@ -0,0 +1,22 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + deletionPolicy: "NoDelete" + targetRefs: + horizontalPodAutoscalerName: "sample" + scaleTargetRef: + kind: Deployment + name: sample + resourcePolicy: + - containerName: istio-proxy + autoscalingPolicy: + cpu: Horizontal + memory: Vertical + - containerName: nginx + autoscalingPolicy: + cpu: Horizontal + memory: Vertical \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/success-remove-all-horizontal/deployment.yaml b/api/v1beta1/testdata/validating/success-remove-all-horizontal/deployment.yaml new file mode 100644 index 00000000..d431d7f5 --- /dev/null +++ b/api/v1beta1/testdata/validating/success-remove-all-horizontal/deployment.yaml @@ -0,0 +1,26 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: sample + namespace: default + labels: + app: nginx +spec: + replicas: 3 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: istio-proxy + image: istio-proxy:1.0.0 + ports: + - containerPort: 81 + - name: nginx + image: nginx:1.14.2 + ports: + - containerPort: 80 \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/success-remove-all-horizontal/hpa.yaml b/api/v1beta1/testdata/validating/success-remove-all-horizontal/hpa.yaml new file mode 100644 index 00000000..fa1875c6 --- /dev/null +++ b/api/v1beta1/testdata/validating/success-remove-all-horizontal/hpa.yaml @@ -0,0 +1,27 @@ +apiVersion: autoscaling/v2 +kind: HorizontalPodAutoscaler +metadata: + name: sample + namespace: default +spec: + maxReplicas: 10 + metrics: + - type: ContainerResource + containerResource: + name: cpu + container: nginx + target: + type: Utilization + averageUtilization: 60 + - type: ContainerResource + containerResource: + name: cpu + container: istio-proxy + target: + type: Utilization + averageUtilization: 60 + minReplicas: 3 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: sample \ No newline at end of file diff --git a/api/v1beta1/testdata/validating/success-remove-all-horizontal/updating-tortoise.yaml b/api/v1beta1/testdata/validating/success-remove-all-horizontal/updating-tortoise.yaml new file mode 100644 index 00000000..216bec5e --- /dev/null +++ b/api/v1beta1/testdata/validating/success-remove-all-horizontal/updating-tortoise.yaml @@ -0,0 +1,21 @@ +apiVersion: autoscaling.mercari.com/v1beta1 +kind: Tortoise +metadata: + name: tortoise-sample + namespace: default +spec: + updateMode: "Off" + deletionPolicy: "DeleteAll" + targetRefs: + scaleTargetRef: + kind: Deployment + name: sample + resourcePolicy: + - containerName: istio-proxy + autoscalingPolicy: + cpu: Vertical + memory: Vertical + - containerName: nginx + autoscalingPolicy: + cpu: Vertical + memory: Vertical \ No newline at end of file diff --git a/api/v1beta1/tortoise_webhook.go b/api/v1beta1/tortoise_webhook.go index 767ca37b..3938a63f 100644 --- a/api/v1beta1/tortoise_webhook.go +++ b/api/v1beta1/tortoise_webhook.go @@ -253,19 +253,17 @@ func (r *Tortoise) ValidateUpdate(old runtime.Object) (admission.Warnings, error // newly specified or updated. return nil, fmt.Errorf("%s: immutable field get changed", fieldPath.Child("targetRefs", "horizontalPodAutoscalerName")) } - } else { - if oldTortoise.Spec.TargetRefs.HorizontalPodAutoscalerName != nil { - // Old has HorizontalPodAutoscalerName, but the new one doesn't. - // removed. - return nil, fmt.Errorf("%s: immutable field get changed", fieldPath.Child("targetRefs", "horizontalPodAutoscalerName")) - } } - if hasHorizontal(oldTortoise) && !hasHorizontal(r) && r.Spec.DeletionPolicy == DeletionPolicyNoDelete { - // TODO: add test for this. + if hasHorizontal(oldTortoise) && !hasHorizontal(r) { + if r.Spec.DeletionPolicy == DeletionPolicyNoDelete { + // The old one has horizontal, but the new one doesn't have any. + return nil, fmt.Errorf("%s: no horizontal policy exists. It will cause the deletion of HPA and you need to specify DeleteAll to allow the deletion.", fieldPath.Child("targetRefs", "resourcePolicy", "autoscalingPolicy")) + } - // Old has horizontal, but the new one doesn't have any. - return nil, fmt.Errorf("%s: no horizontal policy exists. It will cause the deletion of HPA and you need to specify DeleteAll to allow the deletion.", fieldPath.Child("targetRefs", "resourcePolicy", "autoscalingPolicy")) + if r.Spec.TargetRefs.HorizontalPodAutoscalerName != nil { + return nil, fmt.Errorf("%s: no horizontal policy exists. It will cause the deletion of HPA and you need to remove horizontalPodAutoscalerName to allow the deletion.", fieldPath.Child("targetRefs", "horizontalPodAutoscalerName")) + } } if reflect.DeepEqual(oldTortoise.Spec.ResourcePolicy, r.Spec.ResourcePolicy) { diff --git a/api/v1beta1/tortoise_webhook_test.go b/api/v1beta1/tortoise_webhook_test.go index b64b57d9..93d86081 100644 --- a/api/v1beta1/tortoise_webhook_test.go +++ b/api/v1beta1/tortoise_webhook_test.go @@ -82,7 +82,7 @@ func mutateTest(before, after, deployment string) { Expect(ret.Spec).Should(Equal(afterTortoise.Spec)) } -func validateTest(tortoise, hpa, deployment string, valid bool) { +func validateCreationTest(tortoise, hpa, deployment string, valid bool) { ctx := context.Background() y, err := os.ReadFile(deployment) @@ -131,27 +131,99 @@ func validateTest(tortoise, hpa, deployment string, valid bool) { } } +func validateUpdateTest(tortoise, existingTortoise, hpa, deployment string, valid bool) { + ctx := context.Background() + + y, err := os.ReadFile(deployment) + Expect(err).NotTo(HaveOccurred()) + deploy := &v1.Deployment{} + err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(deploy) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Create(ctx, deploy) + Expect(err).NotTo(HaveOccurred()) + defer func() { + err = k8sClient.Delete(ctx, deploy) + Expect(err).NotTo(HaveOccurred()) + }() + + y, err = os.ReadFile(hpa) + Expect(err).NotTo(HaveOccurred()) + hpaObj := &autoscalingv2.HorizontalPodAutoscaler{} + err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(hpaObj) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Create(ctx, hpaObj) + Expect(err).NotTo(HaveOccurred()) + defer func() { + err = k8sClient.Delete(ctx, hpaObj) + Expect(err).NotTo(HaveOccurred()) + }() + + y, err = os.ReadFile(existingTortoise) + Expect(err).NotTo(HaveOccurred()) + tortoiseObj := &Tortoise{} + err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(tortoiseObj) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Create(ctx, tortoiseObj) + + y, err = os.ReadFile(tortoise) + Expect(err).NotTo(HaveOccurred()) + tortoiseObj = &Tortoise{} + err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(tortoiseObj) + Expect(err).NotTo(HaveOccurred()) + + t := &Tortoise{} + err = k8sClient.Get(ctx, types.NamespacedName{Name: tortoiseObj.GetName(), Namespace: tortoiseObj.GetNamespace()}, t) + Expect(err).NotTo(HaveOccurred()) + t.Spec = tortoiseObj.Spec + err = k8sClient.Update(ctx, t) + + if valid { + Expect(err).NotTo(HaveOccurred(), "Tortoise: %v", tortoiseObj) + + // cleanup + err = k8sClient.Delete(ctx, tortoiseObj) + Expect(err).NotTo(HaveOccurred()) + } else { + Expect(err).To(HaveOccurred(), "Tortoise: %v", tortoiseObj) + statusErr := &apierrors.StatusError{} + Expect(errors.As(err, &statusErr)).To(BeTrue()) + expected := tortoiseObj.Annotations["message"] + Expect(statusErr.ErrStatus.Message).To(ContainSubstring(expected)) + } +} + var _ = Describe("Tortoise Webhook", func() { Context("mutating", func() { It("should mutate a Tortoise", func() { mutateTest(filepath.Join("testdata", "mutating", "before.yaml"), filepath.Join("testdata", "mutating", "after.yaml"), filepath.Join("testdata", "mutating", "deployment.yaml")) }) }) - Context("validating", func() { + Context("validating(creation)", func() { It("should create a valid Tortoise", func() { - validateTest(filepath.Join("testdata", "validating", "success", "tortoise.yaml"), filepath.Join("testdata", "validating", "success", "hpa.yaml"), filepath.Join("testdata", "validating", "success", "deployment.yaml"), true) + validateCreationTest(filepath.Join("testdata", "validating", "success", "tortoise.yaml"), filepath.Join("testdata", "validating", "success", "hpa.yaml"), filepath.Join("testdata", "validating", "success", "deployment.yaml"), true) }) It("invalid: Tortoise has Horizontal for the container, but HPA doens't have ContainerResource metric for that container", func() { - validateTest(filepath.Join("testdata", "validating", "no-metric-in-hpa", "tortoise.yaml"), filepath.Join("testdata", "validating", "no-metric-in-hpa", "hpa.yaml"), filepath.Join("testdata", "validating", "no-metric-in-hpa", "deployment.yaml"), false) + validateCreationTest(filepath.Join("testdata", "validating", "no-metric-in-hpa", "tortoise.yaml"), filepath.Join("testdata", "validating", "no-metric-in-hpa", "hpa.yaml"), filepath.Join("testdata", "validating", "no-metric-in-hpa", "deployment.yaml"), false) }) It("invalid: HPA has ContainerResource metric for the container, but autoscalingPolicy in tortoise isn't Horizontal", func() { - validateTest(filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "hpa.yaml"), filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "deployment.yaml"), false) + validateCreationTest(filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "hpa.yaml"), filepath.Join("testdata", "validating", "no-horizontal-in-tortoise", "deployment.yaml"), false) }) It("invalid: Tortoise has resource policy for non-existing container", func() { - validateTest(filepath.Join("testdata", "validating", "useless-policy", "tortoise.yaml"), filepath.Join("testdata", "validating", "useless-policy", "hpa.yaml"), filepath.Join("testdata", "validating", "useless-policy", "deployment.yaml"), false) + validateCreationTest(filepath.Join("testdata", "validating", "useless-policy", "tortoise.yaml"), filepath.Join("testdata", "validating", "useless-policy", "hpa.yaml"), filepath.Join("testdata", "validating", "useless-policy", "deployment.yaml"), false) }) It("invalid: Tortoise has HPA specified, but no Horizoltal in autoscalingPolicy", func() { - validateTest(filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "tortoise.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "hpa.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "deployment.yaml"), false) + validateCreationTest(filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "tortoise.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "hpa.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "deployment.yaml"), false) + }) + }) + Context("validating(updating)", func() { + It("successfully remove horizontal", func() { + validateUpdateTest(filepath.Join("testdata", "validating", "success-remove-all-horizontal", "updating-tortoise.yaml"), filepath.Join("testdata", "validating", "success-remove-all-horizontal", "before-tortoise.yaml"), filepath.Join("testdata", "validating", "success-remove-all-horizontal", "hpa.yaml"), filepath.Join("testdata", "validating", "success-remove-all-horizontal", "deployment.yaml"), true) + }) + It("no horizontal policy exists and the deletion policy is NoDelete", func() { + validateUpdateTest(filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "updating-tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "before-tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "hpa.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "deployment.yaml"), false) + }) + It("no horizontal policy exists and HPA is specified", func() { + validateUpdateTest(filepath.Join("testdata", "validating", "no-horizontal-with-hpa", "updating-tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-hpa", "before-tortoise.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "hpa.yaml"), filepath.Join("testdata", "validating", "no-horizontal-with-no-deletion", "deployment.yaml"), false) }) }) })