Add LabelSelector validation in Pod Affinity/AntiAffinity Filter and Score plugins

The lack of this validation on incoming pods causes unpredictable cluster outcomes
when later calculating affinity results against existing pods (see #92714). This fix
quickly addresses the main source where these problems should be caught.

It is unfortunately difficult to add this validation directly to the API server due
to the fact that it may break migrations with existing pods that fail this check. This
is a compromise to address the current issue.

pkg/scheduler/framework/plugins/interpodaffinity/filtering.go

@@ -251,6 +251,9 @@ func (pl *InterPodAffinity) PreFilter(ctx context.Context, cycleState *framework
 	}
 
 	podInfo := framework.NewPodInfo(pod)
+	if podInfo.ParseError != nil {
+		return framework.NewStatus(framework.UnschedulableAndUnresolvable, fmt.Sprintf("parsing pod: %+v", podInfo.ParseError))
+	}
 
 	// existingPodAntiAffinityMap will be used later for efficient check on existing pods' anti-affinity
 	existingPodAntiAffinityMap := getTPMapMatchingExistingAntiAffinity(pod, havePodsWithAffinityNodes)

pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go

@@ -19,6 +19,7 @@ package interpodaffinity
 import (
 	"context"
 	"reflect"
+	"strings"
 	"testing"
 
 	"k8s.io/api/core/v1"
@@ -772,6 +773,80 @@ func TestRequiredAffinitySingleNode(t *testing.T) {
 			),
 			name: "PodAntiAffinity symmetry check b2: incoming pod and existing pod partially match each other on AffinityTerms",
 		},
+		{
+			name: "PodAffinity fails PreFilter with an invalid affinity label syntax",
+			pod: createPodWithAffinityTerms(defaultNamespace, "", podLabel,
+				[]v1.PodAffinityTerm{
+					{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "service",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"{{.bad-value.}}"},
+								},
+							},
+						},
+						TopologyKey: "region",
+					},
+				},
+				[]v1.PodAffinityTerm{
+					{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "service",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"antivirusscan", "value2"},
+								},
+							},
+						},
+						TopologyKey: "node",
+					},
+				}),
+			node: &node1,
+			wantStatus: framework.NewStatus(
+				framework.UnschedulableAndUnresolvable,
+				"invalid label value",
+			),
+		},
+		{
+			name: "PodAntiAffinity fails PreFilter with an invalid antiaffinity label syntax",
+			pod: createPodWithAffinityTerms(defaultNamespace, "", podLabel,
+				[]v1.PodAffinityTerm{
+					{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "service",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"foo"},
+								},
+							},
+						},
+						TopologyKey: "region",
+					},
+				},
+				[]v1.PodAffinityTerm{
+					{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "service",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"{{.bad-value.}}"},
+								},
+							},
+						},
+						TopologyKey: "node",
+					},
+				}),
+			node: &node1,
+			wantStatus: framework.NewStatus(
+				framework.UnschedulableAndUnresolvable,
+				"invalid label value",
+			),
+		},
 	}
 
 	for _, test := range tests {
@@ -783,12 +858,15 @@ func TestRequiredAffinitySingleNode(t *testing.T) {
 			state := framework.NewCycleState()
 			preFilterStatus := p.PreFilter(context.Background(), state, test.pod)
 			if !preFilterStatus.IsSuccess() {
-				t.Errorf("prefilter failed with status: %v", preFilterStatus)
-			}
-			nodeInfo := mustGetNodeInfo(t, snapshot, test.node.Name)
-			gotStatus := p.Filter(context.Background(), state, test.pod, nodeInfo)
-			if !reflect.DeepEqual(gotStatus, test.wantStatus) {
-				t.Errorf("status does not match: %v, want: %v", gotStatus, test.wantStatus)
+				if !strings.Contains(preFilterStatus.Message(), test.wantStatus.Message()) {
+					t.Errorf("prefilter failed with status: %v", preFilterStatus)
+				}
+			} else {
+				nodeInfo := mustGetNodeInfo(t, snapshot, test.node.Name)
+				gotStatus := p.Filter(context.Background(), state, test.pod, nodeInfo)
+				if !reflect.DeepEqual(gotStatus, test.wantStatus) {
+					t.Errorf("status does not match: %v, want: %v", gotStatus, test.wantStatus)
+				}
 			}
 		})
 	}

pkg/scheduler/framework/plugins/interpodaffinity/scoring.go

@@ -160,9 +160,15 @@ func (pl *InterPodAffinity) PreScore(
 		}
 	}
 
+	podInfo := framework.NewPodInfo(pod)
+	if podInfo.ParseError != nil {
+		// Ideally we never reach here, because errors will be caught by PreFilter
+		return framework.NewStatus(framework.Error, fmt.Sprintf("parsing pod: %+v", podInfo.ParseError))
+	}
+
 	state := &preScoreState{
 		topologyScore: make(map[string]map[string]int64),
-		podInfo:       framework.NewPodInfo(pod),
+		podInfo:       podInfo,
 	}
 
 	topoScores := make([]scoreMap, len(allNodes))

pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go

@@ -19,6 +19,7 @@ package interpodaffinity
 import (
 	"context"
 	"reflect"
+	"strings"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -252,12 +253,56 @@ func TestPreferredAffinity(t *testing.T) {
 		},
 	}
 
+	invalidAffinityLabels := &v1.Affinity{
+		PodAffinity: &v1.PodAffinity{
+			PreferredDuringSchedulingIgnoredDuringExecution: []v1.WeightedPodAffinityTerm{
+				{
+					Weight: 8,
+					PodAffinityTerm: v1.PodAffinityTerm{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "security",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"{{.bad-value.}}"},
+								},
+							},
+						},
+						TopologyKey: "region",
+					},
+				},
+			},
+		},
+	}
+	invalidAntiAffinityLabels := &v1.Affinity{
+		PodAntiAffinity: &v1.PodAntiAffinity{
+			PreferredDuringSchedulingIgnoredDuringExecution: []v1.WeightedPodAffinityTerm{
+				{
+					Weight: 5,
+					PodAffinityTerm: v1.PodAffinityTerm{
+						LabelSelector: &metav1.LabelSelector{
+							MatchExpressions: []metav1.LabelSelectorRequirement{
+								{
+									Key:      "security",
+									Operator: metav1.LabelSelectorOpIn,
+									Values:   []string{"{{.bad-value.}}"},
+								},
+							},
+						},
+						TopologyKey: "az",
+					},
+				},
+			},
+		},
+	}
+
 	tests := []struct {
 		pod          *v1.Pod
 		pods         []*v1.Pod
 		nodes        []*v1.Node
 		expectedList framework.NodeScoreList
 		name         string
+		wantStatus   *framework.Status
 	}{
 		{
 			pod: &v1.Pod{Spec: v1.PodSpec{NodeName: ""}, ObjectMeta: metav1.ObjectMeta{Labels: podLabelSecurityS1}},
@@ -513,6 +558,24 @@ func TestPreferredAffinity(t *testing.T) {
 			expectedList: []framework.NodeScore{{Name: "machine1", Score: framework.MaxNodeScore}, {Name: "machine2", Score: framework.MaxNodeScore}},
 			name:         "Avoid panic when partial nodes in a topology don't have pods with affinity",
 		},
+		{
+			name:       "invalid Affinity fails PreScore",
+			pod:        &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAffinityLabels}},
+			wantStatus: framework.NewStatus(framework.Error, "invalid label value"),
+			nodes: []*v1.Node{
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}},
+			},
+		},
+		{
+			name:       "invalid AntiAffinity fails PreScore",
+			pod:        &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAntiAffinityLabels}},
+			wantStatus: framework.NewStatus(framework.Error, "invalid label value"),
+			nodes: []*v1.Node{
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}},
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -527,25 +590,28 @@ func TestPreferredAffinity(t *testing.T) {
 
 			status := p.PreScore(context.Background(), state, test.pod, test.nodes)
 			if !status.IsSuccess() {
-				t.Errorf("unexpected error: %v", status)
-			}
-			var gotList framework.NodeScoreList
-			for _, n := range test.nodes {
-				nodeName := n.ObjectMeta.Name
-				score, status := p.Score(context.Background(), state, test.pod, nodeName)
+				if !strings.Contains(status.Message(), test.wantStatus.Message()) {
+					t.Errorf("unexpected error: %v", status)
+				}
+			} else {
+				var gotList framework.NodeScoreList
+				for _, n := range test.nodes {
+					nodeName := n.ObjectMeta.Name
+					score, status := p.Score(context.Background(), state, test.pod, nodeName)
+					if !status.IsSuccess() {
+						t.Errorf("unexpected error: %v", status)
+					}
+					gotList = append(gotList, framework.NodeScore{Name: nodeName, Score: score})
+				}
+
+				status = p.ScoreExtensions().NormalizeScore(context.Background(), state, test.pod, gotList)
 				if !status.IsSuccess() {
 					t.Errorf("unexpected error: %v", status)
 				}
-				gotList = append(gotList, framework.NodeScore{Name: nodeName, Score: score})
-			}
 
-			status = p.ScoreExtensions().NormalizeScore(context.Background(), state, test.pod, gotList)
-			if !status.IsSuccess() {
-				t.Errorf("unexpected error: %v", status)
-			}
-
-			if !reflect.DeepEqual(test.expectedList, gotList) {
-				t.Errorf("expected:\n\t%+v,\ngot:\n\t%+v", test.expectedList, gotList)
+				if !reflect.DeepEqual(test.expectedList, gotList) {
+					t.Errorf("expected:\n\t%+v,\ngot:\n\t%+v", test.expectedList, gotList)
+				}
 			}
 
 		})

pkg/scheduler/framework/v1alpha1/types.go

@@ -73,6 +73,7 @@ type PodInfo struct {
 	RequiredAntiAffinityTerms  []AffinityTerm
 	PreferredAffinityTerms     []WeightedAffinityTerm
 	PreferredAntiAffinityTerms []WeightedAffinityTerm
+	ParseError                 error
 }
 
 // AffinityTerm is a processed version of v1.PodAffinityTerm.
@@ -88,53 +89,50 @@ type WeightedAffinityTerm struct {
 	Weight int32
 }
 
-func newAffinityTerm(pod *v1.Pod, term *v1.PodAffinityTerm) *AffinityTerm {
+func newAffinityTerm(pod *v1.Pod, term *v1.PodAffinityTerm) (*AffinityTerm, error) {
 	namespaces := schedutil.GetNamespacesFromPodAffinityTerm(pod, term)
 	selector, err := metav1.LabelSelectorAsSelector(term.LabelSelector)
 	if err != nil {
-		klog.Errorf("Cannot process label selector: %v", err)
-		return nil
+		return nil, err
 	}
-	return &AffinityTerm{Namespaces: namespaces, Selector: selector, TopologyKey: term.TopologyKey}
+	return &AffinityTerm{Namespaces: namespaces, Selector: selector, TopologyKey: term.TopologyKey}, nil
 }
 
 // getAffinityTerms receives a Pod and affinity terms and returns the namespaces and
 // selectors of the terms.
-func getAffinityTerms(pod *v1.Pod, v1Terms []v1.PodAffinityTerm) []AffinityTerm {
+func getAffinityTerms(pod *v1.Pod, v1Terms []v1.PodAffinityTerm) ([]AffinityTerm, error) {
 	if v1Terms == nil {
-		return nil
+		return nil, nil
 	}
 
 	var terms []AffinityTerm
 	for _, term := range v1Terms {
-		t := newAffinityTerm(pod, &term)
-		if t == nil {
-			// We get here if the label selector failed to process, this is not supposed
-			// to happen because the pod should have been validated by the api server.
-			return nil
+		t, err := newAffinityTerm(pod, &term)
+		if err != nil {
+			// We get here if the label selector failed to process
+			return nil, err
 		}
 		terms = append(terms, *t)
 	}
-	return terms
+	return terms, nil
 }
 
 // getWeightedAffinityTerms returns the list of processed affinity terms.
-func getWeightedAffinityTerms(pod *v1.Pod, v1Terms []v1.WeightedPodAffinityTerm) []WeightedAffinityTerm {
+func getWeightedAffinityTerms(pod *v1.Pod, v1Terms []v1.WeightedPodAffinityTerm) ([]WeightedAffinityTerm, error) {
 	if v1Terms == nil {
-		return nil
+		return nil, nil
 	}
 
 	var terms []WeightedAffinityTerm
 	for _, term := range v1Terms {
-		t := newAffinityTerm(pod, &term.PodAffinityTerm)
-		if t == nil {
-			// We get here if the label selector failed to process, this is not supposed
-			// to happen because the pod should have been validated by the api server.
-			return nil
+		t, err := newAffinityTerm(pod, &term.PodAffinityTerm)
+		if err != nil {
+			// We get here if the label selector failed to process
+			return nil, err
 		}
 		terms = append(terms, WeightedAffinityTerm{AffinityTerm: *t, Weight: term.Weight})
 	}
-	return terms
+	return terms, nil
 }
 
 // NewPodInfo return a new PodInfo
@@ -150,12 +148,32 @@ func NewPodInfo(pod *v1.Pod) *PodInfo {
 		}
 	}
 
+	// Attempt to parse the affinity terms
+	var parseErr error
+	requiredAffinityTerms, err := getAffinityTerms(pod, schedutil.GetPodAffinityTerms(pod.Spec.Affinity))
+	if err != nil {
+		parseErr = fmt.Errorf("requiredAffinityTerms: %w", err)
+	}
+	requiredAntiAffinityTerms, err := getAffinityTerms(pod, schedutil.GetPodAntiAffinityTerms(pod.Spec.Affinity))
+	if err != nil {
+		parseErr = fmt.Errorf("requiredAntiAffinityTerms: %w", err)
+	}
+	weightedAffinityTerms, err := getWeightedAffinityTerms(pod, preferredAffinityTerms)
+	if err != nil {
+		parseErr = fmt.Errorf("preferredAffinityTerms: %w", err)
+	}
+	weightedAntiAffinityTerms, err := getWeightedAffinityTerms(pod, preferredAntiAffinityTerms)
+	if err != nil {
+		parseErr = fmt.Errorf("preferredAntiAffinityTerms: %w", err)
+	}
+
 	return &PodInfo{
 		Pod:                        pod,
-		RequiredAffinityTerms:      getAffinityTerms(pod, schedutil.GetPodAffinityTerms(pod.Spec.Affinity)),
-		RequiredAntiAffinityTerms:  getAffinityTerms(pod, schedutil.GetPodAntiAffinityTerms(pod.Spec.Affinity)),
-		PreferredAffinityTerms:     getWeightedAffinityTerms(pod, preferredAffinityTerms),
-		PreferredAntiAffinityTerms: getWeightedAffinityTerms(pod, preferredAntiAffinityTerms),
+		RequiredAffinityTerms:      requiredAffinityTerms,
+		RequiredAntiAffinityTerms:  requiredAntiAffinityTerms,
+		PreferredAffinityTerms:     weightedAffinityTerms,
+		PreferredAntiAffinityTerms: weightedAntiAffinityTerms,
+		ParseError:                 parseErr,
 	}
 }