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

Fixed issues facing while bootstrapping the node using ssh config file. #11531

Conversation

sanga1794
Copy link
Contributor

@sanga1794 sanga1794 commented May 6, 2021

Signed-off-by: sanga17 sausekar@msystechnologies.com

Description

Fixed issues facing while bootstrapping the node using ssh config file.

Related Issue

Fixed: 1) #7053
2) https://github.com/chef/customer-bugs/issues/271

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@sanga1794 sanga1794 requested review from a team as code owners May 6, 2021 14:16
@sanga1794 sanga1794 force-pushed the SangmeshA/Fix_Knife_Boostrap_using_ssh_config_file branch from 59e8744 to 2725a35 Compare May 10, 2021 11:48
@sanga1794 sanga1794 changed the title WIP: Fixed issues facing while bootstrapping the node using ssh config file. Fixed issues facing while bootstrapping the node using ssh config file. May 10, 2021
@sanga1794
Copy link
Contributor Author

@tas50 and @lamont-granquist Please review this PR. Thanks!!


if config[:sudo]
# While using ssh config file user might present in ssh file
user = config[:user] || ssh_config_for_host(config[:host])[:user]
Copy link
Member

Choose a reason for hiding this comment

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

This is already available in @config[:user], which we should use instead of re-doing override parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcparadise, This change will resolve user nil(@config[:user]) issue when we have added user in ssh_config file instead of adding it in bootstrap command.

Copy link
Member

Choose a reason for hiding this comment

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

@config contains the merged results of all config, including the user's ssh config as provided by Net::SSH.config_for_host. You can see this in transport_config[1], where it also merges in missing_opts_from_ssh_config defined later in the same file. This merge currently accepts SSH_CONFIG_OVERRIDE_KEYS, which are user, port, and proxy.

[1] https://github.com/chef/chef/blob/master/knife/lib/chef/knife/bootstrap/train_connector.rb#L225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @marcparadise, will update according to it.

@@ -974,13 +974,17 @@ def ssh_identity_opts
# Reference: https://github.com/chef/chef/blob/master/lib/chef/knife/ssh.rb#L272
opts[:keys_only] = config.key?(:connection_password) == false
else
opts[:key_files] = []
opts[:key_files] = nil
Copy link
Member

Choose a reason for hiding this comment

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

What was this intended to fix?

In train, a value of nil for key_files will get converted to []. Given that, this change and the one below /should/ have a net of no effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That is used for train's CLI usage, IIRC; that is not the path we take.

Our path starts in TrainConnector.connection, which invokes Train.create[1] This returns a new instance of Train::Transports::SSH [2]. We then call instance.connection to retrieve the connection object. connection calls validate_options [3] which in turn initializes key_files as Array(options[:key_files])[4]. When key_files is nil, that will evaluate to [].

[1] https://github.com/chef/chef/blob/master/knife/lib/chef/knife/bootstrap/train_connector.rb#L54
[2] https://github.com/inspec/train/blob/master/lib/train.rb#L20
[3] https://github.com/inspec/train/blob/master/lib/train/transports/ssh.rb#L79.
[4] https://github.com/inspec/train/blob/master/lib/train/transports/ssh.rb#L109

Signed-off-by: sanga17 <sausekar@msystechnologies.com>
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
…ts_in missing user and port variables

Signed-off-by: sanga17 <sausekar@msystechnologies.com>
@sanga1794 sanga1794 force-pushed the SangmeshA/Fix_Knife_Boostrap_using_ssh_config_file branch from c3e5359 to c654e76 Compare July 2, 2021 14:21
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
@marcparadise marcparadise merged commit 6829304 into chef:master Jul 27, 2021
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.

None yet

2 participants