summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLamont Granquist <lamont@scriptkiddie.org>2017-07-20 10:25:19 -0700
committerLamont Granquist <lamont@scriptkiddie.org>2017-07-20 10:25:19 -0700
commitdf6dd3768d5fcc99a348bd52d7e6ffeaa1c391db (patch)
tree8e79fe5fd7b9102ed71a8a731b1492f4d1bd212c
parent020c42b59503692742e6298a35d1bf4b1d2b491c (diff)
downloadchef-df6dd3768d5fcc99a348bd52d7e6ffeaa1c391db.tar.gz
fix retries on JSON POST requests when negotiating protocol version
on the first pass through the JSON middleware it encodes the body. if there's a retry, it re-encodes the body as a string with all its metacharacters escaped. this is a particular issue when doing a first request that is a POST that requires negotiating the API version. when doing a GET it isn't a problem because there's no body payload -- but a POST or a PUT which requires a retry will get garbled and will cause a 500. this happens on hosted right now if trying to POST with a v2 API since hosted is only v1, so there's a retry to downgrade. i also made the same kind of changes to the streaming download requests, but since they're GETs its unclear to me if there was any impact there -- but middleware could have been double-mangling headers on a retry. Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
-rw-r--r--Gemfile1
-rw-r--r--Gemfile.lock12
-rw-r--r--lib/chef/http.rb16
-rw-r--r--spec/spec_helper.rb9
-rw-r--r--spec/unit/http/api_versions_spec.rb9
-rw-r--r--spec/unit/server_api_spec.rb76
6 files changed, 110 insertions, 13 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..f9845c2460 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)
@@ -176,11 +176,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
@@ -223,11 +224,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
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