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

T5931: Add option to append route-target when adding additional imports #3623

Closed

Conversation

l0crian1
Copy link
Contributor

This PR adds the ability to have multiple import/export lines of route-target configs within the VPN address-family.

Change Summary

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T5931

Related PR(s)

Component(s) name

frr, bdgd

Proposed changes

How to test

Prior to the change, you could only configure a single import or export line. This required you to place the route-targets in a space separated string:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1 1:2'

After the change, you can have those on a single line. This logic more falls in line with industry standards when configuring route-targets:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:2'

Verification can be performed within FRR:

router bgp 65000 vrf Admins
 address-family ipv4 unicast
  rt vpn import 1:1 1:2
 exit-address-family

Configurations that contain space separated values can still exist alongside the additional config:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:2'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3 1:4'

router bgp 65000 vrf Admins
 address-family ipv4 unicast
  rt vpn import 1:1 1:2 1:3 1:4
 exit-address-family

Smoketest result

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

l0crian1 added 2 commits June 10, 2024 11:27
Added leafNode type /multi for VPN route-targets
Updated bgpd.frr.j2 to handle list input for route-targets
Added leafNode type /multi for VPN route-targets
Updated bgpd.frr.j2 to handle list input for route-targets
Copy link

👍
No issues in PR Title / Commit Title

Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

CLI syntax changed and a migration script is required

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1 1:2'

The configuration needs to be checked to see if several imports are used before changes and migrating to the new format.

@l0crian1
Copy link
Contributor Author

CLI syntax changed and a migration script is required

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1 1:2'

The configuration needs to be checked to see if several imports are used before changes and migrating to the new format.

A request was made to allow both to be used. If both of these are allowed, then there's no need for any migration:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:2'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3 1:4'

This creates the following for the FRR config:

  rt vpn import 1:1 1:2 1:3 1:4

I think allowing both makes sense. This would provide familiarity for people coming from other products, but maintain familiarity for those that have been using VyOS/FRR.

@fett0
Copy link
Contributor

fett0 commented Jun 10, 2024

@l0crian1 Do you test when both are equal (import/export) and add different import ?

@l0crian1
Copy link
Contributor Author

@l0crian1 Do you test when both are equal (import/export) and add different import ?

@fett0 Do you mean like this?

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn export '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:4'

If so, that works completely fine.

router bgp 65000 vrf Admins
 address-family ipv4 unicast
  rt vpn import 1:3 1:4
  rt vpn export 1:3
 exit-address-family

If you meant something like this:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn both '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:4'

That'll already throw an error since both is prevented from existing alongside either import or export.

@fett0
Copy link
Contributor

fett0 commented Jun 10, 2024

@l0crian1 Do you test when both are equal (import/export) and add different import ?

@fett0 Do you mean like this?

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn export '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:4'

If so, that works completely fine.

router bgp 65000 vrf Admins
 address-family ipv4 unicast
  rt vpn import 1:3 1:4
  rt vpn export 1:3
 exit-address-family

If you meant something like this:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn both '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:4'

That'll already throw an error since both is prevented from existing alongside either import or export.

Exactly ! yes , we've had some problems with those combinations in the past , not all the combinations are possible on FRR and for that reason , that was my question . I like the idea that you propose in this PR.

Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

A request was made to allow both to be used. If both of these are allowed, then there's no need for any migration:

I'd recommend not to use both "styles" as this will lead to a validation hell and also is unintuitive.
As 1.4 syntax is frozen this change will only make it into 1.5 and we should make it <multi/> only and migrate existing configurations.

@l0crian1
Copy link
Contributor Author

A request was made to allow both to be used. If both of these are allowed, then there's no need for any migration:

I'd recommend not to use both "styles" as this will lead to a validation hell and also is unintuitive. As 1.4 syntax is frozen this change will only make it into 1.5 and we should make it <multi/> only and migrate existing configurations.

I'm wary to make that change if not allowing both. The current syntax has existed for 3 years, and if it'll only make it into 1.5, it could be another 3 years before that is LTS. So that is potentially 3-6 years of user created tools that would need to be refactored when they move to 1.5. Could lead to a lot of customer unhappiness in the future.

@sever-sever
Copy link
Member

Change to the correct format; we won't accept both formats
Expected format:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:2'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:4'

Unexpected format:

set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:1'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:2'
set vrf name Admins protocols bgp address-family ipv4-unicast route-target vpn import '1:3 1:4'

Also, the migration script must check if several values are used.

@l0crian1
Copy link
Contributor Author

Going to close this PR for now. The functionality for multiple route-targets already exists, so I worry this change will create more problems in the future than it solves right now.

@l0crian1 l0crian1 closed this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants