Skip to content

Commit fab786c

Browse files
briancainbriancain
authored
Merge pull request hashicorp#11602 from briancain/feature/docker-port-collision-fix
Fixes hashicorp#9067: Ensure new containers don't grab existing bound ports
2 parents 4caa1a8 + 64ed950 commit fab786c

File tree

12 files changed

+222
-71
lines changed

12 files changed

+222
-71
lines changed

‎lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
require "socket"
55

66
require "vagrant/util/is_port_open"
7+
require "vagrant/util/ipv4_interfaces"
78

89
module Vagrant
910
module Action
@@ -26,6 +27,7 @@ module Builtin
2627
#
2728
class HandleForwardedPortCollisions
2829
include Util::IsPortOpen
30+
include Util::IPv4Interfaces
2931

3032
def initialize(app, env)
3133
@app = app
@@ -121,6 +123,7 @@ def handle(env)
121123
in_use = is_forwarded_already(extra_in_use, host_port, host_ip) ||
122124
call_port_checker(port_checker, host_ip, host_port) ||
123125
lease_check(host_ip, host_port)
126+
124127
if in_use
125128
if !repair || !options[:auto_correct]
126129
raise Errors::ForwardPortCollision,
@@ -243,22 +246,19 @@ def is_forwarded_already(extra_in_use, hostport, hostip)
243246
return extra_in_use.fetch(hostport).include?(hostip)
244247
end
245248

246-
def ipv4_interfaces
247-
Socket.getifaddrs.select do |ifaddr|
248-
ifaddr.addr && ifaddr.addr.ipv4?
249-
end.map do |ifaddr|
250-
[ifaddr.name, ifaddr.addr.ip_address]
251-
end
249+
def port_check(host_ip, host_port)
250+
self.class.port_check(@machine, host_ip, host_port)
252251
end
253252

254-
def port_check(host_ip, host_port)
253+
def self.port_check(machine, host_ip, host_port)
254+
@logger = Log4r::Logger.new("vagrant::action::builtin::handle_port_collisions")
255255
# If no host_ip is specified, intention taken to be listen on all interfaces.
256256
test_host_ip = host_ip || "0.0.0.0"
257257
if Util::Platform.windows? && test_host_ip == "0.0.0.0"
258258
@logger.debug("Testing port #{host_port} on all IPv4 interfaces...")
259-
available_interfaces = ipv4_interfaces.select do |interface|
259+
available_interfaces = Vagrant::Util::IPv4Interfaces.ipv4_interfaces.select do |interface|
260260
@logger.debug("Testing #{interface[0]} with IP address #{interface[1]}")
261-
!is_port_open?(interface[1], host_port)
261+
!Vagrant::Util::IsPortOpen.is_port_open?(interface[1], host_port)
262262
end
263263
if available_interfaces.empty?
264264
@logger.debug("Cannot forward port #{host_port} on any interfaces.")
@@ -269,10 +269,10 @@ def port_check(host_ip, host_port)
269269
end
270270
else
271271
# Do a regular check
272-
if test_host_ip == "0.0.0.0" || ipv4_interfaces.detect { |iface| iface[1] == test_host_ip }
273-
is_port_open?(test_host_ip, host_port)
272+
if test_host_ip == "0.0.0.0" || Vagrant::Util::IPv4Interfaces.ipv4_interfaces.detect { |iface| iface[1] == test_host_ip }
273+
Vagrant::Util::IsPortOpen.is_port_open?(test_host_ip, host_port)
274274
else
275-
raise Errors::ForwardPortHostIPNotFound, name: @machine.name, host_ip: host_ip
275+
raise Errors::ForwardPortHostIPNotFound, name: machine.name, host_ip: host_ip
276276
end
277277
end
278278
end

‎lib/vagrant/util/ipv4_interfaces.rb

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
module Vagrant
2+
module Util
3+
module IPv4Interfaces
4+
def ipv4_interfaces
5+
Socket.getifaddrs.select do |ifaddr|
6+
ifaddr.addr && ifaddr.addr.ipv4?
7+
end.map do |ifaddr|
8+
[ifaddr.name, ifaddr.addr.ip_address]
9+
end
10+
end
11+
12+
extend IPv4Interfaces
13+
end
14+
end
15+
end

‎lib/vagrant/util/is_port_open.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ def is_port_open?(host, port)
3434
# Any of the above exceptions signal that the port is closed.
3535
return false
3636
end
37+
38+
extend IsPortOpen
3739
end
3840
end
3941
end

‎plugins/providers/docker/action.rb

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,14 +251,16 @@ def self.action_start
251251
if env[:machine_action] != :run_command
252252
# If the container is NOT created yet, then do some setup steps
253253
# necessary for creating it.
254+
254255
b2.use Call, IsState, :preparing do |env2, b3|
255256
if env2[:result]
256257
b3.use EnvSet, port_collision_repair: true
257258
b3.use HostMachinePortWarning
258259
b3.use HostMachinePortChecker
260+
b3.use ForwardedPorts # This action converts the `ports` param into proper network configs
261+
b3.use PrepareForwardedPortCollisionParams
259262
b3.use HandleForwardedPortCollisions
260263
b3.use SyncedFolders
261-
b3.use ForwardedPorts
262264
b3.use Pull
263265
b3.use Create
264266
b3.use WaitForRunning
@@ -313,6 +315,7 @@ def self.action_suspend
313315
autoload :IsBuild, action_root.join("is_build")
314316
autoload :IsHostMachineCreated, action_root.join("is_host_machine_created")
315317
autoload :Login, action_root.join("login")
318+
autoload :PrepareForwardedPortCollisionParams, action_root.join("prepare_forwarded_port_collision_params")
316319
autoload :PrepareNetworks, action_root.join("prepare_networks")
317320
autoload :PrepareNFSValidIds, action_root.join("prepare_nfs_valid_ids")
318321
autoload :PrepareNFSSettings, action_root.join("prepare_nfs_settings")

‎plugins/providers/docker/action/forwarded_ports.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ def initialize(app, env)
66
@app = app
77
end
88

9+
# Converts the `ports` docker provider param into proper network configs
10+
# of type :forwarded_port
911
def call(env)
1012
env[:machine].provider_config.ports.each do |p|
1113
host_ip = nil

‎plugins/providers/docker/action/prepare_forwarded_port_collision_params.rb

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
module VagrantPlugins
2+
module DockerProvider
3+
module Action
4+
class PrepareForwardedPortCollisionParams
5+
def initialize(app, env)
6+
@app = app
7+
end
8+
9+
def call(env)
10+
machine = env[:machine]
11+
12+
# Get the forwarded ports used by other containers and
13+
# consider those in use as well.
14+
other_used_ports = machine.provider.driver.read_used_ports
15+
env[:port_collision_extra_in_use] = other_used_ports
16+
17+
# Build the remap for any existing collision detections
18+
#
19+
# Note: This remap might not be required yet (as it is with the virtualbox provider)
20+
# so for now we leave the remap hash empty.
21+
remap = {}
22+
env[:port_collision_remap] = remap
23+
24+
# This port checker method calls the custom port_check method
25+
# defined below. If its false, it will go ahead and use the built-in
26+
# port_check method to see if there are any live containers with bound
27+
# ports
28+
docker_port_check = proc { |host_ip, host_port|
29+
result = port_check(env, host_port)
30+
if !result
31+
result = Vagrant::Action::Builtin::HandleForwardedPortCollisions.port_check(machine, host_ip, host_port)
32+
end
33+
result}
34+
env[:port_collision_port_check] = docker_port_check
35+
36+
@app.call(env)
37+
end
38+
39+
protected
40+
41+
# This check is required the docker provider. Containers
42+
# can bind ports but be halted. We don't want new containers to
43+
# grab these bound ports, so this check is here for that since
44+
# the checks above won't detect it
45+
#
46+
# @param [Vagrant::Environment] env
47+
# @param [String] host_port
48+
# @returns [Bool]
49+
def port_check(env, host_port)
50+
extra_in_use = env[:port_collision_extra_in_use]
51+
52+
if extra_in_use
53+
return extra_in_use.include?(host_port.to_s)
54+
else
55+
return false
56+
end
57+
end
58+
end
59+
end
60+
end
61+
end

‎plugins/providers/docker/communicator.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ def container_ssh_command
151151
"-i #{path}"
152152
end
153153

154-
# Use ad-hoc SSH options for the hop on the docker proxy
154+
# Use ad-hoc SSH options for the hop on the docker proxy
155155
if info[:forward_agent]
156156
ssh_args << "-o ForwardAgent=yes"
157157
end

‎plugins/providers/docker/driver.rb

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,41 @@ def image?(id)
126126
result =~ /^#{Regexp.escape(id)}$/
127127
end
128128

129+
# Reads all current docker containers and determines what ports
130+
# are currently registered to be forwarded
131+
# {2222=>#<Set: {"127.0.0.1"}>, 8080=>#<Set: {"*"}>, 9090=>#<Set: {"*"}>}
132+
#
133+
# Note: This is this format because of what the builtin action for resolving colliding
134+
# port forwards expects.
135+
#
136+
# @return [Hash[Set]] used_ports - {forward_port: #<Set: {"host ip address"}>}
137+
def read_used_ports
138+
used_ports = Hash.new{|hash,key| hash[key] = Set.new}
139+
140+
all_containers.each do |c|
141+
container_info = inspect_container(c)
142+
143+
if container_info["HostConfig"]["PortBindings"]
144+
port_bindings = container_info["HostConfig"]["PortBindings"]
145+
next if port_bindings.empty? # Nothing defined, but not nil either
146+
147+
port_bindings.each do |guest_port,host_mapping|
148+
host_mapping.each do |h|
149+
if h["HostIp"] == ""
150+
hostip = "*"
151+
else
152+
hostip = h["HostIp"]
153+
end
154+
hostport = h["HostPort"]
155+
used_ports[hostport].add(hostip)
156+
end
157+
end
158+
end
159+
end
160+
161+
used_ports
162+
end
163+
129164
def running?(cid)
130165
result = execute('docker', 'ps', '-q', '--no-trunc')
131166
result =~ /^#{Regexp.escape cid}$/m

‎test/unit/plugins/providers/docker/driver_test.rb

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,33 @@
309309
end
310310
end
311311

312+
describe '#read_used_ports' do
313+
let(:all_containers) { ["container1\ncontainer2"] }
314+
let(:container_info) { {"Name"=>"/container", "HostConfig"=>{"PortBindings"=>{}}} }
315+
let(:empty_used_ports) { {} }
316+
317+
context "with existing port forwards" do
318+
let(:container_info) { {"Name"=>"/container", "HostConfig"=>{"PortBindings"=>{"22/tcp"=>[{"HostIp"=>"127.0.0.1","HostPort"=>"2222"}] }}} }
319+
let(:used_ports_set) { {"2222"=>Set["127.0.0.1"]} }
320+
321+
it 'should read all port bindings and return a hash of sets' do
322+
allow(subject).to receive(:all_containers).and_return(all_containers)
323+
allow(subject).to receive(:inspect_container).and_return(container_info)
324+
325+
used_ports = subject.read_used_ports
326+
expect(used_ports).to eq(used_ports_set)
327+
end
328+
end
329+
330+
it 'returns empty if no ports are already bound' do
331+
allow(subject).to receive(:all_containers).and_return(all_containers)
332+
allow(subject).to receive(:inspect_container).and_return(container_info)
333+
334+
used_ports = subject.read_used_ports
335+
expect(used_ports).to eq(empty_used_ports)
336+
end
337+
end
338+
312339
describe '#running?' do
313340
let(:result) { subject.running?(cid) }
314341

‎test/unit/vagrant/action/builtin/handle_forwarded_port_collisions_test.rb

Lines changed: 11 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
port_collision_remap: collision_remap, port_collision_repair: collision_repair,
1212
port_collision_port_check: collision_port_check }
1313
}
14+
let(:provider_name) { "default" }
1415
let(:extra_in_use){ nil }
1516
let(:collision_remap){ nil }
1617
let(:collision_repair){ nil }
@@ -19,6 +20,7 @@
1920

2021
let(:machine) do
2122
double("machine").tap do |machine|
23+
allow(machine).to receive(:provider_name).and_return(provider_name)
2224
allow(machine).to receive(:config).and_return(machine_config)
2325
allow(machine).to receive(:env).and_return(machine_env)
2426
end
@@ -77,7 +79,7 @@
7779

7880
it "should check if host port is in use" do
7981
expect(instance).to receive(:is_forwarded_already).and_return(false)
80-
expect(instance).to receive(:is_port_open?).and_return(false)
82+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).and_return(false)
8183
instance.call(env)
8284
end
8385

@@ -145,57 +147,18 @@
145147
describe "#recover" do
146148
end
147149

148-
describe "#ipv4_interfaces" do
149-
let(:name) { double("name") }
150-
let(:address) { double("address") }
151-
152-
let(:ipv4_ifaddr) do
153-
double("ipv4_ifaddr").tap do |ifaddr|
154-
allow(ifaddr).to receive(:name).and_return(name)
155-
allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(true)
156-
allow(ifaddr).to receive_message_chain(:addr, :ip_address).and_return(address)
157-
end
158-
end
159-
160-
let(:ipv6_ifaddr) do
161-
double("ipv6_ifaddr").tap do |ifaddr|
162-
allow(ifaddr).to receive(:name)
163-
allow(ifaddr).to receive_message_chain(:addr, :ipv4?).and_return(false)
164-
end
165-
end
166-
167-
let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr ] }
168-
169-
before do
170-
allow(Socket).to receive(:getifaddrs).and_return(ifaddrs)
171-
end
172-
173-
it "returns a list of IPv4 interfaces with their names and addresses" do
174-
expect(instance.send(:ipv4_interfaces)).to eq([ [name, address] ])
175-
end
176-
177-
context "with nil interface address" do
178-
let(:nil_ifaddr) { double("nil_ifaddr", addr: nil ) }
179-
let(:ifaddrs) { [ ipv4_ifaddr, ipv6_ifaddr, nil_ifaddr ] }
180-
181-
it "filters out nil addr info" do
182-
expect(instance.send(:ipv4_interfaces)).to eq([ [name, address] ])
183-
end
184-
end
185-
end
186-
187150
describe "#port_check" do
188151
let(:host_ip){ "127.0.0.1" }
189152
let(:host_port){ 8080 }
190153
let(:interfaces) { [ ["lo0", "127.0.0.1"], ["eth0", "192.168.1.7"] ] }
191154

192155
before do
193156
instance.instance_variable_set(:@machine, machine)
194-
allow(instance).to receive(:ipv4_interfaces).and_return(interfaces)
157+
allow(Vagrant::Util::IPv4Interfaces).to receive(:ipv4_interfaces).and_return(interfaces)
195158
end
196159

197160
it "should check if the port is open" do
198-
expect(instance).to receive(:is_port_open?).with(host_ip, host_port).and_return(true)
161+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(host_ip, host_port).and_return(true)
199162
instance.send(:port_check, host_ip, host_port)
200163
end
201164

@@ -208,23 +171,23 @@
208171
end
209172

210173
it "should check the port on every IPv4 interface" do
211-
expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port)
212-
expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port)
174+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port)
175+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port)
213176
instance.send(:port_check, host_ip, host_port)
214177
end
215178

216179
it "should return false if the port is closed on any IPv4 interfaces" do
217-
expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port).
180+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port).
218181
and_return(true)
219-
expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port).
182+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port).
220183
and_return(false)
221184
expect(instance.send(:port_check, host_ip, host_port)).to be(false)
222185
end
223186

224187
it "should return true if the port is open on all IPv4 interfaces" do
225-
expect(instance).to receive(:is_port_open?).with(interfaces[0][1], host_port).
188+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[0][1], host_port).
226189
and_return(true)
227-
expect(instance).to receive(:is_port_open?).with(interfaces[1][1], host_port).
190+
expect(Vagrant::Util::IsPortOpen).to receive(:is_port_open?).with(interfaces[1][1], host_port).
228191
and_return(true)
229192
expect(instance.send(:port_check, host_ip, host_port)).to be(true)
230193
end

0 commit comments

Comments
 (0)