Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCU] Mark children of bgp_neighbor as create-only #2008

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented Jan 14, 2022

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:

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)

@ghooo ghooo force-pushed the dev/mghoneim/create_only_bgp_neighbor branch 6 times, most recently from 64b132e to bae44d9 Compare January 14, 2022 23:59
@ghooo ghooo force-pushed the dev/mghoneim/create_only_bgp_neighbor branch from bae44d9 to 38eb8e3 Compare January 17, 2022 09:51
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Jan 19, 2022

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.

I guess you get the conclusion for this code https://github.com/Azure/sonic-buildimage/blob/bb3362760d01055d1c30cbda8ec3bf5ea4f5d7aa/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py#L233. Could yo clarify?


In reply to: 1016164771

["BGP_NEIGHBOR", "*", "asn"],
["BGP_NEIGHBOR", "*", "local_addr"],
["BGP_NEIGHBOR", "*", "nhopself"],
["BGP_NEIGHBOR", "*", "rrclient"],
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BGP_NEIGHBOR

Also applied to BGP_INTERNAL_NEIGHBOR, BGP_MONITORS, BGP_PEER_RANGE, BGP_VOQ_CHASSIS_NEIGHBOR ? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I do this in a separate PR? I need to close the current issue.

#2029

@ghooo
Copy link
Contributor Author

ghooo commented Jan 21, 2022

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.

I guess you get the conclusion for this code https://github.com/Azure/sonic-buildimage/blob/bb3362760d01055d1c30cbda8ec3bf5ea4f5d7aa/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py#L233. Could yo clarify?

Great question, I did manual test to see if changing the attribute value directly would work. I added that to the PR description, please take a look. @qiluo-msft

@ghooo ghooo merged commit 01dfb9c into sonic-net:master Jan 21, 2022
judyjoseph pushed a commit that referenced this pull request Jan 31, 2022
#### 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generic_config_updater: Add Rack: Issues
3 participants