reload service account token after expiry by zshihang · Pull Request #967 · kubernetes-client/java

zshihang · 2020-06-02T01:06:56Z

fixes #652. by default, this will set an expiration period of 1 minutes on service account token. as we are graduating the Beta feature Projected Service Account Volume to GA, we expect clients to reload token before it expires.

k8s-ci-robot · 2020-06-02T01:07:04Z

Welcome @zshihang!

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

zshihang · 2020-06-02T01:09:23Z

/assign @brendandburns

we can use the same approach here to fix #290.

yue9944882

thank you for starting this

yue9944882 · 2020-06-02T04:48:03Z

util/src/main/java/io/kubernetes/client/util/credentials/TokenFileAuthentication.java

+import okhttp3.Request;
+import okhttp3.Response;
+
+public class TokenFileAuthentication implements Authentication, Interceptor {


i presume using Interceptor to hijack the header is a bit overkilling.. the previous approach by AccessTokenAuthentication is setting tokens by ((ApiKeyAuth) auth).setApiKey(apiKey); in the provide call, wdyt if we do the token-reloading similarly?

the current openapi doesn't have reloading logic in ApiKeyAuth which will only be set once in the initialization of in-cluster client. i couldn't figure out a way to achieve what you described without making changes to openapi.

we need something similar to this change OpenAPITools/openapi-generator#6036 (it might take much longer time to merge) to implement reloading.

the current openapi doesn't have reloading logic in ApiKeyAuth which will only be set once in the initialization of in-cluster client.

convinced, can you leave a TODO comment to reference OpenAPITools/openapi-generator#6036 so that we can converge in the future?

yue9944882 · 2020-06-02T04:49:58Z

util/src/main/java/io/kubernetes/client/util/credentials/TokenFileAuthentication.java

+
+  @Override
+  public void provide(ApiClient client) {
+    OkHttpClient withInterceptor = client.getHttpClient().newBuilder().addInterceptor(this).build();


the # of interceptors seems to be growing unboundly upon every provide call, isn't it?

yes, the assumption is made that the provide function is called only once for each in-cluster client. i am not sure when a user will call this twice on the same client instance.

the assumption is made that the provide function is called only once for each in-cluster client.

yea, that's true

yue9944882 · 2020-06-02T05:18:03Z

util/src/main/java/io/kubernetes/client/util/ClientBuilder.java

-            Files.readAllBytes(Paths.get(SERVICEACCOUNT_TOKEN_PATH)), Charset.defaultCharset());
    builder.setCertificateAuthority(Files.readAllBytes(Paths.get(SERVICEACCOUNT_CA_PATH)));
-    builder.setAuthentication(new AccessTokenAuthentication(token));
+    builder.setAuthentication(new TokenFileAuthentication(SERVICEACCOUNT_TOKEN_PATH));


we need to keep backward compatibility and users are expected to opt-in to the change. wdyt add a constructor w/ parameter reloadInterval and factor the original cluster() to adopt an infinite (or zero) interval?

NOTE: even tho minute-ish reloading SA is always recommended and users are even supposed to be not aware of the change, i suppose it will take a release or two for users to get onboard to the SA rotation..

cluster() with reloading should be the default so that users dont have to do anything to migrate and add a oldCluster as the fail safe.

update the InClusterClusterExample to something like:

// new style ApiClient client = ClientBuilder.cluster().build(); // old style ApiClient client = ClientBuilder.oldCluster().build();

WDYT?

cluster() with reloading should be the default so that users dont have to do anything to migrate and add a oldCluster as the fail safe.

yea we should also reflect how to opt-in SA reloading in the example. note that there's two aspects of change the pull is involving:

introducing capability for periodical SA reloading

moving default behavior the in-cluster client to 1-min reloading (preserving the old-style method or not)

the first is good while i'm leaning on doing the second in another release. i will defer to @brendandburns for thoughts

Let's just do the whole thing. We haven't cut a 9.0.0 release yet so I think it's ok to make this change in the 9.0.0 release.

yue9944882

/lgtm

zshihang · 2020-06-03T03:57:00Z

thanks for the review. need /approval label to merge.

yue9944882 · 2020-06-05T08:31:50Z

/approve

k8s-ci-robot · 2020-06-05T08:31:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yue9944882, zshihang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yue9944882]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2020

k8s-ci-robot requested review from brendandburns and mbohlool June 2, 2020 01:07

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 2, 2020

k8s-ci-robot assigned brendandburns Jun 2, 2020

zshihang force-pushed the master branch 2 times, most recently from 774cc46 to dabe51a Compare June 2, 2020 01:14

yue9944882 reviewed Jun 2, 2020

View reviewed changes

reload service account token after expiry

41844ea

zshihang force-pushed the master branch from dabe51a to 41844ea Compare June 2, 2020 21:05

yue9944882 approved these changes Jun 3, 2020

View reviewed changes

k8s-ci-robot assigned yue9944882 Jun 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2020

k8s-ci-robot merged commit 913ecb9 into kubernetes-client:master Jun 5, 2020

tsaarni mentioned this pull request Sep 18, 2021

fix: TokenRefreshInterceptor throws when running incluster fabric8io/kubernetes-client#3445

Merged

11 tasks

finalchild mentioned this pull request Nov 28, 2023

Service account tokens should be reloaded coralogix/zio-k8s#553

Open

Conversation

zshihang commented Jun 2, 2020

Uh oh!

k8s-ci-robot commented Jun 2, 2020

Uh oh!

zshihang commented Jun 2, 2020

Uh oh!

yue9944882 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zshihang Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue9944882 Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue9944882 left a comment

Choose a reason for hiding this comment

Uh oh!

zshihang commented Jun 3, 2020

Uh oh!

yue9944882 commented Jun 5, 2020

Uh oh!

k8s-ci-robot commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zshihang Jun 2, 2020 •

edited

Loading

yue9944882 Jun 2, 2020 •

edited

Loading