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 or bgpcfgd]bgp prefix updating only fail for prefixes_v4 removal when GCU apply-patch #10141

Closed
wen587 opened this issue Mar 3, 2022 · 2 comments · Fixed by sonic-net/sonic-mgmt#5268
Assignees

Comments

@wen587
Copy link
Contributor

wen587 commented Mar 3, 2022

Description

Test based on BGP_ALLOWED_PREFIX yang #10142
bgp prefix removal only fails on prefixes_v4 when GCU apply-patch to remove.
Not sure it is a bug from bgpcfgd or GCU.

Steps to reproduce the issue:

  1. Load bgp prefix config and check it takes into effect
admin@vlab-01:~/hijack$ cat add.load
{
    "BGP_ALLOWED_PREFIXES": {
        "DEPLOYMENT_ID|0": {
            "prefixes_v4": [
                "10.20.0.0/16",
                "10.30.0.0/16"
            ],
            "prefixes_v6": [
                "fc02:20::/64",
                "fc02:30::/64"
            ]
        }
    }
}

admin@vlab-01:~/hijack$ sudo config load -y add.load
Running command: /usr/local/bin/sonic-cfggen -j add.load --write-to-db

admin@vlab-01:~/hijack$ show run all | grep BGP_ALL -A11
    "BGP_ALLOWED_PREFIXES": {
        "DEPLOYMENT_ID|0": {
            "prefixes_v4": [
                "10.20.0.0/16",
                "20.20.0.0/16"
            ],
            "prefixes_v6": [
                "fc02:20::/64",
                "fc02:30::/64"
            ]
        }
    },

admin@vlab-01:~/hijack$ show run bgp | grep prefix
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 10 deny 0.0.0.0/0 le 17
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 20 permit 127.0.0.1/32
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 30 permit 10.20.0.0/16 le 32  <======
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 40 permit 20.20.0.0/16 le 32  <======
ip prefix-list PL_LoopbackV4 seq 5 permit 10.1.0.32/32
ip prefix-list LOCAL_VLAN_IPV4_PREFIX seq 5 permit 192.168.0.0/21
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 10 deny ::/0 le 59
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 20 deny ::/0 ge 65
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 30 permit fc02:20::/64 le 128 <=====
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 40 permit fc02:30::/64 le 128 <=====
ipv6 prefix-list PL_LoopbackV6 seq 5 permit fc00:1::/64
  1. GCU apply patch to remove one prefix value. Process completed. But previous prefix v4 config is still there.
admin@vlab-01:~/hijack$ cat remove_2nd.json
[
 {
  "op": "remove",
  "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v6/1"
 },
 {
  "op": "remove",
  "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/1"
 }
]

admin@vlab-01:~/hijack$ sudo config apply-patch remove_2nd.json
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v6/1"}, {"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/1"}]
Patch Applier: Applying 2 changes in order:
Patch Applier:   * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/1"}]
Patch Applier:   * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v6/1"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.

admin@vlab-01:~/hijack$  sonic-db-cli CONFIG_DB hgetall "BGP_ALLOWED_PREFIXES|DEPLOYMENT_ID|0"
{'prefixes_v4@': '10.20.0.0/16', 'prefixes_v6@': 'fc02:20::/64'}
admin@vlab-01:~/hijack$ show run all | grep BGP_ALLOW -A8
    "BGP_ALLOWED_PREFIXES": {
        "DEPLOYMENT_ID|0": {
            "prefixes_v4": [
                "10.20.0.0/16"
            ],
            "prefixes_v6": [
                "fc02:20::/64"
            ]
        }

admin@vlab-01:~/hijack$ show run bgp | grep prefix
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 10 deny 0.0.0.0/0 le 17
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 20 permit 127.0.0.1/32
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 30 permit 10.20.0.0/16 le 32   <===== correct
ip prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V4 seq 40 permit 20.20.0.0/16 le 32   <===== WRONG
ip prefix-list PL_LoopbackV4 seq 5 permit 10.1.0.32/32
ip prefix-list LOCAL_VLAN_IPV4_PREFIX seq 5 permit 192.168.0.0/21
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 10 deny ::/0 le 59
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 20 deny ::/0 ge 65
ipv6 prefix-list PL_ALLOW_LIST_DEPLOYMENT_ID_0_COMMUNITY_empty_V6 seq 30 permit fc02:20::/64 le 128  <===== correct
ipv6 prefix-list PL_LoopbackV6 seq 5 permit fc00:1::/64

  1. Syslog checking. I don't understand why only v4 fails to remove previous prefix.
Mar  3 05:55:31.211407 vlab-01 NOTICE config: Patch Applier: Applying 2 changes in order:
Mar  3 05:55:31.211574 vlab-01 NOTICE config: Patch Applier:   * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/1"}]
Mar  3 05:55:31.594294 vlab-01 DEBUG bgp#bgpcfgd: Received message : '('DEPLOYMENT_ID|0', 'SET', (('prefixes_v4', '10.20.0.0/16'), ('prefixes_v6', 'fc02:20::/64,fc02:30::/64')))'
Mar  3 05:55:31.595914 vlab-01 INFO bgp#bgpcfgd: BGPAllowListMgr::Updating 'Allow list' policy. deployment_id '0'. community: 'empty' prefix_v4 '['10.20.0.0/16']'. prefix_v6: '['fc02:20::/64', 'fc02:30::/64']'
...
Mar  3 05:55:31.863180 vlab-01 DEBUG bgp#bgpcfgd: BGPAllowListMgr::__update_policy. The peers configuration scheduled for updates
Mar  3 05:55:31.864233 vlab-01 INFO bgp#bgpcfgd: BGPAllowListMgr::Done
Mar  3 05:55:31.866064 vlab-01 DEBUG bgp#bgpcfgd: execute command '['vtysh', '-f', '/tmp/tmpkjuzoy7x']'.
Mar  3 05:55:31.946244 vlab-01 NOTICE config: Patch Applier:   * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v6/1"}]
Mar  3 05:55:32.002791 vlab-01 DEBUG bgp#bgpcfgd: execute command '['vtysh', '-c', 'clear bgp peer-group PEER_V4 soft in']'.
Mar  3 05:55:32.165825 vlab-01 DEBUG bgp#bgpcfgd: execute command '['vtysh', '-c', 'clear bgp peer-group PEER_V6 soft in']'.
Mar  3 05:55:32.349959 vlab-01 DEBUG bgp#bgpcfgd: Received message : '('DEPLOYMENT_ID|0', 'SET', (('prefixes_v4', '10.20.0.0/16'), ('prefixes_v6', 'fc02:20::/64')))'
Mar  3 05:55:32.351224 vlab-01 INFO bgp#bgpcfgd: BGPAllowListMgr::Updating 'Allow list' policy. deployment_id '0'. community: 'empty' prefix_v4 '['10.20.0.0/16']'. prefix_v6: '['fc02:20::/64']'

Describe the results you received:

prefixes_v4 GCU removal complete. But previous config is still there.

Describe the results you expected:

Desired config removal should take effect.

Output of show version:

admin@vlab-01:~/hijack$ show version

SONiC Software Version: SONiC.master.59050-dirty-20211214.171137
Distribution: Debian 11.1
Kernel: 5.10.0-8-2-amd64
Build commit: 2a467f8eb
Build date: Tue Dec 14 17:18:24 UTC 2021
Built by: AzDevOps@sonic-build-workers-000ZFY

Platform: x86_64-kvm_x86_64-r0
HwSKU: Force10-S6000
ASIC: vs
ASIC Count: 1
Serial Number: N/A
Model Number: N/A
Hardware Revision: N/A
Uptime: 05:41:32 up 23 days, 20:21,  2 users,  load average: 0.28, 0.30, 0.28

Output of show techsupport:

(paste your output here or download and attach the file here )

Additional information you deem important (e.g. issue happens only occasionally):

@StormLiangMS StormLiangMS self-assigned this Mar 7, 2022
StormLiangMS added a commit that referenced this issue Mar 10, 2022
…fix list (#10165)

* fix allow list issue

Signed-off-by: stormliang <[email protected]>

* add the ipaddress in the install list

* add unit test

Co-authored-by: Ubuntu <azureuser@SONIC-SH-STORM-02.5pu3m0fajw1edcfltykk1gauxa.gx.internal.cloudapp.net>

Why I did it
Failed to remove part of configuration of bgp allowed prefix list. The details in #10141

How I did it
There are two issues:

In FRR, ipv6 default route is ::/0, but in the configuration, it is 0::/0, string comparison would be false, but why ipv4 failed to remove the allowed prefix list, ipv6 works? Looks into next one for the answer.

The current managers_allow_list doesn’t support removal part of the prefix list. But why IPv6 works in 1? It is because the bug for the IPv6 default route comparison, it would do the update no matter what is the operation (the code will compare the prefix list in the FRR and configuration db, if all configurations in db are presented in FRR, it do nothing, otherwise it will update the prefix list based on the configuration from db).

How to verify it
Follow the step in #10141
@qiluo-msft
Copy link
Collaborator

Is this issue fixed by a PR?

@wen587
Copy link
Contributor Author

wen587 commented Mar 14, 2022

Is this issue fixed by a PR?

Yes.

@wen587 wen587 closed this as completed Mar 14, 2022
abdosi pushed a commit that referenced this issue May 19, 2022
…fix list (#10165)

* fix allow list issue

Signed-off-by: stormliang <[email protected]>

* add the ipaddress in the install list

* add unit test

Co-authored-by: Ubuntu <azureuser@SONIC-SH-STORM-02.5pu3m0fajw1edcfltykk1gauxa.gx.internal.cloudapp.net>

Why I did it
Failed to remove part of configuration of bgp allowed prefix list. The details in #10141

How I did it
There are two issues:

In FRR, ipv6 default route is ::/0, but in the configuration, it is 0::/0, string comparison would be false, but why ipv4 failed to remove the allowed prefix list, ipv6 works? Looks into next one for the answer.

The current managers_allow_list doesn’t support removal part of the prefix list. But why IPv6 works in 1? It is because the bug for the IPv6 default route comparison, it would do the update no matter what is the operation (the code will compare the prefix list in the FRR and configuration db, if all configurations in db are presented in FRR, it do nothing, otherwise it will update the prefix list based on the configuration from db).

How to verify it
Follow the step in #10141
StormLiangMS added a commit to StormLiangMS/sonic-buildimage that referenced this issue May 19, 2022
…fix list (sonic-net#10165)

* fix allow list issue

Signed-off-by: stormliang <[email protected]>

* add the ipaddress in the install list

* add unit test

Co-authored-by: Ubuntu <azureuser@SONIC-SH-STORM-02.5pu3m0fajw1edcfltykk1gauxa.gx.internal.cloudapp.net>

Why I did it
Failed to remove part of configuration of bgp allowed prefix list. The details in sonic-net#10141

How I did it
There are two issues:

In FRR, ipv6 default route is ::/0, but in the configuration, it is 0::/0, string comparison would be false, but why ipv4 failed to remove the allowed prefix list, ipv6 works? Looks into next one for the answer.

The current managers_allow_list doesn’t support removal part of the prefix list. But why IPv6 works in 1? It is because the bug for the IPv6 default route comparison, it would do the update no matter what is the operation (the code will compare the prefix list in the FRR and configuration db, if all configurations in db are presented in FRR, it do nothing, otherwise it will update the prefix list based on the configuration from db).

How to verify it
Follow the step in sonic-net#10141
qiluo-msft pushed a commit that referenced this issue May 24, 2022
…fix list (#10165)

* fix allow list issue

Signed-off-by: stormliang <[email protected]>

* add the ipaddress in the install list

* add unit test

Co-authored-by: Ubuntu <azureuser@SONIC-SH-STORM-02.5pu3m0fajw1edcfltykk1gauxa.gx.internal.cloudapp.net>

Why I did it
Failed to remove part of configuration of bgp allowed prefix list. The details in #10141

How I did it
There are two issues:

In FRR, ipv6 default route is ::/0, but in the configuration, it is 0::/0, string comparison would be false, but why ipv4 failed to remove the allowed prefix list, ipv6 works? Looks into next one for the answer.

The current managers_allow_list doesn’t support removal part of the prefix list. But why IPv6 works in 1? It is because the bug for the IPv6 default route comparison, it would do the update no matter what is the operation (the code will compare the prefix list in the FRR and configuration db, if all configurations in db are presented in FRR, it do nothing, otherwise it will update the prefix list based on the configuration from db).

How to verify it
Follow the step in #10141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants