Reduce requests #823
Reduce requests #823
Conversation
Reduce the requests made to HEAD requests in order to determine the type of authentication required, and actually authenticate. Once authenticated, then make the request. This strategy preserves the content of the request object, particularly when they contains stream objects as in the case of multipart/formdata requests.
|
Can one of the admins verify this patch? |
|
@tamarrow Any chance you could look at this and advise on if/when a release/merge would be possible? Thanks. |
tamarrow-zz
left a comment
tamarrow-zz
left a comment
There was a problem hiding this comment.
Thanks for the pr @huntc! Looks good, just a few comments. Once you make those changes and tests pass we can merge. A release will likely happen next week.
test_marathon.test_add_app_through_http failed: https://teamcity.mesosphere.io/viewLog.html?buildId=459433&buildTypeId=DcosIo_DcosCli_Ci&guest=1
Looks like we need to add a HEAD request handler: https://github.com/dcos/dcos-cli/blob/master/cli/tests/integrations/test_marathon.py#L830
dcos/http.py
Outdated
| else: | ||
| auth = AUTH_CREDS[creds] | ||
|
|
||
| # try request again, with auth |
There was a problem hiding this comment.
we should remove this comment
dcos/http.py
Outdated
| raise DCOSException(msg) | ||
|
|
||
| if response.status_code == 401: | ||
| raise DCOSAuthenticationException(response) |
There was a problem hiding this comment.
we don't want to raise here anymore since we are looping outside this function.
There was a problem hiding this comment.
I left it there because the prev implementation was responsible for throwing it ie I've preserved its contract. I don't think it is good to rely on the outer behaviour unless otherwise specified in its contract.
That said, I'll change it if that's what you want. Please confirm.
There was a problem hiding this comment.
The same behavior will now occur by this: https://github.com/dcos/dcos-cli/pull/823/files#diff-e26d04d9fd1e02892424e2a0efea9411R226
If we keep this here, this loop would never be run more than once: https://github.com/dcos/dcos-cli/pull/823/files#diff-e26d04d9fd1e02892424e2a0efea9411R220
There was a problem hiding this comment.
We can update the pydoc with this information
There was a problem hiding this comment.
Looking further, I guess it is an internal function so perhaps no need to doc further. I'll remove the raising of the exception.
dcos/http.py
Outdated
|
|
||
| response = _request(method, url, is_success, timeout, | ||
| verify=verify, **kwargs) | ||
| response = _request('head', url, is_success, timeout, |
There was a problem hiding this comment.
lets add a comment to explain why we are requesting with `head.
There was a problem hiding this comment.
Will do. I will enhance the pydoc with this info.
|
ok to test |
dcos/http.py
Outdated
| verify=verify) | ||
| i = 0 | ||
| while i < 3 and response.status_code == 401: | ||
| auth_response = _request_with_auth(response, 'head', url, is_success, |
There was a problem hiding this comment.
lets just name this response
There was a problem hiding this comment.
You don't want to overwrite the orig response given that the www-authenticate header will be lost.
|
PR feedback incorporated. I previously ran |
|
@tamarrow I may need your help in getting the integration tests done. Thanks. BTW things are now working well with my DC/OS CLI sub command integration given these changes. I've been using the changes here against our production environment. |
|
Yay, glad to hear it! Re: integration tests: |
|
I'm really sorry to ask - would you mind updating the CLI tests (I'm unable to run them locally)... I'm having great difficulty running them and I feel that we're going to play ping-pong here otherwise. Sorry again to ask. I've granted you access to my repo so that you can push changes there. |
Eliminates our dependency on the DC/OS lib until the day that dcos/dcos-cli#823 gets merged and we have a new lib.
Eliminates our dependency on the DC/OS lib until the day that dcos/dcos-cli#823 gets merged and we have a new lib.
|
@huntc happy to help, I'll get to it as soon as I can. If you want to talk about difficulties with running the tests locally, free free to join our open slack account: http://chat.mesosphere.com/ and we can chat there. |
|
Thanks! I'll post my troubles to your gitter channel today.
|
Eliminates our dependency on the DC/OS lib until the day that dcos/dcos-cli#823 gets merged and we have a new lib.
| dcos_url = config.get_config_val("core.dcos_url") | ||
| auth_url = urllib.parse.urljoin(dcos_url, 'exhibitor/') | ||
| response = _request( | ||
| 'GET', auth_url, is_success, timeout, verify=verify, **kwargs) |
There was a problem hiding this comment.
I'd still use a HEAD here - it is cheaper given the lack of content in reply. You'll still get the headers you need, and your service can potentially optimise in terms of handling HEAD.
| i = 0 | ||
| while i < 3 and response.status_code == 401: | ||
| auth_response = _request_with_auth( | ||
| response, 'GET', auth_url, is_success, timeout, |
There was a problem hiding this comment.
Also, HEAD should be cheaper as mentioned. HEAD should always be interchangeable with GET.
huntc
left a comment
huntc
left a comment
There was a problem hiding this comment.
All I'd suggest is changing to HEAD in place of GET.
One more thing: I'm unsure how "cheap" the Exhibitor URL is... it appears to render some state. You probably want something static, perhaps even an icon or something. This request will occur often.
|
Closing since this bug is fixed with #824 |
3 participants
Reduce the requests made to HEAD requests in order to determine the type of authentication required, and actually authenticate. Once authenticated, then make the request. This strategy preserves the content of the request object, particularly when they contains stream objects as in the case of multipart/formdata requests.
Fixes #818