ListOptions step on each other, HasLabels and MatchingLabels compete

[jsilvela]

When trying to get a List using the variadic ListOptions argument, I ran to unexpected behavior if applying both HasLabels and MatchingLabels.

List(ctx, podList, client.InNamespace(namespace)
    client.MatchingLabels{
        utils.ClusterLabelName: clusterName,
    },
    client.HasLabels{"role"}, // this ensures we are getting instance pods only
)

When watching the code in action, more pods than expected were being returned.
It seemed as if the ListOptions were being OR'ed rather than AND'ed.

Looking into the implementation of MatchingLabels and HasLabels, in their ApplyToList(opts *ListOptions),
both will overwrite whatever values the LabelSelector had before.

opts.LabelSelector = sel

This means, likely, that if we apply both HasLabels and MatchingLabels, the last one applied will "win".
This is rather un-intuitive behavior.
If it is so by design, I think it should be documented.
If possible, I would think a logical AND would be the intuitive expectation.

[alvaroaleman]

/kind bug

Feel free to contribute a fix

[jsilvela]

Yep @alvaroaleman would like to, going to try to make time for this

[jsilvela]

I posted in Slack, but will add my question here, I think it may be relevant:

I am working on a PR for this Issue.

I have a fix that is passing all the tests for HasLabels. It adds Requirement to any existing Selector, rather than overwrite.
I am trying to do the same for MatchingLabels.
But to my surprise, there is a test It("Should produce an invalid selector when given invalid input",
So, the expected behavior, for MatchingLabels is that the ApplyToList can add Requirements that would be rejected if using
labels.NewRequirement(
In controller-runtime I see no other dependency on such behavior, aside from that unit test.
Would it be possible to modify that behavior? Are there other consumers that require it?

[jsilvela]

I.e. I have this for MatchingLabels

// NOTE: the invalid labels will not be applied
	for label, value := range m {
		r, err := labels.NewRequirement(label, selection.Equals, []string{value})
		if err == nil {
			sel = sel.Add(*r)
		}
	}

I saw that unit test is your work @alvaroaleman , I see it was fixing a previous bug. I'm assuming my way above won't work, because it would not produce any selector given invalid input.
Could you help me understand the context?
Many thanks

[alvaroaleman]

@jsilvela the original problem was that invalid input to MatchingLabels would result in a nil selector which in turn selects everything. That was a huge footgun and I've seen this lead to pretty severe bugs a couple of times. The fix was to just construct an invalid selector which will then get rejected by the server. So yeah, your above code that would just ignore errors is a no-go.

At the end of the day, multiple requirements in a labelselector are anded by joining them comma-separated. So how about we make the two ApplyToList funcs update listOpts.Raw.LabelSelector (prefixed by a coma if non-empty)?

I'd also rather have us construct a string label selector in HasLabels, the fact that we currently swallow errors there is a really bad bug. I guess the label key tends to be constant which makes it less likely for ppl to run into that, but its definitely something we should fix.

[jsilvela]

Ah, I see. Great information, thank you. I will start working on that direction today if I can.