Enhance pre-created SubnetSet validation (#1369)

wcpsvc will validate if there is no SubnetPort on the Subnet when changing
vmDefault/podDefault from true to false.
NSX Operator will watch the VPCNetworkConfiguration and remove the Subnet
from default SubnetSet CR if vmDefault/podDefault is changed from true to false.

This PR is to deal with the race condition that SubnetPort is created after wcpsvc
updates the VPCNetworkConfiguration and before NSX Operator updates the
SubnetSet.

In SubnetSet webhook, operator needs to check the following for vm/pod default
SubnetSet
- Subnet cannot be removed from SubnetSet if it is used by the SubnetPort through
the SubnetSet
- Default SubnetSet cannot be deleted if it is used by Pod/SubnetPort

When vmDefault/podDefault is changed from true to false for a Subnet in
VPCNetworkConfiguration, but it is not removed from vm-default or pod-default
SubnetSet due to stale SubnetPort, we should
- update the SubnetSet Status to show the update is failed
- not allow the following-up SubnetPort being created on the Subnet

Signed-off-by: Yanjun Zhou <yanjun.zhou@broadcom.com>

Author: yanjunz97

pkg/apis/vpc/v1alpha1/condition_types.go

@@ -14,6 +14,7 @@ const (
 	AutoSnatEnabled            ConditionType = "AutoSnatEnabled"
 	ExternalIPBlocksConfigured ConditionType = "ExternalIPBlocksConfigured"
 	DeleteFailure              ConditionType = "DeletionFailed"
+	UpdateFailure              ConditionType = "UpdateFailed"
 )
 
 // Condition defines condition of custom resource.

pkg/controllers/common/utils.go

@@ -13,6 +13,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -79,8 +80,36 @@ func GetNSXSubnetsForSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.Subn
 	return subnets, nil
 }
 
-func GetSubnetFromSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.SubnetSet, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, error) {
+func getSubnetFromVPCNetworkConfiguration(vpcService servicecommon.VPCServiceProvider, ns string, defaultSubnetSetFor string) (sets.Set[string], error) {
+	subnetPaths := sets.New[string]()
+	vpcNetworkConfig, err := GetVpcNetworkConfig(vpcService, ns)
+	if err != nil {
+		return subnetPaths, err
+	}
+	for _, subnet := range vpcNetworkConfig.Spec.Subnets {
+		if defaultSubnetSetFor == servicecommon.DefaultPodNetwork && subnet.PodDefault {
+			subnetPaths.Insert(subnet.Path)
+		} else if defaultSubnetSetFor == servicecommon.DefaultVMNetwork && subnet.VMDefault {
+			subnetPaths.Insert(subnet.Path)
+		}
+	}
+	return subnetPaths, nil
+}
+
+// Get a Subnet with available IPs from the pre-created SubnetSet
+func GetSubnetFromSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.SubnetSet, vpcService servicecommon.VPCServiceProvider, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, error) {
 	var errList []error
+	defaultSubnetSetFor := util.GetSubnetSetKind(subnetSet)
+	subnetPathsFromConfig := sets.New[string]()
+	var err error
+	// Get default Subnets from VPCNetworkConfiguration CR
+	if len(defaultSubnetSetFor) > 0 {
+		subnetPathsFromConfig, err = getSubnetFromVPCNetworkConfiguration(vpcService, subnetSet.Namespace, defaultSubnetSetFor)
+		if err != nil {
+			log.Error(err, "Failed to get Subnet for default Network in VPCNetworkConfiguration")
+			return "", err
+		}
+	}
 	for _, subnetName := range *subnetSet.Spec.SubnetNames {
 		subnetCR := &v1alpha1.Subnet{}
 		if err := client.Get(context.Background(), types.NamespacedName{Namespace: subnetSet.Namespace, Name: subnetName}, subnetCR); err != nil {
@@ -94,6 +123,16 @@ func GetSubnetFromSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.SubnetS
 			errList = append(errList, err)
 			continue
 		}
+		// For default network, race between wcpsvc and NSX Operator on removing Subnet from default SubnetSet
+		// may result in difference between Subnets in VPCNetworkConfiguration and default SubnetSet
+		// We should not allow new SubnetPort to be created on the Subnet already removed from VPCNetworkConfiguration
+		// but still exist in SubnetSet CR due to stale SubnetPorts
+		if len(defaultSubnetSetFor) > 0 {
+			if !subnetPathsFromConfig.Has(*nsxSubnet.Path) {
+				log.Info("Shared Subnet for default network is removed from VPCNetworkConfiguration, skipped", "Subnet", *nsxSubnet.Path)
+				continue
+			}
+		}
 		canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet)
 		if err != nil {
 			log.Error(err, "Failed to check capacity of NSX Subnet", "Subnet", subnetName, "SubnetSet", subnetSet.Name, "Namespace", subnetSet.Namespace, "NSXSubnet", nsxSubnet.Id)
@@ -138,12 +177,16 @@ func IsNamespaceInTepLessMode(client k8sclient.Client, namespace string) (bool,
 	return networkInfo.VPCs[0].NetworkStack == v1alpha1.VLANBackedVPC, nil
 }
 
-func AllocateSubnetFromSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.SubnetSet, vpcService servicecommon.VPCServiceProvider, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, *types.UID, *sync.RWMutex, error) {
+func AllocateSubnetFromSubnetSet(client k8sclient.Client, apiReader k8sclient.Reader, subnetSet *v1alpha1.SubnetSet, vpcService servicecommon.VPCServiceProvider, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, *types.UID, *sync.RWMutex, error) {
 	if subnetSet.Spec.SubnetNames != nil {
 		// Use Read lock to allow SubnetPorts created parallelly on the pre-created SubnetSet
 		// and block the SubnetPort creation when the SubnetSet is updated
 		subnetSetLock := RLockSubnetSet(subnetSet.UID)
-		nsxSubnet, err := GetSubnetFromSubnetSet(client, subnetSet, subnetService, subnetPortService)
+		// Retrieve the SubnetSet again to avoid it being updated before acquiring the lock
+		if err := apiReader.Get(context.Background(), types.NamespacedName{Namespace: subnetSet.Namespace, Name: subnetSet.Name}, subnetSet); err != nil {
+			return "", &subnetSet.UID, subnetSetLock, err
+		}
+		nsxSubnet, err := GetSubnetFromSubnetSet(client, subnetSet, vpcService, subnetService, subnetPortService)
 		return nsxSubnet, &subnetSet.UID, subnetSetLock, err
 	}
 	// Use SubnetSet uuid lock to make sure when multiple ports are created on the same SubnetSet, only one Subnet will be created

pkg/controllers/common/utils_test.go

@@ -89,9 +89,22 @@ func TestGetVirtualMachineNameForSubnetPort(t *testing.T) {
 func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 	expectedSubnetPath := "subnet-path-1"
 	subnetSize := int64(32)
+	scheme := runtime.NewScheme()
+	v1alpha1.AddToScheme(scheme)
+	k8sclient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&v1alpha1.SubnetSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "subnetset-1",
+			Namespace: "ns-1",
+		},
+	}).Build()
+	patches := gomonkey.ApplyFunc(GetSubnetFromSubnetSet, func(client client.Client, subnetSet *v1alpha1.SubnetSet, vpcService servicecommon.VPCServiceProvider, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, error) {
+		return expectedSubnetPath, nil
+	})
+	defer patches.Reset()
 	tests := []struct {
 		name           string
 		prepareFunc    func(*testing.T, servicecommon.VPCServiceProvider, servicecommon.SubnetServiceProvider, servicecommon.SubnetPortServiceProvider)
+		subnetSet      *v1alpha1.SubnetSet
 		expectedErr    string
 		expectedResult string
 	}{
@@ -111,6 +124,12 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
 			},
 			expectedResult: expectedSubnetPath,
+			subnetSet: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "subnetset-1",
+					Namespace: "ns-1",
+				},
+			},
 		},
 		{
 			name: "ListVPCFailure",
@@ -121,6 +140,12 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 				vsp.(*pkg_mock.MockVPCServiceProvider).On("ListVPCInfo", mock.Anything).Return([]servicecommon.VPCResourceInfo{})
 			},
 			expectedErr: "no VPC found",
+			subnetSet: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "subnetset-1",
+					Namespace: "ns-1",
+				},
+			},
 		},
 		{
 			name: "CreateSubnet",
@@ -133,6 +158,28 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
 			},
 			expectedResult: expectedSubnetPath,
+			subnetSet: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "subnetset-1",
+					Namespace: "ns-1",
+				},
+			},
+		},
+		{
+			name: "PrecreatedSubnetSet",
+			prepareFunc: func(t *testing.T, vsp servicecommon.VPCServiceProvider, ssp servicecommon.SubnetServiceProvider, spsp servicecommon.SubnetPortServiceProvider) {
+
+			},
+			expectedResult: expectedSubnetPath,
+			subnetSet: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "subnetset-1",
+					Namespace: "ns-1",
+				},
+				Spec: v1alpha1.SubnetSetSpec{
+					SubnetNames: &[]string{"subnet-1"},
+				},
+			},
 		},
 	}
 	for _, tt := range tests {
@@ -141,12 +188,10 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 			ssp := &pkg_mock.MockSubnetServiceProvider{}
 			spsp := &pkg_mock.MockSubnetPortServiceProvider{}
 			tt.prepareFunc(t, vps, ssp, spsp)
-			subnetPath, _, _, err := AllocateSubnetFromSubnetSet(fake.NewClientBuilder().Build(), &v1alpha1.SubnetSet{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "subnetset-1",
-					Namespace: "ns-1",
-				},
-			}, vps, ssp, spsp)
+			subnetPath, subnetSetUID, subnetSetLock, err := AllocateSubnetFromSubnetSet(k8sclient, k8sclient, tt.subnetSet, vps, ssp, spsp)
+			if subnetSetLock != nil {
+				RUnlockSubnetSet(*subnetSetUID, subnetSetLock)
+			}
 			if tt.expectedErr != "" {
 				assert.Contains(t, err.Error(), tt.expectedErr)
 			} else {
@@ -655,14 +700,16 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 	_ = v1alpha1.AddToScheme(scheme)
 
 	ns := "ns-1"
-	path := "/orgs/default/projects/default/vpcs/ns-1/subnets/subnet-1"
-	nsxSubnet := &model.VpcSubnet{Id: servicecommon.String("subnet-1"), Path: &path}
+	path1 := "/orgs/default/projects/default/vpcs/ns-1/subnets/subnet-1"
+	nsxSubnet1 := &model.VpcSubnet{Id: servicecommon.String("subnet-1"), Path: &path1}
+	path2 := "/orgs/default/projects/default/vpcs/ns-1/subnets/subnet-2"
+	nsxSubnet2 := &model.VpcSubnet{Id: servicecommon.String("subnet-2"), Path: &path2}
 
 	tests := []struct {
 		name            string
 		subnetSet       *v1alpha1.SubnetSet
 		existingObjects []runtime.Object
-		mockSetup       func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider)
+		mockSetup       func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches
 		expectedPath    string
 		wantErr         bool
 		errContains     string
@@ -676,12 +723,13 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			existingObjects: []runtime.Object{
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "subnet-1", Namespace: ns}},
 			},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				// We use mock.Anything here to avoid pointer address issues
-				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet, nil)
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
 				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
+				return nil
 			},
-			expectedPath: path,
+			expectedPath: path1,
 			wantErr:      false,
 		},
 		{
@@ -694,13 +742,49 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "full-subnet", Namespace: ns}},
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "ok-subnet", Namespace: ns}},
 			},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
-				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet, nil)
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
 				// First call returns false (full), second call returns true
 				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, nil).Once()
 				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil).Once()
+				return nil
 			},
-			expectedPath: path,
+			expectedPath: path1,
+			wantErr:      false,
+		},
+		{
+			name: "Success - First subnet is removed from profile",
+			subnetSet: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "subnetset-2",
+					Namespace: ns,
+					Labels: map[string]string{
+						servicecommon.LabelDefaultNetwork: servicecommon.DefaultPodNetwork,
+					},
+				},
+				Spec: v1alpha1.SubnetSetSpec{SubnetNames: &[]string{"removed-subnet", "ok-subnet"}},
+			},
+			existingObjects: []runtime.Object{
+				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "removed-subnet", Namespace: ns}},
+				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "ok-subnet", Namespace: ns}},
+			},
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil).Once()
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet2, nil).Once()
+				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil).Once()
+				patches := gomonkey.ApplyFunc(GetVpcNetworkConfig, func(service servicecommon.VPCServiceProvider, ns string) (*v1alpha1.VPCNetworkConfiguration, error) {
+					return &v1alpha1.VPCNetworkConfiguration{
+						Spec: v1alpha1.VPCNetworkConfigurationSpec{
+							Subnets: []v1alpha1.SharedSubnet{
+								{Path: path1, PodDefault: false},
+								{Path: path2, PodDefault: true},
+							},
+						},
+					}, nil
+				})
+				return patches
+			},
+			expectedPath: path2,
 			wantErr:      false,
 		},
 		{
@@ -712,9 +796,10 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			existingObjects: []runtime.Object{
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "subnet-1", Namespace: ns}},
 			},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
-				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet, nil)
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
 				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, nil)
+				return nil
 			},
 			expectedPath: "",
 			wantErr:      true,
@@ -726,7 +811,8 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 				Spec:       v1alpha1.SubnetSetSpec{SubnetNames: &[]string{"missing-subnet"}},
 			},
 			existingObjects: []runtime.Object{},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
+				return nil
 			},
 			wantErr:     true,
 			errContains: "not found",
@@ -740,8 +826,9 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			existingObjects: []runtime.Object{
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "subnet-1", Namespace: ns}},
 			},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				ms.On("GetSubnetByCR", mock.Anything).Return(nil, errors.New("nsx-service-down"))
+				return nil
 			},
 			wantErr:     true,
 			errContains: "nsx-service-down",
@@ -755,9 +842,10 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			existingObjects: []runtime.Object{
 				&v1alpha1.Subnet{ObjectMeta: metav1.ObjectMeta{Name: "subnet-1", Namespace: ns}},
 			},
-			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) {
-				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet, nil)
+			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
+				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
 				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, errors.New("capacity-check-failed"))
+				return nil
 			},
 			wantErr:     true,
 			errContains: "capacity-check-failed",
@@ -770,10 +858,14 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.existingObjects...).Build()
 			mockSubnetSvc := new(pkg_mock.MockSubnetServiceProvider)
 			mockPortSvc := new(pkg_mock.MockSubnetPortServiceProvider)
-			tt.mockSetup(mockSubnetSvc, mockPortSvc)
+			mockVpcSvc := new(pkg_mock.MockVPCServiceProvider)
+			patches := tt.mockSetup(mockSubnetSvc, mockPortSvc)
+			if patches != nil {
+				defer patches.Reset()
+			}
 
 			// Execute
-			result, err := GetSubnetFromSubnetSet(client, tt.subnetSet, mockSubnetSvc, mockPortSvc)
+			result, err := GetSubnetFromSubnetSet(client, tt.subnetSet, mockVpcSvc, mockSubnetSvc, mockPortSvc)
 
 			// Assert
 			if tt.wantErr {