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

MSYS-798 - Fixes for windows administrator password #524

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

dheerajd-msys
Copy link
Contributor

@dheerajd-msys dheerajd-msys commented Apr 20, 2018

Description:
While Bootstrapping an Amazon node with chef on Windows over SSL --winrm-transport ssl fails because we check created instance id which did not exist at that time for administrator user password.

Solution:
Updated code so that until instance is created we won't call check_windows_password_available method for Administrator's password.

Command:

knife ec2 server create --node-name excel-dev-ssl --ebs-size 128 --flavor t2.micro --region us-east-1 --subnet subnet-xxxxxx --image ami-ac8156d1 --security-group-id sg-xxxxxx --security-group-id sg-xxxxxxxx --security-group-id sg-xxxxxx -x 'Administrator' --ssh-key aws-key  --user-data bootstrapwinchefnodeamazon3.ps1 --winrm-transport ssl --winrm-ssl-verify-mode verify_none –p 5986 -VV

Issue fixed:
Bootstrap fails when using --winrm-transport ssl --winrm-ssl-verify-mode verify_none.
Ref: #521

Fixes #521

Signed-off-by: dheerajd-msys [email protected]

@@ -1537,12 +1537,13 @@ def windows_password
if not locate_config_value(:winrm_password)
if locate_config_value(:identity_file)
print "\n#{ui.color("Waiting for Windows Admin password to be available", :magenta)}"
print(".") until check_windows_password_available(@server.id) {
print(".\n") unless @server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there supposed to be a loop here? Before we were calling check_windows_password_available which would sleep for 10 seconds and try the password, and because that was an until we would keep doing it.

Now there's just a single unless on a print, which isn't useful. If we need to not look for the password until @server is valid, I would expect a return false unless @server in check_windows_password_available after the sleep 10.

Signed-off-by: dheerajd-msys <[email protected]>
response = connection.get_password_data(@server.id)
data = File.read(locate_config_value(:identity_file))
config[:winrm_password] = decrypt_admin_password(response.body["passwordData"], data)
if @server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the server isn't available yet for some reason, will we never try to get the windows password?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you basically want a

until @server { ....}
until windows_password_available { ... } 

Copy link

@Vasu1105 Vasu1105 Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to confirm this do we required this line to set user data for Administrator since ec2 creates default user as Administrator and it generates its own password. So our main problem is here we unnecessarily as far as I understand we don't need to add that net user command for Administrator here https:/MsysTechnologiesllc/knife-ec2/blob/c3371c98ce2407bb09512f8d497498891e55dee3/lib/chef/knife/ec2_server_create.rb#L1061

This particular line is causing issue where it calls windows_password method before instance is getting created.

So we should not call above lines of code for Administrator that will automatically resolve the problem for user.

@btm to answer to your question server gets available after create and when we call bootstrap_for_window_node we get windows_password since we verify the winrm connection is successful and then call bootstrap https:/MsysTechnologiesllc/knife-ec2/blob/c3371c98ce2407bb09512f8d497498891e55dee3/lib/chef/knife/ec2_server_create.rb#L821

I have checked with until loop until loop goes in infinite loop for @server check since here create_server_def has call to ssl_config_user_data which calls for windows_password method. This call is before we create the server which will always create problem in case of winrm_user as Administrator and if winrm_password is not provided.

So my suggestion will be just verify do we really need this lines for Administrator user to set user data
https:/MsysTechnologiesllc/knife-ec2/blob/c3371c98ce2407bb09512f8d497498891e55dee3/lib/chef/knife/ec2_server_create.rb#L1061:L1062

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some more points to @Vasu1105 comment:
For Administrator user and chef on Windows over SSL

While creating server here https:/chef/knife-ec2/blob/master/lib/chef/knife/ec2_server_create.rb#L534 call goes to https:/chef/knife-ec2/blob/master/lib/chef/knife/ec2_server_create.rb#L1179 and subsequently to ssl_config_user_data and satisfying all the conditions calls windows_password here
https:/chef/knife-ec2/blob/master/lib/chef/knife/ec2_server_create.rb#L1061. But since server is not created yet, password could not be fetched from https:/chef/knife-ec2/blob/master/lib/chef/knife/ec2_server_create.rb#L1540.

So we need to check for the server availability with if @server. until @server takes this to infinite loop.
Also in the first turn it goes to the else part here https:/chef/knife-ec2/pull/524/files#diff-e2993b8367db674f58a052036f00052aR1546 and fetches the primary details of server for creating server.

And in second try it satisfy the condition of if @server and make the password available from https:/chef/knife-ec2/pull/524/files#diff-e2993b8367db674f58a052036f00052aR1539

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I obviously haven't found the time to test this myself, so I'll trust your testing and stop holding this up.

@btm btm merged commit 61c7bab into chef:master Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Boostrap fails when using --winrm-transport ssl --winrm-ssl-verify-mode verify_none
3 participants