Merge pull request #6284 from chef/lcg/versioned-posts

fix retries on JSON POST requests

6 files changed, 110 insertions, 16 deletions

diff --git a/Gemfile b/Gemfile
index 7ce6a9c57d..b1fa640db9 100644
--- a/Gemfile
+++ b/Gemfile
@@ -50,6 +50,7 @@ end
 group(:development, :test) do
   gem "rake"
   gem "simplecov"
+  gem "webmock"
 
   # for testing new chefstyle rules
   # gem 'chefstyle', github: 'chef/chefstyle'
diff --git a/Gemfile.lock b/Gemfile.lock
index ef9f833290..ebbe0012ef 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -122,6 +122,7 @@ GEM
     addressable (2.4.0)
     appbundler (0.10.0)
       mixlib-cli (~> 1.4)
+      mixlib-shellout (~> 2.0)
     ast (2.3.0)
     backports (3.8.0)
     binding_of_caller (0.7.2)
@@ -140,6 +141,8 @@ GEM
       net-ssh
     coderay (1.1.1)
     concurrent-ruby (1.0.5)
+    crack (0.4.3)
+      safe_yaml (~> 1.0.0)
     debug_inspector (0.0.3)
     diff-lcs (1.3)
     docile (1.1.5)
@@ -175,6 +178,7 @@ GEM
       ffi (>= 1.0.1)
     gyoku (1.3.1)
       builder (>= 2.1.2)
+    hashdiff (0.3.4)
     hashie (3.5.6)
     highline (1.7.8)
     htmlentities (4.3.4)
@@ -321,6 +325,7 @@ GEM
     ruby-shadow (2.5.0)
     rubyntlm (0.6.2)
     rubyzip (1.2.1)
+    safe_yaml (1.0.4)
     sawyer (0.8.1)
       addressable (>= 2.3.5, < 2.6)
       faraday (~> 0.8, < 1.0)
@@ -373,6 +378,10 @@ GEM
       thread_safe (~> 0.1)
     unicode-display_width (1.3.0)
     uuidtools (2.1.5)
+    webmock (3.0.1)
+      addressable (>= 2.3.6)
+      crack (>= 0.3.2)
+      hashdiff
     websocket (1.2.4)
     win32-api (1.5.3-universal-mingw32)
     win32-dir (0.5.1)
@@ -440,7 +449,8 @@ DEPENDENCIES
   simplecov
   tomlrb
   travis
+  webmock
   yard
 
 BUNDLED WITH
-   1.15.1
+   1.15.2
diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index c741dcca97..14dd8b93a5 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -5,7 +5,7 @@
 # Author:: Christopher Brown (<cb@chef.io>)
 # Author:: Christopher Walters (<cw@chef.io>)
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2009-2016 Chef Software, Inc.
+# Copyright:: Copyright 2009-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -144,9 +144,9 @@ class Chef
     def request(method, path, headers = {}, data = false)
       http_attempts ||= 0
       url = create_url(path)
-      method, url, headers, data = apply_request_middleware(method, url, headers, data)
+      processed_method, url, processed_headers, processed_data = apply_request_middleware(method, url, headers, data)
 
-      response, rest_request, return_value = send_http_request(method, url, headers, data)
+      response, rest_request, return_value = send_http_request(processed_method, url, processed_headers, processed_data)
       response, rest_request, return_value = apply_response_middleware(response, rest_request, return_value)
 
       response.error! unless success_response?(response)
@@ -157,7 +157,6 @@ class Chef
       response = e.response
       if response.kind_of?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
         Chef::Log.debug("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
-        sleep(http_retry_delay)
         retry
       else
         raise
@@ -176,11 +175,12 @@ class Chef
       url = create_url(path)
       response, rest_request, return_value = nil, nil, nil
       tempfile = nil
+      data = nil
 
       method = :GET
-      method, url, headers, data = apply_request_middleware(method, url, headers, data)
+      method, url, processed_headers, data = apply_request_middleware(method, url, headers, data)
 
-      response, rest_request, return_value = send_http_request(method, url, headers, data) do |http_response|
+      response, rest_request, return_value = send_http_request(method, url, processed_headers, data) do |http_response|
         if http_response.kind_of?(Net::HTTPSuccess)
           tempfile = stream_to_tempfile(url, http_response, &progress_block)
         end
@@ -196,7 +196,6 @@ class Chef
       response = e.response
       if response.kind_of?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
         Chef::Log.debug("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
-        sleep(http_retry_delay)
         retry
       else
         raise
@@ -223,11 +222,12 @@ class Chef
       url = create_url(path)
       response, rest_request, return_value = nil, nil, nil
       tempfile = nil
+      data = nil
 
       method = :GET
-      method, url, headers, data = apply_request_middleware(method, url, headers, data)
+      method, url, processed_headers, data = apply_request_middleware(method, url, headers, data)
 
-      response, rest_request, return_value = send_http_request(method, url, headers, data) do |http_response|
+      response, rest_request, return_value = send_http_request(method, url, processed_headers, data) do |http_response|
         if http_response.kind_of?(Net::HTTPSuccess)
           tempfile = stream_to_tempfile(url, http_response)
         end
@@ -252,7 +252,6 @@ class Chef
       response = e.response
       if response.kind_of?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
         Chef::Log.debug("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
-        sleep(http_retry_delay)
         retry
       else
         raise
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 1a897ef95f..bace94fcbe 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software, Inc.
+# Copyright:: Copyright 2008-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -30,6 +30,8 @@ $:.unshift File.expand_path("../..", __FILE__)
 require "rubygems"
 require "rspec/mocks"
 
+require "webmock/rspec"
+
 $:.unshift(File.join(File.dirname(__FILE__), "..", "lib"))
 $:.unshift(File.expand_path("../lib", __FILE__))
 $:.unshift(File.dirname(__FILE__))
@@ -215,6 +217,11 @@ RSpec.configure do |config|
   config.run_all_when_everything_filtered = true
 
   config.before(:each) do
+    # it'd be nice to run this with connections blocked or only to localhost, but we do make lots
+    # of real connections, so cannot.  we reset it to allow connections every time to avoid
+    # tests setting connections to be disabled and that state leaking into other tests.
+    WebMock.allow_net_connect!
+
     Chef.reset!
 
     Chef::ChefFS::FileSystemCache.instance.reset!
diff --git a/spec/unit/http/api_versions_spec.rb b/spec/unit/http/api_versions_spec.rb
index 91d46763c2..2ccb847acc 100644
--- a/spec/unit/http/api_versions_spec.rb
+++ b/spec/unit/http/api_versions_spec.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2017, Chef Software, Inc.
+# Copyright:: Copyright 2017-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -27,7 +27,7 @@ describe Chef::HTTP::APIVersions do
   end
 
   let(:method) { "GET" }
-  let(:url) { "http://dummy.com" }
+  let(:url) { "http://localhost:60123" }
   let(:headers) { {} }
   let(:data) { false }
 
@@ -53,8 +53,11 @@ describe Chef::HTTP::APIVersions do
     m
   end
 
+  let(:client) do
+    TestVersionClient.new(url, { version_class: VersionedClassVersions })
+  end
+
   let(:middleware) do
-    client = TestVersionClient.new(url)
     client.middlewares[0]
   end
 
diff --git a/spec/unit/server_api_spec.rb b/spec/unit/server_api_spec.rb
index 05d2a28ed4..3f1d9b0e90 100644
--- a/spec/unit/server_api_spec.rb
+++ b/spec/unit/server_api_spec.rb
@@ -26,12 +26,22 @@ b857vWviwPX2/P6+E3GPdl8IVsKXCvGWOBZWTuNTjQtwbDzsUepWoMgXnlQJSn5I
 YSlLxQKBgQD16Gw9kajpKlzsPa6XoQeGmZALT6aKWJQlrKtUQIrsIWM0Z6eFtX12
 2jjHZ0awuCQ4ldqwl8IfRogWMBkHOXjTPVK0YKWWlxMpD/5+bGPARa5fir8O1Zpo
 Y6S6MeZ69Rp89ma4ttMZ+kwi1+XyHqC/dlcVRW42Zl5Dc7BALRlJjQ==
------END RSA PRIVATE KEY-----"
+-----END RSA PRIVATE KEY-----".freeze
 
 describe Chef::ServerAPI do
   let(:url) { "http://chef.example.com:4000" }
   let(:key_path) { "/tmp/foo" }
 
+  let(:client) do
+    Chef::ServerAPI.new(url)
+  end
+
+  before do
+    Chef::Config[:node_name] = "silent-bob"
+    Chef::Config[:client_key] = CHEF_SPEC_DATA + "/ssl/private_key.pem"
+    Chef::Config[:http_retry_delay] = 0
+  end
+
   describe "#initialize" do
     it "uses the configured key file" do
       allow(IO).to receive(:read).with(key_path).and_return(SIGNING_KEY_DOT_PEM)
@@ -47,4 +57,68 @@ describe Chef::ServerAPI do
       expect(api.options[:raw_key]).to eql(SIGNING_KEY_DOT_PEM)
     end
   end
+
+  context "versioned apis" do
+    class VersionedClassV0
+      extend Chef::Mixin::VersionedAPI
+      minimum_api_version 0
+    end
+
+    class VersionedClassV2
+      extend Chef::Mixin::VersionedAPI
+      minimum_api_version 2
+    end
+
+    class VersionedClassVersions
+      extend Chef::Mixin::VersionedAPIFactory
+      add_versioned_api_class VersionedClassV0
+      add_versioned_api_class VersionedClassV2
+    end
+
+    before do
+      Chef::ServerAPIVersions.instance.reset!
+    end
+
+    let(:versioned_client) do
+      Chef::ServerAPI.new(url, version_class: VersionedClassVersions)
+    end
+
+    it "on protocol negotiation it posts the same message body without doubly-encoding the json string" do
+      WebMock.disable_net_connect!
+      post_body = { bar: "baz" }
+      body_406 = '{"error":"invalid-x-ops-server-api-version","message":"Specified version 2 not supported","min_version":0,"max_version":1}'
+      stub_request(:post, "http://chef.example.com:4000/foo").with(body: post_body.to_json, headers: { "X-Ops-Server-Api-Version" => "2" }).to_return(status: [406, "Not Acceptable"], body: body_406 )
+      stub_request(:post, "http://chef.example.com:4000/foo").with(body: post_body.to_json, headers: { "X-Ops-Server-Api-Version" => "0" }).to_return(status: 200, body: "", headers: {})
+      versioned_client.post("foo", post_body)
+    end
+  end
+
+  context "retrying normal requests" do
+    it "500 on a post retries and posts correctly " do
+      WebMock.disable_net_connect!
+      post_body = { bar: "baz" }
+      headers = { "Accept" => "application/json", "Content-Type" => "application/json", "Accept-Encoding" => "gzip;q=1.0,deflate;q=0.6,identity;q=0.3", "Content-Length" => "13", "Host" => "chef.example.com:4000", "X-Chef-Version" => Chef::VERSION, "X-Ops-Sign" => "algorithm=sha1;version=1.1;", "X-Ops-Userid" => "silent-bob" }
+      stub_request(:post, "http://chef.example.com:4000/foo").with(body: post_body.to_json, headers: headers).to_return(status: [500, "Internal Server Error"])
+      stub_request(:post, "http://chef.example.com:4000/foo").with(body: post_body.to_json, headers: headers).to_return(status: 200, body: "", headers: {})
+      client.post("foo", post_body)
+    end
+
+    it "500 on a put retries and puts correctly " do
+      WebMock.disable_net_connect!
+      put_body = { bar: "baz" }
+      headers = { "Accept" => "application/json", "Content-Type" => "application/json", "Accept-Encoding" => "gzip;q=1.0,deflate;q=0.6,identity;q=0.3", "Content-Length" => "13", "Host" => "chef.example.com:4000", "X-Chef-Version" => Chef::VERSION, "X-Ops-Sign" => "algorithm=sha1;version=1.1;", "X-Ops-Userid" => "silent-bob" }
+      stub_request(:put, "http://chef.example.com:4000/foo").with(body: put_body.to_json, headers: headers).to_return(status: [500, "Internal Server Error"])
+      stub_request(:put, "http://chef.example.com:4000/foo").with(body: put_body.to_json, headers: headers).to_return(status: 200, body: "", headers: {})
+      client.put("foo", put_body)
+    end
+
+    it "500 on a get retries and gets correctly " do
+      WebMock.disable_net_connect!
+      get_body = { bar: "baz" }
+      headers = { "Accept" => "application/json", "Accept-Encoding" => "gzip;q=1.0,deflate;q=0.6,identity;q=0.3", "Host" => "chef.example.com:4000", "X-Chef-Version" => Chef::VERSION, "X-Ops-Sign" => "algorithm=sha1;version=1.1;", "X-Ops-Userid" => "silent-bob" }
+      stub_request(:get, "http://chef.example.com:4000/foo").with(headers: headers).to_return(status: [500, "Internal Server Error"])
+      stub_request(:get, "http://chef.example.com:4000/foo").with(headers: headers).to_return(status: 200, body: "", headers: {})
+      client.get("foo")
+    end
+  end
 end