Merge pull request #11602 from briancain/feature/docker-port-collision-fix

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

lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb

@@ -4,6 +4,7 @@ require "log4r"
 require "socket"
 
 require "vagrant/util/is_port_open"
+require "vagrant/util/ipv4_interfaces"
 
 module Vagrant
   module Action
@@ -26,6 +27,7 @@ module Vagrant
       #
       class HandleForwardedPortCollisions
         include Util::IsPortOpen
+        include Util::IPv4Interfaces
 
         def initialize(app, env)
           @app    = app
@@ -121,6 +123,7 @@ 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)
+
             if in_use
               if !repair || !options[:auto_correct]
                 raise Errors::ForwardPortCollision,
@@ -243,22 +246,19 @@ module Vagrant
           return extra_in_use.fetch(hostport).include?(hostip)
         end
 
-        def ipv4_interfaces
-          Socket.getifaddrs.select do |ifaddr|
-            ifaddr.addr && ifaddr.addr.ipv4?
-          end.map do |ifaddr|
-            [ifaddr.name, ifaddr.addr.ip_address]
-          end
+        def port_check(host_ip, host_port)
+          self.class.port_check(@machine, host_ip, host_port)
         end
 
-        def port_check(host_ip, host_port)
+        def self.port_check(machine, host_ip, host_port)
+          @logger = Log4r::Logger.new("vagrant::action::builtin::handle_port_collisions")
           # If no host_ip is specified, intention taken to be listen on all interfaces.
           test_host_ip = host_ip || "0.0.0.0"
           if Util::Platform.windows? && test_host_ip == "0.0.0.0"
             @logger.debug("Testing port #{host_port} on all IPv4 interfaces...")
-            available_interfaces = ipv4_interfaces.select do |interface|
+            available_interfaces = Vagrant::Util::IPv4Interfaces.ipv4_interfaces.select do |interface|
               @logger.debug("Testing #{interface[0]} with IP address #{interface[1]}")
-              !is_port_open?(interface[1], host_port)
+              !Vagrant::Util::IsPortOpen.is_port_open?(interface[1], host_port)
             end
             if available_interfaces.empty?
               @logger.debug("Cannot forward port #{host_port} on any interfaces.")
@@ -269,10 +269,10 @@ module Vagrant
             end
           else
             # Do a regular check
-            if test_host_ip == "0.0.0.0" || ipv4_interfaces.detect { |iface| iface[1] == test_host_ip }
-              is_port_open?(test_host_ip, host_port)
+            if test_host_ip == "0.0.0.0" || Vagrant::Util::IPv4Interfaces.ipv4_interfaces.detect { |iface| iface[1] == test_host_ip }
+              Vagrant::Util::IsPortOpen.is_port_open?(test_host_ip, host_port)
             else
-              raise Errors::ForwardPortHostIPNotFound, name: @machine.name, host_ip: host_ip
+              raise Errors::ForwardPortHostIPNotFound, name: machine.name, host_ip: host_ip
             end
           end
         end

lib/vagrant/util/ipv4_interfaces.rb

@@ -0,0 +1,15 @@
+module Vagrant
+  module Util
+    module IPv4Interfaces
+      def ipv4_interfaces
+        Socket.getifaddrs.select do |ifaddr|
+          ifaddr.addr && ifaddr.addr.ipv4?
+        end.map do |ifaddr|
+          [ifaddr.name, ifaddr.addr.ip_address]
+        end
+      end
+
+      extend IPv4Interfaces
+    end
+  end
+end

lib/vagrant/util/is_port_open.rb

@@ -34,6 +34,8 @@ module Vagrant
         # Any of the above exceptions signal that the port is closed.
         return false
       end
+
+      extend IsPortOpen
     end
   end
 end

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,61 @@
+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
+          #
+          # Note: This remap might not be required yet (as it is with the virtualbox provider)
+          # so for now we leave the remap hash empty.
+          remap = {}
+          env[:port_collision_remap] = remap
+
+          # This port checker method calls the custom port_check method
+          # defined below. If its false, it will go ahead and use the built-in
+          # port_check method to see if there are any live containers with bound
+          # ports
+          docker_port_check = proc { |host_ip, host_port|
+                                      result = port_check(env, host_port)
+                                      if !result
+                                        result = Vagrant::Action::Builtin::HandleForwardedPortCollisions.port_check(machine, host_ip, host_port)
+                                      end
+                                      result}
+          env[:port_collision_port_check] = docker_port_check
+
+          @app.call(env)
+        end
+
+        protected
+
+        # This check is required 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
+        #
+        # @param [Vagrant::Environment] env
+        # @param [String] host_port
+        # @returns [Bool]
+        def port_check(env, host_port)
+          extra_in_use = env[:port_collision_extra_in_use]
+
+          if extra_in_use
+            return extra_in_use.include?(host_port.to_s)
+          else
+            return false
+          end
+        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,41 @@ 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"]
+            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
@@ -77,7 +79,7 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
 
       it "should check if host port is in use" do
         expect(instance).to receive(:is_forwarded_already).and_return(false)
-        expect(instance).to receive(:is_port_open?).and_return(false)
+        expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).and_return(false)
         instance.call(env)
       end
 
@@ -145,45 +147,6 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
   describe "#recover" do
   end
 
-  describe "#ipv4_interfaces" do
-    let(:name) { double("name") }
-    let(:address) { double("address") }
-
-    let(:ipv4_ifaddr) do
-      double("ipv4_ifaddr").tap do |ifaddr|
-        allow(ifaddr).to receive(:name).and_return(name)
-        allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(true)
-        allow(ifaddr).to receive_message_chain(:addr, :ip_address).and_return(address)
-      end
-    end
-
-    let(:ipv6_ifaddr) do
-      double("ipv6_ifaddr").tap do |ifaddr|
-        allow(ifaddr).to receive(:name)
-        allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(false)
-      end
-    end
-
-    let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr ] }
-
-    before do
-      allow(Socket).to receive(:getifaddrs).and_return(ifaddrs)
-    end
-
-    it "returns a list of IPv4 interfaces with their names and addresses" do
-      expect(instance.send(:ipv4_interfaces)).to eq([ [name, address] ])
-    end
-
-    context "with nil interface address" do
-      let(:nil_ifaddr) { double("nil_ifaddr", addr: nil ) }
-      let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr, nil_ifaddr ] }
-
-      it "filters out nil addr info" do
-        expect(instance.send(:ipv4_interfaces)).to eq([ [name, address] ])
-      end
-    end
-  end
-
   describe "#port_check" do
     let(:host_ip){ "127.0.0.1" }
     let(:host_port){ 8080 }
@@ -191,11 +154,11 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
 
     before do
       instance.instance_variable_set(:@machine, machine)
-      allow(instance).to receive(:ipv4_interfaces).and_return(interfaces)
+      allow(Vagrant::Util::IPv4Interfaces).to receive(:ipv4_interfaces).and_return(interfaces)
     end
 
     it "should check if the port is open" do
-      expect(instance).to receive(:is_port_open?).with(host_ip, host_port).and_return(true)
+      expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(host_ip, host_port).and_return(true)
       instance.send(:port_check, host_ip, host_port)
     end
 
@@ -208,23 +171,23 @@ describe Vagrant::Action::Builtin::HandleForwardedPortCollisions do
         end
 
         it "should check the port on every IPv4 interface" do
-          expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port)
-          expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port)
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port)
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port)
           instance.send(:port_check, host_ip, host_port)
         end
 
         it "should return false if the port is closed on any IPv4 interfaces" do
-          expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port).
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port).
             and_return(true)
-          expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port).
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port).
             and_return(false)
           expect(instance.send(:port_check, host_ip, host_port)).to be(false)
         end
 
         it "should return true if the port is open on all IPv4 interfaces" do
-          expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port).
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port).
             and_return(true)
-          expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port).
+          expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port).
             and_return(true)
           expect(instance.send(:port_check, host_ip, host_port)).to be(true)
         end

test/unit/vagrant/util/ipv4_interfaces_test.rb

@@ -0,0 +1,47 @@
+require File.expand_path("../../../base", __FILE__)
+
+require "vagrant/util/ipv4_interfaces"
+
+describe Vagrant::Util::IPv4Interfaces do
+  subject { described_class }
+
+  describe "#ipv4_interfaces" do
+    let(:name) { double("name") }
+    let(:address) { double("address") }
+
+    let(:ipv4_ifaddr) do
+      double("ipv4_ifaddr").tap do |ifaddr|
+        allow(ifaddr).to receive(:name).and_return(name)
+        allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(true)
+        allow(ifaddr).to receive_message_chain(:addr, :ip_address).and_return(address)
+      end
+    end
+
+    let(:ipv6_ifaddr) do
+      double("ipv6_ifaddr").tap do |ifaddr|
+        allow(ifaddr).to receive(:name)
+        allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(false)
+      end
+    end
+
+    let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr ] }
+
+    before do
+      allow(Socket).to receive(:getifaddrs).and_return(ifaddrs)
+    end
+
+    it "returns a list of IPv4 interfaces with their names and addresses" do
+      expect(subject.ipv4_interfaces).to eq([ [name, address] ])
+    end
+
+    context "with nil interface address" do
+      let(:nil_ifaddr) { double("nil_ifaddr", addr: nil ) }
+      let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr, nil_ifaddr ] }
+
+      it "filters out nil addr info" do
+        expect(subject.ipv4_interfaces).to eq([ [name, address] ])
+      end
+    end
+  end
+end
+

test/unit/vagrant/util/is_port_open_test.rb

@@ -5,11 +5,7 @@ require "socket"
 require "vagrant/util/is_port_open"
 
 describe Vagrant::Util::IsPortOpen do
-  let(:klass) do
-    Class.new do
-      extend Vagrant::Util::IsPortOpen
-    end
-  end
+  subject { described_class }
 
   let(:open_port)   { 52811 }
   let(:closed_port) { 52811 }
@@ -36,7 +32,7 @@ describe Vagrant::Util::IsPortOpen do
     end
 
     # Verify that we report the port is open
-    expect(klass.is_port_open?("127.0.0.1", open_port)).to be
+    expect(subject.is_port_open?("127.0.0.1", open_port)).to be
 
     # Kill the thread
     thr[:die] = true
@@ -47,17 +43,17 @@ describe Vagrant::Util::IsPortOpen do
     # This CAN fail, since port 52811 might actually be in use, but I'm
     # not sure what to do except choose some random port and hope for the
     # best, really.
-    expect(klass.is_port_open?("127.0.0.1", closed_port)).not_to be
+    expect(subject.is_port_open?("127.0.0.1", closed_port)).not_to be
   end
 
   it "should handle connection refused" do
     expect(TCPSocket).to receive(:new).with("0.0.0.0", closed_port).and_raise(Errno::ECONNREFUSED)
-    expect(klass.is_port_open?("0.0.0.0", closed_port)).to be(false)
+    expect(subject.is_port_open?("0.0.0.0", closed_port)).to be(false)
   end
 
   it "should raise an error if cannot assign requested address" do
     expect(TCPSocket).to receive(:new).with("0.0.0.0", open_port).and_raise(Errno::EADDRNOTAVAIL)
-    expect { klass.is_port_open?("0.0.0.0", open_port) }.to raise_error(Errno::EADDRNOTAVAIL)
+    expect { subject.is_port_open?("0.0.0.0", open_port) }.to raise_error(Errno::EADDRNOTAVAIL)
   end
 end