Merge pull request #96522 from alculquicondor/validate-added-node-affinity

Return field.Errors from node affinity parsing

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")
+	path := field.NewPath("addedAffinity")
-	var allErrs field.ErrorList
+	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.
+			errs = append(errs, err)
-			allErrs = append(allErrs, field.Invalid(f.Child("requiredDuringSchedulingIgnoredDuringExecution"), affinity.RequiredDuringSchedulingIgnoredDuringExecution, err.Error()))
 		}
 	}
 	// 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.
+			errs = append(errs, err)
-			allErrs = append(allErrs, field.Invalid(f.Child("preferredDuringSchedulingIgnoredDuringExecution"), affinity.PreferredDuringSchedulingIgnoredDuringExecution, err.Error()))
 		}
 	}
-	return allErrs.ToAggregate()
+	return errors.Flatten(errors.NewAggregate(errs))
 }

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.Error{
-				field.Invalid(field.NewPath("addedAffinity", "preferredDuringSchedulingIgnoredDuringExecution"), nil, ""),
+					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)
 			}
 		})

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

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",
     ],
 )

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.
+// WithPath sets a field.Path to be used as root for parse errors.
-func NewNodeSelector(ns *v1.NodeSelector) (*NodeSelector, error) {
+func WithPath(p *field.Path) ParseOption {
-	lazy := NewLazyErrorNodeSelector(ns)
+	return func(o *parseOptions) {
-	var errs []error
+		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 {
+		if len(term.parseErrs) > 0 {
-			errs = append(errs, term.parseErr)
+			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)
+		match, tErrs := term.match(nodeLabels, nodeFields)
-		if err != nil {
+		if len(tErrs) > 0 {
-			errs = append(errs, term.parseErr)
+			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) {
+func NewPreferredSchedulingTerms(terms []v1.PreferredSchedulingTerm, opts ...ParseOption) (*PreferredSchedulingTerms, error) {
-	var errs []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 {
+		if len(parsedTerm.parseErrs) > 0 {
-			errs = append(errs, parsedTerm.parseErr)
+			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)
+		p := path.Child("matchExpressions")
-		if parsedTerm.parseErr != nil {
+		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) {
+func (t *nodeSelectorTerm) match(nodeLabels labels.Set, nodeFields fields.Set) (bool, field.ErrorList) {
-	if t.parseErr != nil {
+	if t.parseErrs != nil {
-		return false, t.parseErr
+		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",
+				errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element"))
-					len(expr.Values), expr.Operator)
+			} 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",
+				errs = append(errs, field.Invalid(p.Child("values"), expr.Values, "must have one element"))
-					len(expr.Values), expr.Operator)
+			} 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
+}

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{
+			wantErr: field.ErrorList{
-				errors.New(`unexpected number of value (2) for node field selector operator "In"`),
+				&field.Error{
-				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]')`),
+					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) {
+			if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" {
-				t.Fatalf("NewNodeSelector returned error %q, want %q", err, tt.wantErr)
+				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{
+			wantErr: field.ErrorList{
-				errors.New(`unexpected number of value (2) for node field selector operator "In"`),
+				&field.Error{
-				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]')`),
+					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) {
+			if diff := cmp.Diff(tt.wantErr, err, ignoreBadValue); diff != "" {
-				t.Fatalf("NewPreferredSchedulingTerms returned error %q, want %q", err, tt.wantErr)
+				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
+		in       []v1.NodeSelectorRequirement
-		out       labels.Selector
+		out      labels.Selector
-		expectErr bool
+		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)
+		out, err := nodeSelectorRequirementsAsSelector(tc.in, field.NewPath("root"))
-		if err == nil && tc.expectErr {
+		if diff := cmp.Diff(tc.wantErrs, err, ignoreBadValue); diff != "" {
-			t.Errorf("[%v]expected error but got none.", i)
+			t.Errorf("nodeSelectorRequirementsAsSelector returned unexpected error (-want,+got):\n%s", diff)
-		}
-		if err != nil && !tc.expectErr {
-			t.Errorf("[%v]did not expect error but got: %v", i, err)
 		}
 		if !reflect.DeepEqual(out, tc.out) {
 			t.Errorf("[%v]expected:\n\t%+v\nbut got:\n\t%+v", i, tc.out, out)