Bug#926239: Acknowledgement (unblock: facter/3.11.0-2)
Attaching the source debdiff I forgot :)
diff -Nru facter-3.11.0/debian/changelog facter-3.11.0/debian/changelog
--- facter-3.11.0/debian/changelog 2018-12-01 01:19:32.000000000 +0200
+++ facter-3.11.0/debian/changelog 2019-04-02 14:24:17.000000000 +0300
@@ -1,3 +1,13 @@
+facter (3.11.0-2) unstable; urgency=medium
+
+ * Acknowledge 3.11.0-1.1 NMU. Thanks to gregor herrmann!
+ * B-D on ruby (Closes: #897556)
+ * Do not allow unsuitable custom facts to override built-in facts
+ (Closes: #926197)
+ * Bump Standards-Version to 4.3.0; no changes needed
+
+ -- Apollon Oikonomopoulos <apoikos@debian.org> Tue, 02 Apr 2019 14:24:17 +0300
+
facter (3.11.0-1.1) unstable; urgency=medium
* Non-maintainer upload.
diff -Nru facter-3.11.0/debian/control facter-3.11.0/debian/control
--- facter-3.11.0/debian/control 2018-12-01 01:16:40.000000000 +0200
+++ facter-3.11.0/debian/control 2019-04-02 14:24:17.000000000 +0300
@@ -28,7 +28,8 @@
libssl-dev,
rapidjson-dev,
ruby-all-dev,
-Standards-Version: 4.1.3
+ ruby,
+Standards-Version: 4.3.0
Vcs-Git: https://salsa.debian.org/puppet-team/facter.git
Vcs-Browser: https://salsa.debian.org/puppet-team/facter
Homepage: https://github.com/puppetlabs/facter
diff -Nru facter-3.11.0/debian/patches/fix-custom-facts-overriding-core.patch facter-3.11.0/debian/patches/fix-custom-facts-overriding-core.patch
--- facter-3.11.0/debian/patches/fix-custom-facts-overriding-core.patch 1970-01-01 02:00:00.000000000 +0200
+++ facter-3.11.0/debian/patches/fix-custom-facts-overriding-core.patch 2019-04-02 14:21:58.000000000 +0300
@@ -0,0 +1,259 @@
+From 4b7d4a7bebe20433c170b57771645cf774f1f281 Mon Sep 17 00:00:00 2001
+From: Enis Inan <enis.inan@puppet.com>
+Date: Sat, 6 Oct 2018 18:44:17 -0700
+Subject: [PATCH] (FACT-1873) Use builtin fact if custom facts don't resolve or
+ have 0 wt.
+
+Previously, if any builtin facts have conflicting custom facts that all
+fail to resolve, and at least one of the custom facts has a weight > 0,
+Facter will not report a value for that fact. This is undesirable
+behavior because we always want to fall back to the builtin value if
+all custom facts fail to resolve. Note that the "weight > 0" part is
+important because Facter _will_ use the builtin value if all custom
+facts have 0 weight.
+
+Furthermore, the builtin fact should have precedence over 0 weight
+custom facts. The existing code enforces this condition when _all_
+custom facts have 0 weight, but it does _not_ enforce the condition
+when only _some_ custom facts have 0 weight. In the latter case, the 0
+weight custom facts will take precedence over the builtin fact value.
+
+This commit fixes the fact resolution logic so that it falls back to the
+built-in fact if all custom facts fail to resolve, while also ensuring
+that it has precedence over 0 weight custom facts.
+---
+ .../conflicts_with_builtin_fact.rb | 106 ++++++++++++++++++
+ lib/src/ruby/fact.cc | 98 +++++++++-------
+ 2 files changed, 163 insertions(+), 41 deletions(-)
+ create mode 100644 acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb
+
+diff --git a/acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb b/acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb
+new file mode 100644
+index 00000000..34c5e615
+--- /dev/null
++++ b/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
+diff --git a/lib/src/ruby/fact.cc b/lib/src/ruby/fact.cc
+index 9cdc3633..97ce14bc 100644
+--- a/lib/src/ruby/fact.cc
++++ b/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);
+--
+2.20.1
+
diff -Nru facter-3.11.0/debian/patches/series facter-3.11.0/debian/patches/series
--- facter-3.11.0/debian/patches/series 2018-03-19 14:25:26.000000000 +0200
+++ facter-3.11.0/debian/patches/series 2019-04-02 14:21:58.000000000 +0300
@@ -2,3 +2,4 @@
ruby-fix-library-name.patch
disable-facter-smoke.patch
rapidjson-1.1-compat.patch
+fix-custom-facts-overriding-core.patch
Reply to: