Skip to content
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.

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

Fix exponential backoff for api.LifetimeWatcher #26383

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

andrewstucki
Copy link
Contributor

I just opened up a bug for this PR, this fixes it. Here's the original context from the bug:

Describe the bug
We were using the Vault LifetimeWatcher from the api package in an internal project and noticed an issue with the backoff behavior of token renewal that was causing a bunch of our tests to fail when we upgraded to a new version of Vault.

The bug is here:

var sleepDuration time.Duration
if errorBackoff == nil {
sleepDuration = r.calculateSleepDuration(remainingLeaseDuration, priorDuration)
} else if errorBackoff.NextBackOff() == backoff.Stop {
return err
}

sleepDuration appears to be the time.Duration used prior to re-running the renewal loop. In the case when errBackoff is nil, then a simple backoff duration is calculated based on the call to calculateSleepDuration. If errBackoff is not nil then sleepDuration is never set and the timeout in the following select block immediately fires again.

In our testing environment this was caught because our mock Vault server was returning an invalid response, so the renew operation was failing and we were getting an inordinate amount of immediate retries.

The fix is just refactoring the above block to capture the errBackoff.NextBackoff() value as sleepDuration.

fixes: #26382

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Apr 11, 2024
@andrewstucki andrewstucki requested a review from a team April 11, 2024 21:20
Copy link

CI Results:
All Go tests succeeded! ✅

Copy link

Build Results:
All builds succeeded! ✅

@peteski22 peteski22 added the bug Used to indicate a potential bug label Apr 12, 2024
Copy link

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Please could you add a changelog file before it's merged?

@andrewstucki
Copy link
Contributor Author

andrewstucki commented Apr 12, 2024

@peteski22 Thanks for the review. Added a changelog entry, not entirely sure of the vault project labeling conventions for changelog entries, so let me know if I should do something other than api. Also, do I need to label this with some sort of milestone label? Also, backports? Not sure if we do backport releases for the subpackage libraries or not.

Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding the changelog entry!

@andrewstucki
Copy link
Contributor Author

@ccapurso / @peteski22 : Just FYI, looks like I don't have permission to merge since I'm not part of the Vault org, so think someone from your team will have to. Thanks!

@ccapurso
Copy link
Contributor

I can merge. Thanks again for the fix!

@ccapurso ccapurso merged commit 57cb563 into main Apr 12, 2024
85 of 86 checks passed
@ccapurso ccapurso deleted the lifetime-watcher-timeout-fix branch April 12, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exponential backoff on api.LifetimeWatcher does not work
3 participants