From 324228f66828a7f3fdecd116fae3e31778e640b6 Mon Sep 17 00:00:00 2001 From: Bruno J <60146370+bru-jer-work@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:00:35 +0000 Subject: [PATCH 1/2] task(GCP VPC peering) Allows object deletion in any circunstances (#805) --- .../cloud-control/vpcpeering_gcp_test.go | 148 ++++++++++++++++++ pkg/kcp/provider/gcp/mock/vpcPeeringStore.go | 25 ++- .../gcp/vpcpeering/loadRemoteNetwork.go | 5 + .../gcp/vpcpeering/loadRemoteVpcPeering.go | 5 + .../gcpvpcpeering/deleteKcpRemoteNetwork.go | 10 +- pkg/skr/gcpvpcpeering/deleteKcpVpcPeering.go | 12 +- 6 files changed, 184 insertions(+), 21 deletions(-) diff --git a/internal/controller/cloud-control/vpcpeering_gcp_test.go b/internal/controller/cloud-control/vpcpeering_gcp_test.go index 8d5286c35..59e2cf704 100644 --- a/internal/controller/cloud-control/vpcpeering_gcp_test.go +++ b/internal/controller/cloud-control/vpcpeering_gcp_test.go @@ -2,6 +2,7 @@ package cloudcontrol import ( pb "cloud.google.com/go/compute/apiv1/computepb" + "errors" cloudcontrolv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-control/v1beta1" networkPkg "github.com/kyma-project/cloud-manager/pkg/kcp/network" scopePkg "github.com/kyma-project/cloud-manager/pkg/kcp/scope" @@ -184,6 +185,153 @@ var _ = Describe("Feature: KCP VpcPeering", func() { Should(Succeed(), "Error deleting VPC Peering") }) + By("Then VpcPeering does not exist", func() { + Eventually(IsDeleted). + WithArguments(infra.Ctx(), infra.KCP().Client(), vpcpeering). + Should(Succeed(), "VPC Peering was not deleted") + }) + }) + + It("Scenario: KCP GCP VpcPeering can be deleted due to issues on the remote network", func() { + const ( + kymaName = "ec697362-8f63-4423-b34f-8a99c0460d46" + kymaNetworkName = kymaName + "--kyma" + kymaProject = "kyma-project" + kymaVpc = "shoot-12345-abc" + remoteNetworkName = "0ab0eca3-3094-4842-9834-7492aaa0639d" + remotePeeringName = "peering-with-permission-error" + remoteVpc = "remote-vpc" + remoteProject = "remote-project" + remoteRefNamespace = "kcp-system" + remoteRefName = "skr-gcp-vpcpeering" + importCustomRoutes = false + ) + + scope := &cloudcontrolv1beta1.Scope{} + + By("Given Scope exists", func() { + scopePkg.Ignore.AddName(kymaName) + + Eventually(CreateScopeGcp). + WithArguments(infra.Ctx(), infra, scope, WithName(kymaName)). + Should(Succeed()) + }) + + // and Given the Kyma network object exists in KCP + kymaNetwork := &cloudcontrolv1beta1.Network{ + Spec: cloudcontrolv1beta1.NetworkSpec{ + Network: cloudcontrolv1beta1.NetworkInfo{ + Reference: &cloudcontrolv1beta1.NetworkReference{ + Gcp: &cloudcontrolv1beta1.GcpNetworkReference{ + GcpProject: kymaProject, + NetworkName: kymaVpc, + }, + }, + }, + Type: cloudcontrolv1beta1.NetworkTypeKyma, + }, + } + + By("And Given Kyma Network exists in KCP", func() { + // Tell Scope reconciler to ignore this kymaName + networkPkg.Ignore.AddName(kymaNetworkName) + + Eventually(CreateObj). + WithArguments(infra.Ctx(), infra.KCP().Client(), kymaNetwork, WithName(kymaNetworkName), WithScope(scope.Name)). + Should(Succeed()) + }) + + // and Given the remote network object exists in KCP + remoteNetwork := &cloudcontrolv1beta1.Network{ + Spec: cloudcontrolv1beta1.NetworkSpec{ + Network: cloudcontrolv1beta1.NetworkInfo{ + Reference: &cloudcontrolv1beta1.NetworkReference{ + Gcp: &cloudcontrolv1beta1.GcpNetworkReference{ + GcpProject: remoteProject, + NetworkName: remoteVpc, + }, + }, + }, + Type: cloudcontrolv1beta1.NetworkTypeExternal, + }, + } + + By("And Given Remote Network exists in KCP", func() { + // Tell Scope reconciler to ignore this kymaName + networkPkg.Ignore.AddName(remoteNetworkName) + + Eventually(CreateObj). + WithArguments(infra.Ctx(), infra.KCP().Client(), remoteNetwork, WithName(remoteNetworkName), WithScope(scope.Name), WithState("Ready")). + Should(Succeed()) + }) + + By("And Given KCP KymaNetwork is Ready", func() { + Eventually(UpdateStatus). + WithArguments(infra.Ctx(), + infra.KCP().Client(), + kymaNetwork, + WithState("Ready"), + WithConditions(KcpReadyCondition())). + Should(Succeed()) + }) + + By("And Given KCP RemoteNetwork is Ready", func() { + Eventually(UpdateStatus). + WithArguments(infra.Ctx(), + infra.KCP().Client(), + remoteNetwork, + WithState("Ready"), + WithConditions(KcpReadyCondition())). + Should(Succeed()) + }) + + vpcpeering := &cloudcontrolv1beta1.VpcPeering{ + Spec: cloudcontrolv1beta1.VpcPeeringSpec{ + Details: &cloudcontrolv1beta1.VpcPeeringDetails{ + LocalNetwork: klog.ObjectRef{ + Name: kymaNetwork.Name, + Namespace: kymaNetwork.Namespace, + }, + RemoteNetwork: klog.ObjectRef{ + Name: remoteNetwork.Name, + Namespace: remoteNetwork.Namespace, + }, + PeeringName: remotePeeringName, + LocalPeeringName: "cm-" + remoteNetworkName, + ImportCustomRoutes: importCustomRoutes, + }, + }, + } + + By("And Given there is no permission to read remote network tags", func() { + infra.GcpMock().SetMockVpcPeeringError(remoteProject, remoteVpc, errors.New("permission denied")) + }) + + By("And Given KCP VpcPeering is created", func() { + Eventually(CreateObj). + WithArguments(infra.Ctx(), infra.KCP().Client(), vpcpeering, + WithName(remoteNetworkName), + WithRemoteRef(remoteRefName), + WithScope(kymaName), + ). + Should(Succeed()) + }) + + By("Then KCP VpcPeering has Error condition", func() { + Eventually(LoadAndCheck). + WithArguments(infra.Ctx(), infra.KCP().Client(), vpcpeering, + NewObjActions(), + HavingConditionTrue(cloudcontrolv1beta1.ConditionTypeError), + ). + Should(Succeed()) + }) + + By("When KCP VpcPeering in error state is deleted", func() { + Eventually(Delete). + WithArguments(infra.Ctx(), infra.KCP().Client(), vpcpeering). + Should(Succeed(), "Error deleting VPC Peering") + }) + By("Then VpcPeering does not exist", func() { Eventually(IsDeleted). WithArguments(infra.Ctx(), infra.KCP().Client(), vpcpeering). diff --git a/pkg/kcp/provider/gcp/mock/vpcPeeringStore.go b/pkg/kcp/provider/gcp/mock/vpcPeeringStore.go index 3fb9161e7..f11caf704 100644 --- a/pkg/kcp/provider/gcp/mock/vpcPeeringStore.go +++ b/pkg/kcp/provider/gcp/mock/vpcPeeringStore.go @@ -12,13 +12,15 @@ type vpcPeeringEntry struct { peering *pb.NetworkPeering } type vpcPeeringStore struct { - m sync.Mutex - items map[string]*vpcPeeringEntry + m sync.Mutex + items map[string]*vpcPeeringEntry + errorMap map[string]error } type VpcPeeringMockClientUtils interface { GetMockVpcPeering(project string, vpc string) *pb.NetworkPeering SetMockVpcPeeringLifeCycleState(project string, vpc string, state pb.NetworkPeering_State) + SetMockVpcPeeringError(project string, vpc string, err error) } func getFullNetworkUrl(project, vpc string) string { @@ -102,12 +104,20 @@ func (s *vpcPeeringStore) GetVpcPeering(ctx context.Context, remotePeeringName s s.m.Lock() defer s.m.Unlock() + network := getFullNetworkUrl(project, vpc) + + if s.errorMap == nil { + s.errorMap = make(map[string]error) + } + + if err, errorExists := s.errorMap[network]; errorExists { + return nil, err + } + if s.items == nil { s.items = make(map[string]*vpcPeeringEntry) } - network := getFullNetworkUrl(project, vpc) - _, peeringExists := s.items[network] if !peeringExists { return nil, nil @@ -144,3 +154,10 @@ func (s *vpcPeeringStore) GetMockVpcPeering(project string, vpc string) *pb.Netw } return s.items[getFullNetworkUrl(project, vpc)].peering } + +func (s *vpcPeeringStore) SetMockVpcPeeringError(project string, vpc string, err error) { + if s.errorMap == nil { + s.errorMap = make(map[string]error) + } + s.errorMap[getFullNetworkUrl(project, vpc)] = err +} diff --git a/pkg/kcp/provider/gcp/vpcpeering/loadRemoteNetwork.go b/pkg/kcp/provider/gcp/vpcpeering/loadRemoteNetwork.go index 6970a2c7b..a94859c6b 100644 --- a/pkg/kcp/provider/gcp/vpcpeering/loadRemoteNetwork.go +++ b/pkg/kcp/provider/gcp/vpcpeering/loadRemoteNetwork.go @@ -31,6 +31,11 @@ func loadRemoteNetwork(ctx context.Context, st composed.State) (error, context.C } if apierrors.IsNotFound(err) { + if composed.IsMarkedForDeletion(state.ObjAsVpcPeering()) { + logger.Error(err, "[KCP GCP VPCPeering loadRemoteNetwork] GCP KCP VpcPeering Remote Network not found, proceeding with deletion") + return nil, nil + } + state.ObjAsVpcPeering().Status.State = string(cloudcontrolv1beta1.ErrorState) return composed.PatchStatus(state.ObjAsVpcPeering()). SetExclusiveConditions(metav1.Condition{ diff --git a/pkg/kcp/provider/gcp/vpcpeering/loadRemoteVpcPeering.go b/pkg/kcp/provider/gcp/vpcpeering/loadRemoteVpcPeering.go index d241c8269..25dc4c654 100644 --- a/pkg/kcp/provider/gcp/vpcpeering/loadRemoteVpcPeering.go +++ b/pkg/kcp/provider/gcp/vpcpeering/loadRemoteVpcPeering.go @@ -22,6 +22,11 @@ func loadRemoteVpcPeering(ctx context.Context, st composed.State) (error, contex remoteVpcPeering, err := state.client.GetVpcPeering(ctx, state.remotePeeringName, state.remoteNetwork.Spec.Network.Reference.Gcp.GcpProject, state.remoteNetwork.Spec.Network.Reference.Gcp.NetworkName) if err != nil { + if composed.IsMarkedForDeletion(state.ObjAsVpcPeering()) { + logger.Info("[KCP GCP VPCPeering loadRemoteVPCPeering] GCP KCP VpcPeering Error fetching Remote Network, proceeding with deletion") + return nil, nil + } + state.ObjAsVpcPeering().Status.State = v1beta1.VirtualNetworkPeeringStateDisconnected logger.Error(err, "Error loading Remote VpcPeering") meta.SetStatusCondition(state.ObjAsVpcPeering().Conditions(), metav1.Condition{ diff --git a/pkg/skr/gcpvpcpeering/deleteKcpRemoteNetwork.go b/pkg/skr/gcpvpcpeering/deleteKcpRemoteNetwork.go index 393a9a485..65f33ae04 100644 --- a/pkg/skr/gcpvpcpeering/deleteKcpRemoteNetwork.go +++ b/pkg/skr/gcpvpcpeering/deleteKcpRemoteNetwork.go @@ -10,23 +10,15 @@ func deleteKcpRemoteNetwork(ctx context.Context, st composed.State) (error, cont state := st.(*State) logger := composed.LoggerFromCtx(ctx) - if !composed.MarkedForDeletionPredicate(ctx, state) { - return nil, nil - } - if state.KcpRemoteNetwork == nil { return nil, nil } if composed.IsMarkedForDeletion(state.KcpRemoteNetwork) { + logger.Info("[SKR GCP VPCPeering deleteKcpRemoteNetwork] KCP Remote Network is marked for deletion, will continue.") return nil, nil } - if state.KcpVpcPeering != nil { - logger.Info("[SKR GCP VPCPeering deleteKcpRemoteNetwork] Waiting for KCP VpcPeering deletion") - return composed.StopWithRequeueDelay(3 * util.Timing.T1000ms()), nil - } - logger.Info("[SKR GCP VPCPeering deleteKcpRemoteNetwork] Deleting GCP KCP Remote Network") err := state.KcpCluster.K8sClient().Delete(ctx, state.KcpRemoteNetwork) diff --git a/pkg/skr/gcpvpcpeering/deleteKcpVpcPeering.go b/pkg/skr/gcpvpcpeering/deleteKcpVpcPeering.go index f16eaae22..dd79cef65 100644 --- a/pkg/skr/gcpvpcpeering/deleteKcpVpcPeering.go +++ b/pkg/skr/gcpvpcpeering/deleteKcpVpcPeering.go @@ -11,25 +11,21 @@ func deleteKcpVpcPeering(ctx context.Context, st composed.State) (error, context state := st.(*State) logger := composed.LoggerFromCtx(ctx) - if !composed.MarkedForDeletionPredicate(ctx, state) { - return nil, nil - } - if state.KcpVpcPeering == nil { - // VpcPeering on SKR is marked for deletion, but not found in KCP, so probably it is already deleted return nil, nil } if composed.IsMarkedForDeletion(state.KcpVpcPeering) { - return nil, nil + logger.Info("[SKR GCP VPCPeering deleteKcpVpcPeering] KCP VpcPeering is marked for deletion, re-queueing until it is deleted.") + return composed.StopWithRequeueDelay(util.Timing.T10000ms()), nil } - logger.Info("Deleting KCP VpcPeering") + logger.Info("[SKR GCP VPCPeering deleteKcpVpcPeering] Deleting KCP VpcPeering") err := state.KcpCluster.K8sClient().Delete(ctx, state.KcpVpcPeering) if err != nil { - return composed.LogErrorAndReturn(err, "Error deleting KCP VpcPeering", composed.StopWithRequeue, ctx) + return composed.LogErrorAndReturn(err, "[SKR GCP VPCPeering deleteKcpVpcPeering] Error deleting KCP VpcPeering", composed.StopWithRequeue, ctx) } state.ObjAsGcpVpcPeering().Status.State = cloudcontrolv1beta1.VirtualNetworkPeeringStateDeleting From c9a254f30bfeb6e453aa1e01a4affee1ad85b546 Mon Sep 17 00:00:00 2001 From: Vladimir Andjelkoski Date: Wed, 27 Nov 2024 18:14:41 +0100 Subject: [PATCH 2/2] chore: fixes race condition in tests --- .../cloud-control/vpcpeering_azure_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/controller/cloud-control/vpcpeering_azure_test.go b/internal/controller/cloud-control/vpcpeering_azure_test.go index b36d34ff5..ce7dea19b 100644 --- a/internal/controller/cloud-control/vpcpeering_azure_test.go +++ b/internal/controller/cloud-control/vpcpeering_azure_test.go @@ -415,6 +415,20 @@ var _ = Describe("Feature: KCP VpcPeering", func() { ).Should(Succeed()) }) + // Waits for the reconcilers to create Azure peerings + By("And Then remote Azure VPC Peering is created", func() { + Eventually(func() error { + p, err := azureMockRemote.GetPeering(infra.Ctx(), remoteResourceGroup, remoteVnetName, remotePeeringName) + if err != nil { + return err + } + if p == nil { + return errors.New("nil peering received") + } + return nil + }).Should(Succeed()) + }) + // Ready ========================================================== By("When Azure VPC Peering is Connected", func() {