From 6ab68b837b8cde57af002ebc35a41b116c9cbc66 Mon Sep 17 00:00:00 2001
From: Trevor Vaughan <tvaughan@onyxpoint.com>
Date: Thu, 27 Feb 2020 21:16:22 -0500
Subject: [PATCH] Fix the firewalld_version fact

* Work around a bug in firewall-cmd and use raw python to get the
  firewalld version if firewalld-cmd does not return anything
* Incorporated the simp-beaker-helpers gem to get access to the helper
  methods

Fixes #263
---
 .sync.yml                                     |  2 +-
 Gemfile                                       |  2 +-
 lib/facter/firewalld_version.rb               | 30 ++++++++++++-
 rakelib/simp.rake                             |  7 +++
 .../{ => suites/default}/00_default_spec.rb   |  5 +++
 spec/spec_helper_acceptance.rb                | 44 ++++++++++---------
 spec/unit/facter/firewalld_version_spec.rb    | 11 ++++-
 7 files changed, 75 insertions(+), 26 deletions(-)
 create mode 100644 rakelib/simp.rake
 rename spec/acceptance/{ => suites/default}/00_default_spec.rb (90%)

diff --git a/.sync.yml b/.sync.yml
index 8cf3d046..ecfaf514 100644
--- a/.sync.yml
+++ b/.sync.yml
@@ -24,7 +24,7 @@ Gemfile:
       - gem: 'mocha'
         version: '~> 1.10.0'
     ':system_tests':
-      - gem: 'beaker-vagrant'
+      - gem: 'simp-beaker-helpers'
 
 .rubocop.yml:
   default_configs:
diff --git a/Gemfile b/Gemfile
index 7229102f..b70c1bdb 100644
--- a/Gemfile
+++ b/Gemfile
@@ -62,7 +62,7 @@ group :system_tests do
   gem 'rbnacl-libsodium',                   :require => false
   gem 'bcrypt_pbkdf',                       :require => false
   gem 'ed25519',                            :require => false
-  gem 'beaker-vagrant',                     :require => false
+  gem 'simp-beaker-helpers',                :require => false
 end
 
 group :release do
diff --git a/lib/facter/firewalld_version.rb b/lib/facter/firewalld_version.rb
index 305411b0..a0fc5196 100644
--- a/lib/facter/firewalld_version.rb
+++ b/lib/facter/firewalld_version.rb
@@ -2,10 +2,36 @@
 Facter.add(:firewalld_version) do
   confine { Process.uid.zero? }
 
-  @firewall_cmd = Facter::Util::Resolution.which('firewall-cmd')
+  @firewall_cmd = Facter::Util::Resolution.which('firewall-offline-cmd')
   confine { @firewall_cmd }
 
   setcode do
-    Facter::Core::Execution.execute(%(#{@firewall_cmd} --version), on_fail: :failed).strip
+    def failed_value?(value)
+      !value || (value == :failed) || value.empty?
+    end
+
+    value = Facter::Core::Execution.execute(%(#{@firewall_cmd} --version), on_fail: :failed)
+
+    if failed_value?(value)
+      # Python gets stuck in some weird places
+      python = Facter::Util::Resolution.which('python')
+      python ||= Facter::Util::Resolution.which('platform-python')
+
+      python_path = '/usr/libexec/platform-python'
+      python ||= python_path if File.exist?(python_path)
+
+      # Because firewall-cmd fails if firewalld is not running
+      workaround_command = %{#{python} -c 'import firewall.config; print(firewall.config.__dict__["VERSION"])'}
+
+      value = Facter::Core::Execution.execute(workaround_command, on_fail: :failed)
+    end
+
+    if failed_value?(value)
+      value = nil
+    else
+      value.strip!
+    end
+
+    value
   end
 end
diff --git a/rakelib/simp.rake b/rakelib/simp.rake
new file mode 100644
index 00000000..c1c667c2
--- /dev/null
+++ b/rakelib/simp.rake
@@ -0,0 +1,7 @@
+begin
+  require 'simp/rake/beaker'
+
+  Simp::Rake::Beaker.new(File.join(File.dirname(__FILE__), '..'))
+rescue LoadError
+  $stderr.puts('simp-beaker-helpers not loaded, some acceptance test functionality may be missing')
+end
diff --git a/spec/acceptance/00_default_spec.rb b/spec/acceptance/suites/default/00_default_spec.rb
similarity index 90%
rename from spec/acceptance/00_default_spec.rb
rename to spec/acceptance/suites/default/00_default_spec.rb
index 040b53e5..f27fb19b 100644
--- a/spec/acceptance/00_default_spec.rb
+++ b/spec/acceptance/suites/default/00_default_spec.rb
@@ -59,6 +59,11 @@ class { 'firewalld':
         test_services = on(host, 'firewall-cmd --list-services --zone=test').output.strip.split(%r{\s+})
         expect(test_services).to include('test_sshd')
       end
+
+      it 'returns a fact when firewalld is not running' do
+        on(host, 'puppet resource service firewalld ensure=stopped')
+        expect(pfact_on(host, 'firewalld_version')).to match(%r{^\d})
+      end
     end
   end
 end
diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb
index ea2ce17a..1cef0730 100644
--- a/spec/spec_helper_acceptance.rb
+++ b/spec/spec_helper_acceptance.rb
@@ -1,30 +1,34 @@
 require 'beaker-rspec'
-require 'pry'
+require 'tmpdir'
+require 'yaml'
+require 'simp/beaker_helpers'
+include Simp::BeakerHelpers
 
 UNSUPPORTED_PLATFORMS = %w[windows Darwin].freeze
 
-unless ENV['RS_PROVISION'] == 'no' || ENV['BEAKER_provision'] == 'no'
-
-  require 'beaker/puppet_install_helper'
-
-  run_puppet_install_helper('agent', ENV['PUPPET_VERSION'])
-
-  RSpec.configure do |c|
-    # Project root
-    proj_root = File.expand_path(File.join(__dir__, '..'))
+unless ENV['BEAKER_provision'] == 'no'
+  hosts.each do |host|
+    # Install Puppet
+    if host.is_pe?
+      install_pe
+    else
+      install_puppet
+    end
+  end
+end
 
-    # Readable test descriptions
-    c.formatter = :documentation
+RSpec.configure do |c|
+  # ensure that environment OS is ready on each host
+  fix_errata_on hosts
 
-    # Don't burn resources if we don't have to
-    c.fail_fast = true
+  # Readable test descriptions
+  c.formatter = :documentation
 
-    # Configure all nodes in nodeset
-    c.before :suite do
-      hosts.each do |host|
-        install_dev_puppet_module_on(host, source: proj_root, module_name: 'firewalld')
-        install_puppet_module_via_pmt_on(host, module_name: 'puppetlabs/stdlib')
-      end
+  # Configure all nodes in nodeset
+  c.before :suite do
+    begin
+      # Install modules and dependencies from spec/fixtures/modules
+      copy_fixture_modules_to(hosts)
     end
   end
 end
diff --git a/spec/unit/facter/firewalld_version_spec.rb b/spec/unit/facter/firewalld_version_spec.rb
index ad0d2563..c4298b3e 100644
--- a/spec/unit/facter/firewalld_version_spec.rb
+++ b/spec/unit/facter/firewalld_version_spec.rb
@@ -6,8 +6,12 @@
 
     Process.stubs(:uid).returns(0)
     Facter::Core::Execution.stubs(:exec).with('uname -s').returns('Linux')
-    Facter::Util::Resolution.stubs(:which).with('firewall-cmd').returns('/usr/bin/firewall-cmd')
-    Facter::Core::Execution.stubs(:execute).with('/usr/bin/firewall-cmd --version', on_fail: :failed).returns(firewalld_version)
+    Facter::Util::Resolution.stubs(:which).with('firewall-offline-cmd').returns('/usr/bin/firewall-offline-cmd')
+    Facter::Core::Execution.stubs(:execute).with('/usr/bin/firewall-offline-cmd --version', on_fail: :failed).returns(firewalld_version)
+  end
+
+  let(:python_args) do
+    %{-c 'import firewall.config; print(firewall.config.__dict__["VERSION"])'}
   end
 
   context 'as a regular user' do
@@ -32,6 +36,9 @@
     let(:firewalld_version) { :failed }
 
     it 'does not return a fact' do
+      Facter::Util::Resolution.stubs(:which).with('python').returns('/usr/bin/python')
+      Facter::Core::Execution.stubs(:execute).with("/usr/bin/python #{python_args}", on_fail: :failed).returns(:failed)
+
       expect(Facter.fact('firewalld_version').value).to be_nil
     end
   end