added code to only get a new logins on 401,403 or 415. #26
Conversation
open4storage
changed the title
1 similar comment
|
@adrianmo this looks good, recommend merge. |
ecsclient/common/token_request.py
Outdated
| if req.status_code == requests.codes.ok: | ||
| log.debug("Token is valid") | ||
| return token | ||
| if req.status_code == 401 or req.status_code == 403: |
There was a problem hiding this comment.
You are assuming that if status code is not 401 or 403 it'll be a 200 OK response, but we may get another response from the server, like a 500 server error.
I like the fact of getting a new token only for 401 and 403 codes, but we should raise an exception if code != 200.
There was a problem hiding this comment.
BTW, @open4storage have you checked what status code ECS returns when the token is invalid/expired. It should only be 401 Unauthorized, not 403. 403 Forbidden means that the token is valid (user is authenticated) but the user has no permissions to access that resource.
2 similar comments
open4storage
changed the title
| if req.status_code == requests.codes.ok: | ||
| log.debug("Token is valid") | ||
| if req.status_code == 200: | ||
| msg = "Token validation error. Code returned: {0}".format(req.status_code) |
There was a problem hiding this comment.
Log message should be "Token validated successfully" or similar.
| return token | ||
|
|
||
| elif req.status_code in [401, 403, 415]: | ||
| msg = "Token validation error. Code returned: {0}".format(req.status_code) |
There was a problem hiding this comment.
Message should be "Invalid token. Trying to get a new one" or similar.
| @@ -98,13 +98,29 @@ def get_token(self): | |||
| token = self._get_existing_token() | |||
|
|
|||
| if token: | |||
| ''' | |||
There was a problem hiding this comment.
Can you change this to a single line comment with the pound sign (#)?
| else: # i.e. 500 or unknown raise an exception | ||
| msg = "Token validation error. Code returned: {0}".format(req.status_code) | ||
| log.error(msg) | ||
| raise ECSClientException(msg) |
There was a problem hiding this comment.
The exception should be raised from the response like this:
raise ECSClientException.from_response(req, message=msg)
There was a problem hiding this comment.
And we should also add a couple of test cases to validate this new behaviour.
|
This PR was manually squashed and merged in commit 47a6152 |
A new token is issue when there is a /whoami that returns non 200. The problem is that 404 for example are not invalid token and just a page that is not found. In such case a 404 will cause the ecsclient to login again. A long running code that produce 404s eventually exhaust all the tokens in the system.