Fix preventTargetPoolToRBSMigration. Properly delete annotation

pkg/l4lb/l4lbcommon.go

@@ -73,6 +73,17 @@ func updateAnnotations(ctx *context.ControllerContext, svc *v1.Service, newL4LBA
 	return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta)
 }
 
+func deleteAnnotation(ctx *context.ControllerContext, svc *v1.Service, annotationKey string) error {
+	newObjectMeta := svc.ObjectMeta.DeepCopy()
+	if _, ok := newObjectMeta.Annotations[annotationKey]; !ok {
+		return nil
+	}
+
+	klog.V(3).Infof("Removing annotation %s from service %v/%v", annotationKey, svc.Namespace, svc.Name)
+	delete(newObjectMeta.Annotations, annotationKey)
+	return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta)
+}
+
 // updateServiceStatus this faction checks if LoadBalancer status changed and patch service if needed.
 func updateServiceStatus(ctx *context.ControllerContext, svc *v1.Service, newStatus *v1.LoadBalancerStatus) error {
 	if helpers.LoadBalancerStatusEqual(&svc.Status.LoadBalancer, newStatus) {

pkg/l4lb/l4netlbcontroller.go

@@ -287,8 +287,13 @@ func (lc *L4NetLBController) revertToTargetPool(service *v1.Service) {
 }
 
 func (lc *L4NetLBController) deleteRBSAnnotation(service *v1.Service) error {
+	err := deleteAnnotation(lc.ctx, service, annotations.RBSAnnotationKey)
+	if err != nil {
+		return fmt.Errorf("deleteAnnotation(_, %v, %s) returned error %v, want nil", service, annotations.RBSAnnotationKey, err)
+	}
+	// update current object annotations, so we do not treat it as RBS after
 	delete(service.Annotations, annotations.RBSAnnotationKey)
-	return updateAnnotations(lc.ctx, service, service.Annotations)
+	return nil
 }
 
 // hasRBSForwardingRule checks if services loadbalancer has forwarding rule pointing to backend service

pkg/l4lb/l4netlbcontroller_test.go

@@ -1126,6 +1126,8 @@ func TestShouldProcessService(t *testing.T) {
 }
 
 func TestPreventTargetPoolToRBSMigration(t *testing.T) {
+	flags.F.MaxIgSize = 1000
+
 	testCases := []struct {
 		desc                         string
 		frHook                       getForwardingRuleHook
@@ -1155,16 +1157,39 @@ func TestPreventTargetPoolToRBSMigration(t *testing.T) {
 			controller.ctx.Cloud.Compute().(*cloud.MockGCE).MockForwardingRules.GetHook = testCase.frHook
 			addNetLBService(controller, svc)
 
+			// test only preventTargetPoolToRBSMigration
 			controller.preventTargetPoolToRBSMigration(svc)
-			updateNetLBService(controller, svc)
 
 			resultSvc, err := controller.ctx.KubeClient.CoreV1().Services(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{})
 			if err != nil {
 				t.Fatalf("controller.ctx.KubeClient.CoreV1().Services(%s).Get(_, %s, _) returned error %v, want nil", svc.Namespace, svc.Name, err)
 			}
 			hasRBSAnnotation := controller.hasRBSAnnotation(resultSvc)
 			if hasRBSAnnotation != testCase.expectRBSAnnotationAfterSync {
-				t.Errorf("hasRBSAnnotation = %t, testCase.expectRBSAnnotationAfterSync = %t, want equal", hasRBSAnnotation, testCase.expectRBSAnnotationAfterSync)
+				t.Errorf("After preventTargetPoolToRBSMigration, hasRBSAnnotation = %t, testCase.expectRBSAnnotationAfterSync = %t, want equal", hasRBSAnnotation, testCase.expectRBSAnnotationAfterSync)
+			}
+
+			// test that whole sync process is skipped
+			svc2 := test.NewL4NetLBRBSServiceMultiplePorts("test-2", []int32{30234})
+			addNetLBService(controller, svc2)
+
+			key, err := common.KeyFunc(svc2)
+			if err != nil {
+				t.Fatalf("common.KeyFunc(%v) returned error %v, want nil", svc2, err)
+			}
+
+			err = controller.sync(key)
+			if err != nil {
+				t.Fatalf("controller.sync(%s) returned error %v, want nil", key, err)
+			}
+
+			resultSvc, err = controller.ctx.KubeClient.CoreV1().Services(svc2.Namespace).Get(context.TODO(), svc2.Name, metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("controller.ctx.KubeClient.CoreV1().Services(%s).Get(_, %s, _) returned error %v, want nil", svc2.Namespace, svc2.Name, err)
+			}
+			hasRBSAnnotation = controller.hasRBSAnnotation(resultSvc)
+			if hasRBSAnnotation != testCase.expectRBSAnnotationAfterSync {
+				t.Errorf("After sync, hasRBSAnnotation = %t, testCase.expectRBSAnnotationAfterSync = %t, want equal", hasRBSAnnotation, testCase.expectRBSAnnotationAfterSync)
 			}
 		})
 	}

pkg/test/gcehooks.go

@@ -97,12 +97,12 @@ func DeleteHealthCheckResourceInUseErrorHook(ctx context.Context, key *meta.Key,
 }
 
 func GetLegacyForwardingRule(ctx context.Context, key *meta.Key, m *cloud.MockForwardingRules) (bool, *compute.ForwardingRule, error) {
-	fwRule := compute.ForwardingRule{Target: "some_target", LoadBalancingScheme: string(cloud.SchemeExternal)}
+	fwRule := compute.ForwardingRule{Target: "some_target", LoadBalancingScheme: string(cloud.SchemeExternal), NetworkTier: cloud.NetworkTierDefault.ToGCEValue()}
 	return true, &fwRule, nil
 }
 
 func GetRBSForwardingRule(ctx context.Context, key *meta.Key, m *cloud.MockForwardingRules) (bool, *compute.ForwardingRule, error) {
-	fwRule := compute.ForwardingRule{BackendService: "some_rbs", LoadBalancingScheme: string(cloud.SchemeExternal)}
+	fwRule := compute.ForwardingRule{BackendService: bsUrl, LoadBalancingScheme: string(cloud.SchemeExternal), NetworkTier: cloud.NetworkTierDefault.ToGCEValue()}
 	return true, &fwRule, nil
 }