[release-4.6] Bug 1953088: master: Delay deleting Namespace's address set for 20 seconds

go-controller/go.mod

@@ -23,6 +23,7 @@ require (
 	github.com/k8snetworkplumbingwg/network-attachment-definition-client v0.0.0-20200626054723-37f83d1996bc
 	github.com/onsi/ginkgo v1.11.0
 	github.com/onsi/gomega v1.8.1
+	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.2.1
 	github.com/satori/go.uuid v0.0.0-20181028125025-b2ce2384e17b // indirect
 	github.com/spf13/afero v1.2.2

go-controller/pkg/ovn/address_set.go

@@ -7,6 +7,8 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/pkg/errors"
+
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
@@ -46,7 +48,10 @@ type AddressSet interface {
 	GetIPv6HashName() string
 	// GetName returns the descriptive name of the address set
 	GetName() string
+	// AddIPs adds the array of IPs to the address set
 	AddIPs(ip []net.IP) error
+	// SetIPs sets the address set to the given array of addresses
+	SetIPs(ip []net.IP) error
 	DeleteIPs(ip []net.IP) error
 	Destroy() error
 }
@@ -122,6 +127,7 @@ func truncateSuffixFromAddressSet(asName string) string {
 	return asName
 }
 
+// DestroyAddressSetInBackingStore ensures an address set is deleted
 func (asf *ovnAddressSetFactory) DestroyAddressSetInBackingStore(name string) error {
 	// We need to handle both legacy and new address sets in this method. Legacy names
 	// will not have v4 and v6 suffix as they were same as namespace name. Hence we will always try to destroy
@@ -272,6 +278,29 @@ func (as *ovnAddressSets) GetName() string {
 	return as.name
 }
 
+func (as *ovnAddressSets) SetIPs(ips []net.IP) error {
+	var err error
+	var listIPv4, listIPv6 []net.IP
+	as.Lock()
+	defer as.Unlock()
+
+	for _, ip := range ips {
+		if utilnet.IsIPv6(ip) {
+			listIPv6 = append(listIPv6, ip)
+		} else {
+			listIPv4 = append(listIPv4, ip)
+		}
+	}
+	if as.ipv6 != nil {
+		err = as.ipv6.setIP(listIPv6)
+	}
+	if as.ipv4 != nil {
+		err = errors.Wrapf(err, "%v", as.ipv4.setIP(listIPv4))
+	}
+
+	return err
+}
+
 func (as *ovnAddressSets) AddIPs(ips []net.IP) error {
 	var err error
 
@@ -360,6 +389,26 @@ func (as *ovnAddressSet) setOrClear() error {
 	return nil
 }
 
+// setIP updates the given address set in OVN to be the given IPs
+func (as *ovnAddressSet) setIP(ips []net.IP) error {
+	var ipList []string
+
+	// delete the old map of IP addresses
+	as.ips = make(map[string]net.IP)
+
+	for _, ip := range ips {
+		ipList = append(ipList, `"`+ip.String()+`"`)
+		as.ips[ip.String()] = ip
+	}
+	joinedIPs := strings.Join(ipList, " ")
+	_, stderr, err := util.RunOVNNbctl("set", "address_set", as.uuid, "addresses="+joinedIPs)
+	if err != nil {
+		return fmt.Errorf("failed to set address set %q, stderr: %q (%v)",
+			asDetail(as), stderr, err)
+	}
+	return nil
+}
+
 func (as *ovnAddressSet) addIP(ip net.IP) error {
 	ipStr := ip.String()
 	if _, ok := as.ips[ipStr]; ok {

go-controller/pkg/ovn/address_set_test.go

@@ -278,6 +278,39 @@ var _ = Describe("OVN Address Set operations", func() {
 				return nil
 			}
 
+			err := app.Run([]string{app.Name})
+			Expect(err).NotTo(HaveOccurred())
+		})
+		It("sets an already set addressSet", func() {
+			app.Action = func(ctx *cli.Context) error {
+				const addr1 string = "1.2.3.4"
+				const addr2 string = "2.3.4.5"
+				const addr3 string = "7.8.9.10"
+
+				_, err := config.InitConfig(ctx, fexec, nil)
+				Expect(err).NotTo(HaveOccurred())
+
+				fexec.AddFakeCmdsNoOutputNoError([]string{
+					"ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid find address_set name=a16990491322166530807",
+				})
+				fexec.AddFakeCmd(&ovntest.ExpectedCmd{
+					Cmd:    `ovn-nbctl --timeout=15 create address_set name=a16990491322166530807 external-ids:name=foobar_v4 addresses="` + addr1 + `"`,
+					Output: fakeUUID,
+				})
+				fexec.AddFakeCmdsNoOutputNoError([]string{
+					`ovn-nbctl --timeout=15 set address_set ` + fakeUUID + ` addresses="` + addr2 + `" ` + `"` + addr3 + `"`,
+				})
+
+				as, err := asFactory.NewAddressSet("foobar", []net.IP{net.ParseIP(addr1)})
+				Expect(err).NotTo(HaveOccurred())
+
+				err = as.SetIPs([]net.IP{net.ParseIP(addr2), net.ParseIP(addr3)})
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(fexec.CalledMatchesExpected()).To(BeTrue(), fexec.ErrorDesc)
+				return nil
+			}
+
 			err := app.Run([]string{app.Name})
 			Expect(err).NotTo(HaveOccurred())
 		})

go-controller/pkg/ovn/fake_address_set_test.go

@@ -4,6 +4,7 @@ import (
 	"net"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 
@@ -136,7 +137,7 @@ func (f *fakeAddressSetFactory) EventuallyExpectNoAddressSet(name string) {
 		defer f.RUnlock()
 		_, ok := f.sets[name]
 		return ok
-	}).Should(BeFalse())
+	}, 25*time.Second).Should(BeFalse())
 }
 
 type removeFunc func(string)
@@ -229,6 +230,11 @@ func (as *fakeAddressSets) AddIPs(ips []net.IP) error {
 	return nil
 }
 
+func (as *fakeAddressSets) SetIPs(ips []net.IP) error {
+	// NOOP
+	return nil
+}
+
 func (as *fakeAddressSets) DeleteIPs(ips []net.IP) error {
 	var err error
 	as.Lock()

go-controller/pkg/ovn/namespace.go

@@ -416,9 +416,36 @@ func (oc *Controller) deleteNamespaceLocked(ns string) *namespaceInfo {
 		return nil
 	}
 	if nsInfo.addressSet != nil {
-		if err := nsInfo.addressSet.Destroy(); err != nil {
-			klog.Errorf(err.Error())
+		// Empty the address set, then delete it after an interval.
+		if err := nsInfo.addressSet.SetIPs(nil); err != nil {
+			klog.Errorf("Warning: failed to empty address set for deleted NS %s: %v", ns, err)
 		}
+
+		// Delete the address set after a short delay.
+		// This is so NetworkPolicy handlers can converge and stop referencing it.
+		addressSet := nsInfo.addressSet
+		go func() {
+			select {
+			case <-oc.stopChan:
+				return
+			case <-time.After(20 * time.Second):
+				// Check to see if the NS was re-added in the meanwhile. If so,
+				// only delete if the new NS's AddressSet shouldn't exist.
+				nsInfo := oc.getNamespaceLocked(ns)
+				if nsInfo != nil {
+					defer nsInfo.Unlock()
+					if nsInfo.addressSet != nil {
+						klog.V(5).Infof("Skipping deferred deletion of AddressSet for NS %s: re-created", ns)
+						return
+					}
+				}
+
+				klog.V(5).Infof("Finishing deferred deletion of AddressSet for NS %s", ns)
+				if err := addressSet.Destroy(); err != nil {
+					klog.Errorf("Failed to delete AddressSet for NS %s: %v", ns, err.Error())
+				}
+			}
+		}()
 	}
 	delete(oc.namespaces, ns)