Merge be19b32 into daa0cb1

acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb

@@ -0,0 +1,106 @@
+test_name 'Facter should appropriately resolve a custom fact when it conflicts with a builtin fact' do
+  tag 'risk:medium'
+
+  def create_custom_fact_on(host, custom_fact_dir, fact_file_name, fact)
+    fact_file_contents = <<-CUSTOM_FACT
+Facter.add(:#{fact[:name]}) do
+  has_weight #{fact[:weight]}
+  setcode do
+    #{fact[:value]}
+  end
+end
+CUSTOM_FACT
+
+    fact_file_path = File.join(custom_fact_dir, fact_file_name) 
+    create_remote_file(host, fact_file_path, fact_file_contents)
+  end
+
+  def clear_custom_facts_on(host, custom_fact_dir)
+    step "Clean-up the previous test's custom facts" do
+      on(agent, "rm -f #{custom_fact_dir}/*")
+    end
+  end
+
+  agents.each do |agent|
+    custom_fact_dir = agent.tmpdir('facter')
+    teardown do
+      on(agent, "rm -rf '#{custom_fact_dir}'")
+    end
+
+    fact_name = 'timezone'
+    builtin_value = on(agent, facter('timezone')).stdout.chomp
+
+    step "Verify that Facter uses the custom fact's value when its weight is > 0" do
+      custom_fact_value = "custom_timezone"
+      create_custom_fact_on(
+        agent,
+        custom_fact_dir,
+        'custom_timezone.rb',
+        name: fact_name,
+        weight: 10,
+        value: "'#{custom_fact_value}'"
+      )
+
+      on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result|
+        assert_match(/#{custom_fact_value}/, result.stdout.chomp, "Facter does not use the custom fact's value when its weight is > 0")
+      end
+    end
+
+    clear_custom_facts_on(agent, custom_fact_dir)
+
+    step "Verify that Facter uses the builtin fact's value when all conflicting custom facts fail to resolve" do
+      [ 'timezone_one.rb', 'timezone_two.rb'].each do |fact_file|
+        create_custom_fact_on(
+          agent,
+          custom_fact_dir,
+          fact_file,
+          { name: fact_name, weight: 10, value: nil }
+        )
+      end
+
+      on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result|
+        assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not use the builtin fact's value when all conflicting custom facts fail to resolve")
+      end
+    end
+
+    step "Verify that Facter gives precedence to the builtin fact over zero weight custom facts" do
+      step "when all custom facts have zero weight" do
+        {
+          'timezone_one.rb' => "'timezone_one'",
+          'timezone_two.rb' => "'timezone_two'"
+        }.each do |fact_file, fact_value|
+          create_custom_fact_on(
+            agent,
+            custom_fact_dir,
+            fact_file,
+            { name: fact_name, weight: 0, value: fact_value }
+          )
+        end
+
+        on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result|
+          assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not give precedence to the builtin fact when all custom facts have zero weight")
+        end
+      end
+
+      clear_custom_facts_on(agent, custom_fact_dir)
+
+      step "when some custom facts have zero weight" do
+        {
+          'timezone_one.rb' => { weight: 10, value: nil },
+          'timezone_two.rb' => { weight: 0, value: "'timezone_two'" }
+        }.each do |fact_file, fact|
+          create_custom_fact_on(
+            agent,
+            custom_fact_dir,
+            fact_file,
+            fact.merge(name: fact_name)
+          )
+        end
+
+        on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result|
+          assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not give precedence to the builtin fact when only some custom facts have zero weight")
+        end
+      end
+    end
+  end
+end

lib/src/ruby/fact.cc

@@ -83,53 +83,69 @@ namespace facter { namespace ruby {
         });
 
         _resolving = true;
-
-        // If no resolutions or the top resolution has a weight of 0, first check the native collection for the fact
-        // This way we treat the "built-in" as implicitly having a resolution with weight 0
         bool add = true;
-        if (_resolutions.empty() || ruby.to_native<resolution>(_resolutions.front())->weight() == 0) {
-            // Check to see the value is in the collection
-            auto value = facts[ruby.to_string(_name)];
-            if (value) {
-                // Already in collection, do not add
-                add = false;
-                _value = facter->to_ruby(value);
-                _weight = value->weight();
-            }
-        }
 
-        if (ruby.is_nil(_value)) {
-            vector<VALUE>::iterator it;
-            ruby.rescue([&]() {
-                volatile VALUE value = ruby.nil_value();
-                size_t weight = 0;
-
-                // Look through the resolutions and find the first allowed resolution that resolves
-                for (it = _resolutions.begin(); it != _resolutions.end(); ++it) {
-                    auto res = ruby.to_native<resolution>(*it);
-                    if (!res->suitable(*facter)) {
-                        continue;
-                    }
-                    value = res->value();
-                    if (!ruby.is_nil(value)) {
-                        weight = res->weight();
-                        break;
-                    }
+        vector<VALUE>::iterator it;
+        ruby.rescue([&]() {
+            volatile VALUE value = ruby.nil_value();
+            size_t weight = 0;
+
+            // Look through the resolutions and find the first allowed resolution that resolves
+            for (it = _resolutions.begin(); it != _resolutions.end(); ++it) {
+                auto res = ruby.to_native<resolution>(*it);
+                if (!res->suitable(*facter)) {
+                    continue;
+                }
+                value = res->value();
+                if (!ruby.is_nil(value)) {
+                    weight = res->weight();
+                    break;
                 }
+            }
 
-                // Set the value to what was resolved
-                _value = value;
-                _weight = weight;
-                return 0;
-            }, [&](VALUE ex) {
-                LOG_ERROR("error while resolving custom fact \"{1}\": {2}", ruby.rb_string_value_ptr(&_name), ruby.exception_to_string(ex));
+            // Set the value to what was resolved
+            _value = value;
+            _weight = weight;
 
-                // Failed, so set to nil
-                _value = ruby.nil_value();
-                _weight = 0;
+            if (! ruby.is_nil(_value) && _weight != 0) {
                 return 0;
-            });
-        }
+            }
+
+            // There's two possibilities here:
+            //   1. None of our resolvers could resolve the value
+            //   2. A resolver of weight 0 resolved the value
+            //
+            // In both cases, we attempt to use the "built-in" fact's
+            // value. This serves as a fallback resolver for Case (1)
+            // while for Case (2), we want built-in values to take
+            // precedence over 0-weight custom facts.
+
+            auto builtin_value = facts[ruby.to_string(_name)];
+            if (! builtin_value) {
+              return 0;
+            }
+            auto builtin_ruby_value = facter->to_ruby(builtin_value);
+
+            // We need this check for Case (2). Otherwise, we risk
+            // overwriting our resolved value in the small chance
+            // that builtin_value exists, but its ruby value is
+            // nil.
+            if (! ruby.is_nil(builtin_ruby_value)) {
+              // Already in collection, do not add
+              add = false;
+              _value = builtin_ruby_value;
+              _weight = builtin_value->weight();
+            }
+
+            return 0;
+        }, [&](VALUE ex) {
+            LOG_ERROR("error while resolving custom fact \"{1}\": {2}", ruby.rb_string_value_ptr(&_name), ruby.exception_to_string(ex));
+
+            // Failed, so set to nil
+            _value = ruby.nil_value();
+            _weight = 0;
+            return 0;
+        });
 
         if (add) {
             facts.add_custom(ruby.to_string(_name), ruby.is_nil(_value) ? nullptr : make_value<ruby::ruby_value>(_value), _weight);