Skip to content

Commit

Permalink
[GCU] Mark children of bgp_neighbor as create-only (sonic-net#2008)
Browse files Browse the repository at this point in the history 
#### What I did

Fixes sonic-net#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)
  • Loading branch information
@ghooo
ghooo authored Jan 21, 2022
1 parent ad1ed4e commit 01dfb9c
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 17 deletions.
51 changes: 36 additions & 15 deletions generic_config_updater/patch_sorter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,23 @@ def __init__(self, path_addressing):
self.create_only_patterns = [
["PORT", "*", "lanes"],
["LOOPBACK_INTERFACE", "*", "vrf_name"],
["BGP_NEIGHBOR", "*", "holdtime"],
["BGP_NEIGHBOR", "*", "keepalive"],
["BGP_NEIGHBOR", "*", "name"],
["BGP_NEIGHBOR", "*", "asn"],
["BGP_NEIGHBOR", "*", "local_addr"],
["BGP_NEIGHBOR", "*", "nhopself"],
["BGP_NEIGHBOR", "*", "rrclient"],
]

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)))
# get create-only paths from current config, simulated config and also target config
# simulated config is the result of the move
# target config is the final config
paths = set(list(self._get_create_only_paths(diff.current_config)) +
list(self._get_create_only_paths(simulated_config)) +
list(self._get_create_only_paths(diff.target_config)))

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

# if parent of create-only field is added, create-only field should be the same as target
# i.e. if field is deleted in target, it should be deleted in the move, or
# if field is present in target, it should be present in the move
if self._parent_added_child_not_as_target(tokens, diff.current_config, simulated_config, diff.target_config):
return False

return True

def _parent_added_child_not_as_target(self, tokens, current_config, simulated_config, target_config):
# if parent is not added, return false
if not self._exist_only_in_first(tokens[:-1], simulated_config, current_config):
return False

child_path = self.path_addressing.create_path(tokens)

# if child is in target, check if child is not in simulated
if self.path_addressing.has_path(target_config, child_path):
return not self.path_addressing.has_path(simulated_config, child_path)
else:
# if child is not in target, check if child is in simulated
return self.path_addressing.has_path(simulated_config, child_path)

def _get_create_only_paths(self, config):
for pattern in self.create_only_patterns:
for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0):
Expand Down Expand Up @@ -472,20 +504,9 @@ def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated
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
path = self.path_addressing.create_path(tokens)
return self.path_addressing.has_path(first_config_ptr, path) and \
not self.path_addressing.has_path(second_config_ptr, path)

class NoDependencyMoveValidator:
"""
Expand Down
80 changes: 80 additions & 0 deletions tests/generic_config_updater/files/patch_sorter_test_success.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2894,5 +2894,85 @@
}
]
]
},
"ADDING_BGP_NEIGHBORS": {
"current_config": {
"BGP_NEIGHBOR": {
"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"
}
}
},
"patch": [
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.59",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.58",
"name": "ARISTA02T1",
"nhopself": "0",
"rrclient": "0"
}
},
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.61",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.60",
"name": "ARISTA03T1",
"nhopself": "0",
"rrclient": "0"
}
}
],
"expected_changes": [
[
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.59",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.58",
"name": "ARISTA02T1",
"nhopself": "0",
"rrclient": "0"
}
}
],
[
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.61",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.60",
"name": "ARISTA03T1",
"nhopself": "0",
"rrclient": "0"
}
}
]
]
}
}
71 changes: 69 additions & 2 deletions tests/generic_config_updater/patch_sorter_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,31 @@ def test_validate__removed_create_only_field_parent_doesnot_remain__success(self
["PORT"],
["PORT"])

def test_validate__parent_added_with_all_create_only_field_that_target_has__success(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
"holdtime": "50", # <== create-only field
}
self.verify_parent_adding(added_parent_value, True)

def test_validate__parent_added_with_create_only_field_but_target_does_not_have_the_field__failure(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
"holdtime": "50", # <== create-only field
"rrclient": "1", # <== create-only field but not in target-config
}
self.verify_parent_adding(added_parent_value, False)

def test_validate__parent_added_without_create_only_field_but_target_have_the_field__failure(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
# Not adding: "holdtime": "50"
}
self.verify_parent_adding(added_parent_value, False)

def test_hard_coded_create_only_paths(self):
config = {
"PORT": {
Expand All @@ -895,17 +920,59 @@ def test_hard_coded_create_only_paths(self):
"Loopback0":{"vrf_name":"vrf0"},
"Loopback1":{},
"Loopback2":{"vrf_name":"vrf1"},
}}
},
"BGP_NEIGHBOR": {
"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"
}
}
}
expected = [
"/PORT/Ethernet0/lanes",
"/PORT/Ethernet2/lanes",
"/LOOPBACK_INTERFACE/Loopback0/vrf_name",
"/LOOPBACK_INTERFACE/Loopback2/vrf_name"
"/LOOPBACK_INTERFACE/Loopback2/vrf_name",
"/BGP_NEIGHBOR/10.0.0.57/asn",
"/BGP_NEIGHBOR/10.0.0.57/holdtime",
"/BGP_NEIGHBOR/10.0.0.57/keepalive",
"/BGP_NEIGHBOR/10.0.0.57/local_addr",
"/BGP_NEIGHBOR/10.0.0.57/name",
"/BGP_NEIGHBOR/10.0.0.57/nhopself",
"/BGP_NEIGHBOR/10.0.0.57/rrclient",
]

actual = self.validator._get_create_only_paths(config)

self.assertCountEqual(expected, actual)

def verify_parent_adding(self, added_parent_value, expected):
current_config = {
"BGP_NEIGHBOR": {}
}

target_config = {
"BGP_NEIGHBOR": {
"10.0.0.57": {
"admin_status": "up",
"asn": "64600",
"holdtime": "50",
}
}
}
diff = ps.Diff(current_config, target_config)
move = ps.JsonMove.from_operation({"op":"add", "path":"/BGP_NEIGHBOR/10.0.0.57", "value": added_parent_value})

actual = self.validator.validate(move, diff)

self.assertEqual(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 []
Expand Down

0 comments on commit 01dfb9c

Please sign in to comment.