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

Make default grace period match Vault's default, this makes it so #1021

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

preetapan
Copy link

users are not surprised by continuous renewals on dynamic secrets with TTLs lower than 5 minutes (the previous default).

…rs are not surprised by continous renewals on TTLs lower than 5 minutes (the previous default).
@preetapan preetapan requested a review from sethvargo October 3, 2017 20:56
config/vault.go Outdated
@@ -11,7 +11,7 @@ const (
// DefaultVaultGrace is the default grace period before which to read a new
// secret from Vault. If a lease is due to expire in 5 minutes, Consul
// Template will read a new secret at that time minus this value.
DefaultVaultGrace = 5 * time.Minute
DefaultVaultGrace = 15 * time.Second
Copy link
Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, because I don't have context for where 5 minutes originally came from.

IMO changing this to match the default in vault allows for better user experience for CT out of the box, but this can very well be managed in the caller (e.g nomad) by setting a smaller grace period. I will say that without knowing implementation details and digging through the code, it was not obvious that the default grace period affected renewal of dynamic token leases in such a drastic way.

@preetapan preetapan requested a review from dadgar October 3, 2017 21:02
Copy link
Contributor

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

For some context, @preetapan and I discussed this via Zoom. We both agree that having CT match the default grace periods for the renewer itself makes sense in the short term. We also have plans for a better long-term fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants