Prevent UDN deletion race condition by checking controller state

Adds GetActiveNetwork method to verify network controllers
are stopped before allowing UDN/CUDN deletion, preventing race
conditions in a scenario where a network is recreated before the
cluster-manager controller stopped running.

Signed-off-by: Patryk Diak <[email protected]>

Author: kyrtapz

go-controller/pkg/clustermanager/clustermanager.go

@@ -146,6 +146,7 @@ func NewClusterManager(
 			ovnClient.UserDefinedNetworkClient,
 			wf.UserDefinedNetworkInformer(), wf.ClusterUserDefinedNetworkInformer(),
 			udntemplate.RenderNetAttachDefManifest,
+			cm.networkManager.Interface(),
 			wf.PodCoreInformer(),
 			wf.NamespaceInformer(),
 			cm.recorder,

go-controller/pkg/clustermanager/userdefinednetwork/controller.go

@@ -38,6 +38,7 @@ import (
 	userdefinednetworkinformer "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/userdefinednetwork/v1/apis/informers/externalversions/userdefinednetwork/v1"
 	userdefinednetworklister "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/userdefinednetwork/v1/apis/listers/userdefinednetwork/v1"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/metrics"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/networkmanager"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 )
 
@@ -72,6 +73,8 @@ type Controller struct {
 	// trying to create an object with the same name.
 	createNetworkLock sync.Mutex
 
+	networkManager networkmanager.Interface
+
 	udnClient         userdefinednetworkclientset.Interface
 	udnLister         userdefinednetworklister.UserDefinedNetworkLister
 	cudnLister        userdefinednetworklister.ClusterUserDefinedNetworkLister
@@ -93,6 +96,7 @@ func New(
 	udnInformer userdefinednetworkinformer.UserDefinedNetworkInformer,
 	cudnInformer userdefinednetworkinformer.ClusterUserDefinedNetworkInformer,
 	renderNadFn RenderNetAttachDefManifest,
+	networkManager networkmanager.Interface,
 	podInformer corev1informer.PodInformer,
 	namespaceInformer corev1informer.NamespaceInformer,
 	eventRecorder record.EventRecorder,
@@ -108,6 +112,7 @@ func New(
 		renderNadFn:                 renderNadFn,
 		podInformer:                 podInformer,
 		namespaceInformer:           namespaceInformer,
+		networkManager:              networkManager,
 		networkInUseRequeueInterval: defaultNetworkInUseCheckInterval,
 		namespaceTracker:            map[string]sets.Set[string]{},
 		eventRecorder:               eventRecorder,
@@ -410,6 +415,12 @@ func (c *Controller) syncUserDefinedNetwork(udn *userdefinednetworkv1.UserDefine
 				return nil, fmt.Errorf("failed to delete NetworkAttachmentDefinition [%s/%s]: %w", udn.Namespace, udn.Name, err)
 			}
 
+			// Ensure that the network controller is stopped(GetActiveNetwork returns nil) before allowing the UDN
+			// to be removed.
+			if c.networkManager.GetActiveNetwork(util.GenerateUDNNetworkName(udn.Namespace, udn.Name)) != nil {
+				return nil, &networkInUseError{err: fmt.Errorf("cannot remove UDN, controller for network %s is still running", util.GenerateUDNNetworkName(udn.Namespace, udn.Name))}
+			}
+
 			controllerutil.RemoveFinalizer(udn, template.FinalizerUserDefinedNetwork)
 			udn, err := c.udnClient.K8sV1().UserDefinedNetworks(udn.Namespace).Update(context.Background(), udn, metav1.UpdateOptions{})
 			if err != nil {
@@ -582,6 +593,12 @@ func (c *Controller) syncClusterUDN(cudn *userdefinednetworkv1.ClusterUserDefine
 				return nil, errors.Join(errs...)
 			}
 
+			// Ensure that the network controller is stopped(GetActiveNetwork returns nil) before allowing the cUDN
+			// to be removed.
+			if c.networkManager.GetActiveNetwork(util.GenerateCUDNNetworkName(cudn.Name)) != nil {
+				return nil, &networkInUseError{err: fmt.Errorf("cannot remove cluster UDN, controller for network %s is still running", util.GenerateCUDNNetworkName(cudn.Name))}
+			}
+
 			var err error
 			controllerutil.RemoveFinalizer(cudn, template.FinalizerUserDefinedNetwork)
 			cudn, err = c.udnClient.K8sV1().ClusterUserDefinedNetworks().Update(context.Background(), cudn, metav1.UpdateOptions{})

go-controller/pkg/clustermanager/userdefinednetwork/controller_test.go

@@ -26,6 +26,8 @@ import (
 	udnclient "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/userdefinednetwork/v1/apis/clientset/versioned"
 	udnfakeclient "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/userdefinednetwork/v1/apis/clientset/versioned/fake"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/factory"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/networkmanager"
+	nmtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/networkmanager"
 	ovntypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
@@ -59,9 +61,11 @@ var _ = Describe("User Defined Network Controller", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(f.Start()).To(Succeed())
 
+		networkManager, err := networkmanager.NewForCluster(&nmtest.FakeControllerManager{}, f, cs, nil)
+		Expect(err).NotTo(HaveOccurred())
 		return New(cs.NetworkAttchDefClient, f.NADInformer(),
 			cs.UserDefinedNetworkClient, f.UserDefinedNetworkInformer(), f.ClusterUserDefinedNetworkInformer(),
-			renderNADStub, f.PodCoreInformer(), f.NamespaceInformer(), nil,
+			renderNADStub, networkManager.Interface(), f.PodCoreInformer(), f.NamespaceInformer(), nil,
 		)
 	}
 

go-controller/pkg/networkmanager/api.go

@@ -39,6 +39,11 @@ type Interface interface {
 	// GetNetwork returns the network of the given name or nil if unknown
 	GetNetwork(name string) util.NetInfo
 
+	// GetActiveNetwork returns the NetInfo currently held by the controller for the given network.
+	// This may differ from the NetInfo returned by GetNetwork which reflects the API state.
+	// Returns nil if there is no running controller for the provided network.
+	GetActiveNetwork(network string) util.NetInfo
+
 	// DoWithLock takes care of locking and unlocking while iterating over all role primary user defined networks.
 	DoWithLock(f func(network util.NetInfo) error) error
 	GetActiveNetworkNamespaces(networkName string) ([]string, error)
@@ -204,4 +209,11 @@ func (nm defaultNetworkManager) GetActiveNetworkNamespaces(_ string) ([]string,
 	return []string{"default"}, nil
 }
 
+func (nm defaultNetworkManager) GetActiveNetwork(network string) util.NetInfo {
+	if network != types.DefaultNetworkName {
+		return nil
+	}
+	return &util.DefaultNetInfo{}
+}
+
 var def Controller = &defaultNetworkManager{}

go-controller/pkg/networkmanager/nad_controller.go

@@ -669,3 +669,13 @@ func (c *nadController) handleNetworkID(old util.NetInfo, new util.MutableNetInf
 
 	return nil
 }
+
+func (c *nadController) GetActiveNetwork(network string) util.NetInfo {
+	c.RLock()
+	defer c.RUnlock()
+	state := c.networkController.getNetworkState(network)
+	if state == nil {
+		return nil
+	}
+	return state.controller
+}

go-controller/pkg/testing/networkmanager/fake.go

@@ -78,6 +78,10 @@ func (fnm *FakeNetworkManager) GetNetwork(networkName string) util.NetInfo {
 	return &util.DefaultNetInfo{}
 }
 
+func (fnm *FakeNetworkManager) GetActiveNetwork(networkName string) util.NetInfo {
+	return fnm.GetNetwork(networkName)
+}
+
 func (fnm *FakeNetworkManager) GetActiveNetworkNamespaces(networkName string) ([]string, error) {
 	namespaces := make([]string, 0)
 	for namespaceName, primaryNAD := range fnm.PrimaryNetworks {