SNOW-694457: Fix leaking HTTP_PROXY and HTTPS_PROXY environment variables #1398
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
ozturkberkay
changed the title
|
@sfc-gh-sfan I was wondering if this can prioritized? |
There was a problem hiding this comment.
This change looks good to me, but I could also see potential dependency on the env var. @sfc-gh-mkeller: What do you think?
|
hi @sfc-gh-sfan @sfc-gh-mkeller could we please look into this PR ? apparently it's still valid and would help. thank you ! |
sfc-gh-mkeller
force-pushed
the
bugfix/leaking-proxy
branch
from
b94add9 to
b9d84af
Compare
There was a problem hiding this comment.
I want to apologize, I dropped the ball on this PR. In exchange I rebased the branch onto main. The code change looks good to me
@sfc-gh-dszmolka do you have access to test this as a last sanity check before merging?
I'd especially like to know that this works with the SOCKS5 change introduced in #1398. So that we'd be sure that my rebase was good!
src/snowflake/connector/proxy.py
Outdated
| auth = ( | ||
| f"{proxy_user or ''}:{proxy_password or ''}@" | ||
| if proxy_user or proxy_password | ||
| else "" | ||
| ) |
There was a problem hiding this comment.
Instead of doing proxy_user or '' and proxy_password or '' I'd personally prefer to set the default values of the function to those. That'd end up changing this code to:
auth = (
f"{proxy_user}:{proxy_password}@"
if proxy_user or proxy_password
else ""
)
I did get caught out by all those ORs, but it doesn't actually change what the code does.
This is more of my nitpick
|
@sfc-gh-aling could you please take this and get it merged once we can sanity check correctness? I did add everything that is new since the PR was opened and I know what they are |
|
@sfc-gh-mkeller @sfc-gh-aling i'm trying to sanity-test this manually , hope this is something you had in mind. test script: # cat PythonConnector1398.py
import snowflake.connector
import os
print(f'Starting test. Proxy envvars: HTTP_PROXY={os.environ.get("HTTP_PROXY")}, HTTPS_PROXY={os.environ.get("HTTPS_PROXY")}')
proxies_to_test = ["http://my.pro.xy", "https://s.my.pro.xy", "socks5://localhost"]
for proxy_to_test in proxies_to_test:
snowflake_config = {
"account": os.environ.get("SFACCOUNT"),
"user": os.environ.get("SFUSER"),
"password": os.environ.get("SFPASS"),
"proxy_host": proxy_to_test,
"proxy_port": "8080",
"no_retry": True,
}
print(f'=> Testing with {proxy_to_test}.')
print(f'Before attempting to connect to Snowflake - HTTP_PROXY={os.environ.get("HTTP_PROXY")}, HTTPS_PROXY={os.environ.get("HTTPS_PROXY")}')
try:
# no such proxies so this will fail
conn = snowflake.connector.connect(**snowflake_config)
conn.close()
except:
print(f"Could not connect to Snowflake.")
print(f'After attempting to connect to Snowflake - HTTP_PROXY={os.environ.get("HTTP_PROXY")}, HTTPS_PROXY={os.environ.get("HTTPS_PROXY")}\n')latest release PythonConnector 3.12.1 which doesn't have this PR(also observe how the this PRLooks good at the first glance. Do you need me to test anything else, or did i miss something perhaps? I would also like to add that even with current-version 3.12.1, subsequent runs of the test script shows proxies as edit: adding an existing proxy (listening on http) to the test shows that the connector installed from the PR correctly passes the request to the proxy without changing the envvar value |
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-694457: Connector is leaking HTTP(S)_PROXY environment variables #1329
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Connector is overwriting the HTTP_PROXY and HTTPS_PROXY environment variables. I think this is a big problem, the client should not overwrite any environment variable, this has caused a bunch of very hard to debug problems for me at work. Since snowflake uses the vendored requests library to send the query requests, why not just pass in the proxies to that instead of changing the environment variables? If there is a strong reason for changing the variables, I think we should do something like this:
This will change the environment variables only for the scope of the contextmanager and revert the variables back to their original values.