Implement Egress Services #3135

oribon · 2022-08-31T08:55:32Z

- What this PR does and why is it needed
This PR implements service egress traffic steering as described in openshift/enhancements#1200.
By annotating a LoadBalancer service with a valid "k8s.ovn.org/egress-service" annotation , users could request that the source IP of egress packets originating from all of the pods that are endpoints of it would be its ingress IP.
This behavior requires that service access from outside the cluster will only work to a designated node and is contradictory to how default services work where a service may be accessed via any node.
Announcing the service externally (for service ingress traffic) would be handled by a separate LoadBalancer provider.

~~TODOs:~~

~~Implement isReachable, for now it always returns true (waiting for Egress IP health monitoring over GRPC #3100)~~
~~Add ETP=Local tests~~

- Special notes for reviewers
The two commits that handle ovnkube-master/ovnkube-node can be reviewed separately.

- How to verify it
Running the unit tests contained in:
go-controller/pkg/ovn/egressservices_test.go
go-controller/pkg/node/gateway_localnet_linux_test.go
Running E2E tests:
test/e2e/egress_services.go (Focusing on IPv4/IPv6 if the cluster does not support one)

- Description for the changelog
Implement Egress Services

coveralls · 2022-08-31T09:38:30Z

Coverage increased (+0.06%) to 52.453% when pulling 19d411a on oribon:service_egress_traffic_steering into 8566415 on ovn-org:master.

trozet

I finished through the master side commit. Great work Ori. I'll review the last 2 commits tmrw.

@jcaamano if you also want to take a look. If you don't have a lot of time focus on the libovsdb portions please.

go-controller/pkg/ovn/egressip.go

go-controller/pkg/ovn/controller/egress_services/egress_services_controller.go

trozet · 2022-08-31T21:30:45Z

go-controller/pkg/ovn/controller/egress_services/egress_services_controller.go

+		}()
+	}
+
+	go c.checkNodesReachability()


so we are going to have more than 1 goroutine doing this same thing? We should only have 1 goroutine doing this and make a universal cache (sync map) that both controllers can query.

agreed over slack that we should do it later as it might be a bit big and may impact egressip too much -
wrote TODOs in multiple places + will open an issue once this is merged so we won't forget addressing this.
in the meantime I added a commit that generalizes the current reachability functions a bit
and pass the controller a isReachable func when it is initialized.
also added a small test that verifies the controller is responding correctly to a node hosting a service becoming unreachable and reachable (which I should have added earlier 🙂).

go-controller/pkg/ovn/controller/egress_services/egress_services_controller.go

go-controller/pkg/ovn/controller/egress_services/egress_services_service.go

trozet · 2022-09-01T21:45:27Z

go-controller/pkg/ovn/controller/egress_services/egress_services_service.go

+		return c.clearServiceResources(key, state)
+	}
+
+	// We check if the service has a local host endpoint to avoid


I don't understand this. So if the service host networked endpoint, you want to avoid enabling the egress service? Why? We can simply have a NAT rule to SNAT that as well to the egress IP, as long as that endpoint is on a node matching the node selector.

whoops
changed here and in the node's side to care only about the service's non-host networked endpoints (added IsHostEndpoint func). is it better now?

go-controller/pkg/ovn/egressservices_test.go

go-controller/pkg/ovn/ovn.go

trozet · 2022-09-01T22:06:33Z

go-controller/pkg/types/const.go

@@ -64,7 +64,8 @@ const (
 	InterNodePolicyPriority               = "1003"
 	HybridOverlaySubnetPriority           = 1002
 	HybridOverlayReroutePriority          = 501
-	DefaultNoRereoutePriority             = 101
+	DefaultNoRereoutePriority             = 102


did you check to make sure on upgrade we handle updating the current acls from 101->102 that would be in use for egress IP?

not yet, will update here once I do

verified now :)

jcaamano

@trozet @oribon

Looked at the pretty minor libovsdbops changes, looked fine to me, just had that minor nit.

jcaamano · 2022-09-02T09:35:05Z

go-controller/pkg/libovsdbops/router.go

+// CreateOrUpdateLogicalRouterPolicyWithPredicateOps looks up a logical
+// router policy from the cache based on a given predicate. If it does not
+// exist, it creates the provided logical router policy. If it does, it
+// updates it. The logical router policy is added to the provided logical
+// router. Returns the corresponding ops
+func CreateOrUpdateLogicalRouterPolicyWithPredicateOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation,
+	routerName string, lrp *nbdb.LogicalRouterPolicy, p logicalRouterPolicyPredicate, fields ...interface{}) ([]libovsdb.Operation, error) {
+	if len(fields) == 0 {
+		fields = onModelUpdatesAllNonDefault()
+	}
+	router := &nbdb.LogicalRouter{
+		Name: routerName,
+	}
+
+	opModels := []operationModel{
+		{
+			Model:          lrp,
+			ModelPredicate: p,
+			OnModelUpdates: fields,
+			DoAfter:        func() { router.Policies = []string{lrp.UUID} },
+			ErrNotFound:    false,
+			BulkOp:         false,
+		},
+		{
+			Model:            router,
+			ModelPredicate:   func(item *nbdb.LogicalRouter) bool { return item.Name == router.Name },
+			OnModelMutations: []interface{}{&router.Policies},
+			ErrNotFound:      true,
+			BulkOp:           false,
+		},
+	}
+
+	m := newModelClient(nbClient)
+	return m.CreateOrUpdateOps(ops, opModels...)
+}
+
+// DeleteLogicalRouterPolicyWithPredicateOps looks up a logical
+// router policy from the cache based on a given predicate and returns the
+// corresponding ops to delete it and remove it from the provided router.
+func DeleteLogicalRouterPolicyWithPredicateOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation, routerName string, p logicalRouterPolicyPredicate) ([]libovsdb.Operation, error) {
+	router := &nbdb.LogicalRouter{
+		Name: routerName,
+	}
+
+	deleted := []*nbdb.LogicalRouterPolicy{}
+	opModels := []operationModel{
+		{
+			ModelPredicate: p,
+			ExistingResult: &deleted,
+			DoAfter:        func() { router.Policies = extractUUIDsFromModels(&deleted) },
+			ErrNotFound:    false,
+			BulkOp:         true,
+		},
+		{
+			Model:            router,
+			ModelPredicate:   func(lr *nbdb.LogicalRouter) bool { return lr.Name == router.Name },
+			OnModelMutations: []interface{}{&router.Policies},
+			ErrNotFound:      true,
+			BulkOp:           false,
+		},
+	}
+
+	m := newModelClient(nbClient)
+	return m.DeleteOps(ops, opModels...)
+}
+


nit: please refactor the non Ops version of these methods to call these and then transact; similar to what we are already doing with other methods that have both Ops and non-Ops variants in this file.

jcaamano · 2022-09-02T09:45:36Z

go-controller/pkg/ovn/controller/egress_services/egress_services_controller.go

+		svcKey, found := item.ExternalIDs["EgressSVC"]
+		if !found {
+			klog.Infof("Egress service repair will delete lrp because it uses the egress service priority but does not belong to one: %v", item)
+			return true
+		}
+
+		svc, found := c.services[svcKey]
+		if !found {
+			klog.Infof("Egress service repair will delete %s because it is no longer a valid egress service: %v", svcKey, item)
+			return true
+		}


BTW I am not sure if we should be bothering with this but this is the perfect use case for adding a secondary index as client index so that looking up policies with ExternalIDs["EgressSVC"]==svcKey is fast.

https://github.com/ovn-org/libovsdb#client-indexes

go-controller/pkg/node/gateway_shared_intf_linux.go

go-controller/pkg/node/gateway_localnet_linux_test.go

flavio-fernandes

/assign @flavio-fernandes

flavio-fernandes · 2022-09-02T21:12:50Z

test/e2e/egress_services.go

@@ -0,0 +1,452 @@
+package e2e


@oribon have you considered what happens when egress-services is configured in a cluster together with egress IPs? I would suspect OVN rules to look a bit similar in that case. Making it predictable as possible would be a big win.

It may be useful to have that integration tested to ensure it behaves as we expect it to. wdyt?

yep, in the enhancement we decided this will use lrps with a higher priority than the eip ones - it's a good idea to have it tested so I'll write something :)

added Should validate egress service has higher priority than EgressIP :)

The no-reroute (allow) policies that ensure east<->west traffic does not break when steering a pod's traffic needs to be created the same for any controller that creates reroute policies (like the egress service controller). This change allows us to reuse the functions that create them in these controllers. Also, we add an ExternalID for the node policy so that a level driven controller can pass the node's key to the delete path as it can't send a deleted node's object. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

This allows us to share the reachability logic used for EgressIP nodes with any controller that cares about monitoring nodes but does not use the egressNode struct. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

Export these functions as they can be used in a controller that watches endpointslices/services like the egress service controller. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

Add the logic that handles ovnkube-master's side of implementing egress services, which watches LoadBalancer services with the "k8s.ovn.org/egress-service" annotation and primarily: * picks a node for the service that would handle all of its traffic. * creates logical router policies on the cluster router that steer the service's pods egress traffic towards the node's mgmt port. * adds the "k8s.ovn.org/egress-service-host" to the service with the node's name. * adds the "egress-service.k8s.ovn.org" label to the node with the service's namespace/name. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

Add the logic that handles ovnkube-node's side of implementing egress services, which watches LoadBalancer services with the "k8s.ovn.org/egress-service-host" annotation and creates the iptables rules that SNATs packets coming from the service's pods to its ingress IP before sending them out. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

Add initial E2E tests for egress services, verifying that the egress traffic of pods backing an egress service is SNATed to the service's ingress IP and that ingress traffic for the service is not broken. We also verify that the nodeSelector config option works correctly by updating it to pick a different node each time. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

trozet

Please add docs/egress-service.md in another PR

Thanks @oribon !

oribon force-pushed the service_egress_traffic_steering branch from c1b7981 to ddeabe4 Compare August 31, 2022 08:59

oribon changed the title ~~Implement Egress Services~~ [WIP]: Implement Egress Services Aug 31, 2022

oribon force-pushed the service_egress_traffic_steering branch from ddeabe4 to f8f4623 Compare August 31, 2022 09:20

oribon changed the title ~~[WIP]: Implement Egress Services~~ Implement Egress Services Aug 31, 2022

oribon force-pushed the service_egress_traffic_steering branch 7 times, most recently from 858d091 to ef00529 Compare September 1, 2022 12:56

trozet requested changes Sep 1, 2022

View reviewed changes

jcaamano reviewed Sep 2, 2022

View reviewed changes

trozet requested changes Sep 2, 2022

View reviewed changes

go-controller/pkg/node/gateway_shared_intf_linux.go Outdated Show resolved Hide resolved

go-controller/pkg/node/gateway_localnet_linux_test.go Show resolved Hide resolved

flavio-fernandes reviewed Sep 2, 2022

View reviewed changes

oribon force-pushed the service_egress_traffic_steering branch 5 times, most recently from 69e0f6e to 955088c Compare September 7, 2022 21:06

oribon added 6 commits September 8, 2022 11:00

EgressIP: Generalize node reachability functions

40e2212

This allows us to share the reachability logic used for EgressIP nodes with any controller that cares about monitoring nodes but does not use the egressNode struct. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

Services controller: Export serviceControllerKey and add IsHostEndpoint

7d02e87

Export these functions as they can be used in a controller that watches endpointslices/services like the egress service controller. Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>

oribon force-pushed the service_egress_traffic_steering branch from 955088c to 19d411a Compare September 8, 2022 08:00

trozet approved these changes Sep 8, 2022

View reviewed changes

trozet merged commit 65019d8 into ovn-org:master Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Egress Services #3135

Implement Egress Services #3135

oribon commented Aug 31, 2022 •

edited

coveralls commented Aug 31, 2022 •

edited

trozet left a comment

trozet Aug 31, 2022

oribon Sep 7, 2022

trozet Sep 1, 2022

flavio-fernandes Sep 2, 2022

oribon Sep 4, 2022

trozet Sep 1, 2022

oribon Sep 4, 2022

oribon Sep 8, 2022

jcaamano left a comment

jcaamano Sep 2, 2022 •

edited

oribon Sep 4, 2022

jcaamano Sep 2, 2022

flavio-fernandes left a comment

flavio-fernandes Sep 2, 2022

oribon Sep 4, 2022

oribon Sep 5, 2022

trozet left a comment

Implement Egress Services #3135

Implement Egress Services #3135

Conversation

oribon commented Aug 31, 2022 • edited

coveralls commented Aug 31, 2022 • edited

trozet 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

jcaamano left a comment

Choose a reason for hiding this comment

jcaamano Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavio-fernandes 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

trozet left a comment

Choose a reason for hiding this comment

oribon commented Aug 31, 2022 •

edited

coveralls commented Aug 31, 2022 •

edited

jcaamano Sep 2, 2022 •

edited