Do not allow unsuitable custom facts to override built-in facts
A confined custom fact may override a built-in fact despite its
confinement not being valid, thus unsetting the built-in fact's value.
This has been fixed upstream in 3.11.6, so cherry-pick upstream commit
eb33a4d59e9b09d6c95028c215aa7d3081c097d3 to fix it.
Closes: #926197
Apollon Oikonomopoulos
5 years ago
| 0 | From 4b7d4a7bebe20433c170b57771645cf774f1f281 Mon Sep 17 00:00:00 2001 | |
| 1 | From: Enis Inan <enis.inan@puppet.com> | |
| 2 | Date: Sat, 6 Oct 2018 18:44:17 -0700 | |
| 3 | Subject: [PATCH] (FACT-1873) Use builtin fact if custom facts don't resolve or | |
| 4 | have 0 wt. | |
| 5 | ||
| 6 | Previously, if any builtin facts have conflicting custom facts that all | |
| 7 | fail to resolve, and at least one of the custom facts has a weight > 0, | |
| 8 | Facter will not report a value for that fact. This is undesirable | |
| 9 | behavior because we always want to fall back to the builtin value if | |
| 10 | all custom facts fail to resolve. Note that the "weight > 0" part is | |
| 11 | important because Facter _will_ use the builtin value if all custom | |
| 12 | facts have 0 weight. | |
| 13 | ||
| 14 | Furthermore, the builtin fact should have precedence over 0 weight | |
| 15 | custom facts. The existing code enforces this condition when _all_ | |
| 16 | custom facts have 0 weight, but it does _not_ enforce the condition | |
| 17 | when only _some_ custom facts have 0 weight. In the latter case, the 0 | |
| 18 | weight custom facts will take precedence over the builtin fact value. | |
| 19 | ||
| 20 | This commit fixes the fact resolution logic so that it falls back to the | |
| 21 | built-in fact if all custom facts fail to resolve, while also ensuring | |
| 22 | that it has precedence over 0 weight custom facts. | |
| 23 | --- | |
| 24 | .../conflicts_with_builtin_fact.rb | 106 ++++++++++++++++++ | |
| 25 | lib/src/ruby/fact.cc | 98 +++++++++------- | |
| 26 | 2 files changed, 163 insertions(+), 41 deletions(-) | |
| 27 | create mode 100644 acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb | |
| 28 | ||
| 29 | diff --git a/acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb b/acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb | |
| 30 | new file mode 100644 | |
| 31 | index 00000000..34c5e615 | |
| 32 | --- /dev/null | |
| 33 | +++ b/acceptance/tests/custom_facts/conflicts_with_builtin_fact.rb | |
| 34 | @@ -0,0 +1,106 @@ | |
| 35 | +test_name 'Facter should appropriately resolve a custom fact when it conflicts with a builtin fact' do | |
| 36 | + tag 'risk:medium' | |
| 37 | + | |
| 38 | + def create_custom_fact_on(host, custom_fact_dir, fact_file_name, fact) | |
| 39 | + fact_file_contents = <<-CUSTOM_FACT | |
| 40 | +Facter.add(:#{fact[:name]}) do | |
| 41 | + has_weight #{fact[:weight]} | |
| 42 | + setcode do | |
| 43 | + #{fact[:value]} | |
| 44 | + end | |
| 45 | +end | |
| 46 | +CUSTOM_FACT | |
| 47 | + | |
| 48 | + fact_file_path = File.join(custom_fact_dir, fact_file_name) | |
| 49 | + create_remote_file(host, fact_file_path, fact_file_contents) | |
| 50 | + end | |
| 51 | + | |
| 52 | + def clear_custom_facts_on(host, custom_fact_dir) | |
| 53 | + step "Clean-up the previous test's custom facts" do | |
| 54 | + on(agent, "rm -f #{custom_fact_dir}/*") | |
| 55 | + end | |
| 56 | + end | |
| 57 | + | |
| 58 | + agents.each do |agent| | |
| 59 | + custom_fact_dir = agent.tmpdir('facter') | |
| 60 | + teardown do | |
| 61 | + on(agent, "rm -rf '#{custom_fact_dir}'") | |
| 62 | + end | |
| 63 | + | |
| 64 | + fact_name = 'timezone' | |
| 65 | + builtin_value = on(agent, facter('timezone')).stdout.chomp | |
| 66 | + | |
| 67 | + step "Verify that Facter uses the custom fact's value when its weight is > 0" do | |
| 68 | + custom_fact_value = "custom_timezone" | |
| 69 | + create_custom_fact_on( | |
| 70 | + agent, | |
| 71 | + custom_fact_dir, | |
| 72 | + 'custom_timezone.rb', | |
| 73 | + name: fact_name, | |
| 74 | + weight: 10, | |
| 75 | + value: "'#{custom_fact_value}'" | |
| 76 | + ) | |
| 77 | + | |
| 78 | + on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result| | |
| 79 | + assert_match(/#{custom_fact_value}/, result.stdout.chomp, "Facter does not use the custom fact's value when its weight is > 0") | |
| 80 | + end | |
| 81 | + end | |
| 82 | + | |
| 83 | + clear_custom_facts_on(agent, custom_fact_dir) | |
| 84 | + | |
| 85 | + step "Verify that Facter uses the builtin fact's value when all conflicting custom facts fail to resolve" do | |
| 86 | + [ 'timezone_one.rb', 'timezone_two.rb'].each do |fact_file| | |
| 87 | + create_custom_fact_on( | |
| 88 | + agent, | |
| 89 | + custom_fact_dir, | |
| 90 | + fact_file, | |
| 91 | + { name: fact_name, weight: 10, value: nil } | |
| 92 | + ) | |
| 93 | + end | |
| 94 | + | |
| 95 | + on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result| | |
| 96 | + assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not use the builtin fact's value when all conflicting custom facts fail to resolve") | |
| 97 | + end | |
| 98 | + end | |
| 99 | + | |
| 100 | + step "Verify that Facter gives precedence to the builtin fact over zero weight custom facts" do | |
| 101 | + step "when all custom facts have zero weight" do | |
| 102 | + { | |
| 103 | + 'timezone_one.rb' => "'timezone_one'", | |
| 104 | + 'timezone_two.rb' => "'timezone_two'" | |
| 105 | + }.each do |fact_file, fact_value| | |
| 106 | + create_custom_fact_on( | |
| 107 | + agent, | |
| 108 | + custom_fact_dir, | |
| 109 | + fact_file, | |
| 110 | + { name: fact_name, weight: 0, value: fact_value } | |
| 111 | + ) | |
| 112 | + end | |
| 113 | + | |
| 114 | + on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result| | |
| 115 | + assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not give precedence to the builtin fact when all custom facts have zero weight") | |
| 116 | + end | |
| 117 | + end | |
| 118 | + | |
| 119 | + clear_custom_facts_on(agent, custom_fact_dir) | |
| 120 | + | |
| 121 | + step "when some custom facts have zero weight" do | |
| 122 | + { | |
| 123 | + 'timezone_one.rb' => { weight: 10, value: nil }, | |
| 124 | + 'timezone_two.rb' => { weight: 0, value: "'timezone_two'" } | |
| 125 | + }.each do |fact_file, fact| | |
| 126 | + create_custom_fact_on( | |
| 127 | + agent, | |
| 128 | + custom_fact_dir, | |
| 129 | + fact_file, | |
| 130 | + fact.merge(name: fact_name) | |
| 131 | + ) | |
| 132 | + end | |
| 133 | + | |
| 134 | + on(agent, facter("--custom-dir=#{custom_fact_dir} timezone")) do |result| | |
| 135 | + assert_match(/#{builtin_value}/, result.stdout.chomp, "Facter does not give precedence to the builtin fact when only some custom facts have zero weight") | |
| 136 | + end | |
| 137 | + end | |
| 138 | + end | |
| 139 | + end | |
| 140 | +end | |
| 141 | diff --git a/lib/src/ruby/fact.cc b/lib/src/ruby/fact.cc | |
| 142 | index 9cdc3633..97ce14bc 100644 | |
| 143 | --- a/lib/src/ruby/fact.cc | |
| 144 | +++ b/lib/src/ruby/fact.cc | |
| 145 | @@ -83,53 +83,69 @@ namespace facter { namespace ruby { | |
| 146 | }); | |
| 147 | ||
| 148 | _resolving = true; | |
| 149 | - | |
| 150 | - // If no resolutions or the top resolution has a weight of 0, first check the native collection for the fact | |
| 151 | - // This way we treat the "built-in" as implicitly having a resolution with weight 0 | |
| 152 | bool add = true; | |
| 153 | - if (_resolutions.empty() || ruby.to_native<resolution>(_resolutions.front())->weight() == 0) { | |
| 154 | - // Check to see the value is in the collection | |
| 155 | - auto value = facts[ruby.to_string(_name)]; | |
| 156 | - if (value) { | |
| 157 | - // Already in collection, do not add | |
| 158 | - add = false; | |
| 159 | - _value = facter->to_ruby(value); | |
| 160 | - _weight = value->weight(); | |
| 161 | - } | |
| 162 | - } | |
| 163 | ||
| 164 | - if (ruby.is_nil(_value)) { | |
| 165 | - vector<VALUE>::iterator it; | |
| 166 | - ruby.rescue([&]() { | |
| 167 | - volatile VALUE value = ruby.nil_value(); | |
| 168 | - size_t weight = 0; | |
| 169 | - | |
| 170 | - // Look through the resolutions and find the first allowed resolution that resolves | |
| 171 | - for (it = _resolutions.begin(); it != _resolutions.end(); ++it) { | |
| 172 | - auto res = ruby.to_native<resolution>(*it); | |
| 173 | - if (!res->suitable(*facter)) { | |
| 174 | - continue; | |
| 175 | - } | |
| 176 | - value = res->value(); | |
| 177 | - if (!ruby.is_nil(value)) { | |
| 178 | - weight = res->weight(); | |
| 179 | - break; | |
| 180 | - } | |
| 181 | + vector<VALUE>::iterator it; | |
| 182 | + ruby.rescue([&]() { | |
| 183 | + volatile VALUE value = ruby.nil_value(); | |
| 184 | + size_t weight = 0; | |
| 185 | + | |
| 186 | + // Look through the resolutions and find the first allowed resolution that resolves | |
| 187 | + for (it = _resolutions.begin(); it != _resolutions.end(); ++it) { | |
| 188 | + auto res = ruby.to_native<resolution>(*it); | |
| 189 | + if (!res->suitable(*facter)) { | |
| 190 | + continue; | |
| 191 | + } | |
| 192 | + value = res->value(); | |
| 193 | + if (!ruby.is_nil(value)) { | |
| 194 | + weight = res->weight(); | |
| 195 | + break; | |
| 196 | } | |
| 197 | + } | |
| 198 | ||
| 199 | - // Set the value to what was resolved | |
| 200 | - _value = value; | |
| 201 | - _weight = weight; | |
| 202 | - return 0; | |
| 203 | - }, [&](VALUE ex) { | |
| 204 | - LOG_ERROR("error while resolving custom fact \"{1}\": {2}", ruby.rb_string_value_ptr(&_name), ruby.exception_to_string(ex)); | |
| 205 | + // Set the value to what was resolved | |
| 206 | + _value = value; | |
| 207 | + _weight = weight; | |
| 208 | ||
| 209 | - // Failed, so set to nil | |
| 210 | - _value = ruby.nil_value(); | |
| 211 | - _weight = 0; | |
| 212 | + if (! ruby.is_nil(_value) && _weight != 0) { | |
| 213 | return 0; | |
| 214 | - }); | |
| 215 | - } | |
| 216 | + } | |
| 217 | + | |
| 218 | + // There's two possibilities here: | |
| 219 | + // 1. None of our resolvers could resolve the value | |
| 220 | + // 2. A resolver of weight 0 resolved the value | |
| 221 | + // | |
| 222 | + // In both cases, we attempt to use the "built-in" fact's | |
| 223 | + // value. This serves as a fallback resolver for Case (1) | |
| 224 | + // while for Case (2), we want built-in values to take | |
| 225 | + // precedence over 0-weight custom facts. | |
| 226 | + | |
| 227 | + auto builtin_value = facts[ruby.to_string(_name)]; | |
| 228 | + if (! builtin_value) { | |
| 229 | + return 0; | |
| 230 | + } | |
| 231 | + auto builtin_ruby_value = facter->to_ruby(builtin_value); | |
| 232 | + | |
| 233 | + // We need this check for Case (2). Otherwise, we risk | |
| 234 | + // overwriting our resolved value in the small chance | |
| 235 | + // that builtin_value exists, but its ruby value is | |
| 236 | + // nil. | |
| 237 | + if (! ruby.is_nil(builtin_ruby_value)) { | |
| 238 | + // Already in collection, do not add | |
| 239 | + add = false; | |
| 240 | + _value = builtin_ruby_value; | |
| 241 | + _weight = builtin_value->weight(); | |
| 242 | + } | |
| 243 | + | |
| 244 | + return 0; | |
| 245 | + }, [&](VALUE ex) { | |
| 246 | + LOG_ERROR("error while resolving custom fact \"{1}\": {2}", ruby.rb_string_value_ptr(&_name), ruby.exception_to_string(ex)); | |
| 247 | + | |
| 248 | + // Failed, so set to nil | |
| 249 | + _value = ruby.nil_value(); | |
| 250 | + _weight = 0; | |
| 251 | + return 0; | |
| 252 | + }); | |
| 253 | ||
| 254 | if (add) { | |
| 255 | facts.add_custom(ruby.to_string(_name), ruby.is_nil(_value) ? nullptr : make_value<ruby::ruby_value>(_value), _weight); | |
| 256 | -- | |
| 257 | 2.20.1 | |
| 258 |