From 9ca558c303b9a303da34aa10e477427a8b07321c Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 19 Mar 2018 15:48:24 -0400 Subject: [PATCH] Update lease renewer logic (#4090) * Add grace period calculation logic to renewer * Update lease renewer logic. It is believed by myself and members of the Nomad team that this logic should be much more robust in terms of causing large numbers of new secret acquisitions caused by a static grace period. See comments in the code for details. Fixes #3414 * Fix some commenting and fix tests * Add more time to test so that integ tests don't time out * Fix some review feedback --- api/renewer.go | 78 ++++++++++++++++++++++++++------- api/renewer_integration_test.go | 53 +++++++++++++--------- api/renewer_test.go | 1 - 3 files changed, 95 insertions(+), 37 deletions(-) diff --git a/api/renewer.go b/api/renewer.go index b50cf814f376..7fd1de7db20d 100644 --- a/api/renewer.go +++ b/api/renewer.go @@ -13,9 +13,6 @@ var ( ErrRenewerNotRenewable = errors.New("secret is not renewable") ErrRenewerNoSecretData = errors.New("returned empty secret data") - // DefaultRenewerGrace is the default grace period - DefaultRenewerGrace = 15 * time.Second - // DefaultRenewerRenewBuffer is the default size of the buffer for renew // messages on the channel. DefaultRenewerRenewBuffer = 5 @@ -111,9 +108,6 @@ func (c *Client) NewRenewer(i *RenewerInput) (*Renewer, error) { } grace := i.Grace - if grace == 0 { - grace = DefaultRenewerGrace - } random := i.Rand if random == nil { @@ -184,6 +178,9 @@ func (r *Renewer) renewAuth() error { return ErrRenewerNotRenewable } + priorDuration := time.Duration(r.secret.Auth.LeaseDuration) * time.Second + r.calculateGrace(priorDuration) + client, token := r.client, r.secret.Auth.ClientToken for { @@ -216,13 +213,28 @@ func (r *Renewer) renewAuth() error { return ErrRenewerNotRenewable } - // Grab the lease duration and sleep duration - note that we grab the auth - // lease duration, not the secret lease duration. + // Grab the lease duration leaseDuration := time.Duration(renewal.Auth.LeaseDuration) * time.Second - sleepDuration := r.sleepDuration(leaseDuration) - // If we are within grace, return now. - if leaseDuration <= r.grace || sleepDuration <= r.grace { + // We keep evaluating a new grace period so long as the lease is + // extending. Once it stops extending, we've hit the max and need to + // rely on the grace duration. + if leaseDuration > priorDuration { + r.calculateGrace(leaseDuration) + } + priorDuration = leaseDuration + + // The sleep duration is set to 2/3 of the current lease duration plus + // 1/3 of the current grace period, which adds jitter. + sleepDuration := time.Duration(float64(leaseDuration.Nanoseconds())*2/3 + float64(r.grace.Nanoseconds())/3) + + // If we are within grace, return now; or, if the amount of time we + // would sleep would land us in the grace period. This helps with short + // tokens; for example, you don't want a current lease duration of 4 + // seconds, a grace period of 3 seconds, and end up sleeping for more + // than three of those seconds and having a very small budget of time + // to renew. + if leaseDuration <= r.grace || leaseDuration-sleepDuration <= r.grace { return nil } @@ -241,6 +253,9 @@ func (r *Renewer) renewLease() error { return ErrRenewerNotRenewable } + priorDuration := time.Duration(r.secret.LeaseDuration) * time.Second + r.calculateGrace(priorDuration) + client, leaseID := r.client, r.secret.LeaseID for { @@ -273,12 +288,28 @@ func (r *Renewer) renewLease() error { return ErrRenewerNotRenewable } - // Grab the lease duration and sleep duration + // Grab the lease duration leaseDuration := time.Duration(renewal.LeaseDuration) * time.Second - sleepDuration := r.sleepDuration(leaseDuration) - // If we are within grace, return now. - if leaseDuration <= r.grace || sleepDuration <= r.grace { + // We keep evaluating a new grace period so long as the lease is + // extending. Once it stops extending, we've hit the max and need to + // rely on the grace duration. + if leaseDuration > priorDuration { + r.calculateGrace(leaseDuration) + } + priorDuration = leaseDuration + + // The sleep duration is set to 2/3 of the current lease duration plus + // 1/3 of the current grace period, which adds jitter. + sleepDuration := time.Duration(float64(leaseDuration.Nanoseconds())*2/3 + float64(r.grace.Nanoseconds())/3) + + // If we are within grace, return now; or, if the amount of time we + // would sleep would land us in the grace period. This helps with short + // tokens; for example, you don't want a current lease duration of 4 + // seconds, a grace period of 3 seconds, and end up sleeping for more + // than three of those seconds and having a very small budget of time + // to renew. + if leaseDuration <= r.grace || leaseDuration-sleepDuration <= r.grace { return nil } @@ -307,3 +338,20 @@ func (r *Renewer) sleepDuration(base time.Duration) time.Duration { return time.Duration(sleep) } + +// 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 *Renewer) calculateGrace(leaseDuration time.Duration) { + if leaseDuration == 0 { + r.grace = 0 + return + } + + leaseNanos := float64(leaseDuration.Nanoseconds()) + jitterMax := 0.1 * leaseNanos + + // For a given lease duration, we want to allow 80-90% of that to elapse, + // so the remaining amount is the grace period + r.grace = time.Duration(jitterMax) + time.Duration(uint64(r.random.Int63())%uint64(jitterMax)) +} diff --git a/api/renewer_integration_test.go b/api/renewer_integration_test.go index 50a775e1261c..e3b83e93fbee 100644 --- a/api/renewer_integration_test.go +++ b/api/renewer_integration_test.go @@ -109,8 +109,8 @@ func TestRenewer_Renew(t *testing.T) { "creation_statements": `` + `CREATE ROLE "{{name}}" WITH LOGIN PASSWORD '{{password}}' VALID UNTIL '{{expiration}}';` + `GRANT SELECT ON ALL TABLES IN SCHEMA public TO "{{name}}";`, - "default_ttl": "1s", - "max_ttl": "3s", + "default_ttl": "5s", + "max_ttl": "10s", }); err != nil { t.Fatal(err) } @@ -139,22 +139,28 @@ func TestRenewer_Renew(t *testing.T) { if !renew.Secret.Renewable { t.Errorf("expected lease to be renewable: %#v", renew) } - if renew.Secret.LeaseDuration > 2 { - t.Errorf("expected lease to < 2s: %#v", renew) + if renew.Secret.LeaseDuration > 5 { + t.Errorf("expected lease to <= 5s: %#v", renew) } - case <-time.After(3 * time.Second): + case <-time.After(5 * time.Second): t.Errorf("no renewal") } - select { - case err := <-v.DoneCh(): - if err != nil { - t.Fatal(err) + outer: + for { + select { + case err := <-v.DoneCh(): + if err != nil { + t.Fatal(err) + } + break outer + case renew := <-v.RenewCh(): + t.Logf("renew called, remaining lease duration: %d", renew.Secret.LeaseDuration) + continue outer + case <-time.After(5 * time.Second): + t.Errorf("no data") + break outer } - case renew := <-v.RenewCh(): - t.Fatalf("should not have renewed (lease should be up): %#v", renew) - case <-time.After(3 * time.Second): - t.Errorf("no data") } }) @@ -205,15 +211,20 @@ func TestRenewer_Renew(t *testing.T) { t.Errorf("no renewal") } - select { - case err := <-v.DoneCh(): - if err != nil { - t.Fatal(err) + outer: + for { + select { + case err := <-v.DoneCh(): + if err != nil { + t.Fatal(err) + } + break outer + case <-v.RenewCh(): + continue outer + case <-time.After(3 * time.Second): + t.Errorf("no data") + break outer } - case renew := <-v.RenewCh(): - t.Fatalf("should not have renewed (lease should be up): %#v", renew) - case <-time.After(3 * time.Second): - t.Errorf("no data") } }) }) diff --git a/api/renewer_test.go b/api/renewer_test.go index 262484e0fa01..d0b828b451ed 100644 --- a/api/renewer_test.go +++ b/api/renewer_test.go @@ -41,7 +41,6 @@ func TestRenewer_NewRenewer(t *testing.T) { }, &Renewer{ secret: &Secret{}, - grace: DefaultRenewerGrace, }, false, },