Fixes #9067: Ensure new containers don't grab existing bound ports

Prior to this commit, if a created but exited container bound a port,
and a new container grabed that same port (say for an ssh port forward),
when the initial container came back up it would fail because the port
also got bound to the second container. This commit fixes that behavior
by first looking at what containers are already bound prior to creating
a container.

lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb

@@ -121,6 +121,17 @@ module Vagrant
             in_use = is_forwarded_already(extra_in_use, host_port, host_ip) ||
               call_port_checker(port_checker, host_ip, host_port) ||
               lease_check(host_ip, host_port)
+
+            # This check is required only for the docker provider. Containers
+            # can bind ports but be halted. We don't want new containers to
+            # grab these bound ports, so this check is here for that since
+            # the checks above won't detect it
+            if !in_use && env[:machine].provider_name == :docker
+              if extra_in_use.keys.include?(host_port.to_s)
+                in_use = true
+              end
+            end
+
             if in_use
               if !repair || !options[:auto_correct]
                 raise Errors::ForwardPortCollision,

plugins/providers/docker/action.rb

@@ -251,14 +251,16 @@ module VagrantPlugins
             if env[:machine_action] != :run_command
               # If the container is NOT created yet, then do some setup steps
               # necessary for creating it.
+
               b2.use Call, IsState, :preparing do |env2, b3|
                 if env2[:result]
                   b3.use EnvSet, port_collision_repair: true
                   b3.use HostMachinePortWarning
                   b3.use HostMachinePortChecker
+                  b3.use ForwardedPorts # This action converts the `ports` param into proper network configs
+                  b3.use PrepareForwardedPortCollisionParams
                   b3.use HandleForwardedPortCollisions
                   b3.use SyncedFolders
-                  b3.use ForwardedPorts
                   b3.use Pull
                   b3.use Create
                   b3.use WaitForRunning
@@ -313,6 +315,7 @@ module VagrantPlugins
       autoload :IsBuild, action_root.join("is_build")
       autoload :IsHostMachineCreated, action_root.join("is_host_machine_created")
       autoload :Login, action_root.join("login")
+      autoload :PrepareForwardedPortCollisionParams, action_root.join("prepare_forwarded_port_collision_params")
       autoload :PrepareNetworks, action_root.join("prepare_networks")
       autoload :PrepareNFSValidIds, action_root.join("prepare_nfs_valid_ids")
       autoload :PrepareNFSSettings, action_root.join("prepare_nfs_settings")

plugins/providers/docker/action/forwarded_ports.rb

@@ -6,6 +6,8 @@ module VagrantPlugins
           @app = app
         end
 
+        # Converts the `ports` docker provider param into proper network configs
+        # of type :forwarded_port
         def call(env)
           env[:machine].provider_config.ports.each do |p|
             host_ip = nil

plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb

@@ -0,0 +1,29 @@
+module VagrantPlugins
+  module DockerProvider
+    module Action
+      class PrepareForwardedPortCollisionParams
+        def initialize(app, env)
+          @app = app
+        end
+
+        def call(env)
+          machine = env[:machine]
+
+          # Get the forwarded ports used by other containers and
+          # consider those in use as well.
+          other_used_ports = machine.provider.driver.read_used_ports
+          env[:port_collision_extra_in_use] = other_used_ports
+
+          # Build the remap for any existing collision detections
+          remap = {}
+          env[:port_collision_remap] = remap
+
+          # Note: This might not be required yet (as it is with the virtualbox provider)
+          # so for now we leave the remap hash empty.
+
+          @app.call(env)
+        end
+      end
+    end
+  end
+end

plugins/providers/docker/communicator.rb

@@ -151,7 +151,7 @@ module VagrantPlugins
           "-i #{path}"
         end
 
-        # Use ad-hoc SSH options for the hop on the docker proxy 
+        # Use ad-hoc SSH options for the hop on the docker proxy
         if info[:forward_agent]
           ssh_args << "-o ForwardAgent=yes"
         end

plugins/providers/docker/driver.rb

@@ -126,6 +126,46 @@ module VagrantPlugins
         result =~ /^#{Regexp.escape(id)}$/
       end
 
+      # Reads all current docker containers and determines what ports
+      # are currently registered to be forwarded
+      # {2222=>#<Set: {"127.0.0.1"}>, 8080=>#<Set: {"*"}>, 9090=>#<Set: {"*"}>}
+      #
+      # Note: This is this format because of what the builtin action for resolving colliding
+      # port forwards expects.
+      #
+      # @return [Hash[Set]] used_ports - {forward_port: #<Set: {"host ip address"}>}
+      def read_used_ports
+        used_ports = Hash.new{|hash,key| hash[key] = Set.new}
+
+        all_containers.each do |c|
+          container_info = inspect_container(c)
+
+          if container_info["HostConfig"]["PortBindings"]
+            # We remove the first character because docker inspect adds a '/' to
+            # the beginning every container name to match the "internal"
+            # implementation of docker:
+            #   https://github.com/moby/moby/issues/6705
+            container_name = container_info["Name"][1..-1]
+            port_bindings = container_info["HostConfig"]["PortBindings"]
+            next if port_bindings.empty? # Nothing defined, but not nil either
+
+            port_bindings.each do |guest_port,host_mapping|
+              host_mapping.each do |h|
+                if h["HostIp"] == ""
+                  hostip = "*"
+                else
+                  hostip = h["HostIp"]
+                end
+                hostport = h["HostPort"]
+                used_ports[hostport].add(hostip)
+              end
+            end
+          end
+        end
+
+        used_ports
+      end
+
       def running?(cid)
         result = execute('docker', 'ps', '-q', '--no-trunc')
         result =~ /^#{Regexp.escape cid}$/m

test/unit/plugins/providers/docker/driver_test.rb

@@ -309,6 +309,33 @@ describe VagrantPlugins::DockerProvider::Driver do
     end
   end
 
+  describe '#read_used_ports' do
+    let(:all_containers) { ["container1\ncontainer2"] }
+    let(:container_info) { {"Name"=>"/container", "HostConfig"=>{"PortBindings"=>{}}} }
+    let(:empty_used_ports) { {} }
+
+    context "with existing port forwards" do
+      let(:container_info) { {"Name"=>"/container", "HostConfig"=>{"PortBindings"=>{"22/tcp"=>[{"HostIp"=>"127.0.0.1","HostPort"=>"2222"}] }}} }
+      let(:used_ports_set) { {"2222"=>Set["127.0.0.1"]} }
+
+      it 'should read all port bindings and return a hash of sets' do
+        allow(subject).to receive(:all_containers).and_return(all_containers)
+        allow(subject).to receive(:inspect_container).and_return(container_info)
+
+        used_ports = subject.read_used_ports
+        expect(used_ports).to eq(used_ports_set)
+      end
+    end
+
+    it 'returns empty if no ports are already bound' do
+      allow(subject).to receive(:all_containers).and_return(all_containers)
+      allow(subject).to receive(:inspect_container).and_return(container_info)
+
+      used_ports = subject.read_used_ports
+      expect(used_ports).to eq(empty_used_ports)
+    end
+  end
+
   describe '#running?' do
     let(:result) { subject.running?(cid) }
 

test/unit/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb

@@ -11,6 +11,7 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
       port_collision_remap: collision_remap, port_collision_repair: collision_repair,
       port_collision_port_check: collision_port_check }
   }
+  let(:provider_name) { "default" }
   let(:extra_in_use){ nil }
   let(:collision_remap){ nil }
   let(:collision_repair){ nil }
@@ -19,6 +20,7 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
 
   let(:machine) do
     double("machine").tap do |machine|
+      allow(machine).to receive(:provider_name).and_return(provider_name)
       allow(machine).to receive(:config).and_return(machine_config)
       allow(machine).to receive(:env).and_return(machine_env)
     end