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

vtysh & zebra: Interface order of ipv4 #16520

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DennyAgussy
Copy link

@DennyAgussy DennyAgussy commented Aug 7, 2024

Feature request: interface ip addresses order #15464

Introduction:
As per the issue mentioned about IP addresses not being maintained in the order in which they are assigned/configured on an interface, we have added a keyword, setorder, which allows to control the order of the IPs on interfaces.

Feature Overview:
With this feature in place, we can control the order of IPs as needed, and when we restart FRR, the order will be restored according to our setorder parameter, which takes the next immediate integer as its value. Any other commands on an interface will not have their order changed in /etc/frr/frr.conf, except for the IPs with the setorder keyword. All other commands will follow the order in which they are configured.

Implementation Details:
The feature ensures that IPs on interfaces are configured according to the setorder parameter. When we restart FRR, this feature initiates by backing up /etc/frr/frr.conf to a temporary file before modifying the order within frr.conf. Subsequently, sorting occurs based on the setorder keyword and updates are pushed to /etc/frr/frr.conf. Now, the IPs are configured in the sorted order as specified.

Testing and Validation:
Required unit and functionality testing have been completed.

NOTE:
This feature works only when vtysh.conf "service integrated-vtysh-config"

For your reference:
Before restarting frr
interface eth0
ip address 2.2.3.178/24 setorder 5
ip address 3.1.3.178/24 setorder 5
ip address 45.46.8.7/24 setorder 4
ip address 55.65.8.7/24 setorder 1
ip address 76.75.3.9/24 setorder 2
exit

After restarting the frr
interface eth0
ip address 55.65.8.7/24 setorder 1
ip address 76.75.3.9/24 setorder 2
ip address 45.46.8.7/24 setorder 4
ip address 2.2.3.178/24 setorder 5
ip address 3.1.3.178/24 setorder 5
exit

The order in which they will be configered by zebra and ip's order in kernel
ip addr show eth0
562: eth0@if563: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default
link/ether 02:42:0c:0c:00:02 brd ff:ff:ff:ff:ff:ff link-netnsid 0
inet 12.12.0.2/16 brd 12.12.255.255 scope global eth0
valid_lft forever preferred_lft forever
inet 55.65.8.7/24 brd 55.65.8.255 scope global eth0
valid_lft forever preferred_lft forever
inet 76.75.3.9/24 brd 76.75.3.255 scope global eth0
valid_lft forever preferred_lft forever
inet 45.46.8.7/24 brd 45.46.8.255 scope global eth0
valid_lft forever preferred_lft forever
inet 2.2.3.178/24 brd 2.2.3.255 scope global eth0
valid_lft forever preferred_lft forever
inet 3.1.3.178/24 brd 3.1.3.255 scope global eth0
valid_lft forever preferred_lft forever

Conclusion
Please reach out to me if any clarifications are required. If required we can also extend our support to integrate this feature with respect to IPv6, contingent upon the acceptance of the current patch.

Thanks
Denny Agussy

@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch 8 times, most recently from acbe8af to 8f2c260 Compare August 7, 2024 09:29
@github-actions github-actions bot added size/XXL and removed size/XL labels Aug 7, 2024
@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from 8f2c260 to acbe8af Compare August 7, 2024 09:31
@github-actions github-actions bot added size/XL and removed size/XXL labels Aug 7, 2024
@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch 4 times, most recently from 6809dac to fcb832a Compare August 7, 2024 15:54
@github-actions github-actions bot added size/XS and removed size/XL labels Aug 7, 2024
@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from fcb832a to 1ed6596 Compare August 7, 2024 15:55
@github-actions github-actions bot added size/XL and removed size/XS labels Aug 7, 2024
@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from 532367c to 95c4bc9 Compare August 7, 2024 16:20
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

First of all, TBH, it adds more complexity rather than usefulness.

What if I have 1000 addresses? Why IPv6 is not implemented? What is the REAL use case with this strict ordering? Dataplane does not know about such a thing like ORDER. How is the ordering shuffled if I add an IP address in the middle, but directly with iproute2? I'm very sceptical about this feature.

@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch 2 times, most recently from 0904574 to 1ed6596 Compare August 8, 2024 06:54
@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from 1ed6596 to cdf1a4a Compare August 8, 2024 12:45
Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

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

First of all, what issue this PR is trying to solve? As discussed in #15464, if it's to specify the preferred source address for connections on this interface, then it can be solved with a route-map set-src clause. If it's something else, please, specify the problem. Adding a lot of complexity without a specific reason is not reasonable.

Second, regarding the implementation. So you basically added the ability to specify order of IP addresses when saving the config. Many problems with this implementation:

  • no actual ordering implementation without FRR restart
  • no guarantee that zebra will actually offload the addresses to the kernel in the same order
  • this won't work with mgmtd API and reading the config in JSON format; to make this work you have to use ordered-by user attribute for IP address list in YANG model instead of adding a new leaf

I think this is pretty hard to implement this properly. Anyway, we have to specify the issue we're trying to solve first.

@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from cdf1a4a to 72ddfcf Compare August 9, 2024 08:40
@donaldsharp donaldsharp requested a review from eqvinox August 13, 2024 15:42
Copy link
Member

@Jafaral Jafaral left a comment

Choose a reason for hiding this comment

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

Can you please change the PR/commit title to be a short sentence? move any details to the body of the commit.

@DennyAgussy DennyAgussy changed the title vtysh & zebra: Interface order of ipv4 feature developed using an addition prameter setorder in ip_address_cmd vtysh & zebra: Interface order of ipv4 Aug 21, 2024
@Jafaral
Copy link
Member

Jafaral commented Aug 22, 2024

Can you please change the PR/commit title to be a short sentence? move any details to the body of the commit.

Thanks for fixing the PR title, can you plz do the same with the commit?

@DennyAgussy DennyAgussy force-pushed the interface-ipv4-order/setorder branch from 72ddfcf to 5437e22 Compare August 23, 2024 10:01
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DennyAgussy
Copy link
Author

DennyAgussy commented Sep 10, 2024

First of all, TBH, it adds more complexity rather than usefulness.

What if I have 1000 addresses? Why IPv6 is not implemented? What is the REAL use case with this strict ordering? Dataplane does not know about such a thing like ORDER. How is the ordering shuffled if I add an IP address in the middle, but directly with iproute2? I'm very sceptical about this feature.

Hi @ton31337
The dataplane, and even the control plane, aren’t affected by this order during configuration time. However, as per the bug, the problem occurs only when start/restart the FRR. This issue arises only during service start or restart. When the configurations for all the FRR services are integrated, the main configuration file’s content for IP is sorted, before the file is read for playback of the configuration. So, when the actual configuration is applied, the IPs are offloaded to Zebra according to the setorder, which in turn offloads the IPs in the same order to the kernel, through netlink socket, making it serialized. We didn’t implement it for ipv6 as we first want to confirm whether this approach can be considered in real-time.

@DennyAgussy
Copy link
Author

DennyAgussy commented Sep 10, 2024

First of all, what issue this PR is trying to solve? As discussed in #15464, if it's to specify the preferred source address for connections on this interface, then it can be solved with a route-map set-src clause. If it's something else, please, specify the problem. Adding a lot of complexity without a specific reason is not reasonable.

Second, regarding the implementation. So you basically added the ability to specify order of IP addresses when saving the config. Many problems with this implementation:

  • no actual ordering implementation without FRR restart
  • no guarantee that zebra will actually offload the addresses to the kernel in the same order
  • this won't work with mgmtd API and reading the config in JSON format; to make this work you have to use ordered-by user attribute for IP address list in YANG model instead of adding a new leaf

I think this is pretty hard to implement this properly. Anyway, we have to specify the issue we're trying to solve first.

Hi @idryzhov

This implementation doesn’t impact any running configuration. The vtysh will offload the IPs in the set order to Zebra via a socket, which will then offload them to the kernel as they are linear. As of now, we haven't considered YANG; we just wanted to determine whether this solution is applicable in the first place.

@idryzhov
Copy link
Contributor

idryzhov commented Sep 10, 2024

That's exactly what I was talking about. You implemented some very specific use case, which doesn't work without restart, doesn't work with YANG, etc. The solution can stop working any time, if libyang changes their sorting algorithm of IP addresses. Which BTW I believe they did in libyang3. You have to implement it in YANG to make sure it actually works.

Anyway, the main question still wasn't answered - what is the actual purpose of having IP addresses sorted in a specific order?

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.

4 participants