From d4be7d57937c28f7b340b4e7262c3104022cfcf9 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov Date: Fri, 1 Apr 2022 10:49:40 -0400 Subject: [PATCH 1/4] Respect increment value in grace period calculations --- api/lifetime_watcher.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/api/lifetime_watcher.go b/api/lifetime_watcher.go index f775dfb15a7d..f06263526f35 100644 --- a/api/lifetime_watcher.go +++ b/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 -// assumptions given the total lease time; it also adds some jitter to not have -// clients be in sync. -func (r *LifetimeWatcher) calculateGrace(leaseDuration time.Duration) { - if leaseDuration <= 0 { +// calculateGrace calculates the grace period based on the minimum of the +// remaining lease duration and the token increment value; it also adds some +// jitter to not have clients be in sync. +func (r *LifetimeWatcher) calculateGrace(leaseDuration, increment time.Duration) { + 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, From 957966cf65287df475359ee508a317d65ba5a4f5 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov Date: Fri, 1 Apr 2022 12:40:42 -0400 Subject: [PATCH 2/4] changelog --- changelog/14836.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/14836.txt diff --git a/changelog/14836.txt b/changelog/14836.txt new file mode 100644 index 000000000000..6ee8d54a2929 --- /dev/null +++ b/changelog/14836.txt @@ -0,0 +1,3 @@ +```release-note:bug +api: Respect increment value in grace period calculations in LifetimeWatcher +``` From 4d643c9a1b51688079511ff3bdfc606b9e63d782 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov Date: Mon, 4 Apr 2022 17:03:18 -0400 Subject: [PATCH 3/4] use explicit variable names in tests --- api/renewer_test.go | 94 ++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/api/renewer_test.go b/api/renewer_test.go index 3fa9ed8e2bcf..da3d4777abfa 100644 --- a/api/renewer_test.go +++ b/api/renewer_test.go @@ -24,28 +24,28 @@ func TestRenewer_NewRenewer(t *testing.T) { err bool }{ { - "nil", - nil, - nil, - true, + name: "nil", + i: nil, + e: nil, + err: true, }, { - "missing_secret", - &RenewerInput{ + name: "missing_secret", + i: &RenewerInput{ Secret: nil, }, - nil, - true, + e: nil, + err: true, }, { - "default_grace", - &RenewerInput{ + name: "default_grace", + i: &RenewerInput{ Secret: &Secret{}, }, - &Renewer{ + e: &Renewer{ secret: &Secret{}, }, - false, + err: false, }, } @@ -98,67 +98,67 @@ func TestLifetimeWatcher(t *testing.T) { expectRenewal bool }{ { - time.Second, - "no_error", - 60, - 60, - func(_ string, _ int) (*Secret, error) { + maxTestTime: time.Second, + name: "no_error", + leaseDurationSeconds: 60, + incrementSeconds: 60, + renew: func(_ string, _ int) (*Secret, error) { return renewedSecret, nil }, - nil, - true, + expectError: nil, + expectRenewal: true, }, { - 5 * time.Second, - "one_error", - 15, - 15, - func(_ string, _ int) (*Secret, error) { + 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, - true, + expectError: nil, + expectRenewal: true, }, { - 15 * time.Second, - "many_errors", - 15, - 15, - func(_ string, _ int) (*Secret, error) { + maxTestTime: 15 * time.Second, + name: "many_errors", + leaseDurationSeconds: 15, + incrementSeconds: 15, + renew: func(_ string, _ int) (*Secret, error) { if caseManyErrorsCount == 3 { return renewedSecret, nil } caseManyErrorsCount++ return nil, fmt.Errorf("renew failure") }, - nil, - true, + expectError: nil, + expectRenewal: true, }, { - 15 * time.Second, - "only_errors", - 15, - 15, - func(_ string, _ int) (*Secret, error) { + maxTestTime: 15 * time.Second, + name: "only_errors", + leaseDurationSeconds: 15, + incrementSeconds: 15, + renew: func(_ string, _ int) (*Secret, error) { return nil, fmt.Errorf("renew failure") }, - nil, - false, + expectError: nil, + expectRenewal: false, }, { - 15 * time.Second, - "negative_lease_duration", - -15, - 15, - func(_ string, _ int) (*Secret, error) { + maxTestTime: 15 * time.Second, + name: "negative_lease_duration", + leaseDurationSeconds: -15, + incrementSeconds: 15, + renew: func(_ string, _ int) (*Secret, error) { return renewedSecret, nil }, - nil, - true, + expectError: nil, + expectRenewal: true, }, } From 0a2d723927590b35e001d124c4174a7eeffb1261 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov Date: Mon, 4 Apr 2022 17:09:34 -0400 Subject: [PATCH 4/4] add short_increment_duration test --- api/renewer_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/api/renewer_test.go b/api/renewer_test.go index da3d4777abfa..2a5d5745ba64 100644 --- a/api/renewer_test.go +++ b/api/renewer_test.go @@ -108,6 +108,17 @@ func TestLifetimeWatcher(t *testing.T) { expectError: nil, expectRenewal: true, }, + { + maxTestTime: time.Second, + name: "short_increment_duration", + leaseDurationSeconds: 60, + incrementSeconds: 10, + renew: func(_ string, _ int) (*Secret, error) { + return renewedSecret, nil + }, + expectError: nil, + expectRenewal: true, + }, { maxTestTime: 5 * time.Second, name: "one_error",