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

@@ -39,6 +39,7 @@ const (
 	IpBlockIndexKey       ExternalIDKey = "ip-block-index"
 	RuleIndex             ExternalIDKey = "rule-index"
 	CIDRKey               ExternalIDKey = types.OvnK8sPrefix + "/cidr"
+	PortPolicyProtocolKey ExternalIDKey = "port-policy-protocol"
 )
 
 // ObjectIDsTypes should only be created here
@@ -129,21 +130,22 @@ var ACLNetpolNode = newObjectIDsType(acl, NetpolNodeOwnerType, []ExternalIDKey{
 
 // ACLNetworkPolicy define a unique index for every network policy ACL.
 // ingress/egress + NetworkPolicy[In/E]gressRule idx - defines given gressPolicy.
-// ACLs are created for every gp.portPolicies:
+// ACLs are created for gp.portPolicies which are grouped by protocol:
 // - for empty policy (no selectors and no ip blocks) - empty ACL (see allIPsMatch)
 // OR
 // - all selector-based peers ACL
 // - for every IPBlock +1 ACL
-// Therefore unique id for a given gressPolicy is portPolicy idx + IPBlock idx
-// (empty policy and all selector-based peers ACLs will have idx=-1)
+// Therefore unique id for a given gressPolicy is protocol name + IPBlock idx
+// (protocol will be "None" if no port policy is defined, and empty policy and all
+// selector-based peers ACLs will have idx=-1)
 var ACLNetworkPolicy = newObjectIDsType(acl, NetworkPolicyOwnerType, []ExternalIDKey{
 	// policy namespace+name
 	ObjectNameKey,
 	// egress or ingress
 	PolicyDirectionKey,
 	// gress rule index
 	GressIdxKey,
-	PortPolicyIndexKey,
+	PortPolicyProtocolKey,
 	IpBlockIndexKey,
 })
 

go-controller/pkg/ovn/base_network_controller_policy.go

@@ -1039,7 +1039,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

go-controller/pkg/ovn/external_ids_syncer/acl/acl_sync.go

@@ -304,7 +304,7 @@ func (syncer *aclSyncer) updateStaleNetpolNodeACLs(legacyACLs []*nbdb.ACL, exist
 
 func (syncer *aclSyncer) getNetpolGressACLDbIDs(policyNamespace, policyName, policyType string,
 	gressIdx, portPolicyIdx, ipBlockIdx int) *libovsdbops.DbObjectIDs {
-	return libovsdbops.NewDbObjectIDs(libovsdbops.ACLNetworkPolicy, syncer.controllerName,
+	objIDs := libovsdbops.NewDbObjectIDs(libovsdbops.ACLNetworkPolicy, syncer.controllerName,
 		map[libovsdbops.ExternalIDKey]string{
 			// policy namespace+name
 			libovsdbops.ObjectNameKey: policyNamespace + ":" + policyName,
@@ -319,9 +319,13 @@ func (syncer *aclSyncer) getNetpolGressACLDbIDs(policyNamespace, policyName, pol
 			// - 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.IpBlockIndexKey: strconv.Itoa(ipBlockIdx),
+			// assign portPolicyIdx to protocol to generate backward compatible key
+			libovsdbops.PortPolicyProtocolKey: strconv.Itoa(portPolicyIdx),
 		})
+	// add port-policy-index in case it is referenced in acl syncer for legacy rules
+	objIDs.GetExternalIDs()[libovsdbops.PortPolicyIndexKey.String()] = strconv.Itoa(portPolicyIdx)
+	return objIDs
 }
 
 func (syncer *aclSyncer) updateStaleGressPolicies(legacyACLs []*nbdb.ACL) (updatedACLs []*nbdb.ACL, err error) {

go-controller/pkg/ovn/gress_policy.go

@@ -14,13 +14,17 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
 	knet "k8s.io/api/networking/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"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 is used to create ACL for gressPolicy that doesn't have ipBlocks
 	emptyIdx = -1
+	// emptyProtocol is used to create ACL for gressPolicy that doesn't have port policies hence no protocols
+	emptyProtocol = "None"
 )
 
 type gressPolicy struct {
@@ -62,25 +66,58 @@ 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 = sets.NewString(TCP, UDP, SCTP)
+
+func (gp *gressPolicy) getProtocolPortsMap() map[string]*gressPolicyPorts {
+	gressProtoPortsMap := make(map[string]*gressPolicyPorts)
+	for _, pp := range gp.portPolicies {
+		if found := supportedProtocols.Has(pp.protocol); !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, ",")))
 	}
-	return foundProtocol, nil
+	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 l4Match
 }
 
 func newGressPolicy(policyType knet.PolicyType, idx int, namespace, name, controllerName string, isNetPolStateless bool, netInfo util.BasicNetInfo) *gressPolicy {
@@ -295,37 +332,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 +372,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 +401,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
@@ -384,14 +410,16 @@ func (gp *gressPolicy) getNetpolACLDbIDs(portPolicyIdx, ipBlockIdx int) *libovsd
 			libovsdbops.PolicyDirectionKey: string(gp.policyType),
 			// gress rule index
 			libovsdbops.GressIdxKey: strconv.Itoa(gp.idx),
-			// acls are created for every gp.portPolicies:
+			// acls are created for every gp.portPolicies which are grouped by protocol:
 			// - for empty policy (no selectors and no ip blocks) - empty ACL
 			// OR
 			// - all selector-based peers ACL
 			// - 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),
+			// Therefore unique id for a given gressPolicy is protocol name + IPBlock idx
+			// (protocol will be "None" if no port policy is defined, and empty policy and all
+			// selector-based peers ACLs will have idx=-1)
+			libovsdbops.IpBlockIndexKey: strconv.Itoa(ipBlockIdx),
+			// protocol key
+			libovsdbops.PortPolicyProtocolKey: protocol,
 		})
 }