puppetlabs / facter

Collect and display system facts
https://puppet.com/open-source/#osp
Apache License 2.0
616 stars 494 forks source link

Evaluate confine block in case-insensitive way #2705

Closed mhashizume closed 2 months ago

mhashizume commented 2 months ago

Prior to this commit when a user provided a confine block, Facter would downcase the value when evaluating it.

For example:

confine :kernel do |value|
  value == "Linux"
end

While Facter's public documentation states that this is a valid way to write a confine block, it would incorrectly and unexpectedly evaluate as false on Linux systems.

However, these other styles of confine would return true:

confine :kernel => "Linux"

confine kernel: "Linux"

This downcasing behavior was introduced in 7a81945 as a way of comparing values in a case-insensitive way. However when block confines were introduced in e4c8689, it added block evaluation before value comparison, making the case-insensitive comparison moot with blocks.

This commit retains existing behavior of evaluating a confine block with a downcased fact value, while adding evaluation with the raw fact value to ensure expected behavior.

mhashizume commented 2 months ago

This was originally tried in https://github.com/puppetlabs/facter/pull/2699 but in that PR I changed

https://github.com/puppetlabs/facter/blob/5335371361dd354da5138c8c07916ec436792838/lib/facter/custom_facts/util/confine.rb#L59

To return !@block.call(value).nil because the original code triggered Rubocop's Style/DoubleNegation cop. Those two approaches are not equivalent, so in this PR I simply disabled the cop.

We realized the behavior on the first PR was not correct because it gave the wrong output for a custom fact used internally in Puppet. That custom fact relies on using multiple resolutions with OS-specific confine blocks like so:

Facter.add('test_fact') do
  confine :kernel do |value|
    value == 'darwin'
  end

  setcode do
    'foo'
  end
end

Facter.add('test_fact') do
  confine :kernel do |value|
    value == 'linux'
  end

  setcode do
    'bar'
  end
end

In this PR I've added test coverage for such circumstances.