Consolidate several L4 OVN ACLs into smaller number of OVN ACLs

If an ingress rule has 5 TCP port specified along with a namespace
selector, then we end up creating 5 OVN ACLs. Instead, we could create
just one ACL by consoldiating the individual ports and/or ranges. For
example:

With this Network Policy,

spec:
  ingress:
    - from:
      - nameSelector:
          matchLabels:
            name: web
      ports:
        - protocol: TCP
          port: 3306
        - protocol: TCP
          port: 80
        - protocol: TCP
          port: 9000
          endPort: 9100

we end up creating following 3 matches
(ip4.src == {$as1} && tcp && tcp.dst==3306 && outport == @pg1)
(ip4.src == {$as1} && tcp && tcp.dst==80 && outport == @pg1)
(ip4.src == {$as1} && tcp && 9000<=tcp.dst<=9100 && outport == @pg1)

With the fix in this commit, we will have only one match
(ip4.src == {$as1} && tcp && (tcp.dst=={3306,80} || \
  9000<=tcp.dst<=9100 && outport == @pg1)

With reduced number of OVN ACLs will result in fewer logical flows
and few OpenFlow flows

Signed-off-by: Xiaobin Qu <[email protected]>
Co-authored-by: Girish Moodalbail <[email protected]>

go-controller/pkg/libovsdbops/db_object_types.go

@@ -34,6 +34,7 @@ const (
 	PortPolicyIndexKey    ExternalIDKey = "port-policy-index"
 	IpBlockIndexKey       ExternalIDKey = "ip-block-index"
 	RuleIndex             ExternalIDKey = "rule-index"
+	PortPolicyProtocolKey ExternalIDKey = "port-policy-protocol"
 )
 
 // ObjectIDsTypes should only be created here
@@ -138,8 +139,8 @@ var ACLNetworkPolicy = newObjectIDsType(acl, NetworkPolicyOwnerType, []ExternalI
 	PolicyDirectionKey,
 	// gress rule index
 	GressIdxKey,
-	PortPolicyIndexKey,
 	IpBlockIndexKey,
+	PortPolicyProtocolKey,
 })
 
 var ACLNetpolNamespace = newObjectIDsType(acl, NetpolNamespaceOwnerType, []ExternalIDKey{

go-controller/pkg/ovn/base_network_controller_policy.go

@@ -995,6 +995,12 @@ func (bnc *BaseNetworkController) createNetworkPolicy(policy *knet.NetworkPolicy
 			return fmt.Errorf("failed to create ops to add port to a port group: %v", err)
 		}
 
+		// Clean up stale ACLs indexed by port policy number
+		ops, err = bnc.cleanupPortPolicyIndexedACLs(np, ops)
+		if err != nil {
+			return fmt.Errorf("failed to clean up stale ACLs for %s/%s: %v", np.namespace, np.name, err)
+		}
+
 		var recordOps []ovsdb.Operation
 		var txOkCallBack func()
 		recordOps, txOkCallBack, _, err = bnc.AddConfigDurationRecord("networkpolicy", policy.Namespace, policy.Name)
@@ -1029,7 +1035,6 @@ func (bnc *BaseNetworkController) createNetworkPolicy(policy *knet.NetworkPolicy
 		if err != nil {
 			return fmt.Errorf("failed to start local pod handler: %v", err)
 		}
-
 		return nil
 	})
 	return np, err
@@ -1463,6 +1468,27 @@ func (bnc *BaseNetworkController) shutdownHandlers(np *networkPolicy) {
 	np.nsHandlerList = make([]*factory.Handler, 0)
 }
 
+func (bnc *BaseNetworkController) cleanupPortPolicyIndexedACLs(np *networkPolicy, ops []ovsdb.Operation) ([]ovsdb.Operation, error) {
+	predicateForIndexedACL := func(item *nbdb.ACL) bool {
+		if item.GetExternalIDs()[libovsdbops.OwnerControllerKey.String()] != bnc.controllerName ||
+			item.GetExternalIDs()[libovsdbops.ObjectNameKey.String()] != getACLPolicyKey(np.namespace, np.name) ||
+			item.GetExternalIDs()[libovsdbops.OwnerTypeKey.String()] != string(libovsdbops.NetworkPolicyOwnerType) {
+			return false
+		}
+		// return true if port-policy-index is in external IDs
+		return item.GetExternalIDs()[libovsdbops.PortPolicyIndexKey.String()] != ""
+	}
+	indexedACLs, err := libovsdbops.FindACLsWithPredicate(bnc.nbClient, predicateForIndexedACL)
+	if err != nil {
+		return nil, err
+	}
+	delOps, err := libovsdbops.DeleteACLsFromPortGroupOps(bnc.nbClient, nil, np.portGroupName, indexedACLs...)
+	if err != nil {
+		return nil, err
+	}
+	return append(ops, delOps...), nil
+}
+
 // The following 2 functions should return the same key for network policy based on k8s on internal networkPolicy object
 func getPolicyKey(policy *knet.NetworkPolicy) string {
 	return fmt.Sprintf("%v/%v", policy.Namespace, policy.Name)

go-controller/pkg/ovn/default_network_controller.go

@@ -437,6 +437,9 @@ func (oc *DefaultNetworkController) Run(ctx context.Context) error {
 	if err := WithSyncDurationMetric("network policy", oc.WatchNetworkPolicy); err != nil {
 		return err
 	}
+	// now that we have added all the OVN ACLs with optimization, it is time to remove the stale OVN
+	// ACL entries from the database
+	//aclsyncer.NewACLSyncer(oc.nbClient, oc.controllerName).CleanStaleNetworkPolicy()
 
 	if config.OVNKubernetesFeature.EnableEgressIP {
 		// This is probably the best starting order for all egress IP handlers.

go-controller/pkg/ovn/gress_policy.go

@@ -14,13 +14,16 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
 	knet "k8s.io/api/networking/v1"
+	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
 )
 
 const (
 	noneMatch = "None"
 	// emptyIdx is used to create ACL for gressPolicy that doesn't have ports or ipBlocks
 	emptyIdx = -1
+	// emptyProtocol
+	emptyProtocol = "None"
 )
 
 type gressPolicy struct {
@@ -62,25 +65,65 @@ type portPolicy struct {
 	endPort  int32
 }
 
-func (pp *portPolicy) getL4Match() (string, error) {
-	var supportedProtocols = []string{TCP, UDP, SCTP}
-	var foundProtocol string
-	for _, protocol := range supportedProtocols {
-		if protocol == pp.protocol {
-			foundProtocol = strings.ToLower(pp.protocol)
-			break
+// for a given ingress/egress rule, captures all the provided port ranges and
+// individual ports
+type gressPolicyPorts struct {
+	portList  []string // list of provided ports as string
+	portRange []string // list of provided port ranges in OVN ACL format
+}
+
+var supportedProtocols = []string{TCP, UDP, SCTP}
+
+func (gp *gressPolicy) getProtocolPortsMap() map[string]*gressPolicyPorts {
+	gressProtoPortsMap := make(map[string]*gressPolicyPorts)
+	for _, pp := range gp.portPolicies {
+		var found bool
+		for _, protocol := range supportedProtocols {
+			if protocol == pp.protocol {
+				found = true
+				break
+			}
+		}
+		if !found {
+			klog.Warningf("Unknown protocol %v, while processing network policy %s/%s",
+				pp.protocol, gp.policyNamespace, gp.policyName)
+			continue
+		}
+		protocol := strings.ToLower(pp.protocol)
+		gpp, ok := gressProtoPortsMap[protocol]
+		if !ok {
+			gpp = &gressPolicyPorts{portList: []string{}, portRange: []string{}}
+			gressProtoPortsMap[protocol] = gpp
+		}
+		if pp.endPort != 0 && pp.endPort != pp.port {
+			gpp.portRange = append(gpp.portRange, fmt.Sprintf("%d<=%s.dst<=%d", pp.port, protocol, pp.endPort))
+		} else if pp.port != 0 {
+			gpp.portList = append(gpp.portList, fmt.Sprintf("%d", pp.port))
 		}
 	}
-	if len(foundProtocol) == 0 {
-		return "", fmt.Errorf("unknown port protocol %v", pp.protocol)
-	}
-	if pp.endPort != 0 && pp.endPort != pp.port {
-		return fmt.Sprintf("%s && %d<=%s.dst<=%d", foundProtocol, pp.port, foundProtocol, pp.endPort), nil
+	return gressProtoPortsMap
+}
 
-	} else if pp.port != 0 {
-		return fmt.Sprintf("%s && %s.dst==%d", foundProtocol, foundProtocol, pp.port), nil
+func getL4Match(protocol string, ports *gressPolicyPorts) string {
+	allL4Matches := []string{}
+	if len(ports.portList) > 0 {
+		// if there is just one port, then don't use `{}`
+		template := "%s.dst==%s"
+		if len(ports.portList) > 1 {
+			template = "%s.dst=={%s}"
+		}
+		allL4Matches = append(allL4Matches, fmt.Sprintf(template, protocol, strings.Join(ports.portList, ",")))
+	}
+	allL4Matches = append(allL4Matches, ports.portRange...)
+	l4Match := protocol
+	if len(allL4Matches) > 0 {
+		template := "%s && %s"
+		if len(allL4Matches) > 1 {
+			template = "%s && (%s)"
+		}
+		l4Match = fmt.Sprintf(template, protocol, strings.Join(allL4Matches, " || "))
 	}
-	return foundProtocol, nil
+	return l4Match
 }
 
 func newGressPolicy(policyType knet.PolicyType, idx int, namespace, name, controllerName string, isNetPolStateless bool, netInfo util.BasicNetInfo) *gressPolicy {
@@ -295,37 +338,26 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
 	} else {
 		lportMatch = fmt.Sprintf("inport == @%s", portGroupName)
 	}
-	var portPolicyIdxs []int
-	if len(gp.portPolicies) == 0 {
-		portPolicyIdxs = []int{emptyIdx}
-	} else {
-		portPolicyIdxs = make([]int, 0, len(gp.portPolicies))
-		for i := 0; i < len(gp.portPolicies); i++ {
-			portPolicyIdxs = append(portPolicyIdxs, i)
-		}
-	}
 	var l4Match string
-	var err error
 	action := nbdb.ACLActionAllowRelated
 	if gp.isNetPolStateless {
 		action = nbdb.ACLActionAllowStateless
 	}
-	for _, portPolIdx := range portPolicyIdxs {
-		if portPolIdx != emptyIdx {
-			portPol := gp.portPolicies[portPolIdx]
-			l4Match, err = portPol.getL4Match()
-			if err != nil {
-				continue
-			}
-		} else {
-			l4Match = noneMatch
+	protocolPortsMap := gp.getProtocolPortsMap()
+	if len(protocolPortsMap) == 0 {
+		protocolPortsMap[emptyProtocol] = nil
+	}
+	for protocol, ports := range protocolPortsMap {
+		l4Match = noneMatch
+		if ports != nil {
+			l4Match = getL4Match(protocol, ports)
 		}
 
 		if len(gp.ipBlocks) > 0 {
 			// Add ACL allow rule for IPBlock CIDR
 			ipBlockMatches := gp.getMatchFromIPBlock(lportMatch, l4Match)
 			for ipBlockIdx, ipBlockMatch := range ipBlockMatches {
-				aclIDs := gp.getNetpolACLDbIDs(portPolIdx, ipBlockIdx)
+				aclIDs := gp.getNetpolACLDbIDs(ipBlockIdx, protocol)
 				acl := BuildACL(aclIDs, types.DefaultAllowPriority, ipBlockMatch, action,
 					aclLogging, gp.aclPipeline)
 				createdACLs = append(createdACLs, acl)
@@ -346,7 +378,7 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
 			} else {
 				addrSetMatch = fmt.Sprintf("%s && %s && %s", l3Match, l4Match, lportMatch)
 			}
-			aclIDs := gp.getNetpolACLDbIDs(portPolIdx, emptyIdx)
+			aclIDs := gp.getNetpolACLDbIDs(emptyIdx, protocol)
 			acl := BuildACL(aclIDs, types.DefaultAllowPriority, addrSetMatch, action,
 				aclLogging, gp.aclPipeline)
 			if l3Match == "" {
@@ -375,7 +407,7 @@ func parseACLPolicyKey(aclPolicyKey string) (string, string, error) {
 	return s[0], s[1], nil
 }
 
-func (gp *gressPolicy) getNetpolACLDbIDs(portPolicyIdx, ipBlockIdx int) *libovsdbops.DbObjectIDs {
+func (gp *gressPolicy) getNetpolACLDbIDs(ipBlockIdx int, protocol string) *libovsdbops.DbObjectIDs {
 	return libovsdbops.NewDbObjectIDs(libovsdbops.ACLNetworkPolicy, gp.controllerName,
 		map[libovsdbops.ExternalIDKey]string{
 			// policy namespace+name
@@ -391,7 +423,9 @@ func (gp *gressPolicy) getNetpolACLDbIDs(portPolicyIdx, ipBlockIdx int) *libovsd
 			// - for every IPBlock +1 ACL
 			// Therefore unique id for given gressPolicy is portPolicy idx + IPBlock idx
 			// (empty policy and all selector-based peers ACLs will have idx=-1)
-			libovsdbops.PortPolicyIndexKey: strconv.Itoa(portPolicyIdx),
-			libovsdbops.IpBlockIndexKey:    strconv.Itoa(ipBlockIdx),
+			//libovsdbops.PortPolicyIndexKey: strconv.Itoa(portPolicyIdx),
+			libovsdbops.IpBlockIndexKey: strconv.Itoa(ipBlockIdx),
+			// protocol key
+			libovsdbops.PortPolicyProtocolKey: protocol,
 		})
 }