From b2d9abe344cde538604f2ca96bf020430319b3f1 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Wed, 6 May 2020 16:01:05 -0700 Subject: [PATCH] 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. --- .../handle_forwarded_port_collisions.rb | 11 +++++ plugins/providers/docker/action.rb | 5 ++- .../docker/action/forwarded_ports.rb | 2 + ...prepare_forwarded_port_collision_params.rb | 29 ++++++++++++++ plugins/providers/docker/communicator.rb | 2 +- plugins/providers/docker/driver.rb | 40 +++++++++++++++++++ .../plugins/providers/docker/driver_test.rb | 27 +++++++++++++ .../handle_forwarded_port_collisions_test.rb | 2 + 8 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb diff --git a/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb b/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb index 3c90f4a2c..23c40ed59 100644 --- a/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb +++ b/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, diff --git a/plugins/providers/docker/action.rb b/plugins/providers/docker/action.rb index b512da387..f0a2d7a05 100644 --- a/plugins/providers/docker/action.rb +++ b/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") diff --git a/plugins/providers/docker/action/forwarded_ports.rb b/plugins/providers/docker/action/forwarded_ports.rb index ed51b086d..728eead59 100644 --- a/plugins/providers/docker/action/forwarded_ports.rb +++ b/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 diff --git a/plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb b/plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb new file mode 100644 index 000000000..cf4548378 --- /dev/null +++ b/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 diff --git a/plugins/providers/docker/communicator.rb b/plugins/providers/docker/communicator.rb index 20b0df912..e36a2f366 100644 --- a/plugins/providers/docker/communicator.rb +++ b/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 diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index 506df5900..11462da21 100644 --- a/plugins/providers/docker/driver.rb +++ b/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=>#, 8080=>#, 9090=>#} + # + # Note: This is this format because of what the builtin action for resolving colliding + # port forwards expects. + # + # @return [Hash[Set]] used_ports - {forward_port: #} + 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 diff --git a/test/unit/plugins/providers/docker/driver_test.rb b/test/unit/plugins/providers/docker/driver_test.rb index 326b0945f..7235ee9b6 100644 --- a/test/unit/plugins/providers/docker/driver_test.rb +++ b/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) } diff --git a/test/unit/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb b/test/unit/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb index 48b7e3a11..aecba56ee 100644 --- a/test/unit/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb +++ b/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