From 6ff20ddeeef06173114c8f18842582ac852bf1cd Mon Sep 17 00:00:00 2001 From: Wenqi Qiu Date: Thu, 25 Jul 2024 10:27:14 +0800 Subject: [PATCH 1/2] Fix the panic caused by invalid memory address or nil pointer dereference when an unexpected NSX response is received. Signed-off-by: Wenqi Qiu --- pkg/controllers/networkinfo/networkinfo_controller.go | 2 +- pkg/nsx/services/subnet/subnet.go | 5 +++++ pkg/nsx/services/subnetport/subnetport.go | 6 ++++++ pkg/nsx/services/vpc/vpc.go | 10 ++++++++-- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/networkinfo/networkinfo_controller.go b/pkg/controllers/networkinfo/networkinfo_controller.go index 5669a2563..329c8f025 100644 --- a/pkg/controllers/networkinfo/networkinfo_controller.go +++ b/pkg/controllers/networkinfo/networkinfo_controller.go @@ -97,7 +97,7 @@ func (r *NetworkInfoReconciler) Reconcile(ctx context.Context, req ctrl.Request) snatIP, path, cidr := "", "", "" // currently, auto snat is not exposed, and use default value True // checking autosnat to support future extension in vpc configuration - if *createdVpc.ServiceGateway.AutoSnat { + if createdVpc.ServiceGateway != nil && createdVpc.ServiceGateway.AutoSnat != nil && *createdVpc.ServiceGateway.AutoSnat { snatIP, err = r.Service.GetDefaultSNATIP(*createdVpc) if err != nil { log.Error(err, "failed to read default SNAT ip from VPC", "VPC", createdVpc.Id) diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go index 0ece82fde..386a0a15f 100644 --- a/pkg/nsx/services/subnet/subnet.go +++ b/pkg/nsx/services/subnet/subnet.go @@ -233,6 +233,11 @@ func (service *SubnetService) GetSubnetStatus(subnet *model.VpcSubnet) ([]model. log.Error(err, "no subnet status found") return nil, err } + if statusList.Results[0].NetworkAddress == nil || statusList.Results[0].GatewayAddress == nil { + err := fmt.Errorf("invalid status result: %+v", statusList.Results[0]) + log.Error(err, "subnet status does not have network address or gateway address", "subnet.Id", subnet.Id) + return nil, err + } return statusList.Results, nil } diff --git a/pkg/nsx/services/subnetport/subnetport.go b/pkg/nsx/services/subnetport/subnetport.go index 5bc8cc426..b0f4f4fe4 100644 --- a/pkg/nsx/services/subnetport/subnetport.go +++ b/pkg/nsx/services/subnetport/subnetport.go @@ -6,6 +6,7 @@ package subnetport import ( "context" "errors" + "fmt" "sync" "time" @@ -248,6 +249,11 @@ func (service *SubnetPortService) GetGatewayPrefixForSubnetPort(obj *v1alpha1.Su return "", -1, err } status := statusList.Results[0] + if status.GatewayAddress == nil { + err := fmt.Errorf("invalid status result: %+v", status) + log.Error(err, "subnet status does not have gateway address", "nsxSubnetPath", nsxSubnetPath) + return "", -1, err + } gateway, err := util.RemoveIPPrefix(*status.GatewayAddress) if err != nil { return "", -1, err diff --git a/pkg/nsx/services/vpc/vpc.go b/pkg/nsx/services/vpc/vpc.go index 577f7a612..24aedc244 100644 --- a/pkg/nsx/services/vpc/vpc.go +++ b/pkg/nsx/services/vpc/vpc.go @@ -179,13 +179,13 @@ func InitializeVPC(service common.Service) (*VPCService, error) { VPCService.VPCNSNetworkConfigStore = VPCNsNetworkConfigStore{ VPCNSNetworkConfigMap: make(map[string]string), } - //initialize vpc store, lbs store and ip blocks store + // initialize vpc store, lbs store and ip blocks store go VPCService.InitializeResourceStore(&wg, fatalErrors, common.ResourceTypeVpc, nil, VPCService.VpcStore) wg.Add(1) go VPCService.InitializeResourceStore(&wg, fatalErrors, common.ResourceTypeLBService, nil, VPCService.LbsStore) go VPCService.InitializeResourceStore(&wg, fatalErrors, common.ResourceTypeIPBlock, nil, VPCService.IpblockStore) - //initalize avi rule related store + // initialize avi rule related store if enableAviAllowRule { VPCService.RuleStore = &AviRuleStore{ResourceStore: common.ResourceStore{ Indexer: cache.NewIndexer(keyFuncAVI, nil), @@ -499,6 +499,12 @@ func (s *VPCService) GetAVISubnetInfo(vpc model.Vpc) (string, string, error) { return "", "", err } + if statusList.Results[0].NetworkAddress == nil { + err := fmt.Errorf("invalid status result: %+v", statusList.Results[0]) + log.Error(err, "subnet status does not have network address", "Subnet", common.AVISubnetLBID) + return "", "", err + } + cidr := *statusList.Results[0].NetworkAddress log.Info("read AVI subnet properties", "Path", path, "CIDR", cidr) return path, cidr, nil From 6f3e7eed5fbf749c76ef670659d30a961d72365b Mon Sep 17 00:00:00 2001 From: Wenqi Qiu Date: Wed, 31 Jul 2024 01:34:32 +0800 Subject: [PATCH 2/2] fix e2e Signed-off-by: Wenqi Qiu --- .../networkinfo/vpcnetworkconfig_handler.go | 2 +- .../networkinfo/vpcnetworkconfig_handler_test.go | 6 +++--- .../testVPC/customize_networkconfig.yaml | 2 +- .../testVPC/customize_networkconfig_updated.yaml | 2 +- .../manifest/testVPC/default_networkconfig.yaml | 2 +- test/e2e/manifest/testVPC/shared_ns.yaml | 6 +++--- .../manifest/testVPC/system_networkconfig.yaml | 2 +- test/e2e/manifest/testVPC/update_ns.yaml | 2 +- test/e2e/nsx_networkinfo_test.go | 16 ++++++++-------- 9 files changed, 20 insertions(+), 20 deletions(-) diff --git a/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go b/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go index e580dbf2a..0cf3066a1 100644 --- a/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go +++ b/pkg/controllers/networkinfo/vpcnetworkconfig_handler.go @@ -151,7 +151,7 @@ func isDefaultNetworkConfigCR(vpcConfigCR v1alpha1.VPCNetworkConfiguration) bool } // parse org id and project id from nsxtProject path -// example /orgs/default/projects/nsx_operator_e2e_test +// example /orgs/default/projects/nsx_operator_test func nsxtProjectPathToId(path string) (string, string, error) { parts := strings.Split(path, "/") if len(parts) < 4 { diff --git a/pkg/controllers/networkinfo/vpcnetworkconfig_handler_test.go b/pkg/controllers/networkinfo/vpcnetworkconfig_handler_test.go index 85a464933..a0e5a8623 100644 --- a/pkg/controllers/networkinfo/vpcnetworkconfig_handler_test.go +++ b/pkg/controllers/networkinfo/vpcnetworkconfig_handler_test.go @@ -18,7 +18,7 @@ func TestNsxtProjectPathToId(t *testing.T) { project string err interface{} }{ - {"1", "/orgs/default/projects/nsx_operator_e2e_test", "default", "nsx_operator_e2e_test", nil}, + {"1", "/orgs/default/projects/nsx_operator_test", "default", "nsx_operator_test", nil}, {"2", "", "", "", "dummy"}, } for _, tt := range tests { @@ -79,7 +79,7 @@ func TestBuildNetworkConfigInfo(t *testing.T) { PrivateIPv4CIDRs: []string{"private-ipb-1", "private-ipb-2"}, DefaultIPv4SubnetSize: 64, DefaultSubnetAccessMode: "Public", - NSXTProject: "/orgs/default/projects/nsx_operator_e2e_test", + NSXTProject: "/orgs/default/projects/nsx_operator_test", } spec2 := v1alpha1.VPCNetworkConfigurationSpec{ DefaultGatewayPath: "test-gw-path-2", @@ -120,7 +120,7 @@ func TestBuildNetworkConfigInfo(t *testing.T) { accessMode string isDefault bool }{ - {"1", testCRD1, "test-gw-path-1", "test-edge-path-1", "default", "nsx_operator_e2e_test", 64, "Public", false}, + {"1", testCRD1, "test-gw-path-1", "test-edge-path-1", "default", "nsx_operator_test", 64, "Public", false}, {"2", testCRD2, "test-gw-path-2", "test-edge-path-2", "anotherOrg", "anotherProject", 32, "Private", false}, {"3", testCRD3, "test-gw-path-2", "test-edge-path-2", "anotherOrg", "anotherProject", 32, "Private", true}, } diff --git a/test/e2e/manifest/testVPC/customize_networkconfig.yaml b/test/e2e/manifest/testVPC/customize_networkconfig.yaml index 913c8e46d..9e6fbdf9c 100644 --- a/test/e2e/manifest/testVPC/customize_networkconfig.yaml +++ b/test/e2e/manifest/testVPC/customize_networkconfig.yaml @@ -9,7 +9,7 @@ spec: # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id} defaultIPv4SubnetSize: 26 - nsxtProject: /orgs/default/projects/nsx_operator_e2e_test + nsxtProject: /orgs/default/projects/nsx_operator_e2e_test_2 externalIPv4Blocks: - /infra/ip-blocks/e2e_test_external_ip_blk privateIPv4CIDRs: diff --git a/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml b/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml index a718cd518..0290155ff 100644 --- a/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml +++ b/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml @@ -9,7 +9,7 @@ spec: # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id} defaultIPv4SubnetSize: 26 - nsxtProject: /orgs/default/projects/nsx_operator_e2e_test + nsxtProject: /orgs/default/projects/nsx_operator_e2e_test_2 externalIPv4Blocks: - /infra/ip-blocks/e2e_test_external_ip_blk privateIPv4CIDRs: diff --git a/test/e2e/manifest/testVPC/default_networkconfig.yaml b/test/e2e/manifest/testVPC/default_networkconfig.yaml index dbfb2edb3..789921407 100644 --- a/test/e2e/manifest/testVPC/default_networkconfig.yaml +++ b/test/e2e/manifest/testVPC/default_networkconfig.yaml @@ -13,7 +13,7 @@ spec: # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id} defaultIPv4SubnetSize: 26 - nsxtProject: /orgs/default/projects/nsx_operator_e2e_test + nsxtProject: /orgs/default/projects/nsx_operator_e2e_test_2 externalIPv4Blocks: - /infra/ip-blocks/e2e_test_external_ip_blk privateIPv4CIDRs: diff --git a/test/e2e/manifest/testVPC/shared_ns.yaml b/test/e2e/manifest/testVPC/shared_ns.yaml index c653e0acd..f23997837 100644 --- a/test/e2e/manifest/testVPC/shared_ns.yaml +++ b/test/e2e/manifest/testVPC/shared_ns.yaml @@ -5,7 +5,7 @@ kind: Namespace metadata: annotations: nsx.vmware.com/vpc_network_config: selfdefinedconfig - name: shared-vpc-ns-0 + name: shared-vpc-ns-9qsg6 --- apiVersion: v1 @@ -13,5 +13,5 @@ kind: Namespace metadata: annotations: nsx.vmware.com/vpc_network_config: selfdefinedconfig - nsx.vmware.com/shared_vpc_namespace: shared-vpc-ns-0 - name: shared-vpc-ns-1 + nsx.vmware.com/shared_vpc_namespace: shared-vpc-ns-9qsg6 + name: shared-vpc-ns-8l99n diff --git a/test/e2e/manifest/testVPC/system_networkconfig.yaml b/test/e2e/manifest/testVPC/system_networkconfig.yaml index 843f7b5d0..e3fec2872 100644 --- a/test/e2e/manifest/testVPC/system_networkconfig.yaml +++ b/test/e2e/manifest/testVPC/system_networkconfig.yaml @@ -10,7 +10,7 @@ spec: # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id} defaultIPv4SubnetSize: 26 - nsxtProject: /orgs/default/projects/nsx_operator_e2e_test + nsxtProject: /orgs/default/projects/nsx_operator_e2e_test_2 externalIPv4Blocks: - /infra/ip-blocks/e2e_test_external_ip_blk privateIPv4CIDRs: diff --git a/test/e2e/manifest/testVPC/update_ns.yaml b/test/e2e/manifest/testVPC/update_ns.yaml index fe2bccd36..61802e5eb 100644 --- a/test/e2e/manifest/testVPC/update_ns.yaml +++ b/test/e2e/manifest/testVPC/update_ns.yaml @@ -5,4 +5,4 @@ kind: Namespace metadata: annotations: nsx.vmware.com/vpc_network_config: selfdefinedconfig - name: update-ns \ No newline at end of file + name: update-ns-lffpb \ No newline at end of file diff --git a/test/e2e/nsx_networkinfo_test.go b/test/e2e/nsx_networkinfo_test.go index 15c911fdb..4828e7572 100644 --- a/test/e2e/nsx_networkinfo_test.go +++ b/test/e2e/nsx_networkinfo_test.go @@ -78,7 +78,7 @@ func TestCustomizedNetworkInfo(t *testing.T) { err := testData.waitForResourceExistByPath(vpcPath, true) assertNil(t, err) - //verify private ipblocks created for vpc + // verify private ipblocks created for vpc p_ipb_id1 := ns_uid + "_" + CustomizedPrivateCIDR1 p_ipb_id2 := ns_uid + "_" + CustomizedPrivateCIDR2 @@ -97,7 +97,7 @@ func TestInfraNetworkInfo(t *testing.T) { err := testData.waitForResourceExistByPath(vpcPath, true) assertNil(t, err) - //verify private ipblocks created for vpc + // verify private ipblocks created for vpc p_ipb_id1 := ns_uid + "_" + InfraPrivateCIDR1 p_ipb_id2 := ns_uid + "_" + InfraPrivateCIDR2 @@ -128,7 +128,7 @@ func TestDefaultNetworkInfo(t *testing.T) { err := testData.waitForResourceExistByPath(vpcPath, true) assertNil(t, err) - //verify private ipblocks created for vpc, id is nsuid + cidr + // verify private ipblocks created for vpc, id is nsuid + cidr p_ipb_id1 := ns_uid + "_" + DefaultPrivateCIDR1 p_ipb_id2 := ns_uid + "_" + DefaultPrivateCIDR2 @@ -148,8 +148,8 @@ func TestDefaultNetworkInfo(t *testing.T) { // ns1 share vpc with ns, delete ns1, vpc should not be deleted func TestSharedNetworkInfo(t *testing.T) { - ns := "shared-vpc-ns-0" - ns1 := "shared-vpc-ns-1" + ns := "shared-vpc-ns-9qsg6" + ns1 := "shared-vpc-ns-8l99n" nsPath, _ := filepath.Abs("./manifest/testVPC/shared_ns.yaml") _ = applyYAML(nsPath, "") @@ -171,7 +171,7 @@ func TestSharedNetworkInfo(t *testing.T) { assertTrue(t, vpcPath == vpcPath1, "vpcPath %s should be the same as vpcPath2 %s", vpcPath, vpcPath1) - //verify private ipblocks created for vpc, id is nsuid + cidr + // verify private ipblocks created for vpc, id is nsuid + cidr p_ipb_id1 := ns_uid + "_" + CustomizedPrivateCIDR1 p_ipb_id2 := ns_uid + "_" + CustomizedPrivateCIDR2 @@ -191,7 +191,7 @@ func TestSharedNetworkInfo(t *testing.T) { // update vpcnetworkconfig, and check vpc is updated func TestUpdateVPCNetworkconfigNetworkInfo(t *testing.T) { - ns := "update-ns" + ns := "update-ns-lffpb" nsPath, _ := filepath.Abs("./manifest/testVPC/update_ns.yaml") _ = applyYAML(nsPath, "") @@ -210,7 +210,7 @@ func TestUpdateVPCNetworkconfigNetworkInfo(t *testing.T) { assertTrue(t, strings.Contains(privateIPv4CIDRs, CustomizedPrivateCIDR2), "privateIPv4CIDRs %s should contain %s", privateIPv4CIDRs, CustomizedPrivateCIDR1) assertNil(t, err) - //verify private ipblocks created for vpc, id is nsuid + cidr + // verify private ipblocks created for vpc, id is nsuid + cidr p_ipb_id1 := ns_uid + "_" + CustomizedPrivateCIDR1 p_ipb_id2 := ns_uid + "_" + CustomizedPrivateCIDR2