-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ExpiredAuthenticationToken errors when using Azure CLI credentials. Fixes #521 #544
Fix ExpiredAuthenticationToken errors when using Azure CLI credentials. Fixes #521 #544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LalitTurbot Please see suggestions, thanks!
azure/service.go
Outdated
@@ -48,6 +48,7 @@ func GetNewSession(ctx context.Context, d *plugin.QueryData, tokenAudience strin | |||
if cachedData, ok := d.ConnectionManager.Cache.Get(cacheKey); ok { | |||
session = cachedData.(*Session) | |||
if session.Expires != nil && WillExpireIn(*session.Expires, 0) { | |||
logger.Info("GetNewSession", "cache expired", "delete cache and obtain new session token") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still want this log message? If so, should this be Trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will convert this and others to the required levels.
Sorry, left over from testing
azure/service.go
Outdated
} | ||
default: | ||
logger.Trace("Getting token for authorizer from Azure CLI") | ||
logger.Warn("Getting token for authorizer from Azure CLI") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Warn("Getting token for authorizer from Azure CLI") | |
logger.Trace("Getting token for authorizer from Azure CLI") |
azure/service.go
Outdated
token, err := cli.GetTokenFromCLI(resource) | ||
if err != nil { | ||
plugin.Logger(ctx).Error("GetNewSession", "cli.GetTokenFromCLI error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin.Logger(ctx).Error("GetNewSession", "cli.GetTokenFromCLI error", err) | |
plugin.Logger(ctx).Error("GetNewSession", "get_token_from_cli_error", err) |
azure/service.go
Outdated
return nil, err | ||
} | ||
|
||
adalToken, err := token.ToADALToken() | ||
expiresOn = types.Time(adalToken.Expires()) | ||
logger.Warn("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Warn("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local()) | |
logger.Trace("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local()) |
Should this be Trace?
azure/service.go
Outdated
} | ||
|
||
// Get the subscription ID and tenant ID from CLI if not set in connection | ||
// config or environment variables | ||
if authMethod == "CLI" && (settings.Values[auth.SubscriptionID] == "" || settings.Values[auth.TenantID] == "") { | ||
logger.Trace("Getting subscription ID and/or tenant ID from from Azure CLI") | ||
logger.Debug("Getting subscription ID and/or tenant ID from from Azure CLI") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Debug("Getting subscription ID and/or tenant ID from from Azure CLI") | |
logger.Trace("Getting subscription ID and/or tenant ID from from Azure CLI") |
Should this be Trace or Debug? It seems like we use both throughout this function, so I'm a bit confused.
azure/service.go
Outdated
if strings.Contains(err.Error(), "invalid_grant") { | ||
return nil, fmt.Errorf("ValidationError: The credential data used by the CLI has expired because you might have changed or reset the password. Please clear your browser's cookies and run 'az login'.") | ||
} | ||
return nil, err | ||
} | ||
authorizer = autorest.NewBearerAuthorizer(&adalToken) | ||
default: | ||
return nil, fmt.Errorf("GetNewSession. invalid authenticaion method, please check plugin configuration and restart plugin.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("GetNewSession. invalid authenticaion method, please check plugin configuration and restart plugin.") | |
return nil, fmt.Errorf("invalid Azure authentication method: %w") |
Example query results
Results