[generic-config-updater] Improving CreateOnly validator and marking /LOOPBACK_INTERFACE/LOOPBACK#/vrf_name as create-only

generic_config_updater/patch_sorter.py

@@ -372,40 +372,58 @@ def validate(self, move, diff):
 
 class CreateOnlyMoveValidator:
     """
-    A class to validate create-only fields are only added/removed but never replaced.
-    Parents of create-only fields are also only added/removed but never replaced when they contain
-    a modified create-only field.
+    A class to validate create-only fields are only created, but never modified/updated. In other words:
+    - Field cannot be replaced.
+    - Field cannot be added, only if the parent is added.
+    - Field cannot be deleted, only if the parent is deleted.
     """
     def __init__(self, path_addressing):
         self.path_addressing = path_addressing
 
-    def validate(self, move, diff):
-        if move.op_type != OperationType.REPLACE:
-            return True
+        # TODO: create-only fields are hard-coded for now, it should be moved to YANG models
+        # Each pattern consist of a list of tokens. Token matching starts from the root level of the config.
+        # Each token is either a specific key or '*' to match all keys.
+        self.create_only_patterns = [
+            ["PORT", "*", "lanes"],
+            ["LOOPBACK_INTERFACE", "*", "vrf_name"],
+        ]
 
-        # The 'create-only' field needs to be common between current and simulated anyway but different.
-        # This means it is enough to just get the paths from current_config, paths that are not common can be ignored.
-        paths = self._get_create_only_paths(diff.current_config)
+    def validate(self, move, diff):
         simulated_config = move.apply(diff.current_config)
+        paths = set(list(self._get_create_only_paths(diff.current_config)) + list(self._get_create_only_paths(simulated_config)))
 
         for path in paths:
             tokens = self.path_addressing.get_path_tokens(path)
             if self._value_exist_but_different(tokens, diff.current_config, simulated_config):
                 return False
+            if self._value_added_but_parent_exist(tokens, diff.current_config, simulated_config):
+                return False
+            if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config):
+                return False
 
         return True
 
-    # TODO: create-only fields are hard-coded for now, it should be moved to YANG models
     def _get_create_only_paths(self, config):
-        if "PORT" not in config:
+        for pattern in self.create_only_patterns:
+            for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0):
+                yield create_only_path
+
+    def _get_create_only_path_recursive(self, config, pattern_tokens, matching_tokens, idx):
+        if idx == len(pattern_tokens):
+            yield '/' + '/'.join(matching_tokens)
             return
 
-        ports = config["PORT"]
+        matching_keys = []
+        if pattern_tokens[idx] == "*":
+            matching_keys = config.keys()
+        elif pattern_tokens[idx] in config:
+            matching_keys = [pattern_tokens[idx]]
 
-        for port in ports:
-            attrs = ports[port]
-            if "lanes" in attrs:
-                yield f"/PORT/{port}/lanes"
+        for key in matching_keys:
+            matching_tokens.append(key)
+            for create_only_path in self._get_create_only_path_recursive(config[key], pattern_tokens, matching_tokens, idx+1):
+                yield create_only_path
+            matching_tokens.pop()
 
     def _value_exist_but_different(self, tokens, current_config_ptr, simulated_config_ptr):
         for token in tokens:
@@ -422,6 +440,46 @@ def _value_exist_but_different(self, tokens, current_config_ptr, simulated_confi
 
         return current_config_ptr != simulated_config_ptr
 
+    def _value_added_but_parent_exist(self, tokens, current_config_ptr, simulated_config_ptr):
+        # if value is not added, return false
+        if not self._exist_only_in_first(tokens, simulated_config_ptr, current_config_ptr):
+            return False
+
+        # if parent is added, return false
+        if self._exist_only_in_first(tokens[:-1], simulated_config_ptr, current_config_ptr):
+            return False
+
+        # otherwise parent exist and value is added
+        return True
+
+    def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated_config_ptr):
+        # if value is not removed, return false
+        if not self._exist_only_in_first(tokens, current_config_ptr, simulated_config_ptr):
+            return False
+
+        # if parent is removed, return false
+        if self._exist_only_in_first(tokens[:-1], current_config_ptr, simulated_config_ptr):
+            return False
+
+        # otherwise parent remained and value is removed
+        return True
+
+    def _exist_only_in_first(self, tokens, first_config_ptr, second_config_ptr):
+        for token in tokens:
+            mod_token = int(token) if isinstance(first_config_ptr, list) else token
+
+            if mod_token not in second_config_ptr:
+                return True
+
+            if mod_token not in first_config_ptr:
+                return False
+
+            first_config_ptr = first_config_ptr[mod_token]
+            second_config_ptr = second_config_ptr[mod_token]
+
+        # tokens exist in both
+        return False
+
 class NoDependencyMoveValidator:
     """
     A class to validate that the modified configs do not have dependency on each other. This should prevent

tests/generic_config_updater/files/config_db_with_loopback_interfaces.json

@@ -0,0 +1,16 @@
+{
+    "LOOPBACK_INTERFACE": {
+        "Loopback0": {},
+        "Loopback0|10.1.0.32/32": {},
+        "Loopback0|1100:1::32/128": {},
+        "Loopback1": {
+            "vrf_name": "Vrf_02"
+        },
+        "Loopback1|20.2.0.32/32": {},
+        "Loopback1|2200:2::32/128": {}
+    },
+    "VRF": {
+        "Vrf_01": {},
+        "Vrf_02": {}
+    }
+}

tests/generic_config_updater/patch_sorter_test.py

@@ -784,13 +784,6 @@ def setUp(self):
         self.validator = ps.CreateOnlyMoveValidator(ps.PathAddressing())
         self.any_diff = ps.Diff({}, {})
 
-    def test_validate__non_replace_operation__success(self):
-        # Assert
-        self.assertTrue(self.validator.validate( \
-            ps.JsonMove(self.any_diff, OperationType.ADD, [], []), self.any_diff))
-        self.assertTrue(self.validator.validate( \
-            ps.JsonMove(self.any_diff, OperationType.REMOVE, [], []), self.any_diff))
-
     def test_validate__no_create_only_field__success(self):
         current_config = {"PORT": {}}
         target_config = {"PORT": {}, "ACL_TABLE": {}}
@@ -833,30 +826,86 @@ def test_validate__different_create_only_field_updating_grandparent__failure(sel
                          ["PORT"],
                          False)
 
-    def test_validate__same_create_only_field_directly_updated__failure(self):
+    def test_validate__same_create_only_field_directly_updated__success(self):
         current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}}
         target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
         self.verify_diff(current_config,
                          target_config,
                          ["PORT", "Ethernet0", "lanes"],
                          ["PORT", "Ethernet0", "lanes"])
 
-    def test_validate__same_create_only_field_updating_parent__failure(self):
+    def test_validate__same_create_only_field_updating_parent__success(self):
         current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}}
         target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
         self.verify_diff(current_config,
                          target_config,
                          ["PORT", "Ethernet0"],
                          ["PORT", "Ethernet0"])
 
-    def test_validate__same_create_only_field_updating_grandparent__failure(self):
+    def test_validate__same_create_only_field_updating_grandparent__success(self):
         current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}}
         target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
         self.verify_diff(current_config,
                          target_config,
                          ["PORT"],
                          ["PORT"])
 
+    def test_validate__added_create_only_field_parent_exist__failure(self):
+        current_config = {"PORT": {"Ethernet0":{}}}
+        target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
+        self.verify_diff(current_config,
+                         target_config,
+                         ["PORT"],
+                         ["PORT"],
+                         expected=False)
+
+    def test_validate__added_create_only_field_parent_doesnot_exist__success(self):
+        current_config = {"PORT": {}}
+        target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
+        self.verify_diff(current_config,
+                         target_config,
+                         ["PORT"],
+                         ["PORT"])
+
+    def test_validate__removed_create_only_field_parent_remain__failure(self):
+        current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
+        target_config = {"PORT": {"Ethernet0":{}}}
+        self.verify_diff(current_config,
+                         target_config,
+                         ["PORT"],
+                         ["PORT"],
+                         expected=False)
+
+    def test_validate__removed_create_only_field_parent_doesnot_remain__success(self):
+        current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}}
+        target_config = {"PORT": {}}
+        self.verify_diff(current_config,
+                         target_config,
+                         ["PORT"],
+                         ["PORT"])
+
+    def test_hard_coded_create_only_paths(self):
+        config = {
+            "PORT": {
+                "Ethernet0":{"lanes":"65"},
+                "Ethernet1":{},
+                "Ethernet2":{"lanes":"66,67"}
+            },
+            "LOOPBACK_INTERFACE": {
+                "Loopback0":{"vrf_name":"vrf0"},
+                "Loopback1":{},
+                "Loopback2":{"vrf_name":"vrf1"},
+            }}
+        expected = [
+            "/PORT/Ethernet0/lanes",
+            "/PORT/Ethernet2/lanes",
+            "/LOOPBACK_INTERFACE/Loopback0/vrf_name",
+            "/LOOPBACK_INTERFACE/Loopback2/vrf_name"
+        ]
+        actual = self.validator._get_create_only_paths(config)
+
+        self.assertCountEqual(expected, actual)
+
     def verify_diff(self, current_config, target_config, current_config_tokens=None, target_config_tokens=None, expected=True):
         # Arrange
         current_config_tokens = current_config_tokens if current_config_tokens else []
@@ -1741,6 +1790,26 @@ def test_sort__replacing_create_only_field__success(self):
         # Assert
         self.assertNotEqual(None, actual)
 
+    def test_sort__adding_create_only_field__success(self):
+        # Arrange
+        patch = jsonpatch.JsonPatch([{"op":"add", "path": "/LOOPBACK_INTERFACE/Loopback0/vrf_name", "value":"Vrf_01"}])
+
+        # Act
+        actual = self.create_patch_sorter(Files.CONFIG_DB_WITH_LOOPBACK_INTERFACES).sort(patch)
+
+        # Assert
+        self.assertNotEqual(None, actual)
+
+    def test_sort__removing_create_only_field__success(self):
+        # Arrange
+        patch = jsonpatch.JsonPatch([{"op":"remove", "path": "/LOOPBACK_INTERFACE/Loopback1/vrf_name"}])
+
+        # Act
+        actual = self.create_patch_sorter(Files.CONFIG_DB_WITH_LOOPBACK_INTERFACES).sort(patch)
+
+        # Assert
+        self.assertNotEqual(None, actual)
+
     def test_sort__inter_dependency_within_same_table__success(self):
         # Arrange
         patch = jsonpatch.JsonPatch([{"op":"add", "path":"/VLAN_INTERFACE", "value": {