OCPBUGS-1750: Trim ACL names according to RFC1123

go-controller/pkg/libovsdbops/acl.go

@@ -2,6 +2,7 @@ package libovsdbops
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 
@@ -159,8 +160,24 @@ func UpdateACLsDirection(nbClient libovsdbclient.Client, acls ...*nbdb.ACL) erro
 	return err
 }
 
-// DeleteACLs deletes the provided ACLs
-func DeleteACLs(nbClient libovsdbclient.Client, acls ...*nbdb.ACL) error {
+// DeleteACLsOps deletes the provided ACLs and returns the corresponding ops
+// portGroupNames and switchPred reminds to delete ACL references for port groups or switches
+// in case port group or switch is not completely deleted
+func DeleteACLsOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation,
+	portGroupNames []string, switchPred switchPredicate, acls ...*nbdb.ACL) ([]libovsdb.Operation, error) {
+	var err error
+	for _, portGroupName := range portGroupNames {
+		ops, err = DeleteACLsFromPortGroupOps(nbClient, ops, portGroupName, acls...)
+		if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
+			return ops, fmt.Errorf("deleting ACLs from port group %s failed: %v", portGroupName, err)
+		}
+	}
+	if switchPred != nil {
+		ops, err = RemoveACLsFromLogicalSwitchesWithPredicateOps(nbClient, ops, switchPred, acls...)
+		if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
+			return ops, fmt.Errorf("deleting ACLs from switch with predicate failed: %v", err)
+		}
+	}
 	opModels := make([]operationModel, 0, len(acls))
 	for i := range acls {
 		// can't use i in the predicate, for loop replaces it in-memory
@@ -175,5 +192,18 @@ func DeleteACLs(nbClient libovsdbclient.Client, acls ...*nbdb.ACL) error {
 	}
 
 	modelClient := newModelClient(nbClient)
-	return modelClient.Delete(opModels...)
+	return modelClient.DeleteOps(ops, opModels...)
+}
+
+// DeleteACLs deletes the provided ACLs
+// portGroupNames and switchPred reminds to delete ACL references for port groups or switches
+// in case port group or switch is not completely deleted
+func DeleteACLs(nbClient libovsdbclient.Client, portGroupNames []string, switchPred switchPredicate, acls ...*nbdb.ACL) error {
+	ops, err := DeleteACLsOps(nbClient, nil, portGroupNames, switchPred, acls...)
+	if err != nil {
+		return err
+	}
+
+	_, err = TransactAndCheck(nbClient, ops)
+	return err
 }

go-controller/pkg/libovsdbops/portgroup.go

@@ -1,11 +1,26 @@
 package libovsdbops
 
 import (
+	"context"
+
 	libovsdbclient "github.com/ovn-org/libovsdb/client"
 	libovsdb "github.com/ovn-org/libovsdb/ovsdb"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
 )
 
+type portGroupPredicate func(group *nbdb.PortGroup) bool
+
+// FindPortGroupsWithPredicate looks up port groups from the cache based on a
+// given predicate
+func FindPortGroupsWithPredicate(nbClient libovsdbclient.Client, p portGroupPredicate) ([]*nbdb.PortGroup, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), types.OVSDBTimeout)
+	defer cancel()
+	found := []*nbdb.PortGroup{}
+	err := nbClient.WhereCache(p).List(ctx, &found)
+	return found, err
+}
+
 // BuildPortGroup builds a port group referencing the provided ports and ACLs
 func BuildPortGroup(hashName, name string, ports []*nbdb.LogicalSwitchPort, acls []*nbdb.ACL) *nbdb.PortGroup {
 	pg := nbdb.PortGroup{

go-controller/pkg/libovsdbops/switch.go

@@ -144,9 +144,11 @@ func AddACLsToLogicalSwitchOps(nbClient libovsdbclient.Client, ops []libovsdb.Op
 	return m.CreateOrUpdateOps(ops, opModels)
 }
 
-// RemoveACLsFromLogicalSwitchesWithPredicate looks up logical switches from the cache
-// based on a given predicate and removes from them the provided ACLs
-func RemoveACLsFromLogicalSwitchesWithPredicate(nbClient libovsdbclient.Client, p switchPredicate, acls ...*nbdb.ACL) error {
+// RemoveACLsFromLogicalSwitchesWithPredicateOps looks up logical switches from the cache
+// based on a given predicate, removes from them the provided ACLs, and returns the
+// corresponding ops
+func RemoveACLsFromLogicalSwitchesWithPredicateOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation,
+	p switchPredicate, acls ...*nbdb.ACL) ([]libovsdb.Operation, error) {
 	sw := nbdb.LogicalSwitch{
 		ACLs: make([]string, 0, len(acls)),
 	}
@@ -162,7 +164,18 @@ func RemoveACLsFromLogicalSwitchesWithPredicate(nbClient libovsdbclient.Client,
 	}
 
 	m := newModelClient(nbClient)
-	return m.Delete(opModel)
+	return m.DeleteOps(ops, opModel)
+}
+
+// RemoveACLsFromLogicalSwitchesWithPredicate looks up logical switches from the cache
+// based on a given predicate and removes from them the provided ACLs
+func RemoveACLsFromLogicalSwitchesWithPredicate(nbClient libovsdbclient.Client, p switchPredicate, acls ...*nbdb.ACL) error {
+	ops, err := RemoveACLsFromLogicalSwitchesWithPredicateOps(nbClient, nil, p, acls...)
+	if err != nil {
+		return err
+	}
+	_, err = TransactAndCheck(nbClient, ops)
+	return err
 }
 
 // UpdateLogicalSwitchSetOtherConfig sets other config on the provided logical

go-controller/pkg/ovn/egressfirewall.go

@@ -375,15 +375,10 @@ func (oc *Controller) deleteEgressFirewallRules(externalID string) error {
 		return nil
 	}
 
-	// delete egress firewall acls off any logical switch which has it
+	// delete egress firewall acls off any logical switch which has it,
+	// then manually remove the egressFirewall ACLs instead of relying on ovsdb garbage collection to do so
 	pSwitch := func(item *nbdb.LogicalSwitch) bool { return true }
-	err = libovsdbops.RemoveACLsFromLogicalSwitchesWithPredicate(oc.nbClient, pSwitch, egressFirewallACLs...)
-	if err != nil {
-		return fmt.Errorf("failed to remove reject acl from logical switches: %v", err)
-	}
-
-	// Manually remove the egressFirewall ACLs instead of relying on ovsdb garbage collection to do so
-	err = libovsdbops.DeleteACLs(oc.nbClient, egressFirewallACLs...)
+	err = libovsdbops.DeleteACLs(oc.nbClient, nil, pSwitch, egressFirewallACLs...)
 	if err != nil {
 		return err
 	}

go-controller/pkg/ovn/policy.go

@@ -49,6 +49,12 @@ const (
 	egressDefaultDenySuffix = "egressDefaultDeny"
 	// arpAllowPolicySuffix is the suffix used when creating default ACLs for a namespace
 	arpAllowPolicySuffix = "ARPallowPolicy"
+	// arpAllowPolicyMatch is the match used when creating default allow ARP ACLs for a namespace
+	arpAllowPolicyMatch = "(arp || nd)"
+	// staleArpAllowPolicyMatch "was" the old match used when creating default allow ARP ACLs for a namespace
+	// NOTE: This is succeed by arpAllowPolicyMatch to allow support for IPV6. This is currently only
+	// used when removing stale ACLs from the syncNetworkPolicy function and should NOT be used in any main logic.
+	staleArpAllowPolicyMatch = "arp"
 )
 
 var NetworkPolicyNotCreated error
@@ -121,32 +127,63 @@ func hashedPortGroup(s string) string {
 func (oc *Controller) updateStaleDefaultDenyACLNames(npType knet.PolicyType, gressSuffix string) error {
 	cleanUpDefaultDeny := make(map[string][]*nbdb.ACL)
 	p := func(item *nbdb.ACL) bool {
-		return item.ExternalIDs[defaultDenyPolicyTypeACLExtIdKey] == string(npType) && // default-deny-policy-type:Egress or default-deny-policy-type:Ingress
-			strings.Contains(item.Match, gressSuffix) && // Match:inport ==	@ablah80448_egressDefaultDeny or Match:inport == @ablah80448_ingressDefaultDeny
-			!strings.Contains(*item.Name, arpAllowPolicySuffix) && // != name: namespace_ARPallowPolicy
-			!strings.Contains(*item.Name, gressSuffix) // filter out already converted ACLs
+		if item.Name != nil { // we don't care about node ACLs
+			aclNameSuffix := strings.Split(*item.Name, "_")
+			if len(aclNameSuffix) == 1 {
+				// doesn't have suffix; no update required; append the actual suffix since this ACL can be skipped
+				aclNameSuffix = append(aclNameSuffix, gressSuffix)
+			}
+			return item.ExternalIDs[defaultDenyPolicyTypeACLExtIdKey] == string(npType) && // default-deny-policy-type:Egress or default-deny-policy-type:Ingress
+				strings.Contains(item.Match, gressSuffix) && // Match:inport ==	@ablah80448_egressDefaultDeny or Match:inport == @ablah80448_ingressDefaultDeny
+				!strings.Contains(item.Match, arpAllowPolicyMatch) && // != match: (arp || nd)
+				!strings.HasPrefix(gressSuffix, aclNameSuffix[1]) // filter out already converted ACLs or ones that are a no-op
+		}
+		return false
 	}
 	gressACLs, err := libovsdbops.FindACLsWithPredicate(oc.nbClient, p)
 	if err != nil {
 		return fmt.Errorf("cannot find NetworkPolicy default deny ACLs: %v", err)
 	}
 	for _, acl := range gressACLs {
 		acl := acl
-		namespace := strings.Split(*acl.Name, "_")[0] // parse the namespace from the ACL name
+		// parse the namespace.Name from the ACL name (if ACL name is 63 chars, then it will fully be namespace.Name)
+		namespace := strings.Split(*acl.Name, "_")[0]
 		cleanUpDefaultDeny[namespace] = append(cleanUpDefaultDeny[namespace], acl)
 	}
 	// loop through the cleanUp map and per namespace update the first ACL's name and delete the rest
 	for namespace, aclList := range cleanUpDefaultDeny {
 		newName := namespacePortGroupACLName(namespace, "", gressSuffix)
 		if len(aclList) > 1 {
 			// this should never be the case but delete everything except 1st ACL
-			err := libovsdbops.DeleteACLs(oc.nbClient, aclList[1:]...)
+			ingressPGName := defaultDenyPortGroup(namespace, ingressDefaultDenySuffix)
+			egressPGName := defaultDenyPortGroup(namespace, egressDefaultDenySuffix)
+			err := libovsdbops.DeleteACLs(oc.nbClient, []string{ingressPGName, egressPGName}, nil, aclList[1:]...)
 			if err != nil {
 				return err
 			}
 		}
-		aclList[0].Name = &newName
-		err := libovsdbops.CreateOrUpdateACLs(oc.nbClient, aclList[0])
+		meter := ""
+		if aclList[0].Meter != nil {
+			meter = *aclList[0].Meter
+		}
+		severity := ""
+		if aclList[0].Severity != nil {
+			severity = *aclList[0].Severity
+		}
+		newACL := libovsdbops.BuildACL(
+			newName, // this is the only thing we need to change, keep the rest same
+			aclList[0].Direction,
+			aclList[0].Priority,
+			aclList[0].Match,
+			aclList[0].Action,
+			meter,
+			severity,
+			aclList[0].Log,
+			aclList[0].ExternalIDs,
+			aclList[0].Options,
+		)
+		newACL.UUID = aclList[0].UUID // for performance
+		err := libovsdbops.CreateOrUpdateACLs(oc.nbClient, newACL)
 		if err != nil {
 			return fmt.Errorf("cannot update old NetworkPolicy ACLs for namespace %s: %v", namespace, err)
 		}
@@ -230,50 +267,48 @@ func (oc *Controller) syncNetworkPolicies(networkPolicies []interface{}) error {
 		}
 	}
 
-	if err := oc.updateStaleDefaultDenyACLNames(knet.PolicyTypeEgress, egressDefaultDenySuffix); err != nil {
-		return fmt.Errorf("cannot clean up egress default deny ACL name: %v", err)
-	}
-	if err := oc.updateStaleDefaultDenyACLNames(knet.PolicyTypeIngress, ingressDefaultDenySuffix); err != nil {
-		return fmt.Errorf("cannot clean up ingress default deny ACL name: %v", err)
-	}
-
 	// remove stale egress and ingress allow arp ACLs that were leftover as a result
 	// of ACL migration for "ARPallowPolicy" when the match changed from "arp" to "(arp || nd)"
 	p = func(item *nbdb.ACL) bool {
-		return strings.Contains(item.Match, " && arp") &&
-			strings.Contains(*item.Name, arpAllowPolicySuffix)
+		return strings.Contains(item.Match, " && "+staleArpAllowPolicyMatch) &&
+			// default-deny-policy-type:Egress or default-deny-policy-type:Ingress
+			(item.ExternalIDs[defaultDenyPolicyTypeACLExtIdKey] == string(knet.PolicyTypeEgress) ||
+				item.ExternalIDs[defaultDenyPolicyTypeACLExtIdKey] == string(knet.PolicyTypeIngress))
 	}
 	gressACLs, err := libovsdbops.FindACLsWithPredicate(oc.nbClient, p)
 	if err != nil {
 		return fmt.Errorf("cannot find stale arp allow ACLs: %v", err)
 	}
 	// Remove these stale ACLs from port groups and then delete them
+	var ops []ovsdb.Operation
 	for _, gressACL := range gressACLs {
 		gressACL := gressACL
 		pgName := ""
 		if strings.Contains(gressACL.Match, "inport") {
 			// egress default ARP allow policy ("inport == @a16323395479447859119_egressDefaultDeny && arp")
 			pgName = strings.TrimPrefix(gressACL.Match, "inport == @")
 		} else if strings.Contains(gressACL.Match, "outport") {
-			// ingress default ARP allow policy ("outport == @a16323395479447859119_egressDefaultDeny && arp")
+			// ingress default ARP allow policy ("outport == @a16323395479447859119_ingressDefaultDeny && arp")
 			pgName = strings.TrimPrefix(gressACL.Match, "outport == @")
 		}
-		pgName = strings.TrimSuffix(pgName, " && arp")
-		ops, err := libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, nil, pgName, gressACL)
-		if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
-			return fmt.Errorf("cannot construct ops to delete ACL %+v from portgroup %s: %v",
-				gressACL, pgName, err)
-		}
-		_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
+		pgName = strings.TrimSuffix(pgName, " && "+staleArpAllowPolicyMatch)
+		ops, err = libovsdbops.DeleteACLsOps(oc.nbClient, ops, []string{pgName}, nil, gressACL)
 		if err != nil {
-			return fmt.Errorf("cannot delete ACL %+v from portgroup %s: %v", gressACL, pgName, err)
+			return fmt.Errorf("failed getting delete acl ops: %v", err)
 		}
 	}
-	err = libovsdbops.DeleteACLs(oc.nbClient, gressACLs...)
+	_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
 	if err != nil {
 		return fmt.Errorf("cannot delete stale arp allow ACLs: %v", err)
 	}
 
+	if err := oc.updateStaleDefaultDenyACLNames(knet.PolicyTypeEgress, egressDefaultDenySuffix); err != nil {
+		return fmt.Errorf("cannot clean up egress default deny ACL name: %v", err)
+	}
+	if err := oc.updateStaleDefaultDenyACLNames(knet.PolicyTypeIngress, ingressDefaultDenySuffix); err != nil {
+		return fmt.Errorf("cannot clean up ingress default deny ACL name: %v", err)
+	}
+
 	return nil
 }
 
@@ -358,7 +393,7 @@ func defaultDenyPortGroup(namespace, gressSuffix string) string {
 
 func buildDenyACLs(namespace, policy, pg, aclLogging string, policyType knet.PolicyType) (denyACL, allowACL *nbdb.ACL) {
 	denyMatch := getACLMatch(pg, "", policyType)
-	allowMatch := getACLMatch(pg, "(arp || nd)", policyType)
+	allowMatch := getACLMatch(pg, arpAllowPolicyMatch, policyType)
 	if policyType == knet.PolicyTypeIngress {
 		denyACL = buildACL(namespace, pg, ingressDefaultDenySuffix, nbdb.ACLDirectionToLport, types.DefaultDenyPriority, denyMatch, nbdb.ACLActionDrop, aclLogging, policyType)
 		allowACL = buildACL(namespace, pg, arpAllowPolicySuffix, nbdb.ACLDirectionToLport, types.DefaultAllowPriority, allowMatch, nbdb.ACLActionAllow, "", policyType)
@@ -420,19 +455,22 @@ func (oc *Controller) deleteDefaultDenyPGAndACLs(namespace, policy string, nsInf
 	egressDenyACL, egressAllowACL := buildDenyACLs(namespace, policy, egressPGName, aclLogging, knet.PolicyTypeEgress)
 	aclsToBeDeleted = append(aclsToBeDeleted, egressDenyACL, egressAllowACL)
 
-	err := libovsdbops.DeletePortGroups(oc.nbClient, ingressPGName, egressPGName)
+	ops, err := libovsdbops.DeletePortGroupsOps(oc.nbClient, nil, ingressPGName, egressPGName)
 	if err != nil {
 		return err
 	}
-
-	nsInfo.portGroupEgressDenyName = ""
-	nsInfo.portGroupIngressDenyName = ""
-
 	// Manually remove the default ACLs instead of relying on ovsdb garbage collection to do so
-	err = libovsdbops.DeleteACLs(oc.nbClient, aclsToBeDeleted...)
+	// don't delete ACL references because port group is completely deleted in the same tnx
+	ops, err = libovsdbops.DeleteACLsOps(oc.nbClient, ops, nil, nil, aclsToBeDeleted...)
 	if err != nil {
 		return err
 	}
+	_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
+	if err != nil {
+		return fmt.Errorf("failed to transact deleteDefaultDenyPGAndACLs: %v", err)
+	}
+	nsInfo.portGroupEgressDenyName = ""
+	nsInfo.portGroupIngressDenyName = ""
 
 	return nil
 }