Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Auth token expire, what watch stream will be happened? #12157

Closed
xkeyideal opened this issue Jul 21, 2020 · 7 comments
Closed

Basic Auth token expire, what watch stream will be happened? #12157

xkeyideal opened this issue Jul 21, 2020 · 7 comments
Labels

Comments

@xkeyideal
Copy link

https://github.com/etcd-io/etcd/blob/master/clientv3/client.go#L254

See the codes, i see when unary invoke rpctypes.ErrInvalidAuthToken will c.authTokenBundle.UpdateAuthToken(resp.Token) and retry the operate, but stream no retry.

When create or reconnect stream connection return ErrInvalidAuthToken, the etcd clientv3 will not work?

@xkeyideal
Copy link
Author

xkeyideal commented Jul 21, 2020

https://github.com/etcd-io/etcd/blob/master/clientv3/watch.go#L282
https://github.com/etcd-io/etcd/blob/master/clientv3/watch.go#L526

// start a stream with the etcd grpc server
if wc, closeErr = w.newWatchClient(); closeErr != nil {
	return
}
if wgs.closeErr != nil {
	closeCh <- WatchResponse{Canceled: true, closeErr: wgs.closeErr}
	break
}

The Watch function can receive closeErr, user may be use the code retry watch.

for {
	watch := etcd.client.Watch(ctx, prefixKey, clientv3.WithPrefix(), clientv3.WithPrevKV(), clientv3.WithRev(revision))
loop:
	for {
		select {
		case <-ctx.Done():
			return
		case resp, ok := <-watch:
			if !ok || resp.Canceled || resp.Err() != nil {
				time.Sleep(100 * time.Millisecond)
				break loop
			}
		}
	}
}

But user can't call getToken for update auth token, will be error again and again.

@xkeyideal
Copy link
Author

So the etcd clientv3 can export getToken function to user

@cfc4n
Copy link
Contributor

cfc4n commented Jul 21, 2020

Can you give a demo for it ?

@xkeyideal
Copy link
Author

xkeyideal commented Jul 22, 2020

@cfc4n

I think this is a bug or feature, when the authtoken expired, watch stream need reconnect will be failed always.

See the second comment.

Because the connection auth token expired the watch will return ErrInvalidAuthToken, and retry or return, but user can't reconnect etcd server, user just exec unary command for passive fresh the auth token then watch will be succeed.

#12135, the pr said same problem.

I suggest solution:

  1. authenticate function return token ttl
auth, err = newAuthenticator(ctx, target, dOpts, c)
if err != nil {
	continue
}
defer auth.close()

var resp *AuthenticateResponse
resp, err = auth.authenticate(ctx, c.Username, c.Password)
if err != nil {
	// return err without retrying other endpoints
	if err == rpctypes.ErrAuthNotEnabled {
		return err
	}
	continue
}

c.authTokenBundle.UpdateAuthToken(resp.Token)
  1. etcd client exec scheduled task for update auth token

  2. export the getToken function to user, can refresh auth token

@cfc4n
Copy link
Contributor

cfc4n commented Jul 23, 2020

I got it. I'll give a PR after PR #12165 merged.

/cc @xiang90 @gyuho
assign to me @cfc4n

@jschwinger233
Copy link

Sorry for chiming in, but in my opinion, grpc-go already provides a perfect interface to refresh credential: https://godoc.org/google.golang.org/grpc/credentials#PerRPCCredentials

type PerRPCCredentials interface {
    // GetRequestMetadata gets the current request metadata, refreshing
    // tokens if required. This should be called by the transport layer on
    // each request, and the data should be populated in headers or other
    // context. If a status code is returned, it will be used as the status
    // for the RPC. uri is the URI of the entry point for the request.
    // When supported by the underlying implementation, ctx can be used for
    // timeout and cancellation. Additionally, RequestInfo data will be
    // available via ctx to this call.
    // TODO(zhaoq): Define the set of the qualified keys instead of leaving
    // it as an arbitrary string.
    GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error)
    // RequireTransportSecurity indicates whether the credentials requires
    // transport security.
    RequireTransportSecurity() bool
}

I noted a ancient bug grpc/grpc-go#3749 which was fixed in the latest grpc grpc/grpc-go#3677, so there will be no concern.

I know it's aggressive to upgrade grpc from ancient version to the latest release, even I can foresee a number of incompatibility like grpc.WithBalancer has been deprecated, but it's better we follow the best practice.

@stale
Copy link

stale bot commented Nov 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 17, 2020
@stale stale bot closed this as completed Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants