Update policyfile URLs and cookbook artifact data format per RFC

6 files changed, 114 insertions, 23 deletions

diff --git a/lib/chef/cookbook_manifest.rb b/lib/chef/cookbook_manifest.rb
index 0d21e9725c..10654a4945 100644
--- a/lib/chef/cookbook_manifest.rb
+++ b/lib/chef/cookbook_manifest.rb
@@ -36,6 +36,7 @@ class Chef
     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 @@ class Chef
     # 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 @@ class Chef
         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 b8f32a61bb..8d302eeec2 100644
--- a/lib/chef/cookbook_version.rb
+++ b/lib/chef/cookbook_version.rb
@@ -78,6 +78,16 @@ class Chef
 
     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 @@ class Chef
       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 d368b055f7..f85d72f121 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -239,7 +239,7 @@ class Chef
       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 @@ class Chef
       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 938f72c743..f985942e09 100644
--- a/spec/unit/cookbook_manifest_spec.rb
+++ b/spec/unit/cookbook_manifest_spec.rb
@@ -24,6 +24,8 @@ describe Chef::CookbookManifest do
 
   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 @@ describe Chef::CookbookManifest do
   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 @@ describe Chef::CookbookManifest do
 
       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 152e5373f0..76727c18e2 100644
--- a/spec/unit/cookbook_uploader_spec.rb
+++ b/spec/unit/cookbook_uploader_spec.rb
@@ -25,11 +25,17 @@ describe Chef::CookbookUploader do
   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 @@ describe Chef::CookbookUploader do
       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 8b6e928a46..e4f7388a1c 100644
--- a/spec/unit/policy_builder/policyfile_spec.rb
+++ b/spec/unit/policy_builder/policyfile_spec.rb
@@ -256,7 +256,7 @@ describe Chef::PolicyBuilder::Policyfile do
 
       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 @@ describe Chef::PolicyBuilder::Policyfile do
 
       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 @@ describe Chef::PolicyBuilder::Policyfile do
         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 @@ describe Chef::PolicyBuilder::Policyfile do
             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 @@ describe Chef::PolicyBuilder::Policyfile do
               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 @@ describe Chef::PolicyBuilder::Policyfile do
         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 @@ describe Chef::PolicyBuilder::Policyfile do
             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