Open tswast opened 3 years ago
Thank you for the detailed repro!
I think you're correct that this likely needs to be tweaked in google-api-core. I'm not quite sure what the best way to do that is yet. I'd like to check on the auth lib implementations in other languages to see how they handle the 401 case.
Description of what's happening:
Since google-cloud-bigquery-storage
uses the gRPC transport, the credential refresh is part of the AuthMetadataPlugin
and is called by gRPC before making the request.
before_request
checks if a credential is "valid" prior to sending the request.
https://github.com/googleapis/google-auth-library-python/blob/f1fee1f4c3d511d9e6ecbc1c0397e743bf2583db/google/auth/credentials.py#L115-L134
Valid means that a token is present and that the token is not expired. This check can fall apart if the clock is incorrect. The credential is expired but self.expired = False
, which means the token isn't refreshed before making the gRPC request.
After the initial failed gRPC request the retry behavior follows the retryable errors and backoff/timeout settings comes from the gRPC service config that sits next to the protos. The retry objects are added to the wrapped methods. Unauthorized
is probably never in that list, since per https://google.aip.dev/194 it usually isn't helpful to retry such an error.
It would be nice to be able to handle this like the Request transport, which would retry the 401 up to two times.
https://github.com/googleapis/google-auth-library-python/blob/f1fee1f4c3d511d9e6ecbc1c0397e743bf2583db/google/auth/transport/requests.py#L492-L501 https://github.com/googleapis/google-auth-library-python/blob/f1fee1f4c3d511d9e6ecbc1c0397e743bf2583db/google/auth/transport/__init__.py#L32-L35
Retry could help, but really it needs to be a retry after refreshing the credentials. I'm thinking about the case when the clock skew is so large that client-side we don't think an access token is expired but it actually is.
That _refresh_status_codes
is a good find! That does seem to be the right place.
I checked with other folks and this is what I heard back:
higher clock skew
Like the 5 minutes clock skew? From "since metadata server token endpoint doesn't generate a new token until 30s before the expiration", perhaps 30s might help and also avoid the [metadata server] issue somewhat?
opt into retrying a 4xx once
I still think this is necessary. I'm not sure why it'd be opt-in, though. It's on by default currently on our REST transport, right? A single retry is probably enough. If it fails, hopefully it's with a retryable error and one layer up at api-core retries can kick in.
@busunkim96 @tswast Is this issue resolved with https://github.com/googleapis/google-auth-library-python/pull/863?
Maybe? I'd be much more comfortable saying yes if there was also the "opt into retrying a 4xx once" feature. Given all the issues I've seen, especially with Cloud Functions, I don't really trust that we can get the token proactively in all cases.
As stated in the issue title, if access token is expired / invalid, but refresh token is still valid, requests fail with
google.api_core.exceptions.Unauthenticated: 401 Request had invalid authentication credentials
. This could happen if the customer's clock is out-of-sync, for example.This issue is in response to a customer issue (internal tracker 191460918).
Environment details
google-auth
version:Steps to reproduce
Works fine:
Fails:
Stack trace:
Desired fix
Refresh and retry after 401 errors. This is subtly different from adding Unauthenticated to the default list of retryable errors in that a refresh should be attempted first.
Perhaps this issue is best addressed in google-api-core?