From 0e6da17a8f86d70d5d4c0a3cd7429b0134a16e96 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Wed, 27 Mar 2019 09:35:37 -0700
Subject: [PATCH] vault: fix renewal time

Renewal time was being calculated as 10s+Intn(lease-10s), so the renewal
time could be very rapid or within 1s of the deadline: [10s, lease)

This commit fixes the renewal time by calculating it as:

	(lease/2) +/- 10s

For a lease of 60s this means the renewal will occur in [20s, 40s).
---
 client/vaultclient/vaultclient.go      | 46 +++++++++++++++++---------
 client/vaultclient/vaultclient_test.go | 42 +++++++++++++++++++++++
 2 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/client/vaultclient/vaultclient.go b/client/vaultclient/vaultclient.go
index 5bebbd883..b5a2c3ba8 100644
--- a/client/vaultclient/vaultclient.go
+++ b/client/vaultclient/vaultclient.go
@@ -421,21 +421,9 @@ func (c *vaultClient) renew(req *vaultClientRenewalRequest) error {
 		}
 	}
 
-	duration := leaseDuration / 2
-	switch {
-	case leaseDuration < 30:
-		// Don't bother about introducing randomness if the
-		// leaseDuration is too small.
-	default:
-		// Give a breathing space of 20 seconds
-		min := 10
-		max := leaseDuration - min
-		rand.Seed(time.Now().Unix())
-		duration = min + rand.Intn(max-min)
-	}
-
 	// Determine the next renewal time
-	next := time.Now().Add(time.Duration(duration) * time.Second)
+	renewalDuration := renewalTime(rand.Intn, leaseDuration)
+	next := time.Now().Add(renewalDuration)
 
 	fatal := false
 	if renewalErr != nil &&
@@ -445,7 +433,7 @@ func (c *vaultClient) renew(req *vaultClientRenewalRequest) error {
 			strings.Contains(renewalErr.Error(), "permission denied")) {
 		fatal = true
 	} else if renewalErr != nil {
-		c.logger.Debug("renewal error details", "req.increment", req.increment, "lease_duration", leaseDuration, "duration", duration)
+		c.logger.Debug("renewal error details", "req.increment", req.increment, "lease_duration", leaseDuration, "renewal_duration", renewalDuration)
 		c.logger.Error("error during renewal of lease or token failed due to a non-fatal error; retrying",
 			"error", renewalErr, "period", next)
 	}
@@ -728,3 +716,31 @@ func (h *vaultDataHeapImp) Pop() interface{} {
 	*h = old[0 : n-1]
 	return entry
 }
+
+// randIntn is the function in math/rand needed by renewalTime. A type is used
+// to ease deterministic testing.
+type randIntn func(int) int
+
+// renewalTime returns when a token should be renewed given its leaseDuration
+// and a randomizer to provide jitter.
+//
+// Leases < 1m will be not jitter.
+func renewalTime(dice randIntn, leaseDuration int) time.Duration {
+	// Start trying to renew at half the lease duration to allow ample time
+	// for latency and retries.
+	renew := leaseDuration / 2
+
+	// Don't bother about introducing randomness if the
+	// leaseDuration is too small.
+	const cutoff = 30
+	if renew < cutoff {
+		return time.Duration(renew) * time.Second
+	}
+
+	// jitter is the amount +/- to vary the renewal time
+	const jitter = 10
+	min := renew - jitter
+	renew = min + dice(jitter*2)
+
+	return time.Duration(renew) * time.Second
+}
diff --git a/client/vaultclient/vaultclient_test.go b/client/vaultclient/vaultclient_test.go
index f3f6a879c..84068a311 100644
--- a/client/vaultclient/vaultclient_test.go
+++ b/client/vaultclient/vaultclient_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/nomad/testutil"
 	vaultapi "github.com/hashicorp/vault/api"
 	vaultconsts "github.com/hashicorp/vault/helper/consts"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -300,3 +301,44 @@ func TestVaultClient_RenewNonexistentLease(t *testing.T) {
 		t.Fatalf("expected \"%s\" or \"%s\" in error message, got \"%v\"", "lease not found", "lease is not renewable", err.Error())
 	}
 }
+
+// TestVaultClient_RenewalTime_Long asserts that for leases over 1m the renewal
+// time is jittered.
+func TestVaultClient_RenewalTime_Long(t *testing.T) {
+	t.Parallel()
+
+	// highRoller is a randIntn func that always returns the max value
+	highRoller := func(n int) int {
+		return n - 1
+	}
+
+	// lowRoller is a randIntn func that always returns the min value (0)
+	lowRoller := func(int) int {
+		return 0
+	}
+
+	assert.Equal(t, 39*time.Second, renewalTime(highRoller, 60))
+	assert.Equal(t, 20*time.Second, renewalTime(lowRoller, 60))
+
+	assert.Equal(t, 309*time.Second, renewalTime(highRoller, 600))
+	assert.Equal(t, 290*time.Second, renewalTime(lowRoller, 600))
+
+	const days3 = 60 * 60 * 24 * 3
+	assert.Equal(t, (days3/2+9)*time.Second, renewalTime(highRoller, days3))
+	assert.Equal(t, (days3/2-10)*time.Second, renewalTime(lowRoller, days3))
+}
+
+// TestVaultClient_RenewalTime_Short asserts that for leases under 1m the renewal
+// time is lease/2.
+func TestVaultClient_RenewalTime_Short(t *testing.T) {
+	t.Parallel()
+
+	dice := func(int) int {
+		require.Fail(t, "dice should not have been called")
+		panic("unreachable")
+	}
+
+	assert.Equal(t, 29*time.Second, renewalTime(dice, 58))
+	assert.Equal(t, 15*time.Second, renewalTime(dice, 30))
+	assert.Equal(t, 1*time.Second, renewalTime(dice, 2))
+}