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/client-go@v11.0.1-0.20190409021438-1a26190bd76a+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/client-go@v11.0.1-0.20190409021438-1a26190bd76a+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/client-go@v11.0.1-0.20190409021438-1a26190bd76a+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: #663

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