Merge pull request #93660 from damemi/1.19-affinity-validation

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

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