Elasticsearch 5 Support Breaks Reconfigure on Redirects

[sdelano]

Preliminary Elasticsearch 5 support added in #1287 breaks reconfigure in OWCA-configured systems. This was discovered when testing the latest packages of Chef and Automate in OWCA. OWCA is configured to the use the Automate-provided Elasticsearch as external search for Chef Server.

It's currently configured as such:

opscode_solr4["external_url"] = "http://localhost:8080/elasticsearch"

In this case, http://localhost:8080/elasticsearch redirects to http://localhost:8080/elasticsearch/, causing the reconfigure to fail when parsing JSON because it's actually parsing the HTML of the redirect message.

Yes, configuring the external search endpoint to go to http://localhost:8080/elasticsearch/ will fix this problem, and I've asked that Amazon do as much, but I believe that our tooling could behave better in the face of common configurations that otherwise results in a completely functional system (e.g. search still works when configured this way, so this helper should work as well).

[sdelano]

I feel like there's an http client within Chef that we could be using that will follow redirects instead of using raw Net::HTTP.

[sdelano]

Looks like Chef::HTTP does just this:

chef (12.19.36)> http = Chef::HTTP.new("http://localhost:8080/elasticsearch")
 => #<Chef::HTTP:0x00000006707c80 @url="http://localhost:8080/elasticsearch", @default_headers={}, @sign_on_redirect=true, @redirects_followed=0, @redirect_limit=10, @keepalives=false, @options={}, @middlewares=[]> 
chef (12.19.36)> http.get("")
 => "{\n  \"name\" : \"Skids\",\n  \"cluster_name\" : \"chef-insights\",\n  \"cluster_uuid\" : \"b0H5j-W9TGibALC5xqRAiQ\",\n  \"version\" : {\n    \"number\" : \"2.4.1\",\n    \"build_hash\" : \"c67dc32e24162035d18d6fe1e952c4cbcbe79d16\",\n    \"build_timestamp\" : \"2016-09-27T18:57:55Z\",\n    \"build_snapshot\" : false,\n    \"lucene_version\" : \"5.5.2\"\n  },\n  \"tagline\" : \"You Know, for Search\"\n}\n" 
chef (12.19.36)> 

[ksubrama]

I knew I was missing an abstraction layer somewhere.

[stevendanna]

👍 on using Chef::HTTP

[sdelano]

It looks like this has been addressed as part of PR #1309, but we should maybe put some further thought into what the correct HTTP library is. Perhaps even going so far as checking what the client team recommends. After reading the code a bit this morning, Chef::HTTP appears to be closely related to making actual Chef API requests. I was attempting to look into the retry logic (it has some built in, so at the very least 1309 is duplicating that logic to some extent.

My suggestions:

• decide between using Chef::HTTP and Chef::HTTP::BasicClient or another variant
• add tests for retryable requests
• add tests for redirects (redirects were addressed but there were no tests added)

[danielsdeleo]

Chef::HTTP::BasicClient is designed to be an implementation detail for Chef::HTTP; Chef::HTTP is the lowest level thing that folks should be using. Due to the history and compat constraints when I refactored the mess that was Chef::REST, Chef::HTTP probably has some behaviors that are somewhat specific to how Chef talks to Chef Server, but things like authentication, gzip, JSON request/response, etc. are factored out. Chef::ServerAPI is a subclass of Chef::HTTP that includes those behaviors; if you need to mix and match, you can make a subclass of Chef::HTTP and pull in the things you want.

[lancewf]

@sdelano I do not see any redirect tests for Chef::HTTP in the Chef repo. We can create some there. I do not see a way to test the redirects for the changes in PR #1309 without just testing Chef::HTTP.

I do see that we can add some tests for the retryable portion for this PR.

[lancewf]

I have added these tests for the retryable portion of the code.
#1316