From 89ea46b610ae4a45e8d4be802e1fd89197b7ae64 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 2 Jun 2017 11:03:32 -0700 Subject: [PATCH 1/4] (PUP-3940) Use proper mime types in Accept header Previously, puppet would send an Accept header containing the format names that the indirected model supported: Accept: json, pson This commit modifies puppet to send the proper mime type for each format: Accept: application/json, text/pson Fortunately, puppet's server-side Request#response_format_for method already accepts both when determining which response format is most suitable. This change also affects the Accept header for cert-related files: GET /puppet-ca/v1/certificate/ca?environment=production&fail_on_404=true HTTP/1.1 Accept: text/plain The puppetserver "puppet-ca" routes have always ignored puppet's Accept header, and always returned "text/plain" content, so there isn't a compatibility issue. Note this change only affects the Accept header, but not Content-Type, as puppet already correctly used mime types in the request body (when 'PUT'ing CSRs, file bucket files, and reports) and the response body (for everything else). --- lib/puppet/indirector/rest.rb | 3 ++- spec/unit/indirector/rest_spec.rb | 22 +++++++++++----------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb index 62a083f0486..106d149f25b 100644 --- a/lib/puppet/indirector/rest.rb +++ b/lib/puppet/indirector/rest.rb @@ -104,8 +104,9 @@ def headers network_formats = model.supported_formats.reject do |format| [:yaml, :b64_zlib_yaml].include?(format) end + mime_types = network_formats.map { |f| model.get_format(f).mime } common_headers = { - "Accept" => network_formats.join(", "), + "Accept" => mime_types.join(', '), Puppet::Network::HTTP::HEADER_PUPPET_VERSION => Puppet.version } diff --git a/spec/unit/indirector/rest_spec.rb b/spec/unit/indirector/rest_spec.rb index 466d64e03f4..1e966a4cc12 100644 --- a/spec/unit/indirector/rest_spec.rb +++ b/spec/unit/indirector/rest_spec.rb @@ -278,13 +278,13 @@ def save_request(key, instance, options={}) it 'excludes yaml from the Accept header' do model.expects(:supported_formats).returns([:json, :pson, :yaml, :binary]) - expect(terminus.headers['Accept']).to eq('json, pson, binary') + expect(terminus.headers['Accept']).to eq('application/json, text/pson, application/octet-stream') end it 'excludes b64_zlib_yaml from the Accept header' do model.expects(:supported_formats).returns([:json, :pson, :b64_zlib_yaml]) - expect(terminus.headers['Accept']).to eq('json, pson') + expect(terminus.headers['Accept']).to eq('application/json, text/pson') end describe "when creating an HTTP client" do @@ -432,10 +432,10 @@ def save_request(key, instance, options={}) expect(terminus.find(request)).to eq(model.new('name', 'decoded body')) end - it "provides an Accept header containing the list of supported formats joined with commas" do - connection.expects(:get).with(anything, has_entry("Accept" => "supported, formats")).returns(response) + it "provides an Accept header containing the list of supported mime types joined with commas" do + connection.expects(:get).with(anything, has_entry("Accept" => "application/json, text/pson")).returns(response) - terminus.model.expects(:supported_formats).returns %w{supported formats} + terminus.model.expects(:supported_formats).returns [:json, :pson] terminus.find(request) end @@ -538,9 +538,9 @@ def save_request(key, instance, options={}) end it "should provide an Accept header containing the list of supported formats joined with commas" do - connection.expects(:get).with(anything, has_entry("Accept" => "supported, formats")).returns(mock_response(200, '')) + connection.expects(:get).with(anything, has_entry("Accept" => "application/json, text/pson")).returns(mock_response(200, '')) - terminus.model.expects(:supported_formats).returns %w{supported formats} + terminus.model.expects(:supported_formats).returns [:json, :pson] terminus.search(request) end @@ -597,9 +597,9 @@ def save_request(key, instance, options={}) end it "should provide an Accept header containing the list of supported formats joined with commas" do - connection.expects(:delete).with(anything, has_entry("Accept" => "supported, formats")).returns(response) + connection.expects(:delete).with(anything, has_entry("Accept" => "application/json, text/pson")).returns(response) - terminus.model.expects(:supported_formats).returns %w{supported formats} + terminus.model.expects(:supported_formats).returns [:json, :pson] terminus.destroy(request) end @@ -657,10 +657,10 @@ def save_request(key, instance, options={}) end it "should provide an Accept header containing the list of supported formats joined with commas" do - connection.expects(:put).with(anything, anything, has_entry("Accept" => "supported, formats")).returns(response) + connection.expects(:put).with(anything, anything, has_entry("Accept" => "application/json, text/pson")).returns(response) instance.expects(:render).returns('') - model.expects(:supported_formats).returns %w{supported formats} + model.expects(:supported_formats).returns [:json, :pson] instance.expects(:mime).returns "supported" terminus.save(request) From b15148963d0fac8be946112964bb767ca25ce608 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 2 Jun 2017 11:44:29 -0700 Subject: [PATCH 2/4] (PUP-3940) Accept application/octet-stream for binary content Modify the agent so it accepts "application/octet-stream" when streaming file and static_file content. The "binary" content type is dropped since the puppetserver static_file_content endpoint has historically has ignored the Accept header, and always returned "application/octet-stream". --- lib/puppet/type/file/source.rb | 6 +++++- spec/unit/type/file/source_spec.rb | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb index 298922e180f..8569c627f43 100644 --- a/lib/puppet/type/file/source.rb +++ b/lib/puppet/type/file/source.rb @@ -16,6 +16,10 @@ module Puppet Puppet::Type.type(:file).newparam(:source) do include Puppet::Network::HTTP::Compression.module + BINARY_MIME_TYPES = [ + Puppet::Network::FormatHandler.format_for('binary').mime + ].join(', ').freeze + attr_accessor :source, :local desc <<-'EOT' A source file, which will be copied into place on the local system. This @@ -289,7 +293,7 @@ def get_from_puppet_source(source_uri, content_uri, &block) request.do_request(:fileserver) do |req| connection = Puppet::Network::HttpPool.http_instance(req.server, req.port) - connection.request_get(Puppet::Network::HTTP::API::IndirectedRoutes.request_to_uri(req), add_accept_encoding({"Accept" => "binary"}), &block) + connection.request_get(Puppet::Network::HTTP::API::IndirectedRoutes.request_to_uri(req), add_accept_encoding({"Accept" => BINARY_MIME_TYPES}), &block) end end diff --git a/spec/unit/type/file/source_spec.rb b/spec/unit/type/file/source_spec.rb index d3e7933d97e..40dd5d2c241 100644 --- a/spec/unit/type/file/source_spec.rb +++ b/spec/unit/type/file/source_spec.rb @@ -612,6 +612,19 @@ resource.write(source) end + it 'should request binary content' do + response.stubs(:code).returns('200') + Puppet::Network::HttpPool.stubs(:http_instance).returns(conn) + source.stubs(:metadata).returns stub_everything('metadata', :source => 'puppet:///test/foo bar', :ftype => 'file') + + conn.unstub(:request_get) + conn.expects(:request_get).with do |_, options| + expect(options).to include('Accept' => 'application/octet-stream') + end.yields(response) + + resource.write(source) + end + describe 'when handling file_content responses' do before do File.open(filename, 'w') {|f| f.write "initial file content"} From a9d1f99edf10c7621cd156ec7a0306deecb61b8e Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Mon, 5 Jun 2017 09:41:11 -0700 Subject: [PATCH 3/4] (PUP-3940) Exclude dot from the agent's Accept header Previously, the agent included `text/dot` in its list of accepted content types. However, if the server ever returned a catalog in that format, the agent would error with: Puppet::Resource::Catalog does not respond to from_dot; can not intern instances from text/dot The issue is that `dot` is not a network serialization format. It's a one-way transformation from the catalog into a graph representation. This commit ensures `dot` is excluded from the agent's Accept header for all model types, not just catalogs. Note this commit doesn't prevent the server from returning a catalog in dot, as you might want to curl the server to export the catalog: curl -H 'Accept: text/dot' https://.../puppet/v3/catalog/?environment= --- lib/puppet/indirector/rest.rb | 2 +- spec/unit/indirector/rest_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb index 106d149f25b..2d038e7f61c 100644 --- a/lib/puppet/indirector/rest.rb +++ b/lib/puppet/indirector/rest.rb @@ -102,7 +102,7 @@ def self.port def headers # yaml is not allowed on the network network_formats = model.supported_formats.reject do |format| - [:yaml, :b64_zlib_yaml].include?(format) + [:yaml, :b64_zlib_yaml, :dot].include?(format) end mime_types = network_formats.map { |f| model.get_format(f).mime } common_headers = { diff --git a/spec/unit/indirector/rest_spec.rb b/spec/unit/indirector/rest_spec.rb index 1e966a4cc12..f32fc2d1521 100644 --- a/spec/unit/indirector/rest_spec.rb +++ b/spec/unit/indirector/rest_spec.rb @@ -287,6 +287,12 @@ def save_request(key, instance, options={}) expect(terminus.headers['Accept']).to eq('application/json, text/pson') end + it 'excludes dot from the Accept header' do + model.expects(:supported_formats).returns([:json, :dot]) + + expect(terminus.headers['Accept']).to eq('application/json') + end + describe "when creating an HTTP client" do it "should use the class's server and port if the indirection request provides neither" do @request = stub 'request', :key => "foo", :server => nil, :port => nil From c1b087898c85ef931f7bf9d781ce26c76a82a285 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 6 Jun 2017 12:31:09 -0700 Subject: [PATCH 4/4] (PUP-3940) Refactor how formats are excluded Simplify the logic for excluding formats that are not allowed or otherwise shouldn't be used on the network. --- lib/puppet/indirector/rest.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb index 2d038e7f61c..9d43f074de0 100644 --- a/lib/puppet/indirector/rest.rb +++ b/lib/puppet/indirector/rest.rb @@ -10,6 +10,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus include Puppet::Network::HTTP::Compression.module IndirectedRoutes = Puppet::Network::HTTP::API::IndirectedRoutes + EXCLUDED_FORMATS = [:yaml, :b64_zlib_yaml, :dot] class << self attr_reader :server_setting, :port_setting @@ -101,9 +102,7 @@ def self.port # Provide appropriate headers. def headers # yaml is not allowed on the network - network_formats = model.supported_formats.reject do |format| - [:yaml, :b64_zlib_yaml, :dot].include?(format) - end + network_formats = model.supported_formats - EXCLUDED_FORMATS mime_types = network_formats.map { |f| model.get_format(f).mime } common_headers = { "Accept" => mime_types.join(', '),