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

Added validator less bootstrap support for knife bootstrap windows winrm #224

Conversation

Vasu1105
Copy link

@Vasu1105 Vasu1105 commented May 7, 2015

  1. This change needs chef >= 12.2
  2. --node-name option needs to be passed while bootstrap
  3. Specs pending

@btm
Copy link
Contributor

btm commented May 13, 2015

@Vasu1105, have you looked at the failing tests?

Fixes #222

@Vasu1105
Copy link
Author

@btm Yes I have fixed test cases for Chef >= 12.2. The test cases failing for chef 11 and I am currently looking into to fix those.

@btm
Copy link
Contributor

btm commented May 18, 2015

This change needs chef >= 12.2

Do you mean you need Chef >= 12.2 for this feature to work?

I presume you don't mean that this breaks knife-windows for anyone on Chef < 12.2?

--node-name option needs to be passed while bootstrap

Please incorporate chef/chef#3325 to require a node be set when using this option, otherwise you see confusing output like this:

Node  exists, overwrite it? (Y/N) n
You said no, so I'm done here.

Specs pending

It looks like you've fixed these.

winrm_bootstrapper.define_singleton_method(:client_builder){nil}

allow(winrm_bootstrapper.client_builder).to receive(:run)
allow(winrm_bootstrapper.client_builder).to receive(:client_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need a note in here about how this is because Chef 11 doesn't have Chef::Knife::Bootstrap.client_builder.

If we're running against chef 12 (reverting 7aab00d), we can just say:

winrm_bootstrapper.client_builder = instance_double("Chef::Knife::Bootstrap::ClientBuilder", :run => nil, :client_path => nil)

I think we need to modify travis to test both chef11 + chef12. I'm looking at that.

@Vasu1105
Copy link
Author

@btm

  1. Yes it won't break the knife-windows if chef version is < 12.2.
  2. I will be working on require --node-name change.
  3. winrm_bootstrapper.client_builder = instance_double("Chef::Knife::Bootstrap::ClientBuilder", :run => nil, :client_path => nil) is possible by using rspec filter where we need to add filter for chef 12 and chef 11
  4. We realized that validatorless bootstrap should not be allowed if --windows-authentication-protocol set to basic. So, we are thinking to add validation for windows-authentication-protocol and transport =ssl as well

@btm
Copy link
Contributor

btm commented May 19, 2015

@Vasu1105 can you rebase this against master so you pull in #229?

Then we could add some rspec filters similar to what's in Chef

def chef_11?
  Chef::VERSION.split('.').first.to_i == 11
end

def chef_12?
  Chef::VERSION.split('.').first.to_i == 12
end

Then I think we need to add code that raises an error if you don't have a validation.pem and you've got Chef 11, and a corresponding test.

@Vasu1105
Copy link
Author

@btm I am looking into all review comments and fixing them. I have rebased my branch.

ui.error("Validatorless bootstrap only supported with negotiate authentication protocol and ssl/plaintext transport")
exit 1
elsif !(Chef::Platform.windows?) && negotiate_auth?
ui.error("Negotiate protocol with plaintext transport is only supported when this tool is invoked from windows based system")
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this duplicate the same error being raised in Chef::Knife::WinrmCommandSharedFunctions.resolve_winrm_transport_options (in winrm_knife_base.rb)?

@btm
Copy link
Contributor

btm commented May 20, 2015

It'd be great if you could rebase this against master instead of merging and force push back onto this branch. I'm seeing some other merge commits leak in from the merge.

@Vasu1105 Vasu1105 force-pushed the vj/knife_windows_validatorless_bootstrap_support branch from 52b188d to c5771f9 Compare May 21, 2015 09:14
@Vasu1105 Vasu1105 force-pushed the vj/knife_windows_validatorless_bootstrap_support branch from c5771f9 to c73998c Compare May 21, 2015 09:16
@btm
Copy link
Contributor

btm commented May 28, 2015

Merged via #234

@btm btm closed this May 28, 2015
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.

5 participants