Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-9389

Add new parameter to File for tempfile staging location



    • Type: New Feature
    • Status: Resolved
    • Priority: Normal
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: PUP 6.5.0
    • Component/s: None
    • Template:
    • Acceptance Criteria:

      A new parameter to the file type which allows users to render a file to a different location, run validation, and after validation works move to a new directory.

      A new parameter to the file type which allows users to render a file to a different location, run validation, and after validation works move to a new directory.
    • Team:
    • Sprint:
      Platform Core KANBAN
    • Release Notes:
    • Release Notes Summary:
      The temporary location used when creating the new content for a File resource can now be customized using the `staging_location` parameter.
    • QA Risk Assessment:
      Needs Assessment


      When Puppet creates a new file it creates it in a temporary location. This location is currently the location the file will exist in plus a unique name, eg /etc/foo123fsd for /etc/foo . In cases where a user wants to run a validate_cmd against the file it should be possible to render this file in a different location, run the validation, and then if the validation passes move the file to it's desired location.

      This avoids issues where, for example, an application reads all the files in a directory when performing a task (think sudo) or an application reloads configs automatically from a directory.

      The changes to make this possible seems doable and I'd be happy to submit a PR but wanted to post a feature here first to ensure this is something that would be acceptable to the Puppet community.

      The change would look something like the following, diffed against Puppet 4.5.2. The replace_file function is used in many other places so would require other modifications, just giving a brief idea of what this might look like (open to other param names)

      diff --git a/vendor/puppet-4.5.2/lib/puppet/type/file.rb b/vendor/puppet-4.5.2/lib/puppet/type/file.rb
      index 8a29286dce..131e6549b8 100644
      --- a/vendor/puppet-4.5.2/lib/puppet/type/file.rb
      +++ b/vendor/puppet-4.5.2/lib/puppet/type/file.rb
      @@ -324,6 +324,29 @@ Puppet::Type.newtype(:file) do
           defaultto '%'
      +  newparam(:staging_location) do
      +    desc "For use with validate_cmd parameter. When running validate_cmd first
      +    render to this staging location. This prevents rendering a temporary file in a location
      +    where it may break a configuration before running a test against the file to ensure that
      +    it is valid"
      +    validate do |value|
      +      unless Puppet::Util.absolute_path?(value)
      +        fail Puppet::Error, "File paths must be fully qualified, not '#{value}'"
      +      end
      +    end
      +    munge do |value|
      +      if value.start_with?('//') and ::File.basename(value) == "/"
      +        # This is a UNC path pointing to a share, so don't add a trailing slash
      +        ::File.expand_path(value)
      +      else
      +        ::File.join(::File.split(::File.expand_path(value)))
      +      end
      +    end
      +  end
         # Autorequire the nearest ancestor directory found in the catalog.
         autorequire(:file) do
           req = []
      @@ -849,7 +872,7 @@ Puppet::Type.newtype(:file) do
           mode_int = mode ? symbolic_mode_to_int(mode, Puppet::Util::DEFAULT_POSIX_MODE) : nil
           if write_temporary_file?
      -      Puppet::Util.replace_file(self[:path], mode_int) do |file|
      +      Puppet::Util.replace_file(self[:path], mode_int, self[:staging_location]) do |file|
      diff --git a/vendor/puppet-4.5.2/lib/puppet/util.rb b/vendor/puppet-4.5.2/lib/puppet/util.rb
      index 011f2108e8..e8818e6906 100644
      --- a/vendor/puppet-4.5.2/lib/puppet/util.rb
      +++ b/vendor/puppet-4.5.2/lib/puppet/util.rb
      @@ -419,7 +419,7 @@ module Util
         # preserved.  This works hard to avoid loss of any metadata, but will result
         # in an inode change for the file.
      -  # Arguments: `filename`, `default_mode`
      +  # Arguments: `filename`, `default_mode`, `staging_location`
         # The filename is the file we are going to replace.
      @@ -431,7 +431,7 @@ module Util
         DEFAULT_POSIX_MODE = 0644
      -  def replace_file(file, default_mode, &block)
      +  def replace_file(file, default_mode, staging_location, &block)
           raise Puppet::DevError, "replace_file requires a block" unless block_given?
           if default_mode
      @@ -449,8 +449,12 @@ module Util
      -      file     = Puppet::FileSystem.pathname(file)
      -      tempfile = Puppet::FileSystem::Uniquefile.new(Puppet::FileSystem.basename_string(file), Puppet::FileSystem.dir_string(file))
      +      file = Puppet::FileSystem.pathname(file)
      +      if staging_location
      +        tempfile = Puppet::FileSystem::Uniquefile.new(Puppet::FileSystem.basename_string(file), staging_location)
      +      else
      +        tempfile = Puppet::FileSystem::Uniquefile.new(Puppet::FileSystem.basename_string(file), Puppet::FileSystem.dir_string(file))
      +      end
             # Set properties of the temporary file before we write the content, because
             # Tempfile doesn't promise to be safe from reading by other people, just





            jacob.helwig Jacob Helwig
            bkochendorfer Brett Kochendorfer
            1 Vote for this issue
            4 Start watching this issue



                Zendesk Support