Fix protocol negotiation for unversioned objects

We generally always do protocol negotiation on the get of the node
object and the Chef::Node is an unversioned API, so we need to
test that we backoff the API version to the highest version the server
offers and then retry again.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

4 files changed, 35 insertions, 14 deletions

diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index eed2a41815..436e5d14eb 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -154,7 +154,7 @@ class Chef
     rescue Net::HTTPClientException => e
       http_attempts += 1
       response = e.response
-      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
+      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts >= 0
         Chef::Log.trace("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
         retry
       else
@@ -193,7 +193,7 @@ class Chef
     rescue Net::HTTPClientException => e
       http_attempts += 1
       response = e.response
-      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
+      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts >= 0
         Chef::Log.trace("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
         retry
       else
@@ -249,7 +249,7 @@ class Chef
     rescue Net::HTTPClientException => e
       http_attempts += 1
       response = e.response
-      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts > 0
+      if response.is_a?(Net::HTTPNotAcceptable) && version_retries - http_attempts >= 0
         Chef::Log.trace("Negotiating protocol version with #{url}, retry #{http_attempts}/#{version_retries}")
         retry
       else
@@ -470,11 +470,7 @@ class Chef
     end
 
     def version_retries
-      @version_retries ||= if options[:version_class]
-                             options[:version_class].possible_requests
-                           else
-                             0
-                           end
+      @version_retries ||= options[:version_class]&.possible_requests || 1
     end
 
     # @api private
diff --git a/lib/chef/http/authenticator.rb b/lib/chef/http/authenticator.rb
index e65e93d2ac..4a29fcea33 100644
--- a/lib/chef/http/authenticator.rb
+++ b/lib/chef/http/authenticator.rb
@@ -68,6 +68,8 @@ class Chef
           version_class.best_request_version
         elsif api_version
           api_version
+        elsif Chef::ServerAPIVersions.instance.negotiated?
+          Chef::ServerAPIVersions.instance.max_server_version.to_s
         else
           DEFAULT_SERVER_API_VERSION
         end
diff --git a/lib/chef/server_api_versions.rb b/lib/chef/server_api_versions.rb
index f550fc1d66..30e2802ef0 100644
--- a/lib/chef/server_api_versions.rb
+++ b/lib/chef/server_api_versions.rb
@@ -51,6 +51,10 @@ class Chef
       @unversioned
     end
 
+    def negotiated?
+      !@versions.nil? || unversioned?
+    end
+
     def reset!
       @versions = nil
       @unversioned = false
diff --git a/spec/unit/server_api_spec.rb b/spec/unit/server_api_spec.rb
index 1b5d729383..5d78c4bd9b 100644
--- a/spec/unit/server_api_spec.rb
+++ b/spec/unit/server_api_spec.rb
@@ -116,17 +116,36 @@ describe Chef::ServerAPI do
 
     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
 
-  it "does not retry a 406 Not Acceptable" do
-    WebMock.disable_net_connect!
-    stub_request(:get, "http://chef.example.com:4000/foo").to_return(status: [406, "Not Acceptable"])
-    expect { client.get("foo") }.to raise_error(Net::HTTPServerException)
+    it "406 on a post does protocol negotiation" 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: {})
+      client.post("foo", post_body)
+    end
+
+    it "406 on a put does protocol negotiation" do
+      WebMock.disable_net_connect!
+      put_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(:put, "http://chef.example.com:4000/foo").with(body: put_body.to_json, headers: { "X-Ops-Server-Api-Version" => "2" }).to_return(status: [406, "Not Acceptable"], body: body_406 )
+      stub_request(:put, "http://chef.example.com:4000/foo").with(body: put_body.to_json, headers: { "X-Ops-Server-Api-Version" => "0" }).to_return(status: 200, body: "", headers: {})
+      client.put("foo", put_body)
+    end
+
+    it "406 on a get does protocol negotiation" do
+      WebMock.disable_net_connect!
+      body_406 = '{"error":"invalid-x-ops-server-api-version","message":"Specified version 2 not supported","min_version":0,"max_version":1}'
+      stub_request(:get, "http://chef.example.com:4000/foo").with(headers: { "X-Ops-Server-Api-Version" => "2" }).to_return(status: [406, "Not Acceptable"], body: body_406 )
+      stub_request(:get, "http://chef.example.com:4000/foo").with(headers: { "X-Ops-Server-Api-Version" => "0" }).to_return(status: 200, body: "", headers: {})
+      client.get("foo")
+    end
   end
 end