From a1f8dc41eff5cb1e6c1e6e23b093dcf75dd057cd Mon Sep 17 00:00:00 2001 From: Ling Samuel Date: Fri, 18 Dec 2020 17:12:25 +0800 Subject: [PATCH] make labels.NewRequirement returns aggregated field.ErrorList, make nodeaffinity parsing function use it Signed-off-by: Ling Samuel --- .../validation/validation_pluginargs.go | 4 +- .../validation/validation_pluginargs_test.go | 18 +- .../interpodaffinity/filtering_test.go | 4 +- .../plugins/interpodaffinity/scoring_test.go | 4 +- .../src/k8s.io/apimachinery/pkg/labels/BUILD | 4 + .../k8s.io/apimachinery/pkg/labels/labels.go | 8 +- .../apimachinery/pkg/labels/selector.go | 66 +++-- .../apimachinery/pkg/labels/selector_test.go | 232 +++++++++++++++--- .../pkg/util/validation/field/path.go | 23 ++ .../scheduling/corev1/nodeaffinity/BUILD | 1 + .../corev1/nodeaffinity/nodeaffinity.go | 77 +++--- .../corev1/nodeaffinity/nodeaffinity_test.go | 26 +- 12 files changed, 336 insertions(+), 131 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index 1d06a6fef09..8773b39dcfd 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go @@ -284,14 +284,14 @@ func ValidateNodeAffinityArgs(args *config.NodeAffinityArgs) error { path := field.NewPath("addedAffinity") var errs []error if ns := affinity.RequiredDuringSchedulingIgnoredDuringExecution; ns != nil { - _, err := nodeaffinity.NewNodeSelector(ns, nodeaffinity.WithPath(path.Child("requiredDuringSchedulingIgnoredDuringExecution"))) + _, err := nodeaffinity.NewNodeSelector(ns, field.WithPath(path.Child("requiredDuringSchedulingIgnoredDuringExecution"))) if err != nil { errs = append(errs, err) } } // TODO: Add validation for requiredDuringSchedulingRequiredDuringExecution when it gets added to the API. if terms := affinity.PreferredDuringSchedulingIgnoredDuringExecution; len(terms) != 0 { - _, err := nodeaffinity.NewPreferredSchedulingTerms(terms, nodeaffinity.WithPath(path.Child("preferredDuringSchedulingIgnoredDuringExecution"))) + _, err := nodeaffinity.NewPreferredSchedulingTerms(terms, field.WithPath(path.Child("preferredDuringSchedulingIgnoredDuringExecution"))) if err != nil { errs = append(errs, err) } diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go index 8a4a2ab23e9..5ca7bc328f0 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go @@ -114,7 +114,7 @@ func TestValidateDefaultPreemptionArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateDefaultPreemptionArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateDefaultPreemptionArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateDefaultPreemptionArgs returned err (-want,+got):\n%s", diff) } }) } @@ -154,7 +154,7 @@ func TestValidateInterPodAffinityArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateInterPodAffinityArgs(tc.args) if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateInterPodAffinityArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateInterPodAffinityArgs returned err (-want,+got):\n%s", diff) } }) } @@ -235,7 +235,7 @@ func TestValidateNodeLabelArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateNodeLabelArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateNodeLabelArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateNodeLabelArgs returned err (-want,+got):\n%s", diff) } }) } @@ -426,7 +426,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidatePodTopologySpreadArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidatePodTopologySpreadArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidatePodTopologySpreadArgs returned err (-want,+got):\n%s", diff) } }) } @@ -668,7 +668,7 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateRequestedToCapacityRatioArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateRequestedToCapacityRatioArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateRequestedToCapacityRatioArgs returned err (-want,+got):\n%s", diff) } }) } @@ -755,7 +755,7 @@ func TestValidateNodeResourcesLeastAllocatedArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateNodeResourcesLeastAllocatedArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) } }) } @@ -842,7 +842,7 @@ func TestValidateNodeResourcesMostAllocatedArgs(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateNodeResourcesMostAllocatedArgs(tc.args) if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) } }) } @@ -927,7 +927,7 @@ func TestValidateNodeAffinityArgs(t *testing.T) { wantErr: field.ErrorList{ &field.Error{ Type: field.ErrorTypeInvalid, - Field: "addedAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0]", + Field: "addedAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].key", }, &field.Error{ Type: field.ErrorTypeInvalid, @@ -940,7 +940,7 @@ func TestValidateNodeAffinityArgs(t *testing.T) { t.Run(tc.name, func(t *testing.T) { err := ValidateNodeAffinityArgs(&tc.args) if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" { - t.Fatalf("ValidatedNodeAffinityArgs returned err (-want,+got):\n%s", diff) + t.Errorf("ValidatedNodeAffinityArgs returned err (-want,+got):\n%s", diff) } }) } diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go index 14c0fe6ccd1..3e9bf5625f4 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go @@ -807,7 +807,7 @@ func TestRequiredAffinitySingleNode(t *testing.T) { node: &node1, wantStatus: framework.NewStatus( framework.UnschedulableAndUnresolvable, - "invalid label value", + `Invalid value: "{{.bad-value.}}"`, ), }, { @@ -844,7 +844,7 @@ func TestRequiredAffinitySingleNode(t *testing.T) { node: &node1, wantStatus: framework.NewStatus( framework.UnschedulableAndUnresolvable, - "invalid label value", + `Invalid value: "{{.bad-value.}}"`, ), }, } diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go index 080058be9f1..f82faab0ef6 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go @@ -561,7 +561,7 @@ func TestPreferredAffinity(t *testing.T) { { name: "invalid Affinity fails PreScore", pod: &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAffinityLabels}}, - wantStatus: framework.NewStatus(framework.Error, "invalid label value"), + wantStatus: framework.NewStatus(framework.Error, `Invalid value: "{{.bad-value.}}"`), nodes: []*v1.Node{ {ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}}, {ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}}, @@ -570,7 +570,7 @@ func TestPreferredAffinity(t *testing.T) { { name: "invalid AntiAffinity fails PreScore", pod: &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAntiAffinityLabels}}, - wantStatus: framework.NewStatus(framework.Error, "invalid label value"), + wantStatus: framework.NewStatus(framework.Error, `Invalid value: "{{.bad-value.}}"`), nodes: []*v1.Node{ {ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}}, {ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}}, diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/BUILD b/staging/src/k8s.io/apimachinery/pkg/labels/BUILD index 45fea8c199e..8608238255d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/BUILD +++ b/staging/src/k8s.io/apimachinery/pkg/labels/BUILD @@ -16,6 +16,9 @@ go_test( deps = [ "//staging/src/k8s.io/apimachinery/pkg/selection:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/github.com/google/go-cmp/cmp/cmpopts:go_default_library", ], ) @@ -33,6 +36,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/selection:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//vendor/k8s.io/klog/v2:go_default_library", ], ) diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/labels.go b/staging/src/k8s.io/apimachinery/pkg/labels/labels.go index d6bbeeaca78..a7d0db99c33 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/labels.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/labels.go @@ -20,6 +20,8 @@ import ( "fmt" "sort" "strings" + + "k8s.io/apimachinery/pkg/util/validation/field" ) // Labels allows you to present labels independently from their storage. @@ -143,7 +145,7 @@ func Equals(labels1, labels2 Set) bool { // ConvertSelectorToLabelsMap converts selector string to labels map // and validates keys and values -func ConvertSelectorToLabelsMap(selector string) (Set, error) { +func ConvertSelectorToLabelsMap(selector string, opts ...field.PathOption) (Set, error) { labelsMap := Set{} if len(selector) == 0 { @@ -157,11 +159,11 @@ func ConvertSelectorToLabelsMap(selector string) (Set, error) { return labelsMap, fmt.Errorf("invalid selector: %s", l) } key := strings.TrimSpace(l[0]) - if err := validateLabelKey(key); err != nil { + if err := validateLabelKey(key, field.ToPath(opts...)); err != nil { return labelsMap, err } value := strings.TrimSpace(l[1]) - if err := validateLabelValue(key, value); err != nil { + if err := validateLabelValue(key, value, field.ToPath(opts...)); err != nil { return labelsMap, err } labelsMap[key] = value diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go index e5f1a826403..6304bf63768 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go @@ -26,9 +26,19 @@ import ( "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" ) +var ( + validRequirementOperators = []string{ + string(selection.In), string(selection.NotIn), + string(selection.Equals), string(selection.DoubleEquals), string(selection.NotEquals), + string(selection.Exists), string(selection.DoesNotExist), + string(selection.GreaterThan), string(selection.LessThan), + } +) + // Requirements is AND of all requirements. type Requirements []Requirement @@ -139,42 +149,47 @@ type Requirement struct { // of characters. See validateLabelKey for more details. // // The empty string is a valid value in the input values set. -func NewRequirement(key string, op selection.Operator, vals []string) (*Requirement, error) { - if err := validateLabelKey(key); err != nil { - return nil, err +// Returned error, if not nil, is guaranteed to be an aggregated field.ErrorList +func NewRequirement(key string, op selection.Operator, vals []string, opts ...field.PathOption) (*Requirement, error) { + var allErrs field.ErrorList + path := field.ToPath(opts...) + if err := validateLabelKey(key, path.Child("key")); err != nil { + allErrs = append(allErrs, err) } + + valuePath := path.Child("values") switch op { case selection.In, selection.NotIn: if len(vals) == 0 { - return nil, fmt.Errorf("for 'in', 'notin' operators, values set can't be empty") + allErrs = append(allErrs, field.Invalid(valuePath, vals, "for 'in', 'notin' operators, values set can't be empty")) } case selection.Equals, selection.DoubleEquals, selection.NotEquals: if len(vals) != 1 { - return nil, fmt.Errorf("exact-match compatibility requires one single value") + allErrs = append(allErrs, field.Invalid(valuePath, vals, "exact-match compatibility requires one single value")) } case selection.Exists, selection.DoesNotExist: if len(vals) != 0 { - return nil, fmt.Errorf("values set must be empty for exists and does not exist") + allErrs = append(allErrs, field.Invalid(valuePath, vals, "values set must be empty for exists and does not exist")) } case selection.GreaterThan, selection.LessThan: if len(vals) != 1 { - return nil, fmt.Errorf("for 'Gt', 'Lt' operators, exactly one value is required") + allErrs = append(allErrs, field.Invalid(valuePath, vals, "for 'Gt', 'Lt' operators, exactly one value is required")) } for i := range vals { if _, err := strconv.ParseInt(vals[i], 10, 64); err != nil { - return nil, fmt.Errorf("for 'Gt', 'Lt' operators, the value must be an integer") + allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i], "for 'Gt', 'Lt' operators, the value must be an integer")) } } default: - return nil, fmt.Errorf("operator '%v' is not recognized", op) + allErrs = append(allErrs, field.NotSupported(path.Child("operator"), op, validRequirementOperators)) } for i := range vals { - if err := validateLabelValue(key, vals[i]); err != nil { - return nil, err + if err := validateLabelValue(key, vals[i], valuePath.Index(i)); err != nil { + allErrs = append(allErrs, err) } } - return &Requirement{key: key, operator: op, strValues: vals}, nil + return &Requirement{key: key, operator: op, strValues: vals}, allErrs.ToAggregate() } func (r *Requirement) hasValue(value string) bool { @@ -560,6 +575,7 @@ type Parser struct { l *Lexer scannedItems []ScannedItem position int + path *field.Path } // ParserContext represents context during parsing: @@ -653,7 +669,7 @@ func (p *Parser) parseRequirement() (*Requirement, error) { return nil, err } if operator == selection.Exists || operator == selection.DoesNotExist { // operator found lookahead set checked - return NewRequirement(key, operator, []string{}) + return NewRequirement(key, operator, []string{}, field.WithPath(p.path)) } operator, err = p.parseOperator() if err != nil { @@ -669,7 +685,7 @@ func (p *Parser) parseRequirement() (*Requirement, error) { if err != nil { return nil, err } - return NewRequirement(key, operator, values.List()) + return NewRequirement(key, operator, values.List(), field.WithPath(p.path)) } @@ -687,7 +703,7 @@ func (p *Parser) parseKeyAndInferOperator() (string, selection.Operator, error) err := fmt.Errorf("found '%s', expected: identifier", literal) return "", "", err } - if err := validateLabelKey(literal); err != nil { + if err := validateLabelKey(literal, p.path); err != nil { return "", "", err } if t, _ := p.lookahead(Values); t == EndOfStringToken || t == CommaToken { @@ -833,8 +849,8 @@ func (p *Parser) parseExactValue() (sets.String, error) { // the KEY exists and can be any VALUE. // (5) A requirement with just !KEY requires that the KEY not exist. // -func Parse(selector string) (Selector, error) { - parsedSelector, err := parse(selector) +func Parse(selector string, opts ...field.PathOption) (Selector, error) { + parsedSelector, err := parse(selector, field.ToPath(opts...)) if err == nil { return parsedSelector, nil } @@ -845,8 +861,8 @@ func Parse(selector string) (Selector, error) { // The callers of this method can then decide how to return the internalSelector struct to their // callers. This function has two callers now, one returns a Selector interface and the other // returns a list of requirements. -func parse(selector string) (internalSelector, error) { - p := &Parser{l: &Lexer{s: selector, pos: 0}} +func parse(selector string, path *field.Path) (internalSelector, error) { + p := &Parser{l: &Lexer{s: selector, pos: 0}, path: path} items, err := p.parse() if err != nil { return nil, err @@ -855,16 +871,16 @@ func parse(selector string) (internalSelector, error) { return internalSelector(items), err } -func validateLabelKey(k string) error { +func validateLabelKey(k string, path *field.Path) *field.Error { if errs := validation.IsQualifiedName(k); len(errs) != 0 { - return fmt.Errorf("invalid label key %q: %s", k, strings.Join(errs, "; ")) + return field.Invalid(path, k, strings.Join(errs, "; ")) } return nil } -func validateLabelValue(k, v string) error { +func validateLabelValue(k, v string, path *field.Path) *field.Error { if errs := validation.IsValidLabelValue(v); len(errs) != 0 { - return fmt.Errorf("invalid label value: %q: at key: %q: %s", v, k, strings.Join(errs, "; ")) + return field.Invalid(path.Key(k), v, strings.Join(errs, "; ")) } return nil } @@ -918,6 +934,6 @@ func SelectorFromValidatedSet(ls Set) Selector { // processing on selector requirements. // See the documentation for Parse() function for more details. // TODO: Consider exporting the internalSelector type instead. -func ParseToRequirements(selector string) ([]Requirement, error) { - return parse(selector) +func ParseToRequirements(selector string, opts ...field.PathOption) ([]Requirement, error) { + return parse(selector, field.ToPath(opts...)) } diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go index aa3fb2e54bc..683451c9d0d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go @@ -17,13 +17,19 @@ limitations under the License. package labels import ( - "fmt" "reflect" "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +var ( + ignoreDetail = cmpopts.IgnoreFields(field.Error{}, "Detail") ) func TestSelectorParse(t *testing.T) { @@ -303,27 +309,179 @@ func TestRequirementConstructor(t *testing.T) { Key string Op selection.Operator Vals sets.String - Success bool + WantErr field.ErrorList }{ - {"x", selection.In, nil, false}, - {"x", selection.NotIn, sets.NewString(), false}, - {"x", selection.In, sets.NewString("foo"), true}, - {"x", selection.NotIn, sets.NewString("foo"), true}, - {"x", selection.Exists, nil, true}, - {"x", selection.DoesNotExist, nil, true}, - {"1foo", selection.In, sets.NewString("bar"), true}, - {"1234", selection.In, sets.NewString("bar"), true}, - {"y", selection.GreaterThan, sets.NewString("1"), true}, - {"z", selection.LessThan, sets.NewString("6"), true}, - {"foo", selection.GreaterThan, sets.NewString("bar"), false}, - {"barz", selection.LessThan, sets.NewString("blah"), false}, - {strings.Repeat("a", 254), selection.Exists, nil, false}, //breaks DNS rule that len(key) <= 253 + { + Key: "x1", + Op: selection.In, + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values", + BadValue: []string{}, + }, + }, + }, + { + Key: "x2", + Op: selection.NotIn, + Vals: sets.NewString(), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values", + BadValue: []string{}, + }, + }, + }, + { + Key: "x3", + Op: selection.In, + Vals: sets.NewString("foo"), + }, + { + Key: "x4", + Op: selection.NotIn, + Vals: sets.NewString("foo"), + }, + { + Key: "x5", + Op: selection.Equals, + Vals: sets.NewString("foo", "bar"), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values", + BadValue: []string{"bar", "foo"}, + }, + }, + }, + { + Key: "x6", + Op: selection.Exists, + }, + { + Key: "x7", + Op: selection.DoesNotExist, + }, + { + Key: "x8", + Op: selection.Exists, + Vals: sets.NewString("foo"), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values", + BadValue: []string{"foo"}, + }, + }, + }, + { + Key: "x9", + Op: selection.In, + Vals: sets.NewString("bar"), + }, + { + Key: "x10", + Op: selection.In, + Vals: sets.NewString("bar"), + }, + { + Key: "x11", + Op: selection.GreaterThan, + Vals: sets.NewString("1"), + }, + { + Key: "x12", + Op: selection.LessThan, + Vals: sets.NewString("6"), + }, + { + Key: "x13", + Op: selection.GreaterThan, + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values", + BadValue: []string{}, + }, + }, + }, + { + Key: "x14", + Op: selection.GreaterThan, + Vals: sets.NewString("bar"), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values[0]", + BadValue: "bar", + }, + }, + }, + { + Key: "x15", + Op: selection.LessThan, + Vals: sets.NewString("bar"), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values[0]", + BadValue: "bar", + }, + }, + }, + { + Key: strings.Repeat("a", 254), //breaks DNS rule that len(key) <= 253 + Op: selection.Exists, + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "key", + BadValue: strings.Repeat("a", 254), + }, + }, + }, + { + Key: "x16", + Op: selection.Equals, + Vals: sets.NewString(strings.Repeat("a", 254)), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values[0][x16]", + BadValue: strings.Repeat("a", 254), + }, + }, + }, + { + Key: "x17", + Op: selection.Equals, + Vals: sets.NewString("a b"), + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values[0][x17]", + BadValue: "a b", + }, + }, + }, + { + Key: "x18", + Op: "unsupportedOp", + WantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeNotSupported, + Field: "operator", + BadValue: selection.Operator("unsupportedOp"), + }, + }, + }, } for _, rc := range requirementConstructorTests { - if _, err := NewRequirement(rc.Key, rc.Op, rc.Vals.List()); err == nil && !rc.Success { - t.Errorf("expected error with key:%#v op:%v vals:%v, got no error", rc.Key, rc.Op, rc.Vals) - } else if err != nil && rc.Success { - t.Errorf("expected no error with key:%#v op:%v vals:%v, got:%v", rc.Key, rc.Op, rc.Vals, err) + _, err := NewRequirement(rc.Key, rc.Op, rc.Vals.List()) + if diff := cmp.Diff(rc.WantErr.ToAggregate(), err, ignoreDetail); diff != "" { + t.Errorf("NewRequirement test %v returned unexpected error (-want,+got):\n%s", rc.Key, diff) } } } @@ -714,27 +872,41 @@ func TestValidatedSelectorFromSet(t *testing.T) { name string input Set expectedSelector internalSelector - expectedError error + expectedError field.ErrorList }{ { - name: "Simple Set, no error", - input: Set{"key": "val"}, - expectedSelector: internalSelector([]Requirement{{key: "key", operator: selection.Equals, strValues: []string{"val"}}}), + name: "Simple Set, no error", + input: Set{"key": "val"}, + expectedSelector: internalSelector{ + Requirement{ + key: "key", + operator: selection.Equals, + strValues: []string{"val"}, + }, + }, }, { - name: "Invalid Set, value too long", - input: Set{"Key": "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui"}, - expectedError: fmt.Errorf(`invalid label value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": at key: "Key": must be no more than 63 characters`), + name: "Invalid Set, value too long", + input: Set{"Key": "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui"}, + expectedError: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "values[0][Key]", + BadValue: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui", + }, + }, }, } for _, tc := range tests { selector, err := ValidatedSelectorFromSet(tc.input) - if !reflect.DeepEqual(err, tc.expectedError) { - t.Fatalf("expected error %v, got error %v", tc.expectedError, err) + if diff := cmp.Diff(tc.expectedError.ToAggregate(), err, ignoreDetail); diff != "" { + t.Errorf("ValidatedSelectorFromSet %#v returned unexpected error (-want,+got):\n%s", tc.name, diff) } - if err == nil && !reflect.DeepEqual(selector, tc.expectedSelector) { - t.Errorf("expected selector %v, got selector %v", tc.expectedSelector, selector) + if err == nil { + if diff := cmp.Diff(tc.expectedSelector, selector, cmp.AllowUnexported(Requirement{})); diff != "" { + t.Errorf("ValidatedSelectorFromSet %#v returned unexpected selector (-want,+got):\n%s", tc.name, diff) + } } } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/path.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/path.go index f9be7ac3391..daccb058903 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/path.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/path.go @@ -22,6 +22,29 @@ import ( "strconv" ) +type pathOptions struct { + path *Path +} + +// PathOption modifies a pathOptions +type PathOption func(o *pathOptions) + +// WithPath generates a PathOption +func WithPath(p *Path) PathOption { + return func(o *pathOptions) { + o.path = p + } +} + +// ToPath produces *Path from a set of PathOption +func ToPath(opts ...PathOption) *Path { + c := &pathOptions{} + for _, opt := range opts { + opt(c) + } + return c.path +} + // Path represents the path from some root to a particular field. type Path struct { name string // the name of this field or "" if this is an index diff --git a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD index 3574bd8a6b8..25fe927f983 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD +++ b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD @@ -11,6 +11,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/selection:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", ], ) diff --git a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go index c9bca617fd3..4157f772cad 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go +++ b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go @@ -21,12 +21,10 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation/field" ) -// ParseOption is an option for parsing. -type ParseOption func(*parseOptions) - // NodeSelector is a runtime representation of v1.NodeSelector. type NodeSelector struct { lazy LazyErrorNodeSelector @@ -38,37 +36,27 @@ type LazyErrorNodeSelector struct { terms []nodeSelectorTerm } -// WithPath sets a field.Path to be used as root for parse errors. -func WithPath(p *field.Path) ParseOption { - return func(o *parseOptions) { - o.path = p - } -} - // NewNodeSelector returns a NodeSelector or aggregate parsing errors found. -func NewNodeSelector(ns *v1.NodeSelector, opts ...ParseOption) (*NodeSelector, error) { +func NewNodeSelector(ns *v1.NodeSelector, opts ...field.PathOption) (*NodeSelector, error) { lazy := NewLazyErrorNodeSelector(ns, opts...) - var errs field.ErrorList + var errs []error for _, term := range lazy.terms { if len(term.parseErrs) > 0 { errs = append(errs, term.parseErrs...) } } if len(errs) != 0 { - return nil, errs.ToAggregate() + return nil, errors.Flatten(errors.NewAggregate(errs)) } return &NodeSelector{lazy: *lazy}, nil } // NewLazyErrorNodeSelector creates a NodeSelector that only reports parse // errors when no terms match. -func NewLazyErrorNodeSelector(ns *v1.NodeSelector, opts ...ParseOption) *LazyErrorNodeSelector { - o := parseOptions{} - for _, opt := range opts { - opt(&o) - } +func NewLazyErrorNodeSelector(ns *v1.NodeSelector, opts ...field.PathOption) *LazyErrorNodeSelector { + p := field.ToPath(opts...) parsedTerms := make([]nodeSelectorTerm, 0, len(ns.NodeSelectorTerms)) - path := o.path.Child("nodeSelectorTerms") + path := p.Child("nodeSelectorTerms") for i, term := range ns.NodeSelectorTerms { // nil or empty term selects no objects if isEmptyNodeSelectorTerm(&term) { @@ -100,7 +88,7 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) { nodeLabels := labels.Set(node.Labels) nodeFields := extractNodeFields(node) - var errs field.ErrorList + var errs []error for _, term := range ns.terms { match, tErrs := term.match(nodeLabels, nodeFields) if len(tErrs) > 0 { @@ -111,7 +99,7 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) { return true, nil } } - return false, errs.ToAggregate() + return false, errors.Flatten(errors.NewAggregate(errs)) } // PreferredSchedulingTerms is a runtime representation of []v1.PreferredSchedulingTerms. @@ -121,15 +109,12 @@ type PreferredSchedulingTerms struct { // NewPreferredSchedulingTerms returns a PreferredSchedulingTerms or all the parsing errors found. // If a v1.PreferredSchedulingTerm has a 0 weight, its parsing is skipped. -func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...ParseOption) (*PreferredSchedulingTerms, error) { - o := parseOptions{} - for _, opt := range opts { - opt(&o) - } - var errs field.ErrorList +func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...field.PathOption) (*PreferredSchedulingTerms, error) { + p := field.ToPath(opts...) + var errs []error parsedTerms := make([]preferredSchedulingTerm, 0, len(terms)) for i, term := range terms { - path := o.path.Index(i) + path := p.Index(i) if term.Weight == 0 || isEmptyNodeSelectorTerm(&term.Preference) { continue } @@ -144,7 +129,7 @@ func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...Par } } if len(errs) != 0 { - return nil, errs.ToAggregate() + return nil, errors.Flatten(errors.NewAggregate(errs)) } return &PreferredSchedulingTerms{terms: parsedTerms}, nil } @@ -179,26 +164,30 @@ func extractNodeFields(n *v1.Node) fields.Set { type nodeSelectorTerm struct { matchLabels labels.Selector matchFields fields.Selector - parseErrs field.ErrorList + parseErrs []error } func newNodeSelectorTerm(term *v1.NodeSelectorTerm, path *field.Path) nodeSelectorTerm { var parsedTerm nodeSelectorTerm + var errs []error if len(term.MatchExpressions) != 0 { p := path.Child("matchExpressions") - parsedTerm.matchLabels, parsedTerm.parseErrs = nodeSelectorRequirementsAsSelector(term.MatchExpressions, p) - if parsedTerm.parseErrs != nil { - return parsedTerm + parsedTerm.matchLabels, errs = nodeSelectorRequirementsAsSelector(term.MatchExpressions, p) + if errs != nil { + parsedTerm.parseErrs = append(parsedTerm.parseErrs, errs...) } } if len(term.MatchFields) != 0 { p := path.Child("matchFields") - parsedTerm.matchFields, parsedTerm.parseErrs = nodeSelectorRequirementsAsFieldSelector(term.MatchFields, p) + parsedTerm.matchFields, errs = nodeSelectorRequirementsAsFieldSelector(term.MatchFields, p) + if errs != nil { + parsedTerm.parseErrs = append(parsedTerm.parseErrs, errs...) + } } return parsedTerm } -func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, field.ErrorList) { +func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, []error) { if t.parseErrs != nil { return false, t.parseErrs } @@ -213,11 +202,11 @@ func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) ( // nodeSelectorRequirementsAsSelector converts the []NodeSelectorRequirement api type into a struct that implements // labels.Selector. -func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement, path *field.Path) (labels.Selector, field.ErrorList) { +func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement, path *field.Path) (labels.Selector, []error) { if len(nsm) == 0 { return labels.Nothing(), nil } - var errs field.ErrorList + var errs []error selector := labels.NewSelector() for i, expr := range nsm { p := path.Index(i) @@ -239,11 +228,9 @@ func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement, path * errs = append(errs, field.NotSupported(p.Child("operator"), expr.Operator, nil)) continue } - r, err := labels.NewRequirement(expr.Key, op, expr.Values) + r, err := labels.NewRequirement(expr.Key, op, expr.Values, field.WithPath(p)) if err != nil { - // TODO(#96167): Use error as returned by labels.NewRequirement once it - // is a field.Error. - errs = append(errs, field.Invalid(p, expr, err.Error())) + errs = append(errs, err) } else { selector = selector.Add(*r) } @@ -261,11 +248,11 @@ var validFieldSelectorOperators = []string{ // nodeSelectorRequirementsAsFieldSelector converts the []NodeSelectorRequirement core type into a struct that implements // fields.Selector. -func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement, path *field.Path) (fields.Selector, field.ErrorList) { +func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement, path *field.Path) (fields.Selector, []error) { if len(nsr) == 0 { return fields.Nothing(), nil } - var errs field.ErrorList + var errs []error var selectors []fields.Selector for i, expr := range nsr { @@ -300,7 +287,3 @@ type preferredSchedulingTerm struct { nodeSelectorTerm weight int } - -type parseOptions struct { - path *field.Path -} diff --git a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go index 6d923095f9e..d08c8c93acf 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go +++ b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go @@ -82,8 +82,8 @@ func TestNodeSelectorMatch(t *testing.T) { }, &field.Error{ Type: field.ErrorTypeInvalid, - Field: "nodeSelectorTerms[2].matchExpressions[0]", - Detail: `invalid label key "invalid key": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + Field: "nodeSelectorTerms[2].matchExpressions[0].key", + Detail: `name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, }, }.ToAggregate(), }, @@ -150,7 +150,7 @@ func TestNodeSelectorMatch(t *testing.T) { t.Run(tt.name, func(t *testing.T) { nodeSelector, err := NewNodeSelector(&tt.nodeSelector) if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" { - t.Fatalf("NewNodeSelector returned unexpected error (-want,+got):\n%s", diff) + t.Errorf("NewNodeSelector returned unexpected error (-want,+got):\n%s", diff) } if tt.wantErr != nil { return @@ -218,8 +218,8 @@ func TestPreferredSchedulingTermsScore(t *testing.T) { }, &field.Error{ Type: field.ErrorTypeInvalid, - Field: "[2].matchExpressions[0]", - Detail: `invalid label key "invalid key": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + Field: "[2].matchExpressions[0].key", + Detail: `name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, }, }.ToAggregate(), }, @@ -281,7 +281,7 @@ func TestPreferredSchedulingTermsScore(t *testing.T) { t.Run(tt.name, func(t *testing.T) { prefSchedTerms, err := NewPreferredSchedulingTerms(tt.prefSchedTerms) if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" { - t.Fatalf("NewPreferredSchedulingTerms returned unexpected error (-want,+got):\n%s", diff) + t.Errorf("NewPreferredSchedulingTerms returned unexpected error (-want,+got):\n%s", diff) } if tt.wantErr != nil { return @@ -308,9 +308,9 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { return out } tc := []struct { - in []v1.NodeSelectorRequirement - out labels.Selector - wantErrs field.ErrorList + in []v1.NodeSelectorRequirement + out labels.Selector + wantErr []error }{ {in: nil, out: labels.Nothing()}, {in: []v1.NodeSelectorRequirement{}, out: labels.Nothing()}, @@ -324,7 +324,11 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { Operator: v1.NodeSelectorOpExists, Values: []string{"bar", "baz"}, }}, - wantErrs: field.ErrorList{field.Invalid(field.NewPath("root").Index(0), nil, "values set must be empty for exists and does not exist")}, + wantErr: []error{ + field.ErrorList{ + field.Invalid(field.NewPath("root").Index(0).Child("values"), nil, "values set must be empty for exists and does not exist"), + }.ToAggregate(), + }, }, { in: []v1.NodeSelectorRequirement{{ @@ -346,7 +350,7 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { for i, tc := range tc { out, err := nodeSelectorRequirementsAsSelector(tc.in, field.NewPath("root")) - if diff := cmp.Diff(tc.wantErrs, err, ignoreBadValue); diff != "" { + if diff := cmp.Diff(tc.wantErr, err, ignoreBadValue); diff != "" { t.Errorf("nodeSelectorRequirementsAsSelector returned unexpected error (-want,+got):\n%s", diff) } if !reflect.DeepEqual(out, tc.out) {