success/fail callbacks for lease maintenance

[bsless]

Lease maintenance doesn't expose any API for user control or notification if it fails.

Would it be possible to expose it via callbacks?

[greglook]

I'm designing this for the 2.x rewrite, that's a good callout that this should be visible to library clients.

[greglook]

@bsless a couple questions about your use-case - what information would you want to be passed to these callbacks?

The current design in 2.x supports the following options on the relevant secret-reading calls:

• An :on-renew callback will be called with the updated lease information on a successful renewal.
• An :on-rotate callback will be called with the newly-read secret data on a successful rotation.
• An :on-error callback will be called with any exceptions thrown while attempting to renew or rotate a lease.

These are set independently per-lease, so you can have different behavior for different secrets. Are these sufficient to cover your needs?

[bsless]

@greglook I think so, but if you don't mind, a couple of points related to this design:

• Should the client be passed as an argument? What if users want to take actions regarding the client in the callback?
• How about having an option to set higher level callbacks, too? I might not want to have different callbacks per lease for all cases. To facilitate this, these callbacks could be determined on the client level or lease level, then (or lease-cb client-cb noop)

What do you think?

[greglook]

Passing the client to the callbacks seems like a good idea - originally I had been thinking that the callback could close over the client, either directly or with a partial application. Having the maintenance code pass it in simplifies specifying the callbacks though, since you can just pass the function directly.

The way you phrased the second point makes it seem like you're imagining a unified callback interface - am I interpreting that right? That would then mean callbacks have a signature like (f client action lease result) where the actions would be :renew, :rotate, or :error, or perhaps :renew-success, :renew-error, :rotate-success, :rotate-error. The result would be either the secret data or an exception. 🤔 What do you think?

[bsless]

I wasn't exactly imagining a unified callbacks interfaces, but that isn't mutually exclusive. I thought of a mechanism where you could create a client with a default callback for on-error, then when creating the lease you could override the callback with one specifically for that lease.

I think having a unified callback is a bit confusing, because of the combination of cases, but maybe it's a finite pool and easy to think about.

Pros and cons of tiered callbacks: Saves boilerplate Adds confusion and complicates implementation in client User confusion (which callback was actually called)

Pros and cons of unified callbacks: Unified interface Complicates implementation for user: might push towards implementing one callback which dispatches via multimethod or case on the action (you know it'll happen) May not make sense when actually implemented (?)

Against my original suggestion, it seems that to begin with, callbacks should be just per lease and as originally suggested.

Another thing to consider is which things fail and why - was the failure at the raw HTTP level? (such as user machine isn't connected to the internet), or is it a vault failure? (authentication failed) What should be retried, and when? Which leads me down another rabbit hole of - should the HTTP client be abstracted away from the Vault API client?