From 012245c5b9c1e535067e0ee3c39a00d3b8d7f253 Mon Sep 17 00:00:00 2001 From: Mike Dame Date: Mon, 3 Aug 2020 15:20:24 -0400 Subject: [PATCH] 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. --- .../plugins/interpodaffinity/filtering.go | 3 + .../interpodaffinity/filtering_test.go | 90 +++++++++++++++-- .../plugins/interpodaffinity/scoring.go | 8 +- .../plugins/interpodaffinity/scoring_test.go | 96 ++++++++++++++++--- pkg/scheduler/framework/v1alpha1/types.go | 66 ++++++++----- 5 files changed, 217 insertions(+), 46 deletions(-) diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go b/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go index 1a1bf641b93..596f1b95df3 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go +++ b/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) diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go index c4879d39b7c..99ecd7a84e9 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go +++ b/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) + } } }) } diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go b/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go index 5848121dee6..c27ae5f41ef 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go +++ b/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)) diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go index 766e4fc35f1..0defe8a2f5d 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go +++ b/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) + } } }) diff --git a/pkg/scheduler/framework/v1alpha1/types.go b/pkg/scheduler/framework/v1alpha1/types.go index 1e464bf9c97..915c5d1ec25 100644 --- a/pkg/scheduler/framework/v1alpha1/types.go +++ b/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, } }