Experimental ipv4 over ipv6 #203

mkalcok · 2024-11-06T22:19:59Z

Please review commit by commit.

This PR includes experimental OVN patches that enable adding IPv4 routes with IPv6 next hop. This allows us to enable advertising IPv4 routes via BGP unnumbered session.

Now that we carry multiple patch series, we should keep them logically separate. It makes it easier to identify which series the patch belongs to. Signed-off-by: Martin Kalcok <[email protected]>

FRR 10.1.1 carries multiple bug fixes [0] that enable properly setting IPV6 LLA as a next hop for IPV4 addresses learned via BGP. [0] https://github.com/FRRouting/frr/releases/tag/frr-10.1.1 Signed-off-by: Martin Kalcok <[email protected]>

Test 'bgp_single_peer' had a misleading description, promising to test single BGP connection per chassis in the test. Instead, it only tested a single connection on a single chassis. Functionality as described in the original 'bgp_single_peer' test can be achieved by parametrizing original 'bgp_multiple_peers' test. This cuts down on code repetition for the small increase in the test code complexity. Signed-off-by: Martin Kalcok <[email protected]>

By applying patches proposed to upstream OVN that enable IPv4 routes with IPv6 nexthop [0], we can experimentally enable this functionality in the MicroOVN as well. When user enables BGP in MicroOVN, IPv4 routes leaked by the OVN will be advertised by the FRR. The next hop address for these routes will be IPv6 Link Local address of the OVN router that originally announced the route. [0] https://patchwork.ozlabs.org/project/ovn/list/?series=410497&archive=both&state=* Signed-off-by: Martin Kalcok <[email protected]>

This testsuite verifies that IPv4 networks advertised by the OVN to their BGP neighbors (TOR), can be reached via TOR from external networks. Now that we have data plane tests, original 'bgp.bats' test suite was renamed to 'bgp_control_plane.bats' Signed-off-by: Martin Kalcok <[email protected]>

fnordahl

Thanks for your work on this! The approach looks good and I have a few comments in-line.

fnordahl · 2024-11-07T07:47:07Z

tests/test_helper/bats/bgp_data_plane.bats

+    echo "# ($TEST_CONTAINER) Create LSP '$guest_vm_lsp' ($guest_vm_cidr)"
+    lxc_exec "$TEST_CONTAINER" \
+        "microovn.ovn-nbctl \
+         -- \
+         lsp-add $guest_ls $guest_vm_lsp \
+         -- \
+         lsp-set-addresses $guest_vm_lsp '$guest_vm_mac $guest_vm_cidr'"
+
+    echo "# ($TEST_CONTAINER) Create netns '$guest_vm_ns' and move VM interface '$guest_vm_iface' into it"
+    lxc_exec "$TEST_CONTAINER" "ip netns add $guest_vm_ns"
+    lxc_exec "$TEST_CONTAINER" \
+        "microovn.ovs-vsctl \
+         -- \
+         add-port br-int $guest_vm_iface \
+         -- \
+         set Interface $guest_vm_iface type=internal external_ids:iface-id=$guest_vm_lsp"
+    netns_ifadd "$TEST_CONTAINER" "$guest_vm_ns" "$guest_vm_iface" "$guest_vm_mac" "$guest_vm_cidr"
+    wait_until "microovn_lsp_up $TEST_CONTAINER $guest_vm_lsp"


Any reason not to use the microovn_add_vif function for this?:

microovn/tests/test_helper/microovn.bash

Lines 665 to 701 in 0d5d663

# microovn_add_vif CONTAINER NSNAME IFNAME

#

# Create LSP in LS for CONTAINER, create OVS internal interface with IFNAME,

# attach it to LSP and move it into network namespace NSNAME.

#

# LLADDR and CIDR is generated based on integer found after the last ``-`` in

# CONTAINER which currently needs to be a value between 0-9.

function microovn_add_vif() {

local container=$1; shift

local ns_name=$1; shift

local if_name=$1; shift

local n

n=$(microovn_extract_ctn_n "$container")

assert test "$n" -le 255

local lladdr

printf -v lladdr "00:00:02:00:01:%02x" "$n"

local cidr

printf -v cidr "10.42.%d.10/24" "$n"

local ls_name="${MICROOVN_PREFIX_LS}-${container}"

local lsp_name="${container}-${ns_name}-${if_name}"

lxc_exec "$container" \

"microovn.ovn-nbctl \

-- \

lsp-add $ls_name $lsp_name \

-- \

lsp-set-addresses $lsp_name '$lladdr $cidr'"

lxc_exec "$container" \

"microovn.ovs-vsctl \

-- \

add-port br-int $if_name \

-- \

set Interface $if_name type=internal external_ids:iface-id=$lsp_name"

netns_ifadd "$container" "$ns_name" "$if_name" "$lladdr" "$cidr"

wait_until "microovn_lsp_up $container $lsp_name"

}

I found the function to be tightly coupled with the microovn_add_gw_router which creates LS required by this function and connects it to the LR. However seeing as nothing currently uses microovn_add_vif, perhaps it could be modified to take ls_name as an argument.

It is in use on main, and adding an optional parameter to supply ls_name would be nice.

fnordahl · 2024-11-07T07:57:31Z

tests/test_helper/bats/bgp_data_plane.bats

+    # traffic seems to be a hit-or-miss.
+    local neighbor_lla
+    local egress_port="lrp-$TEST_CONTAINER-$OVN_CONTAINER_INT_IFACE"
+    neighbor_lla=$(microovn_bgp_neighbor_address "$TEST_CONTAINER" "$vrf_device" "${OVN_CONTAINER_INT_IFACE}-bgp")


While retrieving this from vtysh is interesting, we already have two other sources of this information readily available (e.g. ipv6-lla field from ovn-nbctl show or ip a show ${OVN_CONTAINER_INT_IFACE}-bgp).

Why is this third source necessary?

We need the IP of the neighbor, not the local port, so the ovn-nbctl isn't an option.
While true that we could parse output of the ip a show on the BGP_PEER host, I find the output format bit hard to process. I was also thinking about future test scenarios with multiple BGP peers per single chassis, this approach will let you find out addr of your neighbors from the host itself, instead of having to figure out which host (container) is the BGP neighbor for the particular chassis.

fnordahl · 2024-11-07T07:57:56Z

tests/test_helper/bgp_utils.bash

@@ -107,3 +107,18 @@ function microovn_bgp_established() {

    grep -A 2 "Hostname: $neighbor$" <<< "$status" | grep "BGP state = Established"
 }
+
+
+function microovn_bgp_neighbor_address() {


See comment above on the necessity of this function.

Unrelated, but I see that I forgot to add documentation for this function.

fnordahl · 2024-11-07T08:00:25Z

patches/ipv4_over_ipv6/0001-northd-Handle-routing-for-other-address-families.patch

@@ -0,0 +1,1276 @@
+From f5822a6f6da04b35aeda88fd6cb8cec3d7e3212b Mon Sep 17 00:00:00 2001


We know that there are issues with these patches, yet there is no mention of this.

Please add a sentence in the commit message for the commit adding this patch to MicroOVN, citing the known issue and a direct link to Numan's comment as well as our ongoing work to address this issue.

Two new optional parameters allow this function to create VIFs connected to specific LS and with specific IP address. Signed-off-by: Martin Kalcok <[email protected]>

This testsuite verifies that IPv4 networks advertised by the OVN to their BGP neighbors (TOR), can be reached via TOR from external networks. Now that we have data plane tests, original 'bgp.bats' test suite was renamed to 'bgp_control_plane.bats' Signed-off-by: Martin Kalcok <[email protected]>

fnordahl · 2024-11-08T15:02:53Z

tests/test_helper/microovn.bash

+    fi
+
+    if [ -z "$cidr" ]; then
+        cidr="10.42.$n.10/24"


This is called prior to n being defined, however I see this code has already diverged from main, so instead of having you do another iteration on this bit I forwarded your patch with some modifications to a PR for main: #204

In that patch I also fix the use of shift above, which in its current form does not make the options optional.

Thanks for opening that "backport" to main and the fixes. I tested the shift behavior without set -e so I didn't noticed that it returns non-zero RC if it runs out of arguments.

mkalcok added 5 commits November 5, 2024 14:57

snap: organize OVN patches by series

23d913d

Now that we carry multiple patch series, we should keep them logically separate. It makes it easier to identify which series the patch belongs to. Signed-off-by: Martin Kalcok <[email protected]>

snap: Bump FRR version to 10.1.1

0668f4c

FRR 10.1.1 carries multiple bug fixes [0] that enable properly setting IPV6 LLA as a next hop for IPV4 addresses learned via BGP. [0] https://github.com/FRRouting/frr/releases/tag/frr-10.1.1 Signed-off-by: Martin Kalcok <[email protected]>

mkalcok requested a review from a team as a code owner November 6, 2024 22:19

fnordahl requested changes Nov 7, 2024

View reviewed changes

mkalcok added 2 commits November 8, 2024 14:20

tests: Add optional parameters to 'microovn_add_vif'

6b95718

Two new optional parameters allow this function to create VIFs connected to specific LS and with specific IP address. Signed-off-by: Martin Kalcok <[email protected]>

fnordahl approved these changes Nov 8, 2024

View reviewed changes

fnordahl merged commit ac98dff into canonical:experimental/24.09-bgp Nov 8, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental ipv4 over ipv6 #203

Experimental ipv4 over ipv6 #203

mkalcok commented Nov 6, 2024

fnordahl left a comment

fnordahl Nov 7, 2024

mkalcok Nov 7, 2024

fnordahl Nov 7, 2024

fnordahl Nov 7, 2024

mkalcok Nov 7, 2024

fnordahl Nov 7, 2024

mkalcok Nov 7, 2024

fnordahl Nov 7, 2024

mkalcok Nov 7, 2024

fnordahl Nov 8, 2024

mkalcok Nov 8, 2024

	# microovn_add_vif CONTAINER NSNAME IFNAME
	#
	# Create LSP in LS for CONTAINER, create OVS internal interface with IFNAME,
	# attach it to LSP and move it into network namespace NSNAME.
	#
	# LLADDR and CIDR is generated based on integer found after the last ``-`` in
	# CONTAINER which currently needs to be a value between 0-9.
	function microovn_add_vif() {
	local container=$1; shift
	local ns_name=$1; shift
	local if_name=$1; shift

	local n
	n=$(microovn_extract_ctn_n "$container")
	assert test "$n" -le 255
	local lladdr
	printf -v lladdr "00:00:02:00:01:%02x" "$n"
	local cidr
	printf -v cidr "10.42.%d.10/24" "$n"
	local ls_name="${MICROOVN_PREFIX_LS}-${container}"
	local lsp_name="${container}-${ns_name}-${if_name}"

	lxc_exec "$container" \
	"microovn.ovn-nbctl \
	-- \
	lsp-add $ls_name $lsp_name \
	-- \
	lsp-set-addresses $lsp_name '$lladdr $cidr'"
	lxc_exec "$container" \
	"microovn.ovs-vsctl \
	-- \
	add-port br-int $if_name \
	-- \
	set Interface $if_name type=internal external_ids:iface-id=$lsp_name"
	netns_ifadd "$container" "$ns_name" "$if_name" "$lladdr" "$cidr"
	wait_until "microovn_lsp_up $container $lsp_name"
	}

		@@ -0,0 +1,1276 @@
		From f5822a6f6da04b35aeda88fd6cb8cec3d7e3212b Mon Sep 17 00:00:00 2001

Experimental ipv4 over ipv6 #203

Experimental ipv4 over ipv6 #203

Conversation

mkalcok commented Nov 6, 2024

fnordahl left a 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

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