Merge pull request #1881 from cezarygerard/cgz-fix-service-type-change

NetLB controller: cleanup RBS resources in case service type changed

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,10 +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)
 }
 

pkg/l4lb/l4netlbcontroller_test.go

@@ -516,6 +516,73 @@ func TestProcessServiceDeletion(t *testing.T) {
 	deleteNetLBService(lc, svc)
 }
 
+func TestProcessRBSServiceTypeTransition(t *testing.T) {
+	testCases := []struct {
+		desc      string
+		finalType v1.ServiceType
+	}{
+		{
+			desc:      "Change from RBS to ClusterIP should delete RBS resources",
+			finalType: v1.ServiceTypeClusterIP,
+		},
+		{
+			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 {
+		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)