From 70d2cb5f4d5d8453c01038ed086e7bb92353b21b Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 24 Feb 2017 18:26:56 -0800 Subject: [PATCH] rpc: Remove time check for each RPC calls. (#3804) This removal comes to avoid some redundant requirements which are adding more problems on a production setup. Here are the list of checks for time as they happen - Fresh connect (during server startup) - CORRECT - A reconnect after network disconnect - CORRECT - For each RPC call - INCORRECT. Verifying time for each RPC aggravates a situation where a RPC call is rejected in a sequence of events due to enough load on a production setup. 3 second might not be enough time window for the call to be initiated and received by the server. --- cmd/admin-rpc-server_test.go | 11 ++++------- cmd/auth-rpc-client.go | 3 --- cmd/lock-rpc-server_test.go | 21 --------------------- cmd/rpc-common.go | 14 -------------- cmd/s3-peer-rpc-handlers_test.go | 3 +-- cmd/storage-rpc-server_test.go | 13 ------------- 6 files changed, 5 insertions(+), 60 deletions(-) diff --git a/cmd/admin-rpc-server_test.go b/cmd/admin-rpc-server_test.go index dbc097a8d..44705a7a2 100644 --- a/cmd/admin-rpc-server_test.go +++ b/cmd/admin-rpc-server_test.go @@ -53,7 +53,7 @@ func testAdminCmd(cmd cmdType, t *testing.T) { <-globalServiceSignalCh }() - ga := AuthRPCArgs{AuthToken: reply.AuthToken, RequestTime: time.Now().UTC()} + ga := AuthRPCArgs{AuthToken: reply.AuthToken} genReply := AuthRPCReply{} switch cmd { case restartCmd: @@ -108,8 +108,7 @@ func TestReInitDisks(t *testing.T) { } authArgs := AuthRPCArgs{ - AuthToken: reply.AuthToken, - RequestTime: time.Now().UTC(), + AuthToken: reply.AuthToken, } authReply := AuthRPCReply{} @@ -134,8 +133,7 @@ func TestReInitDisks(t *testing.T) { } authArgs = AuthRPCArgs{ - AuthToken: fsReply.AuthToken, - RequestTime: time.Now().UTC(), + AuthToken: fsReply.AuthToken, } authReply = AuthRPCReply{} // Attempt ReInitDisks service on a FS backend. @@ -171,8 +169,7 @@ func TestGetConfig(t *testing.T) { } authArgs := AuthRPCArgs{ - AuthToken: reply.AuthToken, - RequestTime: time.Now().UTC(), + AuthToken: reply.AuthToken, } configReply := ConfigReply{} diff --git a/cmd/auth-rpc-client.go b/cmd/auth-rpc-client.go index f59fb8b5f..c8385badf 100644 --- a/cmd/auth-rpc-client.go +++ b/cmd/auth-rpc-client.go @@ -111,7 +111,6 @@ func (authClient *AuthRPCClient) Login() (err error) { // call makes a RPC call after logs into the server. func (authClient *AuthRPCClient) call(serviceMethod string, args interface { SetAuthToken(authToken string) - SetRequestTime(requestTime time.Time) }, reply interface{}) (err error) { // On successful login, execute RPC call. if err = authClient.Login(); err == nil { @@ -119,7 +118,6 @@ func (authClient *AuthRPCClient) call(serviceMethod string, args interface { // Set token and timestamp before the rpc call. args.SetAuthToken(authClient.authToken) authClient.Unlock() - args.SetRequestTime(time.Now().UTC()) // Do RPC call. err = authClient.rpcClient.Call(serviceMethod, args, reply) @@ -130,7 +128,6 @@ func (authClient *AuthRPCClient) call(serviceMethod string, args interface { // Call executes RPC call till success or globalAuthRPCRetryThreshold on ErrShutdown. func (authClient *AuthRPCClient) Call(serviceMethod string, args interface { SetAuthToken(authToken string) - SetRequestTime(requestTime time.Time) }, reply interface{}) (err error) { // Done channel is used to close any lingering retry routine, as soon diff --git a/cmd/lock-rpc-server_test.go b/cmd/lock-rpc-server_test.go index 3251a37bb..90661ca9b 100644 --- a/cmd/lock-rpc-server_test.go +++ b/cmd/lock-rpc-server_test.go @@ -85,7 +85,6 @@ func TestLockRpcServerLock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // Claim a lock var result bool @@ -119,7 +118,6 @@ func TestLockRpcServerLock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la2.SetAuthToken(token) - la2.SetRequestTime(time.Now().UTC()) err = locker.Lock(&la2, &result) if err != nil { @@ -143,7 +141,6 @@ func TestLockRpcServerUnlock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // First test return of error when attempting to unlock a lock that does not exist var result bool @@ -153,7 +150,6 @@ func TestLockRpcServerUnlock(t *testing.T) { } // Create lock (so that we can release) - la.SetRequestTime(time.Now().UTC()) err = locker.Lock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -162,7 +158,6 @@ func TestLockRpcServerUnlock(t *testing.T) { } // Finally test successful release of lock - la.SetRequestTime(time.Now().UTC()) err = locker.Unlock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -191,7 +186,6 @@ func TestLockRpcServerRLock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // Claim a lock var result bool @@ -225,7 +219,6 @@ func TestLockRpcServerRLock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la2.SetAuthToken(token) - la2.SetRequestTime(time.Now().UTC()) err = locker.RLock(&la2, &result) if err != nil { @@ -249,7 +242,6 @@ func TestLockRpcServerRUnlock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // First test return of error when attempting to unlock a read-lock that does not exist var result bool @@ -259,7 +251,6 @@ func TestLockRpcServerRUnlock(t *testing.T) { } // Create first lock ... (so that we can release) - la.SetRequestTime(time.Now().UTC()) err = locker.RLock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -275,7 +266,6 @@ func TestLockRpcServerRUnlock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la2.SetAuthToken(token) - la2.SetRequestTime(time.Now().UTC()) // ... and create a second lock on same resource err = locker.RLock(&la2, &result) @@ -286,7 +276,6 @@ func TestLockRpcServerRUnlock(t *testing.T) { } // Test successful release of first read lock - la.SetRequestTime(time.Now().UTC()) err = locker.RUnlock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -311,7 +300,6 @@ func TestLockRpcServerRUnlock(t *testing.T) { } // Finally test successful release of second (and last) read lock - la2.SetRequestTime(time.Now().UTC()) err = locker.RUnlock(&la2, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -340,7 +328,6 @@ func TestLockRpcServerForceUnlock(t *testing.T) { ServiceEndpoint: "rpc-path", }) laForce.SetAuthToken(token) - laForce.SetRequestTime(time.Now().UTC()) // First test that UID should be empty var result bool @@ -351,7 +338,6 @@ func TestLockRpcServerForceUnlock(t *testing.T) { // Then test force unlock of a lock that does not exist (not returning an error) laForce.LockArgs.UID = "" - laForce.SetRequestTime(time.Now().UTC()) err = locker.ForceUnlock(&laForce, &result) if err != nil { t.Errorf("Expected no error, got %#v", err) @@ -364,7 +350,6 @@ func TestLockRpcServerForceUnlock(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // Create lock ... (so that we can force unlock) err = locker.Lock(&la, &result) @@ -375,14 +360,12 @@ func TestLockRpcServerForceUnlock(t *testing.T) { } // Forcefully unlock the lock (not returning an error) - laForce.SetRequestTime(time.Now().UTC()) err = locker.ForceUnlock(&laForce, &result) if err != nil { t.Errorf("Expected no error, got %#v", err) } // Try to get lock again (should be granted) - la.SetRequestTime(time.Now().UTC()) err = locker.Lock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -391,7 +374,6 @@ func TestLockRpcServerForceUnlock(t *testing.T) { } // Finally forcefully unlock the lock once again - laForce.SetRequestTime(time.Now().UTC()) err = locker.ForceUnlock(&laForce, &result) if err != nil { t.Errorf("Expected no error, got %#v", err) @@ -410,7 +392,6 @@ func TestLockRpcServerExpired(t *testing.T) { ServiceEndpoint: "rpc-path", }) la.SetAuthToken(token) - la.SetRequestTime(time.Now().UTC()) // Unknown lock at server will return expired = true var expired bool @@ -425,7 +406,6 @@ func TestLockRpcServerExpired(t *testing.T) { // Create lock (so that we can test that it is not expired) var result bool - la.SetRequestTime(time.Now().UTC()) err = locker.Lock(&la, &result) if err != nil { t.Errorf("Expected %#v, got %#v", nil, err) @@ -433,7 +413,6 @@ func TestLockRpcServerExpired(t *testing.T) { t.Errorf("Expected %#v, got %#v", true, result) } - la.SetRequestTime(time.Now().UTC()) err = locker.Expired(&la, &expired) if err != nil { t.Errorf("Expected no error, got %#v", err) diff --git a/cmd/rpc-common.go b/cmd/rpc-common.go index eeaa5e0c3..876f335d6 100644 --- a/cmd/rpc-common.go +++ b/cmd/rpc-common.go @@ -37,10 +37,6 @@ func isRequestTimeAllowed(requestTime time.Time) bool { type AuthRPCArgs struct { // Authentication token to be verified by the server for every RPC call. AuthToken string - - // Request time to be verified by the server for every RPC call. - // This is an addition check over Authentication token for time drifting. - RequestTime time.Time } // SetAuthToken - sets the token to the supplied value. @@ -48,11 +44,6 @@ func (args *AuthRPCArgs) SetAuthToken(authToken string) { args.AuthToken = authToken } -// SetRequestTime - sets the requestTime to the supplied value. -func (args *AuthRPCArgs) SetRequestTime(requestTime time.Time) { - args.RequestTime = requestTime -} - // IsAuthenticated - validated whether this auth RPC args are already authenticated or not. func (args AuthRPCArgs) IsAuthenticated() error { // Check whether the token is valid @@ -60,11 +51,6 @@ func (args AuthRPCArgs) IsAuthenticated() error { return errInvalidToken } - // Check if the request time is within the allowed skew limit. - if !isRequestTimeAllowed(args.RequestTime) { - return errServerTimeMismatch - } - // Good to go. return nil } diff --git a/cmd/s3-peer-rpc-handlers_test.go b/cmd/s3-peer-rpc-handlers_test.go index 4e7378cb9..825539940 100644 --- a/cmd/s3-peer-rpc-handlers_test.go +++ b/cmd/s3-peer-rpc-handlers_test.go @@ -20,7 +20,6 @@ import ( "encoding/json" "path" "testing" - "time" ) type TestRPCS3PeerSuite struct { @@ -62,7 +61,7 @@ func TestS3PeerRPC(t *testing.T) { // Test S3 RPC handlers func (s *TestRPCS3PeerSuite) testS3PeerRPC(t *testing.T) { // Validate for invalid token. - args := AuthRPCArgs{AuthToken: "garbage", RequestTime: time.Now().UTC()} + args := AuthRPCArgs{AuthToken: "garbage"} rclient := newRPCClient(s.testAuthConf.serverAddr, s.testAuthConf.serviceEndpoint, false) defer rclient.Close() err := rclient.Call("S3.SetBucketNotificationPeer", &args, &AuthRPCReply{}) diff --git a/cmd/storage-rpc-server_test.go b/cmd/storage-rpc-server_test.go index 48082e8ad..9b5e001fa 100644 --- a/cmd/storage-rpc-server_test.go +++ b/cmd/storage-rpc-server_test.go @@ -98,33 +98,28 @@ func TestStorageRPCInvalidToken(t *testing.T) { } // 1. DiskInfoHandler diskInfoReply := &disk.Info{} - badAuthRPCArgs.RequestTime = time.Now().UTC() err = storageRPC.DiskInfoHandler(&badAuthRPCArgs, diskInfoReply) errorIfInvalidToken(t, err) // 2. MakeVolHandler makeVolArgs := &badGenericVolArgs - makeVolArgs.AuthRPCArgs.RequestTime = time.Now().UTC() makeVolReply := &AuthRPCReply{} err = storageRPC.MakeVolHandler(makeVolArgs, makeVolReply) errorIfInvalidToken(t, err) // 3. ListVolsHandler listVolReply := &ListVolsReply{} - badAuthRPCArgs.RequestTime = time.Now().UTC() err = storageRPC.ListVolsHandler(&badAuthRPCArgs, listVolReply) errorIfInvalidToken(t, err) // 4. StatVolHandler statVolReply := &VolInfo{} statVolArgs := &badGenericVolArgs - statVolArgs.AuthRPCArgs.RequestTime = time.Now().UTC() err = storageRPC.StatVolHandler(statVolArgs, statVolReply) errorIfInvalidToken(t, err) // 5. DeleteVolHandler deleteVolArgs := &badGenericVolArgs - deleteVolArgs.AuthRPCArgs.RequestTime = time.Now().UTC() deleteVolReply := &AuthRPCReply{} err = storageRPC.DeleteVolHandler(deleteVolArgs, deleteVolReply) errorIfInvalidToken(t, err) @@ -133,7 +128,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { statFileArgs := &StatFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - statFileArgs.AuthRPCArgs.RequestTime = time.Now().UTC() statReply := &FileInfo{} err = storageRPC.StatFileHandler(statFileArgs, statReply) errorIfInvalidToken(t, err) @@ -142,7 +136,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { listDirArgs := &ListDirArgs{ AuthRPCArgs: badAuthRPCArgs, } - listDirArgs.AuthRPCArgs.RequestTime = time.Now().UTC() listDirReply := &[]string{} err = storageRPC.ListDirHandler(listDirArgs, listDirReply) errorIfInvalidToken(t, err) @@ -151,13 +144,11 @@ func TestStorageRPCInvalidToken(t *testing.T) { readFileArgs := &ReadFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - readFileArgs.AuthRPCArgs.RequestTime = time.Now().UTC() readFileReply := &[]byte{} err = storageRPC.ReadAllHandler(readFileArgs, readFileReply) errorIfInvalidToken(t, err) // 9. ReadFileHandler - readFileArgs.AuthRPCArgs.RequestTime = time.Now().UTC() err = storageRPC.ReadFileHandler(readFileArgs, readFileReply) errorIfInvalidToken(t, err) @@ -165,7 +156,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { prepFileArgs := &PrepareFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - prepFileArgs.AuthRPCArgs.RequestTime = time.Now().UTC() prepFileReply := &AuthRPCReply{} err = storageRPC.PrepareFileHandler(prepFileArgs, prepFileReply) errorIfInvalidToken(t, err) @@ -174,7 +164,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { appendArgs := &AppendFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - appendArgs.AuthRPCArgs.RequestTime = time.Now().UTC() appendReply := &AuthRPCReply{} err = storageRPC.AppendFileHandler(appendArgs, appendReply) errorIfInvalidToken(t, err) @@ -183,7 +172,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { delFileArgs := &DeleteFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - delFileArgs.AuthRPCArgs.RequestTime = time.Now().UTC() delFileRely := &AuthRPCReply{} err = storageRPC.DeleteFileHandler(delFileArgs, delFileRely) errorIfInvalidToken(t, err) @@ -192,7 +180,6 @@ func TestStorageRPCInvalidToken(t *testing.T) { renameArgs := &RenameFileArgs{ AuthRPCArgs: badAuthRPCArgs, } - renameArgs.AuthRPCArgs.RequestTime = time.Now().UTC() renameReply := &AuthRPCReply{} err = storageRPC.RenameFileHandler(renameArgs, renameReply) errorIfInvalidToken(t, err)