From 040b856ab244b001746bfe1e5962d6eba37868b5 Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> Date: Tue, 14 Jan 2025 16:41:27 +0300 Subject: [PATCH 1/3] add validation Signed-off-by: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> --- .../pkg/controller/cvi/cvi_webhook.go | 21 +- .../k8s-validation/validate-k8s-utils.go | 337 ++++++++++++++++++ .../vd/internal/validator/name_validator.go | 51 +++ .../pkg/controller/vd/vd_webhook.go | 1 + .../pkg/controller/vi/vi_webhook.go | 14 +- .../internal/validators/affinity_validator.go | 58 +++ .../validators/affinity_validator_test.go | 208 +++++++++++ .../validators/topology_spread_validator.go | 61 ++++ .../vm/internal/validators/validators_test.go | 29 ++ .../pkg/controller/vm/vm_webhook.go | 2 + 10 files changed, 778 insertions(+), 4 deletions(-) create mode 100644 images/virtualization-artifact/pkg/controller/k8s-validation/validate-k8s-utils.go create mode 100644 images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator_test.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/validators_test.go diff --git a/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go b/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go index a7ecf4a983..d187ecb03d 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go +++ b/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go @@ -18,7 +18,9 @@ package cvi import ( "context" + "errors" "fmt" + "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -41,8 +43,15 @@ func NewValidator(logger *log.Logger) *Validator { } func (v *Validator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { - err := fmt.Errorf("misconfigured webhook rules: create operation not implemented") - v.logger.Error("Ensure the correctness of ValidatingWebhookConfiguration", "err", err) + cvi, ok := obj.(*virtv2.ClusterVirtualImage) + if !ok { + return nil, fmt.Errorf("expected a new ClusterVirtualImage but got a %T", obj) + } + + if strings.Contains(cvi.ObjectMeta.Name, ".") { + return nil, errors.New("ClusterVirtualImage name is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]") + } + return nil, nil } @@ -59,6 +68,8 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj v.logger.Info("Validating ClusterVirtualImage") + var warnings admission.Warnings + if oldCVI.Generation == newCVI.Generation { return nil, nil } @@ -68,7 +79,11 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj return nil, fmt.Errorf("ClusterVirtualImage is in a Ready state: configuration changes are not available") } - return nil, nil + if strings.Contains(newCVI.ObjectMeta.Name, ".") { + warnings = append(warnings, "ClusterVirtualImage name is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another image with valid name to avoid problems with future updates.") + } + + return warnings, nil } func (v *Validator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { diff --git a/images/virtualization-artifact/pkg/controller/k8s-validation/validate-k8s-utils.go b/images/virtualization-artifact/pkg/controller/k8s-validation/validate-k8s-utils.go new file mode 100644 index 0000000000..40eddbbc99 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/k8s-validation/validate-k8s-utils.go @@ -0,0 +1,337 @@ +/* +Copyright 2014 The Kubernetes Authors. +Copyright 2024 Flant JSC + +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. +*/ + +/* + +The code here is taken from https://github.com/kubernetes/kubernetes/blob/29fb8e8b5a41b2a7d760190284bae7f2829312d3/pkg/apis/core/validation/validation.go#L3288 +The "core" package is change to exported package "k8s.io/api/core/v1" in +order to avoid dependency on kubernetes/kubernetes + +https://github.com/kubernetes/kubernetes/blame/29fb8e8b5a41b2a7d760190284bae7f2829312d3/pkg/apis/core/validation/validation.go#L3288 +The code is very stable, it was barely changed during the past years. +It makes it easier to copy and maintain instead of vendoring the whole kubernetes as module or +creating dry runs of the pod object during admission validation. +*/ + +package k8s_validation + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" + apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + unversionedvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +const isNotPositiveErrorMsg string = `must be greater than zero` + +// ValidateNamespaceName can be used to check whether the given namespace name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +var ValidateNamespaceName = apimachineryvalidation.ValidateNamespaceName + +// ValidateNodeName can be used to check whether the given node name is valid. +// Prefix indicates this name will be used as part of generation, in which case +// trailing dashes are allowed. +var ValidateNodeName = apimachineryvalidation.NameIsDNSSubdomain + +var nodeFieldSelectorValidators = map[string]func(string, bool) []string{ + metav1.ObjectNameField: ValidateNodeName, +} + +// ValidateAffinity checks if given affinities are valid +func ValidateAffinity(affinity *v1alpha2.VMAffinity, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if affinity != nil { + if affinity.NodeAffinity != nil { + allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, fldPath.Child("nodeAffinity"))...) + } + if affinity.VirtualMachineAndPodAffinity != nil { + allErrs = append(allErrs, validatePodAffinity(affinity.VirtualMachineAndPodAffinity, fldPath.Child("virtualMachineAndPodAffinity"))...) + } + if affinity.VirtualMachineAndPodAntiAffinity != nil { + allErrs = append(allErrs, validatePodAntiAffinity(affinity.VirtualMachineAndPodAntiAffinity, fldPath.Child("virtualMachineAndPodAntiAffinity"))...) + } + } + + return allErrs +} + +// validateNodeAffinity tests that the specified nodeAffinity fields have valid data +func validateNodeAffinity(na *corev1.NodeAffinity, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + // TODO: Uncomment the next three lines once RequiredDuringSchedulingRequiredDuringExecution is implemented. + // if na.RequiredDuringSchedulingRequiredDuringExecution != nil { + // allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingRequiredDuringExecution, fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) + // } + if na.RequiredDuringSchedulingIgnoredDuringExecution != nil { + allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) + } + if len(na.PreferredDuringSchedulingIgnoredDuringExecution) > 0 { + allErrs = append(allErrs, ValidatePreferredSchedulingTerms(na.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) + } + return allErrs +} + +// ValidatePreferredSchedulingTerms tests that the specified SoftNodeAffinity fields has valid data +func ValidatePreferredSchedulingTerms(terms []corev1.PreferredSchedulingTerm, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + for i, term := range terms { + if term.Weight <= 0 || term.Weight > 100 { + allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("weight"), term.Weight, "must be in the range 1-100")) + } + + allErrs = append(allErrs, ValidateNodeSelectorTerm(term.Preference, fldPath.Index(i).Child("preference"))...) + } + return allErrs +} + +// validatePodAffinity tests that the specified podAffinity fields have valid data +func validatePodAffinity(virtualMachineAndPodAffinity *v1alpha2.VirtualMachineAndPodAffinity, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + // TODO:Uncomment below code once RequiredDuringSchedulingRequiredDuringExecution is implemented. + // if podAffinity.RequiredDuringSchedulingRequiredDuringExecution != nil { + // allErrs = append(allErrs, validatePodAffinityTerms(podAffinity.RequiredDuringSchedulingRequiredDuringExecution, false, + // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) + //} + if virtualMachineAndPodAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { + allErrs = append(allErrs, validatePodAffinityTerms(virtualMachineAndPodAffinity.RequiredDuringSchedulingIgnoredDuringExecution, + fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) + } + if virtualMachineAndPodAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { + allErrs = append(allErrs, validateWeightedPodAffinityTerms(virtualMachineAndPodAffinity.PreferredDuringSchedulingIgnoredDuringExecution, + fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) + } + return allErrs +} + +// validateWeightedPodAffinityTerms tests that the specified weightedPodAffinityTerms fields have valid data +func validateWeightedPodAffinityTerms(weightedPodAffinityTerms []v1alpha2.WeightedVirtualMachineAndPodAffinityTerm, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + for j, weightedTerm := range weightedPodAffinityTerms { + if weightedTerm.Weight <= 0 || weightedTerm.Weight > 100 { + allErrs = append(allErrs, field.Invalid(fldPath.Index(j).Child("weight"), weightedTerm.Weight, "must be in the range 1-100")) + } + allErrs = append(allErrs, validatePodAffinityTerm(weightedTerm.VirtualMachineAndPodAffinityTerm, fldPath.Index(j).Child("virtualMachineAndPodAffinityTerm"))...) + } + return allErrs +} + +// validatePodAffinityTerm tests that the specified podAffinityTerm fields have valid data +func validatePodAffinityTerm(podAffinityTerm v1alpha2.VirtualMachineAndPodAffinityTerm, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.LabelSelector, unversionedvalidation.LabelSelectorValidationOptions{}, fldPath.Child("labelSelector"))...) + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.NamespaceSelector, unversionedvalidation.LabelSelectorValidationOptions{}, fldPath.Child("namespaceSelector"))...) + + for _, name := range podAffinityTerm.Namespaces { + for _, msg := range ValidateNamespaceName(name, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), name, msg)) + } + } + if len(podAffinityTerm.TopologyKey) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("topologyKey"), "can not be empty")) + } + return append(allErrs, unversionedvalidation.ValidateLabelName(podAffinityTerm.TopologyKey, fldPath.Child("topologyKey"))...) +} + +// validatePodAntiAffinity tests that the specified podAntiAffinity fields have valid data +func validatePodAntiAffinity(podAntiAffinity *v1alpha2.VirtualMachineAndPodAntiAffinity, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + // TODO:Uncomment below code once RequiredDuringSchedulingRequiredDuringExecution is implemented. + // if podAntiAffinity.RequiredDuringSchedulingRequiredDuringExecution != nil { + // allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingRequiredDuringExecution, false, + // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) + //} + if podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { + allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution, + fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) + } + if podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { + allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, + fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) + } + return allErrs +} + +// ValidateNodeSelector tests that the specified nodeSelector fields has valid data +func ValidateNodeSelector(nodeSelector *corev1.NodeSelector, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + termFldPath := fldPath.Child("nodeSelectorTerms") + if len(nodeSelector.NodeSelectorTerms) == 0 { + return append(allErrs, field.Required(termFldPath, "must have at least one node selector term")) + } + + for i, term := range nodeSelector.NodeSelectorTerms { + allErrs = append(allErrs, ValidateNodeSelectorTerm(term, termFldPath.Index(i))...) + } + + return allErrs +} + +// validatePodAffinityTerms tests that the specified podAffinityTerms fields have valid data +func validatePodAffinityTerms(podAffinityTerms []v1alpha2.VirtualMachineAndPodAffinityTerm, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + for i, podAffinityTerm := range podAffinityTerms { + allErrs = append(allErrs, validatePodAffinityTerm(podAffinityTerm, fldPath.Index(i))...) + } + return allErrs +} + +// ValidateNodeSelectorTerm tests that the specified node selector term has valid data +func ValidateNodeSelectorTerm(term corev1.NodeSelectorTerm, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + for j, req := range term.MatchExpressions { + allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchExpressions").Index(j))...) + } + + for j, req := range term.MatchFields { + allErrs = append(allErrs, ValidateNodeFieldSelectorRequirement(req, fldPath.Child("matchFields").Index(j))...) + } + + return allErrs +} + +// ValidateNodeSelectorRequirement tests that the specified NodeSelectorRequirement fields has valid data +func ValidateNodeSelectorRequirement(rq corev1.NodeSelectorRequirement, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + switch rq.Operator { + case corev1.NodeSelectorOpIn, corev1.NodeSelectorOpNotIn: + if len(rq.Values) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("values"), "must be specified when `operator` is 'In' or 'NotIn'")) + } + case corev1.NodeSelectorOpExists, corev1.NodeSelectorOpDoesNotExist: + if len(rq.Values) > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("values"), "may not be specified when `operator` is 'Exists' or 'DoesNotExist'")) + } + + case corev1.NodeSelectorOpGt, corev1.NodeSelectorOpLt: + if len(rq.Values) != 1 { + allErrs = append(allErrs, field.Required(fldPath.Child("values"), "must be specified single value when `operator` is 'Lt' or 'Gt'")) + } + default: + allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), rq.Operator, "not a valid selector operator")) + } + + allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(rq.Key, fldPath.Child("key"))...) + + return allErrs +} + +// ValidateNodeFieldSelectorRequirement tests that the specified NodeSelectorRequirement fields has valid data +func ValidateNodeFieldSelectorRequirement(req corev1.NodeSelectorRequirement, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + switch req.Operator { + case corev1.NodeSelectorOpIn, corev1.NodeSelectorOpNotIn: + if len(req.Values) != 1 { + allErrs = append(allErrs, field.Required(fldPath.Child("values"), + "must be only one value when `operator` is 'In' or 'NotIn' for node field selector")) + } + default: + allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), req.Operator, "not a valid selector operator")) + } + + if vf, found := nodeFieldSelectorValidators[req.Key]; !found { + allErrs = append(allErrs, field.Invalid(fldPath.Child("key"), req.Key, "not a valid field selector key")) + } else { + for i, v := range req.Values { + for _, msg := range vf(v, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("values").Index(i), v, msg)) + } + } + } + + return allErrs +} + +var supportedScheduleActions = sets.NewString(string(corev1.DoNotSchedule), string(corev1.ScheduleAnyway)) + +// ValidateTopologySpreadConstraints validates given TopologySpreadConstraints. +func ValidateTopologySpreadConstraints(constraints []corev1.TopologySpreadConstraint, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + for i, constraint := range constraints { + subFldPath := fldPath.Index(i) + if err := ValidateMaxSkew(subFldPath.Child("maxSkew"), constraint.MaxSkew); err != nil { + allErrs = append(allErrs, err) + } + if errs := ValidateTopologyKey(subFldPath.Child("topologyKey"), constraint.TopologyKey); errs != nil { + allErrs = append(allErrs, errs...) + } + if err := ValidateWhenUnsatisfiable(subFldPath.Child("whenUnsatisfiable"), constraint.WhenUnsatisfiable); err != nil { + allErrs = append(allErrs, err) + } + + // this is missing in upstream codebase https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L6571-L6600 + // issue captured here https://github.com/kubernetes/kubernetes/issues/111791#issuecomment-1211184962 + allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(constraint.LabelSelector, unversionedvalidation.LabelSelectorValidationOptions{}, fldPath.Child("labelSelector"))...) + + // tuple {topologyKey, whenUnsatisfiable} denotes one kind of spread constraint + if err := ValidateSpreadConstraintNotRepeat(subFldPath.Child("{topologyKey, whenUnsatisfiable}"), constraint, constraints[i+1:]); err != nil { + allErrs = append(allErrs, err) + } + } + + return allErrs +} + +// ValidateMaxSkew tests that the argument is a valid MaxSkew. +func ValidateMaxSkew(fldPath *field.Path, maxSkew int32) *field.Error { + if maxSkew <= 0 { + return field.Invalid(fldPath, maxSkew, isNotPositiveErrorMsg) + } + return nil +} + +// ValidateTopologyKey tests that the argument is a valid TopologyKey. +func ValidateTopologyKey(fldPath *field.Path, topologyKey string) field.ErrorList { + allErrs := field.ErrorList{} + if len(topologyKey) == 0 { + return append(allErrs, field.Required(fldPath, "can not be empty")) + } + return unversionedvalidation.ValidateLabelName(topologyKey, fldPath) +} + +// ValidateWhenUnsatisfiable tests that the argument is a valid UnsatisfiableConstraintAction. +func ValidateWhenUnsatisfiable(fldPath *field.Path, action corev1.UnsatisfiableConstraintAction) *field.Error { + if !supportedScheduleActions.Has(string(action)) { + return field.NotSupported(fldPath, action, supportedScheduleActions.List()) + } + return nil +} + +// ValidateSpreadConstraintNotRepeat tests that if `constraint` duplicates with `existingConstraintPairs` +// on TopologyKey and WhenUnsatisfiable fields. +func ValidateSpreadConstraintNotRepeat(fldPath *field.Path, constraint corev1.TopologySpreadConstraint, restingConstraints []corev1.TopologySpreadConstraint) *field.Error { + for _, restingConstraint := range restingConstraints { + if constraint.TopologyKey == restingConstraint.TopologyKey && + constraint.WhenUnsatisfiable == restingConstraint.WhenUnsatisfiable { + return field.Duplicate(fldPath, fmt.Sprintf("{%v, %v}", constraint.TopologyKey, constraint.WhenUnsatisfiable)) + } + } + return nil +} diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go new file mode 100644 index 0000000000..8a2fbd5e49 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go @@ -0,0 +1,51 @@ +/* +Copyright 2024 Flant JSC + +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 validator + +import ( + "context" + "errors" + "strings" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type NameValidator struct{} + +func NewNameValidator() *NameValidator { + return &NameValidator{} +} + +func (v *NameValidator) ValidateCreate(_ context.Context, vd *virtv2.VirtualDisk) (admission.Warnings, error) { + if strings.Contains(vd.ObjectMeta.Name, ".") { + return nil, errors.New("virtual disk name cannot contain '.'") + } + + return nil, nil +} + +func (v *NameValidator) ValidateUpdate(_ context.Context, _, newVD *virtv2.VirtualDisk) (admission.Warnings, error) { + if strings.Contains(newVD.ObjectMeta.Name, ".") { + var warnings admission.Warnings + warnings = append(warnings, "virtual disk name contain '.', it may be cause of problems in future, please recreate resource.") + return warnings, nil + } + + return nil, nil +} diff --git a/images/virtualization-artifact/pkg/controller/vd/vd_webhook.go b/images/virtualization-artifact/pkg/controller/vd/vd_webhook.go index 94cacb0391..7e668f2b8e 100644 --- a/images/virtualization-artifact/pkg/controller/vd/vd_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vd/vd_webhook.go @@ -44,6 +44,7 @@ func NewValidator(client client.Client) *Validator { validator.NewPVCSizeValidator(client), validator.NewSpecChangesValidator(), validator.NewISOSourceValidator(client), + validator.NewNameValidator(), }, } } diff --git a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go index 322586e917..829f37adb5 100644 --- a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go @@ -18,8 +18,10 @@ package vi import ( "context" + "errors" "fmt" "reflect" + "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -47,6 +49,10 @@ func (v *Validator) ValidateCreate(_ context.Context, obj runtime.Object) (admis return nil, fmt.Errorf("expected a new VirtualMachine but got a %T", obj) } + if strings.Contains(vi.ObjectMeta.Name, ".") { + return nil, errors.New("virtual image name cannot contain '.'") + } + if vi.Spec.Storage == virtv2.StorageKubernetes { warnings := admission.Warnings{ fmt.Sprintf("Using the `%s` storage type is deprecated. It is recommended to use `%s` instead.", @@ -71,6 +77,8 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj v.logger.Info("Validating VirtualImage") + var warnings admission.Warnings + if oldVI.Generation == newVI.Generation { return nil, nil } @@ -86,7 +94,11 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj } } - return nil, nil + if strings.Contains(newVI.ObjectMeta.Name, ".") { + warnings = append(warnings, "virtual image name contain '.', it may be cause of problems in future, please recreate resource.") + } + + return warnings, nil } func (v *Validator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go new file mode 100644 index 0000000000..dbb9b16adf --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go @@ -0,0 +1,58 @@ +/* +Copyright 2024 Flant JSC + +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 validators + +import ( + "context" + "errors" + "fmt" + + k8sfield "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + k8sUtils "github.com/deckhouse/virtualization-controller/pkg/controller/k8s-validation" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type AffinityValidator struct{} + +func NewAffinityValidator() *AffinityValidator { + return &AffinityValidator{} +} + +func (v *AffinityValidator) ValidateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return v.Validate(vm) +} + +func (v *AffinityValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return v.Validate(newVM) +} + +func (v *AffinityValidator) Validate(vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + var errs []error + + errorList := k8sUtils.ValidateAffinity(vm.Spec.Affinity, k8sfield.NewPath("spec")) + for _, err := range errorList { + errs = append(errs, err) + } + + if len(errs) > 0 { + return nil, fmt.Errorf("errors while validating affinity: %w", errors.Join(errs...)) + } + + return nil, nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator_test.go new file mode 100644 index 0000000000..1fb1e6fcf4 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator_test.go @@ -0,0 +1,208 @@ +/* +Copyright 2024 Flant JSC + +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 validators_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + + "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/validators" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +var _ = Describe("AffinityValidator", func() { + validator := validators.NewAffinityValidator() + + Context("VM with no affinities", func() { + vm := &v1alpha2.VirtualMachine{} + + It("Should be no error", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).Should(BeNil()) + }) + }) + + Context("VM with node affinity with nodeselectorterms empty requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{}, + }, + }, + }, + }, + } + + It("Should return error", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).Should(HaveOccurred()) + }) + }) + + Context("VM with node affinity with correct nodeselectorterms requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "key", + Operator: corev1.NodeSelectorOpExists, + }, + }, + }, + }, + }, + }, + }, + }, + } + + It("Should pass validation", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).Should(BeNil()) + }) + }) + + Context("VM with node affinity with incorrect nodeselectorterms requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "key", + Operator: corev1.NodeSelectorOpIn, + }, + }, + }, + }, + }, + }, + }, + }, + } + + It("Should not pass validation", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).ShouldNot(BeNil()) + }) + }) + + Context("VM with node affinity with correct nodeselectorterms requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 50, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "key", + Operator: corev1.NodeSelectorOpExists, + }, + }, + }, + }, + }, + }, + }, + }, + } + + It("Should pass validation", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).Should(BeNil()) + }) + }) + + Context("VM with node affinity with incorrect nodeselectorterms requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 50, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "key", + Operator: corev1.NodeSelectorOpIn, + }, + }, + }, + }, + }, + }, + }, + }, + } + + It("Should not pass validation", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).ShouldNot(BeNil()) + }) + }) + + Context("VM with node affinity with incorrect nodeselectorterms requirement", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + Affinity: &v1alpha2.VMAffinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: -1, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "key", + Operator: corev1.NodeSelectorOpExists, + }, + }, + }, + }, + }, + }, + }, + }, + } + + It("Should not pass validation", func() { + warnings, err := validator.Validate(vm) + Expect(warnings).Should(BeEmpty()) + Expect(err).ShouldNot(BeNil()) + }) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go new file mode 100644 index 0000000000..92aed88f3f --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go @@ -0,0 +1,61 @@ +/* +Copyright 2024 Flant JSC + +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 validators + +import ( + "context" + "errors" + "fmt" + + k8sfield "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + k8sUtils "github.com/deckhouse/virtualization-controller/pkg/controller/k8s-validation" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type TopologySpreadConstraintValidator struct{} + +func NewTopologySpreadConstraintValidator() *TopologySpreadConstraintValidator { + return &TopologySpreadConstraintValidator{} +} + +func (v *TopologySpreadConstraintValidator) ValidateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return v.Validate(vm) +} + +func (v *TopologySpreadConstraintValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return v.Validate(newVM) +} + +func (v *TopologySpreadConstraintValidator) Validate(vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + var errs []error + + errorList := k8sUtils.ValidateTopologySpreadConstraints( + vm.Spec.TopologySpreadConstraints, + k8sfield.NewPath("spec").Child("topologySpreadConstraints"), + ) + for _, err := range errorList { + errs = append(errs, err) + } + + if len(errs) > 0 { + return nil, fmt.Errorf("errors while validating topology spread constraints: %w", errors.Join(errs...)) + } + + return nil, nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/validators_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/validators_test.go new file mode 100644 index 0000000000..965a734a3f --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/validators_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2024 Flant JSC + +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 validators_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestValidators(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Validators") +} diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go index 5fc52ced2b..3bea1142be 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go @@ -49,6 +49,8 @@ func NewValidator(ipam internal.IPAM, client client.Client, service *service.Blo validators.NewBlockDeviceSpecRefsValidator(), validators.NewSizingPolicyValidator(client), validators.NewBlockDeviceLimiterValidator(service, log), + validators.NewAffinityValidator(), + validators.NewTopologySpreadConstraintValidator(), }, log: log.With("webhook", "validation"), } From c5ad115f3c7d6b1d5dcf6b46f48432e9464715a6 Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> Date: Mon, 27 Jan 2025 15:11:34 +0300 Subject: [PATCH 2/3] refactoring Signed-off-by: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> --- .../pkg/controller/vd/internal/validator/name_validator.go | 4 ++-- .../virtualization-artifact/pkg/controller/vi/vi_webhook.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go index 8a2fbd5e49..26c05a4fda 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go @@ -34,7 +34,7 @@ func NewNameValidator() *NameValidator { func (v *NameValidator) ValidateCreate(_ context.Context, vd *virtv2.VirtualDisk) (admission.Warnings, error) { if strings.Contains(vd.ObjectMeta.Name, ".") { - return nil, errors.New("virtual disk name cannot contain '.'") + return nil, errors.New("VirtualDisk name is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]") } return nil, nil @@ -43,7 +43,7 @@ func (v *NameValidator) ValidateCreate(_ context.Context, vd *virtv2.VirtualDisk func (v *NameValidator) ValidateUpdate(_ context.Context, _, newVD *virtv2.VirtualDisk) (admission.Warnings, error) { if strings.Contains(newVD.ObjectMeta.Name, ".") { var warnings admission.Warnings - warnings = append(warnings, "virtual disk name contain '.', it may be cause of problems in future, please recreate resource.") + warnings = append(warnings, "VirtualDisk name is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another disk with valid name to avoid problems with future updates.") return warnings, nil } diff --git a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go index 829f37adb5..75c9a93fd2 100644 --- a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go @@ -50,7 +50,7 @@ func (v *Validator) ValidateCreate(_ context.Context, obj runtime.Object) (admis } if strings.Contains(vi.ObjectMeta.Name, ".") { - return nil, errors.New("virtual image name cannot contain '.'") + return nil, errors.New("VirtualImage name is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]") } if vi.Spec.Storage == virtv2.StorageKubernetes { @@ -95,7 +95,7 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj } if strings.Contains(newVI.ObjectMeta.Name, ".") { - warnings = append(warnings, "virtual image name contain '.', it may be cause of problems in future, please recreate resource.") + warnings = append(warnings, "VirtualImage name is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another image with valid name to avoid problems with future updates.") } return warnings, nil From d1fe2e9d3ba5a2291a00cc4b3707d27527111833 Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> Date: Mon, 27 Jan 2025 15:33:40 +0300 Subject: [PATCH 3/3] refactoring Signed-off-by: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com> --- .../vm/internal/validators/affinity_validator.go | 10 ++-------- .../internal/validators/topology_spread_validator.go | 10 ++-------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go index dbb9b16adf..4c9ad99a70 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/affinity_validator.go @@ -18,7 +18,6 @@ package validators import ( "context" - "errors" "fmt" k8sfield "k8s.io/apimachinery/pkg/util/validation/field" @@ -43,15 +42,10 @@ func (v *AffinityValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2 } func (v *AffinityValidator) Validate(vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { - var errs []error - - errorList := k8sUtils.ValidateAffinity(vm.Spec.Affinity, k8sfield.NewPath("spec")) - for _, err := range errorList { - errs = append(errs, err) - } + errs := k8sUtils.ValidateAffinity(vm.Spec.Affinity, k8sfield.NewPath("spec")) if len(errs) > 0 { - return nil, fmt.Errorf("errors while validating affinity: %w", errors.Join(errs...)) + return nil, fmt.Errorf("errors while validating affinity: %w", errs.ToAggregate()) } return nil, nil diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go index 92aed88f3f..8b47632060 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/topology_spread_validator.go @@ -18,7 +18,6 @@ package validators import ( "context" - "errors" "fmt" k8sfield "k8s.io/apimachinery/pkg/util/validation/field" @@ -43,18 +42,13 @@ func (v *TopologySpreadConstraintValidator) ValidateUpdate(_ context.Context, _, } func (v *TopologySpreadConstraintValidator) Validate(vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { - var errs []error - - errorList := k8sUtils.ValidateTopologySpreadConstraints( + errs := k8sUtils.ValidateTopologySpreadConstraints( vm.Spec.TopologySpreadConstraints, k8sfield.NewPath("spec").Child("topologySpreadConstraints"), ) - for _, err := range errorList { - errs = append(errs, err) - } if len(errs) > 0 { - return nil, fmt.Errorf("errors while validating topology spread constraints: %w", errors.Join(errs...)) + return nil, fmt.Errorf("errors while validating topology spread constraints: %w", errs.ToAggregate()) } return nil, nil