From fd0a9fd4f3af209dd4ae47c3d9c8a53e677e797e Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Mon, 17 Jan 2022 15:09:08 -0500
Subject: [PATCH] rpc: move the index defaulting to setQueryMeta.

This safeguard should be safe to apply in general. We are already
applying it to non-blocking queries that call blockingQuery, so it
should be fine to apply it to others.
---
 agent/consul/rpc.go               | 23 +++++++++++------------
 agent/consul/txn_endpoint_test.go |  1 +
 agent/structs/structs.go          |  4 +++-
 agent/txn_endpoint_test.go        |  6 ++++--
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/agent/consul/rpc.go b/agent/consul/rpc.go
index ddf4299508..b536aad36b 100644
--- a/agent/consul/rpc.go
+++ b/agent/consul/rpc.go
@@ -984,18 +984,6 @@ func (s *Server) blockingQuery(queryOpts structs.QueryOptionsCompat, queryMeta s
 			return err
 		}
 
-		// Note we check queryOpts.MinQueryIndex is greater than zero to determine if
-		// blocking was requested by client, NOT meta.Index since the state function
-		// might return zero if something is not initialized and care wasn't taken to
-		// handle that special case (in practice this happened a lot so fixing it
-		// systematically here beats trying to remember to add zero checks in every
-		// state method). We also need to ensure that unless there is an error, we
-		// return an index > 0 otherwise the client will never block and burn CPU and
-		// requests.
-		if queryMeta.GetIndex() < 1 {
-			queryMeta.SetIndex(1)
-		}
-
 		if queryMeta.GetIndex() > minQueryIndex {
 			return nil
 		}
@@ -1034,6 +1022,17 @@ func (s *Server) setQueryMeta(m structs.QueryMetaCompat, token string) {
 		m.SetKnownLeader(s.raft.Leader() != "")
 	}
 	maskResultsFilteredByACLs(token, m)
+
+	// Always set a non-zero QueryMeta.Index. Generally we expect the
+	// QueryMeta.Index to be set to structs.RaftIndex.ModifyIndex. If the query
+	// returned no results we expect it to be set to the max index of the table,
+	// however we can't guarantee this always happens.
+	// To prevent a client from accidentally performing many non-blocking queries
+	// (which causes lots of unnecessary load), we always set a default value of 1.
+	// This is sufficient to prevent the unnecessary load in most cases.
+	if m.GetIndex() < 1 {
+		m.SetIndex(1)
+	}
 }
 
 // consistentRead is used to ensure we do not perform a stale
diff --git a/agent/consul/txn_endpoint_test.go b/agent/consul/txn_endpoint_test.go
index 9619dc8815..10178866a0 100644
--- a/agent/consul/txn_endpoint_test.go
+++ b/agent/consul/txn_endpoint_test.go
@@ -821,6 +821,7 @@ func TestTxn_Read(t *testing.T) {
 		},
 		QueryMeta: structs.QueryMeta{
 			KnownLeader: true,
+			Index:       1,
 		},
 	}
 	require.Equal(expected, out)
diff --git a/agent/structs/structs.go b/agent/structs/structs.go
index cb84f77d99..168c70efe1 100644
--- a/agent/structs/structs.go
+++ b/agent/structs/structs.go
@@ -370,7 +370,9 @@ func (q QueryBackend) String() string {
 // QueryMeta allows a query response to include potentially
 // useful metadata about a query
 type QueryMeta struct {
-	// Index in the raft log of the latest item returned by the query.
+	// Index in the raft log of the latest item returned by the query. If the
+	// query did not return any results the Index will be a value that will
+	// change when a new item is added.
 	Index uint64
 
 	// If AllowStale is used, this is time elapsed since
diff --git a/agent/txn_endpoint_test.go b/agent/txn_endpoint_test.go
index 1d6d3f01be..6b3a7c4680 100644
--- a/agent/txn_endpoint_test.go
+++ b/agent/txn_endpoint_test.go
@@ -10,11 +10,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/raft"
+	"github.com/stretchr/testify/assert"
+
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/testrpc"
-	"github.com/hashicorp/raft"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestTxnEndpoint_Bad_JSON(t *testing.T) {
@@ -385,6 +386,7 @@ func TestTxnEndpoint_KV_Actions(t *testing.T) {
 				},
 				QueryMeta: structs.QueryMeta{
 					KnownLeader: true,
+					Index:       1,
 				},
 			}
 			assert.Equal(t, expected, txnResp)