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 vault retry logic on failed calls #1269

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Conversation

eikenb
Copy link
Contributor

@eikenb eikenb commented Aug 27, 2019

The original problem was that for non-renewable vault secrets that it
was having trouble fetching, it would wait the standard exponential
backoff time plus the configured sleep time (like it does between
successful fetches). When what it should do is use the sleep time
between successful fetches and exponential backoff on failures.

While fixing this I cleaned up the code to make the logic clear.
The issue existed in both vault_read and vault_write, and they shared a
common chunk of renew logic between them and with vault_token. So I
refactored that out into a common function.

Fixes #1224

@eikenb eikenb added bug vault Related to the Vault integration labels Aug 27, 2019
@eikenb eikenb added this to the 0.21.1 milestone Aug 27, 2019
@eikenb eikenb requested review from a team August 27, 2019 22:06
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Overall this looks great. Just the few very minor points.

dependency/vault_read.go Outdated Show resolved Hide resolved
dependency/vault_read.go Show resolved Hide resolved
dependency/vault_write.go Outdated Show resolved Hide resolved
dependency/vault_write.go Outdated Show resolved Hide resolved
The original problem was that for non-renewable vault secrets that it
was having trouble fetching, it would wait the standard exponential
backoff time plus the configured sleep time (like it does between
successful fetches). When what it should do is use the sleep time
between successful fetches and exponential backoff on failures.

While fixing this I cleaned up the code to make the logic clear.
The issue existed in both vault_read and vault_write, and they shared a
common chunk of renew logic between them and with vault_token. So I
refactored that out into a common function.

Fixes #1224
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM

@eikenb eikenb merged commit a96de95 into master Aug 30, 2019
@eikenb eikenb deleted the issue-1224-fix-vault-retry branch August 30, 2019 20:07
@vapopov
Copy link

vapopov commented Sep 2, 2019

@mkeeler @eikenb
Hello, this part of the code fail blocking operation of lease duration, when i use it with vault pki endpoint it just recursively creates tons of certificates

select {
case <-d.sleepCh:
default:
}

How this part of code should work at all? We just ignore time channel and always pass through default:

@vapopov
Copy link

vapopov commented Sep 2, 2019

In previous implementation version 0.20.0, it was working as expected.
it terms of vault pki, certificates can't be renewable, but we have to block Fetch operation until lease duration is not reached, otherwise we get loop of certificate generation

select {
case <-time.After(dur):
// The lease is almost expired, it's time to request a new one.
case <-d.stopCh:
return nil, nil, ErrStopped
}

@eikenb
Copy link
Contributor Author

eikenb commented Sep 5, 2019

Hey @vapopvo, thanks for the comments. I missed these at the time but others reported the issue. I just pushed the fix for this in 0.21.3. In the future if you see these things please open a new issue and then reference things like this. I don't check my github notifications every day and can easily miss comments on already merged PRs (or closed issues).

Thanks.

@vapopov
Copy link

vapopov commented Sep 6, 2019

@eikenb good, thanks a lot will do, the template of issues just prevented me to spend time a lot of writing it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug vault Related to the Vault integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consul template retries
4 participants