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

Support pluggable ExecutorServices for background processes #97

Closed
greglook opened this issue Nov 28, 2022 · 4 comments
Closed

Support pluggable ExecutorServices for background processes #97

greglook opened this issue Nov 28, 2022 · 4 comments
Assignees
Milestone

Comments

@greglook
Copy link
Collaborator

The background lease and token renewal logic runs on a dedicated background "timer" thread, which periodically checks for any necessary maintenance, performs it, and then sleeps for a fixed period. Any callbacks (in the 2.x branch) are invoked directly by the same singular thread. This is a simple approach, but doesn't behave well in the face of blocking - a hung callback could indefinitely prevent the timer thread from performing maintenance.

To address this, and to improve the client's ability to fit into other execution models, the client could opt to use a provided ScheduledExecutorService for the maintenance logic, and optionally another ExecutorService to run the callback actions.

@greglook greglook added this to the 2.x milestone Nov 28, 2022
@greglook greglook changed the title Background processes should support pluggable ExecutorServices Support pluggable ExecutorServices for background processes Nov 28, 2022
@greglook greglook self-assigned this Dec 8, 2022
@greglook
Copy link
Collaborator Author

greglook commented Dec 8, 2022

Added 3e8fb11 for this, but needs a bit of testing.

@jasonjckn
Copy link

jasonjckn commented May 17, 2023

This might be helpful https://github.com/jarohen/chime which uses ScheduledExecutorService underneath.
I've also written a mock 'simulated time' executor for it , sample code @ jarohen/chime#57

@jasonjckn
Copy link

jasonjckn commented May 17, 2023

I don't know the domain you're working in, but i'm finding a lazy approach a cleaner way to go about it, instead of countless threads + watches to update downstream dependent connections on rotating passwords.

e.g. https://developer.hashicorp.com/vault/docs/secrets/ad#a-note-on-lazy-rotation

passwords are rotated lazily, so why not renew leases lazily, and renew tokens lazily, etc.

@greglook
Copy link
Collaborator Author

greglook commented Sep 8, 2023

Lazy rotation works if your there is some chain of calls where your clients are requesting credentials each time they perform an operation and you have an opportunity to use cached values or reach out for new ones on-demand. The AWS SDK is a good example of this; you can implement an AWSCredentialsProvider that wraps a Vault client to do so. The lazy approach does not work in cases where the code that needs the credentials expects to have them "installed" and uses those statically - one example we have is using a database connection pool, where we need the external rotation to proactively generate new creds and update them in the pool configuration before they expire.

Another issue with lazy rotation is that it can lead to quite significant latency outliers. If your code paths are blocked on issuing new credentials occasionally, that's going to show up in your P99 times. This is even ignoring the potential issue of contention, where many threads in the same process concurrently need to request new credentials - you need a locking mechanism of some kind to make sure that only one request goes through.

If you do want the lazy behavior, that's still supported in vault-clj; you can just omit the :rotate? true parameter. If there's a cached credential lease available, it'll be returned immediately, otherwise the client will go ask for a new one.

@greglook greglook closed this as completed Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants