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

frr-mgmt-framework syntax for PREFIX_SET is impossible to use in a route-map #6943

Closed
jhujhiti opened this issue Mar 2, 2021 · 11 comments · Fixed by #8454
Closed

frr-mgmt-framework syntax for PREFIX_SET is impossible to use in a route-map #6943

jhujhiti opened this issue Mar 2, 2021 · 11 comments · Fixed by #8454

Comments

@jhujhiti
Copy link

jhujhiti commented Mar 2, 2021

Description

The templates that write FRR configuration for route-maps and prefix-lists assume opposite and incompatible things about the mode field of each PREFIX_SET.

bgpd.conf.db.pref_list.j2 requires uppercase (eg., IPv4) to match: https://github.com/Azure/sonic-buildimage/blob/a171e6c5e4e51a360a70190903e09decb44ec7e3/src/sonic-frr-mgmt-framework/templates/bgpd/bgpd.conf.db.pref_list.j2#L5
bgpd.conf.db.route_map.j2 requires lowercase (eg., ipv4) to match: https://github.com/Azure/sonic-buildimage/blob/a171e6c5e4e51a360a70190903e09decb44ec7e3/src/sonic-frr-mgmt-framework/templates/bgpd/bgpd.conf.db.route_map.j2#L28

This makes it impossible to define a prefix-list and use it in a route-map.

The latest documentation in sonic-net/SONiC#544 (not sure why this isn't merged) is also wrong. The document says that the database keys are IP_PREFIX_SET and IP_PREFIX, but the templates actually match PREFIX_SET and PREFIX.

Steps to reproduce the issue:

Snippet of /etc/sonic/config_db.json necessary to reproduce:

{
    "DEVICE_METADATA": {
        "localhost": {
            "frr_mgmt_framework_config": "true"
        }
    },
    "ROUTE_MAP": {
        "LOOPBACK|10": {
            "route_operation": "permit",
            "match_prefix_set": "slash32",
            "set_origin": "igp"
        }
    },
    "PREFIX_SET": {
            "slash32": {
                    "mode": "ipv4"
            }
    },
    "PREFIX": {
            "slash32|0.0.0.0/0|32..32": {
                    "action": "permit"
            }
    }
}

Describe the results you received:

The snippet above produces the FRR configuration

route-map LOOPBACK permit 10
 match ip address prefix-list slash32
 set origin igp
!
route-map LOOPBACK deny 10000

with no prefix-list defined. Changing the mode to IPV4 instead results in

ip prefix-list slash32 seq 5 permit 0.0.0.0/0 ge 32 le 32
!
route-map LOOPBACK permit 10
 set origin igp
!
route-map LOOPBACK deny 10000

with no match conditions at all.

Describe the results you expected:

Other parts of the template are case-insensitive, so I'd expect either upper- or lower- case modes to result in

ip prefix-list slash32 seq 5 permit 0.0.0.0/0 ge 32 le 32
!
route-map LOOPBACK permit 10
 match ip address prefix-list slash32
 set origin igp
!
route-map LOOPBACK deny 10000

Output of show version:

Built from master a few days ago: c617825. Confirmed that the bug still exists on master now.

SONiC Software Version: SONiC.master.0-dirty-20210225.203231
Distribution: Debian 10.8
Kernel: 4.19.0-12-2-amd64
Build commit: c6178259
Build date: Thu Feb 25 22:08:55 UTC 2021
Built by: admin@sonic-builder

Platform: x86_64-<irrelevant>
HwSKU: <irrelevant>
ASIC: broadcom
ASIC Count: 1
Serial Number: <irrelevant>
Uptime: 23:09:49 up 23:58,  1 user,  load average: 1.48, 1.22, 1.07

Docker images:
REPOSITORY                    TAG                              IMAGE ID            SIZE
docker-syncd-brcm             latest                           1d03dbb9a55c        680MB
docker-syncd-brcm             master.0-dirty-20210225.203231   1d03dbb9a55c        680MB
docker-snmp                   latest                           508289a491d3        438MB
docker-snmp                   master.0-dirty-20210225.203231   508289a491d3        438MB
docker-teamd                  latest                           dd92523b5fcb        408MB
docker-teamd                  master.0-dirty-20210225.203231   dd92523b5fcb        408MB
docker-nat                    latest                           74f76f1f04b7        411MB
docker-nat                    master.0-dirty-20210225.203231   74f76f1f04b7        411MB
docker-router-advertiser      latest                           eebab6662190        398MB
docker-router-advertiser      master.0-dirty-20210225.203231   eebab6662190        398MB
docker-platform-monitor       latest                           66c7a5e1c931        605MB
docker-platform-monitor       master.0-dirty-20210225.203231   66c7a5e1c931        605MB
docker-macsec                 latest                           0f67d436bbfb        411MB
docker-macsec                 master.0-dirty-20210225.203231   0f67d436bbfb        411MB
docker-lldp                   latest                           c1b744a26c8c        438MB
docker-lldp                   master.0-dirty-20210225.203231   c1b744a26c8c        438MB
docker-database               latest                           5bccc7b5c3b9        397MB
docker-database               master.0-dirty-20210225.203231   5bccc7b5c3b9        397MB
docker-orchagent              latest                           704bb0e1843e        427MB
docker-orchagent              master.0-dirty-20210225.203231   704bb0e1843e        427MB
docker-dhcp-relay             latest                           7033e89cb01f        404MB
docker-dhcp-relay             master.0-dirty-20210225.203231   7033e89cb01f        404MB
docker-sonic-telemetry        latest                           04f550b9a1f1        487MB
docker-sonic-telemetry        master.0-dirty-20210225.203231   04f550b9a1f1        487MB
docker-sonic-mgmt-framework   latest                           66a24bffabf4        616MB
docker-sonic-mgmt-framework   master.0-dirty-20210225.203231   66a24bffabf4        616MB
docker-fpm-frr                latest                           d71c5fa70100        426MB
docker-fpm-frr                master.0-dirty-20210225.203231   d71c5fa70100        426MB
docker-sflow                  latest                           359186926960        409MB
docker-sflow                  master.0-dirty-20210225.203231   359186926960        409MB
@anshuv-mfst
Copy link

@venkatmahalingam - could you please look into the issue, thanks.

@venkatmahalingam
Copy link
Collaborator

venkatmahalingam commented Mar 3, 2021

@jhujhiti HLD is pending to merge because of the BGP/route-map schema dependent YANG models are in review, will fix the table names.

We should use IPv4 & IPv6 values, will create the PR with the fixes, meanwhile, please use the following fixes.

Please change ipv4 -> IPv4 and ipv6 -> IPv6

sonic-buildimage/src/sonic-frr-mgmt-framework/templates/bgpd/bgpd.conf.db.route_map.j2
Line 28 in a171e6c
{% set ip_str = {'ipv4':'ip', 'ipv6':'ipv6' } %}

Also, use mode as "IPv4" instead of "ipv4"

127.0.0.1:6379[4]> hgetall PREFIX_SET|slash32

  1. "mode"
  2. "IPv4"

@jhujhiti
Copy link
Author

jhujhiti commented Mar 3, 2021

Since the HLD is still in a PR, the roadmap link ought to at least be updated. https://github.com/Azure/SONiC/wiki/Sonic-Roadmap-Planning links to an outdated document which, among other things, doesn't mention the need to enable the entire feature with the frr_mgmt_framework_config flag.

@venkatmahalingam
Copy link
Collaborator

HLD is expected to be merged before the next release, will update the HLD soon.

@venkatmahalingam
Copy link
Collaborator

venkatmahalingam commented Apr 21, 2021

The HLD has merged already, @jhujhiti hope you got a chance to verify this issue with the suggested change, please confirm.

@jhujhiti
Copy link
Author

Oh -- yes the template change works. I'd already applied that when I opened the issue.

I see that the HLD is on master, but https://github.com/Azure/SONiC/wiki/Sonic-Roadmap-Planning still links to an outdated version of the HLD.

@venkatmahalingam
Copy link
Collaborator

Oh -- yes the template change works. I'd already applied that when I opened the issue.

I see that the HLD is on master, but https://github.com/Azure/SONiC/wiki/Sonic-Roadmap-Planning still links to an outdated version of the HLD.

Sure, we will fix to point to https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_Design_Doc_Unified_FRR_Mgmt_Interface.md instead of a specific commit.

@bluecmd
Copy link
Contributor

bluecmd commented Aug 12, 2021

+1. I hit this issue with 202012 today. I was trying to understand how the templates were created to allow me to reverse engineer how to write the PREFIX_SET with a ROUTE_MAP - glad I found this issue before spending too much time on it.

Do you have an estimate when this can be fixed and backported to 202012?

I created #8454 to try to help along.

bluecmd added a commit to bluecmd/sonic-buildimage that referenced this issue Aug 12, 2021
This change makes the template match PREFIX_SET and the documentation.

Fixes sonic-net#6943.

Signed-off-by: Christian Svensson <[email protected]>
lguohan pushed a commit that referenced this issue Aug 13, 2021
Fix #6943

This change makes the template match PREFIX_SET and the documentation.

Signed-off-by: Christian Svensson <[email protected]>
@bluecmd
Copy link
Contributor

bluecmd commented Aug 13, 2021

I have verified the fix with the following configuration snippet:

{
	"ROUTE_MAP": {
		"ALLOW-OUT|1": {
			"route_operation": "permit",
			"match_prefix_set": "LOCAL-NETS"
		}
	},
	"PREFIX_SET": {
		"LOCAL-NETS": {
			"mode": "IPv6"
		}
	},
	"PREFIX": {
		"LOCAL-NETS|2a10:11c0::/64|exact": {}
	}
}

It does not seem to work with online configuration through frrcfgd but running systemctl restart bgp causes the objects to show up, which is good enough for me for now.

The only part that did show up during "online" apply was this:

route-map ALLOW-OUT permit 1
 match ipv6 address prefix-list LOCAL-NETS

@venkatmahalingam
Copy link
Collaborator

It does not seem to work with online configuration through frrcfgd

Did you use "config load with above configs" or what method did you use for loading the config? please share the steps.

@bluecmd
Copy link
Contributor

bluecmd commented Aug 13, 2021

Yep! I used config load bgp.json where bgp.json contained the snippet I shared.

lguohan pushed a commit that referenced this issue Aug 15, 2021
Fix #6943

This change makes the template match PREFIX_SET and the documentation.

Signed-off-by: Christian Svensson <[email protected]>
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.

4 participants