Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 14, 2017
Merged

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

merged 1 commit into from
Mar 14, 2017

Conversation

jhernand
Copy link
Contributor

@jhernand jhernand commented Mar 4, 2017

This is a back-port of #13767.

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
Copy link
Contributor Author

jhernand commented Mar 4, 2017

@simaishi Please don't merge yet, as I still need to verify that it works correctly in the Euwe branch. I will let you know when it is verified.

@miq-bot miq-bot changed the title Resolve oVirt IP addresses (backport of ManageIQ/manageiq#13767) [EUWE] Resolve oVirt IP addresses (backport of ManageIQ/manageiq#13767) Mar 4, 2017
@oourfali
Copy link

oourfali commented Mar 6, 2017

@durandom who can merge this one? Shall we re-trigger CI?

@oourfali
Copy link

oourfali commented Mar 6, 2017

@jhernand were you able to verify it works?

@jhernand
Copy link
Contributor Author

jhernand commented Mar 6, 2017

The Travis failures seem unrelated to me. I will close and re-open to trigger Travis again. I will complete the verification today.

@jhernand jhernand closed this Mar 6, 2017
@jhernand jhernand reopened this Mar 6, 2017
@jhernand
Copy link
Contributor Author

jhernand commented Mar 6, 2017

I have verified that this works correctly. However there are now Travis failures that seem related. I am working on that, so please don't merge yet.

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
@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2017

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

@jhernand
Copy link
Contributor Author

jhernand commented Mar 6, 2017

@jhernand
Copy link
Contributor Author

jhernand commented Mar 7, 2017

@simaishi I have addressed all the test issues that are related to the patch, and rebased to use latest tip of the Euwe branch, but there are still Travis build failures. To verify that they aren't related I ran the tests in my own clone of the repository, without my changes, and it is also failing:

https://travis-ci.org/jhernand/manageiq/jobs/208505484

Do you have any suggestion on how to move forward?

@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2017

@jhernand The travis failures in Euwe are currently being worked on. I'll re-run the job once they're fixed, so no need to worry about them for now.

@simaishi simaishi requested review from blomquisg and durandom March 7, 2017 13:32
@jhernand
Copy link
Contributor Author

jhernand commented Mar 7, 2017

Great, thanks @simaishi.

@simaishi
Copy link
Contributor

@blomquisg @durandom is this good to go?

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

Looks good.

@jhernand could you add what you had to change for the EUWE backport. To me it looks just ip adresses in the specs...

@jhernand
Copy link
Contributor Author

Correct @durandom, the only changes are in the specs. In the master branch the specs for the refresh of version 3.0 of oVirt have been removed, and the rest of them have been moved to a refresh directory. So in the euwe branch I had to modify the oVirt 3.0 specs, and move the other ones to the old directory. Do you want this information here or in a commit message?

@durandom
Copy link
Member

@jhernand here is fine. If somebody wonders he'll end up here too... thanks

@simaishi simaishi merged commit 0cd3223 into ManageIQ:euwe Mar 14, 2017
@simaishi simaishi added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants