Handle missing dva files (#341)

.github/workflows/main.yaml

@@ -46,6 +46,14 @@ jobs:
         run: bin/setup_remote faucet
       - name: build docker
         run: bin/retry_cmd bin/build_docker
+      - name: run python tests - test_forchestrator
+        run: testing/python_test test_forchestrator
+      - name: run python tests - test_faucetizer
+        run: testing/python_test test_faucetizer
+      - name: run python tests - test_faucet_state_collector
+        run: testing/python_test test_faucet_state_collector
+      - name: run python tests - test_event
+        run: testing/python_test test_event
       - name: run test
         run: bin/run_test_set base
 

forch/faucetizer.py

@@ -449,6 +449,11 @@ def clear_static_placement(self, mac):
         """Remove static placement for devices with mac if exists"""
         self._static_devices.device_mac_placements.pop(mac.lower(), None)
 
+    def clear_static_placements(self):
+        """Remove all static placement"""
+        for mac in self._static_devices.device_mac_placements:
+            self.clear_static_placement(mac)
+
     def flush_behavioral_config(self, force=False):
         """Generate and write behavioral config to file"""
         if not force and self._config.faucetize_interval_sec:

forch/file_change_watcher.py

@@ -38,8 +38,8 @@ def stop(self):
 
     def register_file_callback(self, file_path, file_change_callback):
         """Register a file handler"""
-        content, _ = self._get_file_data(file_path)
-        self._watched_files[file_path] = FileData(content, hash, file_change_callback)
+        content, content_hash = self._get_file_data(file_path)
+        self._watched_files[file_path] = FileData(content, content_hash, file_change_callback)
 
     def unregister_file_callback(self, file_path):
         """Unregister the handler for a file"""
@@ -66,7 +66,7 @@ def _handle_file_change(self, file_path):
 
     def _get_file_data(self, file_path):
         if not os.path.exists(file_path):
-            return None
+            return None, None
 
         with open(file_path) as file:
             content = file.read()

forch/forchestrator.py

@@ -74,6 +74,7 @@
 )
 
 STATIC_BEHAVIORAL_FILE = 'static_behavior_file'
+STATIC_PLACEMENT_FILE = 'static_placement_file'
 SEGMENTS_VLANS_FILE = 'segments_vlans_file'
 TAIL_ACL_CONFIG = 'tail_acl_config'
 DEFAULT_SEQUESTER_SEGMENT = 'SEQUESTER'
@@ -278,14 +279,28 @@ def _process_static_device_placement(self):
         if not placement_file_name:
             return
         placement_file_path = os.path.join(self._forch_config_dir, placement_file_name)
-        with open(placement_file_path, 'r') as fd:
-            content = fd.read()
-            self._reload_static_device_placement(placement_file_path, content)
+        content = None
+        if os.path.isfile(placement_file_path):
+            with open(placement_file_path, 'r') as fd:
+                content = fd.read()
+        self._reload_static_device_placement(placement_file_path, content)
         self._config_file_watcher.register_file_callback(
             placement_file_path, self._reload_static_device_placement)
 
     def _reload_static_device_placement(self, file_path, new, current=None):
-        new_mac_placements = yaml_content_proto(new, DevicesState).device_mac_placements
+        try:
+            self._logger.info('Reading static device placement file: %s', file_path)
+            new_mac_placements = yaml_content_proto(new, DevicesState).device_mac_placements
+        except Exception as error:
+            msg = f'All auth was disabled: could not load static placement file {file_path}'
+            self._logger.error('%s: %s', msg, error)
+            if self._faucetizer:
+                self._faucetizer.clear_static_placements()
+            with self._states_lock:
+                self._forch_config_errors[STATIC_PLACEMENT_FILE] = msg
+                self._should_ignore_auth_result = True
+            return
+
         current_macs = set()
         if current:
             current_mac_placements = yaml_content_proto(current, DevicesState).device_mac_placements
@@ -313,9 +328,11 @@ def _process_static_device_behavior(self):
         if not behaviors_file_name:
             return
         behaviors_file_path = os.path.join(self._forch_config_dir, behaviors_file_name)
-        with open(behaviors_file_path, 'r') as fd:
-            content = fd.read()
-            self._reload_static_device_behavior(behaviors_file_path, content)
+        content = None
+        if os.path.isfile(behaviors_file_path):
+            with open(behaviors_file_path, 'r') as fd:
+                content = fd.read()
+        self._reload_static_device_behavior(behaviors_file_path, content)
         self._config_file_watcher.register_file_callback(
             behaviors_file_path, self._reload_static_device_behavior)
 

testing/python_lib/test_event.py

@@ -66,7 +66,7 @@ def test_out_of_sequence(self):
 
         try:
             self._forchestrator.main_loop()
-        except MetricsFetchingError as error:
+        except Exception as error:
             # Forchestrator.restore_states() will raise VarzFetchingError as Faucet prometheus
             # client is not enabled, and thus this error implies restore_states() is called.
             print(f'Expected error during restoring states: {error}')

testing/python_lib/test_forchestrator.py

@@ -2,13 +2,23 @@
 
 from unittest.mock import Mock, MagicMock
 import unittest
+from unittest.mock import patch
+import os
+import tempfile
+
 import yaml
+
 from unit_base import ForchestratorTestBase
 from forch.authenticator import Authenticator
+from forch.faucet_state_collector import FaucetStateCollector
+from forch.faucetizer import Faucetizer
+from forch.forchestrator import (Forchestrator, STATIC_BEHAVIORAL_FILE, STATIC_PLACEMENT_FILE,
+                                 SEGMENTS_VLANS_FILE)
+from forch.file_change_watcher import FileChangeWatcher
 from forch.port_state_manager import PortStateManager
 from forch.utils import dict_proto
 from forch.proto.devices_state_pb2 import DevicePlacement, DeviceBehavior
-from forch.proto.forch_configuration_pb2 import OrchestrationConfig
+from forch.proto.forch_configuration_pb2 import ForchConfig, OrchestrationConfig
 from forch.proto.system_state_pb2 import SystemState
 
 
@@ -52,7 +62,9 @@ def test_faucet_config_validation(self):
                 tagged_vlans: [171]
             lldp_beacon: {max_per_interval: 5, send_interval: 5}"""
         faucet_config = yaml.safe_load(faucet_config_str)
-        self.assertFalse(self._forchestrator._validate_config(faucet_config))
+        self.assertEqual(self._forchestrator._validate_config(faucet_config),
+                         [('nz-kiwi-t1sw1:04', 'misconfigured interface config: 0 0 0 0 0'),
+                         ('nz-kiwi-t1sw1:05', 'misconfigured interface config: 0 0 0 0 0')])
 
     def test_config_error_detail(self):
         """Test config detail for config errors"""
@@ -111,12 +123,73 @@ def test_auth_learn(self):
         self.assertEqual(self._get_auth_sm_state('00:11:22:33:44:55'), 'Authorized')
         device_placement = DevicePlacement(switch='switch', port=1, connected=False)
         self._forchestrator._process_device_placement('00:11:22:33:44:55',
-                                                      device_placement, expired_vlan=200)
-        self.assertEqual(self._get_auth_sm_state('00:11:22:33:44:55'), 'Authorized')
-        self._forchestrator._process_device_placement('00:11:22:33:44:55',
-                                                      device_placement, expired_vlan=100)
+                                                      device_placement)
         self.assertEqual(self._get_auth_sm_state('00:11:22:33:44:55'), None)
 
 
+# pylint: disable=protected-access
+class ForchestratorMissingDVAFilesTestCase(unittest.TestCase):
+    """Test case for forchestrator with missing DVA files."""
+    _DEFAULT_FORCH_LOG = '/tmp/forch.log'
+
+    FORCH_CONFIG = """
+    orchestration:
+      %s
+      structural_config_file: faucet.yaml
+      sequester_config:
+        vlan_start: 272
+        vlan_end: 276
+        port_description: TAP
+        auto_sequestering: disabled
+    """
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        os.environ['FORCH_LOG'] = self._DEFAULT_FORCH_LOG
+        os.environ['FORCH_CONFIG_DIR'] = tempfile.mkdtemp()
+        self._forchestrator = None
+
+    @patch.object(Faucetizer, 'flush_behavioral_config', return_value=None)
+    @patch.object(Faucetizer, 'tail_acl_config_valid', return_value=True)
+    def setup(self, config, *args):
+        """setup fixture for each test method"""
+        forch_config = dict_proto(yaml.safe_load(config), ForchConfig)
+        self._forchestrator = Forchestrator(forch_config)
+        self._forchestrator._faucet_collector = FaucetStateCollector(
+            self._forchestrator._config, is_faucetizer_enabled=True)
+        self._forchestrator._should_enable_faucetizer = True
+        with open(os.path.join(os.environ['FORCH_CONFIG_DIR'], 'faucet.yaml'), 'w') as fd:
+            fd.write('segments_to_vlans:')
+        self._forchestrator._config_file_watcher = FileChangeWatcher(
+            os.environ['FORCH_CONFIG_DIR'])
+        self._forchestrator._calculate_orchestration_config()
+        self._forchestrator._initialize_orchestration()
+
+    @patch.object(Faucetizer, 'reload_segments_to_vlans', return_value=None)
+    def test_missing_static_device_behavior_file(self, *args):
+        """Test a missing static_device_behavior file won't crash forch"""
+        addon = """static_device_behavior: missing_file.yaml
+      segments_vlans_file: segments_vlans.yaml"""
+        self.setup(self.FORCH_CONFIG % addon)
+        self.assertTrue(self._forchestrator._should_ignore_auth_result)
+        self.assertIsNotNone(self._forchestrator._forch_config_errors.get(STATIC_BEHAVIORAL_FILE))
+
+    def test_missing_static_device_placement_file(self):
+        """Test a missing static_device_placement file won't crash forch"""
+        addon = """static_device_placement: missing_file.yaml
+      segments_vlans_file: missing_file.yaml"""
+        self.setup(self.FORCH_CONFIG % addon)
+        self.assertTrue(self._forchestrator._should_ignore_auth_result)
+        self.assertIsNotNone(self._forchestrator._forch_config_errors.get(STATIC_PLACEMENT_FILE))
+
+    def test_missing_segments_vlan_file(self):
+        """Test a missing segments_vlan file won't crash forch"""
+        addon = """segments_vlans_file: missing_file.yaml"""
+        self.setup(self.FORCH_CONFIG % addon)
+        self.assertTrue(self._forchestrator._should_ignore_auth_result)
+        self.assertTrue(self._forchestrator._should_ignore_static_behavior)
+        self.assertIsNotNone(self._forchestrator._forch_config_errors.get(SEGMENTS_VLANS_FILE))
+
+
 if __name__ == '__main__':
     unittest.main()