Fix MiqEnvironment.local_ip_address to not prefer loopback #20992

agrare · 2021-01-28T15:58:47Z

#local_ip_address currently prefers loopback addresses since it filters out "private" addresses and not "loopback" addresses.

IMO we should prefer public, then private, then loopback.

https://github.com/ManageIQ/manageiq/pull/20341/files#r566198984
#20812 (comment)
#20986

djberg96 · 2021-01-28T16:05:24Z

Sorry, guess I goofed it before.

👍🏻

agrare · 2021-01-28T16:05:30Z

I'm not sure but I think there might have been a misunderstanding of what Addrinfo considers a "private" address. It looks like it was intended to say !127.0 which would actually be !ipv4_loopback? since the common 192.168, 172.16, and 10. addresses are "private"

agrare · 2021-01-28T16:08:24Z

Not sure if we want to use hostname -i for the spec test on this one, will investigate alternatives

spec/lib/miq_environment_spec.rb

`#local_ip_address` currently prefers loopback addresses since it filters out "private" addresses and not "loopback" addresses. IMO we should prefer public, then private, then loopback.

Fryguy · 2021-01-28T19:31:09Z

lib/miq_environment.rb

+    # prefer private addresses, then take whatever we can get
+    local_addr   = ipv4_addrs.detect { |ip| !ip.ipv4_loopback? && !ip.ipv4_private? }
+    local_addr ||= ipv4_addrs.detect { |ip| !ip.ipv4_loopback? }
+    local_addr ||= ipv4_addrs.first


For funsies, I think a sort would work too?

local_addr = ipv4_addrs.sort_by { |ip| ip.ipv4_loopback? ? 2 : (ip.ipv4_private? ? 1 : 0) }.first

Only thing I'm not sure about is if there are multiple private ips, I'm not sure if this is deterministic

Yeah good point about the return order from Socket not being guaranteed

Alright now we're sorting by the ip_address and added a spec test with multiple private_ips returned by Socket.ip_address_list in different orders.

agrare · 2021-01-29T15:39:28Z

spec/models/miq_server/environment_manager_spec.rb

@@ -2,14 +2,14 @@

 RSpec.describe "Server Environment Management" do
  let(:mac_address) { 'a:1:b:2:c:3:d:4' }
-  let(:hostname) { Socket.gethostname }
-  let(:loopback) { '127.0.0.1' }
+  let(:hostname) { Socket.gethostbyname(Socket.gethostname).first }


Not related to the local_ip_address method but the hostname was also failing for me because this was looking for Socket.gethostname which returns in my case desktop but Socket.gethostbyname(Socket.gethostname).first returns the fully qualified name which in my case is something like desktop.a.b.example.com so even when the IP matched:

1) Server Environment Management .get_network_information when in non-production mode Failure/Error: expect(MiqServer.get_network_information).to eq([ip_address, hostname, mac_address]) expected: ["10.2.4.101", "desktop", "a:1:b:2:c:3:d:4"] got: ["10.2.4.101", "desktop.a.b.example", "a:1:b:2:c:3:d:4"]

agrare · 2021-01-29T15:40:43Z

spec/models/miq_server/environment_manager_spec.rb

-  let(:hostname) { Socket.gethostname }
-  let(:loopback) { '127.0.0.1' }
+  let(:hostname) { Socket.gethostbyname(Socket.gethostname).first }
+  let(:ip_address) { Socket.ip_address_list.select(&:ipv4?).sort_by(&:ip_address).detect(&:ipv4_private?)&.ip_address }


Thinking it might be better to mock this like we mock the MAC Address so we're not dealing with weird travis vs local env issues

agrare · 2021-01-29T15:53:42Z

spec/models/miq_server/environment_manager_spec.rb

@@ -1,15 +1,21 @@
 require 'socket'

 RSpec.describe "Server Environment Management" do
-  let(:mac_address) { 'a:1:b:2:c:3:d:4' }
-  let(:hostname) { Socket.gethostname }
-  let(:loopback) { '127.0.0.1' }


These were only used in this context so I just moved them down

miq-bot · 2021-01-29T16:00:42Z

Checked commits agrare/manageiq@88593d3~...16f8263 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🍰

Fryguy · 2021-11-10T18:32:00Z

Backported to kasparov in commit 9855433.

commit 98554337d27d58d21a383f4fdd0fe2f666478f7e
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Feb 1 17:03:39 2021 -0500

    Merge pull request #20992 from agrare/fix_miq_environment_local_ip_address
    
    Fix MiqEnvironment.local_ip_address to not prefer loopback
    
    (cherry picked from commit fd276bac67283bf050e68ecc40cf95b31c5dd16b)

…dress Fix MiqEnvironment.local_ip_address to not prefer loopback (cherry picked from commit fd276ba)

agrare requested review from Fryguy and gtanzillo as code owners January 28, 2021 15:58

agrare added the bug label Jan 28, 2021

agrare mentioned this pull request Jan 28, 2021

Add ARM64 jobs in Travis-CI #20812

Merged

chessbyte approved these changes Jan 28, 2021

View reviewed changes

agrare force-pushed the fix_miq_environment_local_ip_address branch from 28f85f3 to e746ef8 Compare January 28, 2021 18:37

agrare assigned Fryguy Jan 28, 2021

agrare added the core label Jan 28, 2021

Fryguy reviewed Jan 28, 2021

View reviewed changes

spec/lib/miq_environment_spec.rb Outdated Show resolved Hide resolved

Fix MiqEnvironment.local_ip_address to not prefer loopback

88593d3

`#local_ip_address` currently prefers loopback addresses since it filters out "private" addresses and not "loopback" addresses. IMO we should prefer public, then private, then loopback.

agrare force-pushed the fix_miq_environment_local_ip_address branch from e746ef8 to 88593d3 Compare January 28, 2021 19:21

Fryguy reviewed Jan 28, 2021

View reviewed changes

Sort the ip addresses to ensure reliable returns

0cc1dde

agrare commented Jan 29, 2021

View reviewed changes

Stub Socket.ip_address_list and .gethostbyname

2b3e5c0

agrare commented Jan 29, 2021

View reviewed changes

Test the same address is returned

16f8263

Fryguy merged commit fd276ba into ManageIQ:master Feb 1, 2021

agrare deleted the fix_miq_environment_local_ip_address branch February 1, 2021 22:07

jrafanie mentioned this pull request Sep 16, 2021

MiqEnvironment method 'local_ip_address' returns the loopback address instead of actual address #20432

Closed

Fryguy added the kasparov/yes label Nov 10, 2021

Fryguy added a commit that referenced this pull request Nov 10, 2021

Merge pull request #20992 from agrare/fix_miq_environment_local_ip_ad…

9855433

…dress Fix MiqEnvironment.local_ip_address to not prefer loopback (cherry picked from commit fd276ba)

Fryguy added kasparov/backported and removed kasparov/yes labels Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MiqEnvironment.local_ip_address to not prefer loopback #20992

Fix MiqEnvironment.local_ip_address to not prefer loopback #20992

agrare commented Jan 28, 2021 •

edited

Loading

djberg96 commented Jan 28, 2021

agrare commented Jan 28, 2021

agrare commented Jan 28, 2021

Fryguy Jan 28, 2021 •

edited

Loading

agrare Jan 28, 2021

agrare Jan 29, 2021

agrare Jan 29, 2021

agrare Jan 29, 2021 •

edited

Loading

agrare Jan 29, 2021

miq-bot commented Jan 29, 2021

Fryguy commented Nov 10, 2021

Fix MiqEnvironment.local_ip_address to not prefer loopback #20992

Fix MiqEnvironment.local_ip_address to not prefer loopback #20992

Conversation

agrare commented Jan 28, 2021 • edited Loading

djberg96 commented Jan 28, 2021

agrare commented Jan 28, 2021

agrare commented Jan 28, 2021

Fryguy Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

agrare Jan 28, 2021

Choose a reason for hiding this comment

agrare Jan 29, 2021

Choose a reason for hiding this comment

agrare Jan 29, 2021

Choose a reason for hiding this comment

agrare Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

agrare Jan 29, 2021

Choose a reason for hiding this comment

miq-bot commented Jan 29, 2021

Fryguy commented Nov 10, 2021

agrare commented Jan 28, 2021 •

edited

Loading

Fryguy Jan 28, 2021 •

edited

Loading

agrare Jan 29, 2021 •

edited

Loading