From 209d476cd8c7cf092d350271dfa1fb8553cdeb76 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Fri, 8 Mar 2024 13:23:17 +0800 Subject: [PATCH] fix: validate update AdvancedDaemonSet (#1505) * fix: validate update AdvancedDaemonSet Signed-off-by: hantmac refactor it fix fix Signed-off-by: hantmac * fix ut Signed-off-by: hantmac --------- Signed-off-by: hantmac --- .../daemonset_create_update_handler.go | 104 ++++++++ .../daemonset_validating_handler_test.go | 102 -------- ...ing_handler.go => daemonset_validation.go} | 65 +---- .../validating/daemonset_validation_test.go | 242 ++++++++++++++++++ 4 files changed, 351 insertions(+), 162 deletions(-) create mode 100644 pkg/webhook/daemonset/validating/daemonset_create_update_handler.go delete mode 100644 pkg/webhook/daemonset/validating/daemonset_validating_handler_test.go rename pkg/webhook/daemonset/validating/{daemonset_validating_handler.go => daemonset_validation.go} (78%) create mode 100644 pkg/webhook/daemonset/validating/daemonset_validation_test.go diff --git a/pkg/webhook/daemonset/validating/daemonset_create_update_handler.go b/pkg/webhook/daemonset/validating/daemonset_create_update_handler.go new file mode 100644 index 0000000000..819b08baaa --- /dev/null +++ b/pkg/webhook/daemonset/validating/daemonset_create_update_handler.go @@ -0,0 +1,104 @@ +/* +Copyright 2020 The Kruise Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validating + +import ( + "context" + "net/http" + + admissionv1 "k8s.io/api/admission/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" + genericvalidation "k8s.io/apimachinery/pkg/api/validation" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/klog/v2" + apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" +) + +// ValidateDaemonSetName can be used to check whether the given daemon set name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +var ValidateDaemonSetName = genericvalidation.NameIsDNSSubdomain + +// DaemonSetCreateUpdateHandler handles DaemonSet +type DaemonSetCreateUpdateHandler struct { + // Decoder decodes objects + Decoder *admission.Decoder +} + +func (h *DaemonSetCreateUpdateHandler) validateDaemonSetUpdate(ds, oldDs *appsv1alpha1.DaemonSet) field.ErrorList { + allErrs := apivalidation.ValidateObjectMetaUpdate(&ds.ObjectMeta, &oldDs.ObjectMeta, field.NewPath("metadata")) + daemonset := ds.DeepCopy() + daemonset.Spec.Template = oldDs.Spec.Template + daemonset.Spec.UpdateStrategy = oldDs.Spec.UpdateStrategy + daemonset.Spec.Lifecycle = oldDs.Spec.Lifecycle + daemonset.Spec.BurstReplicas = oldDs.Spec.BurstReplicas + daemonset.Spec.MinReadySeconds = oldDs.Spec.MinReadySeconds + daemonset.Spec.RevisionHistoryLimit = oldDs.Spec.RevisionHistoryLimit + + if !apiequality.Semantic.DeepEqual(daemonset.Spec, oldDs.Spec) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to daemonset spec for fields other than 'BurstReplicas', 'template', 'lifecycle', 'updateStrategy', 'minReadySeconds', and 'revisionHistoryLimit' are forbidden")) + } + allErrs = append(allErrs, validateDaemonSetSpec(&ds.Spec, field.NewPath("spec"))...) + return allErrs +} + +var _ admission.Handler = &DaemonSetCreateUpdateHandler{} + +// Handle handles admission requests. +func (h *DaemonSetCreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + obj := &appsv1alpha1.DaemonSet{} + oldObj := &appsv1alpha1.DaemonSet{} + switch req.AdmissionRequest.Operation { + case admissionv1.Create: + err := h.Decoder.Decode(req, obj) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + allowed, reason, err := validatingDaemonSetFn(ctx, obj) + if err != nil { + klog.Warningf("ds %s/%s action %v fail:%s", obj.Namespace, obj.Name, req.AdmissionRequest.Operation, err.Error()) + return admission.Errored(http.StatusInternalServerError, err) + } + return admission.ValidationResponse(allowed, reason) + + case admissionv1.Update: + err := h.Decoder.Decode(req, obj) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + if err := h.Decoder.DecodeRaw(req.AdmissionRequest.OldObject, oldObj); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + if allErrs := h.validateDaemonSetUpdate(obj, oldObj); len(allErrs) > 0 { + return admission.Errored(http.StatusUnprocessableEntity, allErrs.ToAggregate()) + } + } + + return admission.ValidationResponse(true, "") +} + +var _ admission.DecoderInjector = &DaemonSetCreateUpdateHandler{} + +// InjectDecoder injects the decoder into the DaemonSetCreateUpdateHandler +func (h *DaemonSetCreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { + h.Decoder = d + return nil +} diff --git a/pkg/webhook/daemonset/validating/daemonset_validating_handler_test.go b/pkg/webhook/daemonset/validating/daemonset_validating_handler_test.go deleted file mode 100644 index 83990fa366..0000000000 --- a/pkg/webhook/daemonset/validating/daemonset_validating_handler_test.go +++ /dev/null @@ -1,102 +0,0 @@ -/* -Copyright 2020 The Kruise Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package validating - -import ( - "context" - "reflect" - "testing" - - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/intstr" -) - -func newDaemonset(name string) *appsv1alpha1.DaemonSet { - ds := &appsv1alpha1.DaemonSet{} - ds.Name = name - ds.Namespace = metav1.NamespaceDefault - return ds -} - -func TestValidateDaemonSet(t *testing.T) { - handler := DaemonSetCreateUpdateHandler{} - - for _, c := range []struct { - Title string - Ds *appsv1alpha1.DaemonSet - ExpectAllowResult bool - }{ - { - "selector not match", - func() *appsv1alpha1.DaemonSet { - ds := newDaemonset("ds1") - ds.Spec.Selector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key1": "value1", - }, - } - ds.Spec.Template = corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "key1": "value2", - }, - }, - Spec: corev1.PodSpec{}, - } - return ds - }(), - false, - }, - { - "selector match", - func() *appsv1alpha1.DaemonSet { - maxUnavailable := intstr.FromInt(1) - ds := newDaemonset("ds1") - ds.Spec.Selector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key1": "value1", - }, - } - ds.Spec.Template = corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "key1": "value1", - }, - }, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "a", Image: "b"}}}, - } - ds.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyAlways - ds.Spec.UpdateStrategy = appsv1alpha1.DaemonSetUpdateStrategy{ - Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, - RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ - Type: appsv1alpha1.StandardRollingUpdateType, - MaxUnavailable: &maxUnavailable, - }, - } - return ds - }(), - true, - }, - } { - result, _, err := handler.validatingDaemonSetFn(context.TODO(), c.Ds) - if !reflect.DeepEqual(c.ExpectAllowResult, result) { - t.Fatalf("case: %s, expected result: %v, got: %v, error: %v", c.Title, c.ExpectAllowResult, result, err) - } - } -} diff --git a/pkg/webhook/daemonset/validating/daemonset_validating_handler.go b/pkg/webhook/daemonset/validating/daemonset_validation.go similarity index 78% rename from pkg/webhook/daemonset/validating/daemonset_validating_handler.go rename to pkg/webhook/daemonset/validating/daemonset_validation.go index e659178494..d78ab79a4c 100644 --- a/pkg/webhook/daemonset/validating/daemonset_validating_handler.go +++ b/pkg/webhook/daemonset/validating/daemonset_validation.go @@ -1,30 +1,10 @@ -/* -Copyright 2020 The Kruise Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - package validating import ( "context" "fmt" - "net/http" "strconv" - appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" - webhookutil "github.com/openkruise/kruise/pkg/webhook/util" - "github.com/openkruise/kruise/pkg/webhook/util/convertor" corev1 "k8s.io/api/core/v1" genericvalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,24 +13,15 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/klog/v2" appsvalidation "k8s.io/kubernetes/pkg/apis/apps/validation" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -// ValidateDaemonSetName can be used to check whether the given daemon set name is valid. -// Prefix indicates this name will be used as part of generation, in which case -// trailing dashes are allowed. -var ValidateDaemonSetName = genericvalidation.NameIsDNSSubdomain -// DaemonSetCreateUpdateHandler handles DaemonSet -type DaemonSetCreateUpdateHandler struct { - // Decoder decodes objects - Decoder *admission.Decoder -} + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + webhookutil "github.com/openkruise/kruise/pkg/webhook/util" + "github.com/openkruise/kruise/pkg/webhook/util/convertor" +) -func (h *DaemonSetCreateUpdateHandler) validatingDaemonSetFn(ctx context.Context, obj *appsv1alpha1.DaemonSet) (bool, string, error) { +func validatingDaemonSetFn(ctx context.Context, obj *appsv1alpha1.DaemonSet) (bool, string, error) { allErrs := validateDaemonSet(obj) if len(allErrs) != 0 { return false, "", allErrs.ToAggregate() @@ -195,29 +166,3 @@ func getPercentValue(intOrStringValue intstr.IntOrString) (int, bool) { value, _ := strconv.Atoi(intOrStringValue.StrVal[:len(intOrStringValue.StrVal)-1]) return value, true } - -var _ admission.Handler = &DaemonSetCreateUpdateHandler{} - -// Handle handles admission requests. -func (h *DaemonSetCreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response { - obj := &appsv1alpha1.DaemonSet{} - - err := h.Decoder.Decode(req, obj) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - allowed, reason, err := h.validatingDaemonSetFn(ctx, obj) - if err != nil { - klog.Warningf("ds %s/%s action %v fail:%s", obj.Namespace, obj.Name, req.AdmissionRequest.Operation, err.Error()) - return admission.Errored(http.StatusInternalServerError, err) - } - return admission.ValidationResponse(allowed, reason) -} - -var _ admission.DecoderInjector = &DaemonSetCreateUpdateHandler{} - -// InjectDecoder injects the decoder into the DaemonSetCreateUpdateHandler -func (h *DaemonSetCreateUpdateHandler) InjectDecoder(d *admission.Decoder) error { - h.Decoder = d - return nil -} diff --git a/pkg/webhook/daemonset/validating/daemonset_validation_test.go b/pkg/webhook/daemonset/validating/daemonset_validation_test.go new file mode 100644 index 0000000000..63d79fed06 --- /dev/null +++ b/pkg/webhook/daemonset/validating/daemonset_validation_test.go @@ -0,0 +1,242 @@ +/* +Copyright 2020 The Kruise Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validating + +import ( + "context" + "fmt" + "reflect" + "strconv" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/uuid" + + appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" +) + +func newDaemonset(name string) *appsv1alpha1.DaemonSet { + ds := &appsv1alpha1.DaemonSet{} + ds.Name = name + ds.Namespace = metav1.NamespaceDefault + return ds +} + +func TestValidateDaemonSet(t *testing.T) { + + for _, c := range []struct { + Title string + Ds *appsv1alpha1.DaemonSet + ExpectAllowResult bool + }{ + { + "selector not match", + func() *appsv1alpha1.DaemonSet { + ds := newDaemonset("ds1") + ds.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key1": "value1", + }, + } + ds.Spec.Template = corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "key1": "value2", + }, + }, + Spec: corev1.PodSpec{}, + } + return ds + }(), + false, + }, + { + "selector match", + func() *appsv1alpha1.DaemonSet { + maxUnavailable := intstr.FromInt(1) + ds := newDaemonset("ds1") + ds.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key1": "value1", + }, + } + ds.Spec.Template = corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "key1": "value1", + }, + }, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "a", Image: "b"}}}, + } + ds.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyAlways + ds.Spec.UpdateStrategy = appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + Type: appsv1alpha1.StandardRollingUpdateType, + MaxUnavailable: &maxUnavailable, + }, + } + return ds + }(), + true, + }, + } { + result, _, err := validatingDaemonSetFn(context.TODO(), c.Ds) + if !reflect.DeepEqual(c.ExpectAllowResult, result) { + t.Fatalf("case: %s, expected result: %v, got: %v, error: %v", c.Title, c.ExpectAllowResult, result, err) + } + } +} + +type testCase struct { + spec *appsv1alpha1.DaemonSetSpec + oldSpec *appsv1alpha1.DaemonSetSpec +} + +func TestValidateDaemonSetUpdate(t *testing.T) { + handler := DaemonSetCreateUpdateHandler{} + validLabels := map[string]string{"a": "b"} + validPodTemplate := corev1.PodTemplate{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: validLabels, + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyAlways, + DNSPolicy: corev1.DNSClusterFirst, + Containers: []corev1.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: corev1.TerminationMessageReadFile}}, + }, + }, + } + intOrStr1 := intstr.FromInt(1) + intOrStr2 := intstr.FromInt(2) + successCases := []testCase{ + { + spec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + BurstReplicas: &intOrStr1, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + }, + }, + }, + oldSpec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + BurstReplicas: &intOrStr2, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + }, + }, + }, + }, + { + spec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + BurstReplicas: &intOrStr1, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + }, + }, + }, + oldSpec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + BurstReplicas: &intOrStr2, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + }, + }, + }, + }, + } + uid := uuid.NewUUID() + + for i, successCase := range successCases { + obj := &appsv1alpha1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("ds-%d", i), Namespace: metav1.NamespaceDefault, UID: uid, ResourceVersion: "2"}, + Spec: *successCase.spec, + } + oldObj := &appsv1alpha1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("ds-%d", i), Namespace: metav1.NamespaceDefault, UID: uid, ResourceVersion: "1"}, + Spec: *successCase.oldSpec, + } + t.Run("success case "+strconv.Itoa(i), func(t *testing.T) { + if errs := handler.validateDaemonSetUpdate(obj, oldObj); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + }) + } + + validLabels2 := map[string]string{"c": "d"} + + errorCases := []testCase{ + { + spec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels2}, + BurstReplicas: &intOrStr1, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + MaxSurge: &intOrStr2, + }, + }, + }, + oldSpec: &appsv1alpha1.DaemonSetSpec{ + Template: validPodTemplate.Template, + Selector: &metav1.LabelSelector{MatchLabels: validLabels}, + BurstReplicas: &intOrStr2, + UpdateStrategy: appsv1alpha1.DaemonSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateDaemonSetStrategyType, + RollingUpdate: &appsv1alpha1.RollingUpdateDaemonSet{ + MaxUnavailable: &intOrStr1, + MaxSurge: &intOrStr2, + }, + }, + }, + }, + } + for i, successCase := range errorCases { + obj := &appsv1alpha1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("ds-%d", i), Namespace: metav1.NamespaceDefault, UID: uid, ResourceVersion: "2"}, + Spec: *successCase.spec, + } + oldObj := &appsv1alpha1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("ds-%d", i), Namespace: metav1.NamespaceDefault, UID: uid, ResourceVersion: "1"}, + Spec: *successCase.oldSpec, + } + t.Run("error case "+strconv.Itoa(i), func(t *testing.T) { + if errs := handler.validateDaemonSetUpdate(obj, oldObj); len(errs) == 0 { + t.Errorf("expected fail: %v", errs) + } + }) + } +}