-
Notifications
You must be signed in to change notification settings - Fork 383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[client] Handle invalid proxy settings #3198
[client] Handle invalid proxy settings #3198
Conversation
e8c5251
to
b4d6a2e
Compare
@@ -21,6 +21,37 @@ | |||
|
|||
LOG = get_logger('system') | |||
|
|||
PROXY_VALIDATION_NEEDED = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider using a "local static"-ish variable instead of a global one:
def f():
f.my_static_var = 42
PROXY_VALIDATION_NEEDED = True | ||
|
||
|
||
def proxy_settings_are_valid(transport): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not checking the http[s]
scheme at the beginning, but line 73 says that it will be checked.
Minor thing: maybe the content of this function can go inside validate_proxy_format()
, since it's quite short.
If `HTTP_PROXY` / `HTTPS_PROXY` environment variables are set and the URL is invalid (e.g.: it doesn't contains a schema like localhost:8001) the client will throw the following exception: ``` 'NoneType' object has no attribute 'rfind' File "/codechecker/build/CodeChecker/cc_bin/CodeChecker.py", line 174, in <module> main(commands) File "/codechecker/build/CodeChecker/cc_bin/CodeChecker.py", line 141, in main sys.exit(args.func(args)) File "//codechecker/build/CodeChecker/lib/python3/codechecker_client/cmd/store.py", line 856, in main traceback.print_stack() ``` From this error message it is hard to understand the real problem. Unfortunately Thrift does not handle the use case when invalid proxy format is used (e.g.: no schema is specified). For this reason we need to verify the proxy format in our side.
b4d6a2e
to
71501e7
Compare
@bruntib I created a new commit where I refactored the whole thing. I moved the checking under the |
# Thrift do not handle the use case when invalid proxy format is | ||
# used (e.g.: no schema is specified). For this reason we need to | ||
# verify the proxy format in our side. | ||
self._validate_proxy_format() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If THttpClient throws TTransportException for some reason but not because something bad with proxy settings then this finally branch try to validate proxy settings.
Please call self._validate_proxy_format() in the ValueError branch not in the finally branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the THttpClient
object creation is inside a try/except block, which cannot raise such exception like TTransportException
.
Two scenarios are possible here:
- The user specified the following environment variable:
HTTP_PROXY=http://localhost
/HTTP_PROXY=http://localhost:not_a_number
where the port is not specified or it is not an integer. In this case it will raise a ValueError exception. - The user specified the following environment variable:
HTTP_PROXY=localhost:8001
where the schema is not given. In this case theTHttpClient
object creation will not raise any exception (it will raise exception when an API call happens). So this is the reason when this object creation is done, we need to check the proxy format before any API call happens.
In both cases we need to check the proxy format. So it is not enough to call this function inside the ValueError branch. We need to call it every time when there is no exception.
If
HTTP_PROXY
/HTTPS_PROXY
environment variables are set and theURL is invalid (e.g.: it doesn't contains a schema like localhost:8001)
the client will throw the following exception:
From this error message it is hard to understand the real problem.
Unfortunately Thrift does not handle the use case when invalid proxy format
is used (e.g.: no schema is specified). For this reason we need to verify
the proxy format in our side.