diff --git a/pkg/nsx/services/subnet/builder.go b/pkg/nsx/services/subnet/builder.go index 2fab50585..39d995976 100644 --- a/pkg/nsx/services/subnet/builder.go +++ b/pkg/nsx/services/subnet/builder.go @@ -53,7 +53,7 @@ func convertAccessMode(accessMode string) string { return accessMode } -func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag, useLegacyAPI bool) (*model.VpcSubnet, error) { +func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag) (*model.VpcSubnet, error) { tags = append(service.buildBasicTags(obj), tags...) var nsxSubnet *model.VpcSubnet var staticIpAllocation bool @@ -70,11 +70,7 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag, u if dhcpMode == "" { dhcpMode = v1alpha1.DHCPConfigModeDeactivated } - if useLegacyAPI { - nsxSubnet.DhcpConfig = service.buildDHCPConfig(dhcpMode != v1alpha1.DHCPConfigModeDeactivated) - } else { - nsxSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode) - } + nsxSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode) nsxSubnet.IpAddresses = o.Spec.IPAddresses case *v1alpha1.SubnetSet: // The index is a random string with the length of 8 chars. It is the first 8 chars of the hash @@ -91,11 +87,7 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag, u if dhcpMode == "" { dhcpMode = v1alpha1.DHCPConfigModeDeactivated } - if useLegacyAPI { - nsxSubnet.DhcpConfig = service.buildDHCPConfig(dhcpMode != v1alpha1.DHCPConfigModeDeactivated) - } else { - nsxSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode) - } + nsxSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode) default: return nil, SubnetTypeError } @@ -113,15 +105,6 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag, u return nsxSubnet, nil } -func (service *SubnetService) buildDHCPConfig(enableDHCP bool) *model.VpcSubnetDhcpConfig { - // Subnet DHCP is used by AVI, not needed for now. We need to explicitly mark enableDhcp = false, - // otherwise Subnet will use DhcpConfig inherited from VPC. - dhcpConfig := &model.VpcSubnetDhcpConfig{ - EnableDhcp: Bool(enableDHCP), - } - return dhcpConfig -} - func (service *SubnetService) buildSubnetDHCPConfig(mode string) *model.SubnetDhcpConfig { nsxMode := nsxutil.ParseDHCPMode(mode) subnetDhcpConfig := &model.SubnetDhcpConfig{ diff --git a/pkg/nsx/services/subnet/builder_test.go b/pkg/nsx/services/subnet/builder_test.go index 56ed13ec9..45681e726 100644 --- a/pkg/nsx/services/subnet/builder_test.go +++ b/pkg/nsx/services/subnet/builder_test.go @@ -102,15 +102,9 @@ func TestBuildSubnetForSubnetSet(t *testing.T) { Namespace: "ns-1", }, } - subnet, err := service.buildSubnet(subnetSet, tags, true) - assert.Nil(t, err) - assert.Equal(t, false, *subnet.DhcpConfig.EnableDhcp) - assert.Nil(t, subnet.SubnetDhcpConfig) - assert.Equal(t, true, *subnet.AdvancedConfig.StaticIpAllocation.Enabled) - subnet, err = service.buildSubnet(subnetSet, tags, false) + subnet, err := service.buildSubnet(subnetSet, tags) assert.Nil(t, err) assert.Equal(t, "DHCP_DEACTIVATED", *subnet.SubnetDhcpConfig.Mode) - assert.Nil(t, subnet.DhcpConfig) assert.Equal(t, true, *subnet.AdvancedConfig.StaticIpAllocation.Enabled) } diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go index b14508d89..835d8a7bf 100644 --- a/pkg/nsx/services/subnet/subnet.go +++ b/pkg/nsx/services/subnet/subnet.go @@ -36,8 +36,7 @@ var ( type SubnetService struct { common.Service - SubnetStore *SubnetStore - useLegacyAPI bool + SubnetStore *SubnetStore } // SubnetParameters stores parameters to CRUD Subnet object @@ -84,21 +83,8 @@ func InitializeSubnetService(service common.Service) (*SubnetService, error) { } func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo common.VPCResourceInfo, tags []model.Tag) (subnet *model.VpcSubnet, err error) { - if subnet, err = service.createOrUpdateSubnetWithAPI(obj, vpcInfo, tags, service.useLegacyAPI); err != nil { - if nsxErr, ok := err.(*nsxutil.NSXApiError); ok { - if *nsxErr.ErrorCode == ErrorCodeUnrecognizedField { - log.Info("NSX does not support subnet_dhcp_config, using old API", "error", err) - service.useLegacyAPI = true - subnet, err = service.createOrUpdateSubnetWithAPI(obj, vpcInfo, tags, service.useLegacyAPI) - } - } - } - return subnet, err -} - -func (service *SubnetService) createOrUpdateSubnetWithAPI(obj client.Object, vpcInfo common.VPCResourceInfo, tags []model.Tag, useLegacyAPI bool) (*model.VpcSubnet, error) { uid := string(obj.GetUID()) - nsxSubnet, err := service.buildSubnet(obj, tags, useLegacyAPI) + nsxSubnet, err := service.buildSubnet(obj, tags) if err != nil { log.Error(err, "Failed to build Subnet") return nil, err diff --git a/pkg/nsx/services/subnetport/builder.go b/pkg/nsx/services/subnetport/builder.go index 9eedf124b..b2ef612b6 100644 --- a/pkg/nsx/services/subnetport/builder.go +++ b/pkg/nsx/services/subnetport/builder.go @@ -40,19 +40,13 @@ func (service *SubnetPortService) buildSubnetPort(obj interface{}, nsxSubnet *mo case *v1alpha1.SubnetPort: externalAddressBinding = service.buildExternalAddressBinding(o) } - if nsxSubnet.SubnetDhcpConfig == nil { - if nsxSubnet.DhcpConfig != nil && nsxSubnet.DhcpConfig.EnableDhcp != nil && *nsxSubnet.DhcpConfig.EnableDhcp { - allocateAddresses = "DHCP" - } else { - allocateAddresses = "BOTH" - } + + if nsxSubnet.SubnetDhcpConfig != nil && nsxSubnet.SubnetDhcpConfig.Mode != nil && *nsxSubnet.SubnetDhcpConfig.Mode != nsxutil.ParseDHCPMode(v1alpha1.DHCPConfigModeDeactivated) { + allocateAddresses = "DHCP" } else { - if nsxSubnet.SubnetDhcpConfig.Mode != nil && *nsxSubnet.SubnetDhcpConfig.Mode != nsxutil.ParseDHCPMode(v1alpha1.DHCPConfigModeDeactivated) { - allocateAddresses = "DHCP" - } else { - allocateAddresses = "BOTH" - } + allocateAddresses = "BOTH" } + nsxSubnetPortName := service.BuildSubnetPortName(objMeta) nsxSubnetPortID := service.BuildSubnetPortId(objMeta) // use the subnetPort CR UID as the attachment uid generation to ensure the latter stable diff --git a/pkg/nsx/services/subnetport/subnetport.go b/pkg/nsx/services/subnetport/subnetport.go index 8f341408a..8db3d4c66 100644 --- a/pkg/nsx/services/subnetport/subnetport.go +++ b/pkg/nsx/services/subnetport/subnetport.go @@ -129,9 +129,6 @@ func (service *SubnetPortService) CreateOrUpdateSubnetPort(obj interface{}, nsxS } } enableDHCP := false - if (*nsxSubnet).DhcpConfig != nil && *nsxSubnet.DhcpConfig.EnableDhcp { - enableDHCP = true - } if nsxSubnet.SubnetDhcpConfig != nil && nsxSubnet.SubnetDhcpConfig.Mode != nil && *nsxSubnet.SubnetDhcpConfig.Mode != v1alpha1.DHCPConfigModeDeactivated { enableDHCP = true } diff --git a/test/e2e/nsx_subnet_test.go b/test/e2e/nsx_subnet_test.go index ed7369758..c47c6f956 100644 --- a/test/e2e/nsx_subnet_test.go +++ b/test/e2e/nsx_subnet_test.go @@ -92,7 +92,7 @@ func fetchSubnetBySubnetSet(t *testing.T, subnetSet *v1alpha1.SubnetSet) model.V results, err := testData.queryResource(common.ResourceTypeSubnet, tags) require.NoError(t, err) subnets := transSearchResponsetoSubnet(results) - require.True(t, len(subnets) > 0, "No NSX subnet found") + require.True(t, len(subnets) > 0, "No NSX Subnet found") return subnets[0] } @@ -110,15 +110,15 @@ func defaultSubnetSet(t *testing.T) { assureSubnetPort(t, subnetTestNamespace, "port-e2e-test-1") defer deleteYAML(portPath, subnetTestNamespace) - // 3. Check SubnetSet CR status should be updated with NSX subnet info. + // 3. Check SubnetSet CR status should be updated with NSX Subnet info. subnetSet, err := testData.crdClientset.CrdV1alpha1().SubnetSets(subnetTestNamespace).Get(context.TODO(), common.DefaultPodSubnetSet, v1.GetOptions{}) require.NoError(t, err) require.NotEmpty(t, subnetSet.Status.Subnets, "No Subnet info in SubnetSet") - // 4. Check NSX subnet allocation. + // 4. Check NSX Subnet allocation. networkAddress := subnetSet.Status.Subnets[0].NetworkAddresses assert.True(t, len(networkAddress) > 0, "No network address in SubnetSet") - // 5. Check adding NSX subnet tags. + // 5. Check adding NSX Subnet tags. ns, err := testData.clientset.CoreV1().Namespaces().Get(context.TODO(), subnetTestNamespace, v1.GetOptions{}) require.NoError(t, err) labelKey, labelValue := "subnet-e2e", "add" @@ -135,9 +135,9 @@ func defaultSubnetSet(t *testing.T) { break } } - assert.True(t, found, "Failed to add tags for NSX subnet %s", *(vpcSubnet.Id)) + assert.True(t, found, "Failed to add tags for NSX Subnet %s", *(vpcSubnet.Id)) - // 6. Check updating NSX subnet tags. + // 6. Check updating NSX Subnet tags. ns, err = testData.clientset.CoreV1().Namespaces().Get(context.TODO(), subnetTestNamespace, v1.GetOptions{}) require.NoError(t, err) labelValue = "update" @@ -153,16 +153,16 @@ func defaultSubnetSet(t *testing.T) { break } } - assert.True(t, found, "Failed to update tags for NSX subnet %s", *(vpcSubnet.Id)) + assert.True(t, found, "Failed to update tags for NSX Subnet %s", *(vpcSubnet.Id)) - // 7. Check deleting NSX subnet tags. + // 7. Check deleting NSX Subnet tags. ns, err = testData.clientset.CoreV1().Namespaces().Get(context.TODO(), subnetTestNamespace, v1.GetOptions{}) require.NoError(t, err) delete(ns.Labels, labelKey) newNs, err := testData.clientset.CoreV1().Namespaces().Update(context.TODO(), ns, v1.UpdateOptions{}) time.Sleep(5 * time.Second) require.NoError(t, err) - log.V(2).Info("New Namespace", "namespace", newNs) + log.V(2).Info("New Namespace", "Namespace", newNs) vpcSubnet = fetchSubnetBySubnetSet(t, subnetSet) found = false for _, tag := range vpcSubnet.Tags { @@ -171,7 +171,7 @@ func defaultSubnetSet(t *testing.T) { break } } - assert.False(t, found, "Failed to delete tags for NSX subnet %s", *(vpcSubnet.Id)) + assert.False(t, found, "Failed to delete tags for NSX Subnet %s", *(vpcSubnet.Id)) } func UserSubnetSet(t *testing.T) { @@ -210,7 +210,7 @@ func UserSubnetSet(t *testing.T) { require.NoError(t, applyYAML(portPath, subnetTestNamespace)) assureSubnetPort(t, subnetTestNamespace, portName) - // 3. Check SubnetSet CR status should be updated with NSX subnet info. + // 3. Check SubnetSet CR status should be updated with NSX Subnet info. subnetSet, err := testData.crdClientset.CrdV1alpha1().SubnetSets(subnetTestNamespace).Get(context.TODO(), subnetSetName, v1.GetOptions{}) require.NoError(t, err) require.NotEmpty(t, subnetSet.Status.Subnets, "No Subnet info in SubnetSet") @@ -239,7 +239,7 @@ func UserSubnetSet(t *testing.T) { }) require.NoError(t, err) - // 5. Check NSX subnet allocation. + // 5. Check NSX Subnet allocation. networkAddress := subnetSet.Status.Subnets[0].NetworkAddresses assert.True(t, len(networkAddress) > 0, "No network address in SubnetSet") deleteYAML(portPath, subnetTestNamespace) @@ -262,7 +262,7 @@ func sharedSubnetSet(t *testing.T) { assureSubnetPort(t, subnetTestNamespaceShared, "port-e2e-test-3") defer deleteYAML(portPath, subnetTestNamespaceShared) - // 3. Check SubnetSet CR status should be updated with NSX subnet info. + // 3. Check SubnetSet CR status should be updated with NSX Subnet info. subnetSet, err := testData.crdClientset.CrdV1alpha1().SubnetSets(subnetTestNamespaceTarget).Get(context.TODO(), common.DefaultVMSubnetSet, v1.GetOptions{}) require.NoError(t, err) require.NotEmpty(t, subnetSet.Status.Subnets, "No Subnet info in SubnetSet") @@ -292,12 +292,13 @@ func SubnetCIDR(t *testing.T) { }, }, } + // Create a Subnet with DHCPServer mode _, err := testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Create(context.TODO(), subnet, v1.CreateOptions{}) if err != nil && errors.IsAlreadyExists(err) { err = nil } require.NoError(t, err) - assureSubnet(t, subnetTestNamespace, subnet.Name) + assureSubnet(t, subnetTestNamespace, subnet.Name, "") allocatedSubnet, err := testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Get(context.TODO(), subnet.Name, v1.GetOptions{}) require.NoError(t, err) subnetCRUID := string(allocatedSubnet.UID) @@ -305,6 +306,7 @@ func SubnetCIDR(t *testing.T) { require.Equal(t, 1, len(nsxSubnets)) targetCIDR := allocatedSubnet.Status.NetworkAddresses[0] + // Delete the Subnet err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Delete(context.TODO(), subnet.Name, v1.DeleteOptions{}) require.NoError(t, err) @@ -322,6 +324,7 @@ func SubnetCIDR(t *testing.T) { }) require.NoError(t, err) + // Create another Subnet with the same IPAddresses subnet.Spec.IPAddresses = []string{targetCIDR} _, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Create(context.TODO(), subnet, v1.CreateOptions{}) if err != nil && errors.IsAlreadyExists(err) { @@ -329,7 +332,7 @@ func SubnetCIDR(t *testing.T) { err = nil } require.NoError(t, err) - assureSubnet(t, subnetTestNamespace, subnet.Name) + assureSubnet(t, subnetTestNamespace, subnet.Name, "") allocatedSubnet, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Get(context.TODO(), subnet.Name, v1.GetOptions{}) require.NoError(t, err) require.Equal(t, targetCIDR, allocatedSubnet.Status.NetworkAddresses[0]) @@ -338,6 +341,22 @@ func SubnetCIDR(t *testing.T) { nsxSubnets = testData.fetchSubnetBySubnetUID(t, newSubnetCRUID) require.Equal(t, 1, len(nsxSubnets)) + // Change the DHCP mode from DHCPServer to DHCPDeactived + allocatedSubnet.Spec.SubnetDHCPConfig.Mode = v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated) + _, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Update(context.TODO(), allocatedSubnet, v1.UpdateOptions{}) + require.NoError(t, err) + allocatedSubnet = assureSubnet(t, subnetTestNamespace, subnet.Name, v1alpha1.DHCPConfigModeDeactivated) + nsxSubnets = testData.fetchSubnetBySubnetUID(t, newSubnetCRUID) + require.Equal(t, 1, len(nsxSubnets)) + require.Equal(t, "DHCP_DEACTIVATED", *nsxSubnets[0].SubnetDhcpConfig.Mode) + require.Equal(t, true, *nsxSubnets[0].AdvancedConfig.StaticIpAllocation.Enabled) + + // Change the DHCP mode from DHCPDeactived to DHCPServer + allocatedSubnet.Spec.SubnetDHCPConfig.Mode = v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer) + _, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Update(context.TODO(), allocatedSubnet, v1.UpdateOptions{}) + require.Contains(t, err.Error(), "subnetDHCPConfig cannot switch from DHCPDeactivated to other modes") + + // Delete the Subnet err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Delete(context.TODO(), subnet.Name, v1.DeleteOptions{}) require.NoError(t, err) @@ -364,7 +383,7 @@ func (data *TestData) fetchSubnetBySubnetUID(t *testing.T, subnetUID string) (re return } -func assureSubnet(t *testing.T, ns, subnetName string) (res *v1alpha1.Subnet) { +func assureSubnet(t *testing.T, ns, subnetName string, conditionMsg string) (res *v1alpha1.Subnet) { deadlineCtx, deadlineCancel := context.WithTimeout(context.Background(), 2*defaultTimeout) defer deadlineCancel() err := wait.PollUntilContextTimeout(deadlineCtx, 1*time.Second, 2*defaultTimeout, false, func(ctx context.Context) (done bool, err error) { @@ -378,7 +397,7 @@ func assureSubnet(t *testing.T, ns, subnetName string) (res *v1alpha1.Subnet) { } log.V(2).Info("Subnet status", "status", res.Status) for _, con := range res.Status.Conditions { - if con.Type == v1alpha1.Ready && con.Status == corev1.ConditionTrue { + if con.Type == v1alpha1.Ready && con.Status == corev1.ConditionTrue && strings.Contains(con.Message, conditionMsg) { return true, nil } }