Merge pull request #5960 from chef/tm/retryable_api

Retry API requests if using an unsupported version

9 files changed, 132 insertions, 11 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 e164da262d..674d8f85a7 100644
--- a/lib/chef/http/api_versions.rb
+++ b/lib/chef/http/api_versions.rb
@@ -16,6 +16,7 @@
 #
 
 require "chef/server_api_versions"
+require "chef/json_compat"
 
 class Chef
   class HTTP
@@ -31,8 +32,11 @@ 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(http_response["x-ops-server-api-version"])
+          ServerAPIVersions.instance.set_versions(JSONCompat.parse(http_response["x-ops-server-api-version"]))
         end
         [http_response, rest_request, return_value]
       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 9c2f2f4cdb..17c9838d29 100644
--- a/lib/chef/mixin/versioned_api.rb
+++ b/lib/chef/mixin/versioned_api.rb
@@ -40,11 +40,15 @@ class Chef
       end
 
       def versioned_api_class
+        get_class_for(:max_server_version)
+      end
+
+      def get_class_for(type)
         versioned_interfaces.select do |klass|
           version = klass.send(:minimum_api_version)
           # min and max versions will be nil if we've not made a request to the server yet,
           # in which case we'll just start with the highest version and see what happens
-          ServerAPIVersions.instance.min_server_version.nil? || (version >= ServerAPIVersions.instance.min_server_version && version <= ServerAPIVersions.instance.max_server_version)
+          ServerAPIVersions.instance.min_server_version.nil? || (version >= ServerAPIVersions.instance.min_server_version && version <= ServerAPIVersions.instance.send(type))
         end
           .sort { |a, b| a.send(:minimum_api_version) <=> b.send(:minimum_api_version) }
           .last
@@ -59,6 +63,17 @@ class Chef
         module_eval(str, __FILE__, line_no)
       end
 
+      # When teeing up an HTTP request, we need to be able to ask which API version we should use.
+      # Something in Net::HTTP seems to expect to strip headers, so we return this as a string.
+      def best_request_version
+        klass = get_class_for(:max_server_version)
+        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/lib/chef/server_api_versions.rb b/lib/chef/server_api_versions.rb
index 68dfd5ac90..2a4d0e6a5b 100644
--- a/lib/chef/server_api_versions.rb
+++ b/lib/chef/server_api_versions.rb
@@ -26,11 +26,11 @@ class Chef
     end
 
     def min_server_version
-      !@versions.nil? ? @versions["min_version"] : nil
+      !@versions.nil? ? Integer(@versions["min_version"]) : nil
     end
 
     def max_server_version
-      !@versions.nil? ? @versions["max_version"] : nil
+      !@versions.nil? ? Integer(@versions["max_version"]) : nil
     end
 
     def reset!
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 79c97a1b69..91d46763c2 100644
--- a/spec/unit/http/api_versions_spec.rb
+++ b/spec/unit/http/api_versions_spec.rb
@@ -38,13 +38,14 @@ describe Chef::HTTP::APIVersions do
   let(:response_body) { "Thanks for checking in." }
   let(:response_headers) do
     {
-      "x-ops-server-api-version" => { "min_version" => 0, "max_version" => 2 },
+      "x-ops-server-api-version" => { "min_version" => 0, "max_version" => 2 }.to_json,
     }
   end
 
   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 4f2418ca24..e8b65158c4 100644
--- a/spec/unit/mixin/versioned_api_spec.rb
+++ b/spec/unit/mixin/versioned_api_spec.rb
@@ -87,6 +87,27 @@ describe Chef::Mixin::VersionedAPIFactory do
     end
   end
 
+  describe "#best_request_version" do
+    it "returns a String" do
+      factory_class.add_versioned_api_class V2Class
+      expect(factory_class.best_request_version).to be_a(String)
+    end
+
+    it "returns the most relevant version" do
+      factory_class.add_versioned_api_class V2Class
+      factory_class.add_versioned_api_class V3Class
+      expect(factory_class.best_request_version).to eq("3")
+    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