Egressfirewall use port groups #3968

npinaeva · 2023-10-17T15:28:46Z

We already have namespace port groups that are created by multicast and updated by pod handlers. That implementation has some races because port group is created conditionally. First commit changes that to create namespace port groups similar to namespace address sets on namespace initialization and update multicast handler to use that.
Second commit updates egress firewall to use namespaced port group instead of cluster port group + address set match to reduce the number of OVS flows.

Namespace port groups can be created conditionally, but that would require namespace locking for every port group-related operation, and may be worse for performance than 1 extra port group per namespace

coveralls · 2023-10-18T08:42:08Z

coverage: 50.527% (+0.1%) from 50.422% when pulling b4d82c2 on npinaeva:egressfirewall-portgroup into 9ccf859 on ovn-org:master.

jcaamano

@npinaeva could you please add a brief explanation or comparison to the PR message on how this reduces the number of OVS flows, for posterity.

go-controller/pkg/ovn/base_network_controller_multicast.go

go-controller/pkg/ovn/base_network_controller_namespace.go

jcaamano · 2023-10-20T15:36:55Z

go-controller/pkg/ovn/namespace.go

-// to add pod's IP to the namespace's address set.
-func (oc *DefaultNetworkController) addPodToNamespace(ns string, ips []*net.IPNet) (*gatewayInfo, map[string]gatewayInfo, []ovsdb.Operation, error) {
+// to add pod's IP to the namespace's address set and port group.
+func (oc *DefaultNetworkController) addPodToNamespace(ns string, ips []*net.IPNet, portUUID string) (*gatewayInfo, map[string]gatewayInfo, []ovsdb.Operation, error) {


Honestly, I would like to avoid caching and moving around the port UUID. I would like for libovsdbops to be able to take the port name instead. But this is more a note to self than a comment for this PR.

jcaamano · 2023-10-20T15:37:52Z

go-controller/pkg/ovn/base_network_controller_namespace.go

@@ -247,6 +277,15 @@ func (bnc *BaseNetworkController) ensureNamespaceLockedCommon(ns string, readOnl
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to create address set for namespace: %s, error: %v", ns, err)
 		}
+
+		// namespace port groups are only used by egress firewall and multicast for now
+		if bnc.multicastSupport || config.OVNKubernetesFeature.EnableEgressFirewall {


If you want a second opinion, I wouldn't bother with this and just have the port at all times. But maybe you have your reasons.

At first, I thought about microshift to avoid extra db load (and I don't even know if those have multicast and EF disabled). But then I realized that this helps me to not rewrite all unit tests, because namespace handler would create a port group for every namespace, and all our tests check the db (might be a good reason to improve the unit test framework of course, but for now it requires a lot of changes). So I thought it doesn't make anything worse, but not a strong opinion :)

go-controller/pkg/ovn/namespace.go

go-controller/pkg/ovn/egressfirewall.go

npinaeva

Thanks a lot for the review @jcaamano , I resolved what was obvious and left some comments to discuss more complicated cases, ptal

go-controller/pkg/ovn/base_network_controller_multicast.go

go-controller/pkg/ovn/base_network_controller_namespace.go

npinaeva · 2023-10-23T09:27:10Z

go-controller/pkg/ovn/base_network_controller_namespace.go

@@ -247,6 +277,15 @@ func (bnc *BaseNetworkController) ensureNamespaceLockedCommon(ns string, readOnl
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to create address set for namespace: %s, error: %v", ns, err)
 		}
+
+		// namespace port groups are only used by egress firewall and multicast for now
+		if bnc.multicastSupport || config.OVNKubernetesFeature.EnableEgressFirewall {


At first, I thought about microshift to avoid extra db load (and I don't even know if those have multicast and EF disabled). But then I realized that this helps me to not rewrite all unit tests, because namespace handler would create a port group for every namespace, and all our tests check the db (might be a good reason to improve the unit test framework of course, but for now it requires a lot of changes). So I thought it doesn't make anything worse, but not a strong opinion :)

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/ovn/namespace.go

npinaeva · 2023-10-25T10:15:50Z

caught a new unit test failure :( #3977

go-controller/pkg/ovn/base_network_controller_namespace.go

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/util/batching/batch.go

jcaamano · 2023-10-26T17:13:21Z

reminder that this might have been missed:
#3968 (review)

npinaeva · 2023-10-27T10:05:36Z

reminder that this might have been missed: #3968 (review)

Oh I did update the commit message here 5027f7a with
Previously, all EF ACLs were applied to the clusterPortGroup, and matched source on address set. In that case, all node will have the same set of OVS flows, matching sources that are not scheduled on that node at all. When using port group as a source match, only flow for local ports will be present on every node.
but now I am thinking you meant some example with specific numbers?

jcaamano · 2023-10-27T11:18:42Z

reminder that this might have been missed: #3968 (review)

Oh I did update the commit message here 5027f7a with Previously, all EF ACLs were applied to the clusterPortGroup, and matched source on address set. In that case, all node will have the same set of OVS flows, matching sources that are not scheduled on that node at all. When using port group as a source match, only flow for local ports will be present on every node. but now I am thinking you meant some example with specific numbers?

Ah, sorry I was just looking and the PR message.

I think this explanation is enough to justify the change. If you have numbers for an example, that would satisfy my curiosity. But if you don't have them, that's ok. I guess it would depend on the content of the address set.

jcaamano · 2023-10-30T10:08:12Z

go-controller/pkg/ovn/egressfirewall.go

+		for namespace, acls := range batchNsACLs {
+			if namespace != "" {
+				pgName := oc.getNamespacePortGroupName(namespace)
+				// delete stale ACLs from namespaced port group
+				if !existingEFNamespaces[namespace] {
+					ops, err = libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, ops, pgName, acls...)
+				} else {
+					// re-attach from ClusterPortGroupNameBase to namespaced port group.
+					// port group should exist, because namespace handler will create it.
+					ops, err = libovsdbops.AddACLsToPortGroupOps(oc.nbClient, ops, pgName, acls...)
+				}
+				if err != nil {
+					return fmt.Errorf("failed to build cleanup ops: %w", err)
+				}
+			}
+			// delete all EF ACLs from ClusterPortGroupNameBase
+			ops, err = libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, ops,
+				oc.getClusterPortGroupName(types.ClusterPortGroupNameBase), acls...)
+			if err != nil {
+				return fmt.Errorf("failed to build cleanup from ClusterPortGroup ops: %w", err)
+			}
+		}


Now that we have one PG for the namespace, and if that PG becomes stale because the namespace was deleted, then the PG will be deleted by the namespace sync function and thus the ACLs will be garbage collected with it, we wouldn't need to explicitly delete those ACLs from here.

You might still be doing this for the UTs, but when talking about potential massive ops I would rather handle it specifically in the UTs than in the real code. In any case it might be appropriate to add a comment.

So overall I am looking at the added startup time here because of unconditional massive ops that we might end up doing. Would it make sense to read the cluster PG, and just move the EF ACLs that are still attached to it? Ideally once this thing, that is costing us time, has been done once, we wouldn't need to do it again.

it's almost right, but the problem is that a namespace may not only be deleted, but also just get an egress firewall deleted, in that case it won't get to existingEFNamespaces and a cleanup is needed. That is why the most part will have to stay, but I added an extra condition for clusterPG to reduce the number of ops.

Okay! My final nit would be to reorganize the method a bit. It contains several migrations from several different versions and the thing is getting big and confusing. I would probably split them out to their own methods for clarity. But I know I am pushing it now, so up to you!

that makes sense :) hope I didn't break anything

jcaamano · 2023-11-02T17:03:24Z

This flake might be new

--- FAIL: TestNetworkPolicyV2Conformance/AdminNetworkPolicyEgressSCTP/Should_support_an_'deny-egress'_policy_for_SCTP_protocol;_ensure_rule_ordering_is_respected (3.27s)

https://github.com/ovn-org/ovn-kubernetes/actions/runs/6731387973/job/18297017440?pr=3968

namespace address set. Existing port groups don't need any change, since port group name always was a namespace name. Set ErrNotFound to false for delete acls and ports from port group ops. Namespace handler will create, update and delete namespace port group similar to the namespace address set. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>

cluster port group + address set match to reduce the number of OVS flows. Previously, all EF ACLs were applied to the clusterPortGroup, and matched source on address set. In that case, all node will have the same set of OVS flows, matching sources that are not scheduled on that node at all. When using port group as a source match, only flow for local ports will be present on every node. The port group is created, updated and deleted by namespace handler similar to the namespace address set. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>

npinaeva force-pushed the egressfirewall-portgroup branch from 0ae5503 to 4f77aa9 Compare October 18, 2023 08:13

npinaeva changed the title ~~Egressfirewall portgroup~~ Egressfirewall use port groups Oct 18, 2023

npinaeva marked this pull request as ready for review October 18, 2023 10:43

npinaeva requested review from trozet, dcbw, girishmg and jcaamano as code owners October 18, 2023 10:43

jcaamano requested changes Oct 20, 2023

View reviewed changes

npinaeva force-pushed the egressfirewall-portgroup branch from 4f77aa9 to ed02df8 Compare October 23, 2023 15:38

npinaeva commented Oct 23, 2023

View reviewed changes

npinaeva force-pushed the egressfirewall-portgroup branch 3 times, most recently from 4a75f10 to 3a655ec Compare October 25, 2023 10:10

jcaamano reviewed Oct 25, 2023

View reviewed changes

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

go-controller/pkg/ovn/egressfirewall.go Show resolved Hide resolved

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

npinaeva force-pushed the egressfirewall-portgroup branch 3 times, most recently from dcee783 to 5027f7a Compare October 26, 2023 13:48

jcaamano reviewed Oct 26, 2023

View reviewed changes

go-controller/pkg/util/batching/batch.go Outdated Show resolved Hide resolved

go-controller/pkg/util/batching/batch.go Outdated Show resolved Hide resolved

npinaeva force-pushed the egressfirewall-portgroup branch from 5027f7a to 71b4c1b Compare October 27, 2023 10:17

npinaeva force-pushed the egressfirewall-portgroup branch 3 times, most recently from 9299909 to d772347 Compare October 30, 2023 09:02

jcaamano reviewed Oct 30, 2023

View reviewed changes

npinaeva force-pushed the egressfirewall-portgroup branch 2 times, most recently from 3710ab2 to b4d82c2 Compare October 31, 2023 10:42

jcaamano previously approved these changes Nov 2, 2023

View reviewed changes

npinaeva dismissed jcaamano’s stale review via ee5aab5 November 2, 2023 10:35

npinaeva force-pushed the egressfirewall-portgroup branch from b4d82c2 to ee5aab5 Compare November 2, 2023 10:35

jcaamano approved these changes Nov 2, 2023

View reviewed changes

npinaeva added 2 commits November 2, 2023 19:27

jcaamano force-pushed the egressfirewall-portgroup branch from ee5aab5 to 8b61856 Compare November 2, 2023 18:27

jcaamano merged commit f94332f into ovn-org:master Nov 3, 2023
29 checks passed

npinaeva mentioned this pull request Nov 17, 2023

[release-4.14] OCPBUGS-23395: Egressfirewall use port groups openshift/ovn-kubernetes#1956

Merged

npinaeva deleted the egressfirewall-portgroup branch December 1, 2023 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egressfirewall use port groups #3968

Egressfirewall use port groups #3968

npinaeva commented Oct 17, 2023 •

edited

coveralls commented Oct 18, 2023 •

edited

jcaamano left a comment

jcaamano Oct 20, 2023

jcaamano Oct 20, 2023

npinaeva Oct 23, 2023

npinaeva left a comment

npinaeva Oct 23, 2023

npinaeva commented Oct 25, 2023

jcaamano commented Oct 26, 2023

npinaeva commented Oct 27, 2023

jcaamano commented Oct 27, 2023

jcaamano Oct 30, 2023

npinaeva Oct 31, 2023

jcaamano Oct 31, 2023

npinaeva Oct 31, 2023

jcaamano commented Nov 2, 2023 •

edited

Egressfirewall use port groups #3968

Egressfirewall use port groups #3968

Conversation

npinaeva commented Oct 17, 2023 • edited

coveralls commented Oct 18, 2023 • edited

jcaamano 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

npinaeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npinaeva commented Oct 25, 2023

jcaamano commented Oct 26, 2023

npinaeva commented Oct 27, 2023

jcaamano commented Oct 27, 2023

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 commented Nov 2, 2023 • edited

npinaeva commented Oct 17, 2023 •

edited

coveralls commented Oct 18, 2023 •

edited

jcaamano commented Nov 2, 2023 •

edited