Controller-runtime: client.Get will hung when service-account lack of get and list permissions

[youyongsong]

What happened?

I'm using the k8s client injected by the manager to get a secret resource, but I forgot to add the rbac rules to allow manager to list and get secret resources. Then the client.Get method hung and always retry to list secrets.

Here's the code:

secret := &corev1.Secret{}
err = client.Get(ctx,
    types.NamespacedName{
        Namespace: "default",
        Name:      "my-secret",
    },
    secret)

Here's the log:

E0806 02:11:02.862269       1 reflector.go:126] pkg/mod/k8s.io/[email protected]+incompatible/tools/cache/reflector.go:94: Failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:cluster-registry-system:default" cannot list resource "secrets" in API group "" at the cluster scope
E0806 02:11:03.864770       1 reflector.go:126] pkg/mod/k8s.io/[email protected]+incompatible/tools/cache/reflector.go:94: Failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:cluster-registry-system:default" cannot list resource "secrets" in API group "" at the cluster scope
E0806 02:11:04.867186       1 reflector.go:126] pkg/mod/k8s.io/[email protected]+incompatible/tools/cache/reflector.go:94: Failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:cluster-registry-system:default" cannot list resource "secrets" in API group "" at the cluster scope

What i expected to happen?

I expected the client.Get will return an error immediately instead of always retry to list. Maybe it's a bug of DelegatingClient.

[DirectXMan12]

client.Get should never retry by itself. Are you requeuing? Can you post a simple reproducer here?

[DirectXMan12]

/kind bug

[youyongsong]

client.Get should never retry by itself. Are you requeuing? Can you post a simple reproducer here?

I'm using it in a validating webhook hanlder:

func (v *ClusterValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
    secret := &corev1.Secret{}
    logger.Info("-- begin to fetch secret ---")
    err = client.Get(ctx,
        types.NamespacedName{
            Namespace: c.Spec.AuthInfo.Controller.Namespace,
            Name:      c.Spec.AuthInfo.Controller.Name,
        },
        secret)
    if err != nil {
        logger.Info("-- failed to fetch secret ---")
        logger.Error(err, "unable to fetch secret specified in cluster")
        return &kubernetes.Clientset{}, ErrAuthSecretFetchFailed
    }
    logger.Info("-- secret fetched ---")
}

The log output only contains -- begin to fetch secret ---, the other two breakpoints(-- failed to fetch secret --- and -- secret fetched ---) are never executed.

[DirectXMan12]

where's the value of client coming from? manager.GetClient()?

[DirectXMan12]

If you're using manager.GetClient, you're getting a client that reads from the cache, which means that the request will block until the caches are synced initially. If that's not desired behavior, construct a fresh client with client.New.

[youyongsong]

@DirectXMan12 thanks for explanation.

[shawn-hurley]

I would like to consider re-opening this bug.

I was thinking it may be desirable to have a timeout that then passed the error of the cache sync to the caller while waiting on the cache to sync?

[DirectXMan12]

It's probably not a bad idea to figure out how to plumb context through those operations. Then we can let someone use context to specify a timeout, potentially.

[DirectXMan12]

Can you open another bug for that, @shawn-hurley ?

[jpkrohling]

Is there an issue with @shawn-hurley's suggestion? I'm facing a similar problem and it's not that desirable to have my operator to be locked indefinitely when it will never get unlocked without someone getting notified :-/

[shawn-hurley]

@jpkrohling I think the fix is here: https://github.com/kubernetes-sigs/controller-runtime/pull/663

This pipes the context through and you can use that during the get to time it out.

[rcernich]

I don't think adding a context solves the problem here. What I'm seeing is that a client.Get() blocks indefinitely, because the service account does not have permissions to get/list/watch the resource. Cache or no, the call should return an error, (e.g. unauthorized). Instead, the call hangs and fills the logs with things like:

sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go: 196: Failed to list *v1.ImageStream: imagestreams.image.openshift.io is forbidden: User "system:serviceaccount:istio-operator:istio-operator" cannot list resource "imagestreams" in API group "image.openshift.io" at the cluster scope

As @jpkrohling mentioned, the only way this will ever change is if somebody updates the permission on the service account.

I think the correct fix is to detect non-recoverable errors and return the error promptly to the caller.

[naphta]

Having the same problem, and whilst context does take it from unusable to usable (with some vaguely sketchy error handling and a timeout), it does seem entirely reasonable that you should be returned an error when the cache will never become live.