github/kubernetes
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #116535 from denkensk/fix-match

Browse Source 
feat: forbid to set matchLabelKeys when labelSelector isn’t set in topologySpreadConstraints
This commit is contained in:
Kubernetes Prow Robot
2023-03-14 14:13:04 -07:00
committed by GitHub
parent 27e23bad7d 199c37acef
commit f2e1a67c05
13 changed files with 125 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
pkg/apis/core/types.go
View File

@@ -5972,8 +5972,12 @@ type TopologySpreadConstraint struct {
// spreading will be calculated. The keys are used to lookup values from the
// incoming pod labels, those key-value labels are ANDed with labelSelector
// to select the group of existing pods over which spreading will be calculated
// for the incoming pod. Keys that don't exist in the incoming pod labels will
// for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector.
// MatchLabelKeys cannot be set when LabelSelector isn't set.
// Keys that don't exist in the incoming pod labels will
// be ignored. A null or empty list means only match against labelSelector.
//
// This is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).
// +listType=atomic
// +optional
MatchLabelKeys []string

5
pkg/apis/core/validation/validation.go
View File

@@ -6964,7 +6964,9 @@ func validateMatchLabelKeys(fldPath *field.Path, matchLabelKeys []string, labelS
return nil
}
var allErrs field.ErrorList
labelSelectorKeys := sets.String{}
if labelSelector != nil {
for key := range labelSelector.MatchLabels {
labelSelectorKeys.Insert(key)
@@ -6972,9 +6974,10 @@ func validateMatchLabelKeys(fldPath *field.Path, matchLabelKeys []string, labelS
for _, matchExpression := range labelSelector.MatchExpressions {
labelSelectorKeys.Insert(matchExpression.Key)
}
} else {
allErrs = append(allErrs, field.Forbidden(fldPath, "must not be specified when labelSelector is not set"))
}
allErrs := field.ErrorList{}
for i, key := range matchLabelKeys {
allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(key, fldPath.Index(i))...)
if labelSelectorKeys.Has(key) {

17
pkg/apis/core/validation/validation_test.go
View File

@@ -22091,11 +22091,12 @@ func TestValidateTopologySpreadConstraints(t *testing.T) {
{
MaxSkew: 1,
TopologyKey: "k8s.io/zone",
LabelSelector: &metav1.LabelSelector{},
WhenUnsatisfiable: core.DoNotSchedule,
MatchLabelKeys: []string{"/simple"},
},
},
wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")},
wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")},
},
{
name: "key exists in both matchLabelKeys and labelSelector",
@@ -22116,7 +22117,19 @@ func TestValidateTopologySpreadConstraints(t *testing.T) {
},
},
},
wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")},
wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")},
},
{
name: "key in MatchLabelKeys is forbidden to be specified when labelSelector is not set",
constraints: []core.TopologySpreadConstraint{
{
MaxSkew: 1,
TopologyKey: "k8s.io/zone",
WhenUnsatisfiable: core.DoNotSchedule,
MatchLabelKeys: []string{"foo"},
},
},
wantFieldErrors: field.ErrorList{field.Forbidden(fieldPathMatchLabelKeys, "must not be specified when labelSelector is not set")},
},
{
name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false",

2
pkg/generated/openapi/zz_generated.openapi.go generated
View File

@@ -27285,7 +27285,7 @@ func schema_k8sio_api_core_v1_TopologySpreadConstraint(ref common.ReferenceCallb
},
},
SchemaProps: spec.SchemaProps{
Description: "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.",
Description: "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).",
Type: []string{"array"},
Items: &spec.SchemaOrArray{
Schema: &spec.Schema{

12
pkg/scheduler/framework/plugins/podtopologyspread/common.go
View File

@@ -135,10 +135,14 @@ func (pl *PodTopologySpread) filterTopologySpreadConstraints(constraints []v1.To
func mergeLabelSetWithSelector(matchLabels labels.Set, s labels.Selector) labels.Selector {
mergedSelector := labels.SelectorFromSet(matchLabels)
if requirements, ok := s.Requirements(); ok {
for _, r := range requirements {
mergedSelector = mergedSelector.Add(r)
}
requirements, ok := s.Requirements()
if !ok {
return s
}
for _, r := range requirements {
mergedSelector = mergedSelector.Add(r)
}
return mergedSelector

77
pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go
View File

@@ -1299,6 +1299,83 @@ func TestPreFilterState(t *testing.T) {
},
enableMatchLabelKeys: true,
},
{
name: "key in matchLabelKeys is ignored when LabelSelector is nil when feature gate enabled",
pod: st.MakePod().Name("p").Label("foo", "").
SpreadConstraint(1, "zone", v1.DoNotSchedule, nil, nil, nil, nil, []string{"bar"}).
Obj(),
nodes: []*v1.Node{
st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
},
existingPods: []*v1.Pod{
st.MakePod().Name("p-a1").Node("node-a").Label("foo", "a").Obj(),
st.MakePod().Name("p-a2").Node("node-a").Label("foo", "a").Obj(),
st.MakePod().Name("p-b1").Node("node-b").Label("foo", "a").Obj(),
st.MakePod().Name("p-y1").Node("node-y").Label("foo", "a").Obj(),
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "a").Obj(),
},
want: &preFilterState{
Constraints: []topologySpreadConstraint{
{
MaxSkew: 1,
TopologyKey: "zone",
Selector: mustConvertLabelSelectorAsSelector(t, nil),
MinDomains: 1,
NodeAffinityPolicy: v1.NodeInclusionPolicyHonor,
NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore,
},
},
TpKeyToCriticalPaths: map[string]*criticalPaths{
"zone": {{"zone2", 0}, {"zone1", 0}},
},
TpPairToMatchNum: map[topologyPair]int{
{key: "zone", value: "zone1"}: 0,
{key: "zone", value: "zone2"}: 0,
},
},
enableMatchLabelKeys: true,
},
{
name: "no pod is matched when LabelSelector is nil when feature gate disabled",
pod: st.MakePod().Name("p").Label("foo", "").
SpreadConstraint(1, "zone", v1.DoNotSchedule, nil, nil, nil, nil, []string{"bar"}).
Obj(),
nodes: []*v1.Node{
st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
},
existingPods: []*v1.Pod{
st.MakePod().Name("p-a1").Node("node-a").Label("foo", "a").Obj(),
st.MakePod().Name("p-a2").Node("node-a").Label("foo", "a").Obj(),
st.MakePod().Name("p-b1").Node("node-b").Label("foo", "a").Obj(),
st.MakePod().Name("p-y1").Node("node-y").Label("foo", "a").Obj(),
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "a").Obj(),
},
want: &preFilterState{
Constraints: []topologySpreadConstraint{
{
MaxSkew: 1,
TopologyKey: "zone",
Selector: mustConvertLabelSelectorAsSelector(t, nil),
MinDomains: 1,
NodeAffinityPolicy: v1.NodeInclusionPolicyHonor,
NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore,
},
},
TpKeyToCriticalPaths: map[string]*criticalPaths{
"zone": {{"zone2", 0}, {"zone1", 0}},
},
TpPairToMatchNum: map[topologyPair]int{
{key: "zone", value: "zone1"}: 0,
{key: "zone", value: "zone2"}: 0,
},
},
},
{
name: "key in matchLabelKeys is ignored when it isn't exist in pod.labels",
pod: st.MakePod().Name("p").Label("foo", "").