Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the Subnet creating with stale VPC issue #825

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Fix the Subnet creating with stale VPC issue #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f8873f
Fix the Subnet creating with stale VPC issue
wenqiq Oct 25, 2024
71681fe
Add unit-test
wenqiq Nov 4, 2024
bdadb1d
Update
wenqiq Nov 4, 2024
d5ddcba
Update
wenqiq Nov 5, 2024
5b13451
Rename GetVPCsByNamespaceUID to GetCurrentVPCsByNamespace
wenqiq Nov 5, 2024
0904e44
update
wenqiq Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/controllers/networkinfo/networkinfo_controller.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
timdengyun marked this conversation as resolved.
Show resolved Hide resolved
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)
timdengyun marked this conversation as resolved.
Show resolved Hide resolved
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,
wenqiq marked this conversation as resolved.
Show resolved Hide resolved
// 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
Loading