Skip to content

fix: AuthenticationRefresher wrapper solution #3406

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

fix: AuthenticationRefresher wrapper solution #3406

wants to merge 3 commits into from

Conversation

raykrueger
Copy link

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2024
@k8s-ci-robot
Copy link
Contributor

[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
Copy link
Contributor

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. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 16, 2024
@dims
Copy link

dims commented May 17, 2024

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

@raykrueger
Copy link
Author

@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 raykrueger force-pushed the authentication_refresh branch from 254a190 to 4d97dea Compare May 17, 2024 21:02
@raykrueger
Copy link
Author

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

@raykrueger
Copy link
Author

(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 raykrueger force-pushed the authentication_refresh branch from 4d97dea to 50d46fe Compare May 17, 2024 21:12
@raykrueger
AuthenticationRefresher wrapper solution
ce0f769 
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 kubernetes-client#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(someDelegateAuthenticationInstace, 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 raykrueger force-pushed the authentication_refresh branch from 50d46fe to f99d6df Compare May 17, 2024 21:15
brendandburns
brendandburns reviewed May 18, 2024
View reviewed changes
util/src/main/java/io/kubernetes/client/util/credentials/AuthenticationRefresher.java
@Override
public void provide(ApiClient client) {

synchronized (this) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this lazily on the first call to provide vs. doing it in the contructor? Do you expect that the ApiClient will get garbage collected repeatedly but this authentication will stick around?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about this. Why not just create the executor once in the constructor and be done with it? This current logic seems convulted and I'm not sure I see the value of the lazy evaluation.

@brendandburns
Copy link
Contributor

Few more comments. Thanks.

@raykrueger raykrueger force-pushed the authentication_refresh branch from f99d6df to 7b3bede Compare May 19, 2024 17:16
@raykrueger
Updates to the AuthenticationRefresher timer
29e1fe3 
The AuthenticationRefreher will now keep a instance-level reference to
the current Executor. A singleton style synchronization ensures there is
only ever one Executor active.

Additionally, the APIClient passed into .provide is now heald as a
WeakReference by the Executor. if the APIClient is ever garbage
collected the Executor is cancelled and nulled.
@raykrueger raykrueger force-pushed the authentication_refresh branch from 7b3bede to 29e1fe3 Compare May 19, 2024 17:22
@raykrueger
Copy link
Author

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

@raykrueger
Copy link
Author

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

@yue9944882
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 27, 2024
@brendandburns
Copy link
Contributor

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

@bryantbiggs
Copy link

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

@brendandburns
Copy link
Contributor

@bryantbiggs still waiting afaik.

@brendandburns
Copy link
Contributor

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
Copy link
Contributor

See:
OpenAPITools/openapi-generator#6036

@brendandburns brendandburns mentioned this pull request Aug 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendandburns brendandburns brendandburns left review comments

@yue9944882 yue9944882 Awaiting requested review from yue9944882

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@raykrueger @k8s-ci-robot @dims @brendandburns @yue9944882 @bryantbiggs