UDN egress integration: create logical infra for primary UDNs to egress for layer2 networks #4561

maiqueb · 2024-07-24T17:28:07Z

What this PR does and why is it needed

This PR reacts to the creation of layer2 primary used defined networks by establishing the OVN logical infrastructure (GW routers and external switches in each node) to allow pods connected to that primary user defined network to egress the cluster.

Which issue(s) this PR fixes

Fixes #

Special notes for reviewers

How to verify it

Run the unit tests - this PR introduces unit tests to ensure the ovn topology created is the expected.

Details to documentation updates

Description for the changelog

Create logical infrastructure to allow egress for primary layer2 user defined networks

Does this PR introduce a user-facing change?

Create logical infrastructure to allow egress for primary layer2 user defined networks

go-controller/pkg/ovn/gateway.go

coveralls · 2024-07-25T17:49:15Z

Changes unknown
when pulling dc8ffb8 on maiqueb:udn-egress-integration-layer2
into ** on ovn-org:master**.

trozet

made it up to the udn, layer3, tests: compute the management port IP commit

will continue tmrw.

go-controller/pkg/ovn/secondary_layer2_network_controller.go

trozet · 2024-08-20T19:52:41Z

go-controller/pkg/ovn/secondary_layer2_network_controller.go

+		externalIPs = append(externalIPs, v6MasqIP.IP)
+	}
+
+	// TODO: is this right ? just copied from L3


hostAddrs are used in syncGatewayLogicalNetwork for L3:

relevantHostIPs, err := util.MatchAllIPStringFamily(utilnet.IsIPv6(hostIfAddr.IP), hostAddrs) if err != nil && err != util.ErrorNoIP { return fmt.Errorf("failed to extract the host IP addrs for network %q: %v", gw.netInfo.GetNetworkName(), err) } pbrMngr := gatewayrouter.NewPolicyBasedRoutesManager(gw.nbClient, gw.clusterRouterName, gw.netInfo) if err := pbrMngr.Add(node.Name, hostIfAddr.IP.String(), l3GatewayConfigIP, relevantHostIPs); err != nil { return fmt.Errorf("failed to configure the policy based routes for network %q: %v", gw.netInfo.GetNetworkName(), err)

and it adds a logical route policy that in the past reroutes access to the node IP via the mgmt port:

1004 inport == "rtos-default.l3.primary_ovn-control-plane" && ip4.dst == 172.18.0.2 /* default.l3.primary_ovn-control-plane */ reroute 10.20.0.2

however it looks like @dceara added the masquerade IPs as additional destinations for UDNs:

1004 inport == "rtos-default.l3.primary_ovn-control-plane" && ip4.dst == 169.254.0.11 /* default.l3.primary_ovn-control-plane */ reroute 10.20.0.2

where 169.254.0.11 is the GR IP for the UDN. Not sure why this is needed. @dceara can you help me out?

Also I noticed a bug, where on ovn_cluster_router (default cluster network) we are adding route policies to reach the UDN mgmt port (10.20.0.2):

[root@ovn-control-plane ~]# ovn-nbctl lr-policy-list ovn_cluster_router Routing Policies 1004 inport == "rtos-ovn-control-plane" && ip4.dst == 10.20.0.2 /* ovn-control-plane */ reroute 10.244.0.2 1004 inport == "rtos-ovn-control-plane" && ip4.dst == 172.18.0.2 /* ovn-control-plane */ reroute 10.244.0.2 1004 inport == "rtos-ovn-control-plane" && ip4.dst == 172.20.0.3 /* ovn-control-plane */ reroute 10.244.0.2

Later edit (re-read the code):

@trozet I didn't test but i'm not sure why we'd need a policy route for the masq IP. It's probably just a bug.

So should we drop the masquerade IP from this ?...

As mentioned above, I didn't test. But if things don't break then it's probably fine.

go-controller/pkg/ovn/secondary_layer2_network_controller.go

go-controller/pkg/ovn/gateway.go

trozet · 2024-08-20T20:12:45Z

go-controller/pkg/ovn/gateway.go

-		if err != nil {
-			return fmt.Errorf("error creating static route %+v in %s: %v", lrsr, gw.clusterRouterName, err)
+
+		if gw.clusterRouterName != "" {


These routes are our:

100.64.0.1/32 via 100.64.0.1 dst-ip 100.64.0.2/32 via 100.64.0.2 dst-ip

which we use to override the source ip route that only points to one GR:

10.244.0.0/24 via 100.64.0.1 src-ip

This was used when we have multiple GRs connected to a single ovn_cluster_router (non-IC). In IC we do:

100.64.0.2 100.64.0.2 dst-ip 100.64.0.3 100.88.0.3 dst-ip

Where we send it to the transit IP of the other node. These routes protect us for if the packet comes from another GR on another node with the source IP of the 100.64.0.x (join IP). Then on the reply, we want the packet to go back to the right GR and not just use the source ip route to send it to our own GR.

With layer 2 network there is no ovn_cluster_router, so the reply packet will always go back to the right GR I think.

Meaning these routes are not needed, right ? I.e. there's no action to be taken here. Correct ?

Right no action to be taken. I just wanted to make note of this so I dont forget in the future why its not there for layer 2.

go-controller/pkg/ovn/gateway.go

trozet · 2024-08-20T20:22:17Z

go-controller/pkg/ovn/gateway.go

-		pbrMngr := gatewayrouter.NewPolicyBasedRoutesManager(gw.nbClient, gw.clusterRouterName, gw.netInfo)
-		if err := pbrMngr.Add(node.Name, hostIfAddr.IP.String(), l3GatewayConfigIP, relevantHostIPs); err != nil {
-			return fmt.Errorf("failed to configure the policy based routes for network %q: %v", gw.netInfo.GetNetworkName(), err)
+	if gw.clusterRouterName != "" {


I think you still need this for layer 2. This is for if a pod is trying to reach the node IP. We reroute it to the management port so it can get to the node. Otherwise in shared gateway mode the GR will just consume the packet. You just need to put these on the GR.

OK, after setting these in the GR I see these policies:

kubectl exec -novn-kubernetes ovnkube-node-vp4zn -- ovn-nbctl lr-policy-list GR_tenantblue_ovn-worker Defaulted container "nb-ovsdb" out of: nb-ovsdb, sb-ovsdb, ovn-northd, ovnkube-controller, ovn-controller, ovs-metrics-exporter Routing Policies 1004 inport == "rtos-tenantblue_ovn-worker" && ip4.dst == 10.89.0.2 /* tenantblue_ovn-worker */ reroute 10.128.0.2 1004 inport == "rtos-tenantblue_ovn-worker" && ip4.dst == 169.254.0.11 /* tenantblue_ovn-worker */ reroute 10.128.0.2

which afaiu re-route traffic to the management port.

switch 49c0cec7-2c2d-4ccb-a7ce-42f4f5a49f18 (tenantblue_ovn_layer2_switch) port k8s-tenantblue_ovn-worker addresses: ["0a:5a:a4:3b:29:dc 10.128.0.2"] ...

Is this correct ? I am not sure we need the masquerade IP to be re-routed.

this is correct. I dont think we need hte masquerade IP either, but it doesn't hurt for now. We can clean that up later.

Oh, I had already removed it when I read your reply.

It was pushed as a separate commit; I can get rid of it easily if needed.

all good lets keep it

maiqueb

Thanks; acted on the comments I could.

Let me know if these changes are OK.

go-controller/pkg/ovn/gateway.go

maiqueb · 2024-08-21T10:30:32Z

go-controller/pkg/ovn/gateway.go

-		if err != nil {
-			return fmt.Errorf("error creating static route %+v in %s: %v", lrsr, gw.clusterRouterName, err)
+
+		if gw.clusterRouterName != "" {


Meaning these routes are not needed, right ? I.e. there's no action to be taken here. Correct ?

go-controller/pkg/ovn/gateway.go

go-controller/pkg/ovn/secondary_layer2_network_controller.go

maiqueb · 2024-08-21T11:40:19Z

go-controller/pkg/ovn/gateway.go

-		pbrMngr := gatewayrouter.NewPolicyBasedRoutesManager(gw.nbClient, gw.clusterRouterName, gw.netInfo)
-		if err := pbrMngr.Add(node.Name, hostIfAddr.IP.String(), l3GatewayConfigIP, relevantHostIPs); err != nil {
-			return fmt.Errorf("failed to configure the policy based routes for network %q: %v", gw.netInfo.GetNetworkName(), err)
+	if gw.clusterRouterName != "" {


OK, after setting these in the GR I see these policies:

kubectl exec -novn-kubernetes ovnkube-node-vp4zn -- ovn-nbctl lr-policy-list GR_tenantblue_ovn-worker Defaulted container "nb-ovsdb" out of: nb-ovsdb, sb-ovsdb, ovn-northd, ovnkube-controller, ovn-controller, ovs-metrics-exporter Routing Policies 1004 inport == "rtos-tenantblue_ovn-worker" && ip4.dst == 10.89.0.2 /* tenantblue_ovn-worker */ reroute 10.128.0.2 1004 inport == "rtos-tenantblue_ovn-worker" && ip4.dst == 169.254.0.11 /* tenantblue_ovn-worker */ reroute 10.128.0.2

which afaiu re-route traffic to the management port.

switch 49c0cec7-2c2d-4ccb-a7ce-42f4f5a49f18 (tenantblue_ovn_layer2_switch) port k8s-tenantblue_ovn-worker addresses: ["0a:5a:a4:3b:29:dc 10.128.0.2"] ...

Is this correct ? I am not sure we need the masquerade IP to be re-routed.

maiqueb · 2024-08-21T12:32:36Z

go-controller/pkg/ovn/secondary_layer2_network_controller.go

+		externalIPs = append(externalIPs, v6MasqIP.IP)
+	}
+
+	// TODO: is this right ? just copied from L3


So should we drop the masquerade IP from this ?...

Signed-off-by: Miguel Duarte Barroso <[email protected]>

For layer2 primary UDN we do not need to perform any operations which are specific to the layer3 topology cluster router. These are: - static routes on the cluster router - logical router policies on the cluster router Co-authored-by: Enrique Llorente <[email protected]> Signed-off-by: Miguel Duarte Barroso <[email protected]>

The gateway route has already the .1 address from the network subnet configured at the LRP so network subnet routing is not needed Signed-off-by: Enrique Llorente <[email protected]>

The gateway router LRP should have the network subnet .1 address since for pods to beable to use it to route traffic Signed-off-by: Enrique Llorente <[email protected]>

The node join IP should be configured at the gateway router Signed-off-by: Enrique Llorente <[email protected]>

This commit now ensure the expectation machine computes the expected management IP from the subnet present on the `NetInfo` configuration instead of relying on an hard-coded string. This will be useful later on, as we add unit tests for the layer2 topologies. Signed-off-by: Miguel Duarte Barroso <[email protected]>

go-controller/pkg/ovn/secondary_layer3_network_controller_test.go

trozet · 2024-08-21T14:58:42Z

go-controller/pkg/ovn/multihoming_test.go

+					managementIP := managementPortIP(subnet)
+
+					if em.gatewayConfig != nil {
+						// there are multiple mgmt ports in the cluster, thus the ports must be scoped with the node name


the commit message talks about now with a gateway on L2, we have a mgmt port and a router port. This comment says there are now multiple management ports when there is a gatewayConfig. Why is that? By multiple mgmt ports you mean cause of the default network mgmt port and then the UDN specific mgmt port?

No, we have a management port on each node right ? Hence, their names must feature the node name.

sure, but why do you assume in this function you have more than one node? wouldn't that be up to the test case?

The production code templates the management port names with the node name since there's a management port per node. Hence they're unique.

The test case is actually using a single node. But I need to tell the test what OVN entities to expect; in this case, a single management port, templated with the node name.

I can delete the comment if that's the source of the confusion.

yeah I think it is just the comments there that are confusing

go-controller/pkg/ovn/multihoming_test.go

go-controller/pkg/ovn/secondary_layer2_network_controller_test.go

test/e2e/network_segmentation.go

go-controller/pkg/ovn/secondary_layer2_network_controller_test.go

A follow up commit will introduce unit tests for L2 topology primary UDNs; these networks also feature the management port. Thus, we can expose the creation of the expected LSPs into functions. Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit will change the factory function for layer2 expected OVN entities since for primary UDNs, the logical switch that implements the layer2 network will now have two extra ports: the management port, plus one port to connect to each gateway router in the cluster. In the unit tests, there is a single node, hence, an extra LSP. Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit only adds the basic test structure; as a result, the added tests are marked as pending. A follow-up commit will add the proper expectations, and remove this `pending` mark from the tests. Co-authored-by: Enrique Llorente <[email protected]> Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit adds the correct expectations for the layer2 tests. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit add tests ensuring the OVN entities are properly cleaned up when the layer2 network controller cleanup function is invoked. Signed-off-by: Miguel Duarte Barroso <[email protected]>

…loyments The current solution for layer2 egress does not work for non-IC deployments, since the same logical switch would have multiple router ports with the same addresses. Addressing this limitation will probably involve a new design. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

This way, traffic from the pod intented for the node can reach it over the management port. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

These functions, which allow the multi-homing test tables to received the cluster configuration (and consequential expectations) as parameters will be re-used by the L2 secondary network controller. It should even be re-used in the future by the localnet controller, once it is covered by unit tests. Signed-off-by: Miguel Duarte Barroso <[email protected]>

This function is commonly used by all secondary controllers (L2 and L3). It should even be re-used in the future by the localnet controller, once it is covered by unit tests. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Since the layer2 secondary network controllers do not allocate the ovn networks annotation (the cluster manager does) we are forced to create the pod with a dummy pod networks annotation set. This commit introduces that change. Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb requested a review from a team as a code owner July 24, 2024 17:28

maiqueb requested a review from girishmg July 24, 2024 17:28

github-actions bot added feature/egress-ip Issues related to EgressIP feature area/unit-testing Issues related to adding/updating unit tests labels Jul 24, 2024

maiqueb added the feature/user-defined-network-segmentation All PRs related to User defined network segmentation label Jul 24, 2024

maiqueb commented Jul 24, 2024

View reviewed changes

go-controller/pkg/ovn/gateway.go Outdated Show resolved Hide resolved

maiqueb force-pushed the udn-egress-integration-layer2 branch from e6d799e to dc8ffb8 Compare July 25, 2024 17:21

This was referenced Jul 26, 2024

Udn egress integration layer2 follow up #4562

Closed

Udn egress l2 #4579

Closed

maiqueb force-pushed the udn-egress-integration-layer2 branch from dc8ffb8 to 2c709e7 Compare August 9, 2024 13:08

maiqueb removed the feature/egress-ip Issues related to EgressIP feature label Aug 9, 2024

maiqueb added this to the v1.1.0 milestone Aug 9, 2024

maiqueb force-pushed the udn-egress-integration-layer2 branch from 2c709e7 to 75751ea Compare August 9, 2024 13:13

github-actions bot added the feature/egress-ip Issues related to EgressIP feature label Aug 9, 2024

maiqueb force-pushed the udn-egress-integration-layer2 branch 3 times, most recently from 9460d1b to f2069dc Compare August 12, 2024 17:44

maiqueb mentioned this pull request Aug 19, 2024

Udn egress layer2 #4594

Closed

maiqueb force-pushed the udn-egress-integration-layer2 branch from f2069dc to 643ad39 Compare August 20, 2024 08:09

github-actions bot added the area/e2e-testing label Aug 20, 2024

maiqueb requested review from trozet and tssurya and removed request for girishmg August 20, 2024 08:10

maiqueb mentioned this pull request Aug 20, 2024

UDN, non-IC: egress on layer2 topology e2e tests fail on non-IC deployments #4642

Open

maiqueb force-pushed the udn-egress-integration-layer2 branch from 643ad39 to 7f17aaf Compare August 20, 2024 10:24

trozet requested changes Aug 20, 2024

View reviewed changes

maiqueb force-pushed the udn-egress-integration-layer2 branch 2 times, most recently from 26a6537 to ab1fd03 Compare August 21, 2024 12:34

maiqueb commented Aug 21, 2024

View reviewed changes

maiqueb and others added 7 commits August 21, 2024 15:37

udn, layer2: add COPP for secondary layer2 controllers

07f4744

Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, egress: sync gateway for layer2 primary UDNs

a3a5e81

Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, l2: Skip network subnet route

eb65392

The gateway route has already the .1 address from the network subnet configured at the LRP so network subnet routing is not needed Signed-off-by: Enrique Llorente <[email protected]>

udn, l2: add .1 address to LRP

a400a27

The gateway router LRP should have the network subnet .1 address since for pods to beable to use it to route traffic Signed-off-by: Enrique Llorente <[email protected]>

udn, egress, l2: Use node join ip at LRP

3fab0a4

The node join IP should be configured at the gateway router Signed-off-by: Enrique Llorente <[email protected]>

maiqueb force-pushed the udn-egress-integration-layer2 branch from ab1fd03 to 7632634 Compare August 21, 2024 13:37

trozet reviewed Aug 21, 2024

View reviewed changes

maiqueb added 2 commits August 21, 2024 19:40

maiqueb force-pushed the udn-egress-integration-layer2 branch from a736e56 to 0f4a1c2 Compare August 21, 2024 17:41

trozet mentioned this pull request Aug 21, 2024

UDN: unnecessary logical route policies added to routers when UDN is enabled #4645

Open

qinqon and others added 12 commits August 22, 2024 13:31

udn, layer2, tests: provide the expectations for l2 topologies

a688e1c

This commit adds the correct expectations for the layer2 tests. Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, layer2, e2e tests: run egress tests

c7891bd

Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, e2e tests: adjust expected routes according to the network topology

19b666b

Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, layer2, unit tests: assert network cleanup

6a190dd

This commit add tests ensuring the OVN entities are properly cleaned up when the layer2 network controller cleanup function is invoked. Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, unit tests: allow managementPortIP to be re-used for L2 tests

0b0148c

Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, layer2: add LRPs to the GR router

9422e8d

This way, traffic from the pod intented for the node can reach it over the management port. Signed-off-by: Miguel Duarte Barroso <[email protected]>

udn, layer2: drop network join addresses reroute policy

9cecad0

Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb force-pushed the udn-egress-integration-layer2 branch from 0f4a1c2 to c76d089 Compare August 22, 2024 11:53

maiqueb requested a review from trozet August 22, 2024 11:54

trozet approved these changes Aug 22, 2024

View reviewed changes

trozet merged commit 6c03bc4 into ovn-kubernetes:master Aug 22, 2024
37 of 39 checks passed

maiqueb deleted the udn-egress-integration-layer2 branch August 22, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UDN egress integration: create logical infra for primary UDNs to egress for layer2 networks #4561

UDN egress integration: create logical infra for primary UDNs to egress for layer2 networks #4561

maiqueb commented Jul 24, 2024 •

edited

Loading

coveralls commented Jul 25, 2024

trozet left a comment

trozet Aug 20, 2024

dceara Aug 21, 2024 •

edited

Loading

maiqueb Aug 21, 2024

dceara Aug 21, 2024

trozet Aug 21, 2024

trozet Aug 20, 2024

maiqueb Aug 21, 2024

trozet Aug 21, 2024

trozet Aug 20, 2024

maiqueb Aug 21, 2024

trozet Aug 21, 2024

maiqueb Aug 21, 2024

trozet Aug 21, 2024

tssurya Sep 11, 2024

maiqueb left a comment

maiqueb Aug 21, 2024

maiqueb Aug 21, 2024

maiqueb Aug 21, 2024

trozet Aug 21, 2024

maiqueb Aug 21, 2024

trozet Aug 21, 2024

maiqueb Aug 21, 2024

trozet Aug 22, 2024

UDN egress integration: create logical infra for primary UDNs to egress for layer2 networks #4561

UDN egress integration: create logical infra for primary UDNs to egress for layer2 networks #4561

Conversation

maiqueb commented Jul 24, 2024 • edited Loading

What this PR does and why is it needed

Which issue(s) this PR fixes

Special notes for reviewers

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

coveralls commented Jul 25, 2024

trozet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dceara Aug 21, 2024 • 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

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

maiqueb 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

maiqueb commented Jul 24, 2024 •

edited

Loading

dceara Aug 21, 2024 •

edited

Loading