Repository navigation
fix(auth): retry transient STS error responses - #18564
Shubham-Padkonde wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements exponential backoff retries for transient HTTP and OAuth error responses in the Secure Token Service (STS) client, updating the request handling logic, error translation, and adding comprehensive unit tests. The review feedback highlights two potential issues: a potential TypeError if response.data is None when decoding the response body, and a potential UnboundLocalError if the exponential backoff loop does not execute, which can be resolved by initializing response_body and retryable before the loop.
| response_body = ( | ||
| response.data.decode("utf-8") | ||
| if hasattr(response.data, "decode") | ||
| else response.data | ||
| ) |
There was a problem hiding this comment.
If response.data is None (which can happen with empty responses or in certain mock/error scenarios), response_body will be assigned None. This will cause a TypeError when json.loads(response_body) is called later in this function, or in utils.handle_error_response(), because json.loads does not accept None. Falling back to an empty string "" avoids this issue and allows the JSON parser to raise a ValueError (or JSONDecodeError), which is already correctly handled by the try/except blocks.
| response_body = ( | |
| response.data.decode("utf-8") | |
| if hasattr(response.data, "decode") | |
| else response.data | |
| ) | |
| response_body = ( | |
| response.data.decode("utf-8") | |
| if hasattr(response.data, "decode") | |
| else (response.data or "") | |
| ) |
| encoded_body = urllib.parse.urlencode(request_body).encode("utf-8") | ||
| for _ in _exponential_backoff.ExponentialBackoff(): |
There was a problem hiding this comment.
To prevent potential UnboundLocalError exceptions if the _exponential_backoff.ExponentialBackoff() generator is empty or mocked to return no elements, initialize response_body and retryable before entering the loop.
encoded_body = urllib.parse.urlencode(request_body).encode("utf-8")
response_body = ""
retryable = False
for _ in _exponential_backoff.ExponentialBackoff():
STS currently raises a non-retryable OAuthError immediately for transient HTTP/OAuth responses, unlike the standard OAuth token helper. This uses the same transient-response classifier and bounded exponential backoff, preserves request authentication and encoded payload across attempts, and carries
retryable=Trueinto the final OAuthError when retries are exhausted.Related to #18548: this addresses the HTTP/OAuth response portion. Transport exceptions propagate unchanged; it does not classify every transport failure (including configuration or TLS failures) as transient.
Tests cover recovery from JSON and text 5xx responses, OAuth
temporarily_unavailable, exhausted retries, permanent errors, a transient-to-permanent transition, stable request bodies, and unchanged transport exceptions. The existing empty revocation-response behavior is retained and documented alongside the retry behavior.sts.pyandutils.py: 100% statement and branch coverage; full package coverage is 99% with existing uncovered paths elsewhere.Checklist: