[Pkg-puppet-devel] [facter] 68/352: (#22944) Don't execute external facts 19 times

[Stig Sandbeck Mathisen]

This is an automated email from the git hooks/post-receive script.

ssm pushed a commit to branch master
in repository facter.

commit ab999818f52519c579843c596db62af017b16b07
Author: Josh Cooper <josh at puppetlabs.com>
Date:   Thu Dec 5 22:10:11 2013 -0800

    (#22944) Don't execute external facts 19 times
    
    Previously, facter would load every external fact many, many times. For
    executable facts, e.g. powershell, this means executing the fact. For
    text based facts, it means reading the file, and parsing its output. On
    Darwin, it was common for facts to be executed 6 times; on Windows,
    19 times.
    
    The problem occurs whenever facter requires a file, e.g. `ec2.rb`, which
    references another fact, e.g. `:macaddress` outside the body of any
    resolution, and facter has not yet required `macaddress.rb`.
    
    For example, the following will cause all external facts to be loaded:
    
        if Facter.value(:macaddress)
          Facter.add(:ec2) do
            ...
          end
        end
    
    Note that this is triggered because macaddress comes after ec2.
    Similarly, `blockdevices.rb` references the `:kernel` fact outside of
    any resolution, so that's another round of external fact evaluation.
    
    This commit keeps adds a boolean flag to ensure we only execute external
    facts once.
---
 acceptance/tests/runs_external_facts_once.rb | 59 ++++++++++++++++++++++++++++
 lib/facter/util/collection.rb                | 11 +++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/acceptance/tests/runs_external_facts_once.rb b/acceptance/tests/runs_external_facts_once.rb
new file mode 100644
index 0000000..afd6907
--- /dev/null
+++ b/acceptance/tests/runs_external_facts_once.rb
@@ -0,0 +1,59 @@
+test_name "#22944: Facter executes external executable facts many times"
+
+unix_content = <<EOM
+#!/bin/sh
+echo "SCRIPT CALLED" >&2
+echo "test=value"
+EOM
+
+win_content = <<EOM
+echo "SCRIPT CALLED" >&2
+echo "test=value"
+EOM
+
+agents.each do |agent|
+  step "Agent #{agent}: create external executable fact"
+
+  # assume we're running as root
+  if agent['platform'] =~ /windows/
+    if on(agent, facter('kernelmajversion')).stdout.chomp.to_f < 6.0
+      factsd = 'C:/Documents and Settings/All Users/Application Data/PuppetLabs/facter/facts.d'
+    else
+      factsd = 'C:/ProgramData/PuppetLabs/facter/facts.d'
+    end
+    ext = '.bat'
+    content = win_content
+  else
+    factsd = '/etc/facter/facts.d'
+    ext = '.sh'
+    content = unix_content
+  end
+
+
+  step "Agent #{agent}: create facts.d directory"
+  on(agent, "mkdir -p '#{factsd}'")
+
+
+  step "Agent #{agent}: create external fact"
+  ext_fact = "#{factsd}/external_fact#{ext}"
+
+  teardown do
+    on(agent, "rm -f '#{ext_fact}'")
+  end
+
+  create_remote_file(agent, ext_fact, content)
+
+  step "Agent #{agent}: make it executable"
+  on(agent, "chmod +x '#{ext_fact}'")
+
+  step "Agent #{agent}: ensure it only executes once"
+  on(agent, facter) do
+    lines = stderr.split('\n')
+    times = lines.count { |line| line =~ /SCRIPT CALLED/ }
+    if times == 1
+      step "External executable fact executed once"
+    else
+      fail_test "External fact executed #{times} times, expected once: #{stderr}"
+    end
+  end
+end
diff --git a/lib/facter/util/collection.rb b/lib/facter/util/collection.rb
index 7c02ce0..4b91e10 100644
--- a/lib/facter/util/collection.rb
+++ b/lib/facter/util/collection.rb
@@ -106,13 +106,13 @@ class Facter::Util::Collection
 
   def load(name)
     internal_loader.load(name)
-    external_loader.load(self)
+    load_external_facts
   end
 
   # Load all known facts.
   def load_all
     internal_loader.load_all
-    external_loader.load(self)
+    load_external_facts
   end
 
   def internal_loader
@@ -146,4 +146,11 @@ class Facter::Util::Collection
   def canonicalize(name)
     name.to_s.downcase.to_sym
   end
+
+  def load_external_facts
+    if ! @external_facts_loaded
+      @external_facts_loaded = true
+      external_loader.load(self)
+    end
+  end
 end

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git