reload service account token after expiry #967
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @zshihang! |
k8s-ci-robot
added
the
size/L
|
/assign @brendandburns we can use the same approach here to fix #290. |
zshihang
force-pushed
the
master
branch
2 times, most recently
from
774cc46
to
dabe51a
Compare
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
|
|
||
| public class TokenFileAuthentication implements Authentication, Interceptor { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
|
||
| @Override | ||
| public void provide(ApiClient client) { | ||
| OkHttpClient withInterceptor = client.getHttpClient().newBuilder().addInterceptor(this).build(); |
There was a problem hiding this comment.
the # of interceptors seems to be growing unboundly upon every provide call, isn't it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
the assumption is made that the provide function is called only once for each in-cluster client.
yea, that's true
| builder.setCertificateAuthority(Files.readAllBytes(Paths.get(SERVICEACCOUNT_CA_PATH))); | ||
| builder.setAuthentication(new AccessTokenAuthentication(token)); | ||
| builder.setAuthentication(new TokenFileAuthentication(SERVICEACCOUNT_TOKEN_PATH)); |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
k8s-ci-robot
added
the
lgtm
|
thanks for the review. need /approval label to merge. |
|
/approve |
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
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.