test: make reading pf rules context aware

Author: jaredhendrickson13

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsFirewallRuleTestCase.inc

@@ -11,8 +11,44 @@ use RESTAPI\Models\RoutingGateway;
 use RESTAPI\Models\TrafficShaper;
 use RESTAPI\Models\TrafficShaperLimiter;
 use RESTAPI\Models\TrafficShaperLimiterQueue;
+use RESTAPI\Responses\ServerError;
 
 class APIModelsFirewallRuleTestCase extends TestCase {
+    /**
+     * Reads the active ruleset directly from pfctl.
+     */
+    public function read_pfctl_rules(): Command {
+        # Keywords that indicate pf is not ready yet
+        $not_ready_keywords = ['pfctl: DIOCGETRULE: Device busy'];
+
+        # Check the pf ruleset until it appears to be fully loaded, or until we've tried 5 times
+        $attempt = 0;
+        $max_attempts = 5;
+        while ($attempt < $max_attempts) {
+            $cmd = new Command('/sbin/pfctl -sr');
+            $ready = true;
+
+            foreach ($not_ready_keywords as $keyword) {
+                # pf is not ready if any of the keywords are found in the output
+                if (str_contains($cmd->output, $keyword)) {
+                    $ready = false;
+                    $attempt++;
+                    sleep(1);
+                    break;
+                }
+            }
+
+            if ($ready) {
+                return $cmd;
+            }
+        }
+
+        throw new ServerError(
+            message: "pfctl ruleset was not ready after $max_attempts attempts.",
+            response_id: 'API_MODELS_FIREWALL_RULE_TEST_CASE_PFCTL_NOT_READY',
+        );
+    }
+
     /**
      * Checks that multiple interfaces cannot be assigned to a FirewallRule unless `floating` is enabled.
      */
@@ -508,20 +544,20 @@ class APIModelsFirewallRuleTestCase extends TestCase {
         $rule->create(apply: true);
 
         # Ensure the rule with the queue is seen in pfctl
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains($pfctl->output, 'ridentifier ' . $rule->tracker->value . ' queue TestQueue1');
 
         # Update the rule to use a different queue and ensure it is seen in pfctl
         $rule->defaultqueue->value = 'TestQueue2';
         $rule->update(apply: true);
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains($pfctl->output, 'ridentifier ' . $rule->tracker->value . ' queue TestQueue2');
 
         # Delete the rule and ensure the rule referencing the queue no longer exists
         $rule->delete();
         $shaper1->delete();
         $rule->apply();
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_does_not_contain(
             $pfctl->output,
             'ridentifier ' . $rule->tracker->value . ' queue TestQueue1',
@@ -577,7 +613,7 @@ class APIModelsFirewallRuleTestCase extends TestCase {
         $rule->create(apply: true);
 
         # Ensure the rule with the queue is seen in pfctl
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains(
             $pfctl->output,
             'ridentifier ' . $rule->tracker->value . ' queue(TestQueue1, TestQueue2)',
@@ -587,7 +623,7 @@ class APIModelsFirewallRuleTestCase extends TestCase {
         $rule->defaultqueue->value = 'TestQueue2';
         $rule->ackqueue->value = 'TestQueue1';
         $rule->update(apply: true);
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains(
             $pfctl->output,
             'ridentifier ' . $rule->tracker->value . ' queue(TestQueue2, TestQueue1)',
@@ -597,7 +633,7 @@ class APIModelsFirewallRuleTestCase extends TestCase {
         $rule->delete();
         $shaper1->delete();
         $rule->apply();
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_does_not_contain(
             $pfctl->output,
             'ridentifier ' . $rule->tracker->value . ' queue(TestQueue1, TestQueue2)',
@@ -795,10 +831,9 @@ class APIModelsFirewallRuleTestCase extends TestCase {
             async: false,
         );
         $rule->create(apply: true);
-        sleep(3); // Wait a bit to ensure device is not busy
 
         # Ensure the dnpipe is correctly represented
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains(
             $pfctl->output,
             "ridentifier {$rule->tracker->value} dnpipe {$limiter->number->value}",
@@ -843,10 +878,9 @@ class APIModelsFirewallRuleTestCase extends TestCase {
             async: false,
         );
         $rule->create(apply: true);
-        sleep(3); // Wait a bit to ensure device is not busy
 
         # Check pfctl rules and ensure the dnpipe is correctly represented
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains(
             $pfctl->output,
             "ridentifier {$rule->tracker->value} dnqueue {$queue->number->value}",
@@ -894,10 +928,9 @@ class APIModelsFirewallRuleTestCase extends TestCase {
             async: false,
         );
         $rule->create(apply: true);
-        sleep(3); // Wait a bit to ensure device is not busy
 
         # Check pfctl rules and ensure the dnpipe is correctly represented
-        $pfctl = new Command('pfctl -sr');
+        $pfctl = $this->read_pfctl_rules();
         $this->assert_str_contains(
             $pfctl->output,
             "ridentifier {$rule->tracker->value} dnpipe({$limiter1->number->value}, {$limiter2->number->value})",