Retry API requests if using an unsupported versiontm/retryable_api

Signed-off-by: Thom May <thom@chef.io>

8 files changed, 102 insertions, 6 deletions

diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index 12acae953c..c741dcca97 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -142,13 +142,26 @@ class Chef
     # Makes an HTTP request to +path+ with the given +method+, +headers+, and
     # +data+ (if applicable).
     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)
 
       response, rest_request, return_value = send_http_request(method, url, headers, data)
       response, rest_request, return_value = apply_response_middleware(response, rest_request, return_value)
+
       response.error! unless success_response?(response)
       return_value
+
+    rescue Net::HTTPServerException => e
+      http_attempts += 1
+      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
+      end
     rescue Exception => exception
       log_failed_request(response, return_value) unless response.nil?
 
@@ -159,6 +172,7 @@ class Chef
     end
 
     def streaming_request_with_progress(path, headers = {}, &progress_block)
+      http_attempts ||= 0
       url = create_url(path)
       response, rest_request, return_value = nil, nil, nil
       tempfile = nil
@@ -177,6 +191,16 @@ class Chef
         response.error!
       end
       tempfile
+    rescue Net::HTTPServerException => e
+      http_attempts += 1
+      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
+      end
     rescue Exception => e
       log_failed_request(response, return_value) unless response.nil?
       if e.respond_to?(:chef_rest_request=)
@@ -195,6 +219,7 @@ class Chef
     # @yield [tempfile] block to process the tempfile
     # @yieldparams [tempfile<Tempfile>] tempfile
     def streaming_request(path, headers = {})
+      http_attempts ||= 0
       url = create_url(path)
       response, rest_request, return_value = nil, nil, nil
       tempfile = nil
@@ -222,6 +247,16 @@ class Chef
         end
       end
       tempfile
+    rescue Net::HTTPServerException => e
+      http_attempts += 1
+      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
+      end
     rescue Exception => e
       log_failed_request(response, return_value) unless response.nil?
       if e.respond_to?(:chef_rest_request=)
@@ -413,6 +448,10 @@ class Chef
       end
     end
 
+    def version_retries
+      @version_retries ||= options[:version_class].possible_requests
+    end
+
     # @api private
     def http_retry_delay
       config[:http_retry_delay]
diff --git a/lib/chef/http/api_versions.rb b/lib/chef/http/api_versions.rb
index 696c7072bf..674d8f85a7 100644
--- a/lib/chef/http/api_versions.rb
+++ b/lib/chef/http/api_versions.rb
@@ -32,6 +32,9 @@ class Chef
       end
 
       def handle_response(http_response, rest_request, return_value)
+        if http_response.code == "406"
+          ServerAPIVersions.instance.reset!
+        end
         if http_response.key?("x-ops-server-api-version")
           ServerAPIVersions.instance.set_versions(JSONCompat.parse(http_response["x-ops-server-api-version"]))
         end
diff --git a/lib/chef/http/authenticator.rb b/lib/chef/http/authenticator.rb
index 84065bf816..65367af107 100644
--- a/lib/chef/http/authenticator.rb
+++ b/lib/chef/http/authenticator.rb
@@ -30,6 +30,8 @@ class Chef
       attr_reader :raw_key
       attr_reader :attr_names
       attr_reader :auth_credentials
+      attr_reader :version_class
+      attr_reader :api_version
 
       attr_accessor :sign_request
 
@@ -39,15 +41,12 @@ class Chef
         @signing_key_filename = opts[:signing_key_filename]
         @key = load_signing_key(opts[:signing_key_filename], opts[:raw_key])
         @auth_credentials = AuthCredentials.new(opts[:client_name], @key)
-        if opts[:api_version]
-          @api_version = opts[:api_version]
-        else
-          @api_version = DEFAULT_SERVER_API_VERSION
-        end
+        @version_class = opts[:version_class]
+        @api_version = opts[:api_version]
       end
 
       def handle_request(method, url, headers = {}, data = false)
-        headers["X-Ops-Server-API-Version"] = @api_version
+        headers["X-Ops-Server-API-Version"] = request_version
         headers.merge!(authentication_headers(method, url, data, headers)) if sign_requests?
         [method, url, headers, data]
       end
@@ -64,6 +63,16 @@ class Chef
         [http_response, rest_request, return_value]
       end
 
+      def request_version
+        if version_class
+          version_class.best_request_version
+        elsif api_version
+          api_version
+        else
+          DEFAULT_SERVER_API_VERSION
+        end
+      end
+
       def sign_requests?
         auth_credentials.sign_requests? && @sign_request
       end
diff --git a/lib/chef/mixin/versioned_api.rb b/lib/chef/mixin/versioned_api.rb
index 3015345d44..17c9838d29 100644
--- a/lib/chef/mixin/versioned_api.rb
+++ b/lib/chef/mixin/versioned_api.rb
@@ -70,6 +70,10 @@ class Chef
         klass.minimum_api_version.to_s
       end
 
+      def possible_requests
+        versioned_interfaces.length
+      end
+
       def new(*args)
         object = versioned_api_class.allocate
         object.send(:initialize, *args)
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 50ad2a3fb6..8e1faa3060 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -70,6 +70,9 @@ require "chef/chef_fs/file_system_cache"
 
 require "chef/api_client_v1"
 
+require "chef/mixin/versioned_api"
+require "chef/server_api_versions"
+
 if ENV["CHEF_FIPS"] == "1"
   Chef::Config.init_openssl
 end
diff --git a/spec/unit/http/api_versions_spec.rb b/spec/unit/http/api_versions_spec.rb
index 37f1259d86..91d46763c2 100644
--- a/spec/unit/http/api_versions_spec.rb
+++ b/spec/unit/http/api_versions_spec.rb
@@ -45,6 +45,7 @@ describe Chef::HTTP::APIVersions do
   let(:response) do
     m = double("HttpResponse", :body => response_body)
     allow(m).to receive(:key?).with("x-ops-server-api-version").and_return(true)
+    allow(m).to receive(:code).and_return(return_value)
     allow(m).to receive(:[]) do |key|
       response_headers[key]
     end
@@ -66,4 +67,13 @@ describe Chef::HTTP::APIVersions do
     run_api_version_handler
     expect(Chef::ServerAPIVersions.instance.min_server_version).to eq(0)
   end
+
+  context "with an unacceptable api version" do
+    let (:return_value) { "406" }
+    it "resets the list of supported versions" do
+      Chef::ServerAPIVersions.instance.set_versions({ "min_version" => 1, "max_version" => 3 })
+      run_api_version_handler
+      expect(Chef::ServerAPIVersions.instance.min_server_version).to eq(0)
+    end
+  end
 end
diff --git a/spec/unit/http/authenticator_spec.rb b/spec/unit/http/authenticator_spec.rb
index 7fd2bdc821..5de39523cf 100644
--- a/spec/unit/http/authenticator_spec.rb
+++ b/spec/unit/http/authenticator_spec.rb
@@ -38,6 +38,26 @@ describe Chef::HTTP::Authenticator do
           to include({ "X-Ops-Server-API-Version" => Chef::HTTP::Authenticator::DEFAULT_SERVER_API_VERSION })
       end
 
+      context "when version_class is provided" do
+        class V0Class; extend Chef::Mixin::VersionedAPI; minimum_api_version 0; end
+        class V2Class; extend Chef::Mixin::VersionedAPI; minimum_api_version 2; end
+
+        class AuthFactoryClass
+          extend Chef::Mixin::VersionedAPIFactory
+          add_versioned_api_class V0Class
+          add_versioned_api_class V2Class
+        end
+
+        let(:class_instance) { Chef::HTTP::Authenticator.new({ version_class: AuthFactoryClass }) }
+
+        it "uses it to select the correct http version" do
+          Chef::ServerAPIVersions.instance.reset!
+          expect(AuthFactoryClass).to receive(:best_request_version).and_call_original
+          expect(class_instance.handle_request(method, url, headers, data)[2]).
+            to include({ "X-Ops-Server-API-Version" => "2" })
+        end
+      end
+
       context "when api_version is set to something other than the default" do
         let(:class_instance) { Chef::HTTP::Authenticator.new({ :api_version => "-10" }) }
 
diff --git a/spec/unit/mixin/versioned_api_spec.rb b/spec/unit/mixin/versioned_api_spec.rb
index c709871919..e8b65158c4 100644
--- a/spec/unit/mixin/versioned_api_spec.rb
+++ b/spec/unit/mixin/versioned_api_spec.rb
@@ -100,6 +100,14 @@ describe Chef::Mixin::VersionedAPIFactory do
     end
   end
 
+  describe "#possible_requests" do
+    it "returns the number of registered classes" do
+      factory_class.add_versioned_api_class V2Class
+      factory_class.add_versioned_api_class V3Class
+      expect(factory_class.possible_requests).to eq(2)
+    end
+  end
+
   describe "#new" do
     it "creates an instance of the versioned class" do
       factory_class.add_versioned_api_class V2Class