Skip to content

Commit 38eb8e3

Browse files
ghoooghooo
committed
[GCU] Mark children of bgp_neighbor as create-only
1 parent 5398ee4 commit 38eb8e3

File tree

3 files changed

+185
-17
lines changed

3 files changed

+185
-17
lines changed

‎generic_config_updater/patch_sorter.py

+36-15
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

+80
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

+69-2
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)