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.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sbueringer]

For reference this has been fixed via #2363

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

For reference this has been fixed via #2363

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.