From 2b5647503c16285e52c88f85e1a99897a3bd48c3 Mon Sep 17 00:00:00 2001
From: Pierre Cauchois <pierre@cloudflare.com>
Date: Thu, 15 Oct 2020 05:43:06 -0700
Subject: [PATCH] Enforce bounds on MaxQueryTime (#9064)

The MaxQueryTime value used in QueryOptions.HasTimedOut() can be set to
an invalid value that would throw off how RPC requests are retried.

This fix uses the same logic that enforces the MaxQueryTime bounds in the
blockingRPC() call.
---
 nomad/rpc.go             | 13 +++----------
 nomad/structs/structs.go | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/nomad/rpc.go b/nomad/rpc.go
index 69cc49b06..b2d1ad920 100644
--- a/nomad/rpc.go
+++ b/nomad/rpc.go
@@ -31,13 +31,6 @@ import (
 )
 
 const (
-	// maxQueryTime is used to bound the limit of a blocking query
-	maxQueryTime = 300 * time.Second
-
-	// defaultQueryTime is the amount of time we block waiting for a change
-	// if no time is specified. Previously we would wait the maxQueryTime.
-	defaultQueryTime = 300 * time.Second
-
 	// Warn if the Raft command is larger than this.
 	// If it's over 1MB something is probably being abusive.
 	raftWarnSize = 1024 * 1024
@@ -788,10 +781,10 @@ func (r *rpcHandler) blockingRPC(opts *blockingOptions) error {
 	}
 
 	// Restrict the max query time, and ensure there is always one
-	if opts.queryOpts.MaxQueryTime > maxQueryTime {
-		opts.queryOpts.MaxQueryTime = maxQueryTime
+	if opts.queryOpts.MaxQueryTime > structs.MaxBlockingRPCQueryTime {
+		opts.queryOpts.MaxQueryTime = structs.MaxBlockingRPCQueryTime
 	} else if opts.queryOpts.MaxQueryTime <= 0 {
-		opts.queryOpts.MaxQueryTime = defaultQueryTime
+		opts.queryOpts.MaxQueryTime = structs.DefaultBlockingRPCQueryTime
 	}
 
 	// Apply a small amount of jitter to the request
diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
index bcc0a8a5e..409e9b332 100644
--- a/nomad/structs/structs.go
+++ b/nomad/structs/structs.go
@@ -24,6 +24,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/cronexpr"
 	"github.com/hashicorp/go-msgpack/codec"
 	"github.com/hashicorp/go-multierror"
@@ -163,6 +164,13 @@ const (
 
 	// Normalized scorer name
 	NormScorerName = "normalized-score"
+
+	// MaxBlockingRPCQueryTime is used to bound the limit of a blocking query
+	MaxBlockingRPCQueryTime = 300 * time.Second
+
+	// DefaultBlockingRPCQueryTime is the amount of time we block waiting for a change
+	// if no time is specified. Previously we would wait the MaxBlockingRPCQueryTime.
+	DefaultBlockingRPCQueryTime = 300 * time.Second
 )
 
 // Context defines the scope in which a search for Nomad object operates, and
@@ -289,6 +297,14 @@ func (q QueryOptions) AllowStaleRead() bool {
 
 func (q QueryOptions) HasTimedOut(start time.Time, rpcHoldTimeout time.Duration) bool {
 	if q.MinQueryIndex > 0 {
+		// Restrict the max query time, and ensure there is always one
+		if q.MaxQueryTime > MaxBlockingRPCQueryTime {
+			q.MaxQueryTime = MaxBlockingRPCQueryTime
+		} else if q.MaxQueryTime <= 0 {
+			q.MaxQueryTime = DefaultBlockingRPCQueryTime
+		}
+		q.MaxQueryTime += lib.RandomStagger(q.MaxQueryTime / JitterFraction)
+
 		return time.Since(start) > (q.MaxQueryTime + rpcHoldTimeout)
 	}
 	return time.Since(start) > rpcHoldTimeout