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

Support for using the persistent-auth #42

Closed
wants to merge 2 commits into from

Conversation

iNecas
Copy link
Contributor

@iNecas iNecas commented Aug 11, 2014

With persisted_auth set to true, we can by-pass the full ovirt authentication,
which can rapidly speed-up the response times
(at least when running against some external authentication).

In my setup, it is 4x faster with this feature turned on. For more details see
http://www.ovirt.org/Features/RESTSessionManagement.

@lzap
Copy link
Collaborator

lzap commented Aug 12, 2014

Ivan, we definitely need to cover this with integration tests. See to README how to set things up and you can use this bats suite to install ovirt all-in-one setup.

If you use the bats suite, please give the PR 👍 as nobody is willing to test it for some reason :-)

theforeman/foreman-bats#34

Codewise this WFM

@@ -144,7 +138,12 @@ def http_delete(suburl)
def auth_header
# This is the method for strict_encode64:
encoded_credentials = ["#{@credentials[:username]}:#{@credentials[:password]}"].pack("m0").gsub(/\n/,'')
{ :authorization => "Basic " + encoded_credentials }
headers = { :authorization => "Basic " + encoded_credentials }
if persistent_auth
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to add some ovirt version tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will just get ignored by older versions and the basic auth will continue to work: it will just not return the jsessionid at the end, so the basic auth will continue to be used for those cases.

neoseele pushed a commit to neoseele/rbovirt that referenced this pull request Sep 8, 2014
Remove details from vms api call headers to avoid timeout
@jhernand
Copy link
Collaborator

It is better if you avoid using explicitly the JSESSIONID name, as that may change in the server side in the future. Instead of that try to handle correctly all the cookies returned by the server, whatever they are named (not sure if this is possible with the REST client that rbovirt uses).

Also, when implementing this kind of persistent authentication, it is very important to provide a mechanism that can be used to dispose the server side session, otherwise sessions will potentially accumulate in the server and eventually exhaust its resources. Other oVirt API bindings include a disconnect method that takes care of this. This method should send a request to the server, with the session identifier (currently the JSESSIONID cookie), the Prefer: persistent-auth header but no Authorization header:

HEAD /ovirt-engine/api HTTP/1.1
Cookie: JSESSIONID=...
Prefer: persistent-auth

(Note that there is no Authorization header in this request.)

Using the HEAD method for this final request is good because it avoids the transfer of the body of the root resource, which is useless in this case.

@epienbroek
Copy link
Contributor

I just tried this PR and added an initial testcase for it in 29e4501.
During testing I also discovered a copy/paste typo which can be fixed by aa5c1ac
Both changes can be found in my branch @ https://github.com/epienbroek/rbovirt/commits/persistent_test
Feel free to cherry-pick them and add them to this PR

@lzap
Copy link
Collaborator

lzap commented Apr 15, 2015

@iNecas ping ^^ can you incorporate and rebase? Interesting patch, nice one.

@lzap
Copy link
Collaborator

lzap commented Mar 8, 2016

Keep in mind that if we want this to fix Foreman, we need to backport this into 0.0.3x series as the 0.1 version is not compatible with Fog/Foreman.

@lzap
Copy link
Collaborator

lzap commented Mar 11, 2016

It is worth adding a test.

With persisted_auth set to true, we can by-pass the full ovirt
authentication, which can rapidly speed-up the response times
(at least when running against some external authentication).

In my setup, it is 4x faster with this feature turned on. For more
details see http://www.ovirt.org/Features/RESTSessionManagement.
…m when testing

There was also a problem with waiting for a vm to stop before starting.
@iNecas
Copy link
Contributor Author

iNecas commented Mar 11, 2016

Tests added (thanks @epienbroek). I've also added another commit to fix the #100

@lzap
Copy link
Collaborator

lzap commented Mar 17, 2016

This is great work Ivan, I am gonna to test this now.

@lzap
Copy link
Collaborator

lzap commented Mar 17, 2016

Tests passed, except these two, but you fixed bunch of others so this is PASS :-)

Failures:

  1) Admin API VM Life cycle admin basic vm and templates operations behaves like Basic VM Life cycle test_should_start_with_cloudinit
     Failure/Error: @client.vm_start_with_cloudinit(@vm.id, user_data)
     OVIRT::OvirtException:
       Cannot run VM because the VM is in Powering Down status.
     Shared Example Group: "Basic VM Life cycle" called from ./spec/integration/vm_crud_spec.rb:130
     # ./lib/rbovirt.rb:207:in `handle_fault'
     # ./lib/rbovirt.rb:127:in `rescue in http_post'
     # ./lib/rbovirt.rb:124:in `http_post'
     # ./lib/client/vm_api.rb:150:in `vm_start_with_cloudinit'
     # ./spec/integration/vm_crud_spec.rb:47:in `block (2 levels) in <top (required)>'

  2) User API VM Life cycle user basic vm and templates operations behaves like Basic VM Life cycle test_should_start_with_cloudinit
     Failure/Error: @client.vm_start_with_cloudinit(@vm.id, user_data)
     OVIRT::OvirtException:
       Cannot run VM because the VM is in Powering Down status.
     Shared Example Group: "Basic VM Life cycle" called from ./spec/integration/vm_crud_spec.rb:152
     # ./lib/rbovirt.rb:207:in `handle_fault'
     # ./lib/rbovirt.rb:127:in `rescue in http_post'
     # ./lib/rbovirt.rb:124:in `http_post'
     # ./lib/client/vm_api.rb:150:in `vm_start_with_cloudinit'
     # ./spec/integration/vm_crud_spec.rb:47:in `block (2 levels) in <top (required)>'

Merging as well into 0.0.x series.

@lzap
Copy link
Collaborator

lzap commented Mar 17, 2016

Merged as cf4e205, thank you!

@lzap lzap closed this Mar 17, 2016
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.

None yet

5 participants