diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index 61647944771..1d06a6fef09 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go @@ -21,6 +21,7 @@ import ( v1 "k8s.io/api/core/v1" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/component-helpers/scheduling/corev1/nodeaffinity" @@ -280,22 +281,20 @@ func ValidateNodeAffinityArgs(args *config.NodeAffinityArgs) error { return nil } affinity := args.AddedAffinity - f := field.NewPath("addedAffinity") - var allErrs field.ErrorList + path := field.NewPath("addedAffinity") + var errs []error if ns := affinity.RequiredDuringSchedulingIgnoredDuringExecution; ns != nil { - _, err := nodeaffinity.NewNodeSelector(ns) + _, err := nodeaffinity.NewNodeSelector(ns, nodeaffinity.WithPath(path.Child("requiredDuringSchedulingIgnoredDuringExecution"))) if err != nil { - // TODO(#96167): Expand all field.Error(s) returned once constructor use them. - allErrs = append(allErrs, field.Invalid(f.Child("requiredDuringSchedulingIgnoredDuringExecution"), affinity.RequiredDuringSchedulingIgnoredDuringExecution, err.Error())) + 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) + _, err := nodeaffinity.NewPreferredSchedulingTerms(terms, nodeaffinity.WithPath(path.Child("preferredDuringSchedulingIgnoredDuringExecution"))) if err != nil { - // TODO(#96167): Expand all field.Error(s) returned once constructor use them. - allErrs = append(allErrs, field.Invalid(f.Child("preferredDuringSchedulingIgnoredDuringExecution"), affinity.PreferredDuringSchedulingIgnoredDuringExecution, err.Error())) + errs = append(errs, err) } } - return allErrs.ToAggregate() + return errors.Flatten(errors.NewAggregate(errs)) } diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go index cb1fa65b90b..8a4a2ab23e9 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go @@ -925,15 +925,21 @@ func TestValidateNodeAffinityArgs(t *testing.T) { }, }, wantErr: field.ErrorList{ - field.Invalid(field.NewPath("addedAffinity", "requiredDuringSchedulingIgnoredDuringExecution"), nil, ""), - field.Invalid(field.NewPath("addedAffinity", "preferredDuringSchedulingIgnoredDuringExecution"), nil, ""), + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "addedAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0]", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "addedAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].matchFields[0].values", + }, }.ToAggregate(), }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := ValidateNodeAffinityArgs(&tc.args) - if diff := cmp.Diff(err, tc.wantErr, ignoreBadValueDetail); diff != "" { + if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" { t.Fatalf("ValidatedNodeAffinityArgs returned err (-want,+got):\n%s", diff) } }) diff --git a/staging/src/k8s.io/component-helpers/go.mod b/staging/src/k8s.io/component-helpers/go.mod index a8383edc7b2..5265824c36e 100644 --- a/staging/src/k8s.io/component-helpers/go.mod +++ b/staging/src/k8s.io/component-helpers/go.mod @@ -5,6 +5,7 @@ module k8s.io/component-helpers go 1.15 require ( + github.com/google/go-cmp v0.5.2 k8s.io/api v0.0.0 k8s.io/apimachinery v0.0.0 k8s.io/client-go v0.0.0 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 76427553ee2..3574bd8a6b8 100644 --- a/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD +++ b/staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/BUILD @@ -11,7 +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", ], ) @@ -37,6 +37,8 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels: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", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/github.com/google/go-cmp/cmp/cmpopts: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 efca431e615..c9bca617fd3 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 @@ -17,15 +17,16 @@ limitations under the License. package nodeaffinity import ( - "fmt" - v1 "k8s.io/api/core/v1" "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 @@ -37,31 +38,44 @@ type LazyErrorNodeSelector struct { terms []nodeSelectorTerm } -// NewNodeSelector returns a NodeSelector or all parsing errors found. -func NewNodeSelector(ns *v1.NodeSelector) (*NodeSelector, error) { - lazy := NewLazyErrorNodeSelector(ns) - var errs []error +// 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) { + lazy := NewLazyErrorNodeSelector(ns, opts...) + var errs field.ErrorList for _, term := range lazy.terms { - if term.parseErr != nil { - errs = append(errs, term.parseErr) + if len(term.parseErrs) > 0 { + errs = append(errs, term.parseErrs...) } } if len(errs) != 0 { - return nil, errors.NewAggregate(errs) + return nil, errs.ToAggregate() } return &NodeSelector{lazy: *lazy}, nil } // NewLazyErrorNodeSelector creates a NodeSelector that only reports parse // errors when no terms match. -func NewLazyErrorNodeSelector(ns *v1.NodeSelector) *LazyErrorNodeSelector { +func NewLazyErrorNodeSelector(ns *v1.NodeSelector, opts ...ParseOption) *LazyErrorNodeSelector { + o := parseOptions{} + for _, opt := range opts { + opt(&o) + } parsedTerms := make([]nodeSelectorTerm, 0, len(ns.NodeSelectorTerms)) - for _, term := range ns.NodeSelectorTerms { + path := o.path.Child("nodeSelectorTerms") + for i, term := range ns.NodeSelectorTerms { // nil or empty term selects no objects if isEmptyNodeSelectorTerm(&term) { continue } - parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term)) + p := path.Index(i) + parsedTerms = append(parsedTerms, newNodeSelectorTerm(&term, p)) } return &LazyErrorNodeSelector{ terms: parsedTerms, @@ -86,18 +100,18 @@ func (ns *LazyErrorNodeSelector) Match(node *v1.Node) (bool, error) { nodeLabels := labels.Set(node.Labels) nodeFields := extractNodeFields(node) - var errs []error + var errs field.ErrorList for _, term := range ns.terms { - match, err := term.match(nodeLabels, nodeFields) - if err != nil { - errs = append(errs, term.parseErr) + match, tErrs := term.match(nodeLabels, nodeFields) + if len(tErrs) > 0 { + errs = append(errs, tErrs...) continue } if match { return true, nil } } - return false, errors.NewAggregate(errs) + return false, errs.ToAggregate() } // PreferredSchedulingTerms is a runtime representation of []v1.PreferredSchedulingTerms. @@ -107,25 +121,30 @@ 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) (*PreferredSchedulingTerms, error) { - var errs []error +func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...ParseOption) (*PreferredSchedulingTerms, error) { + o := parseOptions{} + for _, opt := range opts { + opt(&o) + } + var errs field.ErrorList parsedTerms := make([]preferredSchedulingTerm, 0, len(terms)) - for _, term := range terms { + for i, term := range terms { + path := o.path.Index(i) if term.Weight == 0 || isEmptyNodeSelectorTerm(&term.Preference) { continue } parsedTerm := preferredSchedulingTerm{ - nodeSelectorTerm: newNodeSelectorTerm(&term.Preference), + nodeSelectorTerm: newNodeSelectorTerm(&term.Preference, path), weight: int(term.Weight), } - if parsedTerm.parseErr != nil { - errs = append(errs, parsedTerm.parseErr) + if len(parsedTerm.parseErrs) > 0 { + errs = append(errs, parsedTerm.parseErrs...) } else { parsedTerms = append(parsedTerms, parsedTerm) } } if len(errs) != 0 { - return nil, errors.NewAggregate(errs) + return nil, errs.ToAggregate() } return &PreferredSchedulingTerms{terms: parsedTerms}, nil } @@ -160,26 +179,28 @@ func extractNodeFields(n *v1.Node) fields.Set { type nodeSelectorTerm struct { matchLabels labels.Selector matchFields fields.Selector - parseErr error + parseErrs field.ErrorList } -func newNodeSelectorTerm(term *v1.NodeSelectorTerm) nodeSelectorTerm { +func newNodeSelectorTerm(term *v1.NodeSelectorTerm, path *field.Path) nodeSelectorTerm { var parsedTerm nodeSelectorTerm if len(term.MatchExpressions) != 0 { - parsedTerm.matchLabels, parsedTerm.parseErr = nodeSelectorRequirementsAsSelector(term.MatchExpressions) - if parsedTerm.parseErr != nil { + p := path.Child("matchExpressions") + parsedTerm.matchLabels, parsedTerm.parseErrs = nodeSelectorRequirementsAsSelector(term.MatchExpressions, p) + if parsedTerm.parseErrs != nil { return parsedTerm } } if len(term.MatchFields) != 0 { - parsedTerm.matchFields, parsedTerm.parseErr = nodeSelectorRequirementsAsFieldSelector(term.MatchFields) + p := path.Child("matchFields") + parsedTerm.matchFields, parsedTerm.parseErrs = nodeSelectorRequirementsAsFieldSelector(term.MatchFields, p) } return parsedTerm } -func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, error) { - if t.parseErr != nil { - return false, t.parseErr +func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, field.ErrorList) { + if t.parseErrs != nil { + return false, t.parseErrs } if t.matchLabels != nil && !t.matchLabels.Matches(nodeLabels) { return false, nil @@ -192,12 +213,14 @@ 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) (labels.Selector, error) { +func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement, path *field.Path) (labels.Selector, field.ErrorList) { if len(nsm) == 0 { return labels.Nothing(), nil } + var errs field.ErrorList selector := labels.NewSelector() - for _, expr := range nsm { + for i, expr := range nsm { + p := path.Index(i) var op selection.Operator switch expr.Operator { case v1.NodeSelectorOpIn: @@ -213,46 +236,63 @@ func nodeSelectorRequirementsAsSelector(nsm []v1.NodeSelectorRequirement) (label case v1.NodeSelectorOpLt: op = selection.LessThan default: - return nil, fmt.Errorf("%q is not a valid node selector operator", expr.Operator) + errs = append(errs, field.NotSupported(p.Child("operator"), expr.Operator, nil)) + continue } r, err := labels.NewRequirement(expr.Key, op, expr.Values) if err != nil { - return nil, err + // TODO(#96167): Use error as returned by labels.NewRequirement once it + // is a field.Error. + errs = append(errs, field.Invalid(p, expr, err.Error())) + } else { + selector = selector.Add(*r) } - selector = selector.Add(*r) + } + if len(errs) != 0 { + return nil, errs } return selector, nil } +var validFieldSelectorOperators = []string{ + string(v1.NodeSelectorOpIn), + string(v1.NodeSelectorOpNotIn), +} + // nodeSelectorRequirementsAsFieldSelector converts the []NodeSelectorRequirement core type into a struct that implements // fields.Selector. -func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement) (fields.Selector, error) { +func nodeSelectorRequirementsAsFieldSelector(nsr []v1.NodeSelectorRequirement, path *field.Path) (fields.Selector, field.ErrorList) { if len(nsr) == 0 { return fields.Nothing(), nil } + var errs field.ErrorList var selectors []fields.Selector - for _, expr := range nsr { + for i, expr := range nsr { + p := path.Index(i) switch expr.Operator { case v1.NodeSelectorOpIn: if len(expr.Values) != 1 { - return nil, fmt.Errorf("unexpected number of value (%d) for node field selector operator %q", - len(expr.Values), expr.Operator) + errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element")) + } else { + selectors = append(selectors, fields.OneTermEqualSelector(expr.Key, expr.Values[0])) } - selectors = append(selectors, fields.OneTermEqualSelector(expr.Key, expr.Values[0])) case v1.NodeSelectorOpNotIn: if len(expr.Values) != 1 { - return nil, fmt.Errorf("unexpected number of value (%d) for node field selector operator %q", - len(expr.Values), expr.Operator) + errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element")) + } else { + selectors = append(selectors, fields.OneTermNotEqualSelector(expr.Key, expr.Values[0])) } - selectors = append(selectors, fields.OneTermNotEqualSelector(expr.Key, expr.Values[0])) default: - return nil, fmt.Errorf("%q is not a valid node field selector operator", expr.Operator) + errs = append(errs, field.NotSupported(p.Child("operator"), expr.Operator, validFieldSelectorOperators)) } } + if len(errs) != 0 { + return nil, errs + } return fields.AndSelectors(selectors...), nil } @@ -260,3 +300,7 @@ 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 738e3b4b69b..6d923095f9e 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 @@ -17,14 +17,19 @@ limitations under the License. package nodeaffinity import ( - "errors" "reflect" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - apierrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +var ( + ignoreBadValue = cmpopts.IgnoreFields(field.Error{}, "BadValue") ) func TestNodeSelectorMatch(t *testing.T) { @@ -69,10 +74,18 @@ func TestNodeSelectorMatch(t *testing.T) { }}, }, }}, - wantErr: apierrors.NewAggregate([]error{ - errors.New(`unexpected number of value (2) for node field selector operator "In"`), - errors.New(`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]')`), - }), + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "nodeSelectorTerms[0].matchFields[0].values", + Detail: "must have one element", + }, + &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]')`, + }, + }.ToAggregate(), }, { name: "node matches field selector, but not labels", @@ -136,8 +149,8 @@ func TestNodeSelectorMatch(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { nodeSelector, err := NewNodeSelector(&tt.nodeSelector) - if !reflect.DeepEqual(err, tt.wantErr) { - t.Fatalf("NewNodeSelector returned error %q, want %q", err, tt.wantErr) + if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" { + t.Fatalf("NewNodeSelector returned unexpected error (-want,+got):\n%s", diff) } if tt.wantErr != nil { return @@ -197,10 +210,18 @@ func TestPreferredSchedulingTermsScore(t *testing.T) { }, }, }, - wantErr: apierrors.NewAggregate([]error{ - errors.New(`unexpected number of value (2) for node field selector operator "In"`), - errors.New(`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]')`), - }), + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "[0].matchFields[0].values", + Detail: "must have one element", + }, + &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]')`, + }, + }.ToAggregate(), }, { name: "invalid field selector but no weight, error not reported", @@ -259,8 +280,8 @@ func TestPreferredSchedulingTermsScore(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { prefSchedTerms, err := NewPreferredSchedulingTerms(tt.prefSchedTerms) - if !reflect.DeepEqual(err, tt.wantErr) { - t.Fatalf("NewPreferredSchedulingTerms returned error %q, want %q", err, tt.wantErr) + if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" { + t.Fatalf("NewPreferredSchedulingTerms returned unexpected error (-want,+got):\n%s", diff) } if tt.wantErr != nil { return @@ -287,9 +308,9 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { return out } tc := []struct { - in []v1.NodeSelectorRequirement - out labels.Selector - expectErr bool + in []v1.NodeSelectorRequirement + out labels.Selector + wantErrs field.ErrorList }{ {in: nil, out: labels.Nothing()}, {in: []v1.NodeSelectorRequirement{}, out: labels.Nothing()}, @@ -303,7 +324,7 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { Operator: v1.NodeSelectorOpExists, Values: []string{"bar", "baz"}, }}, - expectErr: true, + wantErrs: field.ErrorList{field.Invalid(field.NewPath("root").Index(0), nil, "values set must be empty for exists and does not exist")}, }, { in: []v1.NodeSelectorRequirement{{ @@ -324,12 +345,9 @@ func TestNodeSelectorRequirementsAsSelector(t *testing.T) { } for i, tc := range tc { - out, err := nodeSelectorRequirementsAsSelector(tc.in) - if err == nil && tc.expectErr { - t.Errorf("[%v]expected error but got none.", i) - } - if err != nil && !tc.expectErr { - t.Errorf("[%v]did not expect error but got: %v", i, err) + out, err := nodeSelectorRequirementsAsSelector(tc.in, field.NewPath("root")) + if diff := cmp.Diff(tc.wantErrs, err, ignoreBadValue); diff != "" { + t.Errorf("nodeSelectorRequirementsAsSelector returned unexpected error (-want,+got):\n%s", diff) } if !reflect.DeepEqual(out, tc.out) { t.Errorf("[%v]expected:\n\t%+v\nbut got:\n\t%+v", i, tc.out, out)