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 3c90f4a2cc4..23c40ed59d4 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 @@ 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, diff --git a/plugins/providers/docker/action.rb b/plugins/providers/docker/action.rb index b512da387dd..f0a2d7a0506 100644 --- a/plugins/providers/docker/action.rb +++ b/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") diff --git a/plugins/providers/docker/action/forwarded_ports.rb b/plugins/providers/docker/action/forwarded_ports.rb index ed51b086d71..728eead5957 100644 --- a/plugins/providers/docker/action/forwarded_ports.rb +++ b/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 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 00000000000..cf45483788b --- /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 20b0df9122e..e36a2f366fa 100644 --- a/plugins/providers/docker/communicator.rb +++ b/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 diff --git a/plugins/providers/docker/driver.rb b/plugins/providers/docker/driver.rb index 506df5900eb..11462da215e 100644 --- a/plugins/providers/docker/driver.rb +++ b/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=>#, 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 326b0945fbf..7235ee9b678 100644 --- a/test/unit/plugins/providers/docker/driver_test.rb +++ b/test/unit/plugins/providers/docker/driver_test.rb @@ -309,6 +309,33 @@ 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 48b7e3a1164..aecba56eeed 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 @@ 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