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

Add cookies and basic_auth_eager options #776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simmel
Copy link

@simmel simmel commented May 24, 2018

Make automatic cookie management and to use eager/preemptive Basic
Authentication configurable.

Eager/preemptive Basic Auth is convenient but in reality you're doing authentication for every request we're doing and that can be 10s or even 100s per second. That might be fine if you use an internal to Elasticsearch authentication service but if you depend on external services such as Kerberos, LDAP et.al. you will get a lot of more pressure on those services.

I've modified some tests to use my eager setting but there are no tests added at all for cookies. I've tried for a couple of hours but I don't get anywhere since I'm kind of new to rspec and testing.
I'm more than willing to do it but I need assistance and guidance in writing said tests.

@webmat
Copy link
Contributor

webmat commented May 28, 2018

Hi @simmel, first of all, thanks for attempting this.

You can find some pointers on RSpec here: https://relishapp.com/rspec/. I find it's more understandable than the "official" gem documentation on rubydoc.info :-)

Tests around the HTTP connection pool are delicate & tricky, a lot of stuff needs to be faked and neutered (aka mocking). So you're tackling a big chunk. Note that when you're faking things with matches like receive(:method_call), the original :method_call method will be replaced by a test method that only tests whether it was called (or if it was called with expected params). But the original method itself will not actually be called.

Accordingly, an approach you could take to test the workflow of attempting to connect with eager=false is to split the steps in distinct tests. I'm not familiar with the "non-eager" http auth workflow, so maybe my proposed steps here will sound funny, haha. But the point is how each step in the workflow is tested independently:

  1. Attempt to connect without auth, fake the response requesting credentials, check that your code then retries with the auth.
  2. Attempt to connect with basic auth, fake the response setting a cookie, check that your code saves the cookie.
  3. Attempt to connect when the cookie is saved in memory, check that the request will actually send the cookie with the request.
  4. Attempt to connect when an expired cookie is saved in memory, fake the response requesting credentials, check that your code retries with the auth.

Hope this makes sense. I know RSpec looks like magic. It's a double edged sword: really nice when you're used to it, but pretty opaque otherwise ;-)

Final note: the failing test in Travis CI had nothing to do with your code. We sometimes experience flakiness with this error. I re-ran the tests and everything was fine.

@simmel simmel force-pushed the add_cookie_and_eager_options branch 2 times, most recently from 6864955 to ac64a84 Compare June 15, 2018 13:34
@simmel simmel force-pushed the add_cookie_and_eager_options branch 3 times, most recently from 5ddee94 to d9b3dde Compare July 3, 2018 08:51
Make automatic cookie management and to use eager/preemptive Basic
Authentication configurable.
@simmel simmel force-pushed the add_cookie_and_eager_options branch from d9b3dde to 32770ae Compare July 3, 2018 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants