From 7d129a36cd6da43ef8e7bced4e5c6eea63e7a23f Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Wed, 24 Aug 2022 11:26:15 +0000 Subject: [PATCH] WIP --- cmd/glbc/main.go | 1 + pkg/context/context.go | 1 + pkg/healthchecks/healthchecks_l4.go | 37 +++-- pkg/healthchecks/healthchecks_l4_test.go | 1 - pkg/healthchecks/interfaces.go | 2 +- pkg/healthchecks/ipv6.go | 31 ---- pkg/l4lb/l4controller.go | 7 +- pkg/loadbalancers/forwarding_rules_ipv6.go | 51 ++++--- pkg/loadbalancers/l4.go | 86 ++++++----- pkg/loadbalancers/l4_test.go | 165 ++++++++++++--------- pkg/loadbalancers/l4ipv6.go | 51 ++++--- pkg/utils/ipfamily_test.go | 18 +-- pkg/utils/namer/l4_namer.go | 30 ++-- pkg/utils/namer/l4_namer_test.go | 40 +++-- pkg/utils/utils_test.go | 8 +- 15 files changed, 275 insertions(+), 254 deletions(-) delete mode 100644 pkg/healthchecks/ipv6.go diff --git a/cmd/glbc/main.go b/cmd/glbc/main.go index a84dc2e0f8..55c5bff40f 100644 --- a/cmd/glbc/main.go +++ b/cmd/glbc/main.go @@ -199,6 +199,7 @@ func main() { ASMConfigMapName: flags.F.ASMConfigMapBasedConfigCMName, EndpointSlicesEnabled: flags.F.EnableEndpointSlices, MaxIGSize: flags.F.MaxIGSize, + EnableL4ILBDualStack: flags.F.EnableL4ILBDualStack, } ctx := ingctx.NewControllerContext(kubeConfig, kubeClient, backendConfigClient, frontendConfigClient, svcNegClient, ingParamsClient, svcAttachmentClient, cloud, namer, kubeSystemUID, ctxConfig) go app.RunHTTPServer(ctx.HealthCheck) diff --git a/pkg/context/context.go b/pkg/context/context.go index 71c881608a..52a65e8944 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -135,6 +135,7 @@ type ControllerContextConfig struct { ASMConfigMapName string EndpointSlicesEnabled bool MaxIGSize int + EnableL4ILBDualStack bool } // NewControllerContext returns a new shared set of informers. diff --git a/pkg/healthchecks/healthchecks_l4.go b/pkg/healthchecks/healthchecks_l4.go index 8f4ac6b2ee..c474afacae 100644 --- a/pkg/healthchecks/healthchecks_l4.go +++ b/pkg/healthchecks/healthchecks_l4.go @@ -44,6 +44,7 @@ const ( gceHcHealthyThreshold = int64(1) // Defaults to 3 * 8 = 24 seconds before the LB will steer traffic away. gceHcUnhealthyThreshold = int64(3) + l4ILBIPv6HCRangeString = "2600:2d00:1:b029::/64" ) var ( @@ -111,9 +112,8 @@ func (l4hc *l4HealthChecks) EnsureL4DualStackHealthCheck(svc *corev1.Service, na namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC) - hcFwName := namer.L4HealthCheckFirewall(svc.Namespace, svc.Name, sharedHC) hcPath, hcPort := helpers.GetServiceHealthCheckPathPort(svc) - klog.V(3).Infof("Ensuring L4 healthcheck: %s and firewall rule %s from service %s, shared: %v.", hcName, hcFwName, namespacedName.String(), sharedHC) + klog.V(3).Infof("Ensuring L4 healthcheck: %s for service %s, shared: %v.", hcName, namespacedName.String(), sharedHC) if sharedHC { hcPath, hcPort = gce.GetNodesHealthCheckPath(), gce.GetNodesHealthCheckPort() @@ -137,7 +137,8 @@ func (l4hc *l4HealthChecks) EnsureL4DualStackHealthCheck(svc *corev1.Service, na } if needsIPv4 { - klog.V(3).Infof("Healthcheck created, ensuring firewall rule %s", hcFwName) + hcFwName := namer.L4HealthCheckFirewall(svc.Namespace, svc.Name, sharedHC) + klog.V(3).Infof("Healthcheck created, ensuring IPv4 firewall rule %s for service %s", hcFwName, namespacedName.String()) err = l4hc.ensureFirewall(svc, hcFwName, hcPort, sharedHC, nodeNames) if err != nil { return &EnsureL4HealthCheckResult{ @@ -150,7 +151,7 @@ func (l4hc *l4HealthChecks) EnsureL4DualStackHealthCheck(svc *corev1.Service, na if needsIPv6 { ipv6HCFWName := namer.L4IPv6HealthCheckFirewall(svc.Namespace, svc.Name, sharedHC) - klog.V(3).Infof("Healthcheck created, ensuring ipv6 firewall rule %s", ipv6HCFWName) + klog.V(3).Infof("Healthcheck created, ensuring IPv6 firewall rule %s for service %s", ipv6HCFWName, namespacedName.String()) err = l4hc.ensureIPv6Firewall(svc, ipv6HCFWName, hcPort, sharedHC, nodeNames) if err != nil { return &EnsureL4HealthCheckResult{ @@ -167,7 +168,7 @@ func (l4hc *l4HealthChecks) EnsureL4DualStackHealthCheck(svc *corev1.Service, na func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, ipv6HCFWName string, hcPort int32, isSharedHC bool, nodeNames []string) error { hcFWRParams := firewalls.FirewallParams{ PortRanges: []string{strconv.Itoa(int(hcPort))}, - SourceRanges: L4ILBIPv6HCRange.StringSlice(), + SourceRanges: []string{l4ILBIPv6HCRangeString}, Protocol: string(corev1.ProtocolTCP), Name: ipv6HCFWName, NodeNames: nodeNames, @@ -177,12 +178,6 @@ func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, ipv6HCFWName // DeleteHealthCheck deletes health check (and firewall rule) for l4 service. Checks if shared resources are safe to delete. func (l4hc *l4HealthChecks) DeleteHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType) (string, error) { - return l4hc.DeleteDualStackHealthCheck(svc, namer, sharedHC, scope, l4Type, false) -} - -// DeleteDualStackHealthCheck deletes health check (and firewall rule) for l4 service. -// Checks if shared resources are safe to delete. Deletes IPv6 firewall if asked -func (l4hc *l4HealthChecks) DeleteDualStackHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, handleIPv6 bool) (string, error) { hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC) hcFwName := namer.L4HealthCheckFirewall(svc.Namespace, svc.Name, sharedHC) namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} @@ -203,16 +198,19 @@ func (l4hc *l4HealthChecks) DeleteDualStackHealthCheck(svc *corev1.Service, name klog.V(2).Infof("Failed to delete healthcheck %s: shared health check in use.", hcName) return "", nil } - // Health check deleted, now delete the firewall rule - if handleIPv6 { - errResource, err := l4hc.deleteIPv6HealthCheckFirewall(svc, hcName, namer, sharedHC, l4Type) - if err != nil { - return errResource, err - } - } return l4hc.deleteHealthCheckFirewall(svc, hcName, hcFwName, sharedHC, l4Type) } +// DeleteDualStackHealthCheck deletes health check, ipv4 and ipv6 firewall rules for l4 service. +// Checks if shared resources are safe to delete. +func (l4hc *l4HealthChecks) DeleteDualStackHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType) (string, error) { + errResource, err := l4hc.DeleteHealthCheck(svc, namer, sharedHC, scope, l4Type) + if err != nil { + return errResource, err + } + return l4hc.deleteIPv6HealthCheckFirewall(svc, namer, sharedHC, l4Type) +} + func (l4hc *l4HealthChecks) ensureL4HealthCheckInternal(hcName string, svcName types.NamespacedName, shared bool, path string, port int32, scope meta.KeyType, l4Type utils.L4LBType) (*composite.HealthCheck, string, error) { selfLink := "" key, err := composite.CreateKey(l4hc.cloud, hcName, scope) @@ -302,7 +300,8 @@ func (l4hc *l4HealthChecks) deleteHealthCheckFirewall(svc *corev1.Service, hcNam return "", nil } -func (l4hc *l4HealthChecks) deleteIPv6HealthCheckFirewall(svc *corev1.Service, hcName string, namer namer.L4ResourcesNamer, sharedHC bool, l4type utils.L4LBType) (string, error) { +func (l4hc *l4HealthChecks) deleteIPv6HealthCheckFirewall(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, l4type utils.L4LBType) (string, error) { + hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC) ipv6hcFwName := namer.L4IPv6HealthCheckFirewall(svc.Namespace, svc.Name, sharedHC) return l4hc.deleteHealthCheckFirewall(svc, hcName, ipv6hcFwName, sharedHC, l4type) } diff --git a/pkg/healthchecks/healthchecks_l4_test.go b/pkg/healthchecks/healthchecks_l4_test.go index f0fccb53e1..e288b95995 100644 --- a/pkg/healthchecks/healthchecks_l4_test.go +++ b/pkg/healthchecks/healthchecks_l4_test.go @@ -128,5 +128,4 @@ func TestCreateHealthCheck(t *testing.T) { t.Errorf("HealthCheck Scope mismatch! %v != %v", hc.Scope, v.scope) } } - } diff --git a/pkg/healthchecks/interfaces.go b/pkg/healthchecks/interfaces.go index 43339316f7..220439ae4a 100644 --- a/pkg/healthchecks/interfaces.go +++ b/pkg/healthchecks/interfaces.go @@ -68,7 +68,7 @@ type L4HealthChecks interface { // DeleteHealthCheck deletes health check (and firewall rule) for l4 service DeleteHealthCheck(svc *v1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType) (string, error) // DeleteDualStackHealthCheck deletes health check (and firewall rule) for l4 service, deletes IPv6 if asked - DeleteDualStackHealthCheck(svc *v1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, handleIPv6 bool) (string, error) + DeleteDualStackHealthCheck(svc *v1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType) (string, error) } type EnsureL4HealthCheckResult struct { diff --git a/pkg/healthchecks/ipv6.go b/pkg/healthchecks/ipv6.go deleted file mode 100644 index f049a542bd..0000000000 --- a/pkg/healthchecks/ipv6.go +++ /dev/null @@ -1,31 +0,0 @@ -package healthchecks - -import ( - "fmt" - - "k8s.io/klog/v2" - utilnet "k8s.io/utils/net" -) - -const ( - l4ELBIPv6HCRangeString = "2600:1901:8001::/48" - l4ILBIPv6HCRangeString = "2600:2d00:1:b029::/64" -) - -var ( - L4ELBIPv6HCRange utilnet.IPNetSet - L4ILBIPv6HCRange utilnet.IPNetSet -) - -func init() { - var err error - L4ELBIPv6HCRange, err = utilnet.ParseIPNets([]string{l4ELBIPv6HCRangeString}...) - if err != nil { - klog.Fatalf(fmt.Sprintf("utilnet.ParseIPNets([]string{%s}...) returned error %v, want nil", l4ELBIPv6HCRangeString, err)) - } - - L4ILBIPv6HCRange, err = utilnet.ParseIPNets([]string{l4ILBIPv6HCRangeString}...) - if err != nil { - klog.Fatalf(fmt.Sprintf("utilnet.ParseIPNets([]string{%s}...) returned error %v, want nil", l4ILBIPv6HCRangeString, err)) - } -} diff --git a/pkg/l4lb/l4controller.go b/pkg/l4lb/l4controller.go index 1f5189dbb6..1187683ad2 100644 --- a/pkg/l4lb/l4controller.go +++ b/pkg/l4lb/l4controller.go @@ -33,7 +33,6 @@ import ( "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/context" "k8s.io/ingress-gce/pkg/controller/translator" - "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/forwardingrules" l4metrics "k8s.io/ingress-gce/pkg/l4lb/metrics" "k8s.io/ingress-gce/pkg/loadbalancers" @@ -72,6 +71,7 @@ type L4Controller struct { syncTracker utils.TimeTracker forwardingRules ForwardingRulesGetter sharedResourcesLock sync.Mutex + enableDualStack bool } // NewILBController creates a new instance of the L4 ILB controller. @@ -90,6 +90,7 @@ func NewILBController(ctx *context.ControllerContext, stopCh chan struct{}) *L4C namer: ctx.L4Namer, translator: ctx.Translator, forwardingRules: forwardingrules.New(ctx.Cloud, meta.VersionGA, meta.Regional), + enableDualStack: ctx.EnableL4ILBDualStack, } l4c.backendPool = backends.NewPool(ctx.Cloud, l4c.namer) l4c.NegLinker = backends.NewNEGLinker(l4c.backendPool, negtypes.NewAdapter(ctx.Cloud), ctx.Cloud, ctx.SvcNegInformer.GetIndexer()) @@ -214,7 +215,7 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(key string, service *v1.Se Cloud: l4c.ctx.Cloud, Namer: l4c.namer, Recorder: l4c.ctx.Recorder(service.Namespace), - DualStackEnabled: flags.F.EnableL4ILBDualStack, + DualStackEnabled: l4c.enableDualStack, } l4 := loadbalancers.NewL4Handler(l4ilbParams) syncResult := l4.EnsureInternalLoadBalancer(nodeNames, service) @@ -261,7 +262,7 @@ func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service) *lo Cloud: l4c.ctx.Cloud, Namer: l4c.namer, Recorder: l4c.ctx.Recorder(svc.Namespace), - DualStackEnabled: flags.F.EnableL4ILBDualStack, + DualStackEnabled: l4c.enableDualStack, } l4 := loadbalancers.NewL4Handler(l4ilbParams) l4c.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeNormal, "DeletingLoadBalancer", "Deleting load balancer for %s", key) diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index 969031e852..99beae94bd 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" @@ -15,15 +14,15 @@ import ( "k8s.io/legacy-cloud-providers/gce" ) -func (l *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { - expectedIPv6FwdRule, err := l.buildExpectedIPv6ForwardingRule(bsLink, options) +func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { + expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options) if err != nil { - return nil, fmt.Errorf("l.buildExpectedIPv6ForwardingRule(%s, %v) returned error %w, want nil", bsLink, options, err) + return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v) returned error %w, want nil", bsLink, options, err) } - existingIPv6FwdRule, err := l.forwardingRules.Get(expectedIPv6FwdRule.Name) + existingIPv6FwdRule, err := l4.forwardingRules.Get(expectedIPv6FwdRule.Name) if err != nil { - return nil, fmt.Errorf("l.forwardingRules.GetForwardingRule(%s) returned error %w, want nil", expectedIPv6FwdRule.Name, err) + return nil, fmt.Errorf("l4.forwardingRules.GetForwardingRule(%s) returned error %w, want nil", expectedIPv6FwdRule.Name, err) } if existingIPv6FwdRule != nil { @@ -35,47 +34,39 @@ func (l *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*c klog.V(2).Infof("ensureIPv6ForwardingRule: Skipping update of unchanged ipv6 forwarding rule - %s", expectedIPv6FwdRule.Name) return existingIPv6FwdRule, nil } - - frDiff := cmp.Diff(existingIPv6FwdRule, expectedIPv6FwdRule, cmpopts.IgnoreFields(composite.ForwardingRule{}, "IPAddress")) - klog.V(2).Infof("ensureIPv6ForwardingRule: forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing ipv6 forwarding rule.", existingIPv6FwdRule, expectedIPv6FwdRule, frDiff) - - err = l.forwardingRules.Delete(existingIPv6FwdRule.Name) + err = l4.deleteChangedIPv6ForwardingRule(existingIPv6FwdRule, expectedIPv6FwdRule) if err != nil { return nil, err } - l.recorder.Eventf(l.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %q deleted", existingIPv6FwdRule.Name) } klog.V(2).Infof("ensureIPv6ForwardingRule: Creating/Recreating forwarding rule - %s", expectedIPv6FwdRule.Name) - err = l.forwardingRules.Create(expectedIPv6FwdRule) + err = l4.forwardingRules.Create(expectedIPv6FwdRule) if err != nil { return nil, err } - createdFr, err := l.forwardingRules.Get(expectedIPv6FwdRule.Name) + createdFr, err := l4.forwardingRules.Get(expectedIPv6FwdRule.Name) return createdFr, err } -func (l *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { - frName := l.getIPv6FRName() +func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { + frName := l4.getIPv6FRName() - frDesc, err := utils.MakeL4IPv6ForwardingRuleDescription(l.Service) + frDesc, err := utils.MakeL4IPv6ForwardingRuleDescription(l4.Service) if err != nil { return nil, fmt.Errorf("failed to compute description for forwarding rule %s, err: %w", frName, err) } - subnetworkURL := l.cloud.SubnetworkURL() + subnetworkURL := l4.cloud.SubnetworkURL() if options.SubnetName != "" { - key, err := l.CreateKey(frName) + subnetworkURL, err = l4.getSubnetworkURLByName(options.SubnetName) if err != nil { return nil, err } - subnetKey := *key - subnetKey.Name = options.SubnetName - subnetworkURL = cloud.SelfLink(meta.VersionGA, l.cloud.NetworkProjectID(), "subnetworks", &subnetKey) } - svcPorts := l.Service.Spec.Ports + svcPorts := l4.Service.Spec.Ports ports := utils.GetPorts(svcPorts) protocol := utils.GetProtocol(svcPorts) @@ -87,7 +78,7 @@ func (l *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptio LoadBalancingScheme: string(cloud.SchemeInternal), BackendService: bsLink, IpVersion: "IPV6", - Network: l.cloud.NetworkURL(), + Network: l4.cloud.NetworkURL(), Subnetwork: subnetworkURL, AllowGlobalAccess: options.AllowGlobalAccess, NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), @@ -100,6 +91,18 @@ func (l *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptio return fr, nil } +func (l4 *L4) deleteChangedIPv6ForwardingRule(existingFwdRule *composite.ForwardingRule, expectedFwdRule *composite.ForwardingRule) error { + frDiff := cmp.Diff(existingFwdRule, expectedFwdRule, cmpopts.IgnoreFields(composite.ForwardingRule{}, "IPAddress")) + klog.V(2).Infof("IPv6 forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing ipv6 forwarding rule.", existingFwdRule, expectedFwdRule, frDiff) + + err := l4.forwardingRules.Delete(existingFwdRule.Name) + if err != nil { + return err + } + l4.recorder.Eventf(l4.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %q deleted", existingFwdRule.Name) + return nil +} + func EqualIPv6ForwardingRules(fr1, fr2 *composite.ForwardingRule) (bool, error) { id1, err := cloud.ParseResourceURL(fr1.BackendService) if err != nil { diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index d088a05f9a..a091892f85 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -135,7 +135,7 @@ func (l4 *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) *L4ILBSyncR // and delete them if needed. for _, isShared := range []bool{true, false} { if l4.enableDualStack { - resourceInError, err := l4.l4HealthChecks.DeleteDualStackHealthCheck(svc, l4.namer, isShared, meta.Global, utils.ILB, true) + resourceInError, err := l4.l4HealthChecks.DeleteDualStackHealthCheck(svc, l4.namer, isShared, meta.Global, utils.ILB) if err != nil { result.GCEResourceInError = resourceInError result.Error = err @@ -151,31 +151,31 @@ func (l4 *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) *L4ILBSyncR return result } -func (l *L4) deleteIPv4Resources(result *L4ILBSyncResult, bsName string) { - frName := l.GetFRName() - key, err := l.CreateKey(frName) +func (l4 *L4) deleteIPv4Resources(result *L4ILBSyncResult, bsName string) { + frName := l4.GetFRName() + key, err := l4.CreateKey(frName) if err != nil { - klog.Errorf("Failed to create key for LoadBalancer resources with name %s for service %s, err %v", frName, l.NamespacedName.String(), err) + klog.Errorf("Failed to create key for LoadBalancer resources with name %s for service %s, err %v", frName, l4.NamespacedName.String(), err) result.Error = err return } // If any resource deletion fails, log the error and continue cleanup. - if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, meta.VersionGA)); err != nil { - klog.Errorf("Failed to delete forwarding rule for internal loadbalancer service %s, err %v", l.NamespacedName.String(), err) + if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l4.cloud, key, meta.VersionGA)); err != nil { + klog.Errorf("Failed to delete forwarding rule for internal loadbalancer service %s, err %v", l4.NamespacedName.String(), err) result.Error = err result.GCEResourceInError = annotations.ForwardingRuleResource } - if err = ensureAddressDeleted(l.cloud, bsName, l.cloud.Region()); err != nil { - klog.Errorf("Failed to delete address for internal loadbalancer service %s, err %v", l.NamespacedName.String(), err) + if err = ensureAddressDeleted(l4.cloud, bsName, l4.cloud.Region()); err != nil { + klog.Errorf("Failed to delete address for internal loadbalancer service %s, err %v", l4.NamespacedName.String(), err) result.Error = err result.GCEResourceInError = annotations.AddressResource } // delete firewall rule allowing load balancer source ranges - err = l.deleteFirewall(bsName) + err = l4.deleteFirewall(bsName) if err != nil { - klog.Errorf("Failed to delete firewall rule %s for internal loadbalancer service %s, err %v", bsName, l.NamespacedName.String(), err) + klog.Errorf("Failed to delete firewall rule %s for internal loadbalancer service %s, err %v", bsName, l4.NamespacedName.String(), err) result.GCEResourceInError = annotations.FirewallRuleResource result.Error = err } @@ -225,27 +225,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service name := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) options := getILBOptions(l4.Service) - // create healthcheck - sharedHC := !helpers.RequestsOnlyLocalTraffic(l4.Service) - var hcResult *healthchecks.EnsureL4HealthCheckResult - if l4.enableDualStack { - hcResult = l4.l4HealthChecks.EnsureL4DualStackHealthCheck(l4.Service, l4.namer, sharedHC, meta.Global, utils.ILB, nodeNames, utils.NeedsIPv4(l4.Service), utils.NeedsIPv6(l4.Service)) - if hcResult.HCFirewallRuleName != "" { - result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName - } - if hcResult.HCFirewallRuleIPv6Name != "" { - result.Annotations[annotations.FirewallRuleForHealthcheckIPv6Key] = hcResult.HCFirewallRuleIPv6Name - } - } else { - hcResult = l4.l4HealthChecks.EnsureL4HealthCheck(l4.Service, l4.namer, sharedHC, meta.Global, utils.ILB, nodeNames) - result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName - } - if hcResult.Err != nil { - result.GCEResourceInError = hcResult.GceResourceInError - result.Error = hcResult.Err - return result - } - result.Annotations[annotations.HealthcheckKey] = hcResult.HCName + hcLink := l4.provideHealthChecks(nodeNames, result) servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) @@ -257,6 +237,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service if err != nil { klog.Errorf("Failed to lookup existing backend service, ignoring err: %v", err) } + existingFR, err := l4.forwardingRules.Get(l4.GetFRName()) if existingBS != nil && existingBS.Protocol != string(protocol) { klog.Infof("Protocol changed from %q to %q for service %s", existingBS.Protocol, string(protocol), l4.NamespacedName) @@ -282,7 +263,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service } // ensure backend service - bs, err := l4.backendPool.EnsureL4BackendService(name, hcResult.HCLink, string(protocol), string(l4.Service.Spec.SessionAffinity), + bs, err := l4.backendPool.EnsureL4BackendService(name, hcLink, string(protocol), string(l4.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName, meta.VersionGA) if err != nil { result.GCEResourceInError = annotations.BackendServiceResource @@ -325,6 +306,30 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service return result } +func (l *L4) provideHealthChecks(nodeNames []string, result *L4ILBSyncResult) string { + sharedHC := !helpers.RequestsOnlyLocalTraffic(l.Service) + var hcResult *healthchecks.EnsureL4HealthCheckResult + if l.enableDualStack { + hcResult = l.l4HealthChecks.EnsureL4DualStackHealthCheck(l.Service, l.namer, sharedHC, meta.Global, utils.ILB, nodeNames, utils.NeedsIPv4(l.Service), utils.NeedsIPv6(l.Service)) + if hcResult.HCFirewallRuleName != "" { + result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName + } + if hcResult.HCFirewallRuleIPv6Name != "" { + result.Annotations[annotations.FirewallRuleForHealthcheckIPv6Key] = hcResult.HCFirewallRuleIPv6Name + } + } else { + hcResult = l.l4HealthChecks.EnsureL4HealthCheck(l.Service, l.namer, sharedHC, meta.Global, utils.ILB, nodeNames) + result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName + } + if hcResult.Err != nil { + result.GCEResourceInError = hcResult.GceResourceInError + result.Error = hcResult.Err + return "" + } + result.Annotations[annotations.HealthcheckKey] = hcResult.HCName + return hcResult.HCLink +} + func (l4 *L4) getSubnetworkURLByName(subnetName string) (string, error) { subnetKey, err := l4.CreateKey(subnetName) if err != nil { @@ -333,9 +338,12 @@ func (l4 *L4) getSubnetworkURLByName(subnetName string) (string, error) { return cloud.SelfLink(meta.VersionGA, l4.cloud.NetworkProjectID(), "subnetworks", subnetKey), nil } -func (l *L4) ensureIPv4Resources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingFR *composite.ForwardingRule) { - frName := l.GetFRName() - fr, err := l.ensureForwardingRule(frName, bs.SelfLink, options, existingFR) +// ensureIPv4Resources creates resources specific to IPv4 L4 Load Balancers: +// - IPv4 Forwarding Rule +// - IPv4 Firewall +func (l4 *L4) ensureIPv4Resources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingFR *composite.ForwardingRule) { + frName := l4.GetFRName() + fr, err := l4.ensureForwardingRule(frName, bs.SelfLink, options, existingFR) if err != nil { klog.Errorf("EnsureInternalLoadBalancer: Failed to create forwarding rule - %v", err) result.GCEResourceInError = annotations.ForwardingRuleResource @@ -349,13 +357,13 @@ func (l *L4) ensureIPv4Resources(result *L4ILBSyncResult, nodeNames []string, op } // ensure firewalls - sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l.Service) + sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l4.Service) if err != nil { result.Error = err return } - servicePorts := l.Service.Spec.Ports + servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) portRanges := utils.GetServicePortRanges(servicePorts) // Add firewall rule for ILB traffic to nodes @@ -369,7 +377,7 @@ func (l *L4) ensureIPv4Resources(result *L4ILBSyncResult, nodeNames []string, op L4Type: utils.ILB, } - if err := firewalls.EnsureL4LBFirewallForNodes(l.Service, &nodesFWRParams, l.cloud, l.recorder); err != nil { + if err := firewalls.EnsureL4LBFirewallForNodes(l4.Service, &nodesFWRParams, l4.cloud, l4.recorder); err != nil { result.GCEResourceInError = annotations.FirewallRuleResource result.Error = err return diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index d2d4c1eebd..3d7dc889e8 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -1387,25 +1387,25 @@ func TestEnsureInternalDualStackLoadBalancer(t *testing.T) { desc string }{ { - ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, desc: "Test ipv4 ipv6 service", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, }, { + desc: "Test ipv4 ipv6 local service", ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, localPolicy: true, - desc: "Test ipv4 ipv6 local service", }, { - ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, desc: "Test ipv6 ipv4 service", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, }, { - ipFamilies: []v1.IPFamily{v1.IPv4Protocol}, desc: "Test ipv4 service", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol}, }, { - ipFamilies: []v1.IPFamily{v1.IPv6Protocol}, desc: "Test ipv6 service", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol}, }, } @@ -1442,7 +1442,7 @@ func TestEnsureInternalDualStackLoadBalancer(t *testing.T) { } assertInternalDualStackLbResources(t, svc, l, nodeNames, result.Annotations) - backendServiceName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name) + backendServiceName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name) key := meta.RegionalKey(backendServiceName, l.cloud.Region()) bs, err := composite.GetBackendService(l.cloud, key, meta.VersionGA) if err != nil { @@ -1477,68 +1477,15 @@ func TestEnsureInternalDualStackLoadBalancer(t *testing.T) { } } -func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l *L4, nodeNames []string, resourceAnnotations map[string]string) { - // Check that Firewalls are created for the LoadBalancer and the HealthCheck - sharedHC := !servicehelper.RequestsOnlyLocalTraffic(apiService) - resourceName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name) - resourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(apiService.Namespace, apiService.Name), "", meta.VersionGA, false, utils.ILB) +func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l4 *L4, nodeNames []string, resourceAnnotations map[string]string) { + resourceName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) - if err != nil { - t.Errorf("Failed to create description for resources, err %v", err) - } - sharedResourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(apiService.Namespace, apiService.Name), "", meta.VersionGA, true, utils.ILB) - if err != nil { - t.Errorf("Failed to create description for shared resources, err %v", err) - } proto := utils.GetProtocol(apiService.Spec.Ports) expectedAnnotations := make(map[string]string) - hcName := l.namer.L4HealthCheck(apiService.Namespace, apiService.Name, sharedHC) - hcFwName := l.namer.L4HealthCheckFirewall(apiService.Namespace, apiService.Name, sharedHC) - ipv6hcFwName := l.namer.L4IPv6HealthCheckFirewall(apiService.Namespace, apiService.Name, sharedHC) - // hcDesc is the resource description for healthcheck and firewall rule allowing healthcheck. - hcDesc := resourceDesc - if sharedHC { - hcDesc = sharedResourceDesc - } - - type nameAndDesc struct { - fwName string - fwDesc string - } - var fwNamesAndDesc []nameAndDesc - - if utils.NeedsIPv4(apiService) { - fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{resourceName, resourceDesc}) - fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{hcFwName, hcDesc}) - expectedAnnotations[annotations.FirewallRuleForHealthcheckKey] = hcFwName - expectedAnnotations[annotations.FirewallRuleKey] = resourceName - } - if utils.NeedsIPv6(apiService) { - ipv6FirewallName := l.getIPv6FirewallName(resourceName) - fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{ipv6FirewallName, resourceDesc}) - fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{ipv6hcFwName, hcDesc}) - expectedAnnotations[annotations.FirewallRuleForHealthcheckIPv6Key] = ipv6hcFwName - expectedAnnotations[annotations.FirewallRuleIPv6Key] = ipv6FirewallName - } - - for _, info := range fwNamesAndDesc { - firewall, err := l.cloud.GetFirewall(info.fwName) - if err != nil { - t.Fatalf("Failed to fetch firewall rule %q - err %v", info.fwName, err) - } - if !utils.EqualStringSets(nodeNames, firewall.TargetTags) { - t.Fatalf("Expected firewall rule target tags '%v', Got '%v'", nodeNames, firewall.TargetTags) - } - if len(firewall.SourceRanges) == 0 { - t.Fatalf("Unexpected empty source range for firewall rule %v", firewall) - } - if !sharedHC && firewall.Description != info.fwDesc { - t.Errorf("Unexpected description in firewall %q - Expected %s, Got %s", info.fwName, firewall.Description, info.fwDesc) - } - } + hcName := l4.namer.L4HealthCheck(apiService.Namespace, apiService.Name, sharedHC) // Check that HealthCheck is created - healthcheck, err := composite.GetHealthCheck(l.cloud, meta.GlobalKey(hcName), meta.VersionGA) + healthcheck, err := composite.GetHealthCheck(l4.cloud, meta.GlobalKey(hcName), meta.VersionGA) if err != nil { t.Errorf("Failed to fetch healthcheck %s - err %v", hcName, err) } @@ -1553,9 +1500,9 @@ func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l // Check that BackendService exists backendServiceName := resourceName - key := meta.RegionalKey(backendServiceName, l.cloud.Region()) - backendServiceLink := cloud.SelfLink(meta.VersionGA, l.cloud.ProjectID(), "backendServices", key) - bs, err := composite.GetBackendService(l.cloud, key, meta.VersionGA) + key := meta.RegionalKey(backendServiceName, l4.cloud.Region()) + backendServiceLink := cloud.SelfLink(meta.VersionGA, l4.cloud.ProjectID(), "backendServices", key) + bs, err := composite.GetBackendService(l4.cloud, key, meta.VersionGA) if err != nil { t.Errorf("Failed to fetch backend service %s - err %v", backendServiceName, err) } @@ -1575,15 +1522,15 @@ func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l expectedAnnotations[annotations.BackendServiceKey] = backendServiceName subnet := apiService.Annotations[gce.ServiceAnnotationILBSubnet] if subnet == "" { - subnet = l.cloud.SubnetworkURL() + subnet = l4.cloud.SubnetworkURL() } else { key.Name = subnet - subnet = cloud.SelfLink(meta.VersionGA, l.cloud.ProjectID(), "subnetworks", key) + subnet = cloud.SelfLink(meta.VersionGA, l4.cloud.ProjectID(), "subnetworks", key) } // Check that ForwardingRule is created var expectedFwdRules []string if utils.NeedsIPv4(apiService) { - ipv4FRName := l.GetFRName() + ipv4FRName := l4.GetFRName() expectedFwdRules = append(expectedFwdRules, ipv4FRName) if proto == v1.ProtocolTCP { expectedAnnotations[annotations.TCPForwardingRuleKey] = ipv4FRName @@ -1592,7 +1539,7 @@ func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l } } if utils.NeedsIPv6(apiService) { - ipv6FRName := l.getIPv6FRName() + ipv6FRName := l4.getIPv6FRName() expectedFwdRules = append(expectedFwdRules, ipv6FRName) if proto == v1.ProtocolTCP { expectedAnnotations[annotations.TCPForwardingRuleIPv6Key] = ipv6FRName @@ -1601,7 +1548,7 @@ func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l } } for _, frName := range expectedFwdRules { - fwdRule, err := composite.GetForwardingRule(l.cloud, meta.RegionalKey(frName, l.cloud.Region()), meta.VersionGA) + fwdRule, err := composite.GetForwardingRule(l4.cloud, meta.RegionalKey(frName, l4.cloud.Region()), meta.VersionGA) if err != nil { t.Errorf("Failed to fetch forwarding rule %s - err %v", frName, err) @@ -1619,7 +1566,7 @@ func assertInternalDualStackLbResources(t *testing.T, apiService *v1.Service, l t.Errorf("Unexpected subnetwork %q in forwarding rule, expected %q", fwdRule.Subnetwork, subnet) } - addr, err := l.cloud.GetRegionAddress(frName, l.cloud.Region()) + addr, err := l4.cloud.GetRegionAddress(frName, l4.cloud.Region()) if err == nil || addr != nil { t.Errorf("Expected error when looking up ephemeral address, got %v", addr) } @@ -1807,3 +1754,77 @@ func assertInternalLbResourcesDeleted(t *testing.T, apiService *v1.Service, fire t.Errorf("Expected error when looking up IP address after deletion") } } + +func verifyFirewall(l4 *L4, nodeNames []string, fwName, fwDesc string) error { + firewall, err := l4.cloud.GetFirewall(fwName) + if err != nil { + return fmt.Errorf("failed to fetch firewall rule %q - err %w", fwName, err) + } + if !utils.EqualStringSets(nodeNames, firewall.TargetTags) { + return fmt.Errorf("expected firewall rule target tags '%v', Got '%w'", nodeNames, firewall.TargetTags) + } + if len(firewall.SourceRanges) == 0 { + return fmt.Errorf("Unexpected empty source range for firewall rule %v", firewall) + } + if !sharedHC && firewall.Description != info.fwDesc { + return fmt.Errorf("Unexpected description in firewall %q - Expected %s, Got %s", info.fwName, firewall.Description, info.fwDesc) + } +} + +func verifyDualStackHCFirewalls(t *testing.T, nodeNames []string, l4 *L4, expectedAnnotations map[string]string) { + resourceName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) + + sharedHC := !servicehelper.RequestsOnlyLocalTraffic(l4.Service) + resourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4.Service.Namespace, l4.Service.Name), "", meta.VersionGA, false, utils.ILB) + if err != nil { + t.Errorf("Failed to create description for resources, err %v", err) + } + if err != nil { + t.Errorf("Failed to create description for shared resources, err %v", err) + } + sharedResourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4.Service.Namespace, l4.Service.Name), "", meta.VersionGA, true, utils.ILB) + + hcFwName := l4.namer.L4HealthCheckFirewall(l4.Service.Namespace, l4.Service.Name, sharedHC) + ipv6hcFwName := l4.namer.L4IPv6HealthCheckFirewall(l4.Service.Namespace, l4.Service.Name, sharedHC) + // hcDesc is the resource description for healthcheck and firewall rule allowing healthcheck. + hcDesc := resourceDesc + if sharedHC { + hcDesc = sharedResourceDesc + } + + type nameAndDesc struct { + fwName string + fwDesc string + } + var fwNamesAndDesc []nameAndDesc + + if utils.NeedsIPv4(l4.Service) { + fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{resourceName, resourceDesc}) + fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{hcFwName, hcDesc}) + expectedAnnotations[annotations.FirewallRuleForHealthcheckKey] = hcFwName + expectedAnnotations[annotations.FirewallRuleKey] = resourceName + } + if utils.NeedsIPv6(l4.Service) { + ipv6FirewallName := l4.getIPv6FirewallName(resourceName) + fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{ipv6FirewallName, resourceDesc}) + fwNamesAndDesc = append(fwNamesAndDesc, nameAndDesc{ipv6hcFwName, hcDesc}) + expectedAnnotations[annotations.FirewallRuleForHealthcheckIPv6Key] = ipv6hcFwName + expectedAnnotations[annotations.FirewallRuleIPv6Key] = ipv6FirewallName + } + + for _, info := range fwNamesAndDesc { + firewall, err := l4.cloud.GetFirewall(info.fwName) + if err != nil { + t.Fatalf("Failed to fetch firewall rule %q - err %v", info.fwName, err) + } + if !utils.EqualStringSets(nodeNames, firewall.TargetTags) { + t.Fatalf("Expected firewall rule target tags '%v', Got '%v'", nodeNames, firewall.TargetTags) + } + if len(firewall.SourceRanges) == 0 { + t.Fatalf("Unexpected empty source range for firewall rule %v", firewall) + } + if !sharedHC && firewall.Description != info.fwDesc { + t.Errorf("Unexpected description in firewall %q - Expected %s, Got %s", info.fwName, firewall.Description, info.fwDesc) + } + } +} diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index a3fa379b1f..c32aae2105 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -13,8 +13,11 @@ import ( "k8s.io/legacy-cloud-providers/gce" ) -func (l *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, bsName string) { - ipv6fr, err := l.ensureIPv6ForwardingRule(bsLink, options) +// ensureIPv6Resources creates resources specific to IPv6 L4 Load Balancers: +// - IPv6 Forwarding Rule +// - IPv6 Firewall +func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, bsName string) { + ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options) if err != nil { klog.Errorf("ensureIPv6Resources: Failed to create ipv6 forwarding rule - %v", err) syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource @@ -27,8 +30,8 @@ func (l *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string syncResult.Annotations[annotations.UDPForwardingRuleIPv6Key] = ipv6fr.Name } - firewallName := l.getIPv6FirewallName(bsName) - err = l.ensureIPv6Firewall(ipv6fr, firewallName, nodeNames) + firewallName := l4.getIPv6FirewallName(bsName) + err = l4.ensureIPv6Firewall(ipv6fr, firewallName, nodeNames) if err != nil { syncResult.GCEResourceInError = annotations.FirewallRuleIPv6Resource syncResult.Error = err @@ -40,37 +43,37 @@ func (l *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string syncResult.Status = utils.AddIPToLBStatus(syncResult.Status, trimmedIPv6Address) } -func (l *L4) deleteIPv6Resources(syncResult *L4ILBSyncResult, bsName string) { - err := l.deleteIPv6ForwardingRule() +func (l4 *L4) deleteIPv6Resources(syncResult *L4ILBSyncResult, bsName string) { + err := l4.deleteIPv6ForwardingRule() if err != nil { - klog.Errorf("Failed to delete ipv6 forwarding rule for internal loadbalancer service %s, err %v", l.NamespacedName.String(), err) + klog.Errorf("Failed to delete ipv6 forwarding rule for internal loadbalancer service %s, err %v", l4.NamespacedName.String(), err) syncResult.Error = err syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource } - err = l.deleteIPv6Firewall(bsName) + err = l4.deleteIPv6Firewall(bsName) if err != nil { - klog.Errorf("Failed to delete ipv6 firewall rule for internal loadbalancer service %s, err %v", l.NamespacedName.String(), err) + klog.Errorf("Failed to delete ipv6 firewall rule for internal loadbalancer service %s, err %v", l4.NamespacedName.String(), err) syncResult.GCEResourceInError = annotations.FirewallRuleIPv6Resource syncResult.Error = err } } -func (l *L4) getIPv6FRName() string { - protocol := utils.GetProtocol(l.Service.Spec.Ports) - return l.getIPv6FRNameWithProtocol(string(protocol)) +func (l4 *L4) getIPv6FRName() string { + protocol := utils.GetProtocol(l4.Service.Spec.Ports) + return l4.getIPv6FRNameWithProtocol(string(protocol)) } -func (l *L4) getIPv6FRNameWithProtocol(protocol string) string { - return l.namer.L4IPv6ForwardingRule(l.Service.Namespace, l.Service.Name, strings.ToLower(protocol)) +func (l4 *L4) getIPv6FRNameWithProtocol(protocol string) string { + return l4.namer.L4IPv6ForwardingRule(l4.Service.Namespace, l4.Service.Name, strings.ToLower(protocol)) } -func (l *L4) getIPv6FirewallName(bsName string) string { +func (l4 *L4) getIPv6FirewallName(bsName string) string { return bsName + "-ipv6" } -func (l *L4) ensureIPv6Firewall(forwardingRule *composite.ForwardingRule, firewallName string, nodeNames []string) error { - svcPorts := l.Service.Spec.Ports +func (l4 *L4) ensureIPv6Firewall(forwardingRule *composite.ForwardingRule, firewallName string, nodeNames []string) error { + svcPorts := l4.Service.Spec.Ports portRanges := utils.GetServicePortRanges(svcPorts) protocol := utils.GetProtocol(svcPorts) @@ -85,18 +88,18 @@ func (l *L4) ensureIPv6Firewall(forwardingRule *composite.ForwardingRule, firewa L4Type: utils.ILB, } - return firewalls.EnsureL4LBFirewallForNodes(l.Service, &ipv6nodesFWRParams, l.cloud, l.recorder) + return firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder) } -func (l *L4) deleteIPv6ForwardingRule() error { - ipv6FrName := l.getIPv6FRName() - ipv6key, err := l.CreateKey(ipv6FrName) +func (l4 *L4) deleteIPv6ForwardingRule() error { + ipv6FrName := l4.getIPv6FRName() + ipv6key, err := l4.CreateKey(ipv6FrName) if err != nil { return err } - return utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, ipv6key, meta.VersionGA)) + return utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l4.cloud, ipv6key, meta.VersionGA)) } -func (l *L4) deleteIPv6Firewall(bsName string) error { - return l.deleteFirewall(l.getIPv6FirewallName(bsName)) +func (l4 *L4) deleteIPv6Firewall(bsName string) error { + return l4.deleteFirewall(l4.getIPv6FirewallName(bsName)) } diff --git a/pkg/utils/ipfamily_test.go b/pkg/utils/ipfamily_test.go index 9df524bbc8..0f999c06b9 100644 --- a/pkg/utils/ipfamily_test.go +++ b/pkg/utils/ipfamily_test.go @@ -13,30 +13,30 @@ func TestNeedsIPv6(t *testing.T) { desc string }{ { + desc: "Should return false for nil pointer", service: nil, wantNeedsIPv6: false, - desc: "Should return false for nil pointer", }, { + desc: "Should detect ipv6 for dual-stack ip families", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, }}, wantNeedsIPv6: true, - desc: "Should detect ipv6 for dual-stack ip families", }, { + desc: "Should not detect ipv6 for only ipv4 families", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, }}, wantNeedsIPv6: false, - desc: "Should not detect ipv6 for only ipv4 families", }, { + desc: "Should detect ipv6 for only ipv6 families", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv6Protocol}, }}, wantNeedsIPv6: true, - desc: "Should detect ipv6 for only ipv6 families", }, } @@ -58,37 +58,37 @@ func TestNeedsIPv4(t *testing.T) { desc string }{ { + desc: "Should return false for nil pointer", service: nil, wantNeedsIPv4: false, - desc: "Should return false for nil pointer", }, { + desc: "Should handle dual-stack ip families", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, }}, wantNeedsIPv4: true, - desc: "Should handle dual-stack ip families", }, { + desc: "Should handle only ipv4 family", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, }}, wantNeedsIPv4: true, - desc: "Should handle only ipv4 family", }, { + desc: "Should not handle only ipv6 family", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{v1.IPv6Protocol}, }}, wantNeedsIPv4: false, - desc: "Should not handle only ipv6 family", }, { + desc: "Empty families should be recognized as IPv4. Should never happen in real life", service: &v1.Service{Spec: v1.ServiceSpec{ IPFamilies: []v1.IPFamily{}, }}, wantNeedsIPv4: true, - desc: "Empty families should be recognized as IPv4. Should never happen in real life", }, } diff --git a/pkg/utils/namer/l4_namer.go b/pkg/utils/namer/l4_namer.go index f33124b584..9ece3ea608 100644 --- a/pkg/utils/namer/l4_namer.go +++ b/pkg/utils/namer/l4_namer.go @@ -1,7 +1,6 @@ package namer import ( - "fmt" "strings" "k8s.io/ingress-gce/pkg/utils/common" @@ -91,11 +90,11 @@ func (namer *L4Namer) L4HealthCheck(namespace, name string, shared bool) string // L4HealthCheckFirewall returns the name of the L4 LB Healthcheck Firewall func (namer *L4Namer) L4HealthCheckFirewall(namespace, name string, shared bool) string { - if !shared { - l4Name := namer.L4Backend(namespace, name) - return namer.hcFirewallName(l4Name, "") + if shared { + return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedFirewallHcSuffix}, "-") } - return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedFirewallHcSuffix}, "-") + l4Name := namer.L4Backend(namespace, name) + return namer.hcFirewallName(l4Name, "") } // L4IPv6ForwardingRule returns the name of the L4 forwarding rule name based on the service namespace, name and protocol. @@ -105,16 +104,16 @@ func (namer *L4Namer) L4HealthCheckFirewall(namespace, name string, shared bool) // // Output name is at most 63 characters. func (namer *L4Namer) L4IPv6ForwardingRule(namespace, name, protocol string) string { - return namer.L4ForwardingRule(namespace, name, fmt.Sprintf("%s-%s", protocol, ipv6Suffix)) + return getSuffixedName(namer.L4ForwardingRule(namespace, name, protocol), "-"+ipv6Suffix) } // L4IPv6HealthCheckFirewall returns the name of the IPv6 L4 LB health check firewall rule. func (namer *L4Namer) L4IPv6HealthCheckFirewall(namespace, name string, shared bool) string { - if !shared { - l4Name := namer.L4Backend(namespace, name) - return namer.hcFirewallName(l4Name, ipv6Suffix) + if shared { + return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedFirewallHcSuffix, ipv6Suffix}, "-") } - return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedFirewallHcSuffix, ipv6Suffix}, "-") + l4Name := namer.L4Backend(namespace, name) + return namer.hcFirewallName(l4Name, "-"+ipv6Suffix) } // IsNEG indicates if the given name is a NEG following the L4 naming convention. @@ -135,9 +134,12 @@ func (n *L4Namer) hcFirewallName(hcName, suffix string) string { return getSuffixedName(hcName, firewallHcSuffix+suffix) } +func getTrimmedNamespacedName(namespace, name string, maxLength int) string { + return strings.Join(TrimFieldsEvenly(maxLength, namespace, name), "-") +} + func getSuffixedName(name string, suffix string) string { - trimmedName := ensureSpaceForSuffix(name, suffix) - return trimmedName + suffix + return ensureSpaceForSuffix(name, suffix) + suffix } func ensureSpaceForSuffix(name string, suffix string) string { @@ -147,7 +149,3 @@ func ensureSpaceForSuffix(name string, suffix string) string { } return name } - -func getTrimmedNamespacedName(namespace, name string, maxLength int) string { - return strings.Join(TrimFieldsEvenly(maxLength, namespace, name), "-") -} diff --git a/pkg/utils/namer/l4_namer_test.go b/pkg/utils/namer/l4_namer_test.go index e16ff4186e..14f0f16cc6 100644 --- a/pkg/utils/namer/l4_namer_test.go +++ b/pkg/utils/namer/l4_namer_test.go @@ -10,15 +10,17 @@ func TestL4Namer(t *testing.T) { longstring1 := "012345678901234567890123456789012345678901234567890123456789abc" longstring2 := "012345678901234567890123456789012345678901234567890123456789pqr" testCases := []struct { - desc string - namespace string - name string - proto string - sharedHC bool - expectFRName string - expectNEGName string - expectHcFwName string - expectHcName string + desc string + namespace string + name string + proto string + sharedHC bool + expectFRName string + expectIPv6FRName string + expectNEGName string + expectHcFwName string + expectIPv6HcFName string + expectHcName string }{ { "simple case", @@ -27,8 +29,10 @@ func TestL4Namer(t *testing.T) { "TCP", false, "k8s2-tcp-7kpbhpki-namespace-name-956p2p7x", + "k8s2-tcp-7kpbhpki-namespace-name-956p2p7x-ipv6", "k8s2-7kpbhpki-namespace-name-956p2p7x", "k8s2-7kpbhpki-namespace-name-956p2p7x-fw", + "k8s2-7kpbhpki-namespace-name-956p2p7x-fw-ipv6", "k8s2-7kpbhpki-namespace-name-956p2p7x", }, { @@ -38,8 +42,10 @@ func TestL4Namer(t *testing.T) { "TCP", true, "k8s2-tcp-7kpbhpki-namespace-name-956p2p7x", + "k8s2-tcp-7kpbhpki-namespace-name-956p2p7x-ipv6", "k8s2-7kpbhpki-namespace-name-956p2p7x", "k8s2-7kpbhpki-l4-shared-hc-fw", + "k8s2-7kpbhpki-l4-shared-hc-fw-ipv6", "k8s2-7kpbhpki-l4-shared-hc", }, { @@ -49,8 +55,10 @@ func TestL4Namer(t *testing.T) { "UDP", false, "k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm400mg", + "k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm-ipv6", "k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg", "k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm40-fw", + "k8s2-7kpbhpki-01234567890123456789-0123456789012345678--fw-ipv6", "k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg", }, { @@ -60,8 +68,10 @@ func TestL4Namer(t *testing.T) { "UDP", true, "k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm400mg", + "k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm-ipv6", "k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg", "k8s2-7kpbhpki-l4-shared-hc-fw", + "k8s2-7kpbhpki-l4-shared-hc-fw-ipv6", "k8s2-7kpbhpki-l4-shared-hc", }, } @@ -69,21 +79,29 @@ func TestL4Namer(t *testing.T) { newNamer := NewL4Namer(kubeSystemUID, nil) for _, tc := range testCases { frName := newNamer.L4ForwardingRule(tc.namespace, tc.name, strings.ToLower(tc.proto)) + ipv6FrName := newNamer.L4IPv6ForwardingRule(tc.namespace, tc.name, strings.ToLower(tc.proto)) negName := newNamer.L4Backend(tc.namespace, tc.name) hcName := newNamer.L4HealthCheck(tc.namespace, tc.name, tc.sharedHC) hcFwName := newNamer.L4HealthCheckFirewall(tc.namespace, tc.name, tc.sharedHC) - if len(frName) > maxResourceNameLength || len(negName) > maxResourceNameLength || len(hcName) > maxResourceNameLength || len(hcFwName) > maxResourceNameLength { - t.Errorf("%s: got len(frName) == %v, len(negName) == %v, len(hcName) == %v, len(hcFwName) == %v want <= 63", tc.desc, len(frName), len(negName), len(hcName), len(hcFwName)) + ipv6hcFwName := newNamer.L4IPv6HealthCheckFirewall(tc.namespace, tc.name, tc.sharedHC) + if len(frName) > maxResourceNameLength || len(ipv6FrName) > maxResourceNameLength || len(negName) > maxResourceNameLength || len(hcName) > maxResourceNameLength || len(hcFwName) > maxResourceNameLength || len(ipv6hcFwName) > maxResourceNameLength { + t.Errorf("%s: got len(frName) == %v, got len(ipv6FrName) == %v, len(negName) == %v, len(hcName) == %v, len(hcFwName) == %v, len(ipv6hcFwName) == %v want <= 63", tc.desc, len(frName), len(ipv6FrName), len(negName), len(hcName), len(hcFwName), len(ipv6hcFwName)) } if frName != tc.expectFRName { t.Errorf("%s ForwardingRuleName: got %q, want %q", tc.desc, frName, tc.expectFRName) } + if ipv6FrName != tc.expectIPv6FRName { + t.Errorf("%s IPv6 ForwardingRuleName: got %q, want %q", tc.desc, ipv6FrName, tc.expectIPv6FRName) + } if negName != tc.expectNEGName { t.Errorf("%s VMIPNEGName: got %q, want %q", tc.desc, negName, tc.expectFRName) } if hcFwName != tc.expectHcFwName { t.Errorf("%s FirewallName For Healthcheck: got %q, want %q", tc.desc, hcFwName, tc.expectHcFwName) } + if ipv6hcFwName != tc.expectIPv6HcFName { + t.Errorf("%s IPv6 FirewallName For Healthcheck: got %q, want %q", tc.desc, ipv6hcFwName, tc.expectIPv6HcFName) + } if hcName != tc.expectHcName { t.Errorf("%s HealthCheckName: got %q, want %q", tc.desc, hcName, tc.expectHcName) } diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index b050fc0c10..dfcdbc9f2e 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -1525,26 +1525,27 @@ func TestGetServicePortRanges(t *testing.T) { func TestAddIPToLBStatus(t *testing.T) { testCases := []struct { + desc string status *api_v1.LoadBalancerStatus ipsToAdd []string expectedStatus *api_v1.LoadBalancerStatus - desc string }{ { + desc: "Should create empty status ingress if no IPs provided", status: nil, ipsToAdd: []string{}, expectedStatus: &api_v1.LoadBalancerStatus{Ingress: []api_v1.LoadBalancerIngress{}}, - desc: "Should create empty status ingress if no IPs provided", }, { + desc: "Should add IPs to the empty status", status: nil, ipsToAdd: []string{"1.1.1.1", "0::0"}, expectedStatus: &api_v1.LoadBalancerStatus{Ingress: []api_v1.LoadBalancerIngress{ {IP: "1.1.1.1"}, {IP: "0::0"}, }}, - desc: "Should add IPs to the empty status", }, { + desc: "Should add IP to the existing status", status: &api_v1.LoadBalancerStatus{Ingress: []api_v1.LoadBalancerIngress{ {IP: "0::0"}, }}, @@ -1552,7 +1553,6 @@ func TestAddIPToLBStatus(t *testing.T) { expectedStatus: &api_v1.LoadBalancerStatus{Ingress: []api_v1.LoadBalancerIngress{ {IP: "0::0"}, {IP: "1.1.1.1"}, }}, - desc: "Should add IP to the existing status", }, }