java icon indicating copy to clipboard operation
java copied to clipboard

fix: AuthenticationRefresher wrapper solution

Open raykrueger opened this issue 1 year ago • 13 comments
trafficstars

This fixes/relates to #2438 and #290

This commit introduces the AuthenticationRefresher as an implementation of Authentication. It's purpose is to warp another Authentication instance and refresh it very n seconds by calling provide on the wrapped Authentication object.

This code is intedned to be used a solution to issue #2438.

Any Authentication object that provides credentials, that must be refreshed, to the ApiClient do not have a way to do so.

There are two ways to use the AuthenticationRefresher with ClientBuilder. The first option is to use the ClientBuilder.setAuthentication(...) method. An Authentication instance can be wrapped in an AuthenticationRefresher and passed into the setAuthentication method. For example,

ClientBuilder.standard().setAuthentication(
  //wrap an auth and refresh it every 15 minutes (900s)
  new Authentication(someDelegateAuthenticationInstance, 900)
);

This is integrated up into the ClientBuilder.build() method by adding a new authenticationRefreshSeconds field and getter/setter pair. If a refresh interval is set, the Authentication object used by ClientBuilder will be wrapped with an AuthenticationRefresher.

raykrueger avatar May 16 '24 21:05 raykrueger

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: raykrueger Once this PR has been reviewed and has the lgtm label, please assign yue9944882 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar May 16 '24 21:05 k8s-ci-robot

Welcome @raykrueger!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot avatar May 16 '24 21:05 k8s-ci-robot

@raykrueger is there a way/need to be able to cancel the task running in the executor?

dims avatar May 17 '24 19:05 dims

@raykrueger is there a way/need to be able to cancel the task running in the executor?

Yep! I'm combining Brendan's concern with some WeakReference foo to shut the timerdown when the APIClient gets GC'd. Commit incoming!

raykrueger avatar May 17 '24 20:05 raykrueger

I started the code review thing on accident and don't know what to do about that heh.

raykrueger avatar May 17 '24 21:05 raykrueger

(Sorry for spamming, wanted a new line in the previous message). Please have a look at the latest. I've added some safety around time timer and moved it to a field. The timer is now cancelled and nulled out if the APICilent is garbage collected. Additionally I removed the flaky Thread.sleep timing from the tests in favor of Countdown Latches.

raykrueger avatar May 17 '24 21:05 raykrueger

Few more comments. Thanks.

brendandburns avatar May 18 '24 17:05 brendandburns

Ok, I've simplified things a bit. Let me know if this makes better sense. Thanks!

raykrueger avatar May 19 '24 17:05 raykrueger

I pushed some code a few days ago, but didn’t reach out here. Back to you sir!

raykrueger avatar May 26 '24 22:05 raykrueger

/ok-to-test

yue9944882 avatar May 27 '24 04:05 yue9944882

Sorry, I feel like I'm dragging this code review on and on, but I think this code could be simpler. Added a comment.

brendandburns avatar May 27 '24 23:05 brendandburns

any update on this - or is this comment still waiting to be resolved?

bryantbiggs avatar Jul 02 '24 10:07 bryantbiggs

@bryantbiggs still waiting afaik.

brendandburns avatar Jul 02 '24 17:07 brendandburns

btw, I dug into this deeper, this won't work correctly because when we construct the ApiClient the token is set once and it is never revisited.

Making token refresh work will require changes in the upstream code generator, or a patch to ApiClient.

Closing this for now since I think we will need to start with a fresh solution.

brendandburns avatar Aug 16 '24 16:08 brendandburns

See: https://github.com/OpenAPITools/openapi-generator/pull/6036

brendandburns avatar Aug 16 '24 16:08 brendandburns