make labels.NewRequirement returns aggregated field.ErrorList, make n…

…odeaffinity parsing function use it

Signed-off-by: Ling Samuel <lingsamuelgrace@gmail.com>

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)
 		}

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)
 			}
 		})
 	}

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.}}"`,
 			),
 		},
 	}

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}},

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

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

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...))
 }