Merge environment settings

[ricardobranco777]

This PR adds support for the REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE environment variables.

To achieve the verify=False functionality this can be done already without this patch:

import openqa_client.client
client = openqa_client.client.OpenQA_Client(server="https://openqa.suse.de", wait=5, retries=1)
client.session.verify = False

Fixes #15

[AdamWill]

Looking at the underlying requests code here, I think it's safer to pass cert and verify as None rather than explicitly True or False. Well, verify=True is probably OK, but I think cert=False might be unexpected.

[AdamWill]

In fact...wouldn't setting verify=True in this patch make it not possible to effectively do #15 (comment) any more?

[ricardobranco777]

Looking at the underlying requests code here, I think it's safer to pass cert and verify as None rather than explicitly True or False. Well, verify=True is probably OK, but I think cert=False might be unexpected.

I tried with both set to None and it still works. The only difference is that without the environment variable I get:

InsecureRequestWarning: Unverified HTTPS request is being made to host 'openqa.suse.de'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings
  warnings.warn(

[ricardobranco777]

In fact...wouldn't setting verify=True in this patch make it not possible to effectively do #15 (comment) any more?

No. Anyway I force-pushed a fix.

[AdamWill]

Looking at the underlying requests code here, I think it's safer to pass cert and verify as None rather than explicitly True or False. Well, verify=True is probably OK, but I think cert=False might be unexpected.

I tried with both set to None and it still works. The only difference is that without the environment variable I get:

InsecureRequestWarning: Unverified HTTPS request is being made to host 'openqa.suse.de'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings
  warnings.warn(

well, that doesn't sound good. I'll fiddle around with this a bit in a shell tomorrow before merging anything. It's annoying that upstream never actually documents its intentions or expectations for these values...

[ricardobranco777]

Looking at the underlying requests code here, I think it's safer to pass cert and verify as None rather than explicitly True or False. Well, verify=True is probably OK, but I think cert=False might be unexpected.

I tried with both set to None and it still works. The only difference is that without the environment variable I get:

InsecureRequestWarning: Unverified HTTPS request is being made to host 'openqa.suse.de'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings
  warnings.warn(

well, that doesn't sound good. I'll fiddle around with this a bit in a shell tomorrow before merging anything. It's annoying that upstream never actually documents its intentions or expectations for these values...

I do think that the warning is good, actually, and is the usual behaviour with requests where you can optionally mute them with:

import urllib3
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)

[AdamWill]

Oh. I thought you meant that with verify=None you get that warning just with normal requests, which would obviously be bad. If you mean that you get that warning only when you set client.session.verify = False, then yeah, that's all good.

Still, if I'm reading you right, the behaviour if we set verify = True in this patch sounds weird. You're saying that if we do that, then client.session.verify = False still somehow overrides it, but the warning doesn't get printed? That's...strange.

edit: see, on that path I'd expect us to wind up here, where it seems like the request setting - i.e. the one we set in this PR - should override the session setting...

[ricardobranco777]

Still, if I'm reading you right, the behaviour if we set verify = True in this patch sounds weird. You're saying that if we do that, then client.session.verify = False still somehow overrides it, but the warning doesn't get printed? That's...strange.

That's why verify=None is better. There are other oddities as well:

psf/requests#3829

[ricardobranco777]

edit: see, on that path I'd expect us to wind up here, where it seems like the request setting - i.e. the one we set in this PR - should override the session setting...

The dictionary part is for proxies when used like in the documentation:

settings = s.merge_environment_settings(prepped.url, {}, None, None, None)

But supporting proxy environment variables where credentials are passed is not secure.

[AdamWill]

edit: see, on that path I'd expect us to wind up here, where it seems like the request setting - i.e. the one we set in this PR - should override the session setting...

The dictionary part is for proxies when used like in the documentation:

settings = s.merge_environment_settings(prepped.url, {}, None, None, None)

But supporting proxy environment variables where credentials are passed is not secure.

yes, but I was linking to the part above this - since when we do verify = True it's not a dictionary, we should hit the bailout where request_setting wins, so I just don't understand how the session setting could win as you said it did.

But hey, if we all agree that None is the best option, it doesn't matter much :D

[ricardobranco777]

Can we push this to pypi?

[AdamWill]

yeah, sure, just yesterday was a holiday so I wanted to wait till today to see if there's anything else I need to do before making a release.

[AdamWill]

btw, what is this pcw thing? is it for running openqa workers in public clouds? cos I'm kinda interested in that...

[ricardobranco777]

btw, what is this pcw thing? is it for running openqa workers in public clouds? cos I'm kinda interested in that...

pcw lets us monitor resources in the cloud for automatic cleanup in case terraform destroy fails, etc. In this particular case we're interested in cancelled jobs to clean them up faster than the allowed TTL.

[AdamWill]

right, but I mean, what's the context? are you actually running openqa jobs in public cloud, or something else? I've been wanting to do that for a long time but never had the roundtuits...

[ricardobranco777]

right, but I mean, what's the context? are you actually running openqa jobs in public cloud, or something else? I've been wanting to do that for a long time but never had the roundtuits...

The jobs create public cloud instances to test SUSE images and workloads, yes.

[AdamWill]

ohhh, I see. so you have jobs running on your hardware openQA job host machines which are themselves creating cloud instances? Roger. I'm more interested in whether we can possibly replace our farms of hardware job hosts with cloud instances. thanks!

[AdamWill]

OK, release 4.2.2 sent to pypi.

[ricardobranco777]

OK, release 4.2.2 sent to pypi.

Thank you!