From 5aff6660fbd9c8c9adbf0403715cef95a3a73a60 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Tue, 12 May 2020 10:54:23 -0700 Subject: [PATCH] Move port checker method to class rather than instance This commit also moves out the ipv4_interfaces method to be a util method, so that the port checker can also access it as a class mehtod --- .../handle_forwarded_port_collisions.rb | 23 ++++---- lib/vagrant/util/ipv4_interfaces.rb | 13 +++++ lib/vagrant/util/is_port_open.rb | 2 +- .../handle_forwarded_port_collisions_test.rb | 57 +++---------------- .../unit/vagrant/util/ipv4_interfaces_test.rb | 47 +++++++++++++++ test/unit/vagrant/util/is_port_open_test.rb | 14 ++--- 6 files changed, 86 insertions(+), 70 deletions(-) create mode 100644 lib/vagrant/util/ipv4_interfaces.rb create mode 100644 test/unit/vagrant/util/ipv4_interfaces_test.rb diff --git a/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb b/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb index 23c40ed59..61acdf914 100644 --- a/lib/vagrant/action/builtin/handle_forwarded_port_collisions.rb +++ b/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 @@ -254,22 +256,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.") @@ -280,10 +279,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 diff --git a/lib/vagrant/util/ipv4_interfaces.rb b/lib/vagrant/util/ipv4_interfaces.rb new file mode 100644 index 000000000..1d2af5d05 --- /dev/null +++ b/lib/vagrant/util/ipv4_interfaces.rb @@ -0,0 +1,13 @@ +module Vagrant + module Util + module IPv4Interfaces + def self.ipv4_interfaces + Socket.getifaddrs.select do |ifaddr| + ifaddr.addr && ifaddr.addr.ipv4? + end.map do |ifaddr| + [ifaddr.name, ifaddr.addr.ip_address] + end + end + end + end +end diff --git a/lib/vagrant/util/is_port_open.rb b/lib/vagrant/util/is_port_open.rb index b51a01ecd..f4b0f8f6d 100644 --- a/lib/vagrant/util/is_port_open.rb +++ b/lib/vagrant/util/is_port_open.rb @@ -13,7 +13,7 @@ module Vagrant # @param [Integer] port Port to check. # @return [Boolean] `true` if the port is open (listening), `false` # otherwise. - def is_port_open?(host, port) + def self.is_port_open?(host, port) # We wrap this in a timeout because once in awhile the TCPSocket # _will_ hang, but this signals that the port is closed. Timeout.timeout(1) do 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 aecba56ee..8f16954af 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 @@ -79,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 @@ -147,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 } @@ -193,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 @@ -210,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 diff --git a/test/unit/vagrant/util/ipv4_interfaces_test.rb b/test/unit/vagrant/util/ipv4_interfaces_test.rb new file mode 100644 index 000000000..eb907dff1 --- /dev/null +++ b/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 + diff --git a/test/unit/vagrant/util/is_port_open_test.rb b/test/unit/vagrant/util/is_port_open_test.rb index d931a8790..e9c757d8d 100644 --- a/test/unit/vagrant/util/is_port_open_test.rb +++ b/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