Skip to content

Commit 01dfb9c

Browse files
ghoooghooo
authored
[GCU] Mark children of bgp_neighbor as create-only (#2008)
#### What I did Fixes #2007 Most of the children of `/BGP_NEIGHBOR/*` except `admin_status` are create-only field i.e. they can only be created with the neighbor but cannot be modified later. Validated each attribute is read-only by the following steps: * Delete a neighbor * Add the neighbor back without the attribute under test e.g. `holdtime` * show running config for the neighbor * show neighbor config using `show ip bgp neighbor <ip>` * Add just the attribute under test e.g. `holdtime` * show running config for the neighbor -- we can see the attribute is added * show neighbor config using `show ip bgp neighbor <ip>` -- we can see the attribute change did not take effect Example for `holdtime`: ```sh admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i '' . . . Patch applied successfully. admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i '' . . . Error: can't remove a non-existent object '10.0.0.57' admin@vlab-01:~$ sudo config apply-patch add-bgp-neighbor-without-holdtime.json -i '' Patch Applier: Patch application starting. Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57", "value": {"admin_status": "up", "asn": "64600", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0"}}] . . . Patch applied successfully. admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A8 "10.0.0.57": { "admin_status": "up", "asn": "64600", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0" }, admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57 . . . Hold time is 180, keepalive interval is 3 seconds . . . admin@vlab-01:~$ sudo config apply-patch add-holdtime.json -i '' Patch Applier: Patch application starting. Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57/holdtime", "value": "10"}] . . . Patch applied successfully. admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A10 "10.0.0.57": { "admin_status": "up", "asn": "64600", "holdtime": "10", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0" }, "10.0.0.59": { admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57 BGP neighbor is 10.0.0.57, remote AS 64600, local AS 65100, external link . . . Hold time is 180, keepalive interval is 3 seconds . . . admin@vlab-01:~$ ``` Also added a validation to `create-only` fields to reject moves that add their parents without them, because we would have to delete their parents again later and add it back. There is no point. Example assume we have 2 fields marked with create-only namely x,y and they are under c. The patch would be: ``` {"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}} ``` The generated moves would be: ``` {"op":"add", "path":"/a/b/c", "value":{"x":"value_x"}} {"op":"remove", "path":"/a/b/c"} {"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}} ``` There is no point of the first 2 moves, because the `y` is create only and it will require the object to be deleted again then added. #### How I did it Marked the fields as create only #### How to verify it unit-test #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed)
1 parent ad1ed4e commit 01dfb9c

3 files changed

Lines changed: 185 additions & 17 deletions

File tree

‎generic_config_updater/patch_sorter.py‎

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -393,11 +393,23 @@ def __init__(self, path_addressing):
393393
self.create_only_patterns = [
394394
["PORT", "*", "lanes"],
395395
["LOOPBACK_INTERFACE", "*", "vrf_name"],
396+
["BGP_NEIGHBOR", "*", "holdtime"],
397+
["BGP_NEIGHBOR", "*", "keepalive"],
398+
["BGP_NEIGHBOR", "*", "name"],
399+
["BGP_NEIGHBOR", "*", "asn"],
400+
["BGP_NEIGHBOR", "*", "local_addr"],
401+
["BGP_NEIGHBOR", "*", "nhopself"],
402+
["BGP_NEIGHBOR", "*", "rrclient"],
396403
]
397404

398405
def validate(self, move, diff):
399406
simulated_config = move.apply(diff.current_config)
400-
paths = set(list(self._get_create_only_paths(diff.current_config)) + list(self._get_create_only_paths(simulated_config)))
407+
# get create-only paths from current config, simulated config and also target config
408+
# simulated config is the result of the move
409+
# target config is the final config
410+
paths = set(list(self._get_create_only_paths(diff.current_config)) +
411+
list(self._get_create_only_paths(simulated_config)) +
412+
list(self._get_create_only_paths(diff.target_config)))
401413

402414
for path in paths:
403415
tokens = self.path_addressing.get_path_tokens(path)
@@ -408,8 +420,28 @@ def validate(self, move, diff):
408420
if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config):
409421
return False
410422

423+
# if parent of create-only field is added, create-only field should be the same as target
424+
# i.e. if field is deleted in target, it should be deleted in the move, or
425+
# if field is present in target, it should be present in the move
426+
if self._parent_added_child_not_as_target(tokens, diff.current_config, simulated_config, diff.target_config):
427+
return False
428+
411429
return True
412430

431+
def _parent_added_child_not_as_target(self, tokens, current_config, simulated_config, target_config):
432+
# if parent is not added, return false
433+
if not self._exist_only_in_first(tokens[:-1], simulated_config, current_config):
434+
return False
435+
436+
child_path = self.path_addressing.create_path(tokens)
437+
438+
# if child is in target, check if child is not in simulated
439+
if self.path_addressing.has_path(target_config, child_path):
440+
return not self.path_addressing.has_path(simulated_config, child_path)
441+
else:
442+
# if child is not in target, check if child is in simulated
443+
return self.path_addressing.has_path(simulated_config, child_path)
444+
413445
def _get_create_only_paths(self, config):
414446
for pattern in self.create_only_patterns:
415447
for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0):
@@ -472,20 +504,9 @@ def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated
472504
return True
473505

474506
def _exist_only_in_first(self, tokens, first_config_ptr, second_config_ptr):
475-
for token in tokens:
476-
mod_token = int(token) if isinstance(first_config_ptr, list) else token
477-
478-
if mod_token not in second_config_ptr:
479-
return True
480-
481-
if mod_token not in first_config_ptr:
482-
return False
483-
484-
first_config_ptr = first_config_ptr[mod_token]
485-
second_config_ptr = second_config_ptr[mod_token]
486-
487-
# tokens exist in both
488-
return False
507+
path = self.path_addressing.create_path(tokens)
508+
return self.path_addressing.has_path(first_config_ptr, path) and \
509+
not self.path_addressing.has_path(second_config_ptr, path)
489510

490511
class NoDependencyMoveValidator:
491512
"""

‎tests/generic_config_updater/files/patch_sorter_test_success.json‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2894,5 +2894,85 @@
28942894
}
28952895
]
28962896
]
2897+
},
2898+
"ADDING_BGP_NEIGHBORS": {
2899+
"current_config": {
2900+
"BGP_NEIGHBOR": {
2901+
"10.0.0.57": {
2902+
"admin_status": "up",
2903+
"asn": "64600",
2904+
"holdtime": "10",
2905+
"keepalive": "3",
2906+
"local_addr": "10.0.0.56",
2907+
"name": "ARISTA01T1",
2908+
"nhopself": "0",
2909+
"rrclient": "0"
2910+
}
2911+
}
2912+
},
2913+
"patch": [
2914+
{
2915+
"op": "add",
2916+
"path": "/BGP_NEIGHBOR/10.0.0.59",
2917+
"value": {
2918+
"admin_status": "up",
2919+
"asn": "64600",
2920+
"holdtime": "10",
2921+
"keepalive": "3",
2922+
"local_addr": "10.0.0.58",
2923+
"name": "ARISTA02T1",
2924+
"nhopself": "0",
2925+
"rrclient": "0"
2926+
}
2927+
},
2928+
{
2929+
"op": "add",
2930+
"path": "/BGP_NEIGHBOR/10.0.0.61",
2931+
"value": {
2932+
"admin_status": "up",
2933+
"asn": "64600",
2934+
"holdtime": "10",
2935+
"keepalive": "3",
2936+
"local_addr": "10.0.0.60",
2937+
"name": "ARISTA03T1",
2938+
"nhopself": "0",
2939+
"rrclient": "0"
2940+
}
2941+
}
2942+
],
2943+
"expected_changes": [
2944+
[
2945+
{
2946+
"op": "add",
2947+
"path": "/BGP_NEIGHBOR/10.0.0.59",
2948+
"value": {
2949+
"admin_status": "up",
2950+
"asn": "64600",
2951+
"holdtime": "10",
2952+
"keepalive": "3",
2953+
"local_addr": "10.0.0.58",
2954+
"name": "ARISTA02T1",
2955+
"nhopself": "0",
2956+
"rrclient": "0"
2957+
}
2958+
}
2959+
],
2960+
[
2961+
{
2962+
"op": "add",
2963+
"path": "/BGP_NEIGHBOR/10.0.0.61",
2964+
"value": {
2965+
"admin_status": "up",
2966+
"asn": "64600",
2967+
"holdtime": "10",
2968+
"keepalive": "3",
2969+
"local_addr": "10.0.0.60",
2970+
"name": "ARISTA03T1",
2971+
"nhopself": "0",
2972+
"rrclient": "0"
2973+
}
2974+
}
2975+
]
2976+
]
28972977
}
28982978
}

‎tests/generic_config_updater/patch_sorter_test.py‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,31 @@ def test_validate__removed_create_only_field_parent_doesnot_remain__success(self
884884
["PORT"],
885885
["PORT"])
886886

887+
def test_validate__parent_added_with_all_create_only_field_that_target_has__success(self):
888+
added_parent_value = {
889+
"admin_status": "up",
890+
"asn": "64600", # <== create-only field
891+
"holdtime": "50", # <== create-only field
892+
}
893+
self.verify_parent_adding(added_parent_value, True)
894+
895+
def test_validate__parent_added_with_create_only_field_but_target_does_not_have_the_field__failure(self):
896+
added_parent_value = {
897+
"admin_status": "up",
898+
"asn": "64600", # <== create-only field
899+
"holdtime": "50", # <== create-only field
900+
"rrclient": "1", # <== create-only field but not in target-config
901+
}
902+
self.verify_parent_adding(added_parent_value, False)
903+
904+
def test_validate__parent_added_without_create_only_field_but_target_have_the_field__failure(self):
905+
added_parent_value = {
906+
"admin_status": "up",
907+
"asn": "64600", # <== create-only field
908+
# Not adding: "holdtime": "50"
909+
}
910+
self.verify_parent_adding(added_parent_value, False)
911+
887912
def test_hard_coded_create_only_paths(self):
888913
config = {
889914
"PORT": {
@@ -895,17 +920,59 @@ def test_hard_coded_create_only_paths(self):
895920
"Loopback0":{"vrf_name":"vrf0"},
896921
"Loopback1":{},
897922
"Loopback2":{"vrf_name":"vrf1"},
898-
}}
923+
},
924+
"BGP_NEIGHBOR": {
925+
"10.0.0.57": {
926+
"admin_status": "up",
927+
"asn": "64600",
928+
"holdtime": "10",
929+
"keepalive": "3",
930+
"local_addr": "10.0.0.56",
931+
"name": "ARISTA01T1",
932+
"nhopself": "0",
933+
"rrclient": "0"
934+
}
935+
}
936+
}
899937
expected = [
900938
"/PORT/Ethernet0/lanes",
901939
"/PORT/Ethernet2/lanes",
902940
"/LOOPBACK_INTERFACE/Loopback0/vrf_name",
903-
"/LOOPBACK_INTERFACE/Loopback2/vrf_name"
941+
"/LOOPBACK_INTERFACE/Loopback2/vrf_name",
942+
"/BGP_NEIGHBOR/10.0.0.57/asn",
943+
"/BGP_NEIGHBOR/10.0.0.57/holdtime",
944+
"/BGP_NEIGHBOR/10.0.0.57/keepalive",
945+
"/BGP_NEIGHBOR/10.0.0.57/local_addr",
946+
"/BGP_NEIGHBOR/10.0.0.57/name",
947+
"/BGP_NEIGHBOR/10.0.0.57/nhopself",
948+
"/BGP_NEIGHBOR/10.0.0.57/rrclient",
904949
]
950+
905951
actual = self.validator._get_create_only_paths(config)
906952

907953
self.assertCountEqual(expected, actual)
908954

955+
def verify_parent_adding(self, added_parent_value, expected):
956+
current_config = {
957+
"BGP_NEIGHBOR": {}
958+
}
959+
960+
target_config = {
961+
"BGP_NEIGHBOR": {
962+
"10.0.0.57": {
963+
"admin_status": "up",
964+
"asn": "64600",
965+
"holdtime": "50",
966+
}
967+
}
968+
}
969+
diff = ps.Diff(current_config, target_config)
970+
move = ps.JsonMove.from_operation({"op":"add", "path":"/BGP_NEIGHBOR/10.0.0.57", "value": added_parent_value})
971+
972+
actual = self.validator.validate(move, diff)
973+
974+
self.assertEqual(expected, actual)
975+
909976
def verify_diff(self, current_config, target_config, current_config_tokens=None, target_config_tokens=None, expected=True):
910977
# Arrange
911978
current_config_tokens = current_config_tokens if current_config_tokens else []

0 commit comments

Comments
 (0)