Skip to content

⚠️ Default selectors to Nothing if it is nil for MatchingLabelsSelector and MatchingFieldsSelector #3279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 19, 2025
+32 −0
Merged

⚠️ Default selectors to Nothing if it is nil for MatchingLabelsSelector and MatchingFieldsSelector #3279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7d9e4f
Default selectors to Everything if it is nil for MatchingLabelsSelect…
acumino Aug 13, 2025
1d48074
Address Review
acumino Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/client/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,9 @@ type MatchingLabelsSelector struct {

// ApplyToList applies this configuration to the given list options.
func (m MatchingLabelsSelector) ApplyToList(opts *ListOptions) {
if m.Selector == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a label or field selector be set if it has no value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two reasons -

  • When a list call happens with empty/nil label selector, it lists everything, since MatchingLabelsSelector also wants to achieve a similar thing; therefore, in case of a nil label selector, it should list all the objects.
  • Currently if nil is passed for selector in MatchingLabelsSelector the list call panics, which should not happen

Copy link
Member

@sbueringer sbueringer Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, this is the official godoc on metav1.LabelSelector

// A label selector is a label query over a set of resources. The result of matchLabels and
// matchExpressions are ANDed. An *empty* label selector matches *all objects*. A *null*
// label selector matches *no objects*.

If we follow that semantic here as well null would mean match no objects

Copy link
Contributor Author

@acumino acumino Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use -

client.List(ctx, nodeList, &client.ListOptions{LabelSelector: nil})

It lists all objects. What should be the way to go here?

Also

A null label selector matches no objects.

If we follow this, then I can modify the PR to use Nothing instead of Everything when MatchingLabelsSelector's selector is not set so it doesn't panic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you have code that ends up with a nil selector and why that is not considered to be a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about the below changes -

// ApplyToList applies this configuration to the given list options.
func (m MatchingLabelsSelector) ApplyToList(opts *ListOptions) {
	if m.Selector == nil {
		m.Selector = labels.SelectorFromSet(labels.Set{})
	}
	opts.LabelSelector = m
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a nil selector in there is incorrect usage and we can not error. We do recover panics though so it seems all right to me.

Your suggestion makes a nil selector match everything, which as mentioned earlier we do not want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recover panics though so it seems all right to me.

This doesn't allow us to let the panic happen at the first place.

Also, IMO we should allow list all, because as per the doc string MatchingLabelsSelector is supposed to behave the same as LabelSelector, which it doesn't currently.

By my main concern for this PR was that we should not let the panic happen, we should stop it one way or another and not let the user figure it out after trial and error. I am open to suggestions for that.
If we really don't want to list anything in case of nil Selector in MatchingLabelsSelector, I can use labels.Nothing instead of Everything.
This will fix both of our concerns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, IMO we should allow list all, because as per the doc string MatchingLabelsSelector is supposed to behave the same as LabelSelector, which it doesn't currently.

Which could also be fixed by changing LabelSelector rather than MatchingLabelSelector.

I don't personally mind the panic, given our default is to recover panics and this is basically incorrect usage and we don't have a way to report that, as the listOpts can not error. I am okay changing it to labels.Nothing() if you want to do that, but I would veto changing it to labels.Everything() due to potential security implications.

Copy link
Contributor Author

@acumino acumino Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main issue was nil pointer panic, I am ok with nothing also.
Update the PR, PTAL.

m.Selector = labels.Nothing()
}
opts.LabelSelector = m
}

Expand Down Expand Up @@ -677,6 +680,9 @@ type MatchingFieldsSelector struct {

// ApplyToList applies this configuration to the given list options.
func (m MatchingFieldsSelector) ApplyToList(opts *ListOptions) {
if m.Selector == nil {
m.Selector = fields.Nothing()
}
opts.FieldSelector = m
}

Expand Down
26 changes: 26 additions & 0 deletions pkg/client/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,38 @@ var _ = Describe("ListOptions", func() {
o.ApplyToList(newListOpts)
Expect(newListOpts).To(Equal(o))
})
It("Should set LabelSelector with MatchingLabelsSelector", func() {
labelSelector, err := labels.Parse("a=b")
Expect(err).NotTo(HaveOccurred())
newListOpts := &client.ListOptions{}
newListOpts.ApplyOptions([]client.ListOption{client.MatchingLabelsSelector{Selector: labelSelector}})
expectedListOpts := &client.ListOptions{LabelSelector: client.MatchingLabelsSelector{Selector: labelSelector}}
Expect(newListOpts).To(Equal(expectedListOpts))
})
It("Should set LabelSelector to nothing with empty MatchingLabelsSelector", func() {
newListOpts := &client.ListOptions{}
newListOpts.ApplyOptions([]client.ListOption{client.MatchingLabelsSelector{}})
expectedListOpts := &client.ListOptions{LabelSelector: client.MatchingLabelsSelector{Selector: labels.Nothing()}}
Expect(newListOpts).To(Equal(expectedListOpts))
})
It("Should set FieldSelector", func() {
o := &client.ListOptions{FieldSelector: fields.Nothing()}
newListOpts := &client.ListOptions{}
o.ApplyToList(newListOpts)
Expect(newListOpts).To(Equal(o))
})
It("Should set FieldSelector with MatchingFieldsSelector", func() {
newListOpts := &client.ListOptions{}
newListOpts.ApplyOptions([]client.ListOption{client.MatchingFieldsSelector{Selector: fields.Nothing()}})
expectedListOpts := &client.ListOptions{FieldSelector: client.MatchingFieldsSelector{Selector: fields.Nothing()}}
Expect(newListOpts).To(Equal(expectedListOpts))
})
It("Should set FieldSelector to nothing with empty MatchingFieldsSelector", func() {
newListOpts := &client.ListOptions{}
newListOpts.ApplyOptions([]client.ListOption{client.MatchingFieldsSelector{}})
expectedListOpts := &client.ListOptions{FieldSelector: client.MatchingFieldsSelector{Selector: fields.Nothing()}}
Expect(newListOpts).To(Equal(expectedListOpts))
})
It("Should set Namespace", func() {
o := &client.ListOptions{Namespace: "my-ns"}
newListOpts := &client.ListOptions{}
Expand Down