From 5c68f2e5107fa02bfe33ec0a5de2cfdc56b79832 Mon Sep 17 00:00:00 2001
From: ahrtr <wachao@vmware.com>
Date: Fri, 15 Apr 2022 06:59:34 +0800
Subject: [PATCH] Update conssitent_index when applying fails

When clients have no permission to perform whatever operation, then
the applying may fail. We should also move consistent_index forward
in this case, otherwise the consitent_index may smaller than the
snapshot index.
---
 server/etcdserver/server.go       | 16 ++++++++--------
 tests/integration/v3_auth_test.go | 27 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/server/etcdserver/server.go b/server/etcdserver/server.go
index 64255cf43e3..4529a68bb20 100644
--- a/server/etcdserver/server.go
+++ b/server/etcdserver/server.go
@@ -2167,18 +2167,19 @@ func (s *EtcdServer) apply(
 func (s *EtcdServer) applyEntryNormal(e *raftpb.Entry) {
 	shouldApplyV3 := membership.ApplyV2storeOnly
 	applyV3Performed := false
-	defer func() {
-		// The txPostLock callback will not get called in this case,
-		// so we should set the consistent index directly.
-		if s.consistIndex != nil && !applyV3Performed && membership.ApplyBoth == shouldApplyV3 {
-			s.consistIndex.SetConsistentIndex(e.Index, e.Term)
-		}
-	}()
+	var ar *applyResult
 	index := s.consistIndex.ConsistentIndex()
 	if e.Index > index {
 		// set the consistent index of current executing entry
 		s.consistIndex.SetConsistentApplyingIndex(e.Index, e.Term)
 		shouldApplyV3 = membership.ApplyBoth
+		defer func() {
+			// The txPostLockInsideApplyHook will not get called in some cases,
+			// in which we should move the consistent index forward directly.
+			if !applyV3Performed || (ar != nil && ar.err != nil) {
+				s.consistIndex.SetConsistentIndex(e.Index, e.Term)
+			}
+		}()
 	}
 	s.lg.Debug("apply entry normal",
 		zap.Uint64("consistent-index", index),
@@ -2220,7 +2221,6 @@ func (s *EtcdServer) applyEntryNormal(e *raftpb.Entry) {
 		id = raftReq.Header.ID
 	}
 
-	var ar *applyResult
 	needResult := s.w.IsRegistered(id)
 	if needResult || !noSideEffect(&raftReq) {
 		if !needResult && raftReq.Txn != nil {
diff --git a/tests/integration/v3_auth_test.go b/tests/integration/v3_auth_test.go
index 286f2dbe67f..862ebc9688f 100644
--- a/tests/integration/v3_auth_test.go
+++ b/tests/integration/v3_auth_test.go
@@ -46,6 +46,33 @@ func TestV3AuthEmptyUserGet(t *testing.T) {
 	}
 }
 
+// TestV3AuthEmptyUserPut ensures that a put with an empty user will return an empty user error,
+// and the consistent_index should be moved forward even the apply-->Put fails.
+func TestV3AuthEmptyUserPut(t *testing.T) {
+	BeforeTest(t)
+	clus := NewClusterV3(t, &ClusterConfig{
+		Size:          1,
+		SnapshotCount: 3,
+	})
+	defer clus.Terminate(t)
+
+	ctx, cancel := context.WithTimeout(context.TODO(), 30*time.Second)
+	defer cancel()
+
+	api := toGRPC(clus.Client(0))
+	authSetupRoot(t, api.Auth)
+
+	// The SnapshotCount is 3, so there must be at least 3 new snapshot files being created.
+	// The VERIFY logic will check whether the consistent_index >= last snapshot index on
+	// cluster terminating.
+	for i := 0; i < 10; i++ {
+		_, err := api.KV.Put(ctx, &pb.PutRequest{Key: []byte("foo"), Value: []byte("bar")})
+		if !eqErrGRPC(err, rpctypes.ErrUserEmpty) {
+			t.Fatalf("got %v, expected %v", err, rpctypes.ErrUserEmpty)
+		}
+	}
+}
+
 // TestV3AuthTokenWithDisable tests that auth won't crash if
 // given a valid token when authentication is disabled
 func TestV3AuthTokenWithDisable(t *testing.T) {