From b164a234b093364786c553fb18d59c58523dfbef Mon Sep 17 00:00:00 2001 From: Yanjun Zhou Date: Sun, 26 Jan 2025 19:28:13 +0800 Subject: [PATCH] Explicitly check 5 GPRRs for vpc RealizedState Signed-off-by: Yanjun Zhou --- pkg/nsx/services/common/types.go | 2 - .../services/realizestate/realize_state.go | 14 +-- .../realizestate/realize_state_test.go | 16 +-- pkg/nsx/services/subnet/subnet.go | 2 +- pkg/nsx/services/subnet/subnet_test.go | 4 +- pkg/nsx/services/subnetport/subnetport.go | 2 +- pkg/nsx/services/vpc/vpc.go | 31 +++++- pkg/nsx/services/vpc/vpc_test.go | 98 +++++++++++++++++++ pkg/nsx/util/utils.go | 5 + test/e2e/precreated_vpc_test.go | 7 +- 10 files changed, 154 insertions(+), 27 deletions(-) diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go index f728b2966..6f075260b 100644 --- a/pkg/nsx/services/common/types.go +++ b/pkg/nsx/services/common/types.go @@ -120,8 +120,6 @@ const ( DstGroupSuffix = "dst" IpSetGroupSuffix = "ipset" ShareSuffix = "share" - - GatewayInterfaceId = "gateway-interface" ) var ( diff --git a/pkg/nsx/services/realizestate/realize_state.go b/pkg/nsx/services/realizestate/realize_state.go index b7f3c3b16..e9261faaf 100644 --- a/pkg/nsx/services/realizestate/realize_state.go +++ b/pkg/nsx/services/realizestate/realize_state.go @@ -32,7 +32,7 @@ func InitializeRealizeState(service common.Service) *RealizeStateService { // CheckRealizeState allows the caller to check realize status of intentPath with retries. // Backoff defines the maximum retries and the wait interval between two retries. // Check all the entities, all entities should be in the REALIZED state to be treated as REALIZED -func (service *RealizeStateService) CheckRealizeState(backoff wait.Backoff, intentPath string, extraIds []string) error { +func (service *RealizeStateService) CheckRealizeState(backoff wait.Backoff, intentPath string, GPRRTypeList []nsxutil.GPRRType) error { // TODO， ask NSX if there were multiple realize states could we check only the latest one? return retry.OnError(backoff, func(err error) bool { // Won't retry when realized state is `ERROR`. @@ -44,12 +44,12 @@ func (service *RealizeStateService) CheckRealizeState(backoff wait.Backoff, inte return err } entitiesRealized := 0 - extraIdsRealized := 0 + gprrRealized := 0 for _, result := range results.Results { if *result.State == model.GenericPolicyRealizedResource_STATE_REALIZED { - for _, id := range extraIds { - if *result.Id == id { - extraIdsRealized++ + for _, gprr := range GPRRTypeList { + if *result.Id == gprr.Id && *result.EntityType == gprr.EntityType { + gprrRealized++ } } entitiesRealized++ @@ -69,8 +69,8 @@ func (service *RealizeStateService) CheckRealizeState(backoff wait.Backoff, inte return nsxutil.NewRealizeStateError(fmt.Sprintf("%s realized with errors: %s", intentPath, errMsg)) } } - // extraIdsRealized can be greater than extraIds length as id is not unique in result list. - if len(results.Results) != 0 && entitiesRealized == len(results.Results) && extraIdsRealized >= len(extraIds) { + // gprrRealized can be greater than GPRRTypeList length as (id, entity_type) is not unique in result list. + if len(results.Results) != 0 && entitiesRealized == len(results.Results) && gprrRealized >= len(GPRRTypeList) { return nil } return fmt.Errorf("%s not realized", intentPath) diff --git a/pkg/nsx/services/realizestate/realize_state_test.go b/pkg/nsx/services/realizestate/realize_state_test.go index 36be10cf7..2e3f17768 100644 --- a/pkg/nsx/services/realizestate/realize_state_test.go +++ b/pkg/nsx/services/realizestate/realize_state_test.go @@ -64,14 +64,14 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { Steps: 6, } // default project - err := s.CheckRealizeState(backoff, "/orgs/default/projects/default/vpcs/vpc/subnets/subnet/ports/port", []string{}) + err := s.CheckRealizeState(backoff, "/orgs/default/projects/default/vpcs/vpc/subnets/subnet/ports/port", []nsxutil.GPRRType{}) realizeStateError, ok := err.(*nsxutil.RealizeStateError) assert.True(t, ok) assert.Equal(t, realizeStateError.Error(), "/orgs/default/projects/default/vpcs/vpc/subnets/subnet/ports/port realized with errors: [mocked error]") // non default project - err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/ports/port", []string{}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/ports/port", []nsxutil.GPRRType{}) realizeStateError, ok = err.(*nsxutil.RealizeStateError) assert.True(t, ok) @@ -86,7 +86,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), Alarms: []model.PolicyAlarmResource{}, EntityType: common.String("RealizedLogicalRouterPort"), - Id: common.String(common.GatewayInterfaceId), + Id: common.String("gateway-interface"), }, { State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), @@ -97,7 +97,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { }, }, nil }) - err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc", []string{common.GatewayInterfaceId}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc", []nsxutil.GPRRType{{Id: "gateway-interface", EntityType: "RealizedLogicalRouterPort"}}) assert.Equal(t, err, nil) // for lbs, realized with ProviderNotReady and need retry @@ -128,7 +128,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { Jitter: 0, Steps: 1, } - err = s.CheckRealizeState(backoff, "/orgs/default/projects/default/vpcs/vpc/vpc-lbs/default", []string{}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/default/vpcs/vpc/vpc-lbs/default", []nsxutil.GPRRType{}) assert.NotEqual(t, err, nil) _, ok = err.(*nsxutil.RetryRealizeError) assert.Equal(t, ok, true) @@ -161,7 +161,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { }, }, nil }) - err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []string{}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []nsxutil.GPRRType{}) realizeStateError, ok = err.(*nsxutil.RealizeStateError) assert.True(t, ok) @@ -191,7 +191,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { }, }, nil }) - err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []string{}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []nsxutil.GPRRType{}) assert.Equal(t, err, nil) // for subnet, need retry @@ -224,7 +224,7 @@ func TestRealizeStateService_CheckRealizeState(t *testing.T) { Jitter: 0, Steps: 1, } - err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []string{}) + err = s.CheckRealizeState(backoff, "/orgs/default/projects/project-quality/vpcs/vpc/subnets/subnet/", []nsxutil.GPRRType{}) assert.NotEqual(t, err, nil) _, ok = err.(*nsxutil.RealizeStateError) assert.Equal(t, ok, false) diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go index d54076277..7bdc90bd3 100644 --- a/pkg/nsx/services/subnet/subnet.go +++ b/pkg/nsx/services/subnet/subnet.go @@ -135,7 +135,7 @@ func (service *SubnetService) createOrUpdateSubnet(obj client.Object, nsxSubnet // For Subnets, it's important to reuse the already created NSXSubnet. // For SubnetSets, since the ID includes a random value, the created NSX Subnet needs to be deleted and recreated. - if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *nsxSubnet.Path, []string{}); err != nil { + if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *nsxSubnet.Path, []nsxutil.GPRRType{}); err != nil { log.Error(err, "Failed to check subnet realization state", "ID", *nsxSubnet.Id) // Delete the subnet if realization check fails, avoiding creating duplicate subnets continuously. deleteErr := service.DeleteSubnet(*nsxSubnet) diff --git a/pkg/nsx/services/subnet/subnet_test.go b/pkg/nsx/services/subnet/subnet_test.go index ee3d329c7..441e482ab 100644 --- a/pkg/nsx/services/subnet/subnet_test.go +++ b/pkg/nsx/services/subnet/subnet_test.go @@ -481,7 +481,7 @@ func TestSubnetService_createOrUpdateSubnet(t *testing.T) { name: "Update Subnet with RealizedState and deletion error", prepareFunc: func() *gomonkey.Patches { patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState, - func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []string) error { + func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []nsxutil.GPRRType) error { return nsxutil.NewRealizeStateError("mocked realized error") }) patches.ApplyFunc((*SubnetService).DeleteSubnet, func(_ *SubnetService, _ model.VpcSubnet) error { @@ -499,7 +499,7 @@ func TestSubnetService_createOrUpdateSubnet(t *testing.T) { name: "Create Subnet for SubnetSet Success", prepareFunc: func() *gomonkey.Patches { patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState, - func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []string) error { + func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []nsxutil.GPRRType) error { return nil }) patches.ApplyFunc(fakeSubnetsClient.Get, diff --git a/pkg/nsx/services/subnetport/subnetport.go b/pkg/nsx/services/subnetport/subnetport.go index 9c35f6158..079334dfd 100644 --- a/pkg/nsx/services/subnetport/subnetport.go +++ b/pkg/nsx/services/subnetport/subnetport.go @@ -168,7 +168,7 @@ func (service *SubnetPortService) CheckSubnetPortState(obj interface{}, nsxSubne } realizeService := realizestate.InitializeRealizeState(service.Service) - if err := realizeService.CheckRealizeState(util.NSXTRealizeRetry, *nsxSubnetPort.Path, []string{}); err != nil { + if err := realizeService.CheckRealizeState(util.NSXTRealizeRetry, *nsxSubnetPort.Path, []nsxutil.GPRRType{}); err != nil { log.Error(err, "failed to get realized status", "subnetport path", *nsxSubnetPort.Path) if nsxutil.IsRealizeStateError(err) { log.Error(err, "the created subnet port is in error realization state, cleaning the resource", "subnetport", portID) diff --git a/pkg/nsx/services/vpc/vpc.go b/pkg/nsx/services/vpc/vpc.go index a170e62e5..0412c7602 100644 --- a/pkg/nsx/services/vpc/vpc.go +++ b/pkg/nsx/services/vpc/vpc.go @@ -43,6 +43,31 @@ var ( EnforceRevisionCheckParam = false ) +func GetGPRRTypeList(id string) []nsxutil.GPRRType { + return []nsxutil.GPRRType{ + { + Id: fmt.Sprintf("%s-folder", id), + EntityType: "RealizedVpcFolder", + }, + { + Id: "gateway-interface", + EntityType: "RealizedLogicalRouterPort", + }, + { + Id: id, + EntityType: "RealizedLogicalRouter", + }, + { + Id: id, + EntityType: "RealizedLogicalRouterPort", + }, + { + Id: "security-config", + EntityType: "RealizedSecurityFeatureToggle", + }, + } +} + type VPCNetworkInfoStore struct { sync.RWMutex VPCNetworkConfigMap map[string]common.VPCNetworkConfigInfo @@ -856,7 +881,7 @@ func (s *VPCService) createNSXVPC(createdVpc *model.Vpc, nc *common.VPCNetworkCo func (s *VPCService) checkVPCRealizationState(createdVpc *model.Vpc, newVpcPath string) error { log.V(2).Info("Check VPC realization state", "VPC", *createdVpc.Id) realizeService := realizestate.InitializeRealizeState(s.Service) - if err := realizeService.CheckRealizeState(util.NSXTRealizeRetry, newVpcPath, []string{common.GatewayInterfaceId}); err != nil { + if err := realizeService.CheckRealizeState(util.NSXTRealizeRetry, newVpcPath, GetGPRRTypeList(*createdVpc.Id)); err != nil { log.Error(err, "Failed to check VPC realization state", "VPC", *createdVpc.Id) if nsxutil.IsRealizeStateError(err) { log.Error(err, "The created VPC is in error realization state, cleaning the resource", "VPC", *createdVpc.Id) @@ -884,7 +909,7 @@ func (s *VPCService) checkLBSRealization(createdLBS *model.LBService, createdVpc log.V(2).Info("Check LBS realization state", "LBS", *createdLBS.Id) realizeService := realizestate.InitializeRealizeState(s.Service) - if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *newLBS.Path, []string{}); err != nil { + if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *newLBS.Path, []nsxutil.GPRRType{}); err != nil { log.Error(err, "Failed to check LBS realization state", "LBS", *createdLBS.Id) if nsxutil.IsRealizeStateError(err) { log.Error(err, "The created LBS is in error realization state, cleaning the resource", "LBS", *createdLBS.Id) @@ -910,7 +935,7 @@ func (s *VPCService) checkVpcAttachmentRealization(createdAttachment *model.VpcA } log.V(2).Info("Check VPC attachment realization state", "VpcAttachment", *createdAttachment.Id) realizeService := realizestate.InitializeRealizeState(s.Service) - if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *newAttachment.Path, []string{}); err != nil { + if err = realizeService.CheckRealizeState(util.NSXTRealizeRetry, *newAttachment.Path, []nsxutil.GPRRType{}); err != nil { log.Error(err, "Failed to check VPC attachment realization state", "VpcAttachment", *createdAttachment.Id) if nsxutil.IsRealizeStateError(err) { log.Error(err, "The created VPC attachment is in error realization state, cleaning the resource", "VpcAttachment", *createdAttachment.Id) diff --git a/pkg/nsx/services/vpc/vpc_test.go b/pkg/nsx/services/vpc/vpc_test.go index 350e416ec..a2adf8921 100644 --- a/pkg/nsx/services/vpc/vpc_test.go +++ b/pkg/nsx/services/vpc/vpc_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/wait" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -35,6 +36,7 @@ import ( "github.com/vmware-tanzu/nsx-operator/pkg/nsx" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/ratelimiter" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" + "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/realizestate" nsxUtil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util" ) @@ -2133,3 +2135,99 @@ func TestInitializeVPC(t *testing.T) { assert.Equal(t, tc.expectAllVPCNum, len(allVPCs)) } } + +func TestCheckVPCRealizationState(t *testing.T) { + service := &VPCService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + RealizedEntitiesClient: &fakeRealizedEntitiesClient{}, + }, + }, + } + + testCases := []struct { + name string + prepareFunc func() *gomonkey.Patches + expectedErr string + }{ + { + name: "Successful realization", + prepareFunc: func() *gomonkey.Patches { + patches := gomonkey.ApplyFunc(fakeRealizedEntitiesClient.List, + func(_ fakeRealizedEntitiesClient, intentPathParam string, sitePathParam *string) (model.GenericPolicyRealizedResourceListResult, error) { + return model.GenericPolicyRealizedResourceListResult{ + Results: []model.GenericPolicyRealizedResource{ + { + Id: common.String("vpc-1-folder"), + EntityType: common.String("RealizedVpcFolder"), + State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), + }, + { + Id: common.String("gateway-interface"), + EntityType: common.String("RealizedLogicalRouterPort"), + State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), + }, + { + Id: common.String("vpc-1"), + EntityType: common.String("RealizedLogicalRouter"), + State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), + }, + { + Id: common.String("vpc-1"), + EntityType: common.String("RealizedLogicalRouterPort"), + State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), + }, + { + Id: common.String("security-config"), + EntityType: common.String("RealizedSecurityFeatureToggle"), + State: common.String(model.GenericPolicyRealizedResource_STATE_REALIZED), + }, + }, + }, nil + }) + return patches + }, + }, + { + name: "Failed realization", + prepareFunc: func() *gomonkey.Patches { + patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState, + func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []nsxUtil.GPRRType) error { + return nsxUtil.NewRealizeStateError("mocked realized error") + }) + patches.ApplyFunc((*VPCService).DeleteVPC, func(_ *VPCService, _ string) error { + return nil + }) + return patches + }, + expectedErr: "mocked realized error", + }, + { + name: "Failed deletion", + prepareFunc: func() *gomonkey.Patches { + patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState, + func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string, _ []nsxUtil.GPRRType) error { + return nsxUtil.NewRealizeStateError("mocked realized error") + }) + patches.ApplyFunc((*VPCService).DeleteVPC, func(_ *VPCService, _ string) error { + return nsxUtil.NewRealizeStateError("mocked deletion error") + }) + return patches + }, + expectedErr: "mocked deletion error", + }, + } + + for _, tc := range testCases { + if tc.prepareFunc != nil { + patches := tc.prepareFunc() + defer patches.Reset() + } + err := service.checkVPCRealizationState(&model.Vpc{Id: common.String("vpc-1")}, "/orgs/org/projects/project/vpcs/vpc-1") + if tc.expectedErr != "" { + assert.Contains(t, err.Error(), tc.expectedErr) + } else { + assert.Nil(t, err) + } + } +} diff --git a/pkg/nsx/util/utils.go b/pkg/nsx/util/utils.go index 448d2d791..bc06f3eb6 100644 --- a/pkg/nsx/util/utils.go +++ b/pkg/nsx/util/utils.go @@ -57,6 +57,11 @@ type PortAddress struct { IPs []string `json:"ips"` } +type GPRRType struct { + Id string + EntityType string +} + func (e *ErrorDetail) Error() string { msg := fmt.Sprintf("StatusCode is %d,", e.StatusCode) if e.ErrorCode > 0 { diff --git a/test/e2e/precreated_vpc_test.go b/test/e2e/precreated_vpc_test.go index 425311c60..d302af320 100644 --- a/test/e2e/precreated_vpc_test.go +++ b/test/e2e/precreated_vpc_test.go @@ -20,6 +20,7 @@ import ( "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/realizestate" "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vpc" + nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util" pkgutil "github.com/vmware-tanzu/nsx-operator/pkg/util" ) @@ -292,17 +293,17 @@ func (data *TestData) createVPC(orgID, projectID, vpcID string, privateIPs []str log.Info("Successfully requested VPC on NSX", "path", vpcPath) realizeService := realizestate.InitializeRealizeState(common.Service{NSXClient: data.nsxClient.Client}) if pollErr := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (done bool, err error) { - if err = realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, vpcPath, []string{common.GatewayInterfaceId}); err != nil { + if err = realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, vpcPath, vpc.GetGPRRTypeList(vpcID)); err != nil { log.Error(err, "NSX VPC is not yet realized", "path", vpcPath) return false, nil } if lbsPath != "" { - if err := realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, lbsPath, []string{}); err != nil { + if err := realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, lbsPath, []nsxutil.GPRRType{}); err != nil { log.Error(err, "NSX LBS is not yet realized", "path", lbsPath) return false, nil } } - if err = realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, attachmentPath, []string{}); err != nil { + if err = realizeService.CheckRealizeState(pkgutil.NSXTRealizeRetry, attachmentPath, []nsxutil.GPRRType{}); err != nil { log.Error(err, "VPC attachment is not yet realized", "path", attachmentPath) return false, nil }