Let lifetime watcher retry until end of lease

[andrejvanderzee]

This pull request retries renewals on error until the end of the lease (minus grace), instead of abruptly restarting the lifetime watcher (and revoking tokens/secrets as a result).

The problem is described in detail here: #10918.

@ncabatoff

[andrejvanderzee]

@ncabatoff Any change that someone can look at this? We do have a production problem and our current workaround is a rolling restart of all our k8s production deployments every week, just to make sure we do not run into this vault-agent sidecar edge-case. This MR makes vault agents more resilient against temporary problems not being able to renew leases of tokens and secrets.

[andrejvanderzee]

@ncabatoff @jefferai

Can someone please take a look at this Vault Agent resilience issue? Our workaround now is doing rolling upgrades of all our Kubernetes deployments when it nears the TTL of the vault token. Insane.

The current implementation of func (r *LifetimeWatcher) doRenew() error is just not OK, I hope you can acknowledge that?

I think the whole doRenew() requires a proper refactoring, but this PR at least makes Vault Agent more resilient with a patch on the error/problem-path only, which causes vault token and secrets to be unnecessarily renewed to soon!

[andrejvanderzee]

@ncabatoff @jefferai

Ping...

[andrejvanderzee]

@ncabatoff @jefferai

Any chance that this will be picked up?

[ncabatoff]

Hey @andrejvanderzee,

I'm sorry to have left you hanging for so long. I've added a story to the current sprint to tackle this issue. I think this PR is on the right track, though I would like to see some tests. Maybe factor out the first bit of doRenew so that we can provide our own artificial renewFunc when testing. Would you like to work on those, or shall I?

[andrejvanderzee]

Hi @ncabatoff, no problem at all, we are just very happy that it is being picked up now, as it occasionally is causing problems in our prd environments. I won't have much time, so if you could add the tests that would be wonderful :-)

As a side note: To mitigate the problem, we put VAULT_MAX_TRIES on 12. We have noticed that every common error will end up in a "context deadline exceeded", because retrying is handled inside the Vault client and after 12 retries you exceed VAULT_CLIENT_TIMEOUT. It would be nice if somehow we could see the real error.

Thanks for putting this in your sprint :-)

[ncabatoff]

Closing this in favour of #11445.