From 813dde2bf18446da1ca58a36ad997607e1cc12c1 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Tue, 6 Dec 2022 15:16:13 +0000 Subject: [PATCH 1/2] Fix L4 NetLB RBS cleanup when Service Type changed This fixes issue, that on switching Service Type from LoadBalancer to ClusterIP or NodePort RBS resources were not cleaned up --- pkg/l4lb/l4netlbcontroller.go | 4 -- pkg/l4lb/l4netlbcontroller_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index c5e710b7e2..4af9755e44 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -238,10 +238,6 @@ func (lc *L4NetLBController) hasForwardingRuleAnnotation(svc *v1.Service, frName // isRBSBasedService checks if service has either RBS annotation, finalizer or RBSForwardingRule func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool { - if !utils.IsLoadBalancerServiceType(svc) { - return false - } - return lc.hasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc) } diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index da9f7e2491..faf449d80d 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -516,6 +516,65 @@ func TestProcessServiceDeletion(t *testing.T) { deleteNetLBService(lc, svc) } +func TestProcessRBSServiceTypeTransition(t *testing.T) { + testCases := []struct { + desc string + finalType v1.ServiceType + }{ + { + desc: "Change RBS to ClusterIP should delete RBS resources", + finalType: v1.ServiceTypeClusterIP, + }, + { + desc: "Change RBS to NodePort should delete RBS resources", + finalType: v1.ServiceTypeNodePort, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + svc, lc := createAndSyncNetLBSvc(t) + if lc.needsDeletion(svc) { + t.Errorf("Service should not be marked for deletion") + } + + svc.Spec.Type = tc.finalType + updateNetLBService(lc, svc) + if !lc.needsDeletion(svc) { + t.Errorf("RBS after switching to %v should be marked for deletion", tc.finalType) + } + + key, _ := common.KeyFunc(svc) + err := lc.sync(key) + if err != nil { + t.Errorf("Failed to sync service %s, err %v", svc.Name, err) + } + svc, err = lc.ctx.KubeClient.CoreV1().Services(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to lookup service %s, err %v", svc.Name, err) + } + if len(svc.Status.LoadBalancer.Ingress) > 0 { + t.Errorf("Expected LoadBalancer status be deleted - %+v", svc.Status.LoadBalancer) + } + if common.HasGivenFinalizer(svc.ObjectMeta, common.NetLBFinalizerV2) { + t.Errorf("Unexpected LoadBalancer finalizer %v", svc.ObjectMeta.Finalizers) + } + + if err = validateAnnotationsDeleted(svc); err != nil { + t.Errorf("RBS Service annotations have NOT been deleted. Error: %v", err) + } + + igName := lc.namer.InstanceGroup() + _, err = lc.ctx.Cloud.GetInstanceGroup(igName, testGCEZone) + if !utils.IsNotFoundError(err) { + t.Errorf("Failed to delete Instance Group %v, err: %v", igName, err) + } + + deleteNetLBService(lc, svc) + }) + } +} + func TestServiceDeletionWhenInstanceGroupInUse(t *testing.T) { svc, lc := createAndSyncNetLBSvc(t) From d58eab80d568d73891bd98df25dcb11369731886 Mon Sep 17 00:00:00 2001 From: Cezary Zawadka Date: Thu, 8 Dec 2022 17:50:32 +0100 Subject: [PATCH 2/2] NetLB RBS Controller: Change needsDeletion and shouldProcessService logic 1) only process if old or new version of the service was/is RBS 2) only delete if service has RBS annotation/finalizer/forwarding rule, even if the type changed from LoadBalacer to other 3) added 2 test cases for service type change (external name, empty value) --- pkg/l4lb/l4netlbcontroller.go | 11 +++++++++-- pkg/l4lb/l4netlbcontroller_test.go | 12 ++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index 4af9755e44..1b047d60a0 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -128,7 +128,9 @@ func (lc *L4NetLBController) needsAddition(newSvc, oldSvc *v1.Service) bool { // needsDeletion return true if svc required deleting RBS based NetLB func (lc *L4NetLBController) needsDeletion(svc *v1.Service) bool { - if !lc.isRBSBasedService(svc) { + // Check if service has RBS related fields/resources + // this check differs from isRBSBasedService() func by checking also non load balancer type services + if !(lc.hasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)) { return false } if svc.ObjectMeta.DeletionTimestamp != nil { @@ -216,7 +218,7 @@ func (lc *L4NetLBController) needsUpdate(newSvc, oldSvc *v1.Service) bool { // shouldProcessService checks if given service should be process by controller func (lc *L4NetLBController) shouldProcessService(newSvc, oldSvc *v1.Service) bool { - if !lc.isRBSBasedService(newSvc) { + if !(lc.isRBSBasedService(newSvc) || lc.isRBSBasedService(oldSvc)) { return false } if lc.needsAddition(newSvc, oldSvc) || lc.needsUpdate(newSvc, oldSvc) || lc.needsDeletion(newSvc) { @@ -238,6 +240,11 @@ func (lc *L4NetLBController) hasForwardingRuleAnnotation(svc *v1.Service, frName // isRBSBasedService checks if service has either RBS annotation, finalizer or RBSForwardingRule func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool { + // Check if the type=LoadBalancer, so we don't execute API calls o non-LB services + // this call is nil-safe + if !utils.IsLoadBalancerServiceType(svc) { + return false + } return lc.hasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc) } diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index faf449d80d..d17cba7719 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -522,13 +522,21 @@ func TestProcessRBSServiceTypeTransition(t *testing.T) { finalType v1.ServiceType }{ { - desc: "Change RBS to ClusterIP should delete RBS resources", + desc: "Change from RBS to ClusterIP should delete RBS resources", finalType: v1.ServiceTypeClusterIP, }, { - desc: "Change RBS to NodePort should delete RBS resources", + desc: "Change from RBS to NodePort should delete RBS resources", finalType: v1.ServiceTypeNodePort, }, + { + desc: "Change from RBS to ExternalName should delete RBS resources", + finalType: v1.ServiceTypeExternalName, + }, + { + desc: "Change from RBS to empty (default) type should delete RBS resources", + finalType: "", + }, } for _, tc := range testCases {