From dd55c2cd53713dde778afc717fe10d7c7f138bd5 Mon Sep 17 00:00:00 2001 From: Wenqi Qiu Date: Thu, 8 Aug 2024 16:14:36 +0800 Subject: [PATCH] Address comments Signed-off-by: Wenqi Qiu --- .../networkinfo/vpcnetworkconfig_handler.go | 2 +- pkg/nsx/services/vpc/builder.go | 8 +++--- pkg/nsx/services/vpc/builder_test.go | 28 ++++++++----------- pkg/nsx/services/vpc/compare.go | 2 +- pkg/nsx/services/vpc/vpc.go | 10 ++----- 5 files changed, 20 insertions(+), 30 deletions(-) diff --git a/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go b/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go index b84ac39f9..21bc9c402 100644 --- a/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go +++ b/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go @@ -101,7 +101,7 @@ var VPCNetworkConfigurationPredicate = predicate.Funcs{ func buildNetworkConfigInfo(vpcConfigCR v1alpha1.VPCNetworkConfiguration) (*commontypes.VPCNetworkConfigInfo, error) { org, project, err := nsxtProjectPathToId(vpcConfigCR.Spec.NSXProject) if err != nil { - log.Error(err, "failed to parse nsx-t project in network config", "Project Path", vpcConfigCR.Spec.NSXProject) + log.Error(err, "failed to parse NSX project in network config", "Project Path", vpcConfigCR.Spec.NSXProject) return nil, err } diff --git a/pkg/nsx/services/vpc/builder.go b/pkg/nsx/services/vpc/builder.go index d22fef1f9..4c4c25da8 100644 --- a/pkg/nsx/services/vpc/builder.go +++ b/pkg/nsx/services/vpc/builder.go @@ -50,7 +50,7 @@ func buildPrivateIpBlock(networkInfo *v1alpha1.NetworkInfo, nsObj *v1.Namespace, return block } -func buildNSXVPC(obj *v1alpha1.NetworkInfo, nsObj *v1.Namespace, nc common.VPCNetworkConfigInfo, cluster string, pathMap map[string]string, +func buildNSXVPC(obj *v1alpha1.NetworkInfo, nsObj *v1.Namespace, nc common.VPCNetworkConfigInfo, cluster string, nsxVPC *model.Vpc, useAVILB bool) (*model.Vpc, error) { vpc := &model.Vpc{} @@ -80,9 +80,9 @@ func buildNSXVPC(obj *v1alpha1.NetworkInfo, nsObj *v1.Namespace, nc common.VPCNe vpc.VpcConnectivityProfile = &nc.VPCConnectivityProfile } - // TODO: add PrivateIps and remove PrivateIpv4Blocks once the NSX VPC API support private_ips field. - // vpc.PrivateIps = nc.PrivateIPs - vpc.PrivateIpv4Blocks = util.GetMapValues(pathMap) + if nc.PrivateIPs != nil { + vpc.PrivateIps = nc.PrivateIPs + } if nc.ShortID != "" { vpc.ShortId = &nc.ShortID } diff --git a/pkg/nsx/services/vpc/builder_test.go b/pkg/nsx/services/vpc/builder_test.go index f5a2785ee..92d27d213 100644 --- a/pkg/nsx/services/vpc/builder_test.go +++ b/pkg/nsx/services/vpc/builder_test.go @@ -95,38 +95,35 @@ func TestBuildNSXVPC(t *testing.T) { for _, tc := range []struct { name string existingVPC *model.Vpc - pathMap map[string]string useAVILB bool expVPC *model.Vpc }{ { name: "existing VPC not change", existingVPC: &model.Vpc{ - PrivateIpv4Blocks: []string{"192.168.1.0/24"}, + PrivateIps: []string{"192.168.1.0/24"}, }, useAVILB: true, }, { - name: "existing VPC changes private IPv4 blocks", + name: "existing VPC changes private IPv4", existingVPC: &model.Vpc{ - PrivateIpv4Blocks: []string{}, + PrivateIps: []string{}, }, - pathMap: map[string]string{"vpc1": "192.168.3.0/24"}, useAVILB: false, expVPC: &model.Vpc{ - PrivateIpv4Blocks: []string{"192.168.3.0/24"}, - ShortId: common.String("short1"), + PrivateIps: []string{"192.168.1.0/24"}, + ShortId: common.String("short1"), }, }, { name: "create new VPC with AVI load balancer enabled", - pathMap: map[string]string{"vpc1": "192.168.3.0/24"}, useAVILB: true, expVPC: &model.Vpc{ Id: common.String("ns1-netinfouid1"), DisplayName: common.String("ns1-netinfouid1"), LoadBalancerVpcEndpoint: &model.LoadBalancerVPCEndpoint{Enabled: common.Bool(true)}, - PrivateIpv4Blocks: []string{"192.168.3.0/24"}, + PrivateIps: []string{"192.168.1.0/24"}, IpAddressType: common.String("IPV4"), ShortId: common.String("short1"), Tags: []model.Tag{ @@ -139,14 +136,13 @@ func TestBuildNSXVPC(t *testing.T) { }, { name: "create new VPC with AVI load balancer disabled", - pathMap: map[string]string{"vpc1": "192.168.3.0/24"}, useAVILB: false, expVPC: &model.Vpc{ - Id: common.String("ns1-netinfouid1"), - DisplayName: common.String("ns1-netinfouid1"), - PrivateIpv4Blocks: []string{"192.168.3.0/24"}, - IpAddressType: common.String("IPV4"), - ShortId: common.String("short1"), + Id: common.String("ns1-netinfouid1"), + DisplayName: common.String("ns1-netinfouid1"), + PrivateIps: []string{"192.168.1.0/24"}, + IpAddressType: common.String("IPV4"), + ShortId: common.String("short1"), Tags: []model.Tag{ {Scope: common.String("nsx-op/cluster"), Tag: common.String("cluster1")}, {Scope: common.String("nsx-op/version"), Tag: common.String("1.0.0")}, @@ -157,7 +153,7 @@ func TestBuildNSXVPC(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - got, err := buildNSXVPC(netInfoObj, nsObj, nc, clusterStr, tc.pathMap, tc.existingVPC, tc.useAVILB) + got, err := buildNSXVPC(netInfoObj, nsObj, nc, clusterStr, tc.existingVPC, tc.useAVILB) assert.Nil(t, err) assert.Equal(t, tc.expVPC, got) }) diff --git a/pkg/nsx/services/vpc/compare.go b/pkg/nsx/services/vpc/compare.go index 3a5354c6c..45686a7bd 100644 --- a/pkg/nsx/services/vpc/compare.go +++ b/pkg/nsx/services/vpc/compare.go @@ -9,7 +9,7 @@ import ( // currently we only support appending public/private cidrs // so only comparing list size is enough to identify if vcp changed func IsVPCChanged(nc common.VPCNetworkConfigInfo, vpc *model.Vpc) bool { - if len(nc.PrivateIPs) != len(vpc.PrivateIpv4Blocks) { + if len(nc.PrivateIPs) != len(vpc.PrivateIps) { return true } diff --git a/pkg/nsx/services/vpc/vpc.go b/pkg/nsx/services/vpc/vpc.go index 698c16b1a..6ccb8e6be 100644 --- a/pkg/nsx/services/vpc/vpc.go +++ b/pkg/nsx/services/vpc/vpc.go @@ -141,7 +141,7 @@ func (s *VPCService) GetVPCNetworkConfigByNamespace(ns string) *common.VPCNetwor // TBD: for now, if network config info do not contains private cidr, we consider this is // incorrect configuration, and skip creating this VPC CR func (s *VPCService) ValidateNetworkConfig(nc common.VPCNetworkConfigInfo) bool { - return nc.PrivateIPs != nil && len(nc.PrivateIPs) != 0 + return true } // InitializeVPC sync NSX resources @@ -555,12 +555,6 @@ func (s *VPCService) CreateOrUpdateVPC(obj *v1alpha1.NetworkInfo) (*model.Vpc, * log.Info("read network config from store", "NetworkConfig", ncName) - paths, err := s.CreateOrUpdatePrivateIPBlock(obj, nsObj, nc) - if err != nil { - log.Error(err, "failed to process private ip blocks, push event back to queue") - return nil, nil, err - } - // if all private ip blocks are created, then create nsx vpc resource. nsxVPC := &model.Vpc{} if updateVpc { @@ -571,7 +565,7 @@ func (s *VPCService) CreateOrUpdateVPC(obj *v1alpha1.NetworkInfo) (*model.Vpc, * nsxVPC = nil } - createdVpc, err := buildNSXVPC(obj, nsObj, nc, s.NSXConfig.Cluster, paths, nsxVPC, s.NSXConfig.NsxConfig.UseAVILoadBalancer) + createdVpc, err := buildNSXVPC(obj, nsObj, nc, s.NSXConfig.Cluster, nsxVPC, s.NSXConfig.NsxConfig.UseAVILoadBalancer) if err != nil { log.Error(err, "failed to build NSX VPC object") return nil, nil, err