[DCOS_OSS-618] Ignore core.ssl_verify for external requests #1028
Conversation
dcos/http.py
Outdated
| @@ -190,6 +210,7 @@ def _request_match(expected_url, actual_url): | |||
| if is_success(response.status_code): | |||
| return response | |||
| elif response.status_code == 401: | |||
| prompt_login = config.get_config_val("core.prompt_login", toml_config) | |||
There was a problem hiding this comment.
This change seems unrelated. It should either be a second commit in this pull request with a detailed commit message describing its purpose, or a separate PR.
dcos/http.py
Outdated
| @@ -27,6 +27,34 @@ def _default_is_success(status_code): | |||
|
|
|||
| return 200 <= status_code < 300 | |||
|
|
|||
| def _is_request_to_cluster(url, toml_config=None): | |||
There was a problem hiding this comment.
Can we call this _is_request_to_dcos() instead?
|
Also, we need to add an integration test before we can call this done. |
|
I've just pushed a commit according to your comments. What kind of integration test are you thinking of? As it involves an HTTPS request to an external URL I don't see an obvious way to do it. |
| toml_config = config.get_config() | ||
|
|
||
| dcos_url = urlparse(config.get_config_val("core.dcos_url", toml_config)) | ||
| cosmos_url = urlparse( |
There was a problem hiding this comment.
What is cosmos_url? And what happens if it's not set in the toml file (as far as I know, it's usually not set).
There was a problem hiding this comment.
I also don't have it set, in that case it is ignored, urlparse will return an empty ParseResult and this won't be matched.
dcos/http.py
Outdated
| request_to_cluster = _request_match(dcos_url, parsed_url) or \ | ||
| _request_match(cosmos_url, parsed_url) | ||
|
|
||
| return request_to_cluster |
There was a problem hiding this comment.
You can probably just return the or statement directly without assigning to an intermediate variable? Otherwise, you should rename this variable to something like is_request_to_dcos. Up to you.
There was a problem hiding this comment.
I'll set it as is_request_to_dcos for readability.
dcos/http.py
Outdated
| verify = _verify_ssl(verify, toml_config) | ||
| else: | ||
| # Ignore core.ssl_verify if request is outside the DC/OS cluster (DCOS_OSS-618) | ||
| verify = None |
There was a problem hiding this comment.
Instead of doing an if/else here, I would just do:
if verify and _is_request_to_dcos(url, toml_config):
verify = _verify_ssl(verify, toml_config)
There was a problem hiding this comment.
Here verify can be a path, and even though I coulnd't see it in the codebase, my concern was if this function gets called with an explicit custom CA path for the cluster and an external https url. In such cases I wanted to force verify to None.
c634cce to
dd597fa
Compare
Otherwise, when a cluster is configured with a custom CA path, it will try to use it instead of the system's CA bundle and external requests will fail. In such cases, setting it to None will let the requests package enabling it by default.
Otherwise, when a cluster is configured with a custom CA path, it will
try to use it instead of the system's CA bundle and external requests
will fail.
In such cases, setting it to None will let the requests package enabling
it by default.