Merge pull request #20709 from serathius/watch-negative-revision-release-3.5

[release-3.5] Reject watch request with -1 revision

Author: serathius

server/etcdserver/api/v3rpc/watch.go

@@ -267,6 +267,22 @@ func (sws *serverWatchStream) recvLoop() error {
 				// support  >= key queries
 				creq.RangeEnd = []byte{}
 			}
+			if creq.StartRevision < 0 {
+				wr := &pb.WatchResponse{
+					Header:       sws.newResponseHeader(sws.watchStream.Rev()),
+					WatchId:      clientv3.InvalidWatchID,
+					Canceled:     true,
+					Created:      true,
+					CancelReason: rpctypes.ErrCompacted.Error(),
+				}
+
+				select {
+				case sws.ctrlStream <- wr:
+					continue
+				case <-sws.closec:
+					return nil
+				}
+			}
 
 			err := sws.isWatchPermitted(creq)
 			if err != nil {

server/mvcc/watchable_store.go

@@ -354,6 +354,10 @@ func (s *watchableStore) syncWatchers() int {
 	compactionRev := s.store.compactMainRev
 
 	wg, minRev := s.unsynced.choose(maxWatchersPerSync, curRev, compactionRev)
+	if minRev < 0 {
+		s.store.lg.Warn("Unexpected negative revision range start", zap.Int64("minRev", minRev))
+		minRev = 0
+	}
 	minBytes, maxBytes := newRevBytes(), newRevBytes()
 	revToBytes(revision{main: minRev}, minBytes)
 	revToBytes(revision{main: curRev + 1}, maxBytes)

server/proxy/grpcproxy/watch.go

@@ -235,6 +235,17 @@ func (wps *watchProxyStream) recvLoop() error {
 		case *pb.WatchRequest_CreateRequest:
 			cr := uv.CreateRequest
 
+			if cr.StartRevision < 0 {
+				wps.watchCh <- &pb.WatchResponse{
+					Header:       &pb.ResponseHeader{},
+					WatchId:      clientv3.InvalidWatchID,
+					Created:      true,
+					Canceled:     true,
+					CancelReason: rpctypes.ErrCompacted.Error(),
+				}
+				continue
+			}
+
 			if err := wps.checkPermissionForWatch(cr.Key, cr.RangeEnd); err != nil {
 				wps.watchCh <- &pb.WatchResponse{
 					Header:       &pb.ResponseHeader{},

tests/integration/clientv3/watch_test.go

@@ -24,10 +24,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	mvccpb "go.etcd.io/etcd/api/v3/mvccpb"
 	"go.etcd.io/etcd/api/v3/v3rpc/rpctypes"
 	"go.etcd.io/etcd/api/v3/version"
-	"go.etcd.io/etcd/client/v3"
+	clientv3 "go.etcd.io/etcd/client/v3"
 	"go.etcd.io/etcd/server/v3/etcdserver/api/v3rpc"
 	"go.etcd.io/etcd/tests/v3/integration"
 	"google.golang.org/grpc/metadata"
@@ -1214,3 +1215,77 @@ func testWatchClose(t *testing.T, wctx *watchctx) {
 		t.Fatalf("read wch got %v; expected closed channel", wresp)
 	}
 }
+
+func TestWatch(t *testing.T) {
+	integration.BeforeTest(t)
+	clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1})
+	t.Cleanup(func() { clus.Terminate(t) })
+	client := clus.Client(0)
+
+	tcs := []struct {
+		name       string
+		key        string
+		opts       []clientv3.OpOption
+		wantError  error
+		wantEvents []*clientv3.Event
+	}{
+		{
+			name:      "Watch with negative revision",
+			key:       "/",
+			opts:      []clientv3.OpOption{clientv3.WithRev(-1)},
+			wantError: rpctypes.ErrCompacted,
+		},
+		{
+			name: "Watch with zero revision",
+			key:  "/",
+			opts: []clientv3.OpOption{clientv3.WithRev(0)},
+		},
+		{
+			name: "Watch with positive revision",
+			key:  "/",
+			opts: []clientv3.OpOption{clientv3.WithRev(1)},
+		},
+	}
+	ctx := t.Context()
+
+	t.Log("Open watches")
+	watches := make([]clientv3.WatchChan, len(tcs))
+	for i, tc := range tcs {
+		watchCtx, cancel := context.WithTimeout(ctx, time.Second)
+		defer cancel()
+		watches[i] = client.Watch(watchCtx, tc.key, tc.opts...)
+	}
+
+	t.Log("Validate")
+	for i, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := collectEvents(ctx, watches[i])
+			if tc.wantError == nil {
+				require.NoError(t, err)
+			} else {
+				require.ErrorContains(t, err, tc.wantError.Error())
+			}
+		})
+	}
+}
+
+func collectEvents(ctx context.Context, watch clientv3.WatchChan) (events []*clientv3.Event, err error) {
+	for {
+		select {
+		case resp, ok := <-watch:
+			if !ok {
+				return events, nil
+			}
+			err := resp.Err()
+			if err != nil {
+				return events, err
+			}
+			events = append(events, resp.Events...)
+		case <-ctx.Done():
+			return events, ctx.Err()
+		// Watch resync interval * 1.5
+		case <-time.After(150 * time.Millisecond):
+			return events, nil
+		}
+	}
+}