Skip to content

Commit

Permalink
Fix the Subnet creating with stale VPC issue (vmware-tanzu#825)
Browse files Browse the repository at this point in the history 
Fix the Subnet creating with stale VPC issue

Signed-off-by: Wenqi Qiu <[email protected]>
  • Loading branch information
@wenqiq
wenqiq authored Nov 5, 2024
1 parent 2b6d1b7 commit bf1880a
Show file tree
Hide file tree
Showing 6 changed files with 286 additions and 102 deletions.
7 changes: 6 additions & 1 deletion pkg/controllers/networkinfo/networkinfo_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,14 +445,19 @@ func (r *NetworkInfoReconciler) CollectGarbage(ctx context.Context) {
func (r *NetworkInfoReconciler) fetchStaleVPCsByNamespace(ctx context.Context, ns string) ([]*model.Vpc, error) {
isShared, err := r.Service.IsSharedVPCNamespaceByNS(ctx, ns)
if err != nil {
if apierrors.IsNotFound(err) {
// if the Namespace has been deleted, we never know whether it`s a shared Namespace, The GC will delete the stale VPCs
log.Info("Namespace does not exist while fetching stale VPCs", "Namespace", ns)
return nil, nil
}
return nil, fmt.Errorf("failed to check if Namespace is shared for NS %s: %w", ns, err)
}
if isShared {
log.Info("Shared Namespace, skipping deletion of NSX VPC", "Namespace", ns)
return nil, nil
}

return r.Service.GetVPCsByNamespace(ctx, ns), nil
return r.Service.GetVPCsByNamespace(ns), nil
}

func (r *NetworkInfoReconciler) deleteVPCsByName(ctx context.Context, ns string) error {
Expand Down
22 changes: 14 additions & 8 deletions pkg/controllers/networkinfo/networkinfo_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func TestNetworkInfoReconciler_Reconcile(t *testing.T) {
patches = gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
Expand Down Expand Up @@ -805,7 +805,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
})
return patches
Expand All @@ -817,7 +817,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath := "/vpc/1"
return []*model.Vpc{{Path: &vpcPath}}
})
Expand All @@ -834,7 +834,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName"
Expand Down Expand Up @@ -875,7 +875,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName1"
Expand Down Expand Up @@ -948,7 +948,7 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "testNamespace", UID: "fakeNamespaceUID"},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
vpcName := "fakeVPCName"
vpcPath := "fakeVPCPath"
return []*model.Vpc{
Expand Down Expand Up @@ -981,6 +981,12 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
DeletionTimestamp: &metav1.Time{Time: time.Now()}, Finalizers: []string{"test-Finalizers"}},
VPCs: nil,
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
return nil
})
return patches
},
expectRes: common.ResultNormal,
req: reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "testNamespace", Name: "testNetworkInfo"}},
},
Expand All @@ -993,7 +999,7 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
Status: corev1.NamespaceStatus{},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
return gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
return gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
id := "fakeNamespaceUID"
scope := servicecommon.TagScopeNamespaceUID
tag1 := []model.Tag{
Expand Down Expand Up @@ -1068,7 +1074,7 @@ func TestNetworkInfoReconciler_Create(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "testNamespace", UID: "fakeNamespaceUID"},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ctx context.Context, _ string) []*model.Vpc {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcName := "fakeVPCName"
vpcPath := "fakeVPCPath"
return []*model.Vpc{
Expand Down
4 changes: 2 additions & 2 deletions pkg/nsx/services/vpc/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ func (s *VPCStore) GetByIndex(key string, value string) []*model.Vpc {
return vpcs
}

func (s *VPCStore) GetVPCsByNamespace(ns string) []*model.Vpc {
func (s *VPCStore) GetVPCsByNamespaceFromStore(ns string) []*model.Vpc {
return s.GetByIndex(common.TagScopeNamespace, ns)
}

func (s *VPCStore) GetVPCsByNamespaceID(namespaceID string) []*model.Vpc {
func (s *VPCStore) GetVPCsByNamespaceIDFromStore(namespaceID string) []*model.Vpc {
return s.GetByIndex(common.TagScopeNamespaceUID, namespaceID)
}

Expand Down
7 changes: 3 additions & 4 deletions pkg/nsx/services/vpc/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ func TestVPCStore_CRUDResource_List(t *testing.T) {
},
}
vpc1 := model.Vpc{

DisplayName: &vpcName1,
Id: &vpcID1,
Tags: tag1,
Expand All @@ -195,7 +194,6 @@ func TestVPCStore_CRUDResource_List(t *testing.T) {
ExternalIpv4Blocks: []string{"2.2.2.0/24"},
}
vpc2 := model.Vpc{

DisplayName: &vpcName2,
Id: &vpcID2,
Tags: tag2,
Expand Down Expand Up @@ -248,16 +246,17 @@ func TestVPCStore_CRUDResource_List(t *testing.T) {
assert.NoError(t, err)
}
got := vpcStore.List()

assert.Equal(t, tc.expectVPCInStore, len(got))
if tc.searchNameKey != "" {
vpcRes := vpcStore.GetVPCsByNamespace(tc.searchNameKey)
vpcRes := vpcStore.GetVPCsByNamespaceFromStore(tc.searchNameKey)
assert.Equal(t, len(tc.expectResVPC), len(vpcRes))
for _, vpc := range vpcRes {
assert.Contains(t, tc.expectResVPC, *vpc.DisplayName)
}
}
if tc.searchIDKey != "" {
vpcRes := vpcStore.GetVPCsByNamespaceID(tc.searchIDKey)
vpcRes := vpcStore.GetVPCsByNamespaceIDFromStore(tc.searchIDKey)
assert.Equal(t, len(tc.expectResVPC), len(vpcRes))
for _, vpc := range vpcRes {
assert.Contains(t, tc.expectResVPC, *vpc.DisplayName)
Expand Down
62 changes: 38 additions & 24 deletions pkg/nsx/services/vpc/vpc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ import (
stderrors "github.com/vmware/vsphere-automation-sdk-go/lib/vapi/std/errors"
"github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model"
v1 "k8s.io/api/core/v1"
apirrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
"github.com/vmware-tanzu/nsx-operator/pkg/logger"
Expand Down Expand Up @@ -185,13 +183,20 @@ func InitializeVPC(service common.Service) (*VPCService, error) {
return VPCService, nil
}

func (s *VPCService) GetVPCsByNamespace(ctx context.Context, namespace string) []*model.Vpc {
sns, err := s.getSharedVPCNamespaceFromNS(ctx, namespace)
func (s *VPCService) GetCurrentVPCsByNamespace(ctx context.Context, namespace string) []*model.Vpc {
namespaceObj, sharedNamespace, err := s.resolveSharedVPCNamespace(ctx, namespace)
if err != nil {
log.Error(err, "Failed to get Namespace")
return nil
}
return s.VpcStore.GetVPCsByNamespace(util.If(sns == "", namespace, sns).(string))
if sharedNamespace == nil {
return s.VpcStore.GetVPCsByNamespaceIDFromStore(string(namespaceObj.UID))
}
return s.VpcStore.GetVPCsByNamespaceIDFromStore(string(sharedNamespace.UID))
}

func (s *VPCService) GetVPCsByNamespace(namespace string) []*model.Vpc {
return s.VpcStore.GetVPCsByNamespaceFromStore(namespace)
}

func (s *VPCService) ListVPC() []model.Vpc {
Expand Down Expand Up @@ -521,53 +526,61 @@ func (s *VPCService) DeleteLBPool(path string) error {
}

func (s *VPCService) IsSharedVPCNamespaceByNS(ctx context.Context, ns string) (bool, error) {
sharedNS, err := s.getSharedVPCNamespaceFromNS(ctx, ns)
_, sharedNamespaceObj, err := s.resolveSharedVPCNamespace(ctx, ns)
if err != nil {
if apirrors.IsNotFound(err) {
return false, nil
}
return false, err
}
if sharedNS == "" {
if sharedNamespaceObj == nil {
return false, nil
}
if sharedNS != ns {
if sharedNamespaceObj.Name != ns {
return true, nil
}
return false, err
}

func getNamespace(c client.Client, ctx context.Context, ns string) (*v1.Namespace, error) {
func (s *VPCService) getNamespace(ctx context.Context, name string) (*v1.Namespace, error) {
obj := &v1.Namespace{}
if err := c.Get(ctx, types.NamespacedName{Name: ns}, obj); err != nil {
log.Error(err, "Failed to fetch Namespace", "Namespace", ns)
if err := s.Client.Get(ctx, types.NamespacedName{Name: name}, obj); err != nil {
log.Error(err, "Failed to fetch Namespace", "Namespace", name)
return nil, err
}
return obj, nil
}

func (s *VPCService) getSharedVPCNamespaceFromNS(ctx context.Context, ns string) (string, error) {
obj, err := getNamespace(s.Client, ctx, ns)
// resolveSharedVPCNamespace will resolve the Namespace relationship based on VPC sharing,
// whether a shared VPC Namespace exists.
func (s *VPCService) resolveSharedVPCNamespace(ctx context.Context, ns string) (*v1.Namespace, *v1.Namespace, error) {
obj, err := s.getNamespace(ctx, ns)
if err != nil {
return "", err
return nil, nil, err
}

annos := obj.Annotations
// If no annotaion on ns, then this is not a shared VPC ns
if len(annos) == 0 {
return "", nil
return obj, nil, nil
}

// If no annotation nsx.vmware.com/shared_vpc_namespace on ns, this is not a shared vpc
sharedNS, exist := annos[common.AnnotationSharedVPCNamespace]
nsForSharedVPCs, exist := annos[common.AnnotationSharedVPCNamespace]
if !exist {
return "", nil
return obj, nil, nil
}
if nsForSharedVPCs == ns {
return nil, obj, nil
}
sharedNamespace, err := s.getNamespace(ctx, nsForSharedVPCs)
if err != nil {
// if sharedNamespace does not exist, add this case for security,
// It shouldn't happen that a shared Namespace doesn't exist but is used as an annotation on another Namespace.
return nil, nil, err
}
return sharedNS, nil
return nil, sharedNamespace, nil
}

func (s *VPCService) GetNetworkconfigNameFromNS(ctx context.Context, ns string) (string, error) {
obj, err := getNamespace(s.Client, ctx, ns)
obj, err := s.getNamespace(ctx, ns)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -731,7 +744,7 @@ func (s *VPCService) CreateOrUpdateVPC(ctx context.Context, obj *v1alpha1.Networ
return nil, err
}

existingVPC := s.GetVPCsByNamespace(ctx, ns)
existingVPC := s.GetCurrentVPCsByNamespace(ctx, ns)
updateVpc := len(existingVPC) != 0
if updateVpc && isShared { // We now consider only one VPC for one namespace
log.Info("The shared VPC already exist", "Namespace", ns)
Expand Down Expand Up @@ -1109,7 +1122,8 @@ func (s *VPCService) ListVPCInfo(ns string) []common.VPCResourceInfo {
}

// List VPCs from local store.
vpcs := s.GetVPCsByNamespace(context.Background(), ns) // Transparently call the VPCService.GetVPCsByNamespace method
vpcs := s.GetCurrentVPCsByNamespace(context.Background(), ns)
log.Info("Got VPCs by Namespace from store", "Namespace", ns, "VPCCount", len(vpcs))
for _, v := range vpcs {
vpcResourceInfo, err := common.ParseVPCResourcePath(*v.Path)
if err != nil {
Expand Down
Loading

0 comments on commit bf1880a

Please sign in to comment.