Skip to content
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.

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

Fix SSL tunnel socket #157

Merged
merged 2 commits into from
Sep 11, 2014
Merged

Fix SSL tunnel socket #157

merged 2 commits into from
Sep 11, 2014

Conversation

wiggin15
Copy link
Contributor

@wiggin15 wiggin15 commented Sep 9, 2014

Maybe I'm not using the SSLTunnelConnection correctly but this looks like an internal problem - without this change, I'm getting the following error:

pyVmomi.VmomiSupport.HostConnectFault: (vim.fault.HostConnectFault) {
   dynamicType = <unset>,
   dynamicProperty = (vmodl.DynamicProperty) [],
   msg = 'must be _socket.socket, not socket',
   faultCause = <unset>,
   faultMessage = (vmodl.LocalizableMessage) []
}

@hartsock
Copy link
Member

hartsock commented Sep 9, 2014

Could we get an issue for this with steps to reproduce?

@hartsock hartsock self-assigned this Sep 9, 2014
@hartsock hartsock modified the milestone: pyVmomi 5.5.0-2014.2 Sep 9, 2014
@wiggin15
Copy link
Contributor Author

Turns out that this happens because we use gevent's patching.
I will open a ticket with code that reproduces this.

@wiggin15
Copy link
Contributor Author

Updated pull request

resp.fp is a socket._fileobject instead of a socket.socket object,
so it is incompatible with gevent.
Fix issue vmware#163
@wiggin15 wiggin15 mentioned this pull request Sep 11, 2014
@wiggin15 wiggin15 force-pushed the pull_request_2 branch 7 times, most recently from c2e0e83 to 0461e6b Compare September 11, 2014 10:10
@wiggin15
Copy link
Contributor Author

I added a test (without gevent for the moment). The test is a bit tricky because a) vcrpy was not built to be wrapped by ssl so I had to patch it using the existing monkey_patch_vcrpy function, and b) I had to call SoapStubAdapter directly - see comment in #156.
Let me know what you think.

@hartsock
Copy link
Member

Pulled locally for review.

hartsock added a commit that referenced this pull request Sep 11, 2014
@hartsock hartsock merged commit 13a41e2 into vmware:master Sep 11, 2014
@hartsock hartsock added bug and removed needs test labels Sep 11, 2014
@wiggin15 wiggin15 deleted the pull_request_2 branch October 21, 2015 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants