Merge pull request #1453 from npinaeva/ocpbugs-4864

OCPBUGS-4864: [release-4.10] Fix address set cleanup: only delete address sets owned by given object

go-controller/pkg/ovn/address_set/address_set.go

@@ -30,7 +30,7 @@ const (
 	ipv6AddressSetSuffix = "_v6"
 )
 
-type AddressSetIterFunc func(hashedName, namespace, suffix string) error
+type AddressSetIterFunc func(hashedName, name string) error
 type AddressSetDoFunc func(as AddressSet) error
 
 // AddressSetFactory is an interface for managing address set objects
@@ -176,7 +176,9 @@ func (asf *ovnAddressSetFactory) EnsureAddressSet(name string) (AddressSet, erro
 	return &ovnAddressSets{nbClient: asf.nbClient, name: name, ipv4: v4set, ipv6: v6set}, nil
 }
 
-func forEachAddressSet(nbClient libovsdbclient.Client, do func(string) error) error {
+// forEachAddressSet executes a do function on each address set found to have ExternalIDs["name"].
+// do function should take parameters: hashed addr set name, real name
+func forEachAddressSet(nbClient libovsdbclient.Client, do func(string, string) error) error {
 	addrSetList := &[]nbdb.AddressSet{}
 	ctx, cancel := context.WithTimeout(context.Background(), types.OVSDBTimeout)
 	defer cancel()
@@ -191,7 +193,7 @@ func forEachAddressSet(nbClient libovsdbclient.Client, do func(string) error) er
 
 	var errors []error
 	for _, addrSet := range *addrSetList {
-		if err := do(addrSet.ExternalIDs["name"]); err != nil {
+		if err := do(addrSet.Name, addrSet.ExternalIDs["name"]); err != nil {
 			errors = append(errors, err)
 		}
 	}
@@ -203,12 +205,10 @@ func forEachAddressSet(nbClient libovsdbclient.Client, do func(string) error) er
 	return nil
 }
 
-// ProcessEachAddressSet will pass the unhashed address set name, namespace name
-// and the first suffix in the name to the 'iteratorFn' for every address_set in
-// OVN. (Unhashed address set names are of the form namespaceName[.suffix1.suffix2. .suffixN])
+// ProcessEachAddressSet will pass the hashed and unhashed address set name to iteratorFn for every address set.
 func (asf *ovnAddressSetFactory) ProcessEachAddressSet(iteratorFn AddressSetIterFunc) error {
 	processedAddressSets := sets.String{}
-	return forEachAddressSet(asf.nbClient, func(name string) error {
+	return forEachAddressSet(asf.nbClient, func(hashedName, name string) error {
 		// Remove the suffix from the address set name and normalize
 		addrSetName := truncateSuffixFromAddressSet(name)
 		if processedAddressSets.Has(addrSetName) {
@@ -218,13 +218,7 @@ func (asf *ovnAddressSetFactory) ProcessEachAddressSet(iteratorFn AddressSetIter
 			return nil
 		}
 		processedAddressSets.Insert(addrSetName)
-		names := strings.Split(addrSetName, ".")
-		addrSetNamespace := names[0]
-		nameSuffix := ""
-		if len(names) >= 2 {
-			nameSuffix = names[1]
-		}
-		return iteratorFn(addrSetName, addrSetNamespace, nameSuffix)
+		return iteratorFn(hashedName, addrSetName)
 	})
 }
 

go-controller/pkg/ovn/address_set/address_set_cleanup.go

@@ -16,7 +16,7 @@ func NonDualStackAddressSetCleanup(nbClient libovsdbclient.Client) error {
 	const old = 0
 	const new = 1
 	addressSets := map[string][2]bool{}
-	err := forEachAddressSet(nbClient, func(name string) error {
+	err := forEachAddressSet(nbClient, func(hashedName, name string) error {
 		shortName := truncateSuffixFromAddressSet(name)
 		spec, found := addressSets[shortName]
 		if !found {

go-controller/pkg/ovn/address_set/address_set_test.go

@@ -64,22 +64,22 @@ var _ = ginkgo.Describe("OVN Address Set operations", func() {
 	})
 
 	ginkgo.Context("when iterating address sets", func() {
-		ginkgo.It("calls the iterator function for each address set with the given prefix", func() {
+		ginkgo.It("calls the iterator function for each address set", func() {
 			app.Action = func(ctx *cli.Context) error {
 				dbSetup := libovsdbtest.TestSetup{
 					NBData: []libovsdbtest.TestData{
 						&nbdb.AddressSet{
 							Name:        "1",
-							ExternalIDs: map[string]string{"name": "ns1.foo.bar"},
+							ExternalIDs: map[string]string{"name": "foo.bar"},
 						},
 						&nbdb.AddressSet{
 							Name:        "2",
-							ExternalIDs: map[string]string{"name": "ns2.test.test2"},
+							ExternalIDs: map[string]string{"name": "test.test2"},
 						},
 
 						&nbdb.AddressSet{
 							Name:        "3",
-							ExternalIDs: map[string]string{"name": "ns3"},
+							ExternalIDs: map[string]string{"name": "test3"},
 						},
 					},
 				}
@@ -91,33 +91,17 @@ var _ = ginkgo.Describe("OVN Address Set operations", func() {
 
 				_, err = config.InitConfig(ctx, nil, nil)
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-				namespaces := []testAddressSetName{
-					{
-						namespace: "ns1",
-						suffix:    []string{"foo", "bar"},
-					},
-					{
-						namespace: "ns2",
-						suffix:    []string{"test", "test2"},
-					},
-					{
-						namespace: "ns3",
-					},
+				expectedAddressSets := map[string]bool{
+					"foo.bar":    true,
+					"test.test2": true,
+					"test3":      true,
 				}
-
-				err = asFactory.ProcessEachAddressSet(func(addrSetName, namespaceName, nameSuffix string) error {
-					found := false
-					for _, n := range namespaces {
-						name := n.makeNames()
-						if addrSetName == name {
-							found = true
-							gomega.Expect(namespaceName).To(gomega.Equal(n.namespace))
-						}
-					}
-					gomega.Expect(found).To(gomega.BeTrue())
+				err = asFactory.ProcessEachAddressSet(func(hashedName, addrSetName string) error {
+					gomega.Expect(expectedAddressSets[addrSetName]).To(gomega.BeTrue())
+					delete(expectedAddressSets, addrSetName)
 					return nil
 				})
+				gomega.Expect(len(expectedAddressSets)).To(gomega.Equal(0))
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())
 				return nil
 			}

go-controller/pkg/ovn/address_set/fake_address_set.go

@@ -2,14 +2,12 @@ package addressset
 
 import (
 	"net"
-	"strings"
 	"sync"
 	"sync/atomic"
 
 	"github.com/ovn-org/libovsdb/ovsdb"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	utilnet "k8s.io/utils/net"
 
 	"github.com/onsi/gomega"
@@ -72,21 +70,17 @@ func (f *FakeAddressSetFactory) EnsureAddressSet(name string) (AddressSet, error
 
 func (f *FakeAddressSetFactory) ProcessEachAddressSet(iteratorFn AddressSetIterFunc) error {
 	f.Lock()
-	defer f.Unlock()
-	asNames := sets.String{}
+	asNames := map[string]string{}
 	for _, set := range f.sets {
 		asName := truncateSuffixFromAddressSet(set.getName())
-		if asNames.Has(asName) {
+		if _, ok := asNames[asName]; ok {
 			continue
 		}
-		asNames.Insert(asName)
-		parts := strings.Split(asName, ".")
-		addrSetNamespace := parts[0]
-		nameSuffix := ""
-		if len(parts) >= 2 {
-			nameSuffix = parts[1]
-		}
-		if err := iteratorFn(asName, addrSetNamespace, nameSuffix); err != nil {
+		asNames[asName] = set.hashName
+	}
+	f.Unlock()
+	for asName, hashName := range asNames {
+		if err := iteratorFn(hashName, asName); err != nil {
 			return err
 		}
 	}

go-controller/pkg/ovn/egressfirewall.go

@@ -26,6 +26,8 @@ const (
 	egressFirewallAppliedCorrectly = "EgressFirewall Rules applied"
 	egressFirewallAddError         = "EgressFirewall Rules not correctly added"
 	egressFirewallUpdateError      = "EgressFirewall Rules not correctly updated"
+	// egressFirewallACLExtIdKey external ID key for egress firewall ACLs
+	egressFirewallACLExtIdKey = "egressFirewall"
 )
 
 type egressFirewall struct {
@@ -176,7 +178,7 @@ func (oc *Controller) syncEgressFirewallRetriable(egressFirewalls []interface{})
 	ovnEgressFirewalls := make(map[string]struct{})
 
 	for _, egressFirewallACL := range egressFirewallACLs {
-		if ns, ok := egressFirewallACL.ExternalIDs["egressFirewall"]; ok {
+		if ns, ok := egressFirewallACL.ExternalIDs[egressFirewallACLExtIdKey]; ok {
 			// Most egressFirewalls will have more then one ACL but we only need to know if there is one for the namespace
 			// so a map is fine and we will add an entry every iteration but because it is a map will overwrite the previous
 			// entry if it already existed
@@ -285,11 +287,16 @@ func (oc *Controller) deleteEgressFirewall(egressFirewallObj *egressfirewallapi.
 			break
 		}
 	}
+	// delete acls first, then dns address set that is referenced in these acls
+	if err := oc.deleteEgressFirewallRules(egressFirewallObj.Namespace); err != nil {
+		return err
+	}
+
 	if deleteDNS {
 		oc.egressFirewallDNS.Delete(egressFirewallObj.Namespace)
 	}
 
-	return oc.deleteEgressFirewallRules(egressFirewallObj.Namespace)
+	return nil
 }
 
 func (oc *Controller) updateEgressFirewallWithRetry(egressfirewall *egressfirewallapi.EgressFirewall) error {
@@ -362,7 +369,7 @@ func (oc *Controller) createEgressFirewallRules(priority int, match, action, ext
 		Direction:   types.DirectionToLPort,
 		Match:       match,
 		Action:      action,
-		ExternalIDs: map[string]string{"egressFirewall": externalID},
+		ExternalIDs: map[string]string{egressFirewallACLExtIdKey: externalID},
 	}
 
 	// add it's UUID to the necessary logical switches
@@ -408,7 +415,7 @@ func (oc *Controller) createEgressFirewallRules(priority int, match, action, ext
 // deleteEgressFirewallRules delete the specific logical router policy/join switch Acls
 func (oc *Controller) deleteEgressFirewallRules(externalID string) error {
 	// Find ACLs for a given egressFirewall
-	egressFirewallACLs, err := libovsdbops.FindACLsByExternalID(oc.nbClient, map[string]string{"egressFirewall": externalID})
+	egressFirewallACLs, err := libovsdbops.FindACLsByExternalID(oc.nbClient, map[string]string{egressFirewallACLExtIdKey: externalID})
 	if err != nil {
 		return fmt.Errorf("unable to list egress firewall ACLs, cannot cleanup old stale data, err: %v", err)
 	}

go-controller/pkg/ovn/egressfirewall_test.go

@@ -87,7 +87,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "none"},
+					map[string]string{egressFirewallACLExtIdKey: "none"},
 				)
 				purgeACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -100,7 +100,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "default"},
+					map[string]string{egressFirewallACLExtIdKey: "default"},
 				)
 				keepACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -114,7 +114,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "default"},
+					map[string]string{egressFirewallACLExtIdKey: "default"},
 				)
 				otherACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -247,7 +247,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -335,7 +335,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv6ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -431,7 +431,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 
 				udpACL.UUID = libovsdbops.BuildNamedUUID()
@@ -528,7 +528,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -632,7 +632,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -711,7 +711,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "none"},
+					map[string]string{egressFirewallACLExtIdKey: "none"},
 				)
 				purgeACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -724,7 +724,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "default"},
+					map[string]string{egressFirewallACLExtIdKey: "default"},
 				)
 				keepACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -738,7 +738,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "default"},
+					map[string]string{egressFirewallACLExtIdKey: "default"},
 				)
 				otherACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -866,7 +866,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -949,7 +949,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv6ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -1042,7 +1042,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 
 				udpACL.UUID = libovsdbops.BuildNamedUUID()
@@ -1123,7 +1123,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()
 
@@ -1211,7 +1211,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
 					"",
 					"",
 					false,
-					map[string]string{"egressFirewall": "namespace1"},
+					map[string]string{egressFirewallACLExtIdKey: "namespace1"},
 				)
 				ipv4ACL.UUID = libovsdbops.BuildNamedUUID()