diff --git a/pkg/address/hold.go b/pkg/address/hold.go index c9c7e0a357..cf87a5e604 100644 --- a/pkg/address/hold.go +++ b/pkg/address/hold.go @@ -1,6 +1,8 @@ package address import ( + "fmt" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" api_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -9,6 +11,7 @@ import ( "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/utils" + "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog/v2" ) @@ -19,6 +22,8 @@ type HoldConfig struct { Service *api_v1.Service ExistingRules []*composite.ForwardingRule ForwardingRuleDeleter ForwardingRuleDeleter + IPVersion IPVersion + SubnetworkURL string } type ForwardingRuleDeleter interface { @@ -31,32 +36,38 @@ type HoldResult struct { Release func() error } -// HoldExternalIPv4 will determine which IP to use for forwarding rules +// HoldExternal will determine which IP to use for forwarding rules // and will hold it for future forwarding rules. After binding // IP to a forwarding rule call Release to prevent leaks. -func HoldExternalIPv4(cfg HoldConfig) (HoldResult, error) { +func HoldExternal(cfg HoldConfig) (HoldResult, error) { var err error res := HoldResult{ Release: func() error { return nil }, } - log := cfg.Logger.WithName("HoldIPv4") - - // external specific - subnet := "" - name := utils.LegacyForwardingRuleName(cfg.Service) + log := cfg.Logger.WithName("HoldExternal") // Determine IP which will be used for this LB. If no forwarding rule has been established // or specified in the Service spec, then requestedIP = "". rule := pickForwardingRuleToInferIP(cfg.ExistingRules) - res.IP, err = IPv4ToUse(cfg.Cloud, cfg.Recorder, cfg.Service, rule, subnet) + + switch cfg.IPVersion { + case IPv4Version: + res.IP, err = IPv4ToUse(cfg.Cloud, cfg.Recorder, cfg.Service, rule, cfg.SubnetworkURL) + case IPv6Version: + res.IP, err = IPv6ToUse(cfg.Cloud, cfg.Service, rule, cfg.SubnetworkURL, cfg.Logger) + default: + return res, fmt.Errorf("unsupported IP version: '%s', only IPv4 and IPv6 are supported", cfg.IPVersion) + } + if err != nil { - log.Error(err, "IPv4ToUse for service returned error") + log.Error(err, "IPvXToUse for service returned error") return res, err } + log.V(2).Info("IP for service", "ip", res.IP) // We can't use manager for legacy networks if cfg.Cloud.IsLegacyNetwork() { - return res, nil + return res, err } netTier, isFromAnnotation := annotations.NetworkTier(cfg.Service) @@ -67,8 +78,8 @@ func HoldExternalIPv4(cfg HoldConfig) (HoldResult, error) { addrMgr := NewManager( cfg.Cloud, nm, cfg.Cloud.Region(), - subnet, name, res.IP, - cloud.SchemeExternal, netTier, IPv4Version, cfg.Logger, + cfg.SubnetworkURL, name(cfg), res.IP, + cloud.SchemeExternal, netTier, cfg.IPVersion, cfg.Logger, ) // If network tier annotation in Service Spec is present @@ -124,3 +135,11 @@ func tearDownRulesIfNetworkTierMismatch(deleter ForwardingRuleDeleter, existingR } return nil } + +func name(cfg HoldConfig) string { + name := utils.LegacyForwardingRuleName(cfg.Service) + if cfg.IPVersion == IPv6Version { + name = namer.GetSuffixedName(name, "-ipv6") + } + return name +} diff --git a/pkg/address/hold_test.go b/pkg/address/hold_test.go index 010126c903..36815ef78f 100644 --- a/pkg/address/hold_test.go +++ b/pkg/address/hold_test.go @@ -20,13 +20,17 @@ import ( ) const ( - kubeSystemUID = "ksuid123" - namespace = "test-ns" - name = "test-svc" - tcpName = "k8s2-tcp-axyqjz2d-test-ns-test-svc-pyn67fp6" - udpName = "k8s2-udp-axyqjz2d-test-ns-test-svc-pyn67fp6" - legacyName = "aksuid123" - region = "us-central1" + kubeSystemUID = "ksuid123" + namespace = "test-ns" + name = "test-svc" + legacyName = "aksuid123" + tcpName = "k8s2-tcp-axyqjz2d-test-ns-test-svc-pyn67fp6" + udpName = "k8s2-udp-axyqjz2d-test-ns-test-svc-pyn67fp6" + legacyNameIPv6 = "aksuid123-ipv6" + tcpNameIPv6 = "k8s2-tcp-axyqjz2d-test-ns-test-svc-pyn67fp6-ipv6" + udpNameIPv6 = "k8s2-udp-axyqjz2d-test-ns-test-svc-pyn67fp6-ipv6" + region = "us-central1" + subnetURL = "https://www.googleapis.com/compute/v1/projects/mock-project/regions/us-central1/subnetworks/default" ) func TestHoldExternalIPv4(t *testing.T) { @@ -68,6 +72,40 @@ func TestHoldExternalIPv4(t *testing.T) { Managed: address.IPAddrManaged, }, }, + { + desc: "existing address dual stack IPv4 first", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Annotations: map[string]string{ + "networking.gke.io/load-balancer-ip-addresses": reservedIPv4Name + "," + reservedIPv6Name, + }, + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: reservedIPv4, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "existing address dual stack IPv6 first", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Annotations: map[string]string{ + "networking.gke.io/load-balancer-ip-addresses": reservedIPv6Name + "," + reservedIPv4Name, + }, + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: reservedIPv4, + Managed: address.IPAddrManaged, + }, + }, { desc: "legacy forwarding rule exists", service: &api_v1.Service{ @@ -198,10 +236,10 @@ func TestHoldExternalIPv4(t *testing.T) { t.Run(tC.desc, func(t *testing.T) { t.Parallel() // Arrange - cfg := arrange(t, tC.existingRules, tC.service) + cfg := arrangeHoldExternal(t, tC.existingRules, tC.service, address.IPv4Version, "") // Act - got, err := address.HoldExternalIPv4(cfg) + got, err := address.HoldExternal(cfg) if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -254,19 +292,269 @@ func TestHoldExternalIPv4(t *testing.T) { } } -func arrange(t *testing.T, existingRules []*composite.ForwardingRule, service *api_v1.Service) address.HoldConfig { +func TestHoldExternalIPv6(t *testing.T) { + testCases := []struct { + desc string + existingRules []*composite.ForwardingRule + service *api_v1.Service + want address.HoldResult + wantTearDown bool + }{ + { + desc: "no address passed", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: notReservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "existing address", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Annotations: map[string]string{ + "networking.gke.io/load-balancer-ip-addresses": reservedIPv6Name, + }, + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: reservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "existing address dual stack IPv4 first", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Annotations: map[string]string{ + "networking.gke.io/load-balancer-ip-addresses": reservedIPv4Name + "," + reservedIPv6Name, + }, + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: reservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "existing address dual stack IPv6 first", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Annotations: map[string]string{ + "networking.gke.io/load-balancer-ip-addresses": reservedIPv6Name + "," + reservedIPv4Name, + }, + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + want: address.HoldResult{ + IP: reservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "legacy forwarding rule exists", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + existingRules: []*composite.ForwardingRule{ + { + Name: legacyNameIPv6, + IPAddress: notReservedIPv6, + Region: region, + }, + }, + want: address.HoldResult{ + IP: notReservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "mixed protocol forwarding rule exist", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + }, + }, + existingRules: []*composite.ForwardingRule{ + { + Name: tcpNameIPv6, + IPAddress: notReservedIPv6, + Region: region, + }, + { + Name: udpNameIPv6, + IPAddress: notReservedIPv6, + Region: region, + }, + }, + want: address.HoldResult{ + IP: notReservedIPv6, + Managed: address.IPAddrManaged, + }, + }, + { + desc: "tier from annotation: Premium to Premium", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + Annotations: map[string]string{ + annotations.NetworkTierAnnotationKey: "Premium", + }, + }, + }, + existingRules: []*composite.ForwardingRule{ + { + Name: legacyNameIPv6, + IPAddress: notReservedIPv6, + Region: region, + NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), + }, + }, + want: address.HoldResult{ + IP: notReservedIPv6, + Managed: address.IPAddrManaged, + }, + wantTearDown: false, + }, + { + // While this shouldn't be possible + // There was a bug that allowed to create Standard tier IPv6 Forwarding rules + desc: "tier from annotation: Standard to Premium", + service: &api_v1.Service{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: kubeSystemUID, + Annotations: map[string]string{ + annotations.NetworkTierAnnotationKey: "Premium", + }, + }, + }, + existingRules: []*composite.ForwardingRule{ + { + Name: legacyNameIPv6, + IPAddress: notReservedIPv6, + Region: region, + NetworkTier: cloud.NetworkTierStandard.ToGCEValue(), + }, + }, + want: address.HoldResult{ + IP: notReservedIPv6, + Managed: address.IPAddrManaged, + }, + wantTearDown: true, + }, + } + for _, tC := range testCases { + t.Run(tC.desc, func(t *testing.T) { + t.Parallel() + // Arrange + cfg := arrangeHoldExternal(t, tC.existingRules, tC.service, address.IPv6Version, subnetURL) + + // Act + got, err := address.HoldExternal(cfg) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + + // Assert + if got.IP != tC.want.IP { + t.Errorf("address.HoldExternal(_).IP = %q, want %q", got.IP, tC.want.IP) + } + if got.Managed != tC.want.Managed { + t.Errorf("address.HoldExternal(_).Managed = %q, want %q", got.Managed, tC.want.Managed) + } + + // Verify that address exists + region := cfg.Cloud.Region() + addr, err := cfg.Cloud.GetRegionAddressByIP(region, got.IP) + if err != nil || addr == nil { + t.Fatalf("unexpected err: %v", err) + } + if addr.Subnetwork != subnetURL { + t.Errorf("addr.Subnetwork = %q, want %q", addr.Subnetwork, subnetURL) + } + + // Check if release cleans up address + if got.Release == nil { + t.Fatal("address.HoldExternal(_).Release is nil") + } + if err := got.Release(); err != nil { + t.Fatalf("unexpected err: %v", err) + } + addr, err = cfg.Cloud.GetRegionAddress(legacyNameIPv6, region) + if utils.IgnoreHTTPNotFound(err) != nil { + t.Errorf("GetRegionAddress returned err: %v", err) + } + if addr != nil { + t.Errorf("address %v not deleted", legacyNameIPv6) + } + + // Check teardown + for _, rule := range tC.existingRules { + if rule != nil { + got, err := cfg.Cloud.GetRegionForwardingRule(rule.Name, rule.Region) + if utils.IgnoreHTTPNotFound(err) != nil { + t.Errorf("GetRegionForwardingRule returned unexpected err: %v", err) + continue + } + wasDeleted := got == nil + if wasDeleted != tC.wantTearDown { + t.Errorf("%v deleted = %v, want %v", rule.Name, wasDeleted, tC.wantTearDown) + } + } + } + }) + } +} + +func arrangeHoldExternal(t *testing.T, existingRules []*composite.ForwardingRule, service *api_v1.Service, ipVersion address.IPVersion, subnet string) address.HoldConfig { t.Helper() vals := gce.DefaultTestClusterValues() fakeGCE := gce.NewFakeGCECloud(vals) - addr := &compute.Address{ + if err := fakeGCE.ReserveRegionAddress(&compute.Address{ Name: reservedIPv4Name, Region: vals.Region, Address: reservedIPv4, AddressType: "EXTERNAL", + IpVersion: "IPV4", + Subnetwork: subnet, + }, vals.Region); err != nil { + t.Fatal(err) } - if err := fakeGCE.ReserveRegionAddress(addr, vals.Region); err != nil { + if err := fakeGCE.ReserveRegionAddress(&compute.Address{ + Name: reservedIPv6Name, + Region: vals.Region, + Address: reservedIPv6, + AddressType: "EXTERNAL", + IpVersion: "IPV6", + NetworkTier: "PREMIUM", + Subnetwork: subnet, + }, vals.Region); err != nil { t.Fatal(err) } @@ -277,7 +565,12 @@ func arrange(t *testing.T, existingRules []*composite.ForwardingRule, service *a if obj, ok := m.Objects[*key]; ok { ga := obj.ToGA() if ga.Address == "" { - ga.Address = notReservedIPv4 + isIPv4 := ga.IpVersion == "IPV4" || ga.IpVersion == "" + if isIPv4 { + ga.Address = notReservedIPv4 + } else { + ga.Address = notReservedIPv6 + } } m.Objects[*key] = m.Obj(ga) return true, obj.ToGA(), nil @@ -298,5 +591,7 @@ func arrange(t *testing.T, existingRules []*composite.ForwardingRule, service *a ExistingRules: existingRules, Service: service, ForwardingRuleDeleter: provider, + IPVersion: ipVersion, + SubnetworkURL: subnet, } } diff --git a/pkg/forwardingrules/netlb_mixed_manager.go b/pkg/forwardingrules/netlb_mixed_manager.go index d7400c9183..d12f12e5eb 100644 --- a/pkg/forwardingrules/netlb_mixed_manager.go +++ b/pkg/forwardingrules/netlb_mixed_manager.go @@ -79,13 +79,14 @@ func (m *MixedManagerNetLB) EnsureIPv4(backendServiceLink string) (EnsureNetLBRe return res, err } - addressHandle, err := address.HoldExternalIPv4(address.HoldConfig{ + addressHandle, err := address.HoldExternal(address.HoldConfig{ Cloud: m.Cloud, Recorder: m.Recorder, Logger: m.Logger, Service: m.Service, ExistingRules: []*composite.ForwardingRule{existing.Legacy, existing.TCP, existing.UDP}, ForwardingRuleDeleter: m.Provider, + IPVersion: address.IPv4Version, }) if err != nil { return res, err diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 8142da0b2c..96e7c88db1 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -349,13 +349,14 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw return nil, address.IPAddrUndefined, utils.ResourceResync, err } - addrHandle, err := address.HoldExternalIPv4(address.HoldConfig{ + addrHandle, err := address.HoldExternal(address.HoldConfig{ Cloud: l4netlb.cloud, Recorder: l4netlb.recorder, Logger: l4netlb.svcLogger, Service: l4netlb.Service, ExistingRules: []*composite.ForwardingRule{rules.Legacy, rules.TCP, rules.UDP}, ForwardingRuleDeleter: l4netlb.forwardingRules, + IPVersion: address.IPv4Version, }) if err != nil { frLogger.Error(err, "address.HoldExternalIPv4 returned error") @@ -478,17 +479,6 @@ func (l4netlb *L4NetLB) createFwdRule(newFr *composite.ForwardingRule, frLogger return nil } -// tearDownResourcesWithWrongNetworkTier removes forwarding rule or IP address if its Network Tier differs from desired. -func (l4netlb *L4NetLB) tearDownResourcesWithWrongNetworkTier(existingFwdRule *composite.ForwardingRule, svcNetTier cloud.NetworkTier, am *address.Manager, frLogger klog.Logger) error { - if existingFwdRule != nil && existingFwdRule.NetworkTier != svcNetTier.ToGCEValue() { - err := l4netlb.forwardingRules.Delete(existingFwdRule.Name) - if err != nil { - frLogger.Error(err, "l4netlb.forwardingRules.Delete returned error, want nil") - } - } - return am.TearDownAddressIPIfNetworkTierMismatch() -} - func isAddressAlreadyInUseError(err error) bool { // Bad request HTTP status (400) is returned for external Forwarding Rules. alreadyInUseExternal := utils.IsHTTPErrorCode(err, http.StatusBadRequest) && strings.Contains(err.Error(), addressAlreadyInUseMessageExternal) diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index fcdec74a07..e251f43a50 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -25,7 +25,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/address" "k8s.io/ingress-gce/pkg/annotations" @@ -167,15 +166,6 @@ func (l4netlb *L4NetLB) ensureIPv6ForwardingRule(bsLink string) (*composite.Forw } frLogger.V(2).Info("subnetworkURL for service", "subnetworkURL", subnetworkURL) - // Determine IP which will be used for this LB. If no forwarding rule has been established - // or specified in the Service spec, then requestedIP = "". - ipv6AddrToUse, err := address.IPv6ToUse(l4netlb.cloud, l4netlb.Service, existingIPv6FwdRule, subnetworkURL, frLogger) - if err != nil { - frLogger.Error(err, "address.IPv6ToUse for service returned error") - return nil, utils.ResourceResync, err - } - frLogger.V(2).Info("ipv6AddressToUse for service", "ipv6AddressToUse", ipv6AddrToUse) - netTier, isFromAnnotation := annotations.NetworkTier(l4netlb.Service) frLogger.V(2).Info("network tier for service", "networkTier", netTier, "isFromAnnotation", isFromAnnotation) @@ -185,33 +175,35 @@ func (l4netlb *L4NetLB) ensureIPv6ForwardingRule(bsLink string) (*composite.Forw return nil, utils.ResourceResync, utils.NewUnsupportedNetworkTierErr(resourceErr, string(cloud.NetworkTierStandard)) } - // Only for IPv6, address reservation is not supported on Standard Tier - if !l4netlb.cloud.IsLegacyNetwork() && netTier == cloud.NetworkTierPremium { - nm := types.NamespacedName{Namespace: l4netlb.Service.Namespace, Name: l4netlb.Service.Name}.String() - addrMgr := address.NewManager(l4netlb.cloud, nm, l4netlb.cloud.Region(), subnetworkURL, expectedIPv6FrName, ipv6AddrToUse, cloud.SchemeExternal, netTier, address.IPv6Version, frLogger) - - // If network tier annotation in Service Spec is present - // check if it matches network tiers from forwarding rule and external ip Address. - // If they do not match, tear down the existing resources with the wrong tier. - if isFromAnnotation { - if err := l4netlb.tearDownResourcesWithWrongNetworkTier(existingIPv6FwdRule, netTier, addrMgr, frLogger); err != nil { - return nil, utils.ResourceResync, err - } - } - - ipv6AddrToUse, _, err = addrMgr.HoldAddress() + addrHandle, err := address.HoldExternal(address.HoldConfig{ + Cloud: l4netlb.cloud, + Recorder: l4netlb.recorder, + Logger: l4netlb.svcLogger, + Service: l4netlb.Service, + ExistingRules: []*composite.ForwardingRule{existingIPv6FwdRule}, + ForwardingRuleDeleter: l4netlb.forwardingRules, + IPVersion: IPVersionIPv6, + SubnetworkURL: subnetworkURL, + }) + if err != nil { + frLogger.Error(err, "address.HoldExternal returned error") + return nil, utils.ResourceResync, err + } + frLogger.V(2).Info("ipv6AddressToUse for service", "ipv6AddressToUse", addrHandle.IP) + ipv6AddrToUse := addrHandle.IP + defer func() { + // Release the address that was reserved, in all cases. If the forwarding rule was successfully created, + // the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks. + err = addrHandle.Release() if err != nil { - return nil, utils.ResourceResync, err + frLogger.Error(err, "addrHandle.Release returned error: failed to release address reservation, possibly causing an orphan") } - frLogger.V(2).Info("ensureIPv6ForwardingRule: reserved IP for the forwarding rule", "ip", ipv6AddrToUse) - defer func() { - // Release the address that was reserved, in all cases. If the forwarding rule was successfully created, - // the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks. - if err := addrMgr.ReleaseAddress(); err != nil { - frLogger.Error(err, "ensureIPv6ForwardingRule: failed to release address reservation, possibly causing an orphan") - } - }() - } else if existingIPv6FwdRule != nil && existingIPv6FwdRule.NetworkTier != netTier.ToGCEValue() { + }() + + // If there is an IPv6 Standard Forwarding Rule, tear it down. + // This shouldn't be possible, but there was a bug that allowed this in the past. + if (l4netlb.cloud.IsLegacyNetwork() || netTier != cloud.NetworkTierPremium) && + existingIPv6FwdRule != nil && existingIPv6FwdRule.NetworkTier != netTier.ToGCEValue() { frLogger.V(2).Info("deleting forwarding rule for service due to network tier mismatch", "existingTier", existingIPv6FwdRule.NetworkTier, "expectedTier", netTier) err := l4netlb.forwardingRules.Delete(existingIPv6FwdRule.Name) if err != nil {