From 7fcc292495b1daade70779c209a5f925644f3397 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Tue, 17 May 2022 11:16:34 +0000 Subject: [PATCH] Implement migration from L4 Legacy Target Pool based services to L4 RBS - Switch target of existing forwarding rule from target pool to regional backend service - Clean up GCP resources used only by legacy service: target pool, health checks, firewall rules for health checks --- pkg/composite/composite.go | 2 +- pkg/flags/flags.go | 4 +- pkg/l4lb/l4netlbcontroller.go | 30 ++++++++++-- pkg/loadbalancers/forwarding_rules.go | 41 +++++++++++----- pkg/loadbalancers/l4netlb.go | 69 ++++++++++++++++++++++++++- pkg/utils/common/finalizer.go | 3 ++ pkg/utils/namer/interfaces.go | 2 + pkg/utils/namer/l4_namer.go | 5 ++ pkg/utils/utils.go | 12 +++++ 9 files changed, 147 insertions(+), 21 deletions(-) diff --git a/pkg/composite/composite.go b/pkg/composite/composite.go index b7cccf8bd8..ce0b8d4fda 100644 --- a/pkg/composite/composite.go +++ b/pkg/composite/composite.go @@ -185,7 +185,7 @@ func SetUrlMapForTargetHttpProxy(gceCloud *gce.Cloud, key *meta.Key, targetHttpP } } -// SetProxyForForwardingRule() sets the target proxy for a forwarding rule +// SetProxyForForwardingRule sets the target proxy for a forwarding rule func SetProxyForForwardingRule(gceCloud *gce.Cloud, key *meta.Key, forwardingRule *ForwardingRule, targetProxyLink string) error { ctx, cancel := cloud.ContextWithCallTimeout() defer cancel() diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 24f6709760..bee0968cc4 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -202,7 +202,7 @@ values.`) the pod secrets for creating a Kubernetes client.`) flag.StringVar(&F.KubeConfigFile, "kubeconfig", "", `Path to kubeconfig file with authorization and master location information.`) - flag.DurationVar(&F.ResyncPeriod, "sync-period", 30*time.Second, + flag.DurationVar(&F.ResyncPeriod, "sync-period", 10*time.Minute, `Relist and confirm cloud resources this often.`) flag.IntVar(&F.NumL4Workers, "num-l4-workers", 5, `Number of parallel L4 Service worker goroutines.`) @@ -246,7 +246,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 flag.BoolVar(&F.EnableEndpointSlices, "enable-endpoint-slices", false, "Enable using Endpoint Slices API instead of Endpoints API") flag.BoolVar(&F.EnableMultipleIgs, "enable-multiple-igs", false, "Enable using unmanaged instance group management") flag.IntVar(&F.MaxIgSize, "max-ig-size", 1000, "Max number of instances in Instance Group") - flag.DurationVar(&F.MetricsExportInterval, "metrics-export-interval", 10*time.Minute, `Period for calculating and exporting metrics related to state of managed objects.`) + flag.DurationVar(&F.MetricsExportInterval, "metrics-export-interval", 30*time.Second, `Period for calculating and exporting metrics related to state of managed objects.`) } type RateLimitSpecs struct { diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index 6b55266892..5e6aac0d6a 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -251,10 +251,7 @@ 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) + return utils.HasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc) } func (lc *L4NetLBController) checkHealth() error { @@ -333,6 +330,16 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach L4 External LoadBalancer finalizer to service %s/%s, err %w", service.Namespace, service.Name, err)} } + needsLegacyL4Cleanup := utils.HasNeedsLegacyL4CleanupFinalizer(service) + frName := l4netlb.GetFRName() + existingForwardingRule := l4netlb.GetForwardingRule(frName, meta.VersionGA) + if existingForwardingRule != nil && existingForwardingRule.Target != "" { + if err := common.EnsureServiceFinalizer(service, common.NeedsLegacyL4CleanupFinalizer, lc.ctx.KubeClient); err != nil { + return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach NeedsLegacyL4CleanupFinalizer to service %s/%s, err %w", service.Namespace, service.Name, err)} + } + needsLegacyL4Cleanup = true + } + nodeNames, err := utils.GetReadyNodeNames(lc.nodeLister) if err != nil { return &loadbalancers.L4NetLBSyncResult{Error: err} @@ -346,7 +353,7 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 // Use the same function for both create and updates. If controller crashes and restarts, // all existing services will show up as Service Adds. - syncResult := l4netlb.EnsureFrontend(nodeNames, service) + syncResult := l4netlb.EnsureFrontendWithExistingRule(nodeNames, service, existingForwardingRule) if syncResult.Error != nil { lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed", "Error ensuring Resource for L4 External LoadBalancer, err: %v", syncResult.Error) @@ -375,6 +382,19 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err) return syncResult } + + if needsLegacyL4Cleanup { + err = l4netlb.CleanLegacyServiceResources() + if err != nil { + lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "CleanLegacyServiceResourcesFailed", + "Error deleting Legacy L4 External LoadBalancer resources, err: %v", err) + } else { + if err = common.EnsureDeleteServiceFinalizer(service, common.NeedsLegacyL4CleanupFinalizer, lc.ctx.KubeClient); err != nil { + return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to delete NeedsLegacyL4CleanupFinalizer from service %s/%s, err %w", service.Namespace, service.Name, err)} + } + } + } + syncResult.MetricsState.InSuccess = true return syncResult } diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 93e342e1f9..4b3394db08 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -323,7 +323,7 @@ func (l *L4) deleteForwardingRule(name string, version meta.Version) { // ensureExternalForwardingRule creates a forwarding rule with the given name for L4NetLB, // if it does not exist. It updates the existing forwarding rule if needed. -func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite.ForwardingRule, IPAddressType, error) { +func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string, existingFwdRule *composite.ForwardingRule) (*composite.ForwardingRule, IPAddressType, error) { frName := l4netlb.GetFRName() key, err := l4netlb.createKey(frName) if err != nil { @@ -331,7 +331,6 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. } // version used for creating the existing forwarding rule. version := meta.VersionGA - existingFwdRule := l4netlb.GetForwardingRule(frName, version) // Determine IP which will be used for this LB. If no forwarding rule has been established // or specified in the Service spec, then requestedIP = "". @@ -393,6 +392,21 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. networkTierMismatchError := utils.NewNetworkTierErr(resource, existingFwdRule.NetworkTier, fr.NetworkTier) return nil, IPAddrUndefined, networkTierMismatchError } + + // Check if we transition from Target Pool Legacy Service to RBS based one. + // If configs are not equal, we should delete forwarding rule and recreate. + if existingFwdRule.Target != "" && ConfigsEqual(fr, existingFwdRule) { + // If transitioning from Legacy L4 Net LB Service to RBS, + // instead of forwarding rule recreation, call set-target + // on forwarding rule, to minimize down-time + err = composite.SetProxyForForwardingRule(l4netlb.cloud, key, existingFwdRule, bsLink) + if err != nil { + return nil, IPAddrUndefined, err + } + createdFr, err := composite.GetForwardingRule(l4netlb.cloud, key, fr.Version) + return createdFr, isIPManaged, err + } + equal, err := Equal(existingFwdRule, fr) if err != nil { return existingFwdRule, IPAddrUndefined, err @@ -452,6 +466,19 @@ func (l4netlb *L4NetLB) deleteForwardingRule(name string, version meta.Version) } } +// ConfigsEqual check if two forwarding rules are equal in configurations, except for backend targets +func ConfigsEqual(fr1, fr2 *composite.ForwardingRule) bool { + return fr1.IPAddress == fr2.IPAddress && + fr1.IPProtocol == fr2.IPProtocol && + fr1.LoadBalancingScheme == fr2.LoadBalancingScheme && + utils.EqualStringSets(fr1.Ports, fr2.Ports) && + fr1.AllowGlobalAccess == fr2.AllowGlobalAccess && + fr1.AllPorts == fr2.AllPorts && + fr1.Subnetwork == fr2.Subnetwork && + fr1.NetworkTier == fr2.NetworkTier +} + +// Equal checks if two forwarding rules are equal both in configs and in backend targets func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) { id1, err := cloud.ParseResourceURL(fr1.BackendService) if err != nil { @@ -461,15 +488,7 @@ func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) { if err != nil { return false, fmt.Errorf("forwardingRulesEqual(): failed to parse resource URL from FR, err - %w", err) } - return fr1.IPAddress == fr2.IPAddress && - fr1.IPProtocol == fr2.IPProtocol && - fr1.LoadBalancingScheme == fr2.LoadBalancingScheme && - utils.EqualStringSets(fr1.Ports, fr2.Ports) && - id1.Equal(id2) && - fr1.AllowGlobalAccess == fr2.AllowGlobalAccess && - fr1.AllPorts == fr2.AllPorts && - fr1.Subnetwork == fr2.Subnetwork && - fr1.NetworkTier == fr2.NetworkTier, nil + return ConfigsEqual(fr1, fr2) && id1.Equal(id2), nil } // l4lbIPToUse determines which IP address needs to be used in the ForwardingRule. If an IP has been diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index d55ecccab0..4171147b62 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -26,7 +26,9 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" + cloudprovider "k8s.io/cloud-provider" "k8s.io/cloud-provider/service/helpers" "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/backends" @@ -96,7 +98,7 @@ func (l4netlb *L4NetLB) createKey(name string) (*meta.Key, error) { // been created. It is health check, firewall rules, backend service and forwarding rule. // It returns a LoadBalancerStatus with the updated ForwardingRule IP address. // This function does not link instances to Backend Service. -func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) *L4NetLBSyncResult { +func (l4netlb *L4NetLB) EnsureFrontendWithExistingRule(nodeNames []string, svc *corev1.Service, existingForwardingRule *composite.ForwardingRule) *L4NetLBSyncResult { result := &L4NetLBSyncResult{ Annotations: make(map[string]string), StartTime: time.Now(), @@ -140,7 +142,7 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) return result } result.Annotations[annotations.BackendServiceKey] = name - fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink) + fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink, existingForwardingRule) if err != nil { // User can misconfigure the forwarding rule if Network Tier will not match service level Network Tier. result.MetricsState.IsUserError = utils.IsUserError(err) @@ -159,6 +161,10 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) return result } +func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) *L4NetLBSyncResult { + return l4netlb.EnsureFrontendWithExistingRule(nodeNames, svc, nil) +} + // EnsureLoadBalancerDeleted performs a cleanup of all GCE resources for the given loadbalancer service. // It is health check, firewall rules and backend service func (l4netlb *L4NetLB) EnsureLoadBalancerDeleted(svc *corev1.Service) *L4NetLBSyncResult { @@ -295,3 +301,62 @@ func (l4netlb *L4NetLB) createFirewalls(name, hcLink, hcFwName string, hcPort in } return string(protocol), result } + +// CleanLegacyServiceResources deletes resources, used only by legacy L4 NetLB, based on Target Pools and implemented in service controller +func (l4netlb *L4NetLB) CleanLegacyServiceResources() error { + loadBalancerName := cloudprovider.DefaultLoadBalancerName(l4netlb.Service) + serviceName := types.NamespacedName{Namespace: l4netlb.Service.Namespace, Name: l4netlb.Service.Name} + lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName) + + errs := utilerrors.AggregateGoroutines( + func() error { + klog.Infof("cleanLegacyServiceResources(%s): Deleting firewall rule.", lbRefStr) + fwName := gce.MakeFirewallName(loadBalancerName) + err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteFirewall(fwName)) + return err + }, + func() error { + klog.Infof("cleanLegacyServiceResources(%s): Deleting target pool", lbRefStr) + + err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteTargetPool(loadBalancerName, l4netlb.cloud.Region())) + if err != nil { + klog.Errorf("cleanLegacyServiceResources(%v): Failed to delete target pool, got error %s", lbRefStr, err.Error()) + return err + } + + // Deletion of health checks is allowed only after the TargetPool reference is deleted + // Try to delete 2 types of health checks -- used for local traffic policy, and external + // They both use legacy naming scheme, so they are not useful for RBS service, and it is safe to delete them + healthChecksToDelete := []string{loadBalancerName, gce.MakeNodesHealthCheckName(l4netlb.namer.ClusterID())} + + for _, hcToDelete := range healthChecksToDelete { + klog.Infof("cleanLegacyServiceResources(%v): Deleting health check %v", lbRefStr, hcToDelete) + err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteHTTPHealthCheck(hcToDelete)) + if err != nil { + // Delete nodes health checks will fail if any other target pool is using it. + if utils.IsInUsedByError(err) { + klog.V(4).Infof("cleanLegacyServiceResources(%v): Health check %v is in use: %v", lbRefStr, hcToDelete, err) + return nil + } + return err + } + + // If health check is deleted without error, it means no load-balancer is using it. + // So we should delete the health check firewall as well. + fwName := gce.MakeHealthCheckFirewallName(l4netlb.namer.ClusterID(), hcToDelete, hcToDelete != loadBalancerName) + + klog.Infof("cleanLegacyServiceResources(%v): Deleting health check firewall rule %v", lbRefStr, fwName) + err = utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteFirewall(fwName)) + if err != nil { + klog.Errorf("cleanLegacyServiceResources(%v): Failed to delete health check %v firewall rule %v: %v", lbRefStr, hcToDelete, fwName, err) + return err + } + } + return nil + }, + ) + if errs != nil { + return utilerrors.Flatten(errs) + } + return nil +} diff --git a/pkg/utils/common/finalizer.go b/pkg/utils/common/finalizer.go index e3d24a8b00..8038d75a78 100644 --- a/pkg/utils/common/finalizer.go +++ b/pkg/utils/common/finalizer.go @@ -43,6 +43,9 @@ const ( LegacyNetLBFinalizer = "gke.networking.io/l4-netlb-v1" // NetLBFinalizerV2 is the finalizer used by newer controllers that manage L4 External LoadBalancer services. NetLBFinalizerV2 = "gke.networking.io/l4-netlb-v2" + // NeedsLegacyL4CleanupFinalizer is the finalizer used to indicate, + // that Ingress-GCE should clean up GCE resources used for Legacy L4 Target-Pool based Service + NeedsLegacyL4CleanupFinalizer = "gke.networking.io/needs-legacy-l4-cleanup" ) // IsDeletionCandidate is true if the passed in meta contains an ingress finalizer. diff --git a/pkg/utils/namer/interfaces.go b/pkg/utils/namer/interfaces.go index 5394970bd6..650b9d7e1e 100644 --- a/pkg/utils/namer/interfaces.go +++ b/pkg/utils/namer/interfaces.go @@ -91,6 +91,8 @@ type L4ResourcesNamer interface { L4HealthCheck(namespace, name string, shared bool) (string, string) // IsNEG returns if the given name is a VM_IP_NEG name. IsNEG(name string) bool + // ClusterID returns cluster's id + ClusterID() string } type ServiceAttachmentNamer interface { diff --git a/pkg/utils/namer/l4_namer.go b/pkg/utils/namer/l4_namer.go index 19ca7b1474..f34cd96107 100644 --- a/pkg/utils/namer/l4_namer.go +++ b/pkg/utils/namer/l4_namer.go @@ -101,3 +101,8 @@ func (n *L4Namer) hcFirewallName(hcName string) string { } return hcName + firewallHcSuffix } + +// ClusterID returns cluster's id +func (n *L4Namer) ClusterID() string { + return n.v2ClusterUID +} diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 0c921cca2a..55e3b82f47 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -693,6 +693,18 @@ func HasL4NetLBFinalizerV2(svc *api_v1.Service) bool { return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NetLBFinalizerV2, nil) } +// HasNeedsLegacyL4CleanupFinalizer returns true if the given Service has NeedsLegacyL4CleanupFinalizer +func HasNeedsLegacyL4CleanupFinalizer(svc *api_v1.Service) bool { + return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NeedsLegacyL4CleanupFinalizer, nil) +} + +func HasRBSAnnotation(svc *api_v1.Service) bool { + if val, ok := svc.Annotations[annotations.RBSAnnotationKey]; ok && val == annotations.RBSEnabled { + return true + } + return false +} + func LegacyForwardingRuleName(svc *api_v1.Service) string { return cloudprovider.DefaultLoadBalancerName(svc) }