nhrp: T2326: NHRP migration to FRR #4217

aapostoliuk · 2024-12-03T10:29:19Z

NHRP migration to FRR

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): migration to another package

Related Task(s)

https://vyos.dev/T2326

Related PR(s)

Component(s) name

nhrp

Proposed changes

Replaced the old package opnnhrp with FRR nhrpd.
Advantages:

opennhrp project is not supported now.
It fixes compatibility between NHRP and IPSec. FRR manages an IPSec VPN connection.
It will allow the use of IPv6 over DMVPN in the future.

Restrictions:

The tunnel interface subnet mask must be /32

Changes:
Configuration mode:
All commands were remade according to FRR.

vyos@vyos# set protocols nhrp tunnel tun100
Possible completions:
   authentication       NHRP authentication
   holdtime             Holding time in seconds
 > map                  Map tunnel IP to NBMA
   mtu                  Maximum Transmission Unit (MTU)
+  multicast            Map multicast to NBMA
   network-id           NHRP network id
 > nhs                  Map tunnel IP to NBMA of Next Hop Server
   redirect             Enable sending of Cisco style NHRP Traffic Indication packets
   registration-no-unique
                        Zone to be local-zone
   shortcut             Enable creation of shortcut routes. A received NHRP Traffic
                        Indication will trigger the resolution and establishment of a
                        shortcut route

Operational mode:
All commands were remade according to FRR.

vyos@vyos:~$ show ip nhrp
Possible completions:
  cache                 Show NHRP interface connection information
  nhs                   Show NHRP tunnel connection information
  shortcut

vyos@vyos:~$ reset ip nhrp
Possible completions:
  cache                 Clear Dynamic cache entries
  shortcut              Clear Shortcut entries

How to test

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_protocols_nhrp.py
test_01_nhrp_config (__main__.TestProtocolsNHRP.test_01_nhrp_config) ... ok

----------------------------------------------------------------------
Ran 1 test in 9.307s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_ipsec.py
test_dhcp_fail_handling (__main__.TestVPNIPsec.test_dhcp_fail_handling) ... ok
test_dmvpn (__main__.TestVPNIPsec.test_dmvpn) ... ok
test_flex_vpn_vips (__main__.TestVPNIPsec.test_flex_vpn_vips) ... ok
test_remote_access (__main__.TestVPNIPsec.test_remote_access) ... ok
test_remote_access_dhcp_fail_handling (__main__.TestVPNIPsec.test_remote_access_dhcp_fail_handling) ... ok
test_remote_access_eap_tls (__main__.TestVPNIPsec.test_remote_access_eap_tls) ... ok
test_remote_access_no_rekey (__main__.TestVPNIPsec.test_remote_access_no_rekey) ... ok
test_remote_access_pool_range (__main__.TestVPNIPsec.test_remote_access_pool_range) ... ok
test_remote_access_vti (__main__.TestVPNIPsec.test_remote_access_vti) ... ok
test_remote_access_x509 (__main__.TestVPNIPsec.test_remote_access_x509) ... ok
test_site_to_site (__main__.TestVPNIPsec.test_site_to_site) ... ok
test_site_to_site_vti (__main__.TestVPNIPsec.test_site_to_site_vti) ... ok
test_site_to_site_x509 (__main__.TestVPNIPsec.test_site_to_site_x509) ... ok

----------------------------------------------------------------------
Ran 13 tests in 88.830s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_interfaces_tunnel.py
test_add_multiple_ip_addresses (__main__.TunnelInterfaceTest.test_add_multiple_ip_addresses) ... ok
test_add_single_ip_address (__main__.TunnelInterfaceTest.test_add_single_ip_address) ... ok
test_add_to_invalid_vrf (__main__.TunnelInterfaceTest.test_add_to_invalid_vrf) ... ok
test_dhcp_client_options (__main__.TunnelInterfaceTest.test_dhcp_client_options) ... skipped 'not supported'
test_dhcp_disable_interface (__main__.TunnelInterfaceTest.test_dhcp_disable_interface) ... skipped 'not supported'
test_dhcp_vrf (__main__.TunnelInterfaceTest.test_dhcp_vrf) ... skipped 'not supported'
test_dhcpv6_client_options (__main__.TunnelInterfaceTest.test_dhcpv6_client_options) ... skipped 'not supported'
test_dhcpv6_vrf (__main__.TunnelInterfaceTest.test_dhcpv6_vrf) ... skipped 'not supported'
test_dhcpv6pd_auto_sla_id (__main__.TunnelInterfaceTest.test_dhcpv6pd_auto_sla_id) ... skipped 'not supported'
test_dhcpv6pd_manual_sla_id (__main__.TunnelInterfaceTest.test_dhcpv6pd_manual_sla_id) ... skipped 'not supported'
test_eapol (__main__.TunnelInterfaceTest.test_eapol) ... skipped 'not supported'
test_erspan_v1 (__main__.TunnelInterfaceTest.test_erspan_v1) ... ok
test_gretap_parameters_change (__main__.TunnelInterfaceTest.test_gretap_parameters_change) ... ok
test_interface_description (__main__.TunnelInterfaceTest.test_interface_description) ... ok
test_interface_disable (__main__.TunnelInterfaceTest.test_interface_disable) ... ok
test_interface_ip_options (__main__.TunnelInterfaceTest.test_interface_ip_options) ... ok
test_interface_ipv6_options (__main__.TunnelInterfaceTest.test_interface_ipv6_options) ... ok
test_interface_mtu (__main__.TunnelInterfaceTest.test_interface_mtu) ... ok
test_ip6erspan_v2 (__main__.TunnelInterfaceTest.test_ip6erspan_v2) ... ok
test_ipv4_encapsulations (__main__.TunnelInterfaceTest.test_ipv4_encapsulations) ... ok
test_ipv6_encapsulations (__main__.TunnelInterfaceTest.test_ipv6_encapsulations) ... ok
test_ipv6_link_local_address (__main__.TunnelInterfaceTest.test_ipv6_link_local_address) ... ok
test_move_interface_between_vrf_instances (__main__.TunnelInterfaceTest.test_move_interface_between_vrf_instances) ... ok
test_mtu_1200_no_ipv6_interface (__main__.TunnelInterfaceTest.test_mtu_1200_no_ipv6_interface) ... ok
test_multiple_gre_tunnel_different_remote (__main__.TunnelInterfaceTest.test_multiple_gre_tunnel_different_remote) ... ok
test_multiple_gre_tunnel_same_remote (__main__.TunnelInterfaceTest.test_multiple_gre_tunnel_same_remote) ... ok
test_span_mirror (__main__.TunnelInterfaceTest.test_span_mirror) ... skipped 'not supported'
test_tunnel_invalid_source_interface (__main__.TunnelInterfaceTest.test_tunnel_invalid_source_interface) ... ok
test_tunnel_parameters_gre (__main__.TunnelInterfaceTest.test_tunnel_parameters_gre) ... ok
test_tunnel_src_any_gre_key (__main__.TunnelInterfaceTest.test_tunnel_src_any_gre_key) ... ok
test_vif_8021q_interfaces (__main__.TunnelInterfaceTest.test_vif_8021q_interfaces) ... skipped 'not supported'
test_vif_8021q_lower_up_down (__main__.TunnelInterfaceTest.test_vif_8021q_lower_up_down) ... skipped 'not supported'
test_vif_8021q_mtu_limits (__main__.TunnelInterfaceTest.test_vif_8021q_mtu_limits) ... skipped 'not supported'
test_vif_8021q_qos_change (__main__.TunnelInterfaceTest.test_vif_8021q_qos_change) ... skipped 'not supported'
test_vif_s_8021ad_vlan_interfaces (__main__.TunnelInterfaceTest.test_vif_s_8021ad_vlan_interfaces) ... skipped 'not supported'
test_vif_s_protocol_change (__main__.TunnelInterfaceTest.test_vif_s_protocol_change) ... skipped 'not supported'

----------------------------------------------------------------------
Ran 36 tests in 139.949s

OK (skipped=15)

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

github-actions · 2024-12-03T10:29:37Z

👍
No issues in PR Title / Commit Title

github-actions · 2024-12-03T10:29:41Z

✅ No issues found in unused-imports check.. Please refer the workflow run

github-actions · 2024-12-16T21:29:29Z

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

github-actions · 2024-12-17T09:43:57Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

sarthurdev

LGTM

Should also have PR to remove the package build: https://github.com/vyos/vyos-build/tree/current/scripts/package-build/opennhrp

c-po · 2025-01-09T15:55:51Z

python/vyos/frrender.py

@@ -147,6 +148,43 @@ def dict_helper_pim_defaults(pim, path):
        pim = config_dict_merge(default_values, pim)
        return pim

+    def dict_helper_nhrp_defaults(nhrp):
+        nflog_redirect = 1


What are those magic numbers? Please add a proper comment.

I have added comments about these two numbers.
https://github.com/vyos/vyos-1x/pull/4217/files#diff-5fa707da9950d5de7cf947091ea150f12fd562a821f70857f1733ef1bcab9091R152-R155

More about NFLOG in FRR is described in FRR documentation
https://docs.frrouting.org/en/latest/nhrpd.html#hub-functionality
https://docs.frrouting.org/en/latest/nhrpd.html#multicast-functionality

c-po · 2025-01-09T15:56:34Z

python/vyos/frrender.py

+                                       no_tag_node_value_mangle=True)
+
+        for name, profile_conf in profile.items():
+            if 'disable' not in profile_conf:


You can unindent one entire block by using

Suggested change

if 'disable' not in profile_conf:

if 'disable' in profile_conf:

continue

c-po · 2025-01-09T15:56:50Z

python/vyos/frrender.py

@@ -378,6 +428,8 @@ def dict_helper_pim_defaults(pim, path):
        else:
            dict.update({'static' : {'pppoe' : tmp}})

+


Why are those whitespace lines needed?

NHRP migration to FRR

github-actions · 2025-01-09T19:37:34Z

CI integration ❌ failed!

Details

CI logs

CLI Smoketests (no interfaces) ❌ failed
CLI Smoketests (interfaces only) 👍 passed
Config tests 👍 passed
RAID1 tests 👍 passed
TPM tests 👍 passed

aapostoliuk requested a review from a team as a code owner December 3, 2024 10:29

aapostoliuk requested review from dmbaturin, sarthurdev, zdc, jestabro, sever-sever and fett0 December 3, 2024 10:29

github-actions bot assigned aapostoliuk Dec 3, 2024

github-actions bot added the current label Dec 3, 2024

aapostoliuk marked this pull request as draft December 3, 2024 10:30

c-po mentioned this pull request Dec 13, 2024

frr: upgrade to 10.2 and migrate protocols to unified FRRender class #4227

Merged

14 tasks

github-actions bot added the conflicts label Dec 16, 2024

aapostoliuk force-pushed the T2326-circinus branch from a1cf41c to 9d69ee9 Compare December 17, 2024 09:43

github-actions bot removed the conflicts label Dec 17, 2024

aapostoliuk force-pushed the T2326-circinus branch from 9d69ee9 to 957adaa Compare December 19, 2024 14:02

aapostoliuk force-pushed the T2326-circinus branch 2 times, most recently from 82c7537 to 58f3339 Compare January 6, 2025 15:18

aapostoliuk marked this pull request as ready for review January 6, 2025 15:24

aapostoliuk requested review from c-po and nicolas-fort as code owners January 6, 2025 15:24

aapostoliuk force-pushed the T2326-circinus branch 4 times, most recently from 204e822 to 8a80ad9 Compare January 9, 2025 13:40

sarthurdev approved these changes Jan 9, 2025

View reviewed changes

c-po requested changes Jan 9, 2025

View reviewed changes

aapostoliuk force-pushed the T2326-circinus branch from 8a80ad9 to 156330b Compare January 9, 2025 16:16

nhrp: T2326: NHRP migration to FRR

5e8307b

NHRP migration to FRR

aapostoliuk force-pushed the T2326-circinus branch from 156330b to 5e8307b Compare January 9, 2025 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nhrp: T2326: NHRP migration to FRR #4217

nhrp: T2326: NHRP migration to FRR #4217

aapostoliuk commented Dec 3, 2024

github-actions bot commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 17, 2024

sarthurdev left a comment

c-po Jan 9, 2025

aapostoliuk Jan 9, 2025 •

edited

Loading

c-po Jan 9, 2025

aapostoliuk Jan 9, 2025

c-po Jan 9, 2025

aapostoliuk Jan 9, 2025

github-actions bot commented Jan 9, 2025

	if 'disable' not in profile_conf:
	if 'disable' in profile_conf:
	continue

		@@ -378,6 +428,8 @@ def dict_helper_pim_defaults(pim, path):
		else:
		dict.update({'static' : {'pppoe' : tmp}})

nhrp: T2326: NHRP migration to FRR #4217

Are you sure you want to change the base?

nhrp: T2326: NHRP migration to FRR #4217

Conversation

aapostoliuk commented Dec 3, 2024

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

github-actions bot commented Dec 3, 2024 • edited Loading

github-actions bot commented Dec 3, 2024 • edited Loading

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 17, 2024

sarthurdev left a comment

Choose a reason for hiding this comment

c-po Jan 9, 2025

Choose a reason for hiding this comment

aapostoliuk Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

c-po Jan 9, 2025

Choose a reason for hiding this comment

aapostoliuk Jan 9, 2025

Choose a reason for hiding this comment

c-po Jan 9, 2025

Choose a reason for hiding this comment

aapostoliuk Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

Details

github-actions bot commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

aapostoliuk Jan 9, 2025 •

edited

Loading