From f17edf7efb519c7338816f375e7e250dc70a325f Mon Sep 17 00:00:00 2001 From: helayoty Date: Thu, 27 Nov 2025 01:51:17 +0000 Subject: [PATCH] Restructure validation tests to check declarative coverage --- pkg/apis/scheduling/validation/validation.go | 36 ++- .../scheduling/validation/validation_test.go | 305 +++++++++++++----- .../api/scheduling/v1alpha1/generated.proto | 2 +- .../k8s.io/api/scheduling/v1alpha1/types.go | 2 +- 4 files changed, 247 insertions(+), 98 deletions(-) diff --git a/pkg/apis/scheduling/validation/validation.go b/pkg/apis/scheduling/validation/validation.go index b9ecce42b29..5ff3b1d12f7 100644 --- a/pkg/apis/scheduling/validation/validation.go +++ b/pkg/apis/scheduling/validation/validation.go @@ -80,9 +80,9 @@ func validateWorkloadSpec(spec *scheduling.WorkloadSpec, fldPath *field.Path) fi existingPodGroups := sets.New[string]() podGroupsPath := fldPath.Child("podGroups") if len(spec.PodGroups) == 0 { - allErrs = append(allErrs, field.Required(podGroupsPath, "must have at least one item")) + allErrs = append(allErrs, field.Required(podGroupsPath, "must have at least one item").MarkCoveredByDeclarative()) } else if len(spec.PodGroups) > scheduling.WorkloadMaxPodGroups { - allErrs = append(allErrs, field.TooMany(fldPath.Child("podGroups"), len(spec.PodGroups), scheduling.WorkloadMaxPodGroups)) + allErrs = append(allErrs, field.TooMany(podGroupsPath, len(spec.PodGroups), scheduling.WorkloadMaxPodGroups).WithOrigin("maxItems").MarkCoveredByDeclarative()) } else { for i := range spec.PodGroups { allErrs = append(allErrs, validatePodGroup(&spec.PodGroups[i], podGroupsPath.Index(i), existingPodGroups)...) @@ -95,21 +95,21 @@ func validateControllerRef(ref *scheduling.TypedLocalObjectReference, fldPath *f var allErrs = field.ErrorList{} if ref.APIGroup != "" { for _, msg := range validation.IsDNS1123Subdomain(ref.APIGroup) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("apiGroup"), ref.APIGroup, msg)) + allErrs = append(allErrs, field.Invalid(fldPath.Child("apiGroup"), ref.APIGroup, msg).WithOrigin("format=k8s-long-name").MarkCoveredByDeclarative()) } } if ref.Kind == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("kind"), "")) + allErrs = append(allErrs, field.Required(fldPath.Child("kind"), "").MarkCoveredByDeclarative()) } else { for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Kind) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), ref.Kind, msg)) + allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), ref.Kind, msg).WithOrigin("format=k8s-path-segment-name").MarkCoveredByDeclarative()) } } if ref.Name == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) + allErrs = append(allErrs, field.Required(fldPath.Child("name"), "").MarkCoveredByDeclarative()) } else { for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Name) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), ref.Name, msg)) + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), ref.Name, msg).WithOrigin("format=k8s-path-segment-name").MarkCoveredByDeclarative()) } } return allErrs @@ -117,11 +117,19 @@ func validateControllerRef(ref *scheduling.TypedLocalObjectReference, fldPath *f func validatePodGroup(podGroup *scheduling.PodGroup, fldPath *field.Path, existingPodGroups sets.Set[string]) field.ErrorList { var allErrs field.ErrorList - for _, detail := range apivalidation.ValidatePodGroupName(podGroup.Name, false) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), podGroup.Name, detail)) + // To match the declarative validation behavior, we return Required for empty string. + // Declarative validation treats "" as "missing" via validate.RequiredValue() + // and returns early before checking the format constraint. + if podGroup.Name == "" { + allErrs = append(allErrs, field.Required(fldPath.Child("name"), "").MarkCoveredByDeclarative()) + } else { + for _, detail := range apivalidation.ValidatePodGroupName(podGroup.Name, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), podGroup.Name, detail).WithOrigin("format=k8s-short-name").MarkCoveredByDeclarative()) + } } if existingPodGroups.Has(podGroup.Name) { - allErrs = append(allErrs, field.Duplicate(fldPath.Child("name"), podGroup.Name)) + // MarkCoveredByDeclarative is not needed here because the duplicate check is done. + allErrs = append(allErrs, field.Duplicate(fldPath, podGroup).MarkCoveredByDeclarative()) } else { existingPodGroups.Insert(podGroup.Name) } @@ -142,10 +150,10 @@ func validatePodGroupPolicy(policy *scheduling.PodGroupPolicy, fldPath *field.Pa switch { case len(setFields) == 0: - allErrs = append(allErrs, field.Required(fldPath, "exactly one of `basic`, `gang` is required")) + allErrs = append(allErrs, field.Invalid(fldPath, "", "must specify one of: `basic`, `gang`").MarkCoveredByDeclarative()) case len(setFields) > 1: allErrs = append(allErrs, field.Invalid(fldPath, fmt.Sprintf("{%s}", strings.Join(setFields, ", ")), - "exactly one of `basic`, `gang` is required, but multiple fields are set")) + "exactly one of `basic`, `gang` is required, but multiple fields are set").MarkCoveredByDeclarative()) case policy.Basic != nil: allErrs = append(allErrs, validatBasicSchedulingPolicy(policy.Basic, fldPath.Child("basic"))...) case policy.Gang != nil: @@ -186,8 +194,8 @@ func ValidateWorkloadUpdate(workload, oldWorkload *scheduling.Workload) field.Er func validateWorkloadSpecUpdate(spec, oldSpec *scheduling.WorkloadSpec, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList if oldSpec.ControllerRef != nil { - allErrs = apimachineryvalidation.ValidateImmutableField(spec.ControllerRef, oldSpec.ControllerRef, field.NewPath("controllerRef")) + allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(spec.ControllerRef, oldSpec.ControllerRef, fldPath.Child("controllerRef"))...) } - allErrs = append(allErrs, apivalidation.ValidateImmutableField(spec.PodGroups, oldSpec.PodGroups, field.NewPath("podGroups"))...) + allErrs = append(allErrs, apivalidation.ValidateImmutableField(spec.PodGroups, oldSpec.PodGroups, fldPath.Child("podGroups"))...) return allErrs } diff --git a/pkg/apis/scheduling/validation/validation_test.go b/pkg/apis/scheduling/validation/validation_test.go index 8372169b5ae..521fc85972a 100644 --- a/pkg/apis/scheduling/validation/validation_test.go +++ b/pkg/apis/scheduling/validation/validation_test.go @@ -200,89 +200,230 @@ func TestValidateWorkload(t *testing.T) { } } - failureCases := map[string]*scheduling.Workload{ - "no name": mkWorkload(func(w *scheduling.Workload) { - w.Name = "" - }), - "invalid name": mkWorkload(func(w *scheduling.Workload) { - w.Name = ".workload" - }), - "too long name": mkWorkload(func(w *scheduling.Workload) { - w.Name = strings.Repeat("w", 254) - }), - "no namespace": mkWorkload(func(w *scheduling.Workload) { - w.Namespace = "" - }), - "invalid namespace": mkWorkload(func(w *scheduling.Workload) { - w.Namespace = ".ns" - }), - "too long namespace": mkWorkload(func(w *scheduling.Workload) { - w.Namespace = strings.Repeat("n", 64) - }), - "invalid controllerRef apiGroup": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.APIGroup = ".group" - }), - "too long controllerRef apiGroup": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.APIGroup = strings.Repeat("g", 254) - }), - "no controllerRef kind": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.Kind = "" - }), - "invalid controllerRef kind": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.Kind = "/foo" - }), - "no controllerRef name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.Name = "" - }), - "invalid controllerRef name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.ControllerRef.Name = "/baz" - }), - "no pod groups": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups = nil - }), - "too many pod groups": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups = nil - for i := 0; i < scheduling.WorkloadMaxPodGroups+1; i++ { - w.Spec.PodGroups = append(w.Spec.PodGroups, scheduling.PodGroup{ - Name: fmt.Sprintf("group-%v", i), - Policy: scheduling.PodGroupPolicy{ - Basic: &scheduling.BasicSchedulingPolicy{}, - }, - }) - } - }), - "no pod group name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[0].Name = "" - }), - "invalid pod group name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[0].Name = ".group1" - }), - "too long pod group name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[0].Name = strings.Repeat("g", 64) - }), - "no policy set": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[0].Policy = scheduling.PodGroupPolicy{} - }), - "two policies": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[0].Policy.Gang = &scheduling.GangSchedulingPolicy{ - MinCount: 2, - } - }), - "zero min count in gang": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[1].Policy.Gang.MinCount = 0 - }), - "negative min count in gang": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[1].Policy.Gang.MinCount = -1 - }), - "two pod groups with the same name": mkWorkload(func(w *scheduling.Workload) { - w.Spec.PodGroups[1].Name = w.Spec.PodGroups[0].Name - }), + failureCases := map[string]struct { + workload *scheduling.Workload + expectedErrs field.ErrorList + }{ + "no name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Name = "" + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("metadata", "name"), "name or generateName is required"), + }, + }, + "invalid name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Name = ".workload" + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata", "name"), ".workload", "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"), + }, + }, + "too long name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Name = strings.Repeat("w", 254) + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata", "name"), ".name", "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"), + }, + }, + "no namespace": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Namespace = "" + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("metadata", "namespace"), "Required value"), + }, + }, + "invalid namespace": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Namespace = ".ns" + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata", "namespace"), ".ns", "a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"), + }, + }, + "too long namespace": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Namespace = strings.Repeat("n", 64) + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata", "namespace"), strings.Repeat("n", 64), "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"), + }, + }, + "too long controllerRef apiGroup": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.APIGroup = strings.Repeat("g", 254) + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "controllerRef", "apiGroup"), strings.Repeat("n", 64), "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')").MarkCoveredByDeclarative(), + }, + }, + "no pod group name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[0].Name = "" + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec", "podGroups").Index(0).Child("name"), "").MarkCoveredByDeclarative(), + }, + }, + "two policies": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[0].Policy.Gang = &scheduling.GangSchedulingPolicy{ + MinCount: 2, + } + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "podGroups").Index(0).Child("policy"), "{`basic`, `gang`}", "exactly one of `basic`, `gang` is required, but multiple fields are set").MarkCoveredByDeclarative(), + }, + }, + "zero min count in gang": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[1].Policy.Gang.MinCount = 0 + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec", "podGroups").Index(1).Child("policy", "gang", "minCount"), "").MarkCoveredByDeclarative(), + }, + }, + "negative min count in gang": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[1].Policy.Gang.MinCount = -1 + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "podGroups").Index(1).Child("policy", "gang", "minCount"), int64(-1), "must be greater than zero").WithOrigin("minimum").MarkCoveredByDeclarative(), + }, + }, + "two pod groups with the same name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[1].Name = w.Spec.PodGroups[0].Name + }), + expectedErrs: field.ErrorList{ + field.Duplicate(field.NewPath("spec", "podGroups").Index(1), scheduling.PodGroup{Name: "group1", Policy: scheduling.PodGroupPolicy{Gang: &scheduling.GangSchedulingPolicy{MinCount: 1}}}).MarkCoveredByDeclarative(), + }, + }, + "invalid controllerRef apiGroup": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.APIGroup = ".group" + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "controllerRef", "apiGroup"), ".group", "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')").MarkCoveredByDeclarative(), + }, + }, + "no controllerRef kind": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.Kind = "" + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec", "controllerRef", "kind"), "").MarkCoveredByDeclarative(), + }, + }, + "invalid controllerRef kind": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.Kind = "/foo" + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "controllerRef", "kind"), "/foo", "must not contain '/'").MarkCoveredByDeclarative(), + }, + }, + "no controllerRef name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.Name = "" + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec", "controllerRef", "name"), "").MarkCoveredByDeclarative(), + }, + }, + "invalid controllerRef name": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.ControllerRef.Name = "/baz" + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "controllerRef", "name"), "/baz", "must not contain '/'").WithOrigin("format=k8s-short-name").MarkCoveredByDeclarative(), + }, + }, + "no pod groups": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups = nil + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec", "podGroups"), "must have at least one item").MarkCoveredByDeclarative(), + }, + }, + "too many pod groups": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups = nil + for i := 0; i < scheduling.WorkloadMaxPodGroups+1; i++ { + w.Spec.PodGroups = append(w.Spec.PodGroups, scheduling.PodGroup{ + Name: fmt.Sprintf("group-%v", i), + Policy: scheduling.PodGroupPolicy{ + Basic: &scheduling.BasicSchedulingPolicy{}, + }, + }) + } + }), + expectedErrs: field.ErrorList{ + field.TooMany(field.NewPath("spec", "podGroups"), scheduling.WorkloadMaxPodGroups+1, scheduling.WorkloadMaxPodGroups).WithOrigin("maxItems").MarkCoveredByDeclarative(), + }, + }, + "duplicate pod group names": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[1].Name = w.Spec.PodGroups[0].Name + }), + expectedErrs: field.ErrorList{ + field.Duplicate(field.NewPath("spec", "podGroups").Index(1), scheduling.PodGroup{Name: "group1", Policy: scheduling.PodGroupPolicy{Gang: &scheduling.GangSchedulingPolicy{MinCount: 1}}}).MarkCoveredByDeclarative(), + }, + }, + "no policy set": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[0].Policy = scheduling.PodGroupPolicy{} + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "podGroups").Index(0).Child("policy"), "", "must specify one of: `basic`, `gang`").MarkCoveredByDeclarative(), + }, + }, + "multiple policies set": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[0].Policy.Gang = &scheduling.GangSchedulingPolicy{ + MinCount: 2, + } + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "podGroups").Index(0).Child("policy"), "{`basic`, `gang`}", "exactly one of `basic`, `gang` is required, but multiple fields are set").MarkCoveredByDeclarative(), + }, + }, + "negative minCount in gang": { + workload: mkWorkload(func(w *scheduling.Workload) { + w.Spec.PodGroups[1].Policy.Gang.MinCount = -1 + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "podGroups").Index(1).Child("policy", "gang", "minCount"), int64(-1), "must be greater than zero").WithOrigin("minimum").MarkCoveredByDeclarative(), + }, + }, } - for name, workload := range failureCases { - errs := ValidateWorkload(workload) - if len(errs) == 0 { - t.Errorf("Expected failure for %q", name) - } + + for name, tc := range failureCases { + t.Run(name, func(t *testing.T) { + errs := ValidateWorkload(tc.workload) + if len(errs) == 0 { + t.Errorf("Expected failure") + return + } + if len(errs) != len(tc.expectedErrs) { + t.Errorf("Expected %d errors, got %d: %v", len(tc.expectedErrs), len(errs), errs) + return + } + matcher := field.ErrorMatcher{}.ByType().ByField() + matcher.Test(t, tc.expectedErrs, errs) + + for i, err := range errs { + expectedErr := tc.expectedErrs[i] + if err.CoveredByDeclarative != expectedErr.CoveredByDeclarative { + t.Errorf("Error %d: expected CoveredByDeclarative=%v, got %v for error: %v", + i, expectedErr.CoveredByDeclarative, err.CoveredByDeclarative, err) + } + } + }) } } diff --git a/staging/src/k8s.io/api/scheduling/v1alpha1/generated.proto b/staging/src/k8s.io/api/scheduling/v1alpha1/generated.proto index 094622179ca..c43b187f1a7 100644 --- a/staging/src/k8s.io/api/scheduling/v1alpha1/generated.proto +++ b/staging/src/k8s.io/api/scheduling/v1alpha1/generated.proto @@ -198,9 +198,9 @@ message WorkloadSpec { // The maximum number of pod groups is 8. This field is immutable. // // +required - // +k8s:required // +listType=map // +listMapKey=name + // +k8s:required // +k8s:listType=map // +k8s:listMapKey=name // +k8s:maxItems=8 diff --git a/staging/src/k8s.io/api/scheduling/v1alpha1/types.go b/staging/src/k8s.io/api/scheduling/v1alpha1/types.go index 44950a17de9..26a91fc50b7 100644 --- a/staging/src/k8s.io/api/scheduling/v1alpha1/types.go +++ b/staging/src/k8s.io/api/scheduling/v1alpha1/types.go @@ -125,9 +125,9 @@ type WorkloadSpec struct { // The maximum number of pod groups is 8. This field is immutable. // // +required - // +k8s:required // +listType=map // +listMapKey=name + // +k8s:required // +k8s:listType=map // +k8s:listMapKey=name // +k8s:maxItems=8