Merge pull request #3991 from chef/jdm/remote-file-sha2

Modify remote_file cache_control_data to use sha256 for its name 

2 files changed, 99 insertions, 40 deletions

diff --git a/lib/chef/provider/remote_file/cache_control_data.rb b/lib/chef/provider/remote_file/cache_control_data.rb
index f9b729362c..3f39dac625 100644
--- a/lib/chef/provider/remote_file/cache_control_data.rb
+++ b/lib/chef/provider/remote_file/cache_control_data.rb
@@ -145,18 +145,51 @@ class Chef
         end
 
         def load_json_data
-          Chef::FileCache.load("remote_file/#{sanitized_cache_file_basename}")
+          path = sanitized_cache_file_path(sanitized_cache_file_basename)
+          if Chef::FileCache.has_key?(path)
+            Chef::FileCache.load(path)
+          else
+            old_path = sanitized_cache_file_path(sanitized_cache_file_basename_md5)
+            if Chef::FileCache.has_key?(old_path)
+              # We found an old cache control data file. We started using sha256 instead of md5
+              # to name these. Upgrade the file to the new name.
+              Chef::Log.debug("Found old cache control data file at #{old_path}. Moving to #{path}.")
+              Chef::FileCache.load(old_path).tap do |data|
+                Chef::FileCache.store(path, data)
+                Chef::FileCache.delete(old_path)
+              end
+            else
+              raise Chef::Exceptions::FileNotFound
+            end
+          end
         end
 
-        def sanitized_cache_file_basename
+        def sanitized_cache_file_path(basename)
+          "remote_file/#{basename}"
+        end
+
+        def scrubbed_uri
           # Scrub and truncate in accordance with the goals of keeping the name
           # human-readable but within the bounds of local file system
           # path length limits
-          scrubbed_uri = uri.gsub(/\W/, '_')[0..63]
+          uri.gsub(/\W/, '_')[0..63]
+        end
+
+        def sanitized_cache_file_basename
+          uri_sha2 = Chef::Digester.instance.generate_checksum(StringIO.new(uri))
+          cache_file_basename(uri_sha2[0,32])
+        end
+
+
+        def sanitized_cache_file_basename_md5
+          # Old way of creating the file basename
           uri_md5 = Chef::Digester.instance.generate_md5_checksum(StringIO.new(uri))
-          "#{scrubbed_uri}-#{uri_md5}.json"
+          cache_file_basename(uri_md5)
         end
 
+        def cache_file_basename(checksum)
+          "#{scrubbed_uri}-#{checksum}.json"
+        end
       end
     end
   end
diff --git a/spec/unit/provider/remote_file/cache_control_data_spec.rb b/spec/unit/provider/remote_file/cache_control_data_spec.rb
index 11f2af3edc..c154c4228e 100644
--- a/spec/unit/provider/remote_file/cache_control_data_spec.rb
+++ b/spec/unit/provider/remote_file/cache_control_data_spec.rb
@@ -20,12 +20,12 @@ require 'spec_helper'
 require 'uri'
 
 CACHE_FILE_TRUNCATED_FRIENDLY_FILE_NAME_LENGTH = 64
-CACHE_FILE_MD5_HEX_LENGTH = 32
+CACHE_FILE_CHECKSUM_HEX_LENGTH = 32
 CACHE_FILE_JSON_FILE_EXTENSION_LENGTH = 5
 CACHE_FILE_PATH_LIMIT =
   CACHE_FILE_TRUNCATED_FRIENDLY_FILE_NAME_LENGTH +
   1 +
-  CACHE_FILE_MD5_HEX_LENGTH +
+  CACHE_FILE_CHECKSUM_HEX_LENGTH +
   CACHE_FILE_JSON_FILE_EXTENSION_LENGTH # {friendly}-{md5hex}.json == 102
 
 describe Chef::Provider::RemoteFile::CacheControlData do
@@ -36,7 +36,8 @@ describe Chef::Provider::RemoteFile::CacheControlData do
     Chef::Provider::RemoteFile::CacheControlData.load_and_validate(uri, current_file_checksum)
   end
 
-  let(:cache_path) { "remote_file/http___www_google_com_robots_txt-9839677abeeadf0691026e0cabca2339.json" }
+  let(:cache_path) { "remote_file/http___www_google_com_robots_txt-6dc1b24315d0cff764d30344199c6f7b.json" }
+  let(:old_cache_path) { "remote_file/http___www_google_com_robots_txt-9839677abeeadf0691026e0cabca2339.json" }
 
   # the checksum of the file we have on disk already
   let(:current_file_checksum) { "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" }
@@ -44,7 +45,8 @@ describe Chef::Provider::RemoteFile::CacheControlData do
   context "when loading data for an unknown URI" do
 
     before do
-      expect(Chef::FileCache).to receive(:load).with(cache_path).and_raise(Chef::Exceptions::FileNotFound, "nope")
+      expect(Chef::FileCache).to receive(:has_key?).with(cache_path).and_return(false)
+      expect(Chef::FileCache).to receive(:has_key?).with(old_cache_path).and_return(false)
     end
 
     context "and there is no current copy of the file" do
@@ -64,7 +66,8 @@ describe Chef::Provider::RemoteFile::CacheControlData do
     context "and the URI contains a password" do
 
       let(:uri) { URI.parse("http://bob:password@example.org/") }
-      let(:cache_path) { "remote_file/http___bob_XXXX_example_org_-f121caacb74c05a35bcefdf578ed5fc9.json" }
+      let(:cache_path) { "remote_file/http___bob_XXXX_example_org_-44be109aa176a165ef599c12d97af792.json" }
+      let(:old_cache_path) { "remote_file/http___bob_XXXX_example_org_-f121caacb74c05a35bcefdf578ed5fc9.json" }
 
       it "loads the cache data from a path based on a sanitized URI" do
         Chef::Provider::RemoteFile::CacheControlData.load_and_validate(uri, current_file_checksum)
@@ -88,51 +91,73 @@ describe Chef::Provider::RemoteFile::CacheControlData do
       Chef::JSONCompat.to_json(cache)
     end
 
-    before do
-      expect(Chef::FileCache).to receive(:load).with(cache_path).and_return(cache_json_data)
-    end
-
-    context "and there is no on-disk copy of the file" do
-      let(:current_file_checksum) { nil }
-
-      it "returns empty cache control data" do
-        expect(cache_control_data.etag).to be_nil
-        expect(cache_control_data.mtime).to be_nil
+    context "when the cache control data uses sha256 for its name" do
+      before do
+        expect(Chef::FileCache).to receive(:has_key?).with(cache_path).and_return(true)
+        expect(Chef::FileCache).to receive(:load).with(cache_path).and_return(cache_json_data)
       end
-    end
 
-    context "and the cached checksum does not match the on-disk copy" do
-      let(:current_file_checksum) { "e2a8938cc31754f6c067b35aab1d0d4864272e9bf8504536ef3e79ebf8432305" }
+      context "and there is no on-disk copy of the file" do
+        let(:current_file_checksum) { nil }
 
-      it "returns empty cache control data" do
-        expect(cache_control_data.etag).to be_nil
-        expect(cache_control_data.mtime).to be_nil
+        it "returns empty cache control data" do
+          expect(cache_control_data.etag).to be_nil
+          expect(cache_control_data.mtime).to be_nil
+        end
       end
-    end
 
-    context "and the cached checksum matches the on-disk copy" do
+      context "and the cached checksum does not match the on-disk copy" do
+        let(:current_file_checksum) { "e2a8938cc31754f6c067b35aab1d0d4864272e9bf8504536ef3e79ebf8432305" }
 
-      it "populates the cache control data" do
-        expect(cache_control_data.etag).to eq(etag)
-        expect(cache_control_data.mtime).to eq(mtime)
+        it "returns empty cache control data" do
+          expect(cache_control_data.etag).to be_nil
+          expect(cache_control_data.mtime).to be_nil
+        end
       end
-    end
-
-    context "and the cached checksum data is corrupted" do
-      let(:cache_json_data) { '{"foo",,"bar" []}' }
 
-      it "returns empty cache control data" do
-        expect(cache_control_data.etag).to be_nil
-        expect(cache_control_data.mtime).to be_nil
+      context "and the cached checksum matches the on-disk copy" do
+        context "when the filename uses sha256" do
+          before do
+            expect(Chef::FileCache).not_to receive(:has_key?).with(old_cache_path)
+          end
+          it "populates the cache control data" do
+            expect(cache_control_data.etag).to eq(etag)
+            expect(cache_control_data.mtime).to eq(mtime)
+          end
+        end
       end
 
-      context "and it still is valid JSON" do
-        let(:cache_json_data) { '' }
+      context "and the cached checksum data is corrupted" do
+        let(:cache_json_data) { '{"foo",,"bar" []}' }
 
         it "returns empty cache control data" do
           expect(cache_control_data.etag).to be_nil
           expect(cache_control_data.mtime).to be_nil
         end
+
+        context "and it still is valid JSON" do
+          let(:cache_json_data) { '' }
+
+          it "returns empty cache control data" do
+            expect(cache_control_data.etag).to be_nil
+            expect(cache_control_data.mtime).to be_nil
+          end
+        end
+      end
+    end
+
+    context "when the filename uses md5" do
+      before do
+        expect(Chef::FileCache).to receive(:has_key?).with(cache_path).and_return(false)
+        expect(Chef::FileCache).to receive(:has_key?).with(old_cache_path).and_return(true)
+        expect(Chef::FileCache).to receive(:load).with(old_cache_path).and_return(cache_json_data)
+      end
+
+      it "populates the cache control data and creates the cache control data file with the correct path" do
+        expect(Chef::FileCache).to receive(:store).with(cache_path, cache_json_data)
+        expect(Chef::FileCache).to receive(:delete).with(old_cache_path)
+        expect(cache_control_data.etag).to eq(etag)
+        expect(cache_control_data.mtime).to eq(mtime)
       end
     end
   end
@@ -174,7 +199,8 @@ describe Chef::Provider::RemoteFile::CacheControlData do
     context "and the URI contains a password" do
 
       let(:uri) { URI.parse("http://bob:password@example.org/") }
-      let(:cache_path) { "remote_file/http___bob_XXXX_example_org_-f121caacb74c05a35bcefdf578ed5fc9.json" }
+      let(:cache_path) { "remote_file/http___bob_XXXX_example_org_-44be109aa176a165ef599c12d97af792.json" }
+      let(:old_cache_path) { "remote_file/http___bob_XXXX_example_org_-f121caacb74c05a35bcefdf578ed5fc9.json" }
 
       it "writes the data to the cache with a sanitized path name" do
         json_data = cache_control_data.json_data