EgressIP hosted by primary interface (`breth0`) for user defined networks #4530

martinkennelly · 2024-07-16T13:46:13Z

~~- [ ] Refactor ID allocator to allow IDs per a range~~ not going to bother added test case and ensure we dont fail to update EIP status if range exhaustion. I wrote a unit test myself and saw we dont fail but it takes multiple minutes to complete so left it out. Added unit tests to directly test the allocator instead of going through kapi though.

[ ] Add ovs rules in ext bridge to drop pod traffic
Add e2es ( written locally but blocked because secondary network controller bug)
Add unit tests for EIP OVN controller
syncPodAssignmentCache refactor for L3/L2 UDN
syncStaleSNATRules refactor for L3/L2 UDN
[ ] Documentation update
[ ] Convert the logs to be network aware
~~- [ ] Efficient flow resync instead of calling Reconcile for all flows (waiting on dependent piece for this)~~
~~- [ ] limit pkt mark to a range thats compatible with pkt marks for UDNs, currently its 50k to limit~~ Not needed theres a limit of just over 4k.

Depends on #4476

pperiyasamy

reviewed until 93e13cb.

go-controller/pkg/ovn/base_network_controller.go

pperiyasamy · 2024-07-29T03:29:07Z

go-controller/pkg/ovn/base_network_controller_egressip.go

 			for _, namespace := range namespaces {
+				// determine if this network configures EIP for this namespace


it is being used at multiple places, so this logic can be moved to a new method.

I thought about that but i didnt want a wrapper around this as i thought this was easier to read. Id still need to check for error, return err if needed and call continue if not primary and no error. WDYT? I think id rather leave it .

pperiyasamy · 2024-07-29T03:32:49Z

go-controller/pkg/ovn/base_network_controller_egressip.go

+	if n2 != nil {
+		return n2
+	}
+	panic("getNonNilNamespace(): received two nil Namespace pointers")


can you return error instead of panic ?

we should never get into this scenario where theres two nil namespaces passed and if we do I wanted to scream via a panic instead of it possibly hidden in error logs. WDYT?

go-controller/pkg/util/egressip_annotation.go

pperiyasamy · 2024-07-29T05:06:20Z

go-controller/pkg/clustermanager/egressip_controller.go

+			if err != nil {
+				return fmt.Errorf("failed to get mark from EgressIP %s: %v", egressIP.Name, err)
+			}
+			if !mark.IsValid() {


i think it would be good to check for id duplicates.

It shouldn't be required as on startup we reserve any IDs that are already allocated and then we can depend on the ID allocator giving us non-used IDs.

pperiyasamy · 2024-07-29T05:20:36Z

go-controller/pkg/ovn/base_network_controller_egressip.go

+	nadName := bnc.GetNetworkName()
+	if bnc.IsSecondary() {
+		ni, err := bnc.getActiveNetworkForNamespace(pod.Namespace)
+		if err != nil {
+			return fmt.Errorf("failed to get active network for Namespace %s: %v", pod.Name, err)
+		}
+		nadNames := ni.GetNADs()
+		if len(nadNames) == 0 {
+			return fmt.Errorf("expected at least one NAD name for Namespace %s", pod.Namespace)
+		}
+		nadName = nadNames[0] // there should only be one active network
+	}


this can be moved to a new method.

pperiyasamy · 2024-07-29T05:27:09Z

go-controller/pkg/ovn/base_network_controller_egressip.go

+		if err != nil {
+			return fmt.Errorf("failed to get active network for Namespace %s: %v", pod.Name, err)
+		}
+		nadNames := ni.GetNADs()


question: isn't always only one NAD for the network ? why it's list here ?

There can be multiple nads referencing the same network.

pperiyasamy · 2024-07-29T06:57:37Z

go-controller/pkg/ovn/base_network_controller_egressip.go

 		if !namespaceSelector.Matches(oldLabels) && namespaceSelector.Matches(newLabels) {
-			if err := bnc.addNamespaceEgressIPAssignments(egressIP.Name, egressIP.Status.Items, newNamespace, egressIP.Spec.PodSelector); err != nil {
+			if err := bnc.addNamespaceEgressIPAssignments(egressIP.Name, egressIP.Status.Items, mark, newNamespace, egressIP.Spec.PodSelector); err != nil {


can you just pass in egressIP.Annotations all the way and retrieve mark from it in addPodEgressIPAssignment method because it's required only in this method ?

I was trying to only pass the info that was required and tried not to pass the whole map. I thought that was more readable for folks reading the func signatures and deduce what fn argument was used for what.

pperiyasamy · 2024-07-29T07:05:31Z

go-controller/pkg/ovn/base_network_controller_egressip.go

@@ -1644,7 +1674,7 @@ func (e *egressIPZoneController) addPodEgressIPAssignment(egressIPName string, s
 	// exec when node is local OR when pods are local
 	// don't add a reroute policy if the egress node towards which we are adding this doesn't exist
 	if loadedEgressNode && loadedPodNode && isLocalZonePod {
-		ops, err = e.createReroutePolicyOps(ops, podIPs, status, egressIPName, nextHopIP)
+		ops, err = e.createReroutePolicyOps(ops, podIPs, status, mark, egressIPName, nextHopIP)


isn't this redunant to do pkt marking here ? this is already done in GW router, also mark doesn't survive when it goes out to wire.

I was thinking of EIP multi NIC when I wrote this where it can reroute to local mgnt port.

pperiyasamy · 2024-07-29T08:10:51Z

go-controller/pkg/node/gateway_egressip.go

+	v6 map[int]string
+}
+
+func (e markIPs) insert(mark util.EgressIPMark, ip net.IP) {


can we make markIPs methods to deal with just int argument for mark ? Because the validations are already handled with parseEIPMarkIP.

pperiyasamy · 2024-07-29T08:19:48Z

go-controller/pkg/node/gateway_egressip.go

+		}
+		// must always add to cache before adding IP because we want to inform node ip handler that this is not a valid node IP
+		g.cache.insertMarkIP(pktMark, ip)
+		if err = g.addIPToAnnotation(ip); err != nil {


did we really need to add eip's into node annotation ? Looks like this is used only at syncEgressIP, can't we derive it from EIPs statuses ?
why do we need to configure eip on the bridge link ?

If the EIP is deleted while controller is down, we've no way to know what addresses we configured on the bridge and clean it up.

Maybe theres a better idea that I am not thinking of to get the set of IPs we added but I couldnt think of it.

pperiyasamy · 2024-07-29T12:24:21Z

go-controller/pkg/node/gateway_egressip.go

+}
+
+func (g *bridgeEIPAddrManager) syncEgressIP(objs []interface{}) error {
+	// caller must synchronise


typo: synchronize

pperiyasamy · 2024-07-29T12:39:52Z

go-controller/pkg/node/gateway_shared_intf.go

+				if util.IsNetworkSegmentationSupportEnabled() && config.OVNKubernetesFeature.EnableInterconnect &&
+					config.Gateway.Mode != config.GatewayModeDisabled && bridge.eipMarkIPs != nil {
+					if netConfig.masqCTMark != ctMarkOVN {
+						for mark, eip := range bridge.eipMarkIPs.GetIPv4() {


This is really nice! having eipMarkIPs as source of truth to program SNAT OF rules.

martinkennelly · 2024-07-29T18:09:22Z

@pperiyasamy I added some new commits but made no extra changes to the commits you viewed except addressing your comments and it wasnt a large amount of code. yet. Any comments Ive marked resolved, I changed the code. If you disagree, mark them unresolved.

pperiyasamy · 2024-07-30T09:04:36Z

go-controller/pkg/libovsdb/ops/router.go

@@ -256,6 +256,27 @@ func FindLogicalRouterPoliciesWithPredicate(nbClient libovsdbclient.Client, p lo
 	return found, err
 }

+// FindALogicalRouterPoliciesWithPredicate looks up a logical router policies from
+// the cache based on a given predicate
+func FindALogicalRouterPoliciesWithPredicate(nbClient libovsdbclient.Client, routerName string, p logicalRouterPolicyPredicate) ([]*nbdb.LogicalRouterPolicy, error) {


this method seems to be querying LRPs twice from router followed by cache, can't we just apply predicate p on the queried polices from router and just return the matched ones ?

pperiyasamy · 2024-07-30T09:52:20Z

go-controller/pkg/ovn/base_network_controller_egressip.go

@@ -62,24 +65,64 @@ func getEgressIPAddrSetDbIDs(name egressIPAddrSetName, controller string) *libov
 	})
 }

-func getEgressIPLRPNoReRouteDbIDs(priority int, uniqueName egressIPNoReroutePolicyName, ipFamily egressIPFamilyValue) *libovsdbops.DbObjectIDs {
-	return libovsdbops.NewDbObjectIDs(libovsdbops.LogicalRouterPolicyEgressIP, DefaultNetworkControllerName, map[libovsdbops.ExternalIDKey]string{
+func getEgressIPLRPReRouteDbIDs(egressIPName, podNamespace, podName string, ipFamily egressIPFamilyValue, controller string) *libovsdbops.DbObjectIDs {


nice, It's good to have extensive ExternalIDs for LRPs, QoSs and SNATs. this would make predicates just to rely on externalIDs.

pperiyasamy · 2024-07-30T10:10:29Z

go-controller/pkg/ovn/base_network_controller_egressip.go

+		if item.Priority != types.EgressIPReroutePriority || item.ExternalIDs[libovsdbops.OwnerControllerKey.String()] != bnc.controllerName {
+			return false
+		}
+		objReferences := strings.Split(item.ExternalIDs[libovsdbops.ObjectNameKey.String()], "_")


can you add more info about ObjectNameKey. i think it contains eip name and namespaced name of the pod.

pperiyasamy · 2024-07-30T10:13:39Z

go-controller/pkg/ovn/base_network_controller_egressip.go

+		// balancing between hops, but we offer no guarantees except one of the EIPs will work
+		ops, err = libovsdbops.DeleteLogicalRouterPolicyWithPredicateOps(e.nbClient, ops, e.GetNetworkScopedClusterRouterName(), p)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create logical router policy operations on ovn_cluster_router: %v", err)


change the error string reflecting what DeleteLogicalRouterPolicyWithPredicateOps method does

pperiyasamy · 2024-07-30T10:26:23Z

go-controller/pkg/ovn/external_ids_syncer/logical_router_policy/logical_router_policy_sync.go

would review this file later.

pperiyasamy · 2024-07-30T10:29:51Z

go-controller/pkg/libovsdb/ops/model.go

typo in the commit message: suffifient

pperiyasamy · 2024-07-30T10:37:02Z

go-controller/pkg/ovn/base_network_controller_egressip.go

@@ -647,6 +647,9 @@ func (bnc *BaseNetworkController) addPodEgressIPAssignments(name string, statusA
 		klog.Infof("Pod %s is already in completed state, skipping egress ip assignment", podKey)
 		return nil
 	}
+	if util.PodWantsHostNetwork(pod) {


curious to know: are we just finding this only now ?

pperiyasamy · 2024-07-30T10:58:45Z

go-controller/pkg/ovn/base_network_controller_egressip_udn_l3_test.go

+				node2IPv4CIDR := node2IPv4 + "/24"
+				_, node1CDNSubnet, _ := net.ParseCIDR(v4Node1Subnet)
+				_, node1UDNSubnet, _ := net.ParseCIDR(v4Node1Net1)
+				nadName := util.GetNADName(eipNamespace2, nadName1)


the variable names for nad names are bit confusing. can we have it like nadName and nadNamespacedName ?

pperiyasamy · 2024-07-30T11:33:37Z

go-controller/pkg/ovn/base_network_controller_egressip_udn_l3_test.go

+					egressSVCServedPodsASUDNv4,
+					egressIPServedPodsASUDNv4,
+					egressNodeIPsASUDNv4,
+				}


how about validating NAT flows for UDN pods ? can we also do it in the same test ?

pperiyasamy · 2024-07-30T11:40:53Z

go-controller/pkg/ovn/base_network_controller_egressip_udn_l3_test.go

+		})
+	})
+})
+


i think its also worth to test with two UDNs having overlapping pod ip addresses.

..in-order to facilitate future consumption by primary UDNs (secondary controllers). Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

The name is overly generic and will conflict with any other future allocators added to the CM EIP controller. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

..from NATs. We can no longer depend on NATs always being present on a GW router to lookup pod IPs. i.e. UDN use case Signed-off-by: Martin Kennelly <mkennell@redhat.com>

..because UDN NAT to EIP will occur on br-ex. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

CDN will not require a mark. No need to update sync cache as marks do not change for the lifetime of an EIP object and if the OVN constructs are stale, they will be stale for other stale info. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

It does not perform comparisons between types. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Owner references are required for each networks controller to safely manipulate its networks OVN contructs. Sync func is required. Update tests to include owner references. Update EIP controller code to use owner references. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Concurrent create transactions can result in the cache being stale for a very short period. Adding a wait for logical router policies create transactions should prevent duplicate policies in these cases where the predicate does not find the policy in time. However, the current constraint of ensuring prio+match is unique is not suffifient for multi networks. Include ext ID also which may or may not contain LRP meta data. If it doesnt contain meta data, this new requirement has new effect however if it does, it allows the same prio+match fields as long as ext ID field is unique. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Secondary networks may support different IP families than CDN. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Remove stale GW pkt mark LRPs and add any required. Units tests in previous commit for UDN l3. See sync tests. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

I've seen scenarios where address set names are empty, lets return an explicit error instead. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

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

martinkennelly added component/cluster-manager Issues related to cluster-manager component and removed feature/egress-service Issues related to egress service labels Jul 16, 2024

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

martinkennelly force-pushed the eip-net-seg branch from 99ae74b to 089b79e Compare July 17, 2024 09:10

github-actions bot added area/e2e-testing feature/egress-service Issues related to egress service labels Jul 17, 2024

martinkennelly force-pushed the eip-net-seg branch 4 times, most recently from a78132a to 07b2cf8 Compare July 24, 2024 09:24

pperiyasamy reviewed Jul 29, 2024

View reviewed changes

martinkennelly force-pushed the eip-net-seg branch from 9b18b4a to 1cc65e7 Compare July 29, 2024 18:05

pperiyasamy reviewed Jul 30, 2024

View reviewed changes

martinkennelly mentioned this pull request Aug 7, 2024

EIP: co-exist with UDN when not supported #4593

Open

martinkennelly added 11 commits August 27, 2024 11:48

EIP: move controller from DNC to BNC

2467422

..in-order to facilitate future consumption by primary UDNs (secondary controllers). Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP: rename file following movement to BNC

efe424d

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

BNC: Create func to lookup network role by namespace

c02d7d5

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP: restrict EIP controller only to networks that have role primary

9e3b06a

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP: enable controller on UDNs

5879079

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP CM: rename "allocator" EIP node cache

5621ab5

The name is overly generic and will conflict with any other future allocators added to the CM EIP controller. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP CM: add pkt mark to annotations

cd5cf4f

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN controller: fetch LSP by network

626c62e

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: store assigned pod IPs instead of lookup

952dc33

..from NATs. We can no longer depend on NATs always being present on a GW router to lookup pod IPs. i.e. UDN use case Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: NAT only when default cluster network

e5e4a36

..because UDN NAT to EIP will occur on br-ex. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

martinkennelly added 17 commits August 27, 2024 11:48

util: remove requirement from generic func to be comparable

d6e3b8a

It does not perform comparisons between types. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Default bridge: add EIP SNAT

4c90880

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: sync cache only for owned router

7b0cb06

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: skip SNAT cache building for secondary networks

dd577a3

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Ensure default route to ext is multi net aware

87d2d20

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN controller: ignore requests from host net pods

42762da

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: align IP family support with bnc

8b2bba3

Secondary networks may support different IP families than CDN. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN Controller: unit tests for L3 UDN

9756310

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Fake OVN: respect fake address set choice for secondary controller

1b5236d

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP Multi NIC: do not config role primary secondary network namespaces

f8927ae

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP OVN controller: add sync for GW pkt mark LRPs

74a9cc9

Remove stale GW pkt mark LRPs and add any required. Units tests in previous commit for UDN l3. See sync tests. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Add e2es for UDN

e8a4cce

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Explicity reject empty address sets

d411309

I've seen scenarios where address set names are empty, lets return an explicit error instead. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP LRP obj name key: add func to retrieve divider

b3cfb93

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

EIP e2es: test EIP selecting multiple ns with multiple primary networks

476e6f2

Signed-off-by: Martin Kennelly <mkennell@redhat.com>

martinkennelly force-pushed the eip-net-seg branch from 1cc65e7 to 476e6f2 Compare September 2, 2024 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EgressIP hosted by primary interface (`breth0`) for user defined networks #4530

EgressIP hosted by primary interface (`breth0`) for user defined networks #4530

martinkennelly commented Jul 16, 2024 •

edited

Loading

pperiyasamy left a comment

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024 •

edited

Loading

pperiyasamy Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly Jul 29, 2024

martinkennelly Jul 29, 2024

pperiyasamy Jul 29, 2024

pperiyasamy Jul 29, 2024

martinkennelly commented Jul 29, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

pperiyasamy Jul 30, 2024

		for _, namespace := range namespaces {
		// determine if this network configures EIP for this namespace

EgressIP hosted by primary interface (breth0) for user defined networks #4530

Are you sure you want to change the base?

EgressIP hosted by primary interface (breth0) for user defined networks #4530

Conversation

martinkennelly commented Jul 16, 2024 • edited Loading

pperiyasamy 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

martinkennelly Jul 29, 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

Choose a reason for hiding this comment

martinkennelly commented Jul 29, 2024

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

EgressIP hosted by primary interface (`breth0`) for user defined networks #4530

EgressIP hosted by primary interface (`breth0`) for user defined networks #4530

martinkennelly commented Jul 16, 2024 •

edited

Loading

martinkennelly Jul 29, 2024 •

edited

Loading