Repository navigation
Add windows ssh - #519
Add windows ssh#519Cristi1324 wants to merge 8 commits into
Conversation
77cd612 to
410335f
Compare
6b4eb45 to
3536313
Compare
fabi200123
left a comment
fabi200123
left a comment
There was a problem hiding this comment.
Would like someone else to take a look again over this! But besides the account_line = "net.exe user %s %s" % (username, password) LGTM
|
|
||
| @mock.patch.object(wsman.WSManConnection, "from_connection_info") | ||
| @mock.patch.object(windows_ssh.WindowsSSHConnection, "from_connection_info") | ||
| def test_from_connection_info_ssh(self, mock_ssh, mock_winrm): |
There was a problem hiding this comment.
I would also add a test that uses the key. From what I see you only have 1 that uses the password
| ) | ||
| account_line = "" | ||
| if username is not None: | ||
| account_line = "net.exe user %s %s" % (username, password) |
There was a problem hiding this comment.
Just for safety can you add a check before this that the password doesn't have any spaces in it? If by any chance a password like "hello there" gets sent to net.exe user <username> hello there it will fail the command.
There was a problem hiding this comment.
The password requirements are already handled by the providers. Most, if not all providers that use this script with password already generate or allow passwords that do not contain spaces.
| WINRM_HTTPS_PORT = 5986 | ||
|
|
||
|
|
||
| def uses_winrm(connection_info): |
There was a problem hiding this comment.
I think checking on an extra protocol key would be better than just checking on the port number. Then the providers can explicitly set the protocol when it supports/prefers it.
This method could've been simply return connection_info.get('protocol', WINRM_CONSTANT)
Then you won't have extra checks for protocol either in _protocol_from_connection_info for example.
Maybe even rename this method to get_protocol and then it's super clear what it should return and used for.
There was a problem hiding this comment.
Since this comment, a new core option [windows_minion] connection_type, has been added. This is used to decide if the provider should use winrm or ssh for windows worker. Currently it is set to ssh by default, but if minion_port() is not called by the provider, it'll still use winrm.
There was a problem hiding this comment.
The port no. has nothing to do with the protocol type. We shouldn't assume default ports are being used, I can literally set 22 to the winrm listener (or vice-versa) - and yes, I know thus far we assumed, but if we should plan on supporting custom port numbers, we should at least prepare this feature to be extensible in that direction. I think we should make the port customizable at some point, therefore we'll need 2 options. One for protocol, one for port, and we use default protocol ports in case it's not customized.
For this PR, we can go with only protocol and always use default ports. For future, I'd like to see customizable ports option.
| WINRM_HTTPS_PORT = 5986 | ||
|
|
||
|
|
||
| def uses_winrm(connection_info): |
There was a problem hiding this comment.
The port no. has nothing to do with the protocol type. We shouldn't assume default ports are being used, I can literally set 22 to the winrm listener (or vice-versa) - and yes, I know thus far we assumed, but if we should plan on supporting custom port numbers, we should at least prepare this feature to be extensible in that direction. I think we should make the port customizable at some point, therefore we'll need 2 options. One for protocol, one for port, and we use default protocol ports in case it's not customized.
For this PR, we can go with only protocol and always use default ports. For future, I'd like to see customizable ports option.
a494ffb to
8c81098
Compare
An unset port uses 22 for ssh and 5986 for winrm.
8c81098 to
be48300
Compare
Summary
[windows_minion] connection_typedefaults tossh.sshuses port 22.winrmuses port 5986.connection_info.poll_instance_until_reachable()follows that same port rule. SSH runsexit 0. WinRM runswhoami.Merge with: https://github.com/cloudbase/coriolis-docker/pull/11
Provider PRs are optional.