Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation for the WorkloadSpreadSubset patch field #1237

Merged
merged 1 commit into from
Jun 6, 2023
+78 −4
Merged

Add validation for the WorkloadSpreadSubset patch field #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/webhook/util/convertor/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ func ConvertPodTemplateSpec(template *v1.PodTemplateSpec) (*core.PodTemplateSpec
return coreTemplate, nil
}

func ConvertPod(pod *v1.Pod) (*core.Pod, error) {
corePod := &core.Pod{}
defaults.SetDefaultPodSpec(&pod.Spec)
if err := corev1.Convert_v1_Pod_To_core_Pod(pod.DeepCopy(), corePod, nil); err != nil {
return nil, err
}
return corePod, nil
}

func ConvertCoreVolumes(volumes []v1.Volume) ([]core.Volume, error) {
coreVolumes := []core.Volume{}
for _, volume := range volumes {
Expand Down
73 changes: 69 additions & 4 deletions pkg/webhook/workloadspread/validating/workloadspread_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,18 @@ package validating

import (
"context"
"encoding/json"
"fmt"
"math"
"time"

"k8s.io/apimachinery/pkg/util/strategicpatch"

webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
"github.com/openkruise/kruise/pkg/webhook/util/convertor"
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
Expand Down Expand Up @@ -75,7 +81,7 @@ func verifyGroupKind(ref *appsv1alpha1.TargetReference, expectedKind string, exp

func (h *WorkloadSpreadCreateUpdateHandler) validatingWorkloadSpreadFn(obj *appsv1alpha1.WorkloadSpread) field.ErrorList {
// validate ws.spec.
allErrs := h.validateWorkloadSpreadSpec(obj, field.NewPath("spec"))
allErrs := validateWorkloadSpreadSpec(h, obj, field.NewPath("spec"))

// validate whether ws.spec.targetRef is in conflict with others.
wsList := &appsv1alpha1.WorkloadSpreadList{}
Expand All @@ -88,9 +94,10 @@ func (h *WorkloadSpreadCreateUpdateHandler) validatingWorkloadSpreadFn(obj *apps
return allErrs
}

func (h *WorkloadSpreadCreateUpdateHandler) validateWorkloadSpreadSpec(obj *appsv1alpha1.WorkloadSpread, fldPath *field.Path) field.ErrorList {
func validateWorkloadSpreadSpec(h *WorkloadSpreadCreateUpdateHandler, obj *appsv1alpha1.WorkloadSpread, fldPath *field.Path) field.ErrorList {
spec := &obj.Spec
allErrs := field.ErrorList{}
var workloadTemplate client.Object

// validate targetRef
if spec.TargetReference == nil {
Expand All @@ -104,26 +111,51 @@ func (h *WorkloadSpreadCreateUpdateHandler) validateWorkloadSpreadSpec(obj *apps
ok, err := verifyGroupKind(spec.TargetReference, controllerKruiseKindCS.Kind, []string{controllerKruiseKindCS.Group})
if !ok || err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("targetRef"), spec.TargetReference, "TargetReference is not valid for CloneSet."))
} else {
set := &appsv1alpha1.CloneSet{}
if getErr := h.Client.Get(context.TODO(), client.ObjectKey{Name: spec.TargetReference.Name, Namespace: obj.Namespace}, set); getErr == nil {
workloadTemplate = set
}
}
case controllerKindDep.Kind:
ok, err := verifyGroupKind(spec.TargetReference, controllerKindDep.Kind, []string{controllerKindDep.Group})
if !ok || err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("targetRef"), spec.TargetReference, "TargetReference is not valid for Deployment."))
} else {
set := &appsv1.Deployment{}
if getErr := h.Client.Get(context.TODO(), client.ObjectKey{Name: spec.TargetReference.Name, Namespace: obj.Namespace}, set); getErr == nil {
workloadTemplate = set
}
}
case controllerKindRS.Kind:
ok, err := verifyGroupKind(spec.TargetReference, controllerKindRS.Kind, []string{controllerKindRS.Group})
if !ok || err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("targetRef"), spec.TargetReference, "TargetReference is not valid for ReplicaSet."))
} else {
set := &appsv1.ReplicaSet{}
if getErr := h.Client.Get(context.TODO(), client.ObjectKey{Name: spec.TargetReference.Name, Namespace: obj.Namespace}, set); getErr == nil {
workloadTemplate = set
}
}
case controllerKindJob.Kind:
ok, err := verifyGroupKind(spec.TargetReference, controllerKindJob.Kind, []string{controllerKindJob.Group})
if !ok || err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("targetRef"), spec.TargetReference, "TargetReference is not valid for Job."))
} else {
set := &batchv1.Job{}
if getErr := h.Client.Get(context.TODO(), client.ObjectKey{Name: spec.TargetReference.Name, Namespace: obj.Namespace}, set); getErr == nil {
workloadTemplate = set
}
}
case controllerKindSts.Kind:
ok, err := verifyGroupKind(spec.TargetReference, controllerKindSts.Kind, []string{controllerKindSts.Group, controllerKruiseKindAlphaSts.Group, controllerKruiseKindBetaSts.Group})
if !ok || err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("targetRef"), spec.TargetReference, "TargetReference is not valid for StatefulSet."))
} else {
set := &appsv1.StatefulSet{}
if getErr := h.Client.Get(context.TODO(), client.ObjectKey{Name: spec.TargetReference.Name, Namespace: obj.Namespace}, workloadTemplate); getErr == nil {
workloadTemplate = set
furykerry marked this conversation as resolved.
Show resolved Hide resolved
}
}
default:
whiteList, err := configuration.GetWSWatchCustomWorkloadWhiteList(h.Client)
Expand All @@ -145,7 +177,7 @@ func (h *WorkloadSpreadCreateUpdateHandler) validateWorkloadSpreadSpec(obj *apps
}

// validate subsets
allErrs = append(allErrs, validateWorkloadSpreadSubsets(obj, spec.Subsets, fldPath.Child("subsets"))...)
allErrs = append(allErrs, validateWorkloadSpreadSubsets(obj, spec.Subsets, workloadTemplate, fldPath.Child("subsets"))...)

// validate scheduleStrategy
if spec.ScheduleStrategy.Type != "" &&
Expand Down Expand Up @@ -183,7 +215,7 @@ func (h *WorkloadSpreadCreateUpdateHandler) validateWorkloadSpreadSpec(obj *apps
return allErrs
}

func validateWorkloadSpreadSubsets(ws *appsv1alpha1.WorkloadSpread, subsets []appsv1alpha1.WorkloadSpreadSubset, fldPath *field.Path) field.ErrorList {
func validateWorkloadSpreadSubsets(ws *appsv1alpha1.WorkloadSpread, subsets []appsv1alpha1.WorkloadSpreadSubset, workloadTemplate client.Object, fldPath *field.Path) field.ErrorList {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider using podtemplate instead of workloadTemplate as function parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether the podtemplate is o good idea because it's actually a workload such as CloneSet and Deployment.

allErrs := field.ErrorList{}

//if len(subsets) < 2 {
Expand Down Expand Up @@ -254,6 +286,39 @@ func validateWorkloadSpreadSubsets(ws *appsv1alpha1.WorkloadSpread, subsets []ap
}

//TODO validate patch
if subset.Patch.Raw != nil {
// In the case the WorkloadSpread is created before the workload,so no workloadTemplate is obtained, skip the remaining checks.
if workloadTemplate != nil {
// get the PodTemplateSpec from the workload
var podSpec v1.PodTemplateSpec
switch workloadTemplate.GetObjectKind().GroupVersionKind() {
case controllerKruiseKindCS:
podSpec = workloadTemplate.(*appsv1alpha1.CloneSet).Spec.Template
case controllerKindDep:
podSpec = workloadTemplate.(*appsv1.Deployment).Spec.Template
case controllerKindRS:
podSpec = workloadTemplate.(*appsv1.ReplicaSet).Spec.Template
case controllerKindJob:
podSpec = workloadTemplate.(*batchv1.Job).Spec.Template
case controllerKindSts:
podSpec = workloadTemplate.(*appsv1.StatefulSet).Spec.Template
}
podBytes, _ := json.Marshal(podSpec)
modified, err := strategicpatch.StrategicMergePatch(podBytes, subset.Patch.Raw, &v1.Pod{})
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("patch"), string(subset.Patch.Raw), fmt.Sprintf("failed to merge patch: %v", err)))
}
newPod := &v1.Pod{}
if err = json.Unmarshal(modified, newPod); err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("patch"), string(subset.Patch.Raw), fmt.Sprintf("failed to unmarshal: %v", err)))
}
coreNewPod, CovErr := convertor.ConvertPod(newPod)
if CovErr != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("patch"), newPod, fmt.Sprintf("Convert_v1_Pod_To_core_Pod failed: %v", err)))
}
allErrs = append(allErrs, corevalidation.ValidatePodSpec(&coreNewPod.Spec, &coreNewPod.ObjectMeta, fldPath.Index(i).Child("patch"), webhookutil.DefaultPodValidationOptions)...)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about the case that workloadTemplate is nil ? in such case, we cannot use ValidatePodSpec directly, instead, we should check whether the patch had change some fields that should not be changed such as affinity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'll add validation for this case.


//1. All subset maxReplicas must be the same type: int or percent.
//2. Adaptive: the last subset must be not specified.
Expand Down