From beafb0fbb00466b6b78254b8e997c30da3e06fd6 Mon Sep 17 00:00:00 2001 From: Hans Moulron Date: Tue, 6 Aug 2024 17:32:59 +0200 Subject: [PATCH] add onlyif && unless features lint remove debug fixe check_all_attributes lint --- .rubocop_todo.yml | 40 +++- REFERENCE.md | 88 ++++++++ lib/puppet/provider/archive/ruby.rb | 136 +++++++++-- lib/puppet/type/archive.rb | 213 +++++++++++++++++- manifests/nexus.pp | 16 +- spec/acceptance/authentication_spec.rb | 2 +- spec/lib/puppet_spec/compiler.rb | 113 ++++++++++ spec/lib/puppet_spec/files.rb | 111 +++++++++ spec/spec_helper.rb | 8 + .../unit/puppet/provider/archive/ruby_spec.rb | 97 +++++++- spec/unit/puppet/type/archive_spec.rb | 198 ++++++++++++++++ 11 files changed, 988 insertions(+), 34 deletions(-) create mode 100644 spec/lib/puppet_spec/compiler.rb create mode 100644 spec/lib/puppet_spec/files.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 14315837..de6418ee 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2023-08-17 21:26:09 UTC using RuboCop version 1.50.2. +# on 2024-08-06 17:00:26 UTC using RuboCop version 1.50.2. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -12,6 +12,44 @@ Lint/NonAtomicFileOperation: Exclude: - 'lib/puppet/provider/archive/ruby.rb' +# Offense count: 1 +Naming/AccessorMethodName: + Exclude: + - 'lib/puppet/type/archive.rb' + +# Offense count: 1 +# Configuration parameters: MinNameLength, AllowNamesEndingInNumbers, AllowedNames, ForbiddenNames. +# AllowedNames: as, at, by, cc, db, id, if, in, io, ip, of, on, os, pp, to +Naming/MethodParameterName: + Exclude: + - 'spec/lib/puppet_spec/files.rb' + +# Offense count: 9 +# Configuration parameters: AssignmentOnly. +RSpec/InstanceVariable: + Exclude: + - 'spec/unit/puppet/provider/archive/ruby_spec.rb' + - 'spec/unit/puppet/type/archive_spec.rb' + +# Offense count: 1 +# Configuration parameters: EnforcedStyle. +# SupportedStyles: have_received, receive +RSpec/MessageSpies: + Exclude: + - 'spec/lib/puppet_spec/compiler.rb' + +# Offense count: 4 +# Configuration parameters: IgnoreNameless, IgnoreSymbolicNames. +RSpec/VerifiedDoubles: + Exclude: + - 'spec/unit/puppet/type/archive_spec.rb' + +# Offense count: 4 +# Configuration parameters: AllowedVariables. +Style/GlobalVars: + Exclude: + - 'spec/lib/puppet_spec/files.rb' + # Offense count: 1 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: AllowComments. diff --git a/REFERENCE.md b/REFERENCE.md index 1a842069..2d69cc0a 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -632,6 +632,11 @@ Parameters Examples -------- +#### Examples + +##### + +```puppet archive::nexus { '/tmp/jtstand-ui-0.98.jar': url => 'https://oss.sonatype.org', gav => 'org.codehaus.jtstand:jtstand-ui:0.98', @@ -639,6 +644,7 @@ archive::nexus { '/tmp/jtstand-ui-0.98.jar': packaging => 'jar', extract => false, } +``` #### Parameters @@ -895,6 +901,72 @@ whether archive file should be present/absent (default: present) Default value: `present` +##### `onlyif` + +A test command that checks the state of the target system and restricts +when the `archive` can run. If present, Puppet runs this test command +first, and only runs the main command if the test has an exit code of 0 +(success). For example: + + ``` + archive { '/tmp/jta-1.1.jar': + ensure => present, + extract => true, + extract_path => '/tmp', + source => 'http://central.maven.org/maven2/javax/transaction/jta/1.1/jta-1.1.jar', + onlyif => 'test `java -version 2>&1 | awk -F '"' '/version/ {print $2}' | awk -F '.' '{sub("^$", "0", $2); print $1$2}'` -gt 15', + cleanup => true, + env_path => ["/bin", "/usr/bin", "/sbin", "/usr/sbin"], + } + ``` + +Since this command is used in the process of determining whether the +`archive` is already in sync, it must be run during a noop Puppet run. + +This parameter can also take an array of commands. For example: + + onlyif => ['test -f /tmp/file1', 'test -f /tmp/file2'], + +or an array of arrays. For example: + + onlyif => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2'] + +This `archive` would only run if every command in the array has an +exit code of 0 (success). + +##### `unless` + +A test command that checks the state of the target system and restricts +when the `archive` can run. If present, Puppet runs this test command +first, then runs the main command unless the test has an exit code of 0 +(success). For example: + + ``` + archive { '/tmp/jta-1.1.jar': + ensure => present, + extract => true, + extract_path => '/tmp', + source => 'http://central.maven.org/maven2/javax/transaction/jta/1.1/jta-1.1.jar', + unless => 'test `java -version 2>&1 | awk -F '"' '/version/ {print $2}' | awk -F '.' '{sub("^$", "0", $2); print $1$2}'` -gt 15', + cleanup => true, + env_path => ["/bin", "/usr/bin", "/sbin", "/usr/sbin"], + } + ``` + +Since this command is used in the process of determining whether the +`archive` is already in sync, it must be run during a noop Puppet run. + +This parameter can also take an array of commands. For example: + + unless => ['test -f /tmp/file1', 'test -f /tmp/file2'], + +or an array of arrays. For example: + + unless => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2'] + +This `archive` would only run if every command in the array has a +non-zero exit code. + #### Parameters The following parameters are available in the `archive` type. @@ -910,6 +982,8 @@ The following parameters are available in the `archive` type. * [`digest_type`](#-archive--digest_type) * [`digest_url`](#-archive--digest_url) * [`download_options`](#-archive--download_options) +* [`env_path`](#-archive--env_path) +* [`environment`](#-archive--environment) * [`extract`](#-archive--extract) * [`extract_command`](#-archive--extract_command) * [`extract_flags`](#-archive--extract_flags) @@ -998,6 +1072,20 @@ archive file checksum source (instead of specifying checksum) provider download options (affects curl, wget, gs, and only s3 downloads for ruby provider) +##### `env_path` + +The search path used for check execution. +Commands must be fully qualified if no path is specified. Paths +can be specified as an array or as a ' + +##### `environment` + +An array of any additional environment variables you want to set for a +command, such as `[ 'HOME=/root', 'MAIL=root@example.com']`. +Note that if you use this to set PATH, it will override the `path` +attribute. Multiple environment variables should be specified as an +array. + ##### `extract` Valid values: `true`, `false` diff --git a/lib/puppet/provider/archive/ruby.rb b/lib/puppet/provider/archive/ruby.rb index c83c2e41..021bc4ef 100644 --- a/lib/puppet/provider/archive/ruby.rb +++ b/lib/puppet/provider/archive/ruby.rb @@ -5,6 +5,7 @@ require 'securerandom' require 'tempfile' +require 'puppet/util/execution' # This provider implements a simple state-machine. The following attempts to # # document it. In general, `def adjective?` implements a [state], while `def @@ -59,6 +60,7 @@ # Puppet::Type.type(:archive).provide(:ruby) do + include Puppet::Util::Execution optional_commands aws: 'aws' optional_commands gsutil: 'gsutil' defaultfor feature: :microsoft_windows @@ -95,18 +97,6 @@ def tempfile_name end end - def creates - if resource[:extract] == :true - extracted? ? resource[:creates] : 'archive not extracted' - else - resource[:creates] - end - end - - def creates=(_value) - extract - end - def checksum resource[:checksum] || (resource[:checksum] = remote_checksum if resource[:checksum_url]) end @@ -127,7 +117,7 @@ def remote_checksum # returns boolean def checksum?(store_checksum = true) return false unless File.exist? archive_filepath - return true if resource[:checksum_type] == :none + return true if resource[:checksum_type] == :none archive = PuppetX::Bodeco::Archive.new(archive_filepath) archive_checksum = archive.checksum(resource[:checksum_type]) @@ -156,7 +146,7 @@ def extract end def extracted? - resource[:creates] && File.exist?(resource[:creates]) + resource.check_all_attributes end def transfer_download(archive_filepath) @@ -258,4 +248,122 @@ def optional_switch(value, option) [] end end + + # Verify that we have the executable + def checkexe(command) + exe = extractexe(command) + if Facter.value(:osfamily) == 'windows' + if absolute_path?(exe) + if !Puppet::FileSystem.exist?(exe) + raise ArgumentError, format(_("Could not find command '%{exe}'"), exe: exe) + elsif !File.file?(exe) + raise ArgumentError, format(_("'%{exe}' is a %{klass}, not a file"), exe: exe, klass: File.ftype(exe)) + end + end + elsif File.expand_path(exe) == exe + if !Puppet::FileSystem.exist?(exe) + raise ArgumentError, format(_("Could not find command '%{exe}'"), exe: exe) + elsif !File.file?(exe) + raise ArgumentError, format(_("'%{exe}' is a %{klass}, not a file"), exe: exe, klass: File.ftype(exe)) + elsif !File.executable?(exe) + raise ArgumentError, format(_("'%{exe}' is not executable"), exe: exe) + end + end + + if resource[:env_path] + Puppet::Util.withenv PATH: resource[:env_path].join(File::PATH_SEPARATOR) do + return if which(exe) + end + end + + # 'which' will only return the command if it's executable, so we can't + # distinguish not found from not executable + raise ArgumentError, format(_("Could not find command '%{exe}'"), exe: exe) + end + + def environment + env = {} + + if (path = resource[:env_path]) + env[:PATH] = path.join(File::PATH_SEPARATOR) + end + + return env unless (envlist = resource[:environment]) + + envlist = [envlist] unless envlist.is_a? Array + envlist.each do |setting| + unless (match = %r{^(\w+)=((.|\n)*)$}.match(setting)) + warning "Cannot understand environment setting #{setting.inspect}" + next + end + var = match[1] + value = match[2] + + warning "Overriding environment setting '#{var}' with '#{value}'" if env.include?(var) || env.include?(var.to_sym) + + if value.nil? || value.empty? + msg = "Empty environment setting '#{var}'" + Puppet.warn_once('undefined_variables', "empty_env_var_#{var}", msg, resource.file, resource.line) + end + + env[var] = value + end + + env + end + + def run(command, check = false) + checkexe(command) + + debug "Executing#{check ? ' check' : ''} #{command}" + + cwd = resource[:extract] ? resource[:extract_path] : File.dirname(resource[:path]) + # It's ok if cwd is nil. In that case Puppet::Util::Execution.execute() simply will not attempt to + # change the working directory, which is exactly the right behavior when no cwd parameter is + # expressed on the resource. Moreover, attempting to change to the directory that is already + # the working directory can fail under some circumstances, so avoiding the directory change attempt + # is preferable to defaulting cwd to that directory. + + # NOTE: that we are passing "false" for the "override_locale" parameter, which ensures that the user's + # default/system locale will be respected. Callers may override this behavior by setting locale-related + # environment variables (LANG, LC_ALL, etc.) in their 'environment' configuration. + output = Puppet::Util::Execution.execute( + command, + failonfail: false, + combine: true, + cwd: cwd, + uid: resource[:user], + gid: resource[:group], + override_locale: false, + custom_environment: environment, + sensitive: false + ) + # The shell returns 127 if the command is missing. + raise ArgumentError, output if output.exitstatus == 127 + + # Return output twice as processstatus was returned before, but only exitstatus was ever called. + # Output has the exitstatus on it so it is returned instead. This is here twice as changing this + # would result in a change to the underlying API. + [output, output] + end + + def extractexe(command) + if command.is_a? Array + command.first + else + match = %r{^"([^"]+)"|^'([^']+)'}.match(command) + if match + # extract whichever of the two sides matched the content. + match[1] or match[2] + else + command.split(%r{ })[0] + end + end + end + + def validatecmd(command) + exe = extractexe(command) + # if we're not fully qualified, require a path + self.fail "'#{exe}' is not qualified and no path was specified. Please qualify the command or specify a path." if !absolute_path?(exe) && resource[:path].nil? + end end diff --git a/lib/puppet/type/archive.rb b/lib/puppet/type/archive.rb index 6535e86b..db331184 100644 --- a/lib/puppet/type/archive.rb +++ b/lib/puppet/type/archive.rb @@ -8,6 +8,19 @@ Puppet::Type.newtype(:archive) do @doc = 'Manage archive file download, extraction, and cleanup.' + # Create a new check mechanism. It's basically a parameter that + # provides one extra 'check' method. + def self.newcheck(name, options = {}, &block) + @checks ||= {} + + check = newparam(name, options, &block) + @checks[name] = check + end + + def self.checks + @checks.keys + end + ensurable do desc 'whether archive file should be present/absent (default: present)' @@ -103,14 +116,6 @@ def change_to_s(currentvalue, newvalue) defaultto(:undef) end - newproperty(:creates) do - desc 'if file/directory exists, will not download/extract archive.' - - def should_to_s(value) - "extracting in #{resource[:extract_path]} to create #{value}" - end - end - newparam(:cleanup) do desc 'whether archive file will be removed after extraction (true|false).' newvalues(:true, :false) @@ -251,6 +256,174 @@ def should_to_s(value) end end + newparam(:env_path) do + desc "The search path used for check execution. + Commands must be fully qualified if no path is specified. Paths + can be specified as an array or as a '#{File::PATH_SEPARATOR}' separated list." + + # Support both arrays and colon-separated fields. + def value=(*values) + @value = values.flatten.map do |val| + val.split(File::PATH_SEPARATOR) + end.flatten + end + end + + newparam(:environment) do + desc "An array of any additional environment variables you want to set for a + command, such as `[ 'HOME=/root', 'MAIL=root@example.com']`. + Note that if you use this to set PATH, it will override the `path` + attribute. Multiple environment variables should be specified as an + array." + + validate do |values| + values = [values] unless values.is_a? Array + values.each do |value| + raise ArgumentError, "Invalid environment setting '#{value}'" unless value =~ %r{\w+=} + end + end + end + + newcheck(:creates, parent: Puppet::Parameter::Path) do + desc 'if file/directory exists, will not download/extract archive.' + + accept_arrays + + # If the file exists, return false (i.e., don't run the command), + # else return true + def check(value) + # TRANSLATORS 'creates' is a parameter name and should not be translated + debug("Checking that 'creates' path '#{value}' exists") + !Puppet::FileSystem.exist?(value) + end + end + + newcheck(:unless) do + desc <<-EOT + A test command that checks the state of the target system and restricts + when the `archive` can run. If present, Puppet runs this test command + first, then runs the main command unless the test has an exit code of 0 + (success). For example: + + ``` + archive { '/tmp/jta-1.1.jar': + ensure => present, + extract => true, + extract_path => '/tmp', + source => 'http://central.maven.org/maven2/javax/transaction/jta/1.1/jta-1.1.jar', + unless => 'test `java -version 2>&1 | awk -F '"' '/version/ {print $2}' | awk -F '.' '{sub("^$", "0", $2); print $1$2}'` -gt 15', + cleanup => true, + env_path => ["/bin", "/usr/bin", "/sbin", "/usr/sbin"], + } + ``` + + Since this command is used in the process of determining whether the + `archive` is already in sync, it must be run during a noop Puppet run. + + This parameter can also take an array of commands. For example: + + unless => ['test -f /tmp/file1', 'test -f /tmp/file2'], + + or an array of arrays. For example: + + unless => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2'] + + This `archive` would only run if every command in the array has a + non-zero exit code. + EOT + + validate do |cmds| + cmds = [cmds] unless cmds.is_a? Array + + cmds.each do |command| + provider.validatecmd(command) + end + end + + # Return true if the command does not return 0. + def check(value) + begin + output, status = provider.run(value, true) + rescue Timeout::Error + err format('Check %{value} exceeded timeout', value: value.inspect) + return false + end + + if sensitive + debug('[output redacted]') + else + output.split(%r{\n}).each do |line| + debug(line) + end + end + + status.exitstatus != 0 + end + end + + newcheck(:onlyif) do + desc <<-EOT + A test command that checks the state of the target system and restricts + when the `archive` can run. If present, Puppet runs this test command + first, and only runs the main command if the test has an exit code of 0 + (success). For example: + + ``` + archive { '/tmp/jta-1.1.jar': + ensure => present, + extract => true, + extract_path => '/tmp', + source => 'http://central.maven.org/maven2/javax/transaction/jta/1.1/jta-1.1.jar', + onlyif => 'test `java -version 2>&1 | awk -F '"' '/version/ {print $2}' | awk -F '.' '{sub("^$", "0", $2); print $1$2}'` -gt 15', + cleanup => true, + env_path => ["/bin", "/usr/bin", "/sbin", "/usr/sbin"], + } + ``` + + Since this command is used in the process of determining whether the + `archive` is already in sync, it must be run during a noop Puppet run. + + This parameter can also take an array of commands. For example: + + onlyif => ['test -f /tmp/file1', 'test -f /tmp/file2'], + + or an array of arrays. For example: + + onlyif => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2'] + + This `archive` would only run if every command in the array has an + exit code of 0 (success). + EOT + + validate do |cmds| + cmds = [cmds] unless cmds.is_a? Array + + cmds.each do |command| + provider.validatecmd(command) + end + end + + # Return true if the command returns 0. + def check(value) + begin + output, status = provider.run(value, true) + rescue Timeout::Error + err format('Check %{value} exceeded timeout', value: value.inspect) + return false + end + + if sensitive + debug('[output redacted]') + else + output.split(%r{\n}).each do |line| + debug(line) + end + end + + status.exitstatus.zero? + end + end + autorequire(:file) do [ Pathname.new(self[:path]).parent.to_s, @@ -280,4 +453,28 @@ def should_to_s(value) self[:proxy_type] = :none end end + + # Verify that we pass all of the checks. The argument determines whether + # we skip the :refreshonly check, which is necessary because we now check + # within refresh + def check_all_attributes + self.class.checks.each do |check| + next unless @parameters.include?(check) + + val = @parameters[check].value + val = [val] unless val.is_a? Array + val.each do |value| + next if @parameters[check].check(value) + + # Give a debug message so users can figure out what command would have been + # but don't print sensitive commands or parameters in the clear + sourcestring = @parameters[:source].sensitive ? '[command redacted]' : @parameters[:source].value + + debug("'#{sourcestring}' won't be executed because of failed check '#{check}'") + + return true + end + end + false + end end diff --git a/manifests/nexus.pp b/manifests/nexus.pp index 2a9395b8..312e63b7 100644 --- a/manifests/nexus.pp +++ b/manifests/nexus.pp @@ -9,14 +9,14 @@ # # Examples # -------- -# -# archive::nexus { '/tmp/jtstand-ui-0.98.jar': -# url => 'https://oss.sonatype.org', -# gav => 'org.codehaus.jtstand:jtstand-ui:0.98', -# repository => 'codehaus-releases', -# packaging => 'jar', -# extract => false, -# } +# @example +# archive::nexus { '/tmp/jtstand-ui-0.98.jar': +# url => 'https://oss.sonatype.org', +# gav => 'org.codehaus.jtstand:jtstand-ui:0.98', +# repository => 'codehaus-releases', +# packaging => 'jar', +# extract => false, +# } # define archive::nexus ( String $url, diff --git a/spec/acceptance/authentication_spec.rb b/spec/acceptance/authentication_spec.rb index ee9ce86f..fa28c6ae 100644 --- a/spec/acceptance/authentication_spec.rb +++ b/spec/acceptance/authentication_spec.rb @@ -6,7 +6,7 @@ context 'authenticated download' do let(:source) do parser = URI::RFC2396_Parser.new - parser.escape("http://httpbin.org/basic-auth/user/#{password}") + parser.escape("https://httpbin.org/basic-auth/user/#{password}") end let(:pp) do <<-EOS diff --git a/spec/lib/puppet_spec/compiler.rb b/spec/lib/puppet_spec/compiler.rb new file mode 100644 index 00000000..36361816 --- /dev/null +++ b/spec/lib/puppet_spec/compiler.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +# file from https://github.com/puppetlabs/puppet/blob/6.x/spec/lib/puppet_spec/compiler.rb + +module PuppetSpec::Compiler + module_function + + def compile_to_catalog(string, node = Puppet::Node.new('test')) + Puppet[:code] = string + # see lib/puppet/indirector/catalog/compiler.rb#filter + Puppet::Parser::Compiler.compile(node).filter(&:virtual?) + end + + # Does not removed virtual resources in compiled catalog (i.e. keeps unrealized) + def compile_to_catalog_unfiltered(string, node = Puppet::Node.new('test')) + Puppet[:code] = string + # see lib/puppet/indirector/catalog/compiler.rb#filter + Puppet::Parser::Compiler.compile(node) + end + + def compile_to_ral(manifest, node = Puppet::Node.new('test')) + catalog = compile_to_catalog(manifest, node) + ral = catalog.to_ral + ral.finalize + ral + end + + def compile_to_relationship_graph(manifest, prioritizer = Puppet::Graph::SequentialPrioritizer.new) + ral = compile_to_ral(manifest) + graph = Puppet::Graph::RelationshipGraph.new(prioritizer) + graph.populate_from(ral) + graph + end + + def apply_compiled_manifest(manifest, prioritizer = Puppet::Graph::SequentialPrioritizer.new, &block) + catalog = compile_to_ral(manifest) + catalog.resources.each(&block) if block_given? + transaction = Puppet::Transaction.new(catalog, + Puppet::Transaction::Report.new, + prioritizer) + transaction.evaluate + transaction.report.finalize_report + + transaction + end + + def apply_with_error_check(manifest) + apply_compiled_manifest(manifest) do |res| + expect(res).not_to receive(:err) + end + end + + def order_resources_traversed_in(relationships) + order_seen = [] + relationships.traverse { |resource| order_seen << resource.ref } + order_seen + end + + def collect_notices(code, node = Puppet::Node.new('foonode')) + Puppet[:code] = code + compiler = Puppet::Parser::Compiler.new(node) + node.environment.check_for_reparse + logs = [] + Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do + yield(compiler) + end + logs.select { |log| log.level == :notice }.map(&:message) + end + + def eval_and_collect_notices(code, node = Puppet::Node.new('foonode'), topscope_vars = {}) + collect_notices(code, node) do |compiler| + unless topscope_vars.empty? + scope = compiler.topscope + topscope_vars.each { |k, v| scope.setvar(k, v) } + end + if block_given? + compiler.compile do |catalog| + yield(compiler.topscope, catalog) + catalog + end + else + compiler.compile + end + end + end + + # Compiles a catalog, and if source is given evaluates it and returns its result. + # The catalog is returned if no source is given. + # Topscope variables are set before compilation + # Uses a created node 'testnode' if none is given. + # (Parameters given by name) + # + def evaluate(code: 'undef', source: nil, node: Puppet::Node.new('testnode'), variables: {}) + source_location = caller[0] + Puppet[:code] = code + compiler = Puppet::Parser::Compiler.new(node) + unless variables.empty? + scope = compiler.topscope + variables.each { |k, v| scope.setvar(k, v) } + end + + if source.nil? + compiler.compile + # see lib/puppet/indirector/catalog/compiler.rb#filter + return compiler.filter(&:virtual?) + end + + # evaluate given source is the context of the compiled state and return its result + compiler.compile do |_catalog| + Puppet::Pops::Parser::EvaluatingParser.singleton.evaluate_string(compiler.topscope, source, source_location) + end + end +end diff --git a/spec/lib/puppet_spec/files.rb b/spec/lib/puppet_spec/files.rb new file mode 100644 index 00000000..616f32ba --- /dev/null +++ b/spec/lib/puppet_spec/files.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +# file from https://github.com/puppetlabs/puppet/blob/6.x/spec/lib/puppet_spec/files.rb + +require 'fileutils' +require 'tempfile' +require 'tmpdir' +require 'pathname' + +# A support module for testing files. +module PuppetSpec::Files + def self.cleanup + $global_tempfiles ||= [] + while (path = $global_tempfiles.pop) + begin + FileUtils.rm_rf path, secure: true + rescue Errno::ENOENT + # nothing to do + end + end + end + + module_function + + def make_absolute(path) + path = File.expand_path(path) + path[0] = 'c' if Puppet::Util::Platform.windows? + path + end + + def tmpfile(name, dir = nil) + dir ||= Dir.tmpdir + path = Puppet::FileSystem.expand_path(make_tmpname(name, nil).encode(Encoding::UTF_8), dir) + record_tmp(File.expand_path(path)) + + path + end + + def file_containing(name, contents) + file = tmpfile(name) + File.binwrite(file, contents) + file + end + + def script_containing(name, contents) + file = tmpfile(name) + if Puppet::Util::Platform.windows? + file += '.bat' + text = contents[:windows] + else + text = contents[:posix] + end + File.binwrite(file, text) + Puppet::FileSystem.chmod(0o755, file) + file + end + + def tmpdir(name) + dir = Puppet::FileSystem.expand_path(Dir.mktmpdir(name).encode!(Encoding::UTF_8)) + + record_tmp(dir) + + dir + end + + # Copied from ruby 2.4 source + def make_tmpname((prefix, suffix), n) + prefix = (String.try_convert(prefix) or + raise ArgumentError, "unexpected prefix: #{prefix.inspect}") + suffix &&= (String.try_convert(suffix) or + raise ArgumentError, "unexpected suffix: #{suffix.inspect}") + t = Time.now.strftime('%Y%m%d') + path = "#{prefix}#{t}-#{$PROCESS_ID}-#{rand(0x100000000).to_s(36)}".dup + path << "-#{n}" if n + path << suffix if suffix + path + end + + def dir_containing(name, contents_hash) + dir_contained_in(tmpdir(name), contents_hash) + end + + def dir_contained_in(dir, contents_hash) + contents_hash.each do |k, v| + if v.is_a?(Hash) + Dir.mkdir(tmp = File.join(dir, k)) + dir_contained_in(tmp, v) + else + file = File.join(dir, k) + File.binwrite(file, v) + end + end + dir + end + + def record_tmp(tmp) + # ...record it for cleanup, + $global_tempfiles ||= [] + $global_tempfiles << tmp + end + + def expect_file_mode(file, mode) + actual_mode = format('%o', Puppet::FileSystem.stat(file).mode) + target_mode = if Puppet::Util::Platform.windows? + mode + else + "10#{format('%04i', mode.to_i)}" + end + expect(actual_mode).to eq(target_mode) + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ff92c7a2..5f3b6743 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -7,8 +7,16 @@ # We want to do this if lib exists and it hasn't been explicitly set. ENV['COVERAGE'] ||= 'yes' if Dir.exist?(File.expand_path('../lib', __dir__)) +dir = __dir__ +$LOAD_PATH.unshift File.join(dir, 'lib') + require 'voxpupuli/test/spec_helper' +# So everyone else doesn't have to include this base constant. +module PuppetSpec + FIXTURE_DIR = File.join(__dir__, 'fixtures') unless defined?(FIXTURE_DIR) +end + RSpec.configure do |c| c.facterdb_string_keys = true end diff --git a/spec/unit/puppet/provider/archive/ruby_spec.rb b/spec/unit/puppet/provider/archive/ruby_spec.rb index 532f4b7a..58ae3da4 100644 --- a/spec/unit/puppet/provider/archive/ruby_spec.rb +++ b/spec/unit/puppet/provider/archive/ruby_spec.rb @@ -2,10 +2,13 @@ # rubocop:disable RSpec/MultipleMemoizedHelpers require 'spec_helper' +require 'puppet_spec/compiler' +require 'puppet_spec/files' ruby_provider = Puppet::Type.type(:archive).provider(:ruby) - RSpec.describe ruby_provider do + include PuppetSpec::Compiler + include PuppetSpec::Files it_behaves_like 'an archive provider', ruby_provider describe 'ruby provider' do @@ -13,7 +16,7 @@ let(:resource_properties) do { name: name, - source: 's3://home.lan/example.zip' + source: 's3://home.lan/example.zip', } end let(:resource) { Puppet::Type::Archive.new(resource_properties) } @@ -129,6 +132,96 @@ expect { provider.transfer_download(name) }.to raise_error(Puppet::Error, %r{Download file checksum mismatch}) end end + + context 'when handling checks', unless: Puppet::Util::Platform.jruby? do + before do + Puppet[:log_level] = 'debug' + end + + let(:onlyifsecret) { 'onlyifsecret' } + let(:unlesssecret) { 'unlesssecret' } + let(:supersecret) { 'supersecret' } + let(:path) do + if Puppet::Util::Platform.windows? + # The `apply_compiled_manifest` helper doesn't add the `path` fact, so + # we can't reference that in our manifest. Windows PATHs can contain + # double quotes and trailing backslashes, which confuse HEREDOC + # interpolation below. So sanitize it: + ENV['PATH'].split(File::PATH_SEPARATOR). + map { |dir| dir.gsub(%r{"}, '\"').gsub(%r{\\$}, '') }. + map { |dir| Pathname.new(dir).cleanpath.to_s }. + join(File::PATH_SEPARATOR) + else + ENV.fetch('PATH', nil) + end + end + + def echo_from_ruby_exit0(message) + # Escape double quotes due to HEREDOC interpolation below + "ruby -e 'puts \"#{message}\"; exit 0'".gsub(%r{"}, '\"') + end + + def echo_from_ruby_exit1(message) + # Escape double quotes due to HEREDOC interpolation below + "ruby -e 'puts \"#{message}\"; exit 1'".gsub(%r{"}, '\"') + end + + it 'redacts command and onlyif outputs' do + onlyif = echo_from_ruby_exit0(onlyifsecret) + + apply_compiled_manifest(<<-MANIFEST) + archive { '/tmp/favicon.ico': + ensure => present, + source => 'https://www.google.com/favicon.ico', + onlyif => "#{onlyif}", + env_path => "#{path}", + } + MANIFEST + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "Executing check ruby -e 'puts \"onlyifsecret\"; exit 0'", source: %r{Archive\[/tmp/favicon.ico\]})) + end + + it "redacts the command that would have been executed but didn't due to onlyif" do + onlyif = echo_from_ruby_exit1(onlyifsecret) + + apply_compiled_manifest(<<-MANIFEST) + archive { '/tmp/favicon.ico': + ensure => present, + source => 'https://www.google.com/favicon.ico', + onlyif => "#{onlyif}", + env_path => "#{path}", + } + MANIFEST + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "'https://www.google.com/favicon.ico' won't be executed because of failed check 'onlyif'")) + end + + it 'redacts command and unless outputs' do + unlesscmd = echo_from_ruby_exit1(unlesssecret) + + apply_compiled_manifest(<<-MANIFEST) + archive { '/tmp/favicon.ico': + ensure => present, + source => 'https://www.google.com/favicon.ico', + unless => "#{unlesscmd}", + env_path => "#{path}", + } + MANIFEST + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "Executing check ruby -e 'puts \"unlesssecret\"; exit 1'", source: %r{Archive\[/tmp/favicon.ico\]})) + end + + it "redacts the command that would have been executed but didn't due to unless" do + unlesscmd = echo_from_ruby_exit0(unlesssecret) + + apply_compiled_manifest(<<-MANIFEST) + archive { '/tmp/favicon.ico': + ensure => present, + source => 'https://www.google.com/favicon.ico', + unless => "#{unlesscmd}", + env_path => "#{path}", + } + MANIFEST + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "'https://www.google.com/favicon.ico' won't be executed because of failed check 'unless'")) + end + end end end # rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/unit/puppet/type/archive_spec.rb b/spec/unit/puppet/type/archive_spec.rb index e249be08..fc4d8a2d 100644 --- a/spec/unit/puppet/type/archive_spec.rb +++ b/spec/unit/puppet/type/archive_spec.rb @@ -2,8 +2,10 @@ require 'spec_helper' require 'puppet' +require 'puppet_spec/files' describe Puppet::Type.type(:archive) do + include PuppetSpec::Files let(:resource) do Puppet::Type.type(:archive).new( path: '/tmp/example.zip', @@ -134,6 +136,202 @@ end.not_to raise_error end + describe 'when setting environment' do + { 'single values' => 'foo=bar', + 'multiple values' => ['foo=bar', 'baz=quux'], }.each do |name, data| + it "accepts #{name}" do + resource[:environment] = data + expect(resource[:environment]).to eq(data) + end + end + + { 'single values' => 'foo', + 'only values' => %w[foo bar], + 'any values' => ['foo=bar', 'baz'] }.each do |name, data| + it "rejects #{name} without assignment" do + expect { resource[:environment] = data }. + to raise_error Puppet::Error, %r{Invalid environment setting} + end + end + end + + describe '#check' do + describe ':creates' do + let(:exist_file) do + tmpfile('exist') + end + let(:unexist_file) do + tmpfile('unexist') + end + + before do + FileUtils.touch(exist_file) + end + + context 'with a single item' do + it 'runs when the item does not exist' do + resource[:creates] = unexist_file + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run when the item exists' do + resource[:creates] = exist_file + expect(resource.check_all_attributes).to be(true) + end + end + + context 'with an array with one item' do + it 'runs when the item does not exist' do + resource[:creates] = [unexist_file] + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run when the item exists' do + resource[:creates] = [exist_file] + expect(resource.check_all_attributes).to be(true) + end + + it 'does not run when all items exist' do + resource[:creates] = [exist_file] * 3 + end + + context 'when creates is being checked' do + it 'is logged to debug when the path does exist' do + Puppet::Util::Log.level = :debug + resource[:creates] = exist_file + expect(resource.check_all_attributes).to be(true) + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "Checking that 'creates' path '#{exist_file}' exists")) + end + + it 'is logged to debug when the path does not exist' do + Puppet::Util::Log.level = :debug + resource[:creates] = unexist_file + expect(resource.check_all_attributes).to be(false) + expect(@logs).to include(an_object_having_attributes(level: :debug, message: "Checking that 'creates' path '#{unexist_file}' exists")) + end + end + end + end + + { onlyif: { pass: false, fail: true }, + unless: { pass: true, fail: false }, }.each do |param, sense| + describe ":#{param}" do + let(:cmd_pass) do + make_absolute('/magic/pass') + end + let(:cmd_fail) do + make_absolute('/magic/fail') + end + let(:pass_status) do + double('status', exitstatus: sense[:pass] ? 0 : 1) + end + let(:fail_status) do + double('status', exitstatus: sense[:fail] ? 0 : 1) + end + + before do + pass_status = double('status', exitstatus: sense[:pass] ? 0 : 1) + fail_status = double('status', exitstatus: sense[:fail] ? 0 : 1) + + allow(resource.provider).to receive(:checkexe).and_return(true) + [true, false].each do |check| + allow(resource.provider).to receive(:run).with(cmd_pass, check). + and_return(['test output', pass_status]) + allow(resource.provider).to receive(:run).with(cmd_fail, check). + and_return(['test output', fail_status]) + end + end + + context 'with a single item' do + it 'runs if the command exits non-zero' do + resource[param] = cmd_fail + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run if the command exits zero' do + resource[param] = cmd_pass + expect(resource.check_all_attributes).to be(true) + end + end + + context 'with an array with a single item' do + it 'runs if the command exits non-zero' do + resource[param] = [cmd_fail] + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run if the command exits zero' do + resource[param] = [cmd_pass] + expect(resource.check_all_attributes).to be(true) + end + end + + context 'with an array with multiple items' do + it 'runs if all the commands exits non-zero' do + resource[param] = [cmd_fail] * 3 + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run if one command exits zero' do + resource[param] = [cmd_pass, cmd_fail, cmd_pass] + expect(resource.check_all_attributes).to be(true) + end + + it 'does not run if all command exits zero' do + resource[param] = [cmd_pass] * 3 + expect(resource.check_all_attributes).to be(true) + end + end + + context 'with an array of arrays with multiple items' do + before do + [true, false].each do |check| + allow(resource.provider).to receive(:run).with([cmd_pass, '--flag'], check). + and_return(['test output', pass_status]) + allow(resource.provider).to receive(:run).with([cmd_fail, '--flag'], check). + and_return(['test output', fail_status]) + allow(resource.provider).to receive(:run).with([cmd_pass], check). + and_return(['test output', pass_status]) + allow(resource.provider).to receive(:run).with([cmd_fail], check). + and_return(['test output', fail_status]) + end + end + + it 'runs if all the commands exits non-zero' do + resource[param] = [[cmd_fail, '--flag'], [cmd_fail], [cmd_fail, '--flag']] + expect(resource.check_all_attributes).to be(false) + end + + it 'does not run if one command exits zero' do + resource[param] = [[cmd_pass, '--flag'], [cmd_pass], [cmd_fail, '--flag']] + expect(resource.check_all_attributes).to be(true) + end + + it 'does not run if all command exits zero' do + resource[param] = [[cmd_pass, '--flag'], [cmd_pass], [cmd_pass, '--flag']] + expect(resource.check_all_attributes).to be(true) + end + end + + it 'emits output to debug' do + Puppet::Util::Log.level = :debug + resource[param] = cmd_fail + expect(resource.check_all_attributes).to be(false) + expect(@logs.shift.message).to eq('test output') + end + + it 'does not emit output to debug if sensitive is true' do + Puppet::Util::Log.level = :debug + resource[param] = cmd_fail + allow(resource.parameters[param]).to receive(:sensitive).and_return(true) + expect(resource.check_all_attributes).to be(false) + expect(@logs).not_to include(an_object_having_attributes(level: :debug, message: 'test output')) + expect(@logs).to include(an_object_having_attributes(level: :debug, message: '[output redacted]')) + end + end + end + end + describe 'archive autorequire' do let(:file_resource) { Puppet::Type.type(:file).new(name: '/tmp') } let(:archive_resource) do