Faraday retry middleware

[timothysmith0609]

cc @cben (it's been a while since I've played around in this repo. Hope you're doing well!)

We find ourselves rewriting retry logic around kubeclient handlers to deal with intermittent API-server unavailability and figured it would be convenient to declare retry logic inside kubeclient itself. There's definitely some discussion to be had on the actual implementation but I wanted to post my work so far to get the ball rolling.

How it works

Expose a retry_options option to Kubeclient::Client.new that is passed into the Faraday client initializer. This is convenient because:

• It is backwards compatible
• It allows consumers to specify their own retry logic

Discussion

I see the Faraday client is lazily instantiated at the moment, though I've proactively initialized it in order to pass in the retry options inside the client initializer. I'm tempted to always initialize it, and refactor things so that we don't need a separate Faraday client for API vs. explicit-REST requests. Alternatively, we can simply leave retry options as an instance var itself, and pass them into Faraday when necessary. An even more engineered solution might be to extract Faraday client building into its own object 🤭

Interested to hear your thoughts

[cben]

cc @astencel-sumo who knows more about faraday than me at this point. and @grosser.

I'm fine in principle with adding retry_options. I suspect over time more such needs would lead to more options. Which is OK except it creates a kubeclient-specific "dialect" of specifying pretty standard http options. We already have quite a few of that for TLS, timeouts etc...
So I wonder if we could/should expose faraday configuration more directly.

• One of the goals of Replace rest_client with faraday #466 was to allow customizing the faraday adapter e.g.:

  client = Kubeclient::Client.new(...)
  client.faraday_client.adapter(:net_http_persistent)
  

  Would that still work with eager instantiation of faraday client?

If we already decided to expose .faraday_client (we should document it in README!), I think we should at least expose a way to add arbitrary faraday middleware...

An even more engineered solution might be to extract Faraday client building into its own object 🤭

This sounds interesting!

A possibly overlapping goal: for auth renewal #393, I believe we'll need a new object that can be used by multiple Client objects to provide some of the connection options dynamically (auth_options: but also ssl_options: {client_cert:, client_key:}).

Feels like kubeclient might be decomposed into 2 halves:

• one dealing with config and auth, giving you an HTTP (well Faraday) client.
• one taking an HTTP client and using it to help make k8s API requests.

(plus compat code for current Client uses to [mostly] work)

However, the division of work is a bit trickier than Client.new(faraday_client: ...) injection because of how auth expiration works.
In some cases, credentials have a known validity, but in some you just have to try, get an error, then obtain new credentials & re-try.
Until now, I assumed this re-auth-and-retry handling should live inside Client, which will communicate with Auth object before a request and/or after an expiration error a to get fresh credentials. But now that we have Faraday, I wonder if this can wrapped neatly as a middleware??

[Sorry for getting off-topic. Trying to get some idea which direction we should navigate towards... This faraday stuff is new to me 😁]

[andrzej-stencel]

This is very useful, thanks @timothysmith0609 ! Nice piece of middleware, too. I like how it says it will honor the Retry-After header if you ask it to retry on 429 Too Many Requests (according to the docs).

As for lazy initializing of the Faraday client, I think I didn't put much thought into it. I believe this is how it's been before my changes (in the RestClient days) and I just left it unchanged. Not attached to it though, I'm not quite sure it adds any value at all? Hm maybe when one only uses Kubeclient for watches, which currently use their own instance of HTTP client - then the initialization would be unneeded. But that's probably a stretch and not much harm anyway, so I don't think it's a problem to eagerly initialize the client.

[timothysmith0609]

It might just be easier to slip the block in here and always use lines 323, 324; that might actually be the behaviour people would expect, now that I think about it....

[timothysmith0609]

Ok, I've changed this to always include :raise_error and FollowRedirects. I think those assumptions are baked into the client enough to justify their forced inclusion (I'll be sure to document that in the README, mind you)

[timothysmith0609]

I've changed my approach following your suggestion to expose this method which takes a block that yields the Faraday::Connection object, which is really the only place we can specify all the middlewares and other config. Let me know if you think this is on the right track. (Once we settle on an approach I'll do the relevant README updates as well)

[cben]

nice 👍

[grosser]

it looks like this could be called multiple times, but it can't ... so maybe call it replace_faraday_config
... and would seem more intuitive to specify this during initalization anyway like configure_faraday: -> {|c| stuff }

[timothysmith0609]

A possibly overlapping goal: for auth renewal #393, I believe we'll need a new object that can be used by multiple Client objects to provide some of the connection options dynamically (auth_options: but also ssl_options: {client_cert:, client_key:}).

I've started looking into this more closely. I think it's somewhat orthogonal to what I'm trying to do here (we can probably opportunistically refresh tokens on Faraday::Unauthorized errors), but it is something I'd like to solve. It looks like we are being too opaque with the auth options we pass to the client, since it looks like we don't maintain a reference to the kubeconfig that actually generates the client: I think we should at least persist the user entry.

I do think you're right that the way forward is to clean up our abstractions a little bit. The manner in which information flows from config -> client seems too opaque and inflexible, and it would be beneficial to get a global sense of all the clients in use so we can centralize e.g. their token refresh.

[grosser]

FYI for some basic retry logic: #493

[timothysmith0609]

FYI for some basic retry logic: #493

Thanks! Some hesitation I had around writing a non-configurable retry logic were the following:

• Different clients may have different appetites for retrying certain operations (e.g. by HTTP verb)
• A generic configuration allows injection of custom middlewares, which can support wider use-cases

[grosser]

True, but I'd also like there to be a simple `retry a bit` option that does not involve learning faraday middlewares :)

…

On Fri, Mar 5, 2021 at 1:05 PM Timothy Smith ***@***.***> wrote: FYI for some basic retry logic: #493 <#493> Thanks! Some hesitation I had around writing a non-configurable retry logic were the following: - Different clients may have different appetites for retrying certain operations (e.g. by HTTP verb) - A generic configuration allows injection of custom middlewares, which can support wider use-cases — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#490 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACYZ3YMK42U6G5D2KTVO3TCFBQ7ANCNFSM4YG3CGUQ> .

[cben]

I see 2 arguments to prefer a generic configure_faraday over #493:

• This covers more use cases.

• This invents less kubeclient-specific APIs. If you think of an app that uses only kubeclient, than having a direct retry option feels simpler, but if you use several libraries making HTTP requests plus make some yourself looking up how each one of them expresses things like TLS config, timeouts & retries etc. becomes a burden...

  This argument only holds up if you're lucky and at least two use cases share the same underlying HTTP library 😁 It's unfortunate that Ruby has so many with no single winner...
  But Faraday is the one library that allows thinks like retries to be expressed as middlewares separately from the code formulating requests 👏
  (Compare to allow client to retry to avoid common api errors #493 implementation where separate with_retries helper plays a pretty middleware-ish role, but not in a way that users could tap into.)

Could the "learning faraday middlewares" concern be covered by adding an example to the README for retries?

I'm OK with merging either of them if you reach consensus which you prefer.

[timothysmith0609]

This invents less kubeclient-specific APIs

I like this point a lot. It would be great if we could untangle the common.rb and Client object a bit and tidy up the abstractions. That might lead to more pluggable components; at the very least it would enable tightly scoped changes without having to keep the world in our head 😆 .

Could the "learning faraday middlewares" concern be covered by adding an example to the README for retries?

For 99% of people, they will probably be happy to completely ignore this feature, and a quick blurb in the README for the rest of us should be sufficient 🎉

[cben]

@timothysmith0609 please add documentation in README and I'll merge this.

[timothysmith0609]

README is updated 🎉

[timothysmith0609]

Thanks @cben and @grosser!

Relatedly, are you planning on releasing a minor version bump anytime soon?