Refactor: Merge TokenFile auth with refresh auth #3817
Conversation
k8s-ci-robot
added
approved
yue9944882
force-pushed
the
refactor/merge-token-file-auth
branch
2 times, most recently
from
6d6d2e4 to
f1e0eaf
Compare
| try { | ||
| return new String(Files.readAllBytes(Paths.get(file)), Charset.defaultCharset()).trim(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Cannot read file: " + file); |
There was a problem hiding this comment.
We should probably convert this to some sort of non-RuntimeException like "FailedTokenAquisitionException" and handle that inside the RefreshAuthentication.java
There was a problem hiding this comment.
That sounds like a good idea but I ended up adding the exception conversion as a runtime/unchecked exception in the parent class RefreshAuthentication. I also tried making it a checked exception in the token supplier function however it will break backward compatibility so I implemented the unchecked exception in the new commit. one example that compatibility would be broken is here where we're loading the token eagerly in RefreshAuthentication :
so the new checked exception would have to be added to the constructor which will require all places referencing it adding explicit handling the checked exception. does that make sense?
|
One small comment and looks like format/testing is failing. |
Signed-off-by: Min Jin <minkimzz@amazon.com>
yue9944882
force-pushed
the
refactor/merge-token-file-auth
branch
from
f1e0eaf to
80d68d1
Compare
k8s-ci-robot
added
size/L
| @@ -63,19 +65,6 @@ void tokenProvided() throws ApiException { | |||
| 1, | |||
| getRequestedFor(urlPathEqualTo("/api/v1/pods")) | |||
| .withHeader("Authorization", equalTo("Bearer token1"))); | |||
|
|
|||
| this.auth.setFile(SERVICEACCOUNT_TOKEN2_PATH); | |||
There was a problem hiding this comment.
these unit tests are no longer needed as it was originally for testing token expiry/rotation behavior. now it's decoupled/delegated to the RefreshAuthentication class.
|
/lgtm Looks good to me. Let's consider refactoring these exceptions in the future. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, yue9944882 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 |
This pull request addresses the TODO comment:
It simplifies the TokenFileAuthentication by taking RefreshAuthentication as the parent class