From 876e906de0c088e4dacfafb62770ec2daa3d3864 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Sat, 20 Mar 2021 15:38:02 +0000 Subject: [PATCH] Improve id_ssh_key_file finalizing (#1230) Move finalizing the id_ssh_key_file based on how other settings are currently defined to a separate private function and extend the tests to accept defining additional expects/allows within the table. This should apply a consistent set of rules where if the user explicitly supplies the key, it will attempt to resolve it based on the expected ssh directory, and will always retain the explicit setting even if it doesn't exist. Where connect_via_ssh is enabled, it will attempt to detect if the default key exists, otherwise it will disable the setting. If the user does not want automatic guesses, they can explicitly disable by setting it to `nil`. Fixes: #1228 --- lib/vagrant-libvirt/config.rb | 45 ++++++++++--- spec/unit/config_spec.rb | 122 +++++++++++++++++++++++++--------- vagrant-libvirt.gemspec | 1 + 3 files changed, 127 insertions(+), 41 deletions(-) diff --git a/lib/vagrant-libvirt/config.rb b/lib/vagrant-libvirt/config.rb index 479e8dd..c19dcd4 100644 --- a/lib/vagrant-libvirt/config.rb +++ b/lib/vagrant-libvirt/config.rb @@ -714,22 +714,15 @@ module VagrantPlugins params = {} if @connect_via_ssh + finalize_id_ssh_key_file + uri << '+ssh://' uri << @username + '@' if @username uri << ( @host ? @host : 'localhost' ) params['no_verify'] = '1' - - if @id_ssh_key_file - # set default if using ssh while allowing a user using nil to disable this - @id_ssh_key_file = 'id_rsa' if @id_ssh_key_file == UNSET_VALUE - - # set ssh key for access to Libvirt host - # if no slash, prepend $HOME/.ssh/ - @id_ssh_key_file.prepend("#{ENV['HOME']}/.ssh/") if @id_ssh_key_file !~ /\A\// - params['keyfile'] = @id_ssh_key_file - end + params['keyfile'] = @id_ssh_key_file if @id_ssh_key_file else uri << '://' uri << @host if @host @@ -787,6 +780,8 @@ module VagrantPlugins @username = uri.user if @username == nil end + finalize_id_ssh_key_file + @storage_pool_name = 'default' if @storage_pool_name == UNSET_VALUE @snapshot_pool_name = @storage_pool_name if @snapshot_pool_name == UNSET_VALUE @storage_pool_path = nil if @storage_pool_path == UNSET_VALUE @@ -989,6 +984,36 @@ module VagrantPlugins result.qemu_env = c end end + + private + + def resolve_ssh_key_file(key_file) + # set ssh key for access to Libvirt host + # if no slash, prepend $HOME/.ssh/ + key_file.prepend("#{ENV['HOME']}/.ssh/") if key_file && key_file !~ /\A\// + + key_file + end + + def finalize_id_ssh_key_file + # resolve based on the following roles + # 1) if @connect_via_ssh is set to true, and id_ssh_key_file not current set, + # set default if the file exists + # 2) if supplied the key name, attempt to expand based on user home + # 3) otherwise set to nil + + if @connect_via_ssh && @id_ssh_key_file == UNSET_VALUE + # set default if using ssh while allowing a user using nil to disable this + id_ssh_key_file = resolve_ssh_key_file('id_rsa') + id_ssh_key_file = nil if !File.file?(id_ssh_key_file) + elsif @id_ssh_key_file != UNSET_VALUE + id_ssh_key_file = resolve_ssh_key_file(@id_ssh_key_file) + else + id_ssh_key_file = nil + end + + @id_ssh_key_file = id_ssh_key_file + end end end end diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index 3addf7a..0d22b64 100644 --- a/spec/unit/config_spec.rb +++ b/spec/unit/config_spec.rb @@ -1,3 +1,5 @@ +require 'contextual_proc' + require 'spec_helper' require 'support/sharedcontext' @@ -77,76 +79,104 @@ describe VagrantPlugins::ProviderLibvirt::Config do [ # all other set to default {}, {:uri => "custom:///custom_path", :qemu_use_session => false}, - {'LIBVIRT_DEFAULT_URI' => "custom:///custom_path"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "custom:///custom_path"}, + } ], [ # with session {}, {:uri => "qemu:///session", :qemu_use_session => true}, - {'LIBVIRT_DEFAULT_URI' => "qemu:///session"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu:///session"}, + } ], - [ # with session and using ssh + [ # with session and using ssh infer connect by ssh and ignore host {}, {:uri => "qemu+ssh:///session", :qemu_use_session => true}, - {'LIBVIRT_DEFAULT_URI' => "qemu+ssh:///session"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu+ssh:///session"}, + } ], [ # with session and using ssh infer host and connect by ssh {}, {:uri => "qemu+ssh:///session", :qemu_use_session => true, :connect_via_ssh => true, :host => 'localhost'}, - {'LIBVIRT_DEFAULT_URI' => "qemu+ssh:///session"}, - "not yet inferring connect_via_ssh", # once working remove the preceding test + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu+ssh:///session"}, + :allow_failure => "not yet inferring connect_via_ssh", # once working remove the preceding test + } ], [ # with session and using ssh to specific host with additional query options provided {}, {:uri => "qemu+ssh://remote/session?keyfile=my_id_rsa", :qemu_use_session => true}, - {'LIBVIRT_DEFAULT_URI' => "qemu+ssh://remote/session?keyfile=my_id_rsa"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu+ssh://remote/session?keyfile=my_id_rsa"}, + } ], [ # with session and using ssh to specific host with additional query options provided, infer host and ssh {}, {:uri => "qemu+ssh://remote/session?keyfile=my_id_rsa", :qemu_use_session => true, :connect_via_ssh => true, :host => 'remote'}, - {'LIBVIRT_DEFAULT_URI' => "qemu+ssh://remote/session?keyfile=my_id_rsa"}, - "not yet inferring host correctly", # once working remove the preceding test + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu+ssh://remote/session?keyfile=my_id_rsa"}, + :allow_failure => "not yet inferring host correctly", # once working remove the preceding test + } ], [ # when session not set {}, {:uri => "qemu:///system", :qemu_use_session => false}, - {'LIBVIRT_DEFAULT_URI' => "qemu:///system"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu:///system"}, + } ], [ # when session appearing elsewhere {}, {:uri => "qemu://remote/system?keyfile=my_session_id", :qemu_use_session => false}, - {'LIBVIRT_DEFAULT_URI' => "qemu://remote/system?keyfile=my_session_id"}, + { + :env => {'LIBVIRT_DEFAULT_URI' => "qemu://remote/system?keyfile=my_session_id"}, + } ], # ignore LIBVIRT_DEFAULT_URI due to explicit settings [ # when uri explicitly set {:uri => 'qemu:///system'}, {:uri => 'qemu:///system'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], [ # when host explicitly set {:host => 'remote'}, {:uri => 'qemu://remote/system'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], [ # when connect_via_ssh explicitly set {:connect_via_ssh => true}, - {:uri => 'qemu+ssh://localhost/system?no_verify=1&keyfile=/home/tests/.ssh/id_rsa'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + {:uri => 'qemu+ssh://localhost/system?no_verify=1'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], [ # when username explicitly set without ssh {:username => 'my_user' }, {:uri => 'qemu:///system'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], [ # when username explicitly set with host but without ssh {:username => 'my_user', :host => 'remote'}, {:uri => 'qemu://remote/system'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], [ # when password explicitly set {:password => 'some_password'}, - {:uri => 'qemu:///system'}, - {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + {:uri => 'qemu:///system', :password => 'some_password'}, + { + :env => {'LIBVIRT_DEFAULT_URI' => 'qemu://session'}, + } ], # driver settings @@ -178,23 +208,46 @@ describe VagrantPlugins::ProviderLibvirt::Config do # connect_via_ssh settings [ # enabled {:connect_via_ssh => true}, - {:uri => "qemu+ssh://localhost/system?no_verify=1&keyfile=/home/tests/.ssh/id_rsa"}, + {:uri => "qemu+ssh://localhost/system?no_verify=1"}, ], [ # enabled with user {:connect_via_ssh => true, :username => 'my_user'}, - {:uri => "qemu+ssh://my_user@localhost/system?no_verify=1&keyfile=/home/tests/.ssh/id_rsa"}, + {:uri => "qemu+ssh://my_user@localhost/system?no_verify=1"}, ], [ # enabled with host {:connect_via_ssh => true, :host => 'remote_server'}, - {:uri => "qemu+ssh://remote_server/system?no_verify=1&keyfile=/home/tests/.ssh/id_rsa"}, + {:uri => "qemu+ssh://remote_server/system?no_verify=1"}, ], # id_ssh_key_file behaviour + [ # set should take given value + {:connect_via_ssh => true, :id_ssh_key_file => '/path/to/keyfile'}, + {:uri => 'qemu+ssh://localhost/system?no_verify=1&keyfile=/path/to/keyfile', :connect_via_ssh => true}, + ], [ # set should infer use of ssh {:id_ssh_key_file => '/path/to/keyfile'}, - {:uri => "qemu+ssh://localhost/system?no_verify=1&keyfile=/path/to/keyfile"}, - {}, - "setting of ssh key file does not yet enable connect via ssh", + {:uri => 'qemu+ssh://localhost/system?no_verify=1&keyfile=/path/to/keyfile', :connect_via_ssh => true}, + { + :allow_failure => 'setting id_ssh_key_file explicitly does not yet infer ssh connection', # once fixed replace above + } + ], + [ # connect_via_ssh should enable default but ignore due to not existing + {:connect_via_ssh => true}, + {:uri => 'qemu+ssh://localhost/system?no_verify=1', :id_ssh_key_file => nil}, + { + :setup => ContextualProc.new { + expect(File).to receive(:file?).with("/home/tests/.ssh/id_rsa").and_return(false) + } + } + ], + [ # connect_via_ssh should enable default and include due to existing + {:connect_via_ssh => true}, + {:uri => 'qemu+ssh://localhost/system?no_verify=1&keyfile=/home/tests/.ssh/id_rsa', :id_ssh_key_file => '/home/tests/.ssh/id_rsa'}, + { + :setup => ContextualProc.new { + expect(File).to receive(:file?).with("/home/tests/.ssh/id_rsa").and_return(true) + } + } ], # socket behaviour @@ -202,19 +255,26 @@ describe VagrantPlugins::ProviderLibvirt::Config do {:socket => '/var/run/libvirt/libvirt-sock'}, {:uri => "qemu:///system?socket=/var/run/libvirt/libvirt-sock"}, ], - ].each do |inputs, outputs, env, allow_failure| - it "should handle inputs #{inputs} with env #{env}" do + ].each do |inputs, outputs, options| + opts = {} + opts.merge!(options) if options + + it "should handle inputs #{inputs} with env (#{opts[:env]})" do # allow some of these to fail for now if marked as such - if !allow_failure.nil? - pending(allow_failure) + if !opts[:allow_failure].nil? + pending(opts[:allow_failure]) + end + + if !opts[:setup].nil? + opts[:setup].apply(binding) end inputs.each do |k, v| subject.instance_variable_set("@#{k}", v) end - if !env.nil? - env.each do |k, v| + if !opts[:env].nil? + opts[:env].each do |k, v| fake_env[k] = v end end diff --git a/vagrant-libvirt.gemspec b/vagrant-libvirt.gemspec index 90b3e69..6bf6b98 100644 --- a/vagrant-libvirt.gemspec +++ b/vagrant-libvirt.gemspec @@ -16,6 +16,7 @@ Gem::Specification.new do |s| s.require_paths = ['lib'] s.version = VagrantPlugins::ProviderLibvirt.get_version + s.add_development_dependency "contextual_proc" s.add_development_dependency "rspec-core", "~> 3.5.0" s.add_development_dependency "rspec-expectations", "~> 3.5.0" s.add_development_dependency "rspec-mocks", "~> 3.5.0"