From d6dc0b04c6f3c3d8a1461778c0c2604f879b505b Mon Sep 17 00:00:00 2001 From: Thom May Date: Wed, 29 Mar 2017 09:34:09 +0100 Subject: Retry API requests if using an unsupported version Signed-off-by: Thom May --- lib/chef/http.rb | 39 +++++++++++++++++++++++++++++++++++ lib/chef/http/api_versions.rb | 3 +++ lib/chef/http/authenticator.rb | 21 +++++++++++++------ lib/chef/mixin/versioned_api.rb | 4 ++++ spec/spec_helper.rb | 3 +++ spec/unit/http/api_versions_spec.rb | 10 +++++++++ spec/unit/http/authenticator_spec.rb | 20 ++++++++++++++++++ spec/unit/mixin/versioned_api_spec.rb | 8 +++++++ 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 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 -- cgit v1.2.1