From 45e87bdb35dd91e245566a1e1cb6c75864d5298f 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 | 7 ++++ plugins/providers/docker/action.rb | 5 ++- plugins/providers/docker/action/create.rb | 2 + .../docker/action/forwarded_ports.rb | 1 + ...prepare_forwarded_port_collision_params.rb | 26 ++++++++++++ plugins/providers/docker/communicator.rb | 2 +- plugins/providers/docker/driver.rb | 41 +++++++++++++++++++ 7 files changed, 82 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..315b95e307e 100644 --- a/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb +++ b/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb @@ -121,6 +121,13 @@ 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) + + 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/create.rb b/plugins/providers/docker/action/create.rb index 61afc24b5f6..b32cef6e403 100644 --- a/plugins/providers/docker/action/create.rb +++ b/plugins/providers/docker/action/create.rb @@ -138,6 +138,8 @@ def forwarded_ports(include_ssh=false) next end + require 'pry' + binding.pry mappings["#{options[:host]}/#{options[:protocol]}"] = options end diff --git a/plugins/providers/docker/action/forwarded_ports.rb b/plugins/providers/docker/action/forwarded_ports.rb index ed51b086d71..fd3af8284f5 100644 --- a/plugins/providers/docker/action/forwarded_ports.rb +++ b/plugins/providers/docker/action/forwarded_ports.rb @@ -6,6 +6,7 @@ def initialize(app, env) @app = app end + # TODO: Maybe rename this action 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..513d6def25b --- /dev/null +++ b/plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb @@ -0,0 +1,26 @@ +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 + + @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..903e5c5f393 100644 --- a/plugins/providers/docker/driver.rb +++ b/plugins/providers/docker/driver.rb @@ -126,6 +126,47 @@ 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 = {} + 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