From a6fbf127d3eeb38d73fa4b9aec98ef34cf897f85 Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Tue, 24 Mar 2015 17:53:56 -0700 Subject: [PATCH 1/2] Update policyfile URLs and cookbook artifact data format per RFC --- lib/chef/cookbook_manifest.rb | 20 ++++++- lib/chef/cookbook_version.rb | 19 +++++++ lib/chef/policy_builder/policyfile.rb | 8 +-- spec/unit/cookbook_manifest_spec.rb | 21 ++++++- spec/unit/cookbook_uploader_spec.rb | 8 ++- spec/unit/policy_builder/policyfile_spec.rb | 61 ++++++++++++++++----- 6 files changed, 114 insertions(+), 23 deletions(-) diff --git a/lib/chef/cookbook_manifest.rb b/lib/chef/cookbook_manifest.rb index 0d21e9725c8..10654a49452 100644 --- a/lib/chef/cookbook_manifest.rb +++ b/lib/chef/cookbook_manifest.rb @@ -36,6 +36,7 @@ class CookbookManifest def_delegator :@cookbook_version, :root_paths def_delegator :@cookbook_version, :segment_filenames def_delegator :@cookbook_version, :name + def_delegator :@cookbook_version, :identifier def_delegator :@cookbook_version, :metadata def_delegator :@cookbook_version, :full_name def_delegator :@cookbook_version, :version @@ -141,9 +142,16 @@ def to_json(*a) # REST api. If there is an existing document on the server and it # is marked frozen, a PUT will result in a 409 Conflict. def save_url - "#{cookbook_url_path}/#{name}/#{version}" + if policy_mode? + "#{named_cookbook_url}/#{identifier}" + else + "#{named_cookbook_url}/#{version}" + end end + def named_cookbook_url + "#{cookbook_url_path}/#{name}" + end # Adds the `force=true` parameter to the upload URL. This allows # the user to overwrite a frozen cookbook (a PUT against the # normal #save_url raises a 409 Conflict in this case). @@ -214,10 +222,16 @@ def generate_manifest end end - manifest[:cookbook_name] = name.to_s manifest[:metadata] = metadata manifest[:version] = metadata.version - manifest[:name] = full_name + + if policy_mode? + manifest[:name] = name.to_s + manifest[:identifier] = identifier + else + manifest[:name] = full_name + manifest[:cookbook_name] = name.to_s + end @manifest_records_by_path = extract_manifest_records_by_path(manifest) @manifest = manifest diff --git a/lib/chef/cookbook_version.rb b/lib/chef/cookbook_version.rb index b8f32a61bb7..8d302eeec23 100644 --- a/lib/chef/cookbook_version.rb +++ b/lib/chef/cookbook_version.rb @@ -78,6 +78,16 @@ def status attr_accessor :chef_server_rest + # The `identifier` field is used for cookbook_artifacts, which are + # organized on the chef server according to their content. If the + # policy_mode option to CookbookManifest is set to true it will include + # this field in the manifest Hash and in the upload URL. + # + # This field may be removed or have different behavior in the future, don't + # use it in 3rd party code. + # @api private + attr_accessor :identifier + # The first root path is the primary cookbook dir, from which metadata is loaded def root_dir root_paths[0] @@ -458,6 +468,15 @@ def self.json_create(o) cookbook_version end + def self.from_cb_artifact_data(o) + cookbook_version = new(o["name"]) + # We want the Chef::Cookbook::Metadata object to always be inflated + cookbook_version.metadata = Chef::Cookbook::Metadata.from_hash(o["metadata"]) + cookbook_version.manifest = o + cookbook_version.identifier = o["identifier"] + cookbook_version + end + # @deprecated This method was used by the Ruby Chef Server and is no longer # needed. There is no replacement. def generate_manifest_with_urls(&url_generator) diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb index d368b055f73..f85d72f121e 100644 --- a/lib/chef/policy_builder/policyfile.rb +++ b/lib/chef/policy_builder/policyfile.rb @@ -239,7 +239,7 @@ def policy def policyfile_location if Chef::Config[:policy_document_native_api] validate_policy_config! - "policies/#{policy_group}/#{policy_name}" + "policy_groups/#{policy_group}/policies/#{policy_name}" else "data/policyfiles/#{deployment_group}" end @@ -368,11 +368,11 @@ def compat_mode_manifest_for(cookbook_name, lock_data) end def artifact_manifest_for(cookbook_name, lock_data) - xyz_version = lock_data["dotted_decimal_identifier"] - rel_url = "cookbook_artifacts/#{cookbook_name}/#{xyz_version}" + identifier = lock_data["identifier"] + rel_url = "cookbook_artifacts/#{cookbook_name}/#{identifier}" http_api.get(rel_url) rescue Exception => e - message = "Error loading cookbook #{cookbook_name} at version #{xyz_version} from #{rel_url}: #{e.class} - #{e.message}" + message = "Error loading cookbook #{cookbook_name} with identifier #{identifier} from #{rel_url}: #{e.class} - #{e.message}" err = Chef::Exceptions::CookbookNotFound.new(message) err.set_backtrace(e.backtrace) raise err diff --git a/spec/unit/cookbook_manifest_spec.rb b/spec/unit/cookbook_manifest_spec.rb index 938f72c7438..f985942e091 100644 --- a/spec/unit/cookbook_manifest_spec.rb +++ b/spec/unit/cookbook_manifest_spec.rb @@ -24,6 +24,8 @@ let(:version) { "1.2.3" } + let(:identifier) { "9e10455ce2b4a4e29424b7064b1d67a1a25c9d3b" } + let(:metadata) do Chef::Cookbook::Metadata.new.tap do |m| m.version(version) @@ -35,6 +37,7 @@ let(:cookbook_version) do Chef::CookbookVersion.new("tatft", cookbook_root).tap do |c| c.metadata = metadata + c.identifier = identifier end end @@ -212,12 +215,26 @@ def map_to_file_specs(paths) let(:policy_mode) { true } + let(:cookbook_manifest_hash) { cookbook_manifest.to_hash } + + it "sets the identifier in the manifest data" do + expect(cookbook_manifest_hash["identifier"]).to eq("9e10455ce2b4a4e29424b7064b1d67a1a25c9d3b") + end + + it "sets the name to just the name" do + expect(cookbook_manifest_hash["name"]).to eq("tatft") + end + + it "does not set a 'cookbook_name' field" do + expect(cookbook_manifest_hash).to_not have_key("cookbook_name") + end + it "gives the save URL" do - expect(cookbook_manifest.save_url).to eq("cookbook_artifacts/tatft/1.2.3") + expect(cookbook_manifest.save_url).to eq("cookbook_artifacts/tatft/9e10455ce2b4a4e29424b7064b1d67a1a25c9d3b") end it "gives the force save URL" do - expect(cookbook_manifest.force_save_url).to eq("cookbook_artifacts/tatft/1.2.3?force=true") + expect(cookbook_manifest.force_save_url).to eq("cookbook_artifacts/tatft/9e10455ce2b4a4e29424b7064b1d67a1a25c9d3b?force=true") end end diff --git a/spec/unit/cookbook_uploader_spec.rb b/spec/unit/cookbook_uploader_spec.rb index 152e5373f05..76727c18e2c 100644 --- a/spec/unit/cookbook_uploader_spec.rb +++ b/spec/unit/cookbook_uploader_spec.rb @@ -25,11 +25,17 @@ let(:cookbook_loader) do loader = Chef::CookbookLoader.new(File.join(CHEF_SPEC_DATA, "cookbooks")) loader.load_cookbooks + loader.cookbooks_by_name["apache2"].identifier = apache2_identifier + loader.cookbooks_by_name["java"].identifier = java_identifier loader end + let(:apache2_identifier) { "6644e6cb2ade90b8aff2ebb44728958fbc939ebf" } + let(:apache2_cookbook) { cookbook_loader.cookbooks_by_name["apache2"] } + let(:java_identifier) { "edd40c30c4e0ebb3658abde4620597597d2e9c17" } + let(:java_cookbook) { cookbook_loader.cookbooks_by_name["java"] } let(:cookbooks_to_upload) { [apache2_cookbook, java_cookbook] } @@ -175,7 +181,7 @@ def expect_cookbook_create let(:policy_mode) { true } def expected_save_url(cookbook) - "cookbook_artifacts/#{cookbook.name}/#{cookbook.version}" + "cookbook_artifacts/#{cookbook.name}/#{cookbook.identifier}" end it "uploads all files in a sandbox transaction, then creates cookbooks on the server using cookbook_artifacts API" do diff --git a/spec/unit/policy_builder/policyfile_spec.rb b/spec/unit/policy_builder/policyfile_spec.rb index 8b6e928a46f..e4f7388a1ce 100644 --- a/spec/unit/policy_builder/policyfile_spec.rb +++ b/spec/unit/policy_builder/policyfile_spec.rb @@ -256,7 +256,7 @@ def initialize_pb context "and policy_name and policy_group are configured" do - let(:policy_relative_url) { "policies/policy-stage/example" } + let(:policy_relative_url) { "policy_groups/policy-stage/policies/example" } before do expect(http_api).to receive(:get).with(policy_relative_url).and_return(parsed_policyfile_json) @@ -386,6 +386,9 @@ def initialize_pb describe "fetching the desired cookbook set" do + let(:example1_cookbook_data) { double("CookbookVersion Hash for example1 cookbook") } + let(:example2_cookbook_data) { double("CookbookVersion Hash for example2 cookbook") } + let(:example1_cookbook_object) { double("Chef::CookbookVersion for example1 cookbook") } let(:example2_cookbook_object) { double("Chef::CookbookVersion for example2 cookbook") } @@ -396,9 +399,12 @@ def initialize_pb let(:example1_xyz_version) { example1_lock_data["dotted_decimal_identifier"] } let(:example2_xyz_version) { example2_lock_data["dotted_decimal_identifier"] } + let(:example1_identifier) { example1_lock_data["identifier"] } + let(:example2_identifier) { example2_lock_data["identifier"] } + let(:cookbook_synchronizer) { double("Chef::CookbookSynchronizer") } - shared_examples_for "fetching cookbooks" do + shared_examples "fetching cookbooks when they don't exist" do context "and a cookbook is missing" do let(:error404) { Net::HTTPServerException.new("404 message", :body) } @@ -418,7 +424,9 @@ def initialize_pb end end + end + shared_examples_for "fetching cookbooks when they exist" do context "and the cookbooks can be fetched" do before do expect(Chef::Node).to receive(:find_or_create).with(node_name).and_return(node) @@ -426,11 +434,6 @@ def initialize_pb policy_builder.load_node policy_builder.build_node - expect(http_api).to receive(:get).with(cookbook1_url). - and_return(example1_cookbook_object) - expect(http_api).to receive(:get).with(cookbook2_url). - and_return(example2_cookbook_object) - allow(Chef::CookbookSynchronizer).to receive(:new). with(expected_cookbook_hash, events). and_return(cookbook_synchronizer) @@ -457,11 +460,23 @@ def initialize_pb end # shared_examples_for "fetching cookbooks" context "when using compatibility mode (policy_document_native_api == false)" do - include_examples "fetching cookbooks" do + let(:cookbook1_url) { "cookbooks/example1/#{example1_xyz_version}" } + let(:cookbook2_url) { "cookbooks/example2/#{example2_xyz_version}" } - let(:cookbook1_url) { "cookbooks/example1/#{example1_xyz_version}" } - let(:cookbook2_url) { "cookbooks/example2/#{example2_xyz_version}" } + context "when the cookbooks don't exist on the server" do + include_examples "fetching cookbooks when they don't exist" + end + + context "when the cookbooks exist on the server" do + + before do + expect(http_api).to receive(:get).with(cookbook1_url). + and_return(example1_cookbook_object) + expect(http_api).to receive(:get).with(cookbook2_url). + and_return(example2_cookbook_object) + end + include_examples "fetching cookbooks when they exist" end end @@ -474,13 +489,33 @@ def initialize_pb Chef::Config[:policy_name] = "example" end - include_examples "fetching cookbooks" do + let(:cookbook1_url) { "cookbook_artifacts/example1/#{example1_identifier}" } + let(:cookbook2_url) { "cookbook_artifacts/example2/#{example2_identifier}" } + + context "when the cookbooks don't exist on the server" do + include_examples "fetching cookbooks when they don't exist" + end + - let(:cookbook1_url) { "cookbook_artifacts/example1/#{example1_xyz_version}" } - let(:cookbook2_url) { "cookbook_artifacts/example2/#{example2_xyz_version}" } + context "when the cookbooks exist on the server" do + + before do + expect(http_api).to receive(:get).with(cookbook1_url). + and_return(example1_cookbook_data) + expect(http_api).to receive(:get).with(cookbook2_url). + and_return(example2_cookbook_data) + + expect(Chef::CookbookVersion).to receive(:from_cb_artifact_data).with(example1_cookbook_data). + and_return(example1_cookbook_object) + expect(Chef::CookbookVersion).to receive(:from_cb_artifact_data).with(example2_cookbook_data). + and_return(example2_cookbook_object) + end + + include_examples "fetching cookbooks when they exist" end + end end From 93c669196f53d21cf2b973b014c2cde4e0c3313a Mon Sep 17 00:00:00 2001 From: danielsdeleo Date: Tue, 24 Mar 2015 18:14:57 -0700 Subject: [PATCH 2/2] Handle cookbook artfact format differences when fetching cookbooks Cookbook artifacts differ in these ways: * the name field is the cookbook name instead of name+version * there is no "cookbook_name" field * cookbook artifacts don't have a json_class when downloaded from the server * there is an identifier field --- lib/chef/cookbook/remote_file_vendor.rb | 2 +- lib/chef/policy_builder/policyfile.rb | 6 ++++- spec/unit/cookbook/file_vendor_spec.rb | 36 +++++++++++++++++++------ 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/lib/chef/cookbook/remote_file_vendor.rb b/lib/chef/cookbook/remote_file_vendor.rb index 2ddce310014..9d895b168e0 100644 --- a/lib/chef/cookbook/remote_file_vendor.rb +++ b/lib/chef/cookbook/remote_file_vendor.rb @@ -30,7 +30,7 @@ class RemoteFileVendor < FileVendor def initialize(manifest, rest) @manifest = manifest - @cookbook_name = @manifest[:cookbook_name] + @cookbook_name = @manifest[:cookbook_name] || @manifest[:name] @rest = rest end diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb index f85d72f121e..0c7c07f9f37 100644 --- a/lib/chef/policy_builder/policyfile.rb +++ b/lib/chef/policy_builder/policyfile.rb @@ -370,7 +370,7 @@ def compat_mode_manifest_for(cookbook_name, lock_data) def artifact_manifest_for(cookbook_name, lock_data) identifier = lock_data["identifier"] rel_url = "cookbook_artifacts/#{cookbook_name}/#{identifier}" - http_api.get(rel_url) + inflate_cbv_object(http_api.get(rel_url)) rescue Exception => e message = "Error loading cookbook #{cookbook_name} with identifier #{identifier} from #{rel_url}: #{e.class} - #{e.message}" err = Chef::Exceptions::CookbookNotFound.new(message) @@ -378,6 +378,10 @@ def artifact_manifest_for(cookbook_name, lock_data) raise err end + def inflate_cbv_object(raw_manifest) + Chef::CookbookVersion.from_cb_artifact_data(raw_manifest) + end + end end end diff --git a/spec/unit/cookbook/file_vendor_spec.rb b/spec/unit/cookbook/file_vendor_spec.rb index 4fad7d58088..145541a63f1 100644 --- a/spec/unit/cookbook/file_vendor_spec.rb +++ b/spec/unit/cookbook/file_vendor_spec.rb @@ -21,9 +21,6 @@ let(:file_vendor_class) { Class.new(described_class) } - # A manifest is a Hash of the format defined by Chef::CookbookVersion#manifest - let(:manifest) { {:cookbook_name => "bob"} } - context "when configured to fetch files over http" do let(:http) { double("Chef::REST") } @@ -40,19 +37,42 @@ expect(file_vendor_class.initialization_options).to eq(http) end - it "creates a RemoteFileVendor for a given manifest" do - file_vendor = file_vendor_class.create_from_manifest(manifest) - expect(file_vendor).to be_a_kind_of(Chef::Cookbook::RemoteFileVendor) - expect(file_vendor.rest).to eq(http) - expect(file_vendor.cookbook_name).to eq("bob") + context "with a manifest from a cookbook version" do + + # A manifest is a Hash of the format defined by Chef::CookbookVersion#manifest + let(:manifest) { {:cookbook_name => "bob", :name => "bob-1.2.3"} } + + it "creates a RemoteFileVendor for a given manifest" do + file_vendor = file_vendor_class.create_from_manifest(manifest) + expect(file_vendor).to be_a_kind_of(Chef::Cookbook::RemoteFileVendor) + expect(file_vendor.rest).to eq(http) + expect(file_vendor.cookbook_name).to eq("bob") + end + end + context "with a manifest from a cookbook artifact" do + + # A manifest is a Hash of the format defined by Chef::CookbookVersion#manifest + let(:manifest) { {:name => "bob"} } + + it "creates a RemoteFileVendor for a given manifest" do + file_vendor = file_vendor_class.create_from_manifest(manifest) + expect(file_vendor).to be_a_kind_of(Chef::Cookbook::RemoteFileVendor) + expect(file_vendor.rest).to eq(http) + expect(file_vendor.cookbook_name).to eq("bob") + end + + end end context "when configured to load files from disk" do let(:cookbook_path) { %w[/var/chef/cookbooks /var/chef/other_cookbooks] } + # A manifest is a Hash of the format defined by Chef::CookbookVersion#manifest + let(:manifest) { {:cookbook_name => "bob"} } + before do file_vendor_class.fetch_from_disk(cookbook_path) end