From c0af728f43025bb47b209ebbb45604f9c0424354 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 14 Jan 2022 11:41:07 -0500 Subject: [PATCH] Handle invalid selectors properly --- pkg/controller/deployment/util/deployment_util.go | 3 ++- pkg/controller/replicaset/replica_set.go | 5 +++-- pkg/controller/volume/persistentvolume/util/util.go | 1 - pkg/printers/internalversion/printers.go | 8 +++++--- pkg/registry/core/pod/storage/eviction.go | 1 + pkg/scheduler/framework/fake/listers.go | 6 ++++-- .../plugins/defaultpreemption/default_preemption.go | 1 + pkg/scheduler/framework/plugins/volumebinding/binder.go | 1 - .../client-go/listers/apps/v1/daemonset_expansion.go | 7 ++++--- .../client-go/listers/apps/v1/replicaset_expansion.go | 3 ++- .../client-go/listers/apps/v1/statefulset_expansion.go | 3 ++- .../listers/apps/v1beta1/statefulset_expansion.go | 3 ++- .../client-go/listers/apps/v1beta2/daemonset_expansion.go | 7 ++++--- .../listers/apps/v1beta2/replicaset_expansion.go | 3 ++- .../listers/apps/v1beta2/statefulset_expansion.go | 3 ++- .../k8s.io/client-go/listers/batch/v1/job_expansion.go | 6 +++++- .../listers/extensions/v1beta1/daemonset_expansion.go | 7 ++++--- .../listers/extensions/v1beta1/replicaset_expansion.go | 3 ++- .../listers/policy/v1/poddisruptionbudget_expansion.go | 3 +-- .../policy/v1beta1/poddisruptionbudget_expansion.go | 4 +--- 20 files changed, 47 insertions(+), 31 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 2eb94ff59a4..d06d392e97c 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -900,7 +900,8 @@ func GetDeploymentsForReplicaSet(deploymentLister appslisters.DeploymentLister, for _, d := range dList { selector, err := metav1.LabelSelectorAsSelector(d.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid label selector: %v", err) + // This object has an invalid selector, it does not match the replicaset + continue } // If a deployment with a nil or empty selector creeps in, it should match nothing, not everything. if selector.Empty() || !selector.Matches(labels.Set(rs.Labels)) { diff --git a/pkg/controller/replicaset/replica_set.go b/pkg/controller/replicaset/replica_set.go index 3419ce151e1..7298d887f1b 100644 --- a/pkg/controller/replicaset/replica_set.go +++ b/pkg/controller/replicaset/replica_set.go @@ -668,7 +668,7 @@ func (rsc *ReplicaSetController) syncReplicaSet(ctx context.Context, key string) rsNeedsSync := rsc.expectations.SatisfiedExpectations(key) selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - utilruntime.HandleError(fmt.Errorf("error converting pod selector to selector: %v", err)) + utilruntime.HandleError(fmt.Errorf("error converting pod selector to selector for rs %v/%v: %v", namespace, name, err)) return nil } @@ -774,7 +774,8 @@ func (rsc *ReplicaSetController) getIndirectlyRelatedPods(rs *apps.ReplicaSet) ( for _, relatedRS := range rsc.getReplicaSetsWithSameController(rs) { selector, err := metav1.LabelSelectorAsSelector(relatedRS.Spec.Selector) if err != nil { - return nil, err + // This object has an invalid selector, it does not match any pods + continue } pods, err := rsc.podLister.Pods(relatedRS.Namespace).List(selector) if err != nil { diff --git a/pkg/controller/volume/persistentvolume/util/util.go b/pkg/controller/volume/persistentvolume/util/util.go index 754c79adaed..eec8f889ab3 100644 --- a/pkg/controller/volume/persistentvolume/util/util.go +++ b/pkg/controller/volume/persistentvolume/util/util.go @@ -197,7 +197,6 @@ func FindMatchingVolume( if claim.Spec.Selector != nil { internalSelector, err := metav1.LabelSelectorAsSelector(claim.Spec.Selector) if err != nil { - // should be unreachable code due to validation return nil, fmt.Errorf("error creating internal label selector for claim: %v: %v", claimToClaimKey(claim), err) } selector = internalSelector diff --git a/pkg/printers/internalversion/printers.go b/pkg/printers/internalversion/printers.go index 857d20d17e2..1e557d2ee6d 100644 --- a/pkg/printers/internalversion/printers.go +++ b/pkg/printers/internalversion/printers.go @@ -1996,14 +1996,16 @@ func printDeployment(obj *apps.Deployment, options printers.GenerateOptions) ([] age := translateTimestampSince(obj.CreationTimestamp) containers := obj.Spec.Template.Spec.Containers selector, err := metav1.LabelSelectorAsSelector(obj.Spec.Selector) + selectorString := "" if err != nil { - // this shouldn't happen if LabelSelector passed validation - return nil, err + selectorString = "" + } else { + selectorString = selector.String() } row.Cells = append(row.Cells, obj.Name, fmt.Sprintf("%d/%d", int64(readyReplicas), int64(desiredReplicas)), int64(updatedReplicas), int64(availableReplicas), age) if options.Wide { containers, images := layoutContainerCells(containers) - row.Cells = append(row.Cells, containers, images, selector.String()) + row.Cells = append(row.Cells, containers, images, selectorString) } return []metav1.TableRow{row}, nil } diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go index d1ed12fb59b..39f2e041673 100644 --- a/pkg/registry/core/pod/storage/eviction.go +++ b/pkg/registry/core/pod/storage/eviction.go @@ -385,6 +385,7 @@ func (r *EvictionREST) getPodDisruptionBudgets(ctx context.Context, pod *api.Pod } selector, err := metav1.LabelSelectorAsSelector(pdb.Spec.Selector) if err != nil { + // This object has an invalid selector, it does not match the pod continue } // If a PDB with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/pkg/scheduler/framework/fake/listers.go b/pkg/scheduler/framework/fake/listers.go index 1b4482fa3d2..d29658c5b55 100644 --- a/pkg/scheduler/framework/fake/listers.go +++ b/pkg/scheduler/framework/fake/listers.go @@ -125,7 +125,8 @@ func (f ReplicaSetLister) GetPodReplicaSets(pod *v1.Pod) (rss []*appsv1.ReplicaS } selector, err = metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - return + // This object has an invalid selector, it does not match the pod + continue } if selector.Matches(labels.Set(pod.Labels)) { @@ -164,7 +165,8 @@ func (f StatefulSetLister) GetPodStatefulSets(pod *v1.Pod) (sss []*appsv1.Statef } selector, err = metav1.LabelSelectorAsSelector(ss.Spec.Selector) if err != nil { - return + // This object has an invalid selector, it does not match the pod + continue } if selector.Matches(labels.Set(pod.Labels)) { sss = append(sss, ss) diff --git a/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go b/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go index f96b0d1acc4..9282626a0f3 100644 --- a/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go +++ b/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption.go @@ -277,6 +277,7 @@ func filterPodsWithPDBViolation(podInfos []*framework.PodInfo, pdbs []*policy.Po } selector, err := metav1.LabelSelectorAsSelector(pdb.Spec.Selector) if err != nil { + // This object has an invalid selector, it does not match the pod continue } // A PDB with a nil or empty selector matches nothing. diff --git a/pkg/scheduler/framework/plugins/volumebinding/binder.go b/pkg/scheduler/framework/plugins/volumebinding/binder.go index df1000e8e37..7e4405423f3 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/binder.go +++ b/pkg/scheduler/framework/plugins/volumebinding/binder.go @@ -990,7 +990,6 @@ func (b *volumeBinder) nodeHasAccess(node *v1.Node, capacity *storagev1beta1.CSI // Only matching by label is supported. selector, err := metav1.LabelSelectorAsSelector(capacity.NodeTopology) if err != nil { - // This should never happen because NodeTopology must be valid. klog.ErrorS(err, "Unexpected error converting to a label selector", "nodeTopology", capacity.NodeTopology) return false } diff --git a/staging/src/k8s.io/client-go/listers/apps/v1/daemonset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1/daemonset_expansion.go index 83435561a14..667d6fb88ea 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1/daemonset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1/daemonset_expansion.go @@ -60,8 +60,8 @@ func (s *daemonSetLister) GetPodDaemonSets(pod *v1.Pod) ([]*apps.DaemonSet, erro } selector, err = metav1.LabelSelectorAsSelector(daemonSet.Spec.Selector) if err != nil { - // this should not happen if the DaemonSet passed validation - return nil, err + // This object has an invalid selector, it does not match the pod + continue } // If a daemonSet with a nil or empty selector creeps in, it should match nothing, not everything. @@ -96,7 +96,8 @@ func (s *daemonSetLister) GetHistoryDaemonSets(history *apps.ControllerRevision) for _, ds := range list { selector, err := metav1.LabelSelectorAsSelector(ds.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid label selector: %v", err) + // This object has an invalid selector, it does not match the history + continue } // If a DaemonSet with a nil or empty selector creeps in, it should match nothing, not everything. if selector.Empty() || !selector.Matches(labels.Set(history.Labels)) { diff --git a/staging/src/k8s.io/client-go/listers/apps/v1/replicaset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1/replicaset_expansion.go index 675e615aecc..8e093de0a01 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1/replicaset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1/replicaset_expansion.go @@ -55,7 +55,8 @@ func (s *replicaSetLister) GetPodReplicaSets(pod *v1.Pod) ([]*apps.ReplicaSet, e } selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a ReplicaSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/apps/v1/statefulset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1/statefulset_expansion.go index b4912976b69..e79f8a2b46a 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1/statefulset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1/statefulset_expansion.go @@ -59,7 +59,8 @@ func (s *statefulSetLister) GetPodStatefulSets(pod *v1.Pod) ([]*apps.StatefulSet } selector, err = metav1.LabelSelectorAsSelector(ps.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a StatefulSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/apps/v1beta1/statefulset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1beta1/statefulset_expansion.go index 0741792ac7a..7d2c4d9b07a 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1beta1/statefulset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1beta1/statefulset_expansion.go @@ -59,7 +59,8 @@ func (s *statefulSetLister) GetPodStatefulSets(pod *v1.Pod) ([]*apps.StatefulSet } selector, err = metav1.LabelSelectorAsSelector(ps.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a StatefulSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/apps/v1beta2/daemonset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1beta2/daemonset_expansion.go index 3b01aaa487d..e722b63b680 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1beta2/daemonset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1beta2/daemonset_expansion.go @@ -60,8 +60,8 @@ func (s *daemonSetLister) GetPodDaemonSets(pod *v1.Pod) ([]*apps.DaemonSet, erro } selector, err = metav1.LabelSelectorAsSelector(daemonSet.Spec.Selector) if err != nil { - // this should not happen if the DaemonSet passed validation - return nil, err + // This object has an invalid selector, it does not match the pod + continue } // If a daemonSet with a nil or empty selector creeps in, it should match nothing, not everything. @@ -96,7 +96,8 @@ func (s *daemonSetLister) GetHistoryDaemonSets(history *apps.ControllerRevision) for _, ds := range list { selector, err := metav1.LabelSelectorAsSelector(ds.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid label selector: %v", err) + // This object has an invalid selector, it does not match the history object + continue } // If a DaemonSet with a nil or empty selector creeps in, it should match nothing, not everything. if selector.Empty() || !selector.Matches(labels.Set(history.Labels)) { diff --git a/staging/src/k8s.io/client-go/listers/apps/v1beta2/replicaset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1beta2/replicaset_expansion.go index 7562fe99689..bc014b5a69d 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1beta2/replicaset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1beta2/replicaset_expansion.go @@ -55,7 +55,8 @@ func (s *replicaSetLister) GetPodReplicaSets(pod *v1.Pod) ([]*apps.ReplicaSet, e } selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a ReplicaSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/apps/v1beta2/statefulset_expansion.go b/staging/src/k8s.io/client-go/listers/apps/v1beta2/statefulset_expansion.go index 6fa6b9144b2..eae31b82f83 100644 --- a/staging/src/k8s.io/client-go/listers/apps/v1beta2/statefulset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/apps/v1beta2/statefulset_expansion.go @@ -59,7 +59,8 @@ func (s *statefulSetLister) GetPodStatefulSets(pod *v1.Pod) ([]*apps.StatefulSet } selector, err = metav1.LabelSelectorAsSelector(ps.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a StatefulSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/batch/v1/job_expansion.go b/staging/src/k8s.io/client-go/listers/batch/v1/job_expansion.go index fdcd5f32eef..8dc5db7885e 100644 --- a/staging/src/k8s.io/client-go/listers/batch/v1/job_expansion.go +++ b/staging/src/k8s.io/client-go/listers/batch/v1/job_expansion.go @@ -51,7 +51,11 @@ func (l *jobLister) GetPodJobs(pod *v1.Pod) (jobs []batch.Job, err error) { return } for _, job := range list { - selector, _ := metav1.LabelSelectorAsSelector(job.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(job.Spec.Selector) + if err != nil { + // This object has an invalid selector, it does not match the pod + continue + } if !selector.Matches(labels.Set(pod.Labels)) { continue } diff --git a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/daemonset_expansion.go b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/daemonset_expansion.go index 336a4ed831c..f6dd7a963e8 100644 --- a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/daemonset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/daemonset_expansion.go @@ -61,8 +61,8 @@ func (s *daemonSetLister) GetPodDaemonSets(pod *v1.Pod) ([]*v1beta1.DaemonSet, e } selector, err = metav1.LabelSelectorAsSelector(daemonSet.Spec.Selector) if err != nil { - // this should not happen if the DaemonSet passed validation - return nil, err + // This object has an invalid selector, it does not match the pod + continue } // If a daemonSet with a nil or empty selector creeps in, it should match nothing, not everything. @@ -97,7 +97,8 @@ func (s *daemonSetLister) GetHistoryDaemonSets(history *apps.ControllerRevision) for _, ds := range list { selector, err := metav1.LabelSelectorAsSelector(ds.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid label selector: %v", err) + // This object has an invalid selector, it does not match the history object + continue } // If a DaemonSet with a nil or empty selector creeps in, it should match nothing, not everything. if selector.Empty() || !selector.Matches(labels.Set(history.Labels)) { diff --git a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/replicaset_expansion.go b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/replicaset_expansion.go index 1f72644ccab..74114c2bd7d 100644 --- a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/replicaset_expansion.go +++ b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/replicaset_expansion.go @@ -55,7 +55,8 @@ func (s *replicaSetLister) GetPodReplicaSets(pod *v1.Pod) ([]*extensions.Replica } selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - return nil, fmt.Errorf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod + continue } // If a ReplicaSet with a nil or empty selector creeps in, it should match nothing, not everything. diff --git a/staging/src/k8s.io/client-go/listers/policy/v1/poddisruptionbudget_expansion.go b/staging/src/k8s.io/client-go/listers/policy/v1/poddisruptionbudget_expansion.go index f63851ad48b..115ee3f0047 100644 --- a/staging/src/k8s.io/client-go/listers/policy/v1/poddisruptionbudget_expansion.go +++ b/staging/src/k8s.io/client-go/listers/policy/v1/poddisruptionbudget_expansion.go @@ -23,7 +23,6 @@ import ( policy "k8s.io/api/policy/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/klog/v2" ) // PodDisruptionBudgetListerExpansion allows custom methods to be added to @@ -50,7 +49,7 @@ func (s *podDisruptionBudgetLister) GetPodPodDisruptionBudgets(pod *v1.Pod) ([]* pdb := list[i] selector, err = metav1.LabelSelectorAsSelector(pdb.Spec.Selector) if err != nil { - klog.Warningf("invalid selector: %v", err) + // This object has an invalid selector, it does not match the pod continue } diff --git a/staging/src/k8s.io/client-go/listers/policy/v1beta1/poddisruptionbudget_expansion.go b/staging/src/k8s.io/client-go/listers/policy/v1beta1/poddisruptionbudget_expansion.go index dce5dca8208..994947c4f3c 100644 --- a/staging/src/k8s.io/client-go/listers/policy/v1beta1/poddisruptionbudget_expansion.go +++ b/staging/src/k8s.io/client-go/listers/policy/v1beta1/poddisruptionbudget_expansion.go @@ -23,7 +23,6 @@ import ( policy "k8s.io/api/policy/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/klog/v2" ) // PodDisruptionBudgetListerExpansion allows custom methods to be added to @@ -50,8 +49,7 @@ func (s *podDisruptionBudgetLister) GetPodPodDisruptionBudgets(pod *v1.Pod) ([]* pdb := list[i] selector, err = metav1.LabelSelectorAsSelector(pdb.Spec.Selector) if err != nil { - klog.Warningf("invalid selector: %v", err) - // TODO(mml): add an event to the PDB + // This object has an invalid selector, it does not match the pod continue }