diff --git a/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetconnectionbindingmaps.yaml b/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetconnectionbindingmaps.yaml index 9e74b3664..4026dee2b 100644 --- a/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetconnectionbindingmaps.yaml +++ b/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetconnectionbindingmaps.yaml @@ -79,14 +79,13 @@ spec: vlanTrafficTag: description: |- VLANTrafficTag is the VLAN tag configured in the binding. Note, the value of VLANTrafficTag should be - unique on the target Subnet or SubnetSet. + unique on the target Subnet or SubnetSet. When omitted, the operator auto-allocates a VLAN ID. format: int64 maximum: 4094 minimum: 0 type: integer required: - subnetName - - vlanTrafficTag type: object x-kubernetes-validations: - message: Only one of targetSubnetSetName or targetSubnetName can be diff --git a/build/yaml/samples/nsx_v1alpha1_subnetconnectionbindingmap.yaml b/build/yaml/samples/nsx_v1alpha1_subnetconnectionbindingmap.yaml index 073a1f1cc..746595a92 100644 --- a/build/yaml/samples/nsx_v1alpha1_subnetconnectionbindingmap.yaml +++ b/build/yaml/samples/nsx_v1alpha1_subnetconnectionbindingmap.yaml @@ -17,3 +17,13 @@ spec: subnetName: subnet-sample targetSubnetName: subnet1 vlanTrafficTag: 202 +--- +apiVersion: crd.nsx.vmware.com/v1alpha1 +kind: SubnetConnectionBindingMap +metadata: + name: binding-auto + namespace: default +spec: + subnetName: vlan-ext-subnet + targetSubnetSetName: vm-default + # vlanTrafficTag omitted: operator auto-allocates and patches spec.vlanTrafficTag diff --git a/pkg/apis/vpc/v1alpha1/subnetconnectionbindingmap_types.go b/pkg/apis/vpc/v1alpha1/subnetconnectionbindingmap_types.go index b9f1db1a4..7611cc9f8 100644 --- a/pkg/apis/vpc/v1alpha1/subnetconnectionbindingmap_types.go +++ b/pkg/apis/vpc/v1alpha1/subnetconnectionbindingmap_types.go @@ -18,11 +18,11 @@ type SubnetConnectionBindingMapSpec struct { // +kubebuilder:validation:Optional TargetSubnetName string `json:"targetSubnetName,omitempty"` // VLANTrafficTag is the VLAN tag configured in the binding. Note, the value of VLANTrafficTag should be - // unique on the target Subnet or SubnetSet. + // unique on the target Subnet or SubnetSet. When omitted, the operator auto-allocates a VLAN ID. // +kubebuilder:validation:Maximum:=4094 // +kubebuilder:validation:Minimum:=0 - // +kubebuilder:validation:Required - VLANTrafficTag int64 `json:"vlanTrafficTag"` + // +kubebuilder:validation:Optional + VLANTrafficTag *int64 `json:"vlanTrafficTag,omitempty"` } // SubnetConnectionBindingMapStatus defines the observed state of SubnetConnectionBindingMap. @@ -60,6 +60,16 @@ type SubnetConnectionBindingMapList struct { Items []SubnetConnectionBindingMap `json:"items,omitempty"` } +// VLANTrafficTagPtr returns a pointer to the given VLAN traffic tag value. +func VLANTrafficTagPtr(v int64) *int64 { + return &v +} + +// HasVlanTrafficTag reports whether spec.vlanTrafficTag is set. +func (s *SubnetConnectionBindingMapSpec) HasVlanTrafficTag() bool { + return s.VLANTrafficTag != nil +} + func init() { SchemeBuilder.Register(&SubnetConnectionBindingMap{}, &SubnetConnectionBindingMapList{}) } diff --git a/pkg/apis/vpc/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/vpc/v1alpha1/zz_generated.deepcopy.go index 19d668ac2..c8bfd2af8 100644 --- a/pkg/apis/vpc/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/vpc/v1alpha1/zz_generated.deepcopy.go @@ -1034,6 +1034,11 @@ func (in *SubnetConnectionBindingMapList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SubnetConnectionBindingMapSpec) DeepCopyInto(out *SubnetConnectionBindingMapSpec) { *out = *in + if in.VLANTrafficTag != nil { + in, out := &in.VLANTrafficTag, &out.VLANTrafficTag + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubnetConnectionBindingMapSpec. diff --git a/pkg/controllers/common/dependency_watcher_test.go b/pkg/controllers/common/dependency_watcher_test.go index 9bf1fda49..748be4e8a 100644 --- a/pkg/controllers/common/dependency_watcher_test.go +++ b/pkg/controllers/common/dependency_watcher_test.go @@ -98,7 +98,7 @@ func TestPredicateFuncsBindingMap(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetName: "parent1", - VLANTrafficTag: 202, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(202), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -117,7 +117,7 @@ func TestPredicateFuncsBindingMap(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetName: "parent1", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ diff --git a/pkg/controllers/subnet/subnetbinding_handler_test.go b/pkg/controllers/subnet/subnetbinding_handler_test.go index deec1d674..2cf794621 100644 --- a/pkg/controllers/subnet/subnetbinding_handler_test.go +++ b/pkg/controllers/subnet/subnetbinding_handler_test.go @@ -5,7 +5,7 @@ import ( "reflect" "testing" - "github.com/agiledragon/gomonkey/v2" + gomonkey "github.com/agiledragon/gomonkey/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" @@ -33,7 +33,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child1", TargetSubnetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } @@ -45,7 +45,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child1", TargetSubnetName: "parent2", - VLANTrafficTag: 102, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(102), }, } @@ -57,7 +57,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child1", TargetSubnetSetName: "parentSet2", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } @@ -69,7 +69,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child2", TargetSubnetName: "parent3", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } @@ -172,7 +172,7 @@ func TestGetSubnetBindingCRsBySubnet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "subnet1", TargetSubnetName: "subnet2", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), }, } binding2 := &v1alpha1.SubnetConnectionBindingMap{ @@ -183,7 +183,7 @@ func TestGetSubnetBindingCRsBySubnet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "subnet2", TargetSubnetName: "subnet3", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), }, } newScheme := runtime.NewScheme() diff --git a/pkg/controllers/subnetbinding/subnetbinding_controller.go b/pkg/controllers/subnetbinding/subnetbinding_controller.go index 88b2c969b..73947b935 100644 --- a/pkg/controllers/subnetbinding/subnetbinding_controller.go +++ b/pkg/controllers/subnetbinding/subnetbinding_controller.go @@ -27,6 +27,7 @@ import ( servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetbinding" + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vlanpool" ) var ( @@ -45,6 +46,7 @@ type Reconciler struct { Scheme *runtime.Scheme SubnetService *subnet.SubnetService SubnetBindingService *subnetbinding.BindingService + VlanPoolService *vlanpool.Service StatusUpdater common.StatusUpdater } @@ -76,6 +78,7 @@ func NewReconciler(mgr ctrl.Manager, subnetService *subnet.SubnetService, subnet Scheme: mgr.GetScheme(), SubnetService: subnetService, SubnetBindingService: subnetBindingService, + VlanPoolService: vlanpool.NewService(subnetBindingService), StatusUpdater: common.NewStatusUpdater(mgr.GetClient(), subnetBindingService.NSXConfig, recorder, common.MetricResTypeSubnetConnectionBindingMap, "SubnetConnectionBindingMap", "SubnetConnectionBindingMap"), } } @@ -118,6 +121,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return common.ResultRequeueAfter60sec, nil } + if vlanErr := r.reconcileVlanTrafficTag(ctx, bindingMapCR, parentSubnetPaths); vlanErr != nil { + r.StatusUpdater.UpdateFail(ctx, bindingMapCR, vlanErr, "failed to reconcile VLAN traffic tag", updateBindingMapStatusWithUnreadyCondition, "VlanAllocationFailed", vlanErr.message) + if !vlanErr.retry { + return common.ResultNormal, nil + } + return common.ResultRequeue, nil + } + if err := r.SubnetBindingService.CreateOrUpdateSubnetConnectionBindingMap(bindingMapCR, childSubnetPath, parentSubnetPaths); err != nil { // Update SubnetConnectionBindingMap with not-ready condition r.StatusUpdater.UpdateFail(ctx, bindingMapCR, err, "failure to configure SubnetConnectionBindingMaps on NSX", updateBindingMapStatusWithUnreadyCondition, "ConfigureFailed", fmt.Sprintf("Failed to realize SubnetConnectionBindingMap %s on NSX", req.Name)) @@ -276,6 +287,70 @@ func (r *Reconciler) validateDependency(ctx context.Context, bindingMap *v1alpha return childSubnetPath, parentSubnetPaths, nil } +func (r *Reconciler) reconcileVlanTrafficTag(ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, parentSubnetPaths []string) *errorWithRetry { + if bindingMap.Spec.HasVlanTrafficTag() { + vlan := *bindingMap.Spec.VLANTrafficTag + if err := r.VlanPoolService.ValidateManualVlan(parentSubnetPaths, vlan, string(bindingMap.UID)); err != nil { + return &errorWithRetry{ + message: err.Error(), + error: err, + retry: true, + } + } + return nil + } + + childSubnet := &v1alpha1.Subnet{} + childSubnetKey := types.NamespacedName{Namespace: bindingMap.Namespace, Name: bindingMap.Spec.SubnetName} + if err := r.Client.Get(ctx, childSubnetKey, childSubnet); err != nil { + log.Error(err, "Failed to get Subnet CR for VLAN auto allocation", "Subnet", childSubnetKey.String()) + return &errorWithRetry{ + message: fmt.Sprintf("Unable to get Subnet CR %s for VLAN auto allocation", bindingMap.Spec.SubnetName), + error: fmt.Errorf("failed to get Subnet %s in Namespace %s: %w", bindingMap.Spec.SubnetName, bindingMap.Namespace, err), + retry: false, + } + } + + preferred := int64(-1) + if childSubnet.Status.VLANExtension.VLANID != 0 { + preferred = int64(childSubnet.Status.VLANExtension.VLANID) + } + + vlan, err := r.VlanPoolService.Allocate(parentSubnetPaths, string(bindingMap.UID), preferred) + if err != nil { + return &errorWithRetry{ + message: err.Error(), + error: err, + retry: true, + } + } + + if err := r.patchSpecVlanTrafficTag(ctx, bindingMap, vlan); err != nil { + return &errorWithRetry{ + message: fmt.Sprintf("Failed to update spec.vlanTrafficTag: %v", err), + error: err, + retry: true, + } + } + bindingMap.Spec.VLANTrafficTag = v1alpha1.VLANTrafficTagPtr(vlan) + return nil +} + +func (r *Reconciler) patchSpecVlanTrafficTag(ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, vlan int64) error { + key := types.NamespacedName{Namespace: bindingMap.Namespace, Name: bindingMap.Name} + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1alpha1.SubnetConnectionBindingMap{} + if err := r.Client.Get(ctx, key, latest); err != nil { + return err + } + if latest.Spec.HasVlanTrafficTag() && *latest.Spec.VLANTrafficTag == vlan { + return nil + } + latest.Spec.VLANTrafficTag = v1alpha1.VLANTrafficTagPtr(vlan) + return r.Client.Update(ctx, latest) + }) +} + func (r *Reconciler) validateVpcSubnetsBySubnetCR(ctx context.Context, namespace, name string, isTarget bool) ([]string, *v1alpha1.Subnet, *errorWithRetry) { subnetCR := &v1alpha1.Subnet{} subnetKey := types.NamespacedName{Namespace: namespace, Name: name} diff --git a/pkg/controllers/subnetbinding/subnetbinding_controller_test.go b/pkg/controllers/subnetbinding/subnetbinding_controller_test.go index 28fc34210..c4821a891 100644 --- a/pkg/controllers/subnetbinding/subnetbinding_controller_test.go +++ b/pkg/controllers/subnetbinding/subnetbinding_controller_test.go @@ -6,7 +6,7 @@ import ( "reflect" "testing" - "github.com/agiledragon/gomonkey/v2" + gomonkey "github.com/agiledragon/gomonkey/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" @@ -32,6 +32,7 @@ import ( "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetbinding" + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vlanpool" ) type fakeRecorder struct{} @@ -102,7 +103,7 @@ func TestReconcile(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parentSubnetSet", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } for _, tc := range []struct { @@ -184,6 +185,9 @@ func TestReconcile(t *testing.T) { patches := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "validateDependency", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap) (string, []string, *errorWithRetry) { return "/subnet-child", []string{"/subnet-parent"}, nil }) + patches.ApplyPrivateMethod(reflect.TypeOf(r), "reconcileVlanTrafficTag", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, parentSubnetPaths []string) *errorWithRetry { + return nil + }) patches.ApplyMethod(reflect.TypeOf(r.SubnetBindingService), "CreateOrUpdateSubnetConnectionBindingMap", func(_ *subnetbinding.BindingService, subnetBinding *v1alpha1.SubnetConnectionBindingMap, childSubnetPath string, parentSubnetPaths []string) error { return fmt.Errorf("failed to configure NSX") @@ -198,6 +202,9 @@ func TestReconcile(t *testing.T) { patches := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "validateDependency", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap) (string, []string, *errorWithRetry) { return "/subnet-child", []string{"/subnet-parent"}, nil }) + patches.ApplyPrivateMethod(reflect.TypeOf(r), "reconcileVlanTrafficTag", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, parentSubnetPaths []string) *errorWithRetry { + return nil + }) patches.ApplyMethod(reflect.TypeOf(r.SubnetBindingService), "CreateOrUpdateSubnetConnectionBindingMap", func(_ *subnetbinding.BindingService, subnetBinding *v1alpha1.SubnetConnectionBindingMap, childSubnetPath string, parentSubnetPaths []string) error { return nil @@ -287,7 +294,7 @@ func TestValidateDependency(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: childSubnet, TargetSubnetName: targetSubnet, - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } bindingCR2 := &v1alpha1.SubnetConnectionBindingMap{ @@ -298,7 +305,7 @@ func TestValidateDependency(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: childSubnet, TargetSubnetSetName: targetSubnetSet, - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } @@ -740,7 +747,7 @@ func TestUpdateBindingMapStatusWithConditions(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } bindingMap2 := &v1alpha1.SubnetConnectionBindingMap{ @@ -751,7 +758,7 @@ func TestUpdateBindingMapStatusWithConditions(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -770,7 +777,7 @@ func TestUpdateBindingMapStatusWithConditions(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -792,7 +799,7 @@ func TestUpdateBindingMapStatusWithConditions(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -880,7 +887,7 @@ func TestUpdateBindingMapConditionWithRetry(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, }, condition: v1alpha1.Condition{ @@ -903,7 +910,7 @@ func TestUpdateBindingMapConditionWithRetry(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -936,7 +943,7 @@ func TestUpdateBindingMapConditionWithRetry(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -1082,7 +1089,7 @@ func TestPredicateFuncsBindingMaps(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -1101,7 +1108,7 @@ func TestPredicateFuncsBindingMaps(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 102, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(102), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -1120,7 +1127,7 @@ func TestPredicateFuncsBindingMaps(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, Status: v1alpha1.SubnetConnectionBindingMapStatus{ Conditions: []v1alpha1.Condition{ @@ -1265,3 +1272,165 @@ func createFakeReconciler(objs ...client.Object) *Reconciler { return NewReconciler(mgr, subnetService, bindingService) } + +func TestReconcileVlanTrafficTag(t *testing.T) { + bm1 := &v1alpha1.SubnetConnectionBindingMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "bm-1", + UID: "uuid-1", + }, + Spec: v1alpha1.SubnetConnectionBindingMapSpec{ + SubnetName: "subnet-child-1", + TargetSubnetName: "subnet-parent-1", + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), + }, + } + bm2 := &v1alpha1.SubnetConnectionBindingMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "bm-2", + UID: "uuid-2", + }, + Spec: v1alpha1.SubnetConnectionBindingMapSpec{ + SubnetName: "subnet-child-2", + TargetSubnetName: "subnet-parent-2", + }, + } + childSubnet := &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "subnet-child-2", + }, + Status: v1alpha1.SubnetStatus{ + VLANExtension: v1alpha1.VLANExtension{ + VLANID: 300, + }, + }, + } + + for _, tc := range []struct { + name string + bindingMap *v1alpha1.SubnetConnectionBindingMap + objects []client.Object + patches func(t *testing.T, r *Reconciler) *gomonkey.Patches + expErr bool + expVlan *int64 + }{ + { + name: "Manual VLAN successfully validated", + bindingMap: bm1, + patches: func(t *testing.T, r *Reconciler) *gomonkey.Patches { + return gomonkey.ApplyMethod(reflect.TypeOf(r.VlanPoolService), "ValidateManualVlan", func(_ *vlanpool.Service, parentSubnetPaths []string, vlan int64, excludeCRUID string) error { + return nil + }) + }, + expErr: false, + expVlan: v1alpha1.VLANTrafficTagPtr(201), + }, + { + name: "Manual VLAN validation failed", + bindingMap: bm1, + patches: func(t *testing.T, r *Reconciler) *gomonkey.Patches { + return gomonkey.ApplyMethod(reflect.TypeOf(r.VlanPoolService), "ValidateManualVlan", func(_ *vlanpool.Service, parentSubnetPaths []string, vlan int64, excludeCRUID string) error { + return fmt.Errorf("conflict") + }) + }, + expErr: true, + }, + { + name: "Auto allocate VLAN fails on getting child Subnet", + bindingMap: bm2, + patches: func(t *testing.T, r *Reconciler) *gomonkey.Patches { + return gomonkey.ApplyMethod(reflect.TypeOf(r.Client), "Get", func(_ client.Client, ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return fmt.Errorf("cr not found") + }) + }, + expErr: true, + }, + { + name: "Auto allocate VLAN successfully", + bindingMap: bm2, + objects: []client.Object{childSubnet, bm2}, + patches: func(t *testing.T, r *Reconciler) *gomonkey.Patches { + patches := gomonkey.ApplyMethod(reflect.TypeOf(r.VlanPoolService), "Allocate", func(_ *vlanpool.Service, parentSubnetPaths []string, excludeCRUID string, preferred int64) (int64, error) { + return 300, nil + }) + patches.ApplyPrivateMethod(reflect.TypeOf(r), "patchSpecVlanTrafficTag", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, vlan int64) error { + return nil + }) + return patches + }, + expErr: false, + expVlan: v1alpha1.VLANTrafficTagPtr(300), + }, + { + name: "Auto allocate VLAN fails on patch spec", + bindingMap: bm2, + objects: []client.Object{childSubnet, bm2}, + patches: func(t *testing.T, r *Reconciler) *gomonkey.Patches { + patches := gomonkey.ApplyMethod(reflect.TypeOf(r.VlanPoolService), "Allocate", func(_ *vlanpool.Service, parentSubnetPaths []string, excludeCRUID string, preferred int64) (int64, error) { + return 300, nil + }) + patches.ApplyPrivateMethod(reflect.TypeOf(r), "patchSpecVlanTrafficTag", func(_ *Reconciler, ctx context.Context, bindingMap *v1alpha1.SubnetConnectionBindingMap, vlan int64) error { + return fmt.Errorf("patch error") + }) + return patches + }, + expErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.TODO() + r := createFakeReconciler(tc.objects...) + if tc.patches != nil { + patches := tc.patches(t, r) + defer patches.Reset() + } + err := r.reconcileVlanTrafficTag(ctx, tc.bindingMap, []string{"/parent"}) + if tc.expErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + if tc.expVlan != nil { + assert.Equal(t, *tc.expVlan, *tc.bindingMap.Spec.VLANTrafficTag) + } + } + }) + } +} + +func TestPatchSpecVlanTrafficTag(t *testing.T) { + bm1 := &v1alpha1.SubnetConnectionBindingMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "bm-1", + }, + Spec: v1alpha1.SubnetConnectionBindingMapSpec{ + SubnetName: "subnet-child-1", + TargetSubnetName: "subnet-parent-1", + }, + } + r := createFakeReconciler(bm1) + newScheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(newScheme)) + utilruntime.Must(v1alpha1.AddToScheme(newScheme)) + r.Client = fake.NewClientBuilder(). + WithScheme(newScheme). + WithObjects(bm1). + Build() + + ctx := context.TODO() + // Patch new VLAN + err := r.patchSpecVlanTrafficTag(ctx, bm1, 101) + assert.NoError(t, err) + + updatedBM := &v1alpha1.SubnetConnectionBindingMap{} + err = r.Client.Get(ctx, types.NamespacedName{Namespace: "default", Name: "bm-1"}, updatedBM) + assert.NoError(t, err) + assert.Equal(t, int64(101), *updatedBM.Spec.VLANTrafficTag) + + // Patch same VLAN should succeed immediately + err = r.patchSpecVlanTrafficTag(ctx, bm1, 101) + assert.NoError(t, err) +} diff --git a/pkg/controllers/subnetbinding/subnets_handler_test.go b/pkg/controllers/subnetbinding/subnets_handler_test.go index 8264c917b..9b60fb106 100644 --- a/pkg/controllers/subnetbinding/subnets_handler_test.go +++ b/pkg/controllers/subnetbinding/subnets_handler_test.go @@ -163,7 +163,7 @@ func TestRequeueSubnetConnectionBindingMapsBySubnet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } req := reconcile.Request{ @@ -210,7 +210,7 @@ func TestRequeueSubnetConnectionBindingMapsBySubnetSet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetSetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } newScheme := runtime.NewScheme() diff --git a/pkg/controllers/subnetset/subnetbinding_handler_test.go b/pkg/controllers/subnetset/subnetbinding_handler_test.go index d5dbaa302..d4228c0ea 100644 --- a/pkg/controllers/subnetset/subnetbinding_handler_test.go +++ b/pkg/controllers/subnetset/subnetbinding_handler_test.go @@ -5,7 +5,7 @@ import ( "reflect" "testing" - "github.com/agiledragon/gomonkey/v2" + gomonkey "github.com/agiledragon/gomonkey/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" @@ -33,7 +33,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", TargetSubnetName: "parent", - VLANTrafficTag: 101, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(101), }, } @@ -45,7 +45,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child1", TargetSubnetSetName: "parentSet", - VLANTrafficTag: 102, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(102), }, } @@ -57,7 +57,7 @@ var ( Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child1", TargetSubnetSetName: "parentSet2", - VLANTrafficTag: 102, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(102), }, } @@ -148,7 +148,7 @@ func TestGetSubnetBindingCRsBySubnet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "subnet1", TargetSubnetSetName: "subnetset1", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), }, } binding2 := &v1alpha1.SubnetConnectionBindingMap{ @@ -159,7 +159,7 @@ func TestGetSubnetBindingCRsBySubnet(t *testing.T) { Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "subnet2", TargetSubnetName: "subnet1", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), }, } newScheme := runtime.NewScheme() diff --git a/pkg/nsx/services/subnetbinding/builder.go b/pkg/nsx/services/subnetbinding/builder.go index 78b6e4250..fe2921e37 100644 --- a/pkg/nsx/services/subnetbinding/builder.go +++ b/pkg/nsx/services/subnetbinding/builder.go @@ -28,10 +28,14 @@ func (s *BindingService) buildSubnetBindings(binding *v1alpha1.SubnetConnectionB log.Error(err, "failed to parse parent Subnet path, ignore it") continue } + if !binding.Spec.HasVlanTrafficTag() { + log.Error(nil, "SubnetConnectionBindingMap has no vlanTrafficTag", "SubnetConnectionBindingMap", binding.Namespace+"/"+binding.Name) + continue + } bindingMaps[i] = &model.SubnetConnectionBindingMap{ Id: String(s.buildSubnetBindingID(binding, vpcSubnetInfo.ID)), DisplayName: String(binding.Name), - VlanTrafficTag: Int64(binding.Spec.VLANTrafficTag), + VlanTrafficTag: Int64(*binding.Spec.VLANTrafficTag), SubnetPath: &path, Tags: tags, } diff --git a/pkg/nsx/services/subnetbinding/store_test.go b/pkg/nsx/services/subnetbinding/store_test.go index ed4281d69..8a7904871 100644 --- a/pkg/nsx/services/subnetbinding/store_test.go +++ b/pkg/nsx/services/subnetbinding/store_test.go @@ -25,7 +25,7 @@ var ( }, Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child", - VLANTrafficTag: 201, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(201), TargetSubnetSetName: "parent", }, } @@ -37,7 +37,7 @@ var ( }, Spec: v1alpha1.SubnetConnectionBindingMapSpec{ SubnetName: "child2", - VLANTrafficTag: 202, + VLANTrafficTag: v1alpha1.VLANTrafficTagPtr(202), TargetSubnetSetName: "parent2", }, } diff --git a/pkg/nsx/services/subnetbinding/vlan_allocation.go b/pkg/nsx/services/subnetbinding/vlan_allocation.go new file mode 100644 index 000000000..4e562c08a --- /dev/null +++ b/pkg/nsx/services/subnetbinding/vlan_allocation.go @@ -0,0 +1,112 @@ +package subnetbinding + +import ( + "fmt" + "strconv" + "strings" + + "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" + "k8s.io/apimachinery/pkg/util/sets" + + servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" + nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util" +) + +const searchPageSize = int64(1000) + +// CollectUsedVlansOnParentSubnets returns VLAN tags already used on the given parent Subnet paths by +// querying NSX (includes bindings not created by this Supervisor). +func (s *BindingService) CollectUsedVlansOnParentSubnets(parentSubnetPaths []string, excludeCRUID string) (sets.Set[int], error) { + used := sets.New[int]() + for _, parentPath := range parentSubnetPaths { + bindings, err := s.listBindingMapsByParentSubnetPath(parentPath) + if err != nil { + return nil, err + } + for _, bm := range bindings { + if excludeCRUID != "" && bindingMapCRUID(bm) == excludeCRUID { + continue + } + if bm.VlanTrafficTag != nil { + used.Insert(int(*bm.VlanTrafficTag)) + } + } + } + return used, nil +} + +func bindingMapCRUID(bm *model.SubnetConnectionBindingMap) string { + for _, tag := range bm.Tags { + if tag.Scope != nil && *tag.Scope == servicecommon.TagScopeSubnetBindingCRUID && tag.Tag != nil { + return *tag.Tag + } + } + return "" +} + +func (s *BindingService) listBindingMapsByParentSubnetPath(parentSubnetPath string) ([]*model.SubnetConnectionBindingMap, error) { + if s == nil || s.NSXClient == nil || s.NSXClient.QueryClient == nil { + return nil, fmt.Errorf("NSX query client is not initialized") + } + + pathEscaped := strings.ReplaceAll(parentSubnetPath, "/", "\\/") + queryParam := fmt.Sprintf("%s:%s AND marked_for_delete:false AND subnet_path:%s", + servicecommon.ResourceType, ResourceTypeSubnetConnectionBindingMap, pathEscaped) + + converter := servicecommon.NewConverter() + bindings := make([]*model.SubnetConnectionBindingMap, 0) + var cursor *string + pageSize := searchPageSize + + for { + response, err := s.NSXClient.QueryClient.List(queryParam, cursor, nil, &pageSize, nil, nil) + if err != nil { + err = servicecommon.TransError(err) + if _, ok := err.(nsxutil.PageMaxError); ok { + servicecommon.DecrementPageSize(&pageSize) + continue + } + return nil, err + } + + for _, entity := range response.Results { + obj, convErrs := converter.ConvertToGolang(entity, model.SubnetConnectionBindingMapBindingType()) + if len(convErrs) > 0 { + log.Error(convErrs[0], "Failed to convert NSX SubnetConnectionBindingMap from search result") + continue + } + bmV, ok := obj.(model.SubnetConnectionBindingMap) + if !ok { + log.Info("Skipping unexpected search result type for SubnetConnectionBindingMap") + continue + } + bindings = append(bindings, &bmV) + } + + if response.Cursor == nil { + break + } + c, err := strconv.Atoi(*response.Cursor) + if err != nil { + break + } + if response.ResultCount != nil && int64(c) >= *response.ResultCount { + break + } + cursor = response.Cursor + } + + return bindings, nil +} + +// ValidateVlanOnParentSubnets checks that vlan is not already used on any of the parent Subnet paths. +func (s *BindingService) ValidateVlanOnParentSubnets(parentSubnetPaths []string, vlan int64, excludeCRUID string) error { + used, err := s.CollectUsedVlansOnParentSubnets(parentSubnetPaths, excludeCRUID) + if err != nil { + return err + } + if used.Has(int(vlan)) { + return fmt.Errorf("vlanTrafficTag %d is already used on target Subnet or SubnetSet", vlan) + } + return nil +} diff --git a/pkg/nsx/services/subnetbinding/vlan_allocation_test.go b/pkg/nsx/services/subnetbinding/vlan_allocation_test.go new file mode 100644 index 000000000..eaf4ab6eb --- /dev/null +++ b/pkg/nsx/services/subnetbinding/vlan_allocation_test.go @@ -0,0 +1,219 @@ +package subnetbinding + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vmware/vsphere-automation-sdk-go/runtime/data" + "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" + "go.uber.org/mock/gomock" + + search_mocks "github.com/vmware-tanzu/nsx-operator/pkg/mock/searchclient" + "github.com/vmware-tanzu/nsx-operator/pkg/nsx" + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" + nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util" +) + +func TestCollectUsedVlansOnParentSubnets(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + fakeQueryClient := search_mocks.NewMockQueryClient(ctrl) + svc := &BindingService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + QueryClient: fakeQueryClient, + }, + }, + } + + bm1 := &model.SubnetConnectionBindingMap{ + Id: String("id-1"), + DisplayName: String("binding1"), + SubnetPath: String("/parent1"), + ParentPath: String("/child1"), + Path: String("/child1/subnet-connection-binding-maps/id-1"), + VlanTrafficTag: Int64(101), + ResourceType: String(ResourceTypeSubnetConnectionBindingMap), + Tags: []model.Tag{ + {Scope: String(common.TagScopeSubnetBindingCRUID), Tag: String("uuid-1")}, + }, + } + bm2 := &model.SubnetConnectionBindingMap{ + Id: String("id-2"), + DisplayName: String("binding2"), + SubnetPath: String("/parent1"), + ParentPath: String("/child1"), + Path: String("/child1/subnet-connection-binding-maps/id-2"), + VlanTrafficTag: Int64(102), + ResourceType: String(ResourceTypeSubnetConnectionBindingMap), + Tags: []model.Tag{ + {Scope: String(common.TagScopeSubnetBindingCRUID), Tag: String("uuid-2")}, + }, + } + bm3 := &model.SubnetConnectionBindingMap{ + Id: String("id-3"), + DisplayName: String("binding3"), + SubnetPath: String("/parent1"), + ParentPath: String("/child1"), + Path: String("/child1/subnet-connection-binding-maps/id-3"), + VlanTrafficTag: nil, // no tag + ResourceType: String(ResourceTypeSubnetConnectionBindingMap), + } + + converter := common.NewConverter() + dv1, _ := converter.ConvertToVapi(bm1, model.SubnetConnectionBindingMapBindingType()) + dv2, _ := converter.ConvertToVapi(bm2, model.SubnetConnectionBindingMapBindingType()) + dv3, _ := converter.ConvertToVapi(bm3, model.SubnetConnectionBindingMapBindingType()) + + resultCount := int64(3) + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{ + ResultCount: &resultCount, + Results: []*data.StructValue{ + dv1.(*data.StructValue), + dv2.(*data.StructValue), + dv3.(*data.StructValue), + }, + }, nil).Times(1) + + used, err := svc.CollectUsedVlansOnParentSubnets([]string{"/parent1"}, "uuid-1") // Exclude uuid-1 + require.NoError(t, err) + + assert.False(t, used.Has(101)) // Excluded + assert.True(t, used.Has(102)) // Included + assert.Equal(t, 1, used.Len()) +} + +func TestListBindingMapsByParentSubnetPath(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + t.Run("Query client not initialized", func(t *testing.T) { + svc := &BindingService{} + _, err := svc.listBindingMapsByParentSubnetPath("/parent") + require.Error(t, err) + assert.Contains(t, err.Error(), "NSX query client is not initialized") + }) + + t.Run("Success single page", func(t *testing.T) { + fakeQueryClient := search_mocks.NewMockQueryClient(ctrl) + svc := &BindingService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + QueryClient: fakeQueryClient, + }, + }, + } + bm := &model.SubnetConnectionBindingMap{ + Id: String("id-1"), + DisplayName: String("binding1"), + SubnetPath: String("/parent1"), + ParentPath: String("/child1"), + Path: String("/child1/subnet-connection-binding-maps/id-1"), + ResourceType: String(ResourceTypeSubnetConnectionBindingMap), + } + dv, _ := common.NewConverter().ConvertToVapi(bm, model.SubnetConnectionBindingMapBindingType()) + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{ + Results: []*data.StructValue{dv.(*data.StructValue)}, + }, nil).Times(1) + + bindings, err := svc.listBindingMapsByParentSubnetPath("/parent") + require.NoError(t, err) + assert.Len(t, bindings, 1) + assert.Equal(t, "id-1", *bindings[0].Id) + }) + + t.Run("Page max error retry", func(t *testing.T) { + fakeQueryClient := search_mocks.NewMockQueryClient(ctrl) + svc := &BindingService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + QueryClient: fakeQueryClient, + }, + }, + } + + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{}, nsxutil.PageMaxError{}).Times(1) + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{ + Results: []*data.StructValue{}, + }, nil).Times(1) + + bindings, err := svc.listBindingMapsByParentSubnetPath("/parent") + require.NoError(t, err) + assert.Len(t, bindings, 0) + }) + + t.Run("Error listing", func(t *testing.T) { + fakeQueryClient := search_mocks.NewMockQueryClient(ctrl) + svc := &BindingService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + QueryClient: fakeQueryClient, + }, + }, + } + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{}, fmt.Errorf("search error")).Times(1) + + _, err := svc.listBindingMapsByParentSubnetPath("/parent") + require.Error(t, err) + }) +} + +func TestValidateVlanOnParentSubnets(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + fakeQueryClient := search_mocks.NewMockQueryClient(ctrl) + svc := &BindingService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + QueryClient: fakeQueryClient, + }, + }, + } + + bm := &model.SubnetConnectionBindingMap{ + Id: String("id-1"), + DisplayName: String("binding1"), + SubnetPath: String("/parent1"), + ParentPath: String("/child1"), + Path: String("/child1/subnet-connection-binding-maps/id-1"), + VlanTrafficTag: Int64(201), + ResourceType: String(ResourceTypeSubnetConnectionBindingMap), + } + dv, _ := common.NewConverter().ConvertToVapi(bm, model.SubnetConnectionBindingMapBindingType()) + + // Test conflict + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{ + Results: []*data.StructValue{dv.(*data.StructValue)}, + }, nil).Times(1) + + err := svc.ValidateVlanOnParentSubnets([]string{"/parent"}, 201, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "already used") + + // Test success + fakeQueryClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(model.SearchResponse{ + Results: []*data.StructValue{dv.(*data.StructValue)}, + }, nil).Times(1) + + err = svc.ValidateVlanOnParentSubnets([]string{"/parent"}, 202, "") + require.NoError(t, err) +} + +func TestBindingMapCRUID(t *testing.T) { + bm := &model.SubnetConnectionBindingMap{ + Tags: []model.Tag{ + {Scope: String(common.TagScopeSubnetBindingCRUID), Tag: String("test-uuid")}, + {Scope: String(common.TagScopeNamespace), Tag: String("default")}, + }, + } + uid := bindingMapCRUID(bm) + assert.Equal(t, "test-uuid", uid) + + bm2 := &model.SubnetConnectionBindingMap{} + uid2 := bindingMapCRUID(bm2) + assert.Equal(t, "", uid2) +} diff --git a/pkg/nsx/services/vlanpool/vlanpool.go b/pkg/nsx/services/vlanpool/vlanpool.go new file mode 100644 index 000000000..c3b2e9d88 --- /dev/null +++ b/pkg/nsx/services/vlanpool/vlanpool.go @@ -0,0 +1,60 @@ +package vlanpool + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/util/sets" + + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetbinding" +) + +const ( + poolStart = int64(0) + poolEnd = int64(4094) +) + +// VlanCollector collects VLAN IDs in use on parent Subnets from NSX. +type VlanCollector interface { + CollectUsedVlansOnParentSubnets(parentSubnetPaths []string, excludeCRUID string) (sets.Set[int], error) +} + +// Service allocates VLAN IDs for SubnetConnectionBindingMap from the range [0, 4094]. +type Service struct { + collector VlanCollector +} + +func NewService(collector *subnetbinding.BindingService) *Service { + return &Service{collector: collector} +} + +// Allocate picks an available VLAN. When preferred is valid and unused on the target, it is returned; +// otherwise the smallest free ID from [0, 4094] is used. +func (s *Service) Allocate(parentSubnetPaths []string, excludeCRUID string, preferred int64) (int64, error) { + used, err := s.collector.CollectUsedVlansOnParentSubnets(parentSubnetPaths, excludeCRUID) + if err != nil { + return 0, err + } + + if preferred >= poolStart && preferred <= poolEnd && !used.Has(int(preferred)) { + return preferred, nil + } + + for id := poolStart; id <= poolEnd; id++ { + if !used.Has(int(id)) { + return id, nil + } + } + return 0, fmt.Errorf("no available VLAN in pool [%d, %d] for target Subnet or SubnetSet", poolStart, poolEnd) +} + +// ValidateManualVlan checks that vlan is not already used on the parent Subnet paths. +func (s *Service) ValidateManualVlan(parentSubnetPaths []string, vlan int64, excludeCRUID string) error { + used, err := s.collector.CollectUsedVlansOnParentSubnets(parentSubnetPaths, excludeCRUID) + if err != nil { + return err + } + if used.Has(int(vlan)) { + return fmt.Errorf("vlanTrafficTag %d is already used on target Subnet or SubnetSet", vlan) + } + return nil +} diff --git a/pkg/nsx/services/vlanpool/vlanpool_test.go b/pkg/nsx/services/vlanpool/vlanpool_test.go new file mode 100644 index 000000000..387ef8c1d --- /dev/null +++ b/pkg/nsx/services/vlanpool/vlanpool_test.go @@ -0,0 +1,51 @@ +package vlanpool + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/sets" +) + +type fakeVlanCollector struct { + used sets.Set[int] + err error +} + +func (f *fakeVlanCollector) CollectUsedVlansOnParentSubnets(_ []string, _ string) (sets.Set[int], error) { + if f.err != nil { + return nil, f.err + } + return f.used, nil +} + +func TestAllocate_PrefersSubnetVlan(t *testing.T) { + svc := &Service{collector: &fakeVlanCollector{used: sets.New(100)}} + vlan, err := svc.Allocate([]string{"/parent"}, "", 201) + require.NoError(t, err) + assert.Equal(t, int64(201), vlan) +} + +func TestAllocate_FallbackOnConflict(t *testing.T) { + svc := &Service{collector: &fakeVlanCollector{used: sets.New(201)}} + vlan, err := svc.Allocate([]string{"/parent"}, "", 201) + require.NoError(t, err) + assert.Equal(t, int64(0), vlan) +} + +func TestAllocate_PoolExhausted(t *testing.T) { + used := sets.New[int]() + for i := 0; i <= 4094; i++ { + used.Insert(i) + } + svc := &Service{collector: &fakeVlanCollector{used: used}} + _, err := svc.Allocate([]string{"/parent"}, "", -1) + require.Error(t, err) +} + +func TestValidateManualVlan_Conflict(t *testing.T) { + svc := &Service{collector: &fakeVlanCollector{used: sets.New(50)}} + err := svc.ValidateManualVlan([]string{"/parent"}, 50, "") + require.Error(t, err) +}