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.

Sign up for GitHub

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

Jump to bottom

clientv3:get AuthToken gracefully without extra connection. #12165

Merged
merged 3 commits into from
Sep 25, 2020
Merged

clientv3:get AuthToken gracefully without extra connection. #12165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6bed3e
etcdserver: check authinfo if it is not InternalAuthenticateRequest.
cfc4n Aug 17, 2020
79b3ec5
credentials: let GetRequestMetadata() return nil when authToken isn't…
cfc4n Aug 17, 2020
07e9eb0
clientv3: get AuthToken gracefully without extra connection.
cfc4n Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 8 additions & 31 deletions clientv3/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ const (
type UserAddOptions authpb.UserAddOptions

type Auth interface {
// Authenticate login and get token
Authenticate(ctx context.Context, name string, password string) (*AuthenticateResponse, error)

// AuthEnable enables auth of an etcd cluster.
AuthEnable(ctx context.Context) (*AuthEnableResponse, error)

Expand Down Expand Up @@ -129,6 +132,11 @@ func NewAuthFromAuthClient(remote pb.AuthClient, c *Client) Auth {
return api
}

func (auth *authClient) Authenticate(ctx context.Context, name string, password string) (*AuthenticateResponse, error) {
resp, err := auth.remote.Authenticate(ctx, &pb.AuthenticateRequest{Name: name, Password: password}, auth.callOpts...)
return (*AuthenticateResponse)(resp), toErr(ctx, err)
}

func (auth *authClient) AuthEnable(ctx context.Context) (*AuthEnableResponse, error) {
resp, err := auth.remote.AuthEnable(ctx, &pb.AuthEnableRequest{}, auth.callOpts...)
return (*AuthEnableResponse)(resp), toErr(ctx, err)
Expand Down Expand Up @@ -226,34 +234,3 @@ func StrToPermissionType(s string) (PermissionType, error) {
}
return PermissionType(-1), fmt.Errorf("invalid permission type: %s", s)
}

type authenticator struct {
conn *grpc.ClientConn // conn in-use
remote pb.AuthClient
callOpts []grpc.CallOption
}

func (auth *authenticator) authenticate(ctx context.Context, name string, password string) (*AuthenticateResponse, error) {
resp, err := auth.remote.Authenticate(ctx, &pb.AuthenticateRequest{Name: name, Password: password}, auth.callOpts...)
return (*AuthenticateResponse)(resp), toErr(ctx, err)
}

func (auth *authenticator) close() {
auth.conn.Close()
}

func newAuthenticator(ctx context.Context, target string, opts []grpc.DialOption, c *Client) (*authenticator, error) {
conn, err := grpc.DialContext(ctx, target, opts...)
if err != nil {
return nil, err
}

api := &authenticator{
conn: conn,
remote: pb.NewAuthClient(conn),
}
if c != nil {
api.callOpts = c.callOpts
}
return api, nil
}
81 changes: 24 additions & 57 deletions clientv3/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,41 +274,19 @@ func (c *Client) Dial(ep string) (*grpc.ClientConn, error) {
func (c *Client) getToken(ctx context.Context) error {
var err error // return last error in a case of fail

eps := c.Endpoints()
for _, ep := range eps {
var auth *authenticator
// use dial options without dopts to avoid reusing the client balancer
var dOpts []grpc.DialOption
_, host, _ := endpoint.ParseEndpoint(ep)
target := c.resolverGroup.Target(host)
creds := c.dialWithBalancerCreds(ep)
dOpts, err = c.dialSetupOpts(creds, c.cfg.DialOptions...)
if err != nil {
err = fmt.Errorf("failed to configure auth dialer: %v", err)
continue
}
dOpts = append(dOpts, grpc.WithBalancerName(roundRobinBalancerName))
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)
if c.Username == "" || c.Password == "" {
return nil
}

return err
resp, err := c.Auth.Authenticate(ctx, c.Username, c.Password)
if err != nil {
if err == rpctypes.ErrAuthNotEnabled {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a reason to hide this error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means etcd cluster do not enable auth. so return nil .
ref #10428

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return nil, clients continue retrying forever like this when they issue Authenticate() request:

~/g/s/g/etcd [get_authtoken_gracefully]× Â» bin/etcdctl get k1 --user u1:p                                                  -130- 18:08:51
{"level":"warn","ts":"2020-08-15T18:08:51.760+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: authentication is not enabled"}
{"level":"warn","ts":"2020-08-15T18:08:51.762+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = Unauthenticated desc = etcdserver: invalid auth token"}
{"level":"warn","ts":"2020-08-15T18:08:51.764+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: authentication is not enabled"}
...

The behavior is different from the original one. I think it would be related to the failed test cases, let me check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't related to the failed test cases. I'm checking its effect.

}
return err
}
c.authTokenBundle.UpdateAuthToken(resp.Token)
return nil
}

// dialWithBalancer dials the client's current load balanced resolver group. The scheme of the host
Expand All @@ -329,31 +307,7 @@ func (c *Client) dial(target string, creds grpccredentials.TransportCredentials,

if c.Username != "" && c.Password != "" {
c.authTokenBundle = credentials.NewBundle(credentials.Config{})

ctx, cancel := c.ctx, func() {}
if c.cfg.DialTimeout > 0 {
ctx, cancel = context.WithTimeout(ctx, c.cfg.DialTimeout)
}

err = c.getToken(ctx)
if err != nil {
// TODO: Consider retrying transient errors like:
// "error":"rpc error: code = Unavailable desc = etcdserver: leader changed"

// Ignore rpctypes.ErrAuthNotEnabled error.
if toErr(ctx, err) != rpctypes.ErrAuthNotEnabled {
// This logic originates from 62d7bae496 and is not clear why we cannot just return err
// without looking into parent's context.
if err == ctx.Err() && ctx.Err() != c.ctx.Err() {
err = context.DeadlineExceeded
}
cancel()
return nil, err
}
} else {
opts = append(opts, grpc.WithPerRPCCredentials(c.authTokenBundle.PerRPCCredentials()))
}
cancel()
opts = append(opts, grpc.WithPerRPCCredentials(c.authTokenBundle.PerRPCCredentials()))
}

opts = append(opts, c.cfg.DialOptions...)
Expand Down Expand Up @@ -496,6 +450,19 @@ func newClient(cfg *Config) (*Client, error) {
client.Auth = NewAuth(client)
client.Maintenance = NewMaintenance(client)

//get token with established connection
ctx, cancel = client.ctx, func() {}
if client.cfg.DialTimeout > 0 {
ctx, cancel = context.WithTimeout(ctx, client.cfg.DialTimeout)
}
err = client.getToken(ctx)
if err != nil {
client.Close()
cancel()
return nil, err
}
cancel()

if cfg.RejectOldCluster {
if err := client.checkVersion(); err != nil {
client.Close()
Expand Down
3 changes: 3 additions & 0 deletions clientv3/credentials/credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ func (rc *perRPCCredential) GetRequestMetadata(ctx context.Context, s ...string)
rc.authTokenMu.RLock()
authToken := rc.authToken
rc.authTokenMu.RUnlock()
if authToken == "" {
return nil, nil
}
return map[string]string{rpctypes.TokenFieldNameGRPC: authToken}, nil
}

Expand Down
14 changes: 13 additions & 1 deletion clientv3/maintenance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,19 @@ func NewMaintenance(c *Client) Maintenance {
if err != nil {
return nil, nil, fmt.Errorf("failed to dial endpoint %s with maintenance client: %v", endpoint, err)
}
cancel := func() { conn.Close() }

//get token with established connection
dctx := c.ctx
cancel := func() {}
if c.cfg.DialTimeout > 0 {
dctx, cancel = context.WithTimeout(c.ctx, c.cfg.DialTimeout)
}
err = c.getToken(dctx)
cancel()
if err != nil {
return nil, nil, fmt.Errorf("failed to getToken from endpoint %s with maintenance client: %v", endpoint, err)
}
cancel = func() { conn.Close() }
return RetryMaintenanceClient(c, conn), cancel, nil
},
remote: RetryMaintenanceClient(c, c.conn),
Expand Down
17 changes: 10 additions & 7 deletions etcdserver/v3_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,13 +647,16 @@ func (s *EtcdServer) processInternalRaftRequestOnce(ctx context.Context, r pb.In
ID: s.reqIDGen.Next(),
}

authInfo, err := s.AuthInfoFromCtx(ctx)
if err != nil {
return nil, err
}
if authInfo != nil {
r.Header.Username = authInfo.Username
r.Header.AuthRevision = authInfo.Revision
// check authinfo if it is not InternalAuthenticateRequest
if r.Authenticate == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice idea, thanks!

authInfo, err := s.AuthInfoFromCtx(ctx)
if err != nil {
return nil, err
}
if authInfo != nil {
r.Header.Username = authInfo.Username
r.Header.AuthRevision = authInfo.Revision
}
}

data, err := r.Marshal()
Expand Down