Helpers removed

pkg/healthchecksl4/healthchecksl4.go

@@ -24,10 +24,10 @@ import (
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/cloud-provider-gcp/providers/gce"
-	"k8s.io/cloud-provider/service/helpers"
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/composite"
 	"k8s.io/ingress-gce/pkg/firewalls"
@@ -117,11 +117,40 @@ func (l4hc *l4HealthChecks) EnsureHealthCheckWithFirewall(svc *corev1.Service, n
 	return l4hc.EnsureHealthCheckWithDualStackFirewalls(svc, namer, sharedHC, scope, l4Type, nodeNames /*create IPv4*/, true /*don't create IPv6*/, false, svcNetwork, svcLogger)
 }
 
+// GetServiceHealthCheckPathPort returns the path and nodePort programmed into the Cloud LB Health Check
+func getServiceHealthCheckPathPort(service *v1.Service) (string, int32) {
+	if !needsHealthCheck(service) {
+		return "", 0
+	}
+	port := service.Spec.HealthCheckNodePort
+	if port == 0 {
+		return "", 0
+	}
+	return "/healthz", port
+}
+
+// NeedsHealthCheck checks if service needs health check.
+func needsHealthCheck(service *v1.Service) bool {
+	if service.Spec.Type != v1.ServiceTypeLoadBalancer {
+		return false
+	}
+	return requestsOnlyLocalTraffic(service)
+}
+
+// RequestsOnlyLocalTraffic checks if service requests OnlyLocal traffic.
+func requestsOnlyLocalTraffic(service *v1.Service) bool {
+	if service.Spec.Type != v1.ServiceTypeLoadBalancer &&
+		service.Spec.Type != v1.ServiceTypeNodePort {
+		return false
+	}
+	return service.Spec.ExternalTrafficPolicy == v1.ServiceExternalTrafficPolicyLocal
+}
+
 func (l4hc *l4HealthChecks) EnsureHealthCheckWithDualStackFirewalls(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, nodeNames []string, needsIPv4 bool, needsIPv6 bool, svcNetwork network.NetworkInfo, svcLogger klog.Logger) *EnsureHealthCheckResult {
 	namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}
 
 	hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC)
-	hcPath, hcPort := helpers.GetServiceHealthCheckPathPort(svc)
+	hcPath, hcPort := getServiceHealthCheckPathPort(svc)
 	hcLogger := svcLogger.WithValues("healthcheckName", hcName)
 	hcLogger.V(3).Info("Ensuring L4 healthcheck with firewalls for service", "shared", sharedHC)
 

pkg/l4lb/l4controller.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"math/rand"
 	"reflect"
-	"strings"
 	"sync"
 	"time"
 
@@ -297,43 +296,16 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(service *v1.Service, svcLo
 		syncResult.Error = err
 		return syncResult
 	}
-	err = updateServiceStatus(l4c.ctx, service, syncResult.Status, svcLogger)
+	err = updateServiceInformation(l4c.ctx, l4c.enableDualStack, service, syncResult.Status, syncResult.Annotations, svcLogger)
 	if err != nil {
 		l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncLoadBalancerFailed",
 			"Error updating load balancer status: %v", err)
 		syncResult.Error = err
 		return syncResult
 	}
-	if l4c.enableDualStack {
-		l4c.emitEnsuredDualStackEvent(service)
-		if err = updateL4DualStackResourcesAnnotations(l4c.ctx, service, syncResult.Annotations, svcLogger); err != nil {
-			l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncLoadBalancerFailed",
-				"Failed to update Dual Stack annotations for load balancer, err: %v", err)
-			syncResult.Error = fmt.Errorf("failed to set Dual Stack resource annotations, err: %w", err)
-			return syncResult
-		}
-	} else {
-		l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful",
-			"Successfully ensured load balancer resources")
-		if err = updateL4ResourcesAnnotations(l4c.ctx, service, syncResult.Annotations, svcLogger); err != nil {
-			l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncLoadBalancerFailed",
-				"Failed to update annotations for load balancer, err: %v", err)
-			syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err)
-			return syncResult
-		}
-	}
 	return syncResult
 }
 
-func (l4c *L4Controller) emitEnsuredDualStackEvent(service *v1.Service) {
-	var ipFamilies []string
-	for _, ipFamily := range service.Spec.IPFamilies {
-		ipFamilies = append(ipFamilies, string(ipFamily))
-	}
-	l4c.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful",
-		"Successfully ensured %v load balancer resources", strings.Join(ipFamilies, " "))
-}
-
 func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service, svcLogger klog.Logger) *loadbalancers.L4ILBSyncResult {
 	startTime := time.Now()
 	svcLogger.Info("Deleting L4 ILB service")
@@ -359,28 +331,12 @@ func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service, svc
 	// Reset the loadbalancer status first, before resetting annotations.
 	// Other controllers(like service-controller) will process the service update if annotations change, but will ignore a service status change.
 	// Following this order avoids a race condition when a service is changed from LoadBalancer type Internal to External.
-	if err := updateServiceStatus(l4c.ctx, svc, &v1.LoadBalancerStatus{}, svcLogger); err != nil {
+	if err := updateServiceInformation(l4c.ctx, l4c.enableDualStack, svc, &v1.LoadBalancerStatus{}, nil, svcLogger); err != nil {
 		l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
 			"Error resetting load balancer status to empty: %v", err)
 		result.Error = fmt.Errorf("failed to reset ILB status, err: %w", err)
 		return result
 	}
-	// Also remove any ILB annotations from the service metadata
-	if l4c.enableDualStack {
-		if err := updateL4DualStackResourcesAnnotations(l4c.ctx, svc, nil, svcLogger); err != nil {
-			l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
-				"Error resetting DualStack resource annotations for load balancer: %v", err)
-			result.Error = fmt.Errorf("failed to reset DualStack resource annotations, err: %w", err)
-			return result
-		}
-	} else {
-		if err := updateL4ResourcesAnnotations(l4c.ctx, svc, nil, svcLogger); err != nil {
-			l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
-				"Error resetting resource annotations for load balancer: %v", err)
-			result.Error = fmt.Errorf("failed to reset resource annotations, err: %w", err)
-			return result
-		}
-	}
 
 	if err := common.EnsureDeleteServiceFinalizer(svc, common.ILBFinalizerV2, l4c.ctx.KubeClient, svcLogger); err != nil {
 		l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancerFailed",

pkg/l4lb/l4lbcommon.go

@@ -18,13 +18,12 @@ package l4lb
 
 import (
 	"fmt"
-	"reflect"
+	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/cloud-provider-gcp/providers/gce"
-	"k8s.io/cloud-provider/service/helpers"
 	"k8s.io/ingress-gce/pkg/composite"
 	"k8s.io/ingress-gce/pkg/context"
 	l4metrics "k8s.io/ingress-gce/pkg/l4lb/metrics"
@@ -41,9 +40,6 @@ import (
 func computeNewAnnotationsIfNeeded(svc *v1.Service, newAnnotations map[string]string, keysToRemove []string) *metav1.ObjectMeta {
 	newObjectMeta := svc.ObjectMeta.DeepCopy()
 	newObjectMeta.Annotations = mergeAnnotations(newObjectMeta.Annotations, newAnnotations, keysToRemove)
-	if reflect.DeepEqual(svc.Annotations, newObjectMeta.Annotations) {
-		return nil
-	}
 	return newObjectMeta
 }
 
@@ -66,46 +62,74 @@ func mergeAnnotations(existing, lbAnnotations map[string]string, keysToRemove []
 	return existing
 }
 
-// updateL4ResourcesAnnotations checks if new annotations should be added to service and patch service metadata if needed.
-func updateL4ResourcesAnnotations(ctx *context.ControllerContext, svc *v1.Service, newL4LBAnnotations map[string]string, svcLogger klog.Logger) error {
-	svcLogger.V(3).Info("Updating annotations of service")
-	newObjectMeta := computeNewAnnotationsIfNeeded(svc, newL4LBAnnotations, loadbalancers.L4ResourceAnnotationKeys)
-	if newObjectMeta == nil {
-		svcLogger.V(3).Info("Service annotations not changed, skipping patch for service")
+func deleteAnnotation(ctx *context.ControllerContext, svc *v1.Service, annotationKey string, svcLogger klog.Logger) error {
+	newObjectMeta := svc.ObjectMeta.DeepCopy()
+	if _, ok := newObjectMeta.Annotations[annotationKey]; !ok {
 		return nil
 	}
-	svcLogger.V(3).Info("Patching annotations of service")
+	svcLogger.V(3).Info("Removing annotation from service", "annotationKey", annotationKey)
+	delete(newObjectMeta.Annotations, annotationKey)
 	return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta)
 }
 
-// updateL4DualStackResourcesAnnotations checks if new annotations should be added to dual-stack service and patch service metadata if needed.
-func updateL4DualStackResourcesAnnotations(ctx *context.ControllerContext, svc *v1.Service, newL4LBAnnotations map[string]string, svcLogger klog.Logger) error {
-	newObjectMeta := computeNewAnnotationsIfNeeded(svc, newL4LBAnnotations, loadbalancers.L4DualStackResourceAnnotationKeys)
-	if newObjectMeta == nil {
-		return nil
+// LoadBalancerStatusEqual checks if load balancer status are equal
+func loadBalancerStatusEqual(l, r *v1.LoadBalancerStatus) bool {
+	return ingressSliceEqual(l.Ingress, r.Ingress)
+}
+
+func ingressSliceEqual(lhs, rhs []v1.LoadBalancerIngress) bool {
+	if len(lhs) != len(rhs) {
+		return false
 	}
-	svcLogger.V(3).Info("Patching annotations of service")
-	return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta)
+	for i := range lhs {
+		if !ingressEqual(&lhs[i], &rhs[i]) {
+			return false
+		}
+	}
+	return true
 }
 
-func deleteAnnotation(ctx *context.ControllerContext, svc *v1.Service, annotationKey string, svcLogger klog.Logger) error {
-	newObjectMeta := svc.ObjectMeta.DeepCopy()
-	if _, ok := newObjectMeta.Annotations[annotationKey]; !ok {
-		return nil
+func ingressEqual(lhs, rhs *v1.LoadBalancerIngress) bool {
+	if lhs.IP != rhs.IP {
+		return false
 	}
-	svcLogger.V(3).Info("Removing annotation from service", "annotationKey", annotationKey)
-	delete(newObjectMeta.Annotations, annotationKey)
-	return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta)
+	if lhs.Hostname != rhs.Hostname {
+		return false
+	}
+	return true
 }
 
-// updateServiceStatus this faction checks if LoadBalancer status changed and patch service if needed.
-func updateServiceStatus(ctx *context.ControllerContext, svc *v1.Service, newStatus *v1.LoadBalancerStatus, svcLogger klog.Logger) error {
+// updateServiceInformation this faction checks if LoadBalancer changed and patch service if needed.
+func updateServiceInformation(ctx *context.ControllerContext, enableDualStack bool, svc *v1.Service, newStatus *v1.LoadBalancerStatus, newL4LBAnnotations map[string]string, svcLogger klog.Logger) error {
 	svcLogger.V(2).Info("Updating service status", "newStatus", fmt.Sprintf("%+v", newStatus))
-	if helpers.LoadBalancerStatusEqual(&svc.Status.LoadBalancer, newStatus) {
+	if loadBalancerStatusEqual(&svc.Status.LoadBalancer, newStatus) {
 		svcLogger.V(2).Info("New and old statuses are equal, skipping patch")
 		return nil
 	}
-	return patch.PatchServiceLoadBalancerStatus(ctx.KubeClient.CoreV1(), svc, *newStatus)
+
+	var keysToRemove []string
+	if enableDualStack {
+		emitEnsuredDualStackEvent(ctx, svc)
+		keysToRemove = loadbalancers.L4DualStackResourceAnnotationKeys
+	} else {
+		keysToRemove = loadbalancers.L4ResourceAnnotationKeys
+	}
+	svcLogger.V(2).Info("Selected keysToRemove", "keysToRemove", keysToRemove)
+
+	newObjectMeta := computeNewAnnotationsIfNeeded(svc, newL4LBAnnotations, keysToRemove)
+	svcLogger.V(2).Info("Computed new service metadata", "newObjectMeta", newObjectMeta)
+
+	return patch.PatchServiceLoadBalancerInformation(ctx.KubeClient.CoreV1(), svc, *newStatus, *newObjectMeta)
+
+}
+
+func emitEnsuredDualStackEvent(ctx *context.ControllerContext, service *v1.Service) {
+	var ipFamilies []string
+	for _, ipFamily := range service.Spec.IPFamilies {
+		ipFamilies = append(ipFamilies, string(ipFamily))
+	}
+	ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful",
+		"Successfully ensured %v load balancer resources", strings.Join(ipFamilies, " "))
 }
 
 // isHealthCheckDeleted checks if given health check exists in GCE

pkg/l4lb/l4netlbcontroller.go

@@ -550,33 +550,13 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service, svcLogger klog.Lo
 		return syncResult
 	}
 
-	err = updateServiceStatus(lc.ctx, service, syncResult.Status, svcLogger)
+	err = updateServiceInformation(lc.ctx, lc.enableDualStack, service, syncResult.Status, syncResult.Annotations, svcLogger)
 	if err != nil {
 		lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed",
 			"Error updating L4 External LoadBalancer, err: %v", err)
 		syncResult.Error = err
 		return syncResult
 	}
-	if lc.enableDualStack {
-		lc.emitEnsuredDualStackEvent(service)
-
-		if err = updateL4DualStackResourcesAnnotations(lc.ctx, service, syncResult.Annotations, svcLogger); err != nil {
-			lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed",
-				"Failed to update annotations for load balancer, err: %v", err)
-			syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err)
-			return syncResult
-		}
-	} else {
-		lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful",
-			"Successfully ensured L4 External LoadBalancer resources")
-
-		if err = updateL4ResourcesAnnotations(lc.ctx, service, syncResult.Annotations, svcLogger); err != nil {
-			lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed",
-				"Failed to update annotations for load balancer, err: %v", err)
-			syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err)
-			return syncResult
-		}
-	}
 	syncResult.SetMetricsForSuccessfulServiceSync()
 	return syncResult
 }
@@ -675,7 +655,7 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service,
 		return result
 	}
 
-	if err := updateServiceStatus(lc.ctx, svc, &v1.LoadBalancerStatus{}, svcLogger); err != nil {
+	if err := updateServiceInformation(lc.ctx, lc.enableDualStack, svc, &v1.LoadBalancerStatus{}, nil, svcLogger); err != nil {
 		lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
 			"Error resetting L4 External LoadBalancer status to empty, err: %v", err)
 		result.Error = fmt.Errorf("Failed to reset L4 External LoadBalancer status, err: %w", err)
@@ -691,22 +671,6 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service,
 		return result
 	}
 
-	if lc.enableDualStack {
-		if err := updateL4DualStackResourcesAnnotations(lc.ctx, svc, nil, svcLogger); err != nil {
-			lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
-				"Error removing Dual Stack resource annotations: %v", err)
-			result.Error = fmt.Errorf("failed to reset Dual Stack resource annotations, err: %w", err)
-			return result
-		}
-	} else {
-		if err := updateL4ResourcesAnnotations(lc.ctx, svc, nil, svcLogger); err != nil {
-			lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer",
-				"Error removing resource annotations: %v", err)
-			result.Error = fmt.Errorf("failed to reset resource annotations, err: %w", err)
-			return result
-		}
-	}
-
 	// Finalizer needs to be removed last, because after deleting finalizer service can be deleted and
 	// updating annotations or other manipulations will fail
 	if err := common.EnsureDeleteServiceFinalizer(svc, common.NetLBFinalizerV2, lc.ctx.KubeClient, svcLogger); err != nil {

pkg/l4lb/l4netlbcontroller_test.go

@@ -43,7 +43,6 @@ import (
 	networkv1 "k8s.io/cloud-provider-gcp/crd/apis/network/v1"
 	netfake "k8s.io/cloud-provider-gcp/crd/client/network/clientset/versioned/fake"
 	"k8s.io/cloud-provider-gcp/providers/gce"
-	"k8s.io/cloud-provider/service/helpers"
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
 	"k8s.io/ingress-gce/pkg/composite"
@@ -60,12 +59,13 @@ import (
 )
 
 const (
-	FwIPAddress          = "10.0.0.1"
-	loadBalancerIP       = "10.0.0.10"
-	usersIP              = "35.10.211.60"
-	testServiceNamespace = "default"
-	hcNodePort           = int32(10111)
-	userAddrName         = "UserStaticAddress"
+	FwIPAddress                  = "10.0.0.1"
+	loadBalancerIP               = "10.0.0.10"
+	usersIP                      = "35.10.211.60"
+	testServiceNamespace         = "default"
+	hcNodePort                   = int32(10111)
+	userAddrName                 = "UserStaticAddress"
+	loadBalancerCleanupFinalizer = "service.kubernetes.io/load-balancer-cleanup"
 
 	shortSessionAffinityIdleTimeout = int32(20)     // 20 sec could be used for regular Session Affinity
 	longSessionAffinityIdleTimeout  = int32(2 * 60) // 2 min or 120 sec for Strong Session Affinity
@@ -1151,7 +1151,7 @@ func TestIsRBSBasedService(t *testing.T) {
 		},
 		{
 			desc:             "Legacy service should not be marked as RBS",
-			finalizers:       []string{helpers.LoadBalancerCleanupFinalizer},
+			finalizers:       []string{loadBalancerCleanupFinalizer},
 			expectRBSService: false,
 		},
 		{
@@ -1161,7 +1161,7 @@ func TestIsRBSBasedService(t *testing.T) {
 		},
 		{
 			desc:             "Should detect RBS by finalizer when service contains both legacy and NetLB finalizers",
-			finalizers:       []string{helpers.LoadBalancerCleanupFinalizer, common.NetLBFinalizerV2},
+			finalizers:       []string{loadBalancerCleanupFinalizer, common.NetLBFinalizerV2},
 			expectRBSService: true,
 		},
 		{