Merge pull request #1740 from panslava/prevent-target-pool-to-rbs-mig…

…ration

Prevent migration to RBS for legacy Target pool services. Remove RBS annotation for such services

pkg/l4lb/l4netlbcontroller.go

@@ -232,6 +232,65 @@ func (lc *L4NetLBController) hasForwardingRuleAnnotation(svc *v1.Service, frName
 	return false
 }
 
+// isRBSBasedService checks if service has either RBS annotation, finalizer or RBSForwardingRule
+func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool {
+	if svc == nil {
+		return false
+	}
+
+	return lc.hasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)
+}
+
+func (lc *L4NetLBController) hasRBSAnnotation(service *v1.Service) bool {
+	if service == nil {
+		return false
+	}
+
+	if val, ok := service.Annotations[annotations.RBSAnnotationKey]; ok && val == annotations.RBSEnabled {
+		return true
+	}
+	return false
+}
+
+// Target Pool to RBS migration is NOT yet supported and causes service to break (for now).
+// If we detect such case, we remove RBS annotation, so service stays with Legacy Target Pool implementation
+func (lc *L4NetLBController) preventTargetPoolToRBSMigration(service *v1.Service) {
+	if lc.hasRBSAnnotation(service) && lc.hasTargetPoolForwardingRule(service) {
+		lc.revertToTargetPool(service)
+	}
+}
+
+func (lc *L4NetLBController) hasTargetPoolForwardingRule(service *v1.Service) bool {
+	l4netlb := loadbalancers.NewL4NetLB(service, lc.ctx.Cloud, meta.Regional, lc.namer, lc.ctx.Recorder(service.Namespace))
+	frName := l4netlb.GetFRName()
+	if lc.hasForwardingRuleAnnotation(service, frName) {
+		return false
+	}
+
+	existingFR := l4netlb.GetForwardingRule(frName, meta.VersionGA)
+	if existingFR != nil && existingFR.Target != "" {
+		return true
+	}
+	return false
+}
+
+func (lc *L4NetLBController) revertToTargetPool(service *v1.Service) {
+	lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "CanNotMigrateTargetPoolToRBS",
+		"RBS annotation was attached to the Legacy Target Pool service. Migration is not supported. Removing annotation")
+	metrics.IncreaseL4NetLBLegacyToRBSMigrationAttempts()
+
+	err := lc.deleteRBSAnnotation(service)
+	if err != nil {
+		lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "RemoveRBSAnnotationOnTargetPoolServiceFailed",
+			"Failed to delete rbs annotation for load balancer with target pool, err: %v", err)
+	}
+}
+
+func (lc *L4NetLBController) deleteRBSAnnotation(service *v1.Service) error {
+	delete(service.Annotations, annotations.RBSAnnotationKey)
+	return updateAnnotations(lc.ctx, service, service.Annotations)
+}
+
 // hasRBSForwardingRule checks if services loadbalancer has forwarding rule pointing to backend service
 func (lc *L4NetLBController) hasRBSForwardingRule(svc *v1.Service) bool {
 	l4netlb := loadbalancers.NewL4NetLB(svc, lc.ctx.Cloud, meta.Regional, lc.namer, lc.ctx.Recorder(svc.Namespace))
@@ -244,17 +303,6 @@ func (lc *L4NetLBController) hasRBSForwardingRule(svc *v1.Service) bool {
 	return existingFR != nil && existingFR.LoadBalancingScheme == string(cloud.SchemeExternal) && existingFR.BackendService != ""
 }
 
-// isRBSBasedService checks if service has either RBS annotation, finalizer or RBSForwardingRule
-func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool {
-	if svc == nil {
-		return false
-	}
-	if val, ok := svc.Annotations[annotations.RBSAnnotationKey]; ok && val == annotations.RBSEnabled {
-		return true
-	}
-	return utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)
-}
-
 func (lc *L4NetLBController) checkHealth() error {
 	lastEnqueueTime := lc.enqueueTracker.Get()
 	lastSyncTime := lc.syncTracker.Get()
@@ -294,6 +342,9 @@ func (lc *L4NetLBController) sync(key string) error {
 		klog.V(3).Infof("Ignoring sync of non-existent service %s", key)
 		return nil
 	}
+
+	lc.preventTargetPoolToRBSMigration(svc)
+
 	if lc.needsDeletion(svc) {
 		klog.V(3).Infof("Deleting L4 External LoadBalancer resources for service %s", key)
 		result := lc.garbageCollectRBSNetLB(key, svc)

pkg/l4lb/l4netlbcontroller_test.go

@@ -452,7 +452,7 @@ func TestProcessServiceCreateWithUsersProvidedIP(t *testing.T) {
 	}
 	adr, err := lc.ctx.Cloud.GetRegionAddress(userAddrName, lc.ctx.Cloud.Region())
 	if err != nil {
-		t.Errorf("Unexpected error when trying to get regiona address, err: %v", err)
+		t.Errorf("Unexpected error when trying to get regional address, err: %v", err)
 	}
 	if adr == nil {
 		t.Errorf("Address should not be deleted after service deletion")
@@ -1124,3 +1124,48 @@ func TestShouldProcessService(t *testing.T) {
 		}
 	}
 }
+
+func TestPreventTargetPoolToRBSMigration(t *testing.T) {
+	testCases := []struct {
+		desc                         string
+		frHook                       getForwardingRuleHook
+		expectRBSAnnotationAfterSync bool
+	}{
+		{
+			desc:                         "Should remove annotation from target pool service",
+			frHook:                       test.GetLegacyForwardingRule,
+			expectRBSAnnotationAfterSync: false,
+		},
+		{
+			desc:                         "Should not remove annotation from RBS based service",
+			frHook:                       test.GetRBSForwardingRule,
+			expectRBSAnnotationAfterSync: true,
+		},
+		{
+			desc:                         "Should not remove annotation from RBS service without forwarding rule",
+			expectRBSAnnotationAfterSync: true,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			svc := test.NewL4NetLBRBSServiceMultiplePorts("test", []int32{30234})
+			controller := newL4NetLBServiceController()
+
+			controller.ctx.Cloud.Compute().(*cloud.MockGCE).MockForwardingRules.GetHook = testCase.frHook
+			addNetLBService(controller, svc)
+
+			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)
+			}
+		})
+	}
+}

pkg/l4lb/metrics/metrics.go

@@ -24,13 +24,14 @@ import (
 )
 
 const (
-	statusSuccess            = "success"
-	statusError              = "error"
-	L4ilbLatencyMetricName   = "l4_ilb_sync_duration_seconds"
-	L4ilbErrorMetricName     = "l4_ilb_sync_error_count"
-	L4netlbLatencyMetricName = "l4_netlb_sync_duration_seconds"
-	L4netlbErrorMetricName   = "l4_netlb_sync_error_count"
-	l4failedHealthCheckName  = "l4_failed_healthcheck_count"
+	statusSuccess                                 = "success"
+	statusError                                   = "error"
+	L4ilbLatencyMetricName                        = "l4_ilb_sync_duration_seconds"
+	L4ilbErrorMetricName                          = "l4_ilb_sync_error_count"
+	L4netlbLatencyMetricName                      = "l4_netlb_sync_duration_seconds"
+	L4netlbErrorMetricName                        = "l4_netlb_sync_error_count"
+	L4netlbLegacyToRBSMigrationAttemptsMetricName = "l4_netlb_legacy_to_rbs_migration_attempts_count"
+	l4failedHealthCheckName                       = "l4_failed_healthcheck_count"
 )
 
 var (
@@ -88,6 +89,12 @@ var (
 		},
 		[]string{"controller_name"},
 	)
+	l4NetLBLegacyToRBSMigrationAttempts = prometheus.NewCounter(
+		prometheus.CounterOpts{
+			Name: L4netlbLegacyToRBSMigrationAttemptsMetricName,
+			Help: "Count of customer attempts to migrate legacy service to RBS by adding rbs annotation",
+		},
+	)
 )
 
 // init registers l4 ilb nad netlb sync metrics.
@@ -133,7 +140,12 @@ func PublishL4NetLBSyncError(syncType, gceResource, errType string, startTime ti
 	l4NetLBSyncErrorCount.WithLabelValues(syncType, gceResource, errType).Inc()
 }
 
-// PublishL4FailedHealthCheckCount observers failed healt check from controller.
+// PublishL4FailedHealthCheckCount observers failed health check from controller.
 func PublishL4FailedHealthCheckCount(controllerName string) {
 	l4FailedHealthCheckCount.WithLabelValues(controllerName).Inc()
 }
+
+// IncreaseL4NetLBLegacyToRBSMigrationAttempts increases l4NetLBLegacyToRBSMigrationAttempts metric
+func IncreaseL4NetLBLegacyToRBSMigrationAttempts() {
+	l4NetLBLegacyToRBSMigrationAttempts.Inc()
+}