client: periodically reload InClusterConfig token #67359
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
sig/api-machinery
mikedanese
force-pushed
the
reloadtoken
branch
3 times, most recently
from
701b6c8
to
5623242
Compare
|
/retest |
| } | ||
|
|
||
| if _, err := ioutil.ReadFile(tokenFile); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Give more context to the error and check that file isn't empty.
Or create a cachingTokenSource first and do this check via ts.Token()
| } | ||
|
|
||
| func (ts *fileTokenSource) Token() (*oauth2.Token, error) { | ||
| tokb, err := ioutil.ReadFile(ts.path) |
There was a problem hiding this comment.
Check that token isn't empty and strings.TrimSpace it before returning to get rid of trailing newlines
There was a problem hiding this comment.
Disagree here. We should be consistent with how token files are handled elsewhere. We don't want to change the behavior of how we handle token files. That could lead to token files with leading/trailing spaces that work with this (common) client library and fail with others that expect the data to be good
| now func() time.Time | ||
| } | ||
|
|
||
| var _ = oauth2.TokenSource(&cachingTokenSource{}) |
There was a problem hiding this comment.
do the same for fileTokenSource
| var _ = oauth2.TokenSource(&cachingTokenSource{}) | ||
|
|
||
| func (ts *cachingTokenSource) Token() (*oauth2.Token, error) { | ||
| now := time.Now() |
There was a problem hiding this comment.
Add func newCachingTokenSource(base oauth2.TokenSource) and set ts.now and ts.leeway there
|
|
||
| ts := &cachingTokenSource{ | ||
| base: tts, | ||
| tok: c.tok, |
There was a problem hiding this comment.
Recommend leaving tok empty to make sure it works in initial state
There was a problem hiding this comment.
I have test cases that do that.
| now: func() time.Time { return start.Add(c.wait) }, | ||
| } | ||
|
|
||
| gottok, goterr := ts.Token() |
| if got, want := tts.calls, c.wantTSCalls; got != want { | ||
| t.Errorf("unexpected number of Token() calls: got %d, want %d", got, want) | ||
| } | ||
| if got, want := goterr != nil, c.wantErr; got != want { |
There was a problem hiding this comment.
if gotErr != nil && !c.wantErr {
t.Errorf(...)
}
if gotErr == nil && c.wantErr {
t.Errorf(...)
}
| for i := 0; i < 100; i++ { | ||
| go func() { | ||
| ts.Token() | ||
| wg.Done() |
There was a problem hiding this comment.
defer this before calling ts.Token
mikedanese
force-pushed
the
reloadtoken
branch
from
5623242
to
d6d4d56
Compare
|
limiting this to InClusterConfig seems like the safest first step. Are there equivalent methods in the client libraries for other languages we need to update? Would you expect the same updating behavior to apply to referenced token files eventually? I know of pods that run components using a kubeconfig file constructed with caFile/tokenFile references that point to the injected serviceaccount ca/token paths. |
mikedanese
force-pushed
the
reloadtoken
branch
from
d6d4d56
to
b99f717
Compare
|
I would expect this to work tor token-file as well. I'd like to replace the BearerToken field in rest.Config with a oauth2.TokenSource to unify all these methods. I can do that now or later. |
mikedanese
force-pushed
the
reloadtoken
branch
2 times, most recently
from
32a8559
to
8aa066d
Compare
|
/assign @cheftako |
|
/assign @mbohlool |
| return tok, nil | ||
| } | ||
|
|
||
| tok, err := ts.base.Token() |
There was a problem hiding this comment.
if we previously successfully read a token, and failed to re-read a token, I think we should continue to use the previously read token.
| leeway: 1 * time.Minute, | ||
| base: &fileTokenSource{ | ||
| path: path, | ||
| period: 5 * time.Minute, |
There was a problem hiding this comment.
comment explaining why this period was chosen would be good
There was a problem hiding this comment.
If only there was a reason...
|
a couple comments, looks reasonable to me otherwise. if token reloading is desired/required in the future (at least for in-cluster configs), this should be noted in the kubernetes client guidelines, and issues should be opened to track adding it to the client libraries in other languages |
mikedanese
force-pushed
the
reloadtoken
branch
from
8aa066d
to
8c171dc
Compare
|
/uncc |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/lgtm cancel |
k8s-ci-robot
removed
lgtm
mikedanese
force-pushed
the
reloadtoken
branch
from
9ad162a
to
287f6a5
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
The last directory is the csi-api Godeps, that was added after the fact. I'm going to submit a PR to fix, but I'm also going to manually approve to get this in |
cblecker
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: cblecker, liggitt, mikedanese 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 |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
PR to add OWNERS for that new directory: #68174 |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md. |
|
@kubernetes/sig-auth-bugs @kubernetes/sig-api-machinery-bugs - Folks - we're still digging into the source, but so far we believe this is the prime suspect which caused a regression in the in-cluster configuration that is used e2e test framework. xref: #69234 (comment) |
|
Submitted - #69272 |
/sig auth
/sig api-machinery