support CONNECT based proxies #799

goneri · 2019-06-05T18:17:01Z

ensure the proxy configuration is passed to __GetElementTree()
ruse @blacksponge fix for the SSLTunnelConnection class
remove test_ssl_tunnel_http_failure because the error will happen later

closes: #620
closes: #567
closes: #198

vmwclabot · 2019-06-05T18:17:16Z

@goneri, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

- simplify the way we pass the proxy paramerer with just two new parameters. - simplify a bit the way we format the error message - ensure `vmware_host` can also collaborate with a proxy This PR depends on vmware/pyvmomi#799.

goneri · 2019-06-06T15:26:46Z

@tianhao64, @pgbidkar, may I ask you to take a look at this PR? :-)

vmwclabot · 2019-06-06T21:32:27Z

@goneri, VMware has approved your signed contributor license agreement.

pyVmomi/SoapAdapter.py

- simplify the way we pass the proxy paramerer with just two new parameters. - simplify a bit the way we format the error message - ensure `vmware_host` can also collaborate with a proxy This PR depends on vmware/pyvmomi#799.

goneri · 2019-06-12T17:22:49Z

Could you please take a look at the PR, we would like to land in Ansible the HTTP proxy support for the VMware modules ( ansible/ansible#52936 ), and this is a blocker for us.

pgbidkar · 2019-06-13T03:07:26Z

pyVmomi/SoapAdapter.py

+   def __call__(self, path, *args, **kwargs):
+      conn = http_client.HTTPSConnection(path, **kwargs)
+      conn.set_tunnel(self.proxyPath)
+      return conn


Hi @goneri , I am still not convinced about SSLTunnel change. Any change to SSLTunnel needs to test thoroughly. This might break existing users. We have experienced this internally.
Please can you explain a bit about How Cert/Key validation happens with your change?
Provide more details about testing performed for this change.
I will also do test internally.
Thanks

The proxy support is currently not working, so I'm not sure how this patch can break existing users.
I didn't implement the cert/key validation because we don't use it and I don't have good way to test it.

cert Key Validation is required here.

retval.sock = _SocketWrapper(tunnel.sock, keyfile=key_file, certfile=cert_file)

Please can you confirm if your change handles this? I was out of office for 2 days. I will also test in my test env and confirm
Thanks

Hi @pgbidkar, do you have any update on this?

Hi, We are testing your change, but test results doesn't look promising. Trying out different scenarios. I will update the detailed findings by tomorrow EOD.

Thanks

Why do you just pass a path (/sdkTunnel) and not the hostname of the proxy server?

HTTPConnection() exepects a hostname, see: https://docs.python.org/3/library/http.client.html

What's the purpose of sslProxyPath? From my understanding of https://github.com/vmware/pyvmomi/blob/master/pyVmomi/SoapAdapter.py#L1240-L1247 I initially thought it was an alternative way to write the host:port but it seems to be something different.

Why do you just pass a path (/sdkTunnel) and not the hostname of the proxy server?

HTTPConnection() exepects a hostname, see: https://docs.python.org/3/library/http.client.html

/sdkTunnel is tunnel proxy inbuilt in vCenter. I didnt had actuall proxy server setup, hence using /sdkTunnel

What's the purpose of sslProxyPath? From my understanding of https://github.com/vmware/pyvmomi/blob/master/pyVmomi/SoapAdapter.py#L1240-L1247 I initially thought it was an alternative way to write the host:port but it seems to be something different.

'url' is the argument alternative way of sending hostname:port not sslProxyPath. It is proxy path to connect to server. It depends on the way vcenter server setup is done in your infra.

Thank you @pgbidkar for your answer. That was really helpful! I just pushed an updated version of my patch. I now add a new HTTPProxyConnection class, this way, I don't touch the SSLTunnelConnection class. Both are really difference, and I think it's a better approach.

To summarize, if the user set sslProxyPath, we use the original method, if instead httpProxyHost is set, we use the new class. Both should not be used at the same time.

goneri · 2019-06-19T23:14:16Z

tests/test_connect.py

-            connect.SoapStubAdapter('vcsa', 80, httpProxyHost='vcsa').GetConnection()
-        self.assertRaises(http_client.HTTPException, should_fail)
+    @patch('six.moves.http_client.HTTPSConnection')
+    def test_ssl_tunnelwith_cert_file(self, hs):


@pgbidkar I added this test to validate the parameter passing of cert_file and key_file.

- simplify the way we pass the proxy paramerer with just two new parameters. - simplify a bit the way we format the error message - ensure `vmware_host` can also collaborate with a proxy This PR depends on vmware/pyvmomi#799.

Akasurde · 2019-06-25T14:16:28Z

@pgbidkar Could you please confirm if the latest changes solves your comments ? Thanks.

goneri · 2019-06-27T13:57:59Z

@pgbidkar follow-up to #799 (comment), could you retry to run your functional tests?

pgbidkar · 2019-06-27T15:00:23Z

@pgbidkar follow-up to #799 (comment), could you retry to run your functional tests?

Hi @goneri , Sure. checks are failed for your last commit. Please can you check and fix?
Thanks

goneri · 2019-06-27T15:19:50Z

@pgbidkar follow-up to #799 (comment), could you retry to run your functional tests?

Hi @goneri , Sure. checks are failed for your last commit. Please can you check and fix?
Thanks

Oops, I forgot to git add the fixture files.

pyVmomi/SoapAdapter.py

- ensure the proxy configuration is passed to `__GetElementTree()` - reuse @blacksponge fix for the `SSLTunnelConnection` class - add `test_ssl_tunnelwith_cert_file` test to validate that `cert_file` and `key_file` are passed as expected. The patch adds a new `HTTPProxyConnection` class to handle the `CONNECT` based proxies. The patch does not touch the `SSLTunnelConnection` class. Both are really difference, and I think it's a better approach. To summarize, if the user set `sslProxyPath`, we use the original method, if instead `httpProxyHost` is set, we use the new class. Both should not be used at the same time. closes: vmware#620 closes: vmware#567 closes: vmware#198

goneri · 2019-07-09T10:24:16Z

Thanks @pgbidkar for the feedback. I updated my patch. Could you retry?

pgbidkar · 2019-07-11T11:05:18Z

Thanks @pgbidkar for the feedback. I updated my patch. Could you retry?

Hi @goneri Thanks for the update. Functional tests are passing now. Doing some more testing before we ship it. Will keep you posted
Thanks

goneri · 2019-07-11T11:41:39Z

Great news :-), thanks for your time @pgbidkar. Could you include the patch in a new release?

pyVmomi/SoapAdapter.py

goneri · 2019-07-22T06:36:52Z

@pgbidkar, any news on this?

pgbidkar · 2019-07-22T08:59:57Z

@pgbidkar, any news on this?

Hi @goneri , Sorry for the delay. I was out off office due to bad health. I will merge it soon.
Thanks for the patient

Pavan

pgbidkar · 2019-07-23T09:07:56Z

Hi @goneri , Thanks for contributing 👍

goneri · 2019-07-23T10:07:58Z

Awesome, thank you @pgbidkar for your help during the review process 👍. Do you know when the next release will be published?

goneri mentioned this pull request Jun 5, 2019

vmware: refresh the proxy support Akasurde/ansible#6

Merged

goneri mentioned this pull request Jun 6, 2019

VMware: add support for HTTP proxy in connection API ansible/ansible#52936

Merged

pgbidkar reviewed Jun 7, 2019

View reviewed changes

pyVmomi/SoapAdapter.py Outdated Show resolved Hide resolved

goneri force-pushed the fix_proxy branch from e54ac87 to b82dca3 Compare June 7, 2019 15:10

goneri changed the title ~~fix pvmomi the proxy support~~ fix proxy support Jun 7, 2019

pgbidkar reviewed Jun 13, 2019

View reviewed changes

goneri force-pushed the fix_proxy branch from b82dca3 to aef6121 Compare June 19, 2019 23:12

goneri commented Jun 19, 2019

View reviewed changes

This was referenced Jun 20, 2019

VMware: Proxy support for vmware connection ansible/ansible#42221

Closed

VMWare - Rename _facts to _info ansible/ansible#57474

Merged

goneri force-pushed the fix_proxy branch 2 times, most recently from 802eed6 to 48eda76 Compare June 26, 2019 20:46

goneri changed the title ~~fix proxy support~~ support CONNECT based proxies Jun 26, 2019

goneri force-pushed the fix_proxy branch from 48eda76 to 6c9f6f4 Compare June 27, 2019 15:19

pgbidkar suggested changes Jul 3, 2019

View reviewed changes

pyVmomi/SoapAdapter.py Outdated Show resolved Hide resolved

goneri force-pushed the fix_proxy branch from 6c9f6f4 to 52da80c Compare July 9, 2019 10:23

pgbidkar reviewed Jul 11, 2019

View reviewed changes

pyVmomi/SoapAdapter.py Show resolved Hide resolved

pgbidkar merged commit 8c062b3 into vmware:master Jul 23, 2019

goneri deleted the fix_proxy branch July 26, 2019 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support CONNECT based proxies #799

support CONNECT based proxies #799

goneri commented Jun 5, 2019 •

edited

Loading

vmwclabot commented Jun 5, 2019

goneri commented Jun 6, 2019

vmwclabot commented Jun 6, 2019

goneri commented Jun 12, 2019

pgbidkar Jun 13, 2019

goneri Jun 13, 2019

pgbidkar Jun 18, 2019

goneri Jun 25, 2019

pgbidkar Jun 25, 2019 •

edited

Loading

goneri Jun 25, 2019

goneri Jun 25, 2019

pgbidkar Jun 26, 2019

pgbidkar Jun 26, 2019

goneri Jun 26, 2019

goneri Jun 19, 2019

Akasurde commented Jun 25, 2019

goneri commented Jun 27, 2019

pgbidkar commented Jun 27, 2019

goneri commented Jun 27, 2019

goneri commented Jul 9, 2019

pgbidkar commented Jul 11, 2019

goneri commented Jul 11, 2019

goneri commented Jul 22, 2019

pgbidkar commented Jul 22, 2019

pgbidkar commented Jul 23, 2019

goneri commented Jul 23, 2019

support CONNECT based proxies #799

support CONNECT based proxies #799

Conversation

goneri commented Jun 5, 2019 • edited Loading

vmwclabot commented Jun 5, 2019

goneri commented Jun 6, 2019

vmwclabot commented Jun 6, 2019

goneri commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgbidkar Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akasurde commented Jun 25, 2019

goneri commented Jun 27, 2019

pgbidkar commented Jun 27, 2019

goneri commented Jun 27, 2019

goneri commented Jul 9, 2019

pgbidkar commented Jul 11, 2019

goneri commented Jul 11, 2019

goneri commented Jul 22, 2019

pgbidkar commented Jul 22, 2019

pgbidkar commented Jul 23, 2019

goneri commented Jul 23, 2019

goneri commented Jun 5, 2019 •

edited

Loading

pgbidkar Jun 25, 2019 •

edited

Loading