Skip to content

sending err back to fetch to trigger backoff retry #2136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aswanidutt merged 15 commits into from
Mar 4, 2026
Merged

sending err back to fetch to trigger backoff retry #2136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
354e413
sending err back to fetch to trigger retry
aswanidutt Feb 19, 2026
2d77658
adding maxbackoff 5m and maxretries unlimited
aswanidutt Feb 19, 2026
d6bb7af
missed view
aswanidutt Feb 19, 2026
e1ed626
using cenkalti v4 for retry backoff with jitter
aswanidutt Feb 19, 2026
ccec01d
fixing linter
aswanidutt Feb 19, 2026
cc275ee
using the default config_retry instead of custom retry
aswanidutt Feb 25, 2026
04b3bfd
cleaning up
aswanidutt Feb 25, 2026
56e76a8
Update dependency/vault_common_test.go
aswanidutt Feb 26, 2026
0474863
Update dependency/vault_common_test.go
aswanidutt Feb 26, 2026
0f8616c
pr feedback
aswanidutt Feb 26, 2026
b6a7990
pr feedback comments
aswanidutt Feb 26, 2026
373766d
pr comments update
aswanidutt Feb 26, 2026
211504d
trying to minimize the test case changes wrt formatting
aswanidutt Feb 27, 2026
08a2416
format cleanup
aswanidutt Feb 27, 2026
e9f7ae7
adding local variable for seconds convert
aswanidutt Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions dependency/vault_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ func renewSecret(clients *ClientSet, d renewer) error {
}

// leaseCheckWait accepts a secret and returns the recommended amount of
// time to sleep.
func leaseCheckWait(s *Secret) time.Duration {
// time to sleep. Returns an error if TTL=0 to trigger retry mechanism.
func leaseCheckWait(s *Secret) (time.Duration, error) {
// Handle whether this is an auth or a regular secret.
base := s.LeaseDuration
if s.Auth != nil && s.Auth.LeaseDuration > 0 {
Expand Down Expand Up @@ -143,11 +143,15 @@ func leaseCheckWait(s *Secret) time.Duration {
var rotatingSecret bool
if _, ok := s.Data["rotation_period"]; ok && s.LeaseID == "" {
if ttlInterface, ok := s.Data["ttl"]; ok {
if ttlData, err := ttlInterface.(json.Number).Int64(); err == nil {
if ttlData, err := ttlInterface.(json.Number).Int64(); err == nil && ttlData > 0 {
log.Printf("[DEBUG] Found rotation_period and set lease duration to %d seconds", ttlData)
// Add a second for cushion
base = int(ttlData) + 1
rotatingSecret = true
} else if err == nil && ttlData == 0 {
// TTL is 0, return error to trigger retry mechanism
log.Printf("[DEBUG] Found rotation_period with ttl=0, returning error to trigger retry")
return 0, fmt.Errorf("vault rotating secret returned ttl=0, will retry")
}
}
}
Expand Down Expand Up @@ -186,7 +190,7 @@ func leaseCheckWait(s *Secret) time.Duration {
sleep = sleep * finalFraction
}

return time.Duration(sleep)
return time.Duration(sleep), nil
}

// printVaultWarnings prints warnings for a given dependency.
Expand Down
73 changes: 47 additions & 26 deletions dependency/vault_common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ func init() {

func TestVaultRenewDuration(t *testing.T) {
renewable := Secret{LeaseDuration: 100, Renewable: true}
renewableDur := leaseCheckWait(&renewable).Seconds()
Copy link
Copy Markdown
Member

@tvoran tvoran Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the .Seconds() was dropped here? That seems to have forced a lot of changes unrelated to the goal of this PR that I'm not sure are necessary?

At this point I'd say either set this back to .Seconds(), or apply the suggestions I made to the rest of TestVaultRenewDuration():

Copy link
Copy Markdown
Contributor Author

@aswanidutt aswanidutt Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Seconds() dropped because now the leaseCheckWait is returning multiple values not just duration but also error. updated with your suggestions to the rest of TestVaultRenewDuration():

if renewableDur < 16 || renewableDur >= 34 {
t.Fatalf("renewable duration is not within 1/6 to 1/3 of lease duration: %f", renewableDur)
renewableDur, _ := leaseCheckWait(&renewable)
Copy link
Copy Markdown
Member

@tvoran tvoran Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably still a good idea to check the value of err whenever leaseCheckWait() is called, just to be safe.

renewableDurSec := renewableDur.Seconds()
if renewableDurSec < 16 || renewableDurSec >= 34 {
t.Fatalf("renewable duration is not within 1/6 to 1/3 of lease duration: %f", renewableDurSec)
}

nonRenewable := Secret{LeaseDuration: 100}
nonRenewableDur := leaseCheckWait(&nonRenewable).Seconds()
if nonRenewableDur < 80 || nonRenewableDur > 95 {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableDur)
nonRenewableDur, _ := leaseCheckWait(&nonRenewable)
nonRenewableDurSec := nonRenewableDur.Seconds()

if nonRenewableDurSec < 80 || nonRenewableDurSec > 95 {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableDurSec)
}

data := map[string]interface{}{
Expand All @@ -37,11 +40,11 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableRotated := Secret{LeaseDuration: 100, Data: data}
nonRenewableRotatedDur := leaseCheckWait(&nonRenewableRotated).Seconds()

nonRenewableRotatedDur, _ := leaseCheckWait(&nonRenewableRotated)
nonRenewableRotatedDurSec := nonRenewableRotatedDur.Seconds()
// We expect a 1 second cushion
if nonRenewableRotatedDur != 31 {
t.Fatalf("renewable duration is not 31: %f", nonRenewableRotatedDur)
if nonRenewableRotatedDurSec != 31 {
t.Fatalf("renewable duration is not 31: %f", nonRenewableRotatedDurSec)
}

data = map[string]interface{}{
Expand All @@ -50,11 +53,24 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableRotated = Secret{LeaseDuration: 100, Data: data}
nonRenewableRotatedDur = leaseCheckWait(&nonRenewableRotated).Seconds()

nonRenewableRotatedDur, _ = leaseCheckWait(&nonRenewableRotated)
nonRenewableRotatedDurSeconds := nonRenewableRotatedDur.Seconds()
// We expect a 1 second cushion
if nonRenewableRotatedDur != 6 {
t.Fatalf("renewable duration is not 6: %f", nonRenewableRotatedDur)
if nonRenewableRotatedDurSeconds != 6 {
t.Fatalf("renewable duration is not 6: %f", nonRenewableRotatedDurSeconds)
}
// Test TTL=0 case - should return error
data = map[string]interface{}{
"rotation_period": json.Number("30"),
"ttl": json.Number("0"),
}
nonRenewableRotatedZero := Secret{LeaseDuration: 100, Data: data}
_, err := leaseCheckWait(&nonRenewableRotatedZero)
if err == nil {
t.Fatalf("expected error for ttl=0, got nil")
}
if err.Error() != "vault rotating secret returned ttl=0, will retry" {
t.Fatalf("expected ttl=0 error message, got: %v", err)
}

rawExpiration := time.Now().Unix() + 100
Expand All @@ -66,9 +82,11 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableCert := Secret{LeaseDuration: 100, Data: data}
nonRenewableCertDur := leaseCheckWait(&nonRenewableCert).Seconds()
if nonRenewableCertDur < 80 || nonRenewableCertDur > 95 {
t.Fatalf("non renewable certificate duration is not within 80%% to 95%%: %f", nonRenewableCertDur)
nonRenewableCertDur, _ := leaseCheckWait(&nonRenewableCert)
nonRenewableCertDurSec := nonRenewableCertDur.Seconds()

if nonRenewableCertDurSec < 80 || nonRenewableCertDurSec > 95 {
t.Fatalf("non renewable certificate duration is not within 80%% to 95%%: %f", nonRenewableCertDurSec)
}

t.Run("secret ID handling", func(t *testing.T) {
Expand All @@ -80,10 +98,11 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableSecretID := Secret{LeaseDuration: 100, Data: data}
nonRenewableSecretIDDur := leaseCheckWait(&nonRenewableSecretID).Seconds()
nonRenewableSecretIDDur, _ := leaseCheckWait(&nonRenewableSecretID)
nonRenewableSecretIDDurSec := nonRenewableSecretIDDur.Seconds()

if nonRenewableSecretIDDur < 0.80*(60+1) || nonRenewableSecretIDDur > 0.95*(60+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDur)
if nonRenewableSecretIDDurSec < 0.80*(60+1) || nonRenewableSecretIDDurSec > 0.95*(60+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDurSec)
}
})

Expand All @@ -96,10 +115,11 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableSecretID := Secret{LeaseDuration: leaseDuration, Data: data}
nonRenewableSecretIDDur := leaseCheckWait(&nonRenewableSecretID).Seconds()
nonRenewableSecretIDDur, _ := leaseCheckWait(&nonRenewableSecretID)
nonRenewableSecretIDDurSec := nonRenewableSecretIDDur.Seconds()

if nonRenewableSecretIDDur < 0.80*(leaseDuration+1) || nonRenewableSecretIDDur > 0.95*(leaseDuration+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDur)
if nonRenewableSecretIDDurSec < 0.80*(leaseDuration+1) || nonRenewableSecretIDDurSec > 0.95*(leaseDuration+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDurSec)
}
})

Expand All @@ -111,10 +131,11 @@ func TestVaultRenewDuration(t *testing.T) {
}

nonRenewableSecretID := Secret{LeaseDuration: leaseDuration, Data: data}
nonRenewableSecretIDDur := leaseCheckWait(&nonRenewableSecretID).Seconds()
nonRenewableSecretIDDur, _ := leaseCheckWait(&nonRenewableSecretID)
nonRenewableSecretIDDurSec := nonRenewableSecretIDDur.Seconds()

if nonRenewableSecretIDDur < 0.80*(leaseDuration+1) || nonRenewableSecretIDDur > 0.95*(leaseDuration+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDur)
if nonRenewableSecretIDDurSec < 0.80*(leaseDuration+1) || nonRenewableSecretIDDurSec > 0.95*(leaseDuration+1) {
t.Fatalf("renewable duration is not within 80%% to 95%% of lease duration: %f", nonRenewableSecretIDDurSec)
}
})
})
Expand Down
6 changes: 5 additions & 1 deletion dependency/vault_read.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ func (d *VaultReadQuery) Fetch(clients *ClientSet, opts *QueryOptions,
}

if !vaultSecretRenewable(d.secret) {
dur := leaseCheckWait(d.secret)
dur, err := leaseCheckWait(d.secret)
if err != nil {
// TTL=0 case - return error to trigger retry mechanism
return nil, nil, err
}
log.Printf("[TRACE] %s: non-renewable secret, set sleep for %s", d, dur)
d.sleepCh <- dur
}
Expand Down
6 changes: 5 additions & 1 deletion dependency/vault_write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ func (d *VaultWriteQuery) Fetch(clients *ClientSet, opts *QueryOptions,
d.secret = transformSecret(vaultSecret)

if !vaultSecretRenewable(d.secret) {
dur := leaseCheckWait(d.secret)
dur, err := leaseCheckWait(d.secret)
if err != nil {
// TTL=0 case - return error to trigger retry mechanism
return nil, nil, err
}
log.Printf("[TRACE] %s: non-renewable secret, set sleep for %s", d, dur)
d.sleepCh <- dur
}
Expand Down
Loading