Skip to content

Add no_proxy to index searches #5500

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.

Sign up for GitHub

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

Merged
cognifloyd merged 14 commits into from
Mar 31, 2022
Merged

Add no_proxy to index searches #5500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b8c1253
Add no_proxy to index searches
Dec 13, 2021
f886761
Merge branch 'master' into issues-5497/proxy_for_index
minsis Dec 13, 2021
300f09a
Updated changelog
minsis Dec 16, 2021
b02db71
Merge remote-tracking branch 'origin/issues-5497/proxy_for_index' int…
minsis Dec 16, 2021
8ad5e5d
work around for requests bug
minsis Feb 5, 2022
3d4f538
merge from upstream
minsis Feb 5, 2022
b147c70
Merge branch 'master' into issues-5497/proxy_for_index
minsis Feb 5, 2022
6e76bb1
check if proxies are supposed to be used
minsis Feb 5, 2022
ae2dbc2
Fix finding key if missing
minsis Feb 5, 2022
a7afece
fix for linter
minsis Feb 5, 2022
80de473
fix for linter
minsis Feb 5, 2022
2b41eda
Merge branch 'master' into issues-5497/proxy_for_index
minsis Mar 7, 2022
224e0e2
Fix ssl verify for bypassing proxy
minsis Mar 7, 2022
9e975a9
Merge branch 'master' into issues-5497/proxy_for_index
cognifloyd Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ Fixed

Contributed by @khushboobhatia01

* Fixed issue where pack index searches are ignoring no_proxy #5497

Contributed by @minsis

3.6.0 - October 29, 2021
------------------------

Expand Down
18 changes: 17 additions & 1 deletion st2common/st2common/services/packs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import os
import requests
from requests.utils import should_bypass_proxies
import six
from six.moves import range
from oslo_config import cfg
Expand Down Expand Up @@ -83,6 +84,7 @@ def _fetch_and_compile_index(index_urls, logger=None, proxy_config=None):
if proxy_config:
https_proxy = proxy_config.get("https_proxy", None)
http_proxy = proxy_config.get("http_proxy", None)
no_proxy = proxy_config.get("no_proxy", None)
ca_bundle_path = proxy_config.get("proxy_ca_bundle_path", None)

if https_proxy:
Expand All @@ -92,7 +94,17 @@ def _fetch_and_compile_index(index_urls, logger=None, proxy_config=None):
if http_proxy:
proxies_dict["http"] = http_proxy

if no_proxy:
proxies_dict["no"] = no_proxy
Copy link
Member

@arm4b arm4b Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minsis Did you test it BTW and can confirm no_proxy setting works this way via no key?

Copy link
Contributor Author

@minsis minsis Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swear this was working, but I guess not.

I'm pretty sure the no key is correct. I looked in the requests library and when building the proxy info it does a split. Hence the reason the other kyes are http and https.

Looking at the requests docs they never mention no_proxy, but its mentioned in a release note saying its supposed to be no_proxy.

Regardless no and no_proxy doesn't even seem to work at all. I tested this directly using the requests library.

Copy link
Contributor Author

@minsis minsis Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

psf/requests#4871

Looks like maybe this has been broken for quite some time. There's a PR to fix it, but its still open for 15 months.

I suppose we could implement some similar logic as PR?
psf/requests@063f2ae

Copy link
Member

@arm4b arm4b Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swear this was working, but I guess not.

I'm just asking.

I'm good to merge it if you say if the code change was tested manually and works in your environment.
If you can confirm this is 👍 and fixes your case, - just let me know in the PR as the requests doc is a bit confusing indeed.

Copy link
Contributor Author

@minsis minsis Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add no or no_proxy doesn't work unfortunately. So I guess back to square one.

My thought now is to maybe use the same logic the PR is using. Basically if it detects the URL being used is in no_proxy it will remove the proxy info from the request.


for index_url in index_urls:

# TODO:
# Bug in requests doesn't bypass proxies, so we do it ourselves
# If this issue ever gets fixed then we can remove it
# https://github.com/psf/requests/issues/4871
bypass_proxy = should_bypass_proxies(index_url, proxies_dict.get("no"))

index_status = {
"url": index_url,
"packs": 0,
Expand All @@ -102,7 +114,11 @@ def _fetch_and_compile_index(index_urls, logger=None, proxy_config=None):
index_json = None

try:
request = requests.get(index_url, proxies=proxies_dict, verify=verify)
request = requests.get(
index_url,
proxies=proxies_dict if not bypass_proxy else None,
verify=verify if not bypass_proxy else True,
Copy link
Member

@arm4b arm4b Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
verify=verify if not bypass_proxy else True,
verify=verify,

I still don't understand why adjusting the verify logic is needed in this situation.
I thought we were trying to fix no_proxy.

Could you perhaps provide more context or an example?

Copy link
Contributor Author

@minsis minsis Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

st2/st2common/st2common/services/packs.py

Lines 88 to 92 in 224e0e2

ca_bundle_path = proxy_config.get("proxy_ca_bundle_path", None)
if https_proxy:
proxies_dict["https"] = https_proxy
verify = ca_bundle_path or True

Because verify is overwritten with the path to your proxy's ca bundle if proxy_ca_bundle_path environment variable is set. Since its outside the loop there could be a chance where you are specifying a ca bundle path but needing to bypass the proxy.

An example might be (without verify if not bypass_proxy else True,):

proxy_config = {
    "https_proxy": "http://proxy.company.local",
    "http_proxy":  "http://proxy.company.local",
    "no_proxy": "company.local,localhost,my.other.domain",
    "proxy_ca_bundle_path": "/path/to/proxy/ca_bundle"
}

proxies_dict = {
    "https": "http://proxy.company.local",
    "http":  "http://proxy.company.local",
    "no": "company.local,localhost,my.other.domain",
}

# verify is now overwritten from True to '/path/to/proxy/ca_bundle'
verify="/path/to/proxy/ca_bundle"

Now we enter loop:

# Iteration 1
verify="/path/to/proxy/ca_bundle" # permanently set

index_url="https://index.stackstorm.com"
bypass_proxy = False

requests.get(
    "https://index.stackstorm.com",
    proxies={
        "https": "http://proxy.company.local",
        "http":  "http://proxy.company.local",
        "no": "company.local,localhost,my.other.domain",
    },
    verify="/path/to/proxy/ca_bundle",
)
# Iteration 2
verify="/path/to/proxy/ca_bundle" # permanently set

index_url="https://index.mycompany.local"
bypass_proxy = True

requests.get(
    "https://index.mycompany.local",
    proxies=None
    verify="/path/to/proxy/ca_bundle",
)

So in Iteration 2 we would might end up failing because we're still trying to use our proxy's ca bundle, but not using the proxy.

Copy link
Member

@arm4b arm4b Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, thanks for more context 👍

)
request.raise_for_status()
index_json = request.json()
except ValueError as e:
Expand Down