From 91b830d350bbe28f0a563c85dd4cc3aaacb6f2c0 Mon Sep 17 00:00:00 2001 From: bsbds <69835502+bsbds@users.noreply.github.com> Date: Wed, 9 Aug 2023 19:08:18 +0800 Subject: [PATCH] etcdserver: fix `checkIntervals` will not validate correctly Signed-off-by: bsbds <69835502+bsbds@users.noreply.github.com> --- server/etcdserver/api/v3rpc/key.go | 63 +++++++--- server/etcdserver/api/v3rpc/key_test.go | 161 ++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 18 deletions(-) diff --git a/server/etcdserver/api/v3rpc/key.go b/server/etcdserver/api/v3rpc/key.go index 2c1de2a90..3616e4a3e 100644 --- a/server/etcdserver/api/v3rpc/key.go +++ b/server/etcdserver/api/v3rpc/key.go @@ -182,6 +182,7 @@ func checkTxnRequest(r *pb.TxnRequest, maxTxnOps int) error { // there is an overlap, returns an error. If no overlap, return put and delete // sets for recursive evaluation. func checkIntervals(reqs []*pb.RequestOp) (map[string]struct{}, adt.IntervalTree, error) { + puts := make(map[string]struct{}) dels := adt.NewIntervalTree() // collect deletes from this level; build first to check lower level overlapped puts @@ -203,8 +204,13 @@ func checkIntervals(reqs []*pb.RequestOp) (map[string]struct{}, adt.IntervalTree dels.Insert(iv, struct{}{}) } - // collect children puts/deletes - puts := make(map[string]struct{}) + type Txn struct { + Puts map[string]struct{} + Dels adt.IntervalTree + } + var txns []Txn + + // for each txn, collect all puts and deletes of children level for _, req := range reqs { tv, ok := req.Request.(*pb.RequestOp_RequestTxn) if !ok { @@ -218,33 +224,53 @@ func checkIntervals(reqs []*pb.RequestOp) (map[string]struct{}, adt.IntervalTree if err != nil { return nil, dels, err } - for k := range putsThen { - if _, ok := puts[k]; ok { - return nil, dels, rpctypes.ErrGRPCDuplicateKey - } - if dels.Intersects(adt.NewStringAffinePoint(k)) { - return nil, dels, rpctypes.ErrGRPCDuplicateKey - } - puts[k] = struct{}{} - } + + delsThen.Union(delsElse, adt.NewStringAffineInterval("\x00", "")) for k := range putsElse { - if _, ok := puts[k]; ok { - // if key is from putsThen, overlap is OK since - // either then/else are mutually exclusive - if _, isSafe := putsThen[k]; !isSafe { + putsThen[k] = struct{}{} + } + txns = append(txns, Txn{Puts: putsThen, Dels: delsThen}) + } + + // 1. puts in a txn's children level should not overlap with deletes in another txn's children level + // + // Note: This operation is necessary despite the relatively high time complexity. + // The rationale behind this is that for a single txn, its success and failure branches can safely overlap, + // which means that we cannot simply test overlapping using a union all deletes. + for i, txn_x := range txns { + for _, txn_y := range txns[i+1:] { + for k := range txn_x.Puts { + if txn_y.Dels.Intersects(adt.NewStringAffinePoint(k)) { return nil, dels, rpctypes.ErrGRPCDuplicateKey } } + for k := range txn_y.Puts { + if txn_x.Dels.Intersects(adt.NewStringAffinePoint(k)) { + return nil, dels, rpctypes.ErrGRPCDuplicateKey + } + } + } + } + + // 2. puts in a txn's children level also should not overlap with deletes in current level + for _, txn := range txns { + for k := range txn.Puts { + if _, ok := puts[k]; ok { + return nil, dels, rpctypes.ErrGRPCDuplicateKey + } if dels.Intersects(adt.NewStringAffinePoint(k)) { return nil, dels, rpctypes.ErrGRPCDuplicateKey } puts[k] = struct{}{} } - dels.Union(delsThen, adt.NewStringAffineInterval("\x00", "")) - dels.Union(delsElse, adt.NewStringAffineInterval("\x00", "")) } - // collect and check this level's puts + for _, txn := range txns { + dels.Union(txn.Dels, adt.NewStringAffineInterval("\x00", "")) + } + + // 3. puts in current level should not overlap with deletes in current level and deletes + // in all txn children levels for _, req := range reqs { tv, ok := req.Request.(*pb.RequestOp_RequestPut) if !ok || tv.RequestPut == nil { @@ -259,6 +285,7 @@ func checkIntervals(reqs []*pb.RequestOp) (map[string]struct{}, adt.IntervalTree } puts[k] = struct{}{} } + return puts, dels, nil } diff --git a/server/etcdserver/api/v3rpc/key_test.go b/server/etcdserver/api/v3rpc/key_test.go index a585ee89c..732869320 100644 --- a/server/etcdserver/api/v3rpc/key_test.go +++ b/server/etcdserver/api/v3rpc/key_test.go @@ -19,6 +19,8 @@ import ( pb "go.etcd.io/etcd/api/v3/etcdserverpb" "go.etcd.io/etcd/api/v3/v3rpc/rpctypes" + + "github.com/stretchr/testify/assert" ) func TestCheckRangeRequest(t *testing.T) { @@ -66,3 +68,162 @@ func getError(err error) string { return err.Error() } + +func TestCheckIntervals(t *testing.T) { + tests := []struct { + name string + requestOp []*pb.RequestOp + err error + }{ + // check ok + { + name: "No duplicate key is ok", + requestOp: []*pb.RequestOp{put_op, put_op1}, + err: nil, + }, + { + name: "Nested no duplicate key is ok", + requestOp: []*pb.RequestOp{txn_op_put_success, txn_op_put_success1}, + err: nil, + }, + { + name: "Overlap in different branch is ok", + requestOp: []*pb.RequestOp{ + { + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Success: []*pb.RequestOp{put_op}, + Failure: []*pb.RequestOp{del_op}, + }, + }, + }, + }, + err: nil, + }, + // check err + { + name: "Duplicate key should fail", + requestOp: []*pb.RequestOp{put_op, put_op}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Nested duplicate key should fail", + requestOp: []*pb.RequestOp{txn_op_put_success, txn_op_put_success}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Overlap put and delete should fail", + requestOp: []*pb.RequestOp{put_op, del_op}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Overlap put-in-txn and delete should fail", + requestOp: []*pb.RequestOp{txn_op_put_success, del_op}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Overlap put and delete-in-txn should fail", + requestOp: []*pb.RequestOp{put_op, txn_op_del_success}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Nested overlap put and delete should fail, case 0", + requestOp: []*pb.RequestOp{txn_op_put_success, txn_op_del_success}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Nested overlap put and delete should fail, case 1", + requestOp: []*pb.RequestOp{txn_op_put_success, txn_op_del_failure}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Nested overlap put and delete should fail, case 2", + requestOp: []*pb.RequestOp{txn_op_put_failure, txn_op_del_success}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + { + name: "Nested overlap put and delete should fail, case 3 (with the order swapped)", + requestOp: []*pb.RequestOp{txn_op_del_success, txn_op_put_success}, + err: rpctypes.ErrGRPCDuplicateKey, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, _, err := checkIntervals(tt.requestOp) + assert.Equal(t, tt.err, err) + }) + } + +} + +// TestCheckIntervals variables setup. +var ( + put_op = &pb.RequestOp{ + Request: &pb.RequestOp_RequestPut{ + RequestPut: &pb.PutRequest{ + Key: []byte("foo1"), + }, + }, + } + put_op1 = &pb.RequestOp{ + Request: &pb.RequestOp_RequestPut{ + RequestPut: &pb.PutRequest{ + Key: []byte("foo2"), + }, + }, + } + // overlap with `put_op` + del_op = &pb.RequestOp{ + Request: &pb.RequestOp_RequestDeleteRange{ + RequestDeleteRange: &pb.DeleteRangeRequest{ + Key: []byte("foo0"), + RangeEnd: []byte("foo3"), + }, + }, + } + // does not overlap with `put_op` + del_op1 = &pb.RequestOp{ + Request: &pb.RequestOp_RequestDeleteRange{ + RequestDeleteRange: &pb.DeleteRangeRequest{ + Key: []byte("foo2"), + RangeEnd: []byte("foo3"), + }, + }, + } + txn_op_put_success = &pb.RequestOp{ + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Success: []*pb.RequestOp{put_op}, + }, + }, + } + txn_op_put_success1 = &pb.RequestOp{ + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Success: []*pb.RequestOp{put_op1}, + }, + }, + } + txn_op_put_failure = &pb.RequestOp{ + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Success: []*pb.RequestOp{put_op}, + }, + }, + } + txn_op_del_success = &pb.RequestOp{ + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Success: []*pb.RequestOp{del_op}, + }, + }, + } + txn_op_del_failure = &pb.RequestOp{ + Request: &pb.RequestOp_RequestTxn{ + RequestTxn: &pb.TxnRequest{ + Failure: []*pb.RequestOp{del_op}, + }, + }, + } +)