Fix web console for AWS, GCE and enable for RHOS #15901

bmclaughlin · 2017-08-29T13:45:36Z

This fixes Cockpit console from attempting to connect to AWS and GCE on private instead of public ip addresses and enables Cockpit console for RHOS.

Needs to be reviewed at the same time as ManageIQ/manageiq-ui-classic#2039.

@miq-bot add_labels bug, core, ui

https://bugzilla.redhat.com/show_bug.cgi?id=1450109
https://bugzilla.redhat.com/show_bug.cgi?id=1451655

petervo

Mostly looks good, just one testing related regression

petervo · 2017-08-29T14:52:53Z

app/models/vm/operations.rb

-                       MiqCockpitWsWorker.fetch_worker_settings_from_server(miq_server),
-                       ipv4_address || ipaddresses.first)
+                       miq_server.nil? ? nil : MiqCockpitWsWorker.fetch_worker_settings_from_server(miq_server),
+                       ipv4_address || ipaddresses.first).to_s


the to_s here breaks tests

chessbyte · 2017-08-29T15:20:10Z

app/models/container_node.rb

+                   nil
+                 else
+                   ext_management_system.zone.remote_cockpit_ws_miq_server
+                 end


how about the following:

miq_server = ext_management_system.try(:zone).try(:remote_cockpit_ws_miq_server)

chessbyte · 2017-08-29T15:21:37Z

app/models/vm/operations.rb

+                   nil
+                 else
+                   ext_management_system.zone.remote_cockpit_ws_miq_server
+                 end


copy/paste code

chessbyte · 2017-08-29T15:21:46Z

app/models/vm/operations.rb

    MiqCockpit::WS.url(miq_server,
-                       MiqCockpitWsWorker.fetch_worker_settings_from_server(miq_server),
-                       ipv4_address || ipaddresses.first)
+                       miq_server.nil? ? nil : MiqCockpitWsWorker.fetch_worker_settings_from_server(miq_server),


copy/paste code

Fryguy · 2017-08-30T12:36:07Z

app/models/vm/operations.rb

-    ipaddresses.find { |ip| IPAddr.new(ip).ipv4? }
+    if %w(amazon google).include?(vendor.downcase)
+      public_address
+    elsif %w(openstack).include?(vendor.downcase)


Please no provider specific code in the base module. This should be handled via provider overrides.

Fryguy · 2017-08-30T12:36:36Z

@jrafanie Please also review.

bmclaughlin · 2017-08-30T18:05:16Z

@chessbyte @Fryguy @jrafanie @petervo, changes made per reviews, thanks! Ready for another round.

petervo · 2017-08-30T18:15:15Z

app/models/vm/operations.rb

  end

  def ipv4_address
-    ipaddresses.find { |ip| IPAddr.new(ip).ipv4? }
+    return public_address unless public_address.nil?
+    ipaddresses.find { |ip| IPAddr.new(ip).ipv4? unless ip.starts_with?('192') }


Is this robust enough? I think i've seen 10.x IPs on amazon.

Does the VM have any notion if it's hostname is public or not? If it did that might make this simpler.

@petervo Line 25 allows for 10.x ip addresses in my testing (192.x addresses have been unreachable). This solution will even try 192.x ip addresses (unreachable in my testing) in the case the ipv4_address method returns nil in the cockpit_url method.

I'm trying for the lowest common denominator and have verified that we can connect to Amazon, GCE, RHOS, RHEV and VMware virtual machines successfully.

If this isn't robust enough for more complex Amazon or , it can be addressed via provider overrides in a follow-up PR per @Fryguy's comment above [1].

[1] #15901 (review)

I meant 10.x private IPs which in theory would not be reachable.

But I agree this covers most use-cases and like you said it can always be adjusted later if needed

So, this is trying 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 if they're the only ipaddresses on the system?

Can this be:

ipaddresses.find { |ip| IPAddr.new(ip).ipv4? && !ip.starts_with?('192') }

@jrafanie, Yes, the ipv4_address tries for the most common scenarios (public to private) and falls back to ipaddresses.first if nothing matches. All 3 ip addresses above would be tried on their own (experienced while debugging and testing this PR...).

Updated the code per your request above, thanks!

Other than going to some remote system to ask what your public ip address is, I'm not sure what else we can do. I'm good with the changes as it seems to cover most of the bases, until it doesn't and we'll worry about it then.

miq-bot · 2017-09-01T16:57:31Z

This pull request is not mergeable. Please rebase and repush.

https://bugzilla.redhat.com/show_bug.cgi?id=1450109

https://bugzilla.redhat.com/show_bug.cgi?id=1451655

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

miq-bot · 2017-09-01T17:18:07Z

Checked commits bmclaughlin/manageiq@0ade159~...47bcea1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

h-kataria · 2017-09-01T18:01:24Z

verified fix in UI

jrafanie · 2017-09-01T18:19:37Z

LGTM, @petervo's test concern has been addressed, merging so we can merge the UI PR.

simaishi · 2017-11-13T19:43:35Z

@bmclaughlin I assume fine/yes ?

simaishi · 2017-11-20T16:08:46Z

@bmclaughlin ^ ping

bmclaughlin · 2017-12-04T18:14:32Z

@simaishi yes, thank you.

@miq-bot add_label fine/yes

bmclaughlin · 2017-12-05T12:03:28Z

@miq-bot remove_label fine/yes

This will require a Fine specific PR.

simaishi · 2017-12-07T22:36:47Z

Backported to Fine via #16609

miq-bot added bug core ui labels Aug 29, 2017

bmclaughlin mentioned this pull request Aug 29, 2017

Enable Cockpit console for RHOS provider ManageIQ/manageiq-ui-classic#2039

Merged

petervo suggested changes Aug 29, 2017

View reviewed changes

chessbyte reviewed Aug 29, 2017

View reviewed changes

app/models/vm/operations.rb Outdated

nil

else

ext_management_system.zone.remote_cockpit_ws_miq_server

end

Copy link

Member

chessbyte Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy/paste code

chessbyte reviewed Aug 29, 2017

View reviewed changes

Fryguy reviewed Aug 30, 2017

View reviewed changes

bmclaughlin force-pushed the enable-cockpit-for-cloud-providers branch 3 times, most recently from 1c81061 to 8f1cca0 Compare August 30, 2017 18:01

petervo reviewed Aug 30, 2017

View reviewed changes

miq-bot added the unmergeable label Sep 1, 2017

bmclaughlin added 8 commits September 1, 2017 13:10

Fix missing nil check

0ade159

https://bugzilla.redhat.com/show_bug.cgi?id=1450109

Make AWS and GCE connect on public ips for Web Console

41b564d

https://bugzilla.redhat.com/show_bug.cgi?id=1450109

Enable Web Console on RHOS provider

1e1e0b3

https://bugzilla.redhat.com/show_bug.cgi?id=1451655

Conflict resolution

ae845c6

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

Refactoring

dc1731f

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

Fix rubocop error

1ca5c49

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

Fix travis failures

106f7c5

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

Refactoring

47bcea1

https://bugzilla.redhat.com/show_bug.cgi?id=1450109 https://bugzilla.redhat.com/show_bug.cgi?id=1451655

bmclaughlin force-pushed the enable-cockpit-for-cloud-providers branch from 9a1dcc2 to 47bcea1 Compare September 1, 2017 17:15

miq-bot removed the unmergeable label Sep 1, 2017

jrafanie merged commit 24add7e into ManageIQ:master Sep 1, 2017

jrafanie added this to the Sprint 68 Ending Sep 4, 2017 milestone Sep 1, 2017

bmclaughlin deleted the enable-cockpit-for-cloud-providers branch September 1, 2017 19:59

chessbyte assigned jrafanie Sep 5, 2017

miq-bot added the fine/yes label Dec 4, 2017

miq-bot removed the fine/yes label Dec 5, 2017

bmclaughlin mentioned this pull request Dec 6, 2017

[FINE] Enable Web Console on RHOS provider #16609

Merged

simaishi added the fine/backported label Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix web console for AWS, GCE and enable for RHOS #15901

Fix web console for AWS, GCE and enable for RHOS #15901

bmclaughlin commented Aug 29, 2017 •

edited

petervo left a comment

petervo Aug 29, 2017

chessbyte Aug 29, 2017

chessbyte Aug 29, 2017

chessbyte Aug 29, 2017

Fryguy Aug 30, 2017

Fryguy commented Aug 30, 2017

bmclaughlin commented Aug 30, 2017 •

edited

petervo Aug 30, 2017

bmclaughlin Aug 30, 2017

petervo Aug 30, 2017

jrafanie Aug 31, 2017

bmclaughlin Aug 31, 2017

jrafanie Aug 31, 2017

miq-bot commented Sep 1, 2017

miq-bot commented Sep 1, 2017

h-kataria commented Sep 1, 2017

jrafanie commented Sep 1, 2017

simaishi commented Nov 13, 2017

simaishi commented Nov 20, 2017

bmclaughlin commented Dec 4, 2017

bmclaughlin commented Dec 5, 2017

simaishi commented Dec 7, 2017

Fix web console for AWS, GCE and enable for RHOS #15901

Fix web console for AWS, GCE and enable for RHOS #15901

Conversation

bmclaughlin commented Aug 29, 2017 • edited

petervo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Aug 30, 2017

bmclaughlin commented Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 1, 2017

miq-bot commented Sep 1, 2017

h-kataria commented Sep 1, 2017

jrafanie commented Sep 1, 2017

simaishi commented Nov 13, 2017

simaishi commented Nov 20, 2017

bmclaughlin commented Dec 4, 2017

bmclaughlin commented Dec 5, 2017

simaishi commented Dec 7, 2017

bmclaughlin commented Aug 29, 2017 •

edited

bmclaughlin commented Aug 30, 2017 •

edited