mcassaniti's proxy support

[badcure]

Merging #196 with the latest from master.

From the original PR:

This patch adds support for setting the proxy via a string. It also adds support for ignoring any environment variables that may set proxies.

This patch supersedes PR #165 and PR #151, but should implement what was requested in PR #165.

Example HTTP proxy strings are http://server:port and http://user:pass@server:port. An example SOCKS5 proxy string is socks5://user:pass@server:port.

[coveralls]

Coverage increased (+0.8%) to 73.23% when pulling 3c7ae55 on badcure:mcassaniti-proxy_v2 into e7a8502 on diyan:master.

[badcure]

@nitzmahone Let me know what you think, I went ahead and created this in place of the #196 proxy PR.

[nitzmahone]

IIRC, session.trust_env applies to everything; anything that unsets it this way will prevent all the env stuff from working, not just proxies (eg CA cert bundle location, auth settings, etc). proxy_ignore_env probably needs to be a more global config_ignore_env unless you're going to re-create what requests is pulling from the env and pass it in here. It's been a long time since I wrote that code though, so I don't remember exactly how requests manages that for a session (had to go spelunking in the requests code to figure it out).

[nitzmahone]

Also might be good to add a FEATURE_PROXY_SUPPORT = True to __init__.py - was trying to make it so things don't need to use inspect to feature sniff.

[jborean93]

I’m not a fan of the polymorphic proxy option. Prefer the original implementation where proxy is either None (environment - default) as a string the indicate the proxy host and there’s a separate kwarg to ignore the proxy that is True/False. It also makes it easier for a system like Ansible to try and cast the ignore kwarg as a bool where now we would have to check if it is False before trying to cast it as a book.

[badcure]

Would you be opposed to using a keyword instead, such as disabled ?

The reason is I was hoping pywinrm would be more of a "translator" vs a "pass through" to requests if that makes sense. The way I see it, there would be only three scenarios that others will want in priority:

• Use proxies that are provided
• Use the environment variables, if any(default)
• Disable proxies completely.

I also think will help keep the initialization parameters a little clearer overall in the long run.

[badcure]

I am just thinking out loud here, another option could be to disable the environment variable usage completely and let the application's responsibility pull the environment variable.

At least for my usage, being part of an internal environment...I would almost never want the environment variables to be used for winrm because I will be connecting to internal servers directly. Yet, I may have it configured for requests because I am hitting other APIs that are external.

I guess the pain of troubleshooting/discovering connection problems due to a proxy set would be higher than having to explicitly set it IMO. I would rarely think of pywinrm as a wrapper to requests.

[jborean93]

I still don't think having a separate kwarg is a bad idea, it keeps things distinct and easy to understand for users that this is a string and that is a bool. It doesn't necessarily have to be a wrapper for requests, there are a lot of weird things they do there and we are giving people a nice and easy way of specifying a proxy, no proxy, or the default (env vars).

I am just thinking out loud here, another option could be to disable the environment variable usage completely and let the application's responsibility pull the environment variable.

That would be a breaking change which we wouldn't want to have. People right now would rely on the environment vars to set a proxy and removing that ability will break their scripts/setups making unhappy users.

[badcure]

So I guess I would rather hold off on this. With proper versioning and change log documentation, we should not be worried about breaking existing behavior if it makes sense. I have a feeling not too many people are using proxies as it is.

Overall, I would like to start thinking about features as if it was merged, should it fit in the next minor version bump, or next major version bump? If it is a hard no, then which direction do you see pywinrm going overall?

At the very least, I do see it as every applications responsibly to pin the version or version ranges...

[jborean93]

With proper versioning and change log documentation, we should not be worried about breaking existing behavior if it makes sense.

This could be ok if Ansible has a ceiling requirement for pywinrm but it doesn't. There just hasn't been a need for this previously as we've been operating under the assumption that pywinrm won't be including any breaking changes. Alongside this a lot of our docs just say to install pywinrm with pip install pywinrm or <insert package manager> install python-winrm.

Another problem is the hundreds of blog posts and docs people distribute online where they just say to install pywinrm without any version identifiers. We can try and mitigate this in the future by updating our docs to include max version as well as an official extra_requires in our setup.py but this won't solve;

• Content that isn't controlled by us having outdated install information
• Distro packages being distributed across a wide range of versions that may or may not reflect what's available on pypi. This is really tricky to document leading to more complexity for users
• Takes time for our releases to reach saturation among users for the new extra_requires to really apply and we would need to be mindful of this
• We now have a fragmentation between what Ansible supports and what pywinrm is at. Ultimately we want to try and remove this fragmentation but depending on the impact of the breaking change that may not be feasible.

While we are not the sole users of pywinrm and we do need to think of outside users, we do need to keep in mind that Ansible has a sizeable audience and making breaking changes is not in anyones best interest. If a breaking change needs to occur it is a lot better to implement the change behind some feature flag, or deprecation warning, and add support for that flag downstream. Once time has passed for that new version to become more prevalent then we can look at removing the old behaviour.

In the case for this PR, proxy usage isn't an extremely common behaviour but it is definitely something I've seen people use, even more so in a enterprise environment. Currently the only way to manipulate that is through environment variables so if we were take that away then their scripts will break.

[nitzmahone]

@badcure not sure what your prior experience has been in maintaining widely-used open source projects, but breaking changes are the bane of a maintainer's existence. "But we put it in the changelog" doesn't do much to assuage the pitchfork-wielding masses when they've descended on your doorstep because you broke their world.

Yes, you're right that it's a good practice for users to pin versions, but you'll find that it doesn't happen very often, especially when people are solving one-off problems, then moving on. In our case, pywinrm isn't a hard dependency for Ansible, so there might be any number of ways users get it (OS package, direct pip install, etc). We've talked about adding an extras_require entry for it in Ansible so we could pin to a known-good version (at least when Ansible was installed via pip), but there hasn't been a need to-date, and it only solves a small part of the larger problem.

Just trying to save you the hassle of finding out the hard way that making breaking changes to a heavily-used project usually has demoralizing results. If you really want to push that, a long-ish deprecation warning cycle on the behavior that's changing is in order, to allow at least some subset of users that pay attention to such things to adjust, and it's also best to batch up those deprecations in a group if there are other things coming, so they're not having to constantly adjust. Regardless, anytime you do that, expect to play "whack a mole" with people that don't bother to read changelogs/deprecations filing issues for months/years to come.

[badcure]

I just want to start off, I am NOT saying that we simply add breaking changes quickly. I understand there will be time needed, but we should at the very least start talking about it. It is hard not to appear that way when the responses are more commonly around if it will break, and not the direction that makes sense for pywinrm long term.

I completely understand that there are certain applications to where not breaking existing behavior is crutial, such as in Linux: Don't break user space This makes sense, people look to it for its stability. As long as it is stable, they will then look for features to expand.

pywinrm, IMHO, is not that. It is a library(or module) for others to acheive their own goal with their own application. With the current state, I do think we need to start thinking about long term design. The more we add/merge things in a way that doesn't break, the more tech debt we will have in the long run.

With the tools at our disposal such as how semantic versioning is suppose to work, the ability to use version specifiers, change logs, AND the use of DepricatedWarning as mentioned here ... I do think it is very doable to start introducing breaking changes for the benefit of this library. Also in a way that doesn't appear barbaric even if it breaks something.

Just as a little background, I do manage an application with over 100 libraries, and have been on the receiving end of major version changes before. Within the context of pywinrm, I have always seen it more akin to paramiko, impacket, etc. The main reason I am suggesting these things is because I see it more of a connection library than a requests wrapper. Personally, mixing configuration for requests used for our API calls with pywinrm behavior is scary to me. Also providing pywinrm with two options for proxy because of reqeusts allows for it is a little scary as well, it implies that the pywinrm design is tied to requests==2.* and any major upgrade has a potential of wreaking havok on the initilization args. As a high level next major release goal, let me know if you disagree.

Back to your question, with an emphatic yes, I have no problem working with and helping others than eventually find issues. IMHO starting down the path of implementing things properly in a proper and timely fasion is more important than continuing to not break existing behavior.

[nitzmahone]

Totally agree on hiding requests implementation details where possible- this library started out on urllib2- I switched it over to requests so we could take advantage of the easier extensibility, but requests is also not without its limitations...

Back to the case at hand- if you want to proceed in the direction of removing trust_env support completely, and you're down for doing it with deprecation warnings, how about something like the following:

Default proxy and ca_trust_path args to a sentinel value like 'legacy_requests'. In this mode, it enables trust_env and calls requests' merge to get old values. If differing values come in from requests, fire a deprecation warning (so we're only firing it on systems that have the requests envvars set). For ca_trust_path, passing None uses the internal validation behavior (and subject to the server_cert_validation mode), anything else is assumed to be a path to a .pem cert file. For proxy, passing None completely disables proxy support, any other value is interpreted as a verbatim proxy URL. Again, the calling application gets to decide, eg, whether to set an http or https proxy based on whatever logic it feels like implementing. The only case I could see this breaking on is a server redirect that switches protocols, but IIRC there are other things that prevent that from working anyway.

Thoughts?

[badcure]

Yes, actually thinking about that a little: +1. With the deprecation warning for now, that can potentially notify the users of any unexpected behavior in the logs.

I will update it with the changes tomorrow.

[badcure]

Let me know what you think, it should function the way you described. I also limited the deprecated warning message to be displayed once, so it wouldn't spam an application's logs.

Note: I just want to make sure this proxy implementation looks good and there are no concerns. If everything is fine, I will create a separate PR to address ca_trust_path as well to include similar behavior.