Resolve oVirt IP addresses #13767

jhernand · 2017-02-06T11:17:05Z

Since version 4 oVirt rejects connections that use directly the IP address instead of the host name. This is a side effect of the new SSO authentication mechanism. That means that when a oVirt provider is added using an IP address it will not work. Users don't do this frequently, but the system does it automatically when the provider is added using the discovery mechanism, as this mechanism uses IP addresses only. To avoid this issue this patch changes the provider so that when it receives an IP address it tries to convert it to the corresponding fully qualified host name.

There may be situations where the user really wants to use an IP address, and not the host name. For those cases this patch also introduces a new resolve_ip_addresses setting that can be used to disable resolving. This setting will be stored in the config/settings.yml file, and its default value will be true:

:ems:
  :ems_redhat:
    :resolve_ip_addresses: true

https://bugzilla.redhat.com/1417757

jhernand · 2017-02-06T11:22:21Z

@masayag @borod108 @durandom please review.

jhernand · 2017-02-06T14:48:30Z

CF fails to discover RHV 4.0 - IP issue

durandom

Overall nice documented PR.

2 minor issues - see comments.

Also, I think we should involve docs here, because it's not obvious whats going on and we introduce a new setting.

@oourfali do you know how to trigger docs?

durandom · 2017-02-07T08:53:15Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+  # @return [String] The host name.
+  #
+  def resolve_ip_address(address)
+    require 'resolv'


I dont think you need this because you are using stdlib stuff - but then I see it other places too.
@chrisarcand is there a reason to require here? like we do `require 'ostruct' here

Not sure about the rails environment, but in plain Ruby this fails without the require, even if resolv is part of the stdlib.

You do need a require under normal circumstances as while it's part of stdlib, it isn't loaded by default.

The module is present in our current Rails env, it's not clear as to what actually adds it, probably Rails for it's own purposes. So...

It's appropriate to require it in this file as it's a required dependency, but I'd put it at the top of the file instead of within the method itself.

OK. I will put it at the top of the file.

Note that in other cases I have seen the opposite comment: put the requires only where they are needed, not at the top of file. Is there a rationale or common rule about where should requires go?

If there is a hard-and-fast rule, usually it's what we described here: put at the top of the file.

There are specific exceptions, though; For example, in the ovirt_metrics gem I needed to write a custom adapter for dealing with an old version of Postgres on Ovirt databases that isn't supported by Rails 5 (what we run). As the adapter isn't required for Rails < 5, I require it inline based on which version of Rails you're running. I don't want the adapter required when it isn't necessary (it's very large and expensive to load)

Here it looks like you're adding some core functionality that should always run, so we'll consider it a true dependency that we'll specify at the beginning of the file.

Forgot my reference above: https://github.com/ManageIQ/ovirt_metrics/blob/b99861dd1ee52e4717698c470eb6aa5884dea55a/lib/ovirt_metrics.rb#L31-L32

OK, thanks for the explanation @chrisarcand , I will move the require to the top of the file in the next version of the pull request.

durandom · 2017-02-07T08:58:41Z

config/settings.yml

@@ -97,6 +97,7 @@
    :purge_window_size: 10000
 :ems:
  :ems_redhat:
+    :resolve_ip_addresses: true


I think this setting is not really self explanatory. But I dont know how could be really short and good.

resolve_ip_address_for_ovirt_manager_connect: true ?

Can this be commented? I didn't see comments in this file, but if possible, I'd like to write a comment explaining the meaning. Something like this:

# # Indicates if the oVirt provider should try to convert IP addresses # to host names, using a reverse DNS lookup if required. This is # needed because since version 4 of oVirt connections that use # an IP address directly are rejected. Change to 'false' only if you # have explicitly configured the oVirt engine to use IP addresses # and you don't have a working DNS configuration. # :resolve_ip_addresses: true

Assuming comments are allowed, would that be enough?

It's possible, but they dont show up in the UI :(

Thats why I think we should a) do a corresponding doc update. I guess the docs request in the BZ is enough. Though it would be nice to involve the docs team right here on the PR.
@adahms ?

@oourfali what do you think about the naming of this config option?

@durandom - Thank you for adding me to the discussion. If the change is visible in the UI, which it looks like it will be, then we can add a step and a note to the procedure on adding RHV providers to call out this behaviour.

Would that work?

@adahms what is the process to add that text to the providers guide? Sending a pull request to some GitHub repository? Where? Will you take care of it or should I?

@jhernand I have now raised the following bug to address this request, and the content will be added in the upstream and sync'ed with the downstream -

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

Graeat, thanks @adahms. @durandom anything else before merging this pull request?

@jhernand @adahms thanks for working this out and making sure it gets proper attention from docs.
I think its good to go now 👍

@jhernand - Nothing else from documentation. Thank you for calling out this change!

jhernand · 2017-02-07T09:34:21Z

@durandom I requested a release note in the bug. Please review it and tell me if something else is needed in order to trigger docs.

jhernand · 2017-02-07T16:35:08Z

The only change in the new version of the pull request is moving the require 'resolv' to the top of the file.

durandom · 2017-02-07T19:21:06Z

cool, thanks @jhernand - now waiting for @oourfali thoughts on the config setting name and the docs.

durandom · 2017-02-13T10:59:47Z

@jhernand could you re-base on current master, amend the last commit or close/open the PR to trigger the CI?

Since version 4 oVirt rejects connections that use directly the IP address instead of the host name. This is a side effect of the new SSO authentication mechanism. That means that when a oVirt provider is added using an IP address it will not work. Users don't do this frequently, but the system does it automatically when the provider is added using the discovery mechanism, as this mechanism uses IP addresses only. To avoid this issue this patch changes the provider so that when it receives an IP address it tries to convert it to the corresponding fully qualified host name. There may be situations where the user really wants to use an IP address, and not the host name. For those cases this patch also introduces a new 'resolve_ip_addresses' setting that can be used to disable resolving. This setting will be stored in the 'config/settings.yml' file, and its default value will be 'true': :ems: :ems_redhat: :resolve_ip_addresses: true https://bugzilla.redhat.com/1417757

jhernand · 2017-02-13T11:03:34Z

Rebased, no changes.

miq-bot · 2017-02-13T11:07:23Z

Checked commit https://github.com/jhernand/manageiq/commit/8df5bb0ce29b51928ecee75978e30ea80603e582 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 👍

jhernand · 2017-02-15T08:58:12Z

@miq-bot add_label euwe/yes

simaishi · 2017-03-03T18:58:46Z

@jhernand This isn't a clean cherry-pick due to some refactoring done on master branch. Can you make an Euwe-specific PR (referencing this one) or suggest other PRs to backport.

jhernand · 2017-03-04T12:15:14Z

@simaishi Yes, I'll prepare an Euwe-specific PR.

jhernand · 2017-03-04T12:38:54Z

@simaishi Here is the back-port for Euwe: #14183. Please don't merge it yet. I will verify that it works correctly in the Euwe branch and then I will let you know.

[EUWE] Resolve oVirt IP addresses (backport of #13767)

simaishi · 2017-03-14T13:50:57Z

Backported to Euwe via #14183

chessbyte added enhancement providers/rhevm labels Feb 6, 2017

chessbyte assigned blomquisg Feb 6, 2017

durandom suggested changes Feb 7, 2017

View reviewed changes

blomquisg merged commit bf9967a into ManageIQ:master Feb 14, 2017

blomquisg added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 14, 2017

jhernand deleted the resolve_ovirt_ip_addresses branch February 14, 2017 14:27

miq-bot added the euwe/yes label Feb 15, 2017

simaishi added the euwe/conflict label Mar 3, 2017

jhernand mentioned this pull request Mar 4, 2017

[EUWE] Resolve oVirt IP addresses (backport of ManageIQ/manageiq#13767) #14183

Merged

simaishi removed the euwe/conflict label Mar 10, 2017

simaishi added a commit that referenced this pull request Mar 14, 2017

Merge pull request #14183 from jhernand/resolve_ovirt_ip_addresses_euwe

0cd3223

[EUWE] Resolve oVirt IP addresses (backport of #13767)

simaishi added euwe/backported and removed euwe/yes labels Mar 14, 2017

Fryguy added the providers/infrastructure label Jan 24, 2020

Fryguy unassigned blomquisg Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve oVirt IP addresses #13767

Resolve oVirt IP addresses #13767

jhernand commented Feb 6, 2017 •

edited

Loading

jhernand commented Feb 6, 2017

jhernand commented Feb 6, 2017

durandom left a comment

durandom Feb 7, 2017

jhernand Feb 7, 2017

chrisarcand Feb 7, 2017

jhernand Feb 7, 2017

chrisarcand Feb 7, 2017

chrisarcand Feb 7, 2017

jhernand Feb 7, 2017

durandom Feb 7, 2017

jhernand Feb 7, 2017

durandom Feb 7, 2017

durandom Feb 7, 2017

adahms Feb 8, 2017

jhernand Feb 9, 2017

adahms Feb 13, 2017

jhernand Feb 13, 2017

durandom Feb 13, 2017

adahms Feb 14, 2017

jhernand commented Feb 7, 2017

jhernand commented Feb 7, 2017

durandom commented Feb 7, 2017

durandom commented Feb 13, 2017

jhernand commented Feb 13, 2017

miq-bot commented Feb 13, 2017

jhernand commented Feb 15, 2017

simaishi commented Mar 3, 2017

jhernand commented Mar 4, 2017

jhernand commented Mar 4, 2017

simaishi commented Mar 14, 2017

Resolve oVirt IP addresses #13767

Resolve oVirt IP addresses #13767

Conversation

jhernand commented Feb 6, 2017 • edited Loading

jhernand commented Feb 6, 2017

jhernand commented Feb 6, 2017

durandom 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

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

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

jhernand commented Feb 7, 2017

jhernand commented Feb 7, 2017

durandom commented Feb 7, 2017

durandom commented Feb 13, 2017

jhernand commented Feb 13, 2017

miq-bot commented Feb 13, 2017

jhernand commented Feb 15, 2017

simaishi commented Mar 3, 2017

jhernand commented Mar 4, 2017

jhernand commented Mar 4, 2017

simaishi commented Mar 14, 2017

jhernand commented Feb 6, 2017 •

edited

Loading