🐛 MatchingLabels: Make sure requests with invalid labels fail

[alvaroaleman]

Fixes #740

Following up on an idea by @lavalamp this PR makes MatchingLabels construct an invalid selector rather than a (valid) match all selector in case of invalid input, which means the server will reject the request.

/assign @alexeldeib @vincepri

[alexeldeib]

Nice fix. Do you think it's worth adding an invalid call against a real server in client_test.go to catch regressions? I realize your unit test essentially is the server side behavior, so it's really a nit, but can't hurt? I'll defer to you.

/lgtm

[alvaroaleman]

Nice fix. Do you think it's worth adding an invalid call against a real server in client_test.go to catch regressions? I realize your unit test essentially is the server side behavior, so it's really a nit, but can't hurt? I'll defer to you.

I think its fine, the important part is that we verify that even on invalid input a selector is populated rather than getting an empty selector back. Even if this had an integration test, an actual apiserver could behave differently than the one used in the integration test.

[vincepri]

code LGTM, should this be a breaking change though?

[alvaroaleman]

code LGTM, should this be a breaking change though?

No, the previous behavior of "Return empty label selector that matches everything on invalid selector" is a bug and very unexpected.

[vincepri]

fair, we'll add some more details in the release notes for users

[vincepri]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment