Create all existing ACLs in tier2

We have a new feature called Hierarchical
ACLs that is introduced in OVN to enable
support for tiered ACLs. This commit ensures
that from this point on, all ACLs for all features
are created in tier2. By default all
new ACLs must be added to tier2.

Ensure existing ACLs without tiers are migrated post upgrade

Since the column in NBDB is an int,
when OVN schema upgrade happens, by default
the value for this column will be set to 0.

We want all existing ACLs to move to tier2.
This commit ensures all existing ACLs for
all features are migrated towards tier2.
This PR ensures that is done by OVNK controller
upon upgrade restart.

Signed-off-by: Surya Seetharaman <[email protected]>

go-controller/pkg/libovsdbops/acl.go

@@ -3,6 +3,7 @@ package libovsdbops
 import (
 	"context"
 	"fmt"
+
 	libovsdbclient "github.com/ovn-org/libovsdb/client"
 	libovsdb "github.com/ovn-org/libovsdb/ovsdb"
 
@@ -21,7 +22,7 @@ func GetACLName(acl *nbdb.ACL) string {
 
 func getACLMutableFields(acl *nbdb.ACL) []interface{} {
 	return []interface{}{&acl.Action, &acl.Direction, &acl.ExternalIDs, &acl.Log, &acl.Match, &acl.Meter,
-		&acl.Name, &acl.Options, &acl.Priority, &acl.Severity}
+		&acl.Name, &acl.Options, &acl.Priority, &acl.Severity, &acl.Tier}
 }
 
 type aclPredicate func(*nbdb.ACL) bool
@@ -63,7 +64,7 @@ func FindACLs(nbClient libovsdbclient.Client, acls []*nbdb.ACL) ([]*nbdb.ACL, er
 
 // BuildACL builds an ACL with empty optional properties unset
 func BuildACL(name string, direction nbdb.ACLDirection, priority int, match string, action nbdb.ACLAction, meter string,
-	severity nbdb.ACLSeverity, log bool, externalIds map[string]string, options map[string]string) *nbdb.ACL {
+	severity nbdb.ACLSeverity, log bool, externalIds map[string]string, options map[string]string, tier int) *nbdb.ACL {
 	name = fmt.Sprintf("%.63s", name)
 
 	var realName *string
@@ -89,6 +90,7 @@ func BuildACL(name string, direction nbdb.ACLDirection, priority int, match stri
 		Meter:       realMeter,
 		ExternalIDs: externalIds,
 		Options:     options,
+		Tier:        tier,
 	}
 
 	return acl

go-controller/pkg/libovsdbops/acl_test.go

@@ -96,6 +96,23 @@ func TestCreateOrUpdateACL(t *testing.T) {
 				Severity:    &aclSev,
 			},
 		},
+		{
+			desc:       "updates Tiers to tier2",
+			initialACL: initialACL,
+			finalACL: &nbdb.ACL{
+				Action:      nbdb.ACLActionAllow,
+				Direction:   nbdb.ACLDirectionToLport,
+				ExternalIDs: nil,
+				Log:         true,
+				Match:       "match",
+				Meter:       &aclMeter,
+				Name:        &aclName,
+				Options:     map[string]string{"key": "value"},
+				Priority:    1,
+				Severity:    &aclSev,
+				Tier:        2, // default tier
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {

go-controller/pkg/ovn/acl.go

@@ -115,6 +115,7 @@ func BuildACL(dbIDs *libovsdbops.DbObjectIDs, priority int, match, action string
 		log,
 		externalIDs,
 		options,
+		types.DefaultACLTier,
 	)
 	return ACL
 }

go-controller/pkg/ovn/egressfirewall_test.go

@@ -143,6 +143,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						purgeIDs.GetExternalIDs(),
 						nil,
+						t.PlaceHolderACLTier,
 					)
 					purgeACL.UUID = "purgeACL-UUID"
 					// no externalIDs present => dbIDs can't be built
@@ -157,6 +158,9 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						nil,
 						nil,
+						// we should not be in a situation where we have ACLs without externalIDs
+						// but if we do have such lame ACLs then they will interfere with AdminNetPol logic
+						t.PlaceHolderACLTier,
 					)
 					purgeACL2.UUID = "purgeACL2-UUID"
 
@@ -184,6 +188,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						updateIDs.GetExternalIDs(),
 						nil,
+						t.PlaceHolderACLTier,
 					)
 					updateACL.UUID = "updateACL-UUID"
 
@@ -199,6 +204,9 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						nil,
 						nil,
+						// we should not be in a situation where we have unknown ACL that doesn't belong to any feature
+						// but if we do have such lame ACLs then they will interfere with AdminNetPol logic
+						t.PlaceHolderACLTier,
 					)
 					ignoreACL.UUID = "ignoreACL-UUID"
 
@@ -238,6 +246,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 					// match shouldn't have cluster exclusion
 					asHash, _ := getNsAddrSetHashNames(namespace1.Name)
 					updateACL.Match = "(ip4.dst == 1.2.3.4/23) && ip4.src == $" + asHash
+					updateACL.Tier = t.DefaultACLTier // ensure the tier of the ACL is updated from 0 to 2
 
 					expectedDatabaseState := []libovsdb.TestData{
 						purgeACL,
@@ -290,6 +299,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -336,6 +346,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv6ACL.UUID = "ipv6ACL-UUID"
 
@@ -389,6 +400,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					udpACL.UUID = "udpACL-UUID"
 
@@ -436,6 +448,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -496,6 +509,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -613,6 +627,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -697,6 +712,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -782,6 +798,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -862,6 +879,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL.UUID = "ipv4ACL-UUID"
 
@@ -980,6 +998,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 							false,
 							dbIDs.GetExternalIDs(),
 							nil,
+							t.DefaultACLTier,
 						)
 						acl.UUID = "ACL-UUID"
 
@@ -1029,6 +1048,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					acl.UUID = "acl-UUID"
 
@@ -1076,6 +1096,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						aclIDs1.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL1.UUID = "ipv4ACL1-UUID"
 
@@ -1091,6 +1112,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						aclIDs2.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					ipv4ACL2.UUID = "ipv4ACL2-UUID"
 
@@ -1210,6 +1232,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						false,
 						dbIDs.GetExternalIDs(),
 						nil,
+						t.DefaultACLTier,
 					)
 					acl.UUID = "acl-UUID"
 					clusterPortGroup.ACLs = []string{acl.UUID}

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

@@ -2,8 +2,10 @@ package acl
 
 import (
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
+	"time"
 
 	libovsdbclient "github.com/ovn-org/libovsdb/client"
 	libovsdb "github.com/ovn-org/libovsdb/ovsdb"
@@ -67,75 +69,110 @@ func (syncer *aclSyncer) SyncACLs(existingNodes *v1.NodeList) error {
 	if err != nil {
 		return fmt.Errorf("unable to find stale ACLs, cannot update stale data: %v", err)
 	}
-	if len(legacyACLs) == 0 {
-		return nil
-	}
 
-	var updatedACLs []*nbdb.ACL
-	multicastACLs := syncer.updateStaleMulticastACLsDbIDs(legacyACLs)
-	klog.Infof("Found %d stale multicast ACLs", len(multicastACLs))
-	updatedACLs = append(updatedACLs, multicastACLs...)
+	if len(legacyACLs) > 0 {
+		var updatedACLs []*nbdb.ACL
+		multicastACLs := syncer.updateStaleMulticastACLsDbIDs(legacyACLs)
+		klog.Infof("Found %d stale multicast ACLs", len(multicastACLs))
+		updatedACLs = append(updatedACLs, multicastACLs...)
 
-	allowFromNodeACLs := syncer.updateStaleNetpolNodeACLs(legacyACLs, existingNodes.Items)
-	klog.Infof("Found %d stale allow from node ACLs", len(allowFromNodeACLs))
-	updatedACLs = append(updatedACLs, allowFromNodeACLs...)
+		allowFromNodeACLs := syncer.updateStaleNetpolNodeACLs(legacyACLs, existingNodes.Items)
+		klog.Infof("Found %d stale allow from node ACLs", len(allowFromNodeACLs))
+		updatedACLs = append(updatedACLs, allowFromNodeACLs...)
 
-	gressPolicyACLs, err := syncer.updateStaleGressPolicies(legacyACLs)
-	if err != nil {
-		return fmt.Errorf("failed to update gress policy ACLs: %w", err)
-	}
-	klog.Infof("Found %d stale gress ACLs", len(gressPolicyACLs))
-	updatedACLs = append(updatedACLs, gressPolicyACLs...)
+		gressPolicyACLs, err := syncer.updateStaleGressPolicies(legacyACLs)
+		if err != nil {
+			return fmt.Errorf("failed to update gress policy ACLs: %w", err)
+		}
+		klog.Infof("Found %d stale gress ACLs", len(gressPolicyACLs))
+		updatedACLs = append(updatedACLs, gressPolicyACLs...)
 
-	defaultDenyACLs, deleteACLs, err := syncer.updateStaleDefaultDenyNetpolACLs(legacyACLs)
-	if err != nil {
-		return fmt.Errorf("failed to update stale default deny netpol ACLs: %w", err)
-	}
-	klog.Infof("Found %d stale default deny netpol ACLs", len(defaultDenyACLs))
-	updatedACLs = append(updatedACLs, defaultDenyACLs...)
+		defaultDenyACLs, deleteACLs, err := syncer.updateStaleDefaultDenyNetpolACLs(legacyACLs)
+		if err != nil {
+			return fmt.Errorf("failed to update stale default deny netpol ACLs: %w", err)
+		}
+		klog.Infof("Found %d stale default deny netpol ACLs", len(defaultDenyACLs))
+		updatedACLs = append(updatedACLs, defaultDenyACLs...)
 
-	egressFirewallACLs := syncer.updateStaleEgressFirewallACLs(legacyACLs)
-	klog.Infof("Found %d stale egress firewall ACLs", len(gressPolicyACLs))
-	updatedACLs = append(updatedACLs, egressFirewallACLs...)
+		egressFirewallACLs := syncer.updateStaleEgressFirewallACLs(legacyACLs)
+		klog.Infof("Found %d stale egress firewall ACLs", len(gressPolicyACLs))
+		updatedACLs = append(updatedACLs, egressFirewallACLs...)
 
-	// delete stale duplicating acls first
-	_, err = libovsdbops.TransactAndCheck(syncer.nbClient, deleteACLs)
-	if err != nil {
-		return fmt.Errorf("faile to trasact db ops: %v", err)
-	}
+		// delete stale duplicating acls first
+		_, err = libovsdbops.TransactAndCheck(syncer.nbClient, deleteACLs)
+		if err != nil {
+			return fmt.Errorf("faile to trasact db ops: %v", err)
+		}
 
-	// make sure there is only 1 ACL with any given primary ID
-	// 1. collect all existing primary IDs via predicate that will update IDs set, but always return false
-	existingACLPrimaryIDs := sets.Set[string]{}
-	_, err = libovsdbops.FindACLsWithPredicate(syncer.nbClient, func(acl *nbdb.ACL) bool {
-		if acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()] != "" {
-			existingACLPrimaryIDs.Insert(acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()])
+		// make sure there is only 1 ACL with any given primary ID
+		// 1. collect all existing primary IDs via predicate that will update IDs set, but always return false
+		existingACLPrimaryIDs := sets.Set[string]{}
+		_, err = libovsdbops.FindACLsWithPredicate(syncer.nbClient, func(acl *nbdb.ACL) bool {
+			if acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()] != "" {
+				existingACLPrimaryIDs.Insert(acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()])
+			}
+			return false
+		})
+		if err != nil {
+			return fmt.Errorf("failed to find exisitng primary ID acls: %w", err)
 		}
-		return false
-	})
-	if err != nil {
-		return fmt.Errorf("failed to find exisitng primary ID acls: %w", err)
-	}
-	// 2. Check to-be-updated ACLs don't have the same PrimaryID between themselves and with the existingACLPrimaryIDs
-	uniquePrimaryIDACLs := []*nbdb.ACL{}
-	for _, acl := range updatedACLs {
-		primaryID := acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()]
-		if existingACLPrimaryIDs.Has(primaryID) {
-			// don't update that acl, otherwise 2 ACLs with the same primary ID will be in the db
-			klog.Warningf("Skip updating ACL %+v to the new ExternalIDs, since there is another ACL with the same primary ID", acl)
-		} else {
-			existingACLPrimaryIDs.Insert(primaryID)
-			uniquePrimaryIDACLs = append(uniquePrimaryIDACLs, acl)
+		// 2. Check to-be-updated ACLs don't have the same PrimaryID between themselves and with the existingACLPrimaryIDs
+		uniquePrimaryIDACLs := []*nbdb.ACL{}
+		for _, acl := range updatedACLs {
+			primaryID := acl.ExternalIDs[libovsdbops.PrimaryIDKey.String()]
+			if existingACLPrimaryIDs.Has(primaryID) {
+				// don't update that acl, otherwise 2 ACLs with the same primary ID will be in the db
+				klog.Warningf("Skip updating ACL %+v to the new ExternalIDs, since there is another ACL with the same primary ID", acl)
+			} else {
+				existingACLPrimaryIDs.Insert(primaryID)
+				uniquePrimaryIDACLs = append(uniquePrimaryIDACLs, acl)
+			}
+		}
+
+		// update acls with new ExternalIDs
+		err = batching.Batch[*nbdb.ACL](syncer.txnBatchSize, uniquePrimaryIDACLs, func(batchACLs []*nbdb.ACL) error {
+			return libovsdbops.CreateOrUpdateACLs(syncer.nbClient, batchACLs...)
+		})
+		if err != nil {
+			return fmt.Errorf("cannot update stale ACLs: %v", err)
 		}
 	}
 
-	// update acls with new ExternalIDs
-	err = batching.Batch[*nbdb.ACL](syncer.txnBatchSize, uniquePrimaryIDACLs, func(batchACLs []*nbdb.ACL) error {
-		return libovsdbops.CreateOrUpdateACLs(syncer.nbClient, batchACLs...)
-	})
+	// Once all the staleACLs are deleted and the externalIDs have been updated (externalIDs update should be a one-time
+	// upgrade operation), let us now update the tier's of all existing ACLs to types.DefaultACLTier. During upgrades after
+	// the OVN schema changes are applied, the nbdb.ACL.Tier column will be added and every row will be updated to 0 by
+	// default (types.PlaceHolderACLTier). For all features using ACLs (egressFirewall, NetworkPolicy, NodeACLs) we want to
+	// move them to Tier2. We need to do this in reverse order of ACL priority to avoid network traffic disruption during
+	// upgrades window (if not done according to priorities we might end up in a window where the ACL with priority 1000
+	// for default deny is in tier0 while 1001 ACL for allow-ing traffic is in tier2 for a given namespace network policy).
+	// NOTE: This is a one-time operation as no ACLs should ever be created in types.PlaceHolderACLTier moving forward.
+	// Fetch all ACLs in types.PlaceHolderACLTier (Tier0); update their Tier to 2 and batch the ACL update.
+	klog.Info("Updating Tier of existing ACLs...")
+	start := time.Now()
+	aclPred := func(item *nbdb.ACL) bool {
+		return item.Tier == types.PlaceHolderACLTier
+	}
+	aclsInTier0, err := libovsdbops.FindACLsWithPredicate(syncer.nbClient, aclPred)
 	if err != nil {
-		return fmt.Errorf("cannot update stale ACLs: %v", err)
+		return fmt.Errorf("unable to fetch Tier0 ACLs: %v", err)
+	}
+	if len(aclsInTier0) > 0 {
+		sort.Slice(aclsInTier0, func(i, j int) bool {
+			return aclsInTier0[i].Priority < aclsInTier0[j].Priority
+		}) // O(nlogn); unstable sort
+		for _, acl := range aclsInTier0 {
+			acl := acl
+			acl.Tier = types.DefaultACLTier // move tier to 2
+		}
+		// batch ACLs together in order of their priority: lowest first and then highest
+		err = batching.Batch[*nbdb.ACL](syncer.txnBatchSize, aclsInTier0, func(batchACLs []*nbdb.ACL) error {
+			return libovsdbops.CreateOrUpdateACLs(syncer.nbClient, batchACLs...)
+		})
+		if err != nil {
+			return fmt.Errorf("cannot update ACLs to tier2: %v", err)
+		}
 	}
+	klog.Infof("Updating tier's of all ACLs in cluster took %v", time.Since(start))
 	return nil
 }