mirror of
https://github.com/minio/minio.git
synced 2024-12-24 06:05:55 -05:00
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.
This commit is contained in:
parent
cff45db1b9
commit
70d2cb5f4d
@ -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{}
|
||||
|
@ -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
|
||||
|
@ -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)
|
||||
|
@ -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
|
||||
}
|
||||
|
@ -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{})
|
||||
|
@ -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)
|
||||
|
Loading…
Reference in New Issue
Block a user