Fixes hashicorp#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 @@ def handle(env)
             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 @@ def self.action_start
             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 @@ def self.action_suspend
       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 @@ def initialize(app, env)
           @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 @@ def container_ssh_command
           "-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 @@ def image?(id)
         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/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb

@@ -11,6 +11,7 @@
       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 @@
 
   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