config.ssh properly overrides provder-detected [GH-1479]

CHANGELOG.md

@@ -33,6 +33,8 @@ FEATURES:
   - Providers can now support multiple box formats by specifying the
     `box_format:` option.
   - CFEngine provisioner support.
+  - `config.ssh.default` settings introduced to set SSH defaults that
+    providers can still override. [GH-1479]
 
 IMPROVEMENTS:
 
@@ -72,6 +74,7 @@ BUG FIXES:
   - Fix issue parsing extra SSH args in `vagrant ssh` in multi-machine
     environments. [GH-1545]
   - Networks come back up properly on RedHat systems after reboot. [GH-921]
+  - `config.ssh` settings override all detected SSH settings (regression). [GH-1479]
 
 ## 1.1.6 (April 3, 2013)
 

config/default.rb

@@ -1,15 +1,16 @@
 Vagrant.configure("2") do |config|
   config.vagrant.host = :detect
 
-  config.ssh.username = "vagrant"
   config.ssh.guest_port = 22
   config.ssh.keep_alive = true
   config.ssh.max_tries = 100
   config.ssh.timeout = 30
-  config.ssh.forward_agent = false
-  config.ssh.forward_x11 = false
   config.ssh.shell = "bash -l"
 
+  config.ssh.default.username = "vagrant"
+  config.ssh.default.forward_agent = false
+  config.ssh.default.forward_x11 = false
+
   config.vm.usable_port_range = (2200..2250)
   config.vm.box_url = nil
   config.vm.base_mac = nil

lib/vagrant/machine.rb

@@ -257,11 +257,19 @@ module Vagrant
         info.delete(key) if value.nil?
       end
 
-      # Next, we default some fields if they weren't given to us by
+      # We set the defaults
-      # the provider.
+      info[:forward_agent] ||= @config.ssh.default.forward_agent
-      info[:host] ||= @config.ssh.host if @config.ssh.host
+      info[:forward_x11] ||= @config.ssh.default.forward_x11
-      info[:port] ||= @config.ssh.port if @config.ssh.port
+      info[:host] ||= @config.ssh.default.host
-      info[:username] ||= @config.ssh.username if @config.ssh.username
+      info[:port] ||= @config.ssh.default.port
+      info[:private_key_path] ||= @config.ssh.default.private_key_path
+      info[:username] ||= @config.ssh.default.username
+
+      # We set overrides if they are set. These take precedence over
+      # provider-returned data.
+      info[:host] = @config.ssh.host if @config.ssh.host
+      info[:port] = @config.ssh.port if @config.ssh.port
+      info[:username] = @config.ssh.username if @config.ssh.username
 
       # We also set some fields that are purely controlled by Varant
       info[:forward_agent] = @config.ssh.forward_agent

plugins/kernel_v2/config/ssh.rb

@@ -1,46 +1,47 @@
 require "vagrant"
 
+require_relative "ssh_connect"
+
 module VagrantPlugins
   module Kernel_V2
-    class SSHConfig < Vagrant.plugin("2", :config)
+    class SSHConfig < SSHConnectConfig
-      attr_accessor :forward_agent
-      attr_accessor :forward_x11
       attr_accessor :guest_port
-      attr_accessor :host
       attr_accessor :keep_alive
       attr_accessor :max_tries
-      attr_accessor :port
-      attr_accessor :private_key_path
       attr_accessor :shell
       attr_accessor :timeout
-      attr_accessor :username
+
+      attr_reader :default
 
       def initialize
-        @forward_agent    = UNSET_VALUE
+        super
-        @forward_x11      = UNSET_VALUE
+
-        @guest_port       = UNSET_VALUE
+        @guest_port = UNSET_VALUE
-        @host             = UNSET_VALUE
+        @keep_alive = UNSET_VALUE
-        @keep_alive       = UNSET_VALUE
+        @max_tries  = UNSET_VALUE
-        @max_tries        = UNSET_VALUE
+        @shell      = UNSET_VALUE
-        @port             = UNSET_VALUE
+        @timeout    = UNSET_VALUE
-        @private_key_path = UNSET_VALUE
+
-        @shell            = UNSET_VALUE
+        @default    = SSHConnectConfig.new
-        @timeout          = UNSET_VALUE
+      end
-        @username         = UNSET_VALUE
+
+      def merge(other)
+        super.tap do |result|
+          merged_defaults = @default.merge(other.default)
+          result.instance_variable_set(:@default, merged_defaults)
+        end
       end
 
       def finalize!
-        @forward_agent    = false if @forward_agent == UNSET_VALUE
+        super
-        @forward_x11      = false if @forward_x11 == UNSET_VALUE
+
-        @guest_port       = nil if @guest_port == UNSET_VALUE
+        @guest_port = nil if @guest_port == UNSET_VALUE
-        @host             = nil if @host == UNSET_VALUE
+        @keep_alive = false if @keep_alive == UNSET_VALUE
-        @keep_alive       = false if @keep_alive == UNSET_VALUE
+        @max_tries  = nil if @max_tries == UNSET_VALUE
-        @max_tries        = nil if @max_tries == UNSET_VALUE
+        @shell      = nil if @shell == UNSET_VALUE
-        @port             = nil if @port == UNSET_VALUE
+        @timeout    = nil if @timeout == UNSET_VALUE
-        @private_key_path = nil if @private_key_path == UNSET_VALUE
+
-        @shell            = nil if @shell == UNSET_VALUE
+        @default.finalize!
-        @timeout          = nil if @timeout == UNSET_VALUE
-        @username         = nil if @username == UNSET_VALUE
       end
 
       def to_s
@@ -48,20 +49,21 @@ module VagrantPlugins
       end
 
       def validate(machine)
-        errors = _detected_errors
+        errors = super
 
         [:max_tries, :timeout].each do |field|
           value = instance_variable_get("@#{field}".to_sym)
           errors << I18n.t("vagrant.config.common.error_empty", :field => field) if !value
         end
 
-        if private_key_path && \
-          !File.file?(File.expand_path(private_key_path, machine.env.root_path))
-          errors << I18n.t("vagrant.config.ssh.private_key_missing", :path => private_key_path)
-        end
-
         # Return the errors
-        { to_s => errors }
+        result = { to_s => errors }
+
+        # Figure out the errors for the defaults
+        default_errors = @default.validate(machine)
+        result["SSH Defaults"] = default_errors if !default_errors.empty?
+
+        result
       end
     end
   end

plugins/kernel_v2/config/ssh_connect.rb

@@ -0,0 +1,46 @@
+module VagrantPlugins
+  module Kernel_V2
+    class SSHConnectConfig < Vagrant.plugin("2", :config)
+      attr_accessor :forward_agent
+      attr_accessor :forward_x11
+      attr_accessor :host
+      attr_accessor :port
+      attr_accessor :private_key_path
+      attr_accessor :username
+
+      def initialize
+        @forward_agent    = UNSET_VALUE
+        @forward_x11      = UNSET_VALUE
+        @host             = UNSET_VALUE
+        @port             = UNSET_VALUE
+        @private_key_path = UNSET_VALUE
+        @username         = UNSET_VALUE
+      end
+
+      def finalize!
+        @forward_agent    = false if @forward_agent == UNSET_VALUE
+        @forward_x11      = false if @forward_x11 == UNSET_VALUE
+        @host             = nil if @host == UNSET_VALUE
+        @port             = nil if @port == UNSET_VALUE
+        @private_key_path = nil if @private_key_path == UNSET_VALUE
+        @username         = nil if @username == UNSET_VALUE
+      end
+
+      # NOTE: This is _not_ a valid config validation method, since it
+      # returns an _array_ of strings rather than a Hash. This is meant to
+      # be used with a subclass that handles this.
+      #
+      # @return [Array<String>]
+      def validate(machine)
+        errors = _detected_errors
+
+        if @private_key_path && \
+          !File.file?(File.expand_path(@private_key_path, machine.env.root_path))
+          errors << I18n.t("vagrant.config.ssh.private_key_missing", :path => @private_key_path)
+        end
+
+        errors
+      end
+    end
+  end
+end

test/unit/vagrant/machine_test.rb

@@ -296,6 +296,22 @@ describe Vagrant::Machine do
 
           instance.ssh_info[type].should == "bar"
         end
+
+        it "should use the default if no override and no provider" do
+          provider_ssh_info[type] = nil
+          instance.config.ssh.send("#{type}=", nil)
+          instance.config.ssh.default.send("#{type}=", "foo")
+
+          instance.ssh_info[type].should == "foo"
+        end
+
+        it "should use the override if set even with a provider" do
+          provider_ssh_info[type] = "baz"
+          instance.config.ssh.send("#{type}=", "bar")
+          instance.config.ssh.default.send("#{type}=", "foo")
+
+          instance.ssh_info[type].should == "bar"
+        end
       end
 
       it "should set the configured forward agent settings" do