UPSTREAM: <carry>: Enforce label selector validation in scheduler pod…

…Affinity

Origin-commit: 3b65469338b27cebcc74c1ff75de67a6f5cfe2a9

pkg/scheduler/algorithm/predicates/predicates.go

@@ -1181,10 +1181,36 @@ func NewPodAffinityPredicate(info NodeInfo, podLister algorithm.PodLister) algor
 	return checker.InterPodAffinityMatches
 }
 
+func validatePodAffinityValues(pod *v1.Pod) error {
+	if pod.Spec.Affinity == nil {
+		return nil
+	}
+	if pod.Spec.Affinity.PodAntiAffinity != nil {
+		for _, term := range pod.Spec.Affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution {
+			if _, err := metav1.LabelSelectorAsSelector(term.LabelSelector); err != nil {
+				return err
+			}
+		}
+	}
+	if pod.Spec.Affinity.PodAffinity != nil {
+		for _, term := range pod.Spec.Affinity.PodAffinity.RequiredDuringSchedulingIgnoredDuringExecution {
+			if _, err := metav1.LabelSelectorAsSelector(term.LabelSelector); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 // InterPodAffinityMatches checks if a pod can be scheduled on the specified node with pod affinity/anti-affinity configuration.
 // First return value indicates whether a pod can be scheduled on the specified node while the second return value indicates the
 // predicate failure reasons if the pod cannot be scheduled on the specified node.
 func (c *PodAffinityChecker) InterPodAffinityMatches(pod *v1.Pod, meta algorithm.PredicateMetadata, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {
+	// first check to see if the incoming pod has valid affinity values (see https://bugzilla.redhat.com/show_bug.cgi?id=1760807)
+	if err := validatePodAffinityValues(pod); err != nil {
+		return false, []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch}, err
+	}
+
 	node := nodeInfo.Node()
 	if node == nil {
 		return false, nil, fmt.Errorf("node not found")

pkg/scheduler/algorithm/predicates/predicates_test.go

@@ -348,9 +348,9 @@ func TestPodFitsResources(t *testing.T) {
 				schedulercache.Resource{MilliCPU: 1, Memory: 1, ScalarResources: map[v1.ResourceName]int64{extendedResourceB: 1}}),
 			nodeInfo: schedulercache.NewNodeInfo(
 				newResourcePod(schedulercache.Resource{MilliCPU: 0, Memory: 0})),
-			fits: true,
+			fits:                     true,
 			ignoredExtendedResources: sets.NewString(string(extendedResourceB)),
-			test: "skip checking ignored extended resource",
+			test:                     "skip checking ignored extended resource",
 		},
 	}
 
@@ -1947,6 +1947,7 @@ func TestInterPodAffinity(t *testing.T) {
 		fits                 bool
 		test                 string
 		expectFailureReasons []algorithm.PredicateFailureReason
+		expectErr            string
 	}{
 		{
 			pod:  new(v1.Pod),
@@ -2519,6 +2520,70 @@ func TestInterPodAffinity(t *testing.T) {
 			fits: true,
 			test: "verify that PodAntiAffinity from existing pod is respected when pod has no AntiAffinity constraints. satisfy PodAntiAffinity symmetry with the existing pod",
 		},
+		{
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: podLabel,
+				},
+				Spec: v1.PodSpec{
+					Affinity: &v1.Affinity{
+						PodAffinity: &v1.PodAffinity{
+							RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{
+								{
+									LabelSelector: &metav1.LabelSelector{
+										MatchExpressions: []metav1.LabelSelectorRequirement{
+											{
+												Key:      "service",
+												Operator: metav1.LabelSelectorOpIn,
+												Values:   []string{"{{ .Release.Name }}"},
+											},
+										},
+									},
+									TopologyKey: "region",
+								},
+							},
+						},
+					},
+				},
+			},
+			expectFailureReasons: []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch},
+			node:                 &node1,
+			fits:                 false,
+			test:                 "invalid affinity fails with error",
+			expectErr:            "invalid label",
+		},
+		{
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: podLabel,
+				},
+				Spec: v1.PodSpec{
+					Affinity: &v1.Affinity{
+						PodAntiAffinity: &v1.PodAntiAffinity{
+							RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{
+								{
+									LabelSelector: &metav1.LabelSelector{
+										MatchExpressions: []metav1.LabelSelectorRequirement{
+											{
+												Key:      "service",
+												Operator: metav1.LabelSelectorOpIn,
+												Values:   []string{"{{ .Release.Name }}"},
+											},
+										},
+									},
+									TopologyKey: "region",
+								},
+							},
+						},
+					},
+				},
+			},
+			expectFailureReasons: []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch},
+			node:                 &node1,
+			fits:                 false,
+			test:                 "invalid antiaffinity fails with error",
+			expectErr:            "invalid label",
+		},
 	}
 
 	for _, test := range tests {
@@ -2537,12 +2602,21 @@ func TestInterPodAffinity(t *testing.T) {
 		nodeInfo := schedulercache.NewNodeInfo(podsOnNode...)
 		nodeInfo.SetNode(test.node)
 		nodeInfoMap := map[string]*schedulercache.NodeInfo{test.node.Name: nodeInfo}
-		fits, reasons, _ := fit.InterPodAffinityMatches(test.pod, PredicateMetadata(test.pod, nodeInfoMap), nodeInfo)
-		if !fits && !reflect.DeepEqual(reasons, test.expectFailureReasons) {
-			t.Errorf("%s: unexpected failure reasons: %v, want: %v", test.test, reasons, test.expectFailureReasons)
-		}
-		if fits != test.fits {
-			t.Errorf("%s: expected %v got %v", test.test, test.fits, fits)
+		fits, reasons, err := fit.InterPodAffinityMatches(test.pod, PredicateMetadata(test.pod, nodeInfoMap), nodeInfo)
+		if err != nil {
+			if !strings.Contains(err.Error(), test.expectErr) {
+				t.Errorf("unexpected error: %+v", err)
+			}
+		} else {
+			if len(test.expectErr) > 0 {
+				t.Errorf("did not see expected error: %s", test.expectErr)
+			}
+			if !fits && !reflect.DeepEqual(reasons, test.expectFailureReasons) {
+				t.Errorf("%s: unexpected failure reasons: %v, want: %v", test.test, reasons, test.expectFailureReasons)
+			}
+			if fits != test.fits {
+				t.Errorf("%s: expected %v got %v", test.test, test.fits, fits)
+			}
 		}
 	}
 }
@@ -2611,7 +2685,7 @@ func TestInterPodAffinityWithMultipleNodes(t *testing.T) {
 				"machine3": false,
 			},
 			nodesExpectAffinityFailureReasons: [][]algorithm.PredicateFailureReason{nil, nil, {ErrPodAffinityNotMatch, ErrPodAffinityRulesNotMatch}},
-			test: "A pod can be scheduled onto all the nodes that have the same topology key & label value with one of them has an existing pod that match the affinity rules",
+			test:                              "A pod can be scheduled onto all the nodes that have the same topology key & label value with one of them has an existing pod that match the affinity rules",
 		},
 		{
 			pod: &v1.Pod{

pkg/scheduler/algorithm/priorities/interpod_affinity.go

@@ -193,6 +193,28 @@ func (ipa *InterPodAffinity) CalculateInterPodAffinityPriority(pod *v1.Pod, node
 		nodeInfo := nodeNameToInfo[allNodeNames[i]]
 		if nodeInfo.Node() != nil {
 			if hasAffinityConstraints || hasAntiAffinityConstraints {
+				// if there aren't any existing pods to check affinity against, still check that the pod has valid
+				// affinity labels set to prevent future errors (see https://bugzilla.redhat.com/show_bug.cgi?id=1760807)
+				if len(nodeInfo.Pods()) == 0 {
+					if hasAffinityConstraints {
+						for _, term := range affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution {
+							_, err := metav1.LabelSelectorAsSelector(term.PodAffinityTerm.LabelSelector)
+							if err != nil {
+								pm.setError(err)
+								return
+							}
+						}
+					}
+					if hasAntiAffinityConstraints {
+						for _, term := range affinity.PodAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution {
+							_, err := metav1.LabelSelectorAsSelector(term.PodAffinityTerm.LabelSelector)
+							if err != nil {
+								pm.setError(err)
+								return
+							}
+						}
+					}
+				}
 				// We need to process all the nodes.
 				for _, existingPod := range nodeInfo.Pods() {
 					if err := processPod(existingPod); err != nil {

pkg/scheduler/algorithm/priorities/interpod_affinity_test.go

@@ -19,6 +19,7 @@ package priorities
 import (
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
 	"k8s.io/api/core/v1"
@@ -262,12 +263,56 @@ func TestInterPodAffinityPriority(t *testing.T) {
 		},
 	}
 
+	invalidAffinity := &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{"{{ .Release.Name }}"},
+								},
+							},
+						},
+						TopologyKey: "region",
+					},
+				},
+			},
+		},
+	}
+	invalidAntiAffinity := &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{"{{ .Release.Name }}"},
+								},
+							},
+						},
+						TopologyKey: "az",
+					},
+				},
+			},
+		},
+	}
+
 	tests := []struct {
 		pod          *v1.Pod
 		pods         []*v1.Pod
 		nodes        []*v1.Node
 		expectedList schedulerapi.HostPriorityList
 		name         string
+		expectErr    string
 	}{
 		{
 			pod: &v1.Pod{Spec: v1.PodSpec{NodeName: ""}, ObjectMeta: metav1.ObjectMeta{Labels: podLabelSecurityS1}},
@@ -507,6 +552,20 @@ func TestInterPodAffinityPriority(t *testing.T) {
 			expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: schedulerapi.MaxPriority}, {Host: "machine2", Score: 0}, {Host: "machine3", Score: schedulerapi.MaxPriority}, {Host: "machine4", Score: 0}},
 			name:         "Affinity and Anti Affinity and symmetry: considered only preferredDuringSchedulingIgnoredDuringExecution in both pod affinity & anti affinity & symmetry",
 		},
+		{
+			name: "invalid affinity fails with an error",
+			pod:  &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAffinity}},
+			nodes: []*v1.Node{
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}},
+			},
+		},
+		{
+			name: "invalid antiaffinity fails with an error",
+			pod:  &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAntiAffinity}},
+			nodes: []*v1.Node{
+				{ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}},
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -519,10 +578,13 @@ func TestInterPodAffinityPriority(t *testing.T) {
 			}
 			list, err := interPodAffinity.CalculateInterPodAffinityPriority(test.pod, nodeNameToInfo, test.nodes)
 			if err != nil {
-				t.Errorf("unexpected error: %v", err)
-			}
-			if !reflect.DeepEqual(test.expectedList, list) {
-				t.Errorf("expected \n\t%#v, \ngot \n\t%#v\n", test.expectedList, list)
+				if !strings.Contains(err.Error(), test.expectErr) {
+					t.Errorf("unexpected error: %v", err)
+				}
+			} else {
+				if !reflect.DeepEqual(test.expectedList, list) {
+					t.Errorf("expected \n\t%#v, \ngot \n\t%#v\n", test.expectedList, list)
+				}
 			}
 		})
 	}