Commands added for supporting IPv4 MPLS Layer 3 VPN. #4

KaushikNiral · 2020-04-14T18:43:32Z

New address-family ipv4-vpn added.
New neighbor command added for address-family ipv4-vpn.
New label command added inside address-family ipv4-unicast.
Json script for all the above commands are handled.

deastoe · 2020-04-15T12:29:06Z

yang/vyatta-protocols-frr-bgp-v1.yang

@@ -1291,6 +1352,95 @@ module vyatta-protocols-frr-bgp-v1 {
 						}
 						uses neigh_peer_grp_ipv4_lists;
 					}
+
+          container ipv4-vpn {


This container is under the peer-group configuration rather than the neighbor configuration, but the command translations above are looking for neighbor configuration.

So I'm not sure these diffs are working right now? If you want per-neighbor configuration then I think this should be under the bgp-params-neighbor grouping.

Sorry, actually the container ipv4-vpn is placed under the bgp-params-neighbor grouping but somehow it went in wrong place in this PR.
It is fixed now, will send it for review shortly.
I have attached a screenshot verifying that.

deastoe · 2020-04-15T12:31:07Z

yang/vyatta-protocols-frr-bgp-v1.yang

@@ -68,6 +68,32 @@ module vyatta-protocols-frr-bgp-v1 {
 		description "Inital version.";
 	}

+  typedef rt-rd {


This file uses Tab characters for indentation. Please could you use the same for your additions?

We tried to fix this issue.
We will send for review shortly.

wivory · 2020-04-16T06:54:14Z

yang/vyatta-protocols-frr-bgp-v1.yang

@@ -68,6 +68,32 @@ module vyatta-protocols-frr-bgp-v1 {
 		description "Inital version.";
 	}

+	typedef rt-rd {


Any change to a YANG file requires that you add a new revision statement for the change.

Hi Wivory,
This we have resolved.

You've not tested this have you? Our YANG compiler will refuse to accept this as the revision statements are not in reverse chronological order. The 'pyang' tool will also object.

NB - if you have tested this, and your image boots and is configurable with this YANG, please let me know as it certainly shouldn't be working!

Hi Wivory,

We have tested it but we replaced the old revision statement with new one so we didn't see this issue.
We have added the new revision statement in the correct order and tested.
This changes are also committed.

Thanks,

wivory · 2020-04-16T07:14:57Z

yang/vyatta-protocols-frr-bgp-v1.yang

+						uses peer-grp-neighbor-afi-common-settings {
+							refine filter-list/export {
+								must "not(../../peer-group)" {
+									error-message "You may not configure filter-list export for a neighbor in peer-group\n";


IETF max line length is 73 characters, and in DANOS we generally stick to under 80 characters for YANG files so you don't end up having to scroll sideways to read some lines (which really slows down reviews). You also lose the context of the surrounding lines if they disappear off the left of the screen.

Furthermore, in this case you have a set of multiple changes close enough together that the side-scroll bar is miles down the page, making it incredibly difficult to scroll sideways. I have to page down, scroll sideways, then page back up again.

We understand the issue.
We tried to be consistent with the existing file, so we did it in that manner.

Now we have tried to rectify line length our part of code. Please have a look and suggest.

wivory · 2020-04-16T07:18:22Z

yang/vyatta-protocols-frr-bgp-v1.yang

+								}
+							}
+						}
+						uses neigh_peer_grp_ipv4_lists {


For future reference, please always use hyphens in names, never underscores.

Okay we will follow that.
But here the grouping is already mentioned in this way so we just used in our container ipv4-vpn.

Thanks.

Wasn't asking you to change existing code - that's unnecessary churn. Just making a recommendation for naming in future as a matter of style / consistency.

Okay got it.

Thanks,

dewi-morgan · 2020-04-16T08:51:08Z

Can you please detail the testing you have performed and which topologies ?

Are you testing vrf leaking or bgp mpls l3vpn ? If the later, I don't see the frr label vpn export auto under address-family ipv4-unicast being used, only static label value; is there a reason for not providing auto ?

If you have an l3vpn topology (ce-pe-p-pe-ce), can you provide additional detail e.g. the output of sudo ip route show vrfVRFNAME and sh dataplane route routing-instance VRFNAME for the bgp label stack being used on each PE router ? Also the corresponding ILM table sudo ip -f mpls route show and sh dataplane mpls label-table on each PE router ?

Thank you,

1. New address-family ipv4-vpn added. 2. New neighbor command added for address-family ipv4-vpn. 3. New label command added inside address-family ipv4-unicast. 4. Json script for all the above commands are handled. Signed-off-by: kaushik <[email protected]> Signed-off-by: harios <[email protected]>

1. The container ipv4-vpn added under bgp-params-neighbor grouping. 2. The issue with indentation is resolved. Signed-off-by: harios <[email protected]>

1. Revision added. 2. Rectified indentation. Signed-off-by: harios <[email protected]>

KaushikNiral · 2020-04-16T12:40:58Z

Can you please detail the testing you have performed and which topologies ?

Are you testing vrf leaking or bgp mpls l3vpn ? If the later, I don't see the frr label vpn export auto under address-family ipv4-unicast being used, only static label value; is there a reason for not providing auto ?

If you have an l3vpn topology (ce-pe-p-pe-ce), can you provide additional detail e.g. the output of sudo ip route show vrfVRFNAME and sh dataplane route routing-instance VRFNAME for the bgp label stack being used on each PE router ? Also the corresponding ILM table sudo ip -f mpls route show and sh dataplane mpls label-table on each PE router ?

Thank you,

Hi @dewi-morgan ,

Sure, we will share you the details of our topology.
We have performed the IPv4 BGP MPLS L3VPN test. We are initially trying to do it with static labels so we added label vpn export under address-family ipv4-unicast where label value ranges from <0-1048575>.
In the coming days we will also add label vpn export auto under address-family ipv4-unicast.
We are also working in the vyatta-dataplane for making the IPv4 BGP MPLS L3VPN functional end to end.
FYI we have tested this feature with FRR version 7.3
Please find all the screenshot attached below.

The Topology (ce-pe-p-pe-ce)
R1 and R5 are CE (gns3)
R2 is PE (gns3)
R3 is P (gns3)
cloud 1 is PE ( DANOS )
Display of sudo ip route show vrfVRFNAME
Display of sh dataplane route routing-instance VRFNAME
Display of ILM table sudo ip -f mpls route show
Display of sh dataplane mpls label-table

The dataplane mpls label-table is not populated so we are debugging this in vyatta-dataplane.

Thanks,

1 Added correctly the new revision statement for the changes. Signed-off-by: harios <[email protected]>

wivory

YANG changes approved.

dewi-morgan · 2020-04-21T11:34:27Z

yang/vyatta-protocols-frr-bgp-v1.yang

-          configd:help "BGP IPv4 VPN settings";
-          uses network-ipv4-vpn;
-        }
+				container ipv4-vpn {


Rather than use container ipv4-vpn, can this be changed to vpnv4-unicast ? I understand that the ipv4-vpn is used by the underlying frr configuration, but the vpnv4-unicast / vpnv6-unicast naming is in line with other vendors usage. The mapping would then be from vpnv4-unicast to ipv4-vpn to generate the corresponding frr config from the yang.

Rather than use container ipv4-vpn, can this be changed to vpnv4-unicast ? I understand that the ipv4-vpn is used by the underlying frr configuration, but the vpnv4-unicast / vpnv6-unicast naming is in line with other vendors usage. The mapping would then be from vpnv4-unicast to ipv4-vpn to generate the corresponding frr config from the yang.

Hi ,

We took a note for the change you mentioned here.
When we send PR on the commands for supporting 6PE and 6VPE feature we will rectify that.

Thanks,
Kaushik

looks good, Thank You,

1. Added "vpnv6-unicast" and "ipv6-labeled-unicast" address family in bgp. 2. Added support for "label vpn export auto" in bgp. 3. The address-family "ipv4-vpn" is now replaced with "vpnv4-unicast". Signed-off-by: harios <[email protected]>

deastoe · 2020-11-27T10:56:36Z

Hi @KaushikNiral, @hariosniral,

There are now some conflicts in these diffs. Please could you do a rebase and squash, then we can finish the review and get this merged.

To resolve the revision statement conflicts, please simply update the description on the existing "2020-11-23" revision.

Thanks.

dewi-morgan

I think bgp should be allocating labels rather than asking the user to select a static label. We should now allow the user to select static bgp vpn labels in the yang. Setting "label vpn export auto" allocates and programs an ILM vpn bgp label. I think this configuration can be applied via the configs/commands/bgp_vrf.json instead e.g.

"/routing/routing-instance/@element/protocols/bgp/@element/address-family/ipv4-unicast/route-distinguisher": ["rd vpn export {/@text}", "import vpn", "export vpn", "label vpn export auto"],

dewi-morgan · 2020-12-17T11:03:49Z

yang/vyatta-protocols-frr-bgp-v1.yang

+        type rt-rd;
+        default "100:1";
+      }
+      leaf label {


why is this needed ? bgp should allocate the vpn label. The "label vpn export auto" configuration assigns a vrf label ? Why can't this be done instead of using a static label ?

dewi-morgan · 2020-12-17T11:06:16Z

yang/vyatta-protocols-frr-bgp-v1.yang

+              leaf export {
+                description "For routes leaked from current address-family to vpn";
+                configd:help "For routes leaked from current address-family to vpn";
+                type uint32 {


why is this needed ? bgp should allocate the vpn label. The "label vpn export auto" configuration assigns a vrf label ? Why can't this be done instead of using a static label ?

dewi-morgan · 2020-12-17T11:13:59Z

yang/vyatta-protocols-frr-bgp-v1.yang

+      leaf addr {
+        description "BGP network";
+        type types:ipv4-prefix {
+          configd:normalize "normalize ipv4-prefix";


why is this required from ? Doesn't it make more sense to specify the network from the routing instance for the address family ? Which rd is used if you configure e.g.

set protocols bgp 65201 address-family ipv4-vpn network 1.2.3.0/24

dewi-morgan · 2020-12-17T11:30:40Z

yang/vyatta-protocols-frr-bgp-v1.yang

+				type rt-rd;
+				default "100:1";
+			}
+			leaf label {


setting a static vpn label should not be necessary. I don't think this should be exposed in the yang.

dewi-morgan · 2020-12-17T11:31:46Z

yang/vyatta-protocols-frr-bgp-v1.yang

+								description "For routes leaked from current address-family to vpn";
+								configd:help "For routes leaked from current address-family to vpn";
+								type union {
+									type uint32 {


I don't think setting a static vpn label is necessary. I don't think this should be exposed in the yang.

dewi-morgan · 2020-12-17T11:32:21Z

yang/vyatta-protocols-frr-bgp-v1.yang

+								configd:help "For routes leaked from current address-family to vpn";
+								type union {
+									type uint32 {
+										range 0..1048575;


same comment, I don't think setting a static bgp label should be exposed in the yang.

ekoyle · 2021-07-07T19:58:35Z

What is left for this issue? Is it only hung up on the static labels?

KaushikNiral force-pushed the niral_v4_mpls_l3_vpn_support branch from 3b4711b to fb1fb81 Compare April 14, 2020 18:44

deastoe suggested changes Apr 15, 2020

View reviewed changes

wivory reviewed Apr 16, 2020

View reviewed changes

KaushikNiral and others added 3 commits April 16, 2020 17:02

Commands added for supporting IPv4 MPLS Layer 3 VPN.

a1649db

1. The container ipv4-vpn added under bgp-params-neighbor grouping. 2. The issue with indentation is resolved. Signed-off-by: harios <[email protected]>

Commands added for supporting IPv4 MPLS Layer 3 VPN.

7a05e84

1. Revision added. 2. Rectified indentation. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_v4_mpls_l3_vpn_support branch from 9218513 to 7a05e84 Compare April 16, 2020 11:33

Commands added for supporting IPv4 MPLS Layer 3 VPN.

bf6b3c1

1 Added correctly the new revision statement for the changes. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_v4_mpls_l3_vpn_support branch from 2678cfc to bf6b3c1 Compare April 17, 2020 14:42

wivory approved these changes Apr 20, 2020

View reviewed changes

dewi-morgan reviewed Apr 21, 2020

View reviewed changes

hariosniral force-pushed the niral_v4_mpls_l3_vpn_support branch from f8dacd5 to b75fdd8 Compare May 19, 2020 12:14

dewi-morgan suggested changes Dec 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commands added for supporting IPv4 MPLS Layer 3 VPN. #4

Commands added for supporting IPv4 MPLS Layer 3 VPN. #4

KaushikNiral commented Apr 14, 2020

deastoe Apr 15, 2020

KaushikNiral Apr 15, 2020

deastoe Apr 15, 2020

KaushikNiral Apr 15, 2020

wivory Apr 16, 2020

KaushikNiral Apr 16, 2020

wivory Apr 17, 2020 •

edited

Loading

KaushikNiral Apr 17, 2020

wivory Apr 16, 2020

KaushikNiral Apr 16, 2020

wivory Apr 16, 2020

KaushikNiral Apr 16, 2020

wivory Apr 17, 2020

KaushikNiral Apr 17, 2020

dewi-morgan commented Apr 16, 2020 •

edited

Loading

KaushikNiral commented Apr 16, 2020

wivory left a comment

dewi-morgan Apr 21, 2020

KaushikNiral May 11, 2020 •

edited

Loading

dewi-morgan May 22, 2020

deastoe commented Nov 27, 2020

dewi-morgan left a comment •

edited

Loading

dewi-morgan Dec 17, 2020

dewi-morgan Dec 17, 2020

dewi-morgan Dec 17, 2020

dewi-morgan Dec 17, 2020

dewi-morgan Dec 17, 2020

dewi-morgan Dec 17, 2020

ekoyle commented Jul 7, 2021

Commands added for supporting IPv4 MPLS Layer 3 VPN. #4

Are you sure you want to change the base?

Commands added for supporting IPv4 MPLS Layer 3 VPN. #4

Conversation

KaushikNiral commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wivory Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dewi-morgan commented Apr 16, 2020 • edited Loading

KaushikNiral commented Apr 16, 2020

wivory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaushikNiral May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deastoe commented Nov 27, 2020

dewi-morgan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekoyle commented Jul 7, 2021

wivory Apr 17, 2020 •

edited

Loading

dewi-morgan commented Apr 16, 2020 •

edited

Loading

KaushikNiral May 11, 2020 •

edited

Loading

dewi-morgan left a comment •

edited

Loading