Repository: saltstack/salt
Pull request: Add FreeBSD support to pf executor
### What does this PR do?
Enables FreeBSD support for the pf module; adds FreeBSD to OS whitelist and makes list of available loglevels OS-dependent.
### What issues does this PR fix or reference?
This PR fixes #33248.
### Previous Behavior
Does not execute under FreeBSD.
### New Behavior
Will now execute on FreeBSD or OpenBSD.
### Tests written?
Yes
### Commits signed with GPG?
Yes

Problem statement:
Execution module for FreeBSD Packet Filter
### Description of Issue/Question

I searched for an existing execution module that would be an equivalent of `iptables` for FreeBSD's `pf`. I couldn't find one in the docs/repo ( I looked in the develop branch). Is there a planned `pf` execution module for the future? Or maybe there is one already but I've missed it. Thanks!
### Setup

(Please provide relevant configs and/or SLS files (Be sure to remove sensitive info).)
N/A
### Steps to Reproduce Issue

(Include debug logs if possible and relevant.)
N/A
### Versions Report

(Provided by running `salt --versions-report`. Please also mention any differences in master/minion versions.)
N/A

Hints from issue discussion:
@avb76 i also did not see one available as well in the code. I will go ahead and label this as a feature request. i think this is a great idea :)

@Ch3LL: great :)

@avb76 I just noticed this issue, you may want to have a look at the new module I've proposed in #45905 to see if it works on FreeBSD too.
@jasperla Your pf module works just fine on FreeBSD; the only exception is that the available loglevels are different:

- none
- urgent
- misc
- loud

Commit message: Add FreeBSD support to pf executor

Patch to review:
diff --git a/salt/modules/pf.py b/salt/modules/pf.py
index 7bb495e140ab..b5cd62511141 100644
--- a/salt/modules/pf.py
+++ b/salt/modules/pf.py
@@ -22,13 +22,16 @@
 
 def __virtual__():
     '''
-    Only works on OpenBSD for now; other systems with pf (macOS, FreeBSD, etc)
-    need to be tested before enabling them.
+    Only works on OpenBSD and FreeBSD for now; other systems with pf (macOS,
+    FreeBSD, etc) need to be tested before enabling them.
     '''
-    if __grains__['os'] == 'OpenBSD' and salt.utils.path.which('pfctl'):
+    tested_oses = ['FreeBSD', 'OpenBSD']
+    if __grains__['os'] in tested_oses and salt.utils.path.which('pfctl'):
         return True
 
-    return (False, 'The pf execution module cannot be loaded: either the system is not OpenBSD or the pfctl binary was not found')
+    return (False, 'The pf execution module cannot be loaded: either the '
+            'OS (' + __grains__['os'] + ') is not tested or the pfctl binary '
+            'was not found')
 
 
 def enable():
@@ -99,7 +102,7 @@ def loglevel(level):
 
     level:
         Log level. Should be one of the following: emerg, alert, crit, err, warning, notice,
-        info or debug.
+        info or debug (OpenBSD); or none, urgent, misc, loud (FreeBSD).
 
     CLI example:
 
@@ -111,7 +114,11 @@ def loglevel(level):
     # always made a change.
     ret = {'changes': True}
 
-    all_levels = ['emerg', 'alert', 'crit', 'err', 'warning', 'notice', 'info', 'debug']
+    myos = __grains__['os']
+    if myos == 'FreeBSD':
+        all_levels = ['none', 'urgent', 'misc', 'loud']
+    else:
+        all_levels = ['emerg', 'alert', 'crit', 'err', 'warning', 'notice', 'info', 'debug']
     if level not in all_levels:
         raise SaltInvocationError('Unknown loglevel: {0}'.format(level))
 
diff --git a/tests/unit/modules/test_pf.py b/tests/unit/modules/test_pf.py
index 3b2306ac270c..32d2f0799535 100644
--- a/tests/unit/modules/test_pf.py
+++ b/tests/unit/modules/test_pf.py
@@ -66,14 +66,29 @@ def test_disable_when_disabled(self):
         with patch.dict(pf.__salt__, {'cmd.run_all': mock_cmd}):
             self.assertFalse(pf.disable()['changes'])
 
-    def test_loglevel(self):
+    def test_loglevel_freebsd(self):
         '''
         Tests setting a loglevel.
         '''
         ret = {}
         ret['retcode'] = 0
         mock_cmd = MagicMock(return_value=ret)
-        with patch.dict(pf.__salt__, {'cmd.run_all': mock_cmd}):
+        with patch.dict(pf.__salt__, {'cmd.run_all': mock_cmd}), \
+                patch.dict(pf.__grains__, {'os': 'FreeBSD'}):
+            res = pf.loglevel('urgent')
+            mock_cmd.assert_called_once_with('pfctl -x urgent',
+                    output_loglevel='trace', python_shell=False)
+            self.assertTrue(res['changes'])
+
+    def test_loglevel_openbsd(self):
+        '''
+        Tests setting a loglevel.
+        '''
+        ret = {}
+        ret['retcode'] = 0
+        mock_cmd = MagicMock(return_value=ret)
+        with patch.dict(pf.__salt__, {'cmd.run_all': mock_cmd}), \
+                patch.dict(pf.__grains__, {'os': 'OpenBSD'}):
             res = pf.loglevel('crit')
             mock_cmd.assert_called_once_with('pfctl -x crit',
                     output_loglevel='trace', python_shell=False)


Code review comments:
diff_hunk: @@ -66,14 +66,29 @@ def test_disable_when_disabled(self):
         with patch.dict(pf.__salt__, {'cmd.run_all': mock_cmd}):
             self.assertFalse(pf.disable()['changes'])
 
-    def test_loglevel(self):
+    def test_loglevel_freebsd(self):
path: tests/unit/modules/test_pf.py
text: @user1:
Why are we changing an existing test to apply specifically to FreeBSD? This should be a separate test or this test should be extended to support both the existing and the new instead of modified like this.

@author:
The existing tests do it both possible ways; I was looking at e.g. `test/unit/modules/test_cron.py`, but there are also tests that have all the different OS variations in one test case (e.g. `./integration/states/test_pkg.py`). I'm not particularly attached to either, but I can definitely rewrite as one TC.