Controller-runtime: Listing objects with a label selector that exceeds 63 characters returns non-matching objects and no error

[flawedmatrix]

I was investigating a bug in a controller where the wrong objects were being deleted during reconciliation. Upon closer inspection, it looks like the List() method was returning objects that didn't match the label selector provided, and that this occurs when the label selector exceeds the 63 character limit.

While an error is returned from the API in this case, List() returns all objects of the type passed in.

I've managed to reproduce this behavior with a minimal example with v0.4.0 of controller-runtime (against a kind cluster):

Setup

var dummyNamespace corev1.Namespace = corev1.Namespace{
    ObjectMeta: metav1.ObjectMeta{
        Name: "some-namespace-name",
        Labels: map[string]string{
            "namespaceUniqueIdentifier": strings.Repeat("a", 63),
        },
    },
}
err = k8sClient.Create(context.Background(), &dummyNamespace)
if err != nil {
    log.Fatalln("Error creating namespace", err)
}

Test function

func listNamespacesWithLabel(k8sClient client.Client, testNo int, label string) {
    listOption := client.MatchingLabels{
        "namespaceUniqueIdentifier": label,
    }

    nsList := corev1.NamespaceList{}
    err := k8sClient.List(context.Background(), &nsList, listOption)
    if err != nil {
        log.Fatalln("Error listing namespaces", err)
    }

    fmt.Printf("\n#################### TEST NO %d\n", testNo)
    fmt.Printf("%d namespaces listed\n", len(nsList.Items))
    for _, ns := range nsList.Items {
        fmt.Printf("Name %s, Labels: %#v \n", ns.Name, ns.Labels)
    }
}

Test

listNamespacesWithLabel(k8sClient, 1, strings.Repeat("a", 63))
/*
Expected 1 namespaces
Actual Result:
#################### TEST NO 1
1 namespaces listed
Name some-namespace-name, Labels: map[string]string{"namespaceUniqueIdentifier":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}
*/

listNamespacesWithLabel(k8sClient, 2, strings.Repeat("b", 63))
/*
Expected 0 namespaces
Actual Result:
#################### TEST NO 2
0 namespaces listed
*/

listNamespacesWithLabel(k8sClient, 3, strings.Repeat("b", 64))
/*
Expected error (or 0 namespaces)
Actual Result:
#################### TEST NO 3
5 namespaces listed
Name default, Labels: map[string]string(nil)
Name kube-node-lease, Labels: map[string]string(nil)
Name kube-public, Labels: map[string]string(nil)
Name kube-system, Labels: map[string]string(nil)
Name some-namespace-name, Labels: map[string]string{"namespaceUniqueIdentifier":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}
*/

While it's possible to avoid this issue in the controller, this still seems like unexpected behavior from the client.

[rajathagasthya]

From a little bit of debugging, I think this is what is happening:

1. MatchingLabels.ApplyToList uses apimachinery's labels.SelectorFromSet to get a label selector.
   https://github.com/kubernetes-sigs/controller-runtime/blob/efbeb2728794b105247bbf27c76a5c4949a73010/pkg/client/options.go#L383-L384
2. labels.SelectorFromSet is actually eating up all the validation errors (not sure why) when it constructs Requirements in case of label keys and values exceeding 63 chars.
   https://github.com/kubernetes/apimachinery/blob/3253b0a30d67e7e362b8615e463156bac729c82f/pkg/labels/selector.go#L881-L883
3. When a validation error occurs in labels.SelectorFromSet, it returns an internalSelector{} (with empty Requirements). An empty internalSelector{} will match everything, which is why you see all the namespaces being returned on an invalid label value.
   https://github.com/kubernetes/apimachinery/blob/3253b0a30d67e7e362b8615e463156bac729c82f/pkg/labels/selector.go#L346-L352
   https://github.com/kubernetes/apimachinery/blob/3253b0a30d67e7e362b8615e463156bac729c82f/pkg/labels/selector.go#L64-L67

There are some selector parsing functions we could make use of while setting labels in ListOptions, but none of the options interfaces return an error so I'm not sure what's the correct way to solve this. Also, I think we should be adding some related tests.

I'll defer to @DirectXMan12 for his input on how best to solve this.

[DirectXMan12]

I suspect SelectorFromSet assumes a pre-validated set, which we're obv not passing it. We could have opts hold an error and then throw that error when doing the fetch, which would allow us to make use of the error-throwing parsing logic.

SelectorFromSet needs a godoc comment upstream though -- that's a pretty big footgun.

[rajathagasthya]

I suspect SelectorFromSet assumes a pre-validated set, which we're obv not passing it.

I doubt it because there is another function that assumes a pre-validated set: https://github.com/kubernetes/apimachinery/blob/3253b0a30d67e7e362b8615e463156bac729c82f/pkg/labels/selector.go#L893-L894

This one might just be an oversight upstream. Anyway, I created https://github.com/kubernetes/kubernetes/issues/86792 to track it.

[vincepri]

/priority awaiting-more-evidence
/help

Might need more investigation around the issue reported upstream. The issue might not be fixed in controller-runtime.

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/priority awaiting-more-evidence
/help

Might need more investigation around the issue reported upstream. The issue might not be fixed in controller-runtime.

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.

[alvaroaleman]

I think we have to deprecate opts.MatchingLabels, because its exported and we can not return an error in ApplyToList.

We need to instead have something like NewMatchingLabelsSelector(map[string]string)(Option, error) to have a place where we can return the error.