From fa3f0f404c4c373368151163f6b3412a6f4b411f Mon Sep 17 00:00:00 2001
From: Florin Dragos <florin@puppet.com>
Date: Tue, 15 Sep 2020 12:17:25 +0300
Subject: [PATCH] (FACT-2786) Fix fact caching if fact is defined in multiple
 groups

---
 .../tests/custom_facts/cached_custom_fact.rb  |  2 +-
 .../weighted_cached_custom_facts.rb           |  2 +-
 .../ttls_cached_fact_in_multiple_groups.rb    | 57 +++++++++++++++++
 .../ttls_cached_individual_fact_name.rb       | 63 +++++++++++++++++++
 .../custom_facts/util/directory_loader.rb     |  2 +-
 lib/facter/framework/config/fact_groups.rb    | 38 ++++++++++-
 lib/facter/framework/core/cache_manager.rb    | 48 ++++++++------
 spec/facter/cache_manager_spec.rb             | 34 ++++++----
 8 files changed, 210 insertions(+), 36 deletions(-)
 create mode 100644 acceptance/tests/options/config_file/ttls_cached_fact_in_multiple_groups.rb
 create mode 100644 acceptance/tests/options/config_file/ttls_cached_individual_fact_name.rb

diff --git a/acceptance/tests/custom_facts/cached_custom_fact.rb b/acceptance/tests/custom_facts/cached_custom_fact.rb
index 0f7a452548..bf7a083859 100644
--- a/acceptance/tests/custom_facts/cached_custom_fact.rb
+++ b/acceptance/tests/custom_facts/cached_custom_fact.rb
@@ -73,7 +73,7 @@
 
     step 'should read from the cached file for a custom fact that has been cached' do
       on(agent, facter("#{custom_fact_name} --debug", environment: env)) do |facter_result|
-        assert_match(/Loading cached custom facts from file ".+"|loading cached values for cached-custom-facts facts/, facter_result.stderr,
+        assert_match(/Loading cached custom facts from file ".+"|loading cached values for random_custom_fact facts/, facter_result.stderr,
                      'Expected debug message to state that cached custom facts are read from file')
       end
     end
diff --git a/acceptance/tests/custom_facts/weighted_cached_custom_facts.rb b/acceptance/tests/custom_facts/weighted_cached_custom_facts.rb
index dcd9a363ea..3cc04ee30d 100644
--- a/acceptance/tests/custom_facts/weighted_cached_custom_facts.rb
+++ b/acceptance/tests/custom_facts/weighted_cached_custom_facts.rb
@@ -85,7 +85,7 @@
 
     step 'should read from the cached file for a custom fact that has been cached' do
       on(agent, facter("#{duplicate_custom_fact_name} --debug", environment: env)) do |facter_result|
-        assert_match(/Loading cached custom facts from file ".+"|loading cached values for cached-custom-facts facts/, facter_result.stderr,
+        assert_match(/Loading cached custom facts from file ".+"|loading cached values for random_custom_fact facts/, facter_result.stderr,
                      'Expected debug message to state that cached custom facts are read from file')
       end
     end
diff --git a/acceptance/tests/options/config_file/ttls_cached_fact_in_multiple_groups.rb b/acceptance/tests/options/config_file/ttls_cached_fact_in_multiple_groups.rb
new file mode 100644
index 0000000000..aeca1824f7
--- /dev/null
+++ b/acceptance/tests/options/config_file/ttls_cached_fact_in_multiple_groups.rb
@@ -0,0 +1,57 @@
+# This test verifies that individual facts can be cached
+test_name "ttls config with fact in multiple groups should not cache fact twice" do
+  tag 'risk:high'
+
+  require 'facter/acceptance/user_fact_utils'
+  extend Facter::Acceptance::UserFactUtils
+
+  # This fact must be resolvable on ALL platforms
+  # Do NOT use the 'kernel' fact as it is used to configure the tests
+  cached_fact_name = 'os.name'
+  first_fact_group = 'first'
+  second_fact_group = 'second'
+
+  config = <<EOM
+facts : {
+  ttls : [
+      { "#{first_fact_group}" : 30 days },
+      { "#{second_fact_group}" : 1 days },
+  ]
+}
+fact-groups : {
+  #{first_fact_group}: [#{cached_fact_name}],
+  #{second_fact_group}: ["os"],
+}
+EOM
+
+  agents.each do |agent|
+    step "Agent #{agent}: create cache file with individual fact" do
+      config_dir = get_default_fact_dir(agent['platform'], on(agent, facter('kernelmajversion')).stdout.chomp.to_f)
+      config_file = File.join(config_dir, "facter.conf")
+      cached_facts_dir = get_cached_facts_dir(agent['platform'], on(agent, facter('kernelmajversion')).stdout.chomp.to_f)
+
+      first_cached_fact_file = File.join(cached_facts_dir, first_fact_group)
+      second_cached_fact_file = File.join(cached_facts_dir, second_fact_group)
+
+      # Setup facter conf
+      agent.mkdir_p(config_dir)
+      create_remote_file(agent, config_file, config)
+
+      teardown do
+        agent.rm_rf(config_dir)
+        agent.rm_rf(cached_facts_dir)
+      end
+
+      step "should create a JSON file for a fact that is to be cached" do
+        agent.rm_rf(cached_facts_dir)
+        on(agent, facter("--debug")) do |facter_output|
+          assert_match(/caching values for .+ facts/, facter_output.stderr, "Expected debug message to state that values will be cached")
+        end
+        first_cat_output = agent.cat(first_cached_fact_file)
+        assert_match(/#{cached_fact_name}/, first_cat_output.strip, "Expected cached fact file to contain fact information")
+        second_cat_output = agent.cat(second_cached_fact_file)
+        assert_not_match(/#{cached_fact_name}/, second_cat_output.strip, "Expected cached fact file to not contain fact information")
+      end
+    end
+  end
+end
diff --git a/acceptance/tests/options/config_file/ttls_cached_individual_fact_name.rb b/acceptance/tests/options/config_file/ttls_cached_individual_fact_name.rb
new file mode 100644
index 0000000000..30fe568f00
--- /dev/null
+++ b/acceptance/tests/options/config_file/ttls_cached_individual_fact_name.rb
@@ -0,0 +1,63 @@
+# This test verifies that individual facts can be cached
+test_name "ttls config that contains fact name caches individual facts" do
+  tag 'risk:high'
+
+  require 'facter/acceptance/user_fact_utils'
+  extend Facter::Acceptance::UserFactUtils
+
+  # This fact must be resolvable on ALL platforms
+  # Do NOT use the 'kernel' fact as it is used to configure the tests
+  cached_fact_name = 'system_uptime.days'
+  cached_fact_value = "CACHED_FACT_VALUE"
+  cached_fact_content = <<EOM
+{
+ "#{cached_fact_name}": "#{cached_fact_value}"
+}
+EOM
+
+  config = <<EOM
+facts : {
+  ttls : [
+      { "#{cached_fact_name}" : 30 days }
+  ]
+}
+EOM
+
+  agents.each do |agent|
+    step "Agent #{agent}: create cache file with individual fact" do
+      config_dir = get_default_fact_dir(agent['platform'], on(agent, facter('kernelmajversion')).stdout.chomp.to_f)
+      config_file = File.join(config_dir, "facter.conf")
+      cached_facts_dir = get_cached_facts_dir(agent['platform'], on(agent, facter('kernelmajversion')).stdout.chomp.to_f)
+
+      cached_fact_file = File.join(cached_facts_dir, cached_fact_name)
+
+      # Setup facter conf
+      agent.mkdir_p(config_dir)
+      create_remote_file(agent, config_file, config)
+
+      teardown do
+        agent.rm_rf(config_dir)
+        agent.rm_rf(cached_facts_dir)
+      end
+
+      step "should create a JSON file for a fact that is to be cached" do
+        agent.rm_rf(cached_facts_dir)
+        on(agent, facter("--debug")) do |facter_output|
+          assert_match(/caching values for .+ facts/, facter_output.stderr, "Expected debug message to state that values will be cached")
+        end
+        cat_output = agent.cat(cached_fact_file)
+        assert_match(/#{cached_fact_name}/, cat_output.strip, "Expected cached fact file to contain fact information")
+      end
+
+      step "should read from a cached JSON file for a fact that has been cached" do
+        agent.mkdir_p(cached_facts_dir)
+        create_remote_file(agent, cached_fact_file, cached_fact_content)
+
+        on(agent, facter("#{cached_fact_name} --debug")) do
+          assert_match(/loading cached values for .+ facts/, stderr, "Expected debug message to state that values are read from cache")
+          assert_match(/#{cached_fact_value}/, stdout, "Expected fact to match the cached fact file")
+        end
+      end
+    end
+  end
+end
diff --git a/lib/facter/custom_facts/util/directory_loader.rb b/lib/facter/custom_facts/util/directory_loader.rb
index 22b34047cb..e363208c95 100644
--- a/lib/facter/custom_facts/util/directory_loader.rb
+++ b/lib/facter/custom_facts/util/directory_loader.rb
@@ -59,7 +59,7 @@ def load_directory_entries(_collection)
           basename = File.basename(file)
           next if file_blocked?(basename)
 
-          if facts.find { |f| f.name == basename } && cm.group_cached?(basename)
+          if facts.find { |f| f.name == basename } && cm.fact_cache_enabled?(basename)
             Facter.log_exception(Exception.new("Caching is enabled for group \"#{basename}\" while "\
               'there are at least two external facts files with the same filename'))
           else
diff --git a/lib/facter/framework/config/fact_groups.rb b/lib/facter/framework/config/fact_groups.rb
index efd06870a1..6789d3f2f9 100644
--- a/lib/facter/framework/config/fact_groups.rb
+++ b/lib/facter/framework/config/fact_groups.rb
@@ -2,7 +2,7 @@
 
 module Facter
   class FactGroups
-    attr_reader :groups, :block_list
+    attr_reader :groups, :block_list, :facts_ttls
 
     @groups_ttls = []
 
@@ -14,6 +14,10 @@ def initialize(group_list_path = nil)
       @groups ||= File.readable?(@groups_file_path) ? Hocon.load(@groups_file_path) : {}
       load_groups
       load_groups_from_options
+      load_facts_ttls
+
+      # Reverse sort facts so that children have precedence when caching. eg: os.macosx vs os
+      @facts_ttls = @facts_ttls.sort.reverse.to_h
     end
 
     # Breakes down blocked groups in blocked facts
@@ -31,6 +35,9 @@ def blocked_facts
 
     # Get the group name a fact is part of
     def get_fact_group(fact_name)
+      fact = get_fact(fact_name)
+      return fact[:group] if fact
+
       @groups.detect { |k, v| break k if Array(v).find { |f| fact_name =~ /^#{f}.*/ } }
     end
 
@@ -41,6 +48,15 @@ def get_group_ttls(group_name)
       ttls_to_seconds(ttls[group_name])
     end
 
+    def get_fact(fact_name)
+      return @facts_ttls[fact_name] if @facts_ttls[fact_name]
+
+      result = @facts_ttls.select { |name, fact| break fact if fact_name =~ /^#{name}\..*/ }
+      return nil if result == {}
+
+      result
+    end
+
     private
 
     def load_groups_from_options
@@ -55,10 +71,28 @@ def load_groups_from_options
       end
     end
 
+    def load_facts_ttls
+      @facts_ttls ||= {}
+      return if @groups_ttls == []
+
+      @groups_ttls.reduce(:merge).each do |group, ttls|
+        ttls = ttls_to_seconds(ttls)
+        if @groups[group]
+          @groups[group].each do |fact|
+            if (@facts_ttls[fact] && @facts_ttls[fact][:ttls] < ttls) || @facts_ttls[fact].nil?
+              @facts_ttls[fact] = { ttls: ttls, group: group }
+            end
+          end
+        else
+          @facts_ttls[group] = { ttls: ttls, group: group }
+        end
+      end
+    end
+
     def load_groups
       config = ConfigReader.init(Options[:config])
       @block_list = config.block_list || {}
-      @groups_ttls = config.ttls || {}
+      @groups_ttls = config.ttls || []
       @groups.merge!(config.fact_groups) if config.fact_groups
     end
 
diff --git a/lib/facter/framework/core/cache_manager.rb b/lib/facter/framework/core/cache_manager.rb
index 92170bc7a5..4654aa4a17 100644
--- a/lib/facter/framework/core/cache_manager.rb
+++ b/lib/facter/framework/core/cache_manager.rb
@@ -40,31 +40,42 @@ def cache_facts(resolved_facts)
       end
     end
 
-    def group_cached?(group_name)
-      cached = @fact_groups.get_group_ttls(group_name) ? true : false
-      delete_cache(group_name) unless cached
+    def fact_cache_enabled?(fact_name)
+      fact = @fact_groups.get_fact(fact_name)
+      cached = if fact
+                 !fact[:ttls].nil?
+               else
+                 false
+               end
+
+      fact_group = @fact_groups.get_fact_group(fact_name)
+      delete_cache(fact_group) if fact_group && !cached
       cached
     end
 
     private
 
     def resolve_fact(searched_fact)
-      group_name =  if searched_fact.file
-                      searched_fact.name
-                    else
-                      @fact_groups.get_fact_group(searched_fact.name)
-                    end
+      return unless fact_cache_enabled?(searched_fact.name)
 
-      return unless group_name
+      fact = if searched_fact.file
+               @fact_groups.get_fact(File.basename(searched_fact.file))
+             else
+               @fact_groups.get_fact(searched_fact.name)
+             end
 
-      return unless group_cached?(group_name)
+      return unless fact
 
-      return unless check_ttls?(group_name)
+      return unless check_ttls?(fact[:group], fact[:ttls])
 
-      data = read_group_json(group_name)
+      read_fact(searched_fact, fact[:group])
+    end
+
+    def read_fact(searched_fact, fact_group)
+      data = read_group_json(fact_group)
       return unless data
 
-      @log.debug("loading cached values for #{group_name} facts")
+      @log.debug("loading cached values for #{searched_fact.name} facts")
 
       create_facts(searched_fact, data)
     end
@@ -93,7 +104,7 @@ def cache_fact(fact)
                    end
       return if !group_name || fact.value.nil?
 
-      return unless group_cached?(group_name)
+      return unless fact_cache_enabled?(fact.name)
 
       @groups[group_name] ||= {}
       @groups[group_name][fact.name] = fact.value
@@ -106,10 +117,12 @@ def write_cache
       end
 
       @groups.each do |group_name, data|
-        next unless check_ttls?(group_name)
+        next unless check_ttls?(group_name, @fact_groups.get_group_ttls(group_name))
 
-        @log.debug("caching values for #{group_name} facts")
         cache_file_name = File.join(@cache_dir, group_name)
+        next if File.readable?(cache_file_name)
+
+        @log.debug("caching values for #{group_name} facts")
         File.write(cache_file_name, JSON.pretty_generate(data))
       end
     end
@@ -128,8 +141,7 @@ def read_group_json(group_name)
       @groups[group_name] = data
     end
 
-    def check_ttls?(group_name)
-      ttls = @fact_groups.get_group_ttls(group_name)
+    def check_ttls?(group_name, ttls)
       return false unless ttls
 
       cache_file_name = File.join(@cache_dir, group_name)
diff --git a/spec/facter/cache_manager_spec.rb b/spec/facter/cache_manager_spec.rb
index 2b12c737ee..ee540348cb 100644
--- a/spec/facter/cache_manager_spec.rb
+++ b/spec/facter/cache_manager_spec.rb
@@ -25,6 +25,8 @@
   let(:group_name) { 'operating system' }
   let(:cache_file_name) { File.join(cache_dir, group_name) }
   let(:fact_groups) { instance_spy(Facter::FactGroups) }
+  let(:os_fact) { { ttls: 60, group: 'operating system' } }
+  let(:external_fact) { { ttls: 60, group: 'ext_file.txt' } }
 
   before do
     allow(LegacyFacter::Util::Config).to receive(:facts_cache_dir).and_return(cache_dir)
@@ -72,7 +74,10 @@
         allow(File).to receive(:directory?).with(cache_dir).and_return(true)
         allow(fact_groups).to receive(:get_fact_group).with('os').and_return(group_name)
         allow(fact_groups).to receive(:get_fact_group).with('my_custom_fact').and_return(nil)
+        allow(fact_groups).to receive(:get_fact_group).with('ext_file.txt').and_return(nil)
         allow(fact_groups).to receive(:get_group_ttls).with('ext_file.txt').and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('ext_file.txt').and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('my_custom_fact').and_return(nil)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(true)
         allow(File).to receive(:mtime).with(cache_file_name).and_return(Time.now)
         allow(Facter::Util::FileHelper).to receive(:safe_read).with(cache_file_name).and_return(cached_core_fact)
@@ -80,7 +85,7 @@
       end
 
       it 'returns cached fact' do
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(1000)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(os_fact)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(true)
         allow(File).to receive(:readable?).with(File.join(cache_dir, 'ext_file.txt')).and_return(false)
 
@@ -91,7 +96,7 @@
       end
 
       it 'returns searched fact' do
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(1000)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(os_fact)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(true)
         allow(File).to receive(:readable?).with(File.join(cache_dir, 'ext_file.txt')).and_return(false)
 
@@ -103,9 +108,10 @@
       end
 
       it 'deletes cache file' do
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(nil)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(true)
         allow(File).to receive(:delete).with(cache_file_name)
+        allow(fact_groups).to receive(:get_fact_group).with('os').and_return(group_name)
         allow(File).to receive(:readable?).with(File.join(cache_dir, 'ext_file.txt')).and_return(false)
 
         cache_manager.resolve_facts(searched_facts)
@@ -113,8 +119,9 @@
       end
 
       it 'returns cached external facts' do
-        allow(fact_groups).to receive(:get_group_ttls).with('ext_file.txt').and_return(1000)
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('my_custom_fact').and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('ext_file.txt').and_return(external_fact)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(false)
         allow(Facter::Util::FileHelper).to receive(:safe_read).with(File.join(cache_dir, 'ext_file.txt'))
                                                               .and_return(cached_external_fact)
@@ -135,7 +142,7 @@
       before do
         allow(File).to receive(:directory?).with(cache_dir).and_return(true)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(false)
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(nil)
         allow(fact_groups).to receive(:get_fact_group).with('os').and_return(group_name)
         allow(File).to receive(:write).with(cache_file_name, cached_core_fact)
         allow(Facter::Options).to receive(:[]).with(:cache).and_return(true)
@@ -150,10 +157,10 @@
     context 'with cache group' do
       before do
         allow(File).to receive(:directory?).with(cache_dir).and_return(true)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(os_fact)
         allow(fact_groups).to receive(:get_fact_group).with('os').and_return(group_name)
         allow(fact_groups).to receive(:get_fact_group).with('my_custom_fact').and_return(nil)
         allow(fact_groups).to receive(:get_fact_group).with('my_external_fact').and_return(nil)
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(1000)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(false)
         allow(File).to receive(:write).with(cache_file_name, cached_core_fact)
         allow(Facter::Options).to receive(:[]).with(:cache).and_return(true)
@@ -166,35 +173,36 @@
     end
   end
 
-  describe '#group_cached?' do
+  describe '#fact_cache_enabled?' do
     context 'with ttls' do
       before do
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(1000)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(os_fact)
         allow(File).to receive(:readable?).with(cache_file_name).and_return(false)
       end
 
       it 'returns true' do
-        result = cache_manager.group_cached?(group_name)
+        result = cache_manager.fact_cache_enabled?('os')
         expect(result).to be true
       end
     end
 
     context 'without ttls' do
       before do
-        allow(fact_groups).to receive(:get_group_ttls).with(group_name).and_return(nil)
+        allow(fact_groups).to receive(:get_fact).with('os').and_return(nil)
+        allow(fact_groups).to receive(:get_fact_group).with('os').and_return(group_name)
         allow(Facter::Options).to receive(:[]).with(:cache).and_return(true)
         allow(File).to receive(:delete).with(cache_file_name)
       end
 
       it 'returns false' do
         allow(File).to receive(:readable?).with(cache_file_name).and_return(false)
-        result = cache_manager.group_cached?(group_name)
+        result = cache_manager.fact_cache_enabled?('os')
         expect(result).to be false
       end
 
       it 'deletes invalid cache file' do
         allow(File).to receive(:readable?).with(cache_file_name).and_return(true)
-        cache_manager.group_cached?(group_name)
+        cache_manager.fact_cache_enabled?('os')
         expect(File).to have_received(:delete).with(cache_file_name)
       end
     end