From 221b4715b645fdcf8adbdfc6577e1d7f6871ac2c Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Mon, 28 Jul 2014 23:40:10 -0700 Subject: decide if we're creating/unlinking up front --- lib/chef/provider/file.rb | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) (limited to 'lib/chef') diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index 2dffcc6ddc..5b7ae0f75d 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -58,6 +58,9 @@ class Chef attr_reader :deployment_strategy + attr_accessor :needs_creating + attr_accessor :needs_unlinking + def initialize(new_resource, run_context) @content_class ||= Chef::Provider::File::Content if new_resource.respond_to?(:atomic_update) @@ -82,11 +85,17 @@ class Chef load_resource_attributes_from_file(@current_resource) end @current_resource + + # true if there is a non-file thing in the way that we need to unlink first + @needs_unlinking = @new_resource.force_unlink && l_exist?(@new_resource.path) && !real_file?(@new_resource.path) + # true if we are going to be creating a new file + @needs_creating = !::File.exist?(@new_resource.path) end def define_resource_requirements # deep inside FAC we have to assert requirements, so call FACs hook to set that up access_controls.define_resource_requirements + # Make sure the parent directory exists, otherwise fail. For why-run assume it would have been created. requirements.assert(:create, :create_if_missing, :touch) do |a| parent_directory = ::File.dirname(@new_resource.path) @@ -96,7 +105,7 @@ class Chef end # Make sure the file is deletable if it exists, otherwise fail. - if ::File.exists?(@new_resource.path) + if ::File.exist?(@new_resource.path) requirements.assert(:delete) do |a| a.assertion { ::File.writable?(@new_resource.path) } a.failure_message(Chef::Exceptions::InsufficientPermissions,"File #{@new_resource.path} exists but is not writable so it cannot be deleted") @@ -125,10 +134,10 @@ class Chef end def action_create_if_missing - if ::File.exists?(@new_resource.path) - Chef::Log.debug("#{@new_resource} exists at #{@new_resource.path} taking no action.") - else + if needs_creating? action_create + else + Chef::Log.debug("#{@new_resource} exists at #{@new_resource.path} taking no action.") end end @@ -294,40 +303,27 @@ class Chef end def do_unlink - @file_unlinked = false if @new_resource.force_unlink - if l_exist?(@new_resource.path) && !real_file?(@new_resource.path) + if needs_unlinking? # unlink things that aren't normal files description = "unlink #{file_type_string(@new_resource.path)} at #{@new_resource.path}" converge_by(description) do unlink(@new_resource.path) end @current_resource.checksum = nil - @file_unlinked = true end end end - def file_unlinked? - @file_unlinked == true - end - def do_create_file - @file_created = false - if !::File.exists?(@new_resource.path) || file_unlinked? + if needs_creating? || needs_unlinking? converge_by("create new file #{@new_resource.path}") do deployment_strategy.create(@new_resource.path) Chef::Log.info("#{@new_resource} created file #{@new_resource.path}") end - @file_created = true end end - # do_contents_changes needs to know if do_create_file created a file or not - def file_created? - @file_created == true - end - def do_backup(file = nil) Chef::Util::Backup.new(@new_resource, file).backup! end @@ -337,7 +333,7 @@ class Chef end def update_file_contents - do_backup unless file_created? + do_backup unless needs_creating? || needs_unlinking? deployment_strategy.deploy(tempfile.path, ::File.realpath(@new_resource.path)) Chef::Log.info("#{@new_resource} updated file contents #{@new_resource.path}") if managing_content? @@ -364,7 +360,7 @@ class Chef description << "suppressed sensitive resource" else diff.diff(@current_resource.path, tempfile.path) - @new_resource.diff( diff.for_reporting ) unless file_created? + @new_resource.diff( diff.for_reporting ) unless needs_creating? || needs_unlinking? description << diff.for_output end @@ -417,7 +413,6 @@ class Chef end def load_resource_attributes_from_file(resource) - if Chef::Platform.windows? # This is a work around for CHEF-3554. # OC-6534: is tracking the real fix for this workaround. @@ -429,6 +424,13 @@ class Chef acl_scanner.set_all! end + def needs_creating? + !!@needs_creating + end + + def needs_unlinking? + !!@needs_unlinking + end end end end -- cgit v1.2.1 From ccfa237afe8609dc34316c68de715c89240b9230 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Tue, 29 Jul 2014 11:14:57 -0700 Subject: generate content up front --- lib/chef/provider/file.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'lib/chef') diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index 5b7ae0f75d..2988902f48 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -125,6 +125,7 @@ class Chef end def action_create + do_generate_content do_unlink do_create_file do_contents_changes @@ -302,6 +303,10 @@ class Chef end end + def do_generate_content + tempfile + end + def do_unlink if @new_resource.force_unlink if needs_unlinking? @@ -404,7 +409,7 @@ class Chef end def tempfile - content.tempfile + @tempfile ||= content.tempfile end def short_cksum(checksum) -- cgit v1.2.1 From 760fe11fb5cb13c27c2e83a6b3ea075de833cd20 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 30 Jul 2014 15:59:00 -0700 Subject: bypass idempotency in the cookbook_collection class yo dawg, I put some idempotency in your idempotency because it heard you really liked idempotency. the file provider already handles this, the cookbook_collection does not need to re-checksum the source to try to be idempotent, which is what the last argument to the destination path does. here we just want to lookup a file in the cookbook cache and fetch it if necessary and return the path into the cache. the cookbook cache should not need to know about where we're going to be deploying it. as a side effect this avoids a second needless checksum. --- lib/chef/provider/cookbook_file/content.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/chef') diff --git a/lib/chef/provider/cookbook_file/content.rb b/lib/chef/provider/cookbook_file/content.rb index cb777dd916..9f49ba885c 100644 --- a/lib/chef/provider/cookbook_file/content.rb +++ b/lib/chef/provider/cookbook_file/content.rb @@ -28,7 +28,7 @@ class Chef def file_for_provider cookbook = run_context.cookbook_collection[resource_cookbook] - file_cache_location = cookbook.preferred_filename_on_disk_location(run_context.node, :files, @new_resource.source, @new_resource.path) + file_cache_location = cookbook.preferred_filename_on_disk_location(run_context.node, :files, @new_resource.source) if file_cache_location.nil? nil else -- cgit v1.2.1 From afc5473a1a2a8a8c4c89437dfc590b7a3276feee Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 30 Jul 2014 16:31:13 -0700 Subject: add comment on magic behavior --- lib/chef/provider/file.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'lib/chef') diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index 2988902f48..5c3e83ce52 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -304,6 +304,7 @@ class Chef end def do_generate_content + # referencing the tempfile magically causes content to be generated tempfile end -- cgit v1.2.1 From a1bed084e17adfd2a1a2f1026628bed8e2304fd2 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 30 Jul 2014 17:44:08 -0700 Subject: some coderage cleanup now i can sort of understand what manage_symlink_source actually does and this reads correct to me. --- lib/chef/provider/file.rb | 63 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 16 deletions(-) (limited to 'lib/chef') diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index 5c3e83ce52..bae8e107ad 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -75,21 +75,40 @@ class Chef def load_current_resource # Let children resources override constructing the @current_resource - @current_resource ||= Chef::Resource::File.new(@new_resource.name) - @current_resource.path(@new_resource.path) - if ::File.exists?(@current_resource.path) && ::File.file?(::File.realpath(@current_resource.path)) + @current_resource ||= Chef::Resource::File.new(new_resource.name) + current_resource.path(new_resource.path) + + # true if there is a symlink and we need to manage what it points at + @managing_symlink = file_class.symlink?(new_resource.path) && ( new_resource.manage_symlink_source || new_resource.manage_symlink_source.nil? ) + + # true if there is a non-file thing in the way that we need to unlink first + @needs_unlinking = + if ::File.exist?(new_resource.path) + if managing_symlink? + !symlink_to_real_file?(new_resource.path) + else + !real_file?(new_resource.path) + end + else + false + end + + # true if we are going to be creating a new file + @needs_creating = !::File.exist?(new_resource.path) || needs_unlinking? + + if !needs_creating? + # we are updating an existing file if managing_content? - Chef::Log.debug("#{@new_resource} checksumming file at #{@new_resource.path}.") - @current_resource.checksum(checksum(@current_resource.path)) + Chef::Log.debug("#{new_resource} checksumming file at #{new_resource.path}.") + current_resource.checksum(checksum(current_resource.path)) + else + # if the file does not exist or is not a file, then the checksum is invalid/pointless + current_resource.checksum(nil) end - load_resource_attributes_from_file(@current_resource) + load_resource_attributes_from_file(current_resource) end - @current_resource - # true if there is a non-file thing in the way that we need to unlink first - @needs_unlinking = @new_resource.force_unlink && l_exist?(@new_resource.path) && !real_file?(@new_resource.path) - # true if we are going to be creating a new file - @needs_creating = !::File.exist?(@new_resource.path) + current_resource end def define_resource_requirements @@ -135,7 +154,7 @@ class Chef end def action_create_if_missing - if needs_creating? + unless ::File.exist?(@new_resource.path) action_create else Chef::Log.debug("#{@new_resource} exists at #{@new_resource.path} taking no action.") @@ -288,6 +307,15 @@ class Chef !file_class.symlink?(path) && ::File.file?(path) end + # like real_file? that follows (sane) symlinks + def symlink_to_real_file?(path) + begin + real_file?(::File.realpath(path)) + rescue Errno::ELOOP, Errno::ENOENT + false + end + end + # Similar to File.exist?, but also returns true in the case that the # named file is a broken symlink. def l_exist?(path) @@ -316,13 +344,12 @@ class Chef converge_by(description) do unlink(@new_resource.path) end - @current_resource.checksum = nil end end end def do_create_file - if needs_creating? || needs_unlinking? + if needs_creating? converge_by("create new file #{@new_resource.path}") do deployment_strategy.create(@new_resource.path) Chef::Log.info("#{@new_resource} created file #{@new_resource.path}") @@ -339,7 +366,7 @@ class Chef end def update_file_contents - do_backup unless needs_creating? || needs_unlinking? + do_backup unless needs_creating? deployment_strategy.deploy(tempfile.path, ::File.realpath(@new_resource.path)) Chef::Log.info("#{@new_resource} updated file contents #{@new_resource.path}") if managing_content? @@ -366,7 +393,7 @@ class Chef description << "suppressed sensitive resource" else diff.diff(@current_resource.path, tempfile.path) - @new_resource.diff( diff.for_reporting ) unless needs_creating? || needs_unlinking? + @new_resource.diff( diff.for_reporting ) unless needs_creating? description << diff.for_output end @@ -430,6 +457,10 @@ class Chef acl_scanner.set_all! end + def managing_symlink? + !!@managing_symlink + end + def needs_creating? !!@needs_creating end -- cgit v1.2.1 From 900d23fdbab5137309ea458c17ed079863b5dc07 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 30 Jul 2014 18:45:53 -0700 Subject: validate checksums on downloads --- lib/chef/exceptions.rb | 6 ++++++ lib/chef/provider/file.rb | 14 +++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) (limited to 'lib/chef') diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb index 22fafaa4dc..e3c7d630ce 100644 --- a/lib/chef/exceptions.rb +++ b/lib/chef/exceptions.rb @@ -331,6 +331,12 @@ class Chef end end + class ChecksumMismatch < RuntimeError + def initialize(res_cksum, cont_cksum) + super "Checksum on resource (#{res_cksum}) does not match checksum on content (#{cont_cksum})" + end + end + class BadProxyURI < RuntimeError; end end end diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index bae8e107ad..f35df07c1f 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -60,6 +60,7 @@ class Chef attr_accessor :needs_creating attr_accessor :needs_unlinking + attr_accessor :managing_symlink def initialize(new_resource, run_context) @content_class ||= Chef::Provider::File::Content @@ -145,6 +146,7 @@ class Chef def action_create do_generate_content + do_validate_content do_unlink do_create_file do_contents_changes @@ -336,6 +338,16 @@ class Chef tempfile end + def tempfile_checksum + @tempfile_checksum ||= checksum(tempfile.path) + end + + def do_validate_content + if new_resource.checksum && tempfile && ( new_resource.checksum != tempfile_checksum ) + raise Chef::Exceptions::ChecksumMismatch.new(short_cksum(new_resource.checksum), short_cksum(tempfile_checksum)) + end + end + def do_unlink if @new_resource.force_unlink if needs_unlinking? @@ -433,7 +445,7 @@ class Chef def contents_changed? Chef::Log.debug "calculating checksum of #{tempfile.path} to compare with #{@current_resource.checksum}" - checksum(tempfile.path) != @current_resource.checksum + tempfile_checksum != @current_resource.checksum end def tempfile -- cgit v1.2.1 From a37de1bdfc364b83564774dc6d032908d3782b61 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 30 Jul 2014 18:50:13 -0700 Subject: small code rearrangement, avoid re-checksumming --- lib/chef/provider/file.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'lib/chef') diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb index f35df07c1f..e7c5ca856f 100644 --- a/lib/chef/provider/file.rb +++ b/lib/chef/provider/file.rb @@ -75,10 +75,6 @@ class Chef end def load_current_resource - # Let children resources override constructing the @current_resource - @current_resource ||= Chef::Resource::File.new(new_resource.name) - current_resource.path(new_resource.path) - # true if there is a symlink and we need to manage what it points at @managing_symlink = file_class.symlink?(new_resource.path) && ( new_resource.manage_symlink_source || new_resource.manage_symlink_source.nil? ) @@ -97,6 +93,10 @@ class Chef # true if we are going to be creating a new file @needs_creating = !::File.exist?(new_resource.path) || needs_unlinking? + # Let children resources override constructing the @current_resource + @current_resource ||= Chef::Resource::File.new(new_resource.name) + current_resource.path(new_resource.path) + if !needs_creating? # we are updating an existing file if managing_content? @@ -397,7 +397,7 @@ class Chef # the file? on the next line suppresses the case in why-run when we have a not-file here that would have otherwise been removed if ::File.file?(@new_resource.path) && contents_changed? description = [ "update content in file #{@new_resource.path} from \ -#{short_cksum(@current_resource.checksum)} to #{short_cksum(checksum(tempfile.path))}" ] +#{short_cksum(@current_resource.checksum)} to #{short_cksum(tempfile_checksum)}" ] # Hide the diff output if the resource is marked as a sensitive resource if @new_resource.sensitive -- cgit v1.2.1