aad fallback to real auth if refresh token fails, fixes #82776 #86481
Conversation
k8s-ci-robot
added
release-note
|
Welcome @tdihp! |
|
Hi @tdihp. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/kind bug |
k8s-ci-robot
added
area/provider/azure
|
Could you remove |
|
@weinong would you like to have a look? |
| if err != nil { | ||
| return nil, fmt.Errorf("storing the refreshed token in configuration: %v", err) | ||
| return nil, fmt.Errorf("acquiring a new fresh token: %v", err) |
There was a problem hiding this comment.
nit: Failed to acquire new token: %v
There was a problem hiding this comment.
The original code was always using ": %v", err for err message, maybe I can refactor it in this PR?
staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
Outdated
Show resolved
Hide resolved
|
|
||
| // 3. extend if valid token but expired | ||
| if token != nil && token.token.IsExpired() { | ||
| klog.V(4).Infof("Refreshing token.") |
|
|
||
| // set cfg to cache | ||
| if err == nil { | ||
| klog.V(4).Infof("Saving cache after load cfg") |
|
/assign @liggitt |
|
/milestone v1.18 |
| } | ||
| if !token.token.IsExpired() { | ||
|
|
||
| if err == nil { | ||
| ts.cache.setToken(azureTokenKey, token) |
There was a problem hiding this comment.
this will now set an expired token in the cache... is that what we want?
| if err != nil { | ||
| return nil, fmt.Errorf("refreshing the expired token: %v", err) | ||
| if _, ok := err.(adal.TokenRefreshError); ok { |
There was a problem hiding this comment.
should this use autorest.IsTokenRefreshError() instead?
|
Here is how I would approach it (while trying to avoid deeply nested Diff from current PR: diff --git staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
index f97bc6d2309..4949a757f1a 100644
--- staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
+++ staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
@@ -214,29 +214,24 @@ func (ts *azureTokenSource) Token() (*azureToken, error) {
// retrieve from config if no cache
if token == nil {
- token, err = ts.retrieveTokenFromCfg()
-
+ tokenFromCfg, err := ts.retrieveTokenFromCfg()
if err == nil {
- ts.cache.setToken(azureTokenKey, token)
+ token = tokenFromCfg
}
}
- // quick exit for caching scenario
- if token != nil && !token.token.IsExpired() {
- return token, nil
- }
-
if token != nil && token.token.IsExpired() {
klog.V(4).Info("Refreshing token.")
token, err = ts.Refresh(token)
- if err != nil {
- if _, ok := err.(adal.TokenRefreshError); ok {
- // When Refresh fails, token will be reset to nil
- // so that the inner token source will be used to acquire new
- klog.V(4).Infof("Failed to refresh expired token, proceed to auth: %v", err)
- } else {
- return nil, fmt.Errorf("unexpected error when refreshing token: %v", err)
- }
+ switch {
+ case err == nil:
+ // all good
+ case autorest.IsTokenRefreshError(err):
+ // When Refresh fails, token will be reset to nil
+ // so that the inner token source will be used to acquire new
+ klog.V(4).Infof("Failed to refresh expired token, proceed to auth: %v", err)
+ default:
+ return nil, fmt.Errorf("unexpected error when refreshing token: %v", err)
}
}
@@ -245,17 +240,23 @@ func (ts *azureTokenSource) Token() (*azureToken, error) {
if err != nil {
return nil, fmt.Errorf("failed acquiring new token: %v", err)
}
- // corner condition, newly got token is valid but expired
- if token.token.IsExpired() {
- return nil, fmt.Errorf("newly acquired token is expired")
- }
}
- ts.cache.setToken(azureTokenKey, token)
+ // sanity check
+ if token == nil {
+ return nil, fmt.Errorf("unable to acquire token")
+ }
+
+ // corner condition, newly got token is valid but expired
+ if token.token.IsExpired() {
+ return nil, fmt.Errorf("newly acquired token is expired")
+ }
+
err = ts.storeTokenInCfg(token)
if err != nil {
return nil, fmt.Errorf("storing the refreshed token in configuration: %v", err)
}
+ ts.cache.setToken(azureTokenKey, token)
return token, nil
}Diff from master: diff --git staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
index 11ef7afb213..4949a757f1a 100644
--- staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
+++ staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure.go
@@ -180,6 +180,7 @@ type azureToken struct {
type tokenSource interface {
Token() (*azureToken, error)
+ Refresh(*azureToken) (*azureToken, error)
}
type azureTokenSource struct {
@@ -210,33 +211,53 @@ func (ts *azureTokenSource) Token() (*azureToken, error) {
var err error
token := ts.cache.getToken(azureTokenKey)
+
+ // retrieve from config if no cache
if token == nil {
- token, err = ts.retrieveTokenFromCfg()
- if err != nil {
- token, err = ts.source.Token()
- if err != nil {
- return nil, fmt.Errorf("acquiring a new fresh token: %v", err)
- }
- }
- if !token.token.IsExpired() {
- ts.cache.setToken(azureTokenKey, token)
- err = ts.storeTokenInCfg(token)
- if err != nil {
- return nil, fmt.Errorf("storing the token in configuration: %v", err)
- }
+ tokenFromCfg, err := ts.retrieveTokenFromCfg()
+ if err == nil {
+ token = tokenFromCfg
}
}
- if token.token.IsExpired() {
- token, err = ts.refreshToken(token)
- if err != nil {
- return nil, fmt.Errorf("refreshing the expired token: %v", err)
+
+ if token != nil && token.token.IsExpired() {
+ klog.V(4).Info("Refreshing token.")
+ token, err = ts.Refresh(token)
+ switch {
+ case err == nil:
+ // all good
+ case autorest.IsTokenRefreshError(err):
+ // When Refresh fails, token will be reset to nil
+ // so that the inner token source will be used to acquire new
+ klog.V(4).Infof("Failed to refresh expired token, proceed to auth: %v", err)
+ default:
+ return nil, fmt.Errorf("unexpected error when refreshing token: %v", err)
}
- ts.cache.setToken(azureTokenKey, token)
- err = ts.storeTokenInCfg(token)
+ }
+
+ if token == nil {
+ token, err = ts.source.Token()
if err != nil {
- return nil, fmt.Errorf("storing the refreshed token in configuration: %v", err)
+ return nil, fmt.Errorf("failed acquiring new token: %v", err)
}
}
+
+ // sanity check
+ if token == nil {
+ return nil, fmt.Errorf("unable to acquire token")
+ }
+
+ // corner condition, newly got token is valid but expired
+ if token.token.IsExpired() {
+ return nil, fmt.Errorf("newly acquired token is expired")
+ }
+
+ err = ts.storeTokenInCfg(token)
+ if err != nil {
+ return nil, fmt.Errorf("storing the refreshed token in configuration: %v", err)
+ }
+ ts.cache.setToken(azureTokenKey, token)
+
return token, nil
}
@@ -314,7 +335,13 @@ func (ts *azureTokenSource) storeTokenInCfg(token *azureToken) error {
return nil
}
-func (ts *azureTokenSource) refreshToken(token *azureToken) (*azureToken, error) {
+func (ts *azureTokenSource) Refresh(token *azureToken) (*azureToken, error) {
+ return ts.source.Refresh(token)
+}
+
+// refresh outdated token with adal.
+// adal.RefreshTokenError will be returned if error occur during refreshing.
+func (ts *azureTokenSourceDeviceCode) Refresh(token *azureToken) (*azureToken, error) {
env, err := azure.EnvironmentFromName(token.environment)
if err != nil {
return nil, err |
|
Thanks @enj for the suggestion! It looks cleaner than my attempt! Let me try it. OK if I borrow in this PR? |
Sure. /milestone v1.19 Moving to 1.19 since we are past 1.18 code freeze. |
k8s-ci-robot
removed
the
lgtm
|
This LGTM. @tdihp did you confirm this by running it against AAD? Squash the change into a single commit. |
|
@enj Thanks for confirming, will do my build confirmation again and squash :) |
|
lgtm, please squash to a single commit (and don't put the issue number in the commit title, it spams the issue) |
… add more tests. Signed-off-by: Ping He <tdihp@hotmail.com>
tdihp
force-pushed
the
feature/aad-fallback-real-auth
branch
from
6ac7bfa
to
26c97fa
Compare
|
Thanks @liggitt , rebased & squashed & modified commit message. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, feiskyer, liggitt, tdihp 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
|
/retest Review the full test history for this PR. Silence the bot with an |
Signed-off-by: Ping He tdihp@hotmail.com
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #82776
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: