(FACT-2786) Fix fact caching if fact is defined in multiple groups (#2089)

Author: florindragos

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

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

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

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

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

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
 

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)