Respect increment value in grace period calculations (api/LifetimeWatcher) (#14836)

api/lifetime_watcher.go

@@ -113,7 +113,9 @@ type LifetimeWatcherInput struct {
 
 	// The new TTL, in seconds, that should be set on the lease. The TTL set
 	// here may or may not be honored by the vault server, based on Vault
-	// configuration or any associated max TTL values.
+	// configuration or any associated max TTL values. If specified, the
+	// minimum of this value and the remaining lease duration will be used
+	// for grace period calculations.
 	Increment int
 
 	// RenewBehavior controls what happens when a renewal errors or the
@@ -257,7 +259,7 @@ func (r *LifetimeWatcher) doRenewWithOptions(tokenMode bool, nonRenewable bool,
 
 	initialTime := time.Now()
 	priorDuration := time.Duration(initLeaseDuration) * time.Second
-	r.calculateGrace(priorDuration)
+	r.calculateGrace(priorDuration, time.Duration(r.increment)*time.Second)
 	var errorBackoff backoff.BackOff
 
 	for {
@@ -345,7 +347,7 @@ func (r *LifetimeWatcher) doRenewWithOptions(tokenMode bool, nonRenewable bool,
 			// extending. Once it stops extending, we've hit the max and need to
 			// rely on the grace duration.
 			if remainingLeaseDuration > priorDuration {
-				r.calculateGrace(remainingLeaseDuration)
+				r.calculateGrace(remainingLeaseDuration, time.Duration(r.increment)*time.Second)
 			}
 			priorDuration = remainingLeaseDuration
 
@@ -373,16 +375,21 @@ func (r *LifetimeWatcher) doRenewWithOptions(tokenMode bool, nonRenewable bool,
 	}
 }
 
-// calculateGrace calculates the grace period based on a reasonable set of
+// calculateGrace calculates the grace period based on the minimum of the
-// assumptions given the total lease time; it also adds some jitter to not have
+// remaining lease duration and the token increment value; it also adds some
-// clients be in sync.
+// jitter to not have clients be in sync.
-func (r *LifetimeWatcher) calculateGrace(leaseDuration time.Duration) {
+func (r *LifetimeWatcher) calculateGrace(leaseDuration, increment time.Duration) {
-	if leaseDuration <= 0 {
+	minDuration := leaseDuration
+	if minDuration > increment && increment > 0 {
+		minDuration = increment
+	}
+
+	if minDuration <= 0 {
 		r.grace = 0
 		return
 	}
 
-	leaseNanos := float64(leaseDuration.Nanoseconds())
+	leaseNanos := float64(minDuration.Nanoseconds())
 	jitterMax := 0.1 * leaseNanos
 
 	// For a given lease duration, we want to allow 80-90% of that to elapse,

api/renewer_test.go

@@ -24,28 +24,28 @@ func TestRenewer_NewRenewer(t *testing.T) {
 		err  bool
 	}{
 		{
-			"nil",
+			name: "nil",
-			nil,
+			i:    nil,
-			nil,
+			e:    nil,
-			true,
+			err:  true,
 		},
 		{
-			"missing_secret",
+			name: "missing_secret",
-			&RenewerInput{
+			i: &RenewerInput{
 				Secret: nil,
 			},
-			nil,
+			e:   nil,
-			true,
+			err: true,
 		},
 		{
-			"default_grace",
+			name: "default_grace",
-			&RenewerInput{
+			i: &RenewerInput{
 				Secret: &Secret{},
 			},
-			&Renewer{
+			e: &Renewer{
 				secret: &Secret{},
 			},
-			false,
+			err: false,
 		},
 	}
 
@@ -98,67 +98,78 @@ func TestLifetimeWatcher(t *testing.T) {
 		expectRenewal        bool
 	}{
 		{
-			time.Second,
+			maxTestTime:          time.Second,
-			"no_error",
+			name:                 "no_error",
-			60,
+			leaseDurationSeconds: 60,
-			60,
+			incrementSeconds:     60,
-			func(_ string, _ int) (*Secret, error) {
+			renew: func(_ string, _ int) (*Secret, error) {
 				return renewedSecret, nil
 			},
-			nil,
+			expectError:   nil,
-			true,
+			expectRenewal: true,
 		},
 		{
-			5 * time.Second,
+			maxTestTime:          time.Second,
-			"one_error",
+			name:                 "short_increment_duration",
-			15,
+			leaseDurationSeconds: 60,
-			15,
+			incrementSeconds:     10,
-			func(_ string, _ int) (*Secret, error) {
+			renew: func(_ string, _ int) (*Secret, error) {
+				return renewedSecret, nil
+			},
+			expectError:   nil,
+			expectRenewal: true,
+		},
+		{
+			maxTestTime:          5 * time.Second,
+			name:                 "one_error",
+			leaseDurationSeconds: 15,
+			incrementSeconds:     15,
+			renew: func(_ string, _ int) (*Secret, error) {
 				if caseOneErrorCount == 0 {
 					caseOneErrorCount++
 					return nil, fmt.Errorf("renew failure")
 				}
 				return renewedSecret, nil
 			},
-			nil,
+			expectError:   nil,
-			true,
+			expectRenewal: true,
 		},
 		{
-			15 * time.Second,
+			maxTestTime:          15 * time.Second,
-			"many_errors",
+			name:                 "many_errors",
-			15,
+			leaseDurationSeconds: 15,
-			15,
+			incrementSeconds:     15,
-			func(_ string, _ int) (*Secret, error) {
+			renew: func(_ string, _ int) (*Secret, error) {
 				if caseManyErrorsCount == 3 {
 					return renewedSecret, nil
 				}
 				caseManyErrorsCount++
 				return nil, fmt.Errorf("renew failure")
 			},
-			nil,
+			expectError:   nil,
-			true,
+			expectRenewal: true,
 		},
 		{
-			15 * time.Second,
+			maxTestTime:          15 * time.Second,
-			"only_errors",
+			name:                 "only_errors",
-			15,
+			leaseDurationSeconds: 15,
-			15,
+			incrementSeconds:     15,
-			func(_ string, _ int) (*Secret, error) {
+			renew: func(_ string, _ int) (*Secret, error) {
 				return nil, fmt.Errorf("renew failure")
 			},
-			nil,
+			expectError:   nil,
-			false,
+			expectRenewal: false,
 		},
 		{
-			15 * time.Second,
+			maxTestTime:          15 * time.Second,
-			"negative_lease_duration",
+			name:                 "negative_lease_duration",
-			-15,
+			leaseDurationSeconds: -15,
-			15,
+			incrementSeconds:     15,
-			func(_ string, _ int) (*Secret, error) {
+			renew: func(_ string, _ int) (*Secret, error) {
 				return renewedSecret, nil
 			},
-			nil,
+			expectError:   nil,
-			true,
+			expectRenewal: true,
 		},
 	}
 

changelog/14836.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api: Respect increment value in grace period calculations in LifetimeWatcher
+```