Add no_proxy to index searches

[minsis]

Adding no_proxy option for index searches.

Fixes #5497

[CLAassistant]

All committers have signed the CLA.

[arm4b]

Thanks!

Can you also add the Changelog record for this enhancement?

[minsis]

@armab Updated

[arm4b]

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

[minsis]

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.

[minsis]

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

[arm4b]

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.

[minsis]

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.

[minsis]

@armab I've done another commit to do similar to what the bug fix is supposed to provide from the requests library.

Could you also do me a favor and just double check my upstream merge. I get confused on dealing with merge conflicts and such.

[minsis]

@armab I also tested this and it works now.

[arm4b]

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?

[minsis]

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.

[arm4b]

I see now, thanks for more context 👍

[arm4b]

Thanks for the contribution 👍
The change is marked for the upcoming v3.7.0 release.

Before that, if you can add a unit test for the new logic in packs.py, - that would be helpful as proxy behavior is highly inconsistent between the different tools.

[minsis]

I thought about writing a test earlier, but I'm not sure how I would write a test for this. I'm still learning on writing good unit and/or integration testing. This one is particular difficult in my mind because of the looping mechanism. Any suggestions?

[cognifloyd]

Tests for this would be awesome. However, there are no unit tests for things that retrieve the pack index. Writing tests for all of these index functions would be much more involved than merely a test for the proxy config.

I'm going to merge this now. I'm satisfied with the manual testing for this fix.