From dd4db7bdab73755c4caade2edee7ef1430b35606 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Fri, 26 May 2023 21:06:44 +0000 Subject: [PATCH 1/2] Reserve Static IPv6 address before syncing L4 ILB This will prevent losing address while recreating forwarding rule --- go.mod | 5 +- go.sum | 10 +- pkg/loadbalancers/address_manager.go | 72 +++++- pkg/loadbalancers/address_manager_test.go | 70 +++++- pkg/loadbalancers/forwarding_rules.go | 4 +- pkg/loadbalancers/forwarding_rules_ipv6.go | 29 ++- pkg/loadbalancers/l4.go | 100 +++++---- pkg/loadbalancers/l4_test.go | 8 +- pkg/loadbalancers/l4ipv6.go | 17 +- .../k8s-cloud-provider/pkg/cloud/mock/mock.go | 11 +- .../k8s-cloud-provider/pkg/cloud/observe.go | 8 +- .../k8s-cloud-provider/pkg/cloud/utils.go | 70 ++++-- vendor/github.com/kr/pretty/diff.go | 32 ++- vendor/github.com/kr/pretty/formatter.go | 17 +- .../github.com/rogpeppe/go-internal/LICENSE | 27 +++ .../rogpeppe/go-internal/fmtsort/mapelem.go | 23 ++ .../go-internal/fmtsort/mapelem_1.11.go | 24 ++ .../rogpeppe/go-internal/fmtsort/sort.go | 210 ++++++++++++++++++ vendor/modules.txt | 7 +- 19 files changed, 636 insertions(+), 108 deletions(-) create mode 100644 vendor/github.com/rogpeppe/go-internal/LICENSE create mode 100644 vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem.go create mode 100644 vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem_1.11.go create mode 100644 vendor/github.com/rogpeppe/go-internal/fmtsort/sort.go diff --git a/go.mod b/go.mod index 2c0040e324..4fa3cd50af 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,9 @@ module k8s.io/ingress-gce go 1.20 require ( - github.com/GoogleCloudPlatform/k8s-cloud-provider v1.23.0 + github.com/GoogleCloudPlatform/k8s-cloud-provider v1.24.0 github.com/google/go-cmp v0.5.9 - github.com/kr/pretty v0.2.0 + github.com/kr/pretty v0.3.0 github.com/prometheus/client_golang v1.14.0 github.com/prometheus/client_model v0.3.0 github.com/spf13/cobra v1.6.0 @@ -64,6 +64,7 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/common v0.37.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect + github.com/rogpeppe/go-internal v1.9.0 // indirect go.opencensus.io v0.24.0 // indirect golang.org/x/crypto v0.1.0 // indirect golang.org/x/net v0.9.0 // indirect diff --git a/go.sum b/go.sum index 3b3c32aa78..600f9cdb7f 100644 --- a/go.sum +++ b/go.sum @@ -39,8 +39,8 @@ cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= -github.com/GoogleCloudPlatform/k8s-cloud-provider v1.23.0 h1:t7VxYo6K75axT6J43S/QAOhnFChl5ul5umel4FWFW+M= -github.com/GoogleCloudPlatform/k8s-cloud-provider v1.23.0/go.mod h1:cBEoHknnCbHPtbZIkqPCCTlKeo/7ehAJcwOMQOlV1kk= +github.com/GoogleCloudPlatform/k8s-cloud-provider v1.24.0 h1:a/2jEb6YZq88x+36TcyUu9widTOcGlndZc4L54r+Jys= +github.com/GoogleCloudPlatform/k8s-cloud-provider v1.24.0/go.mod h1:SZTgFVlGGwQ/X9NQsExTrYerE+frOeQvIuHCTuAGODI= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= @@ -210,8 +210,9 @@ github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxv github.com/konsorten/go-windows-terminal-sequences v1.0.3/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= -github.com/kr/pretty v0.2.0 h1:s5hAObm+yFO5uHYt5dYjxi2rXrsnmRpJx4OYvIWUaQs= github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= +github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= @@ -272,6 +273,9 @@ github.com/prometheus/procfs v0.8.0 h1:ODq8ZFEaYeCaZOJlZZdJA2AbQR98dSHSM1KW/You5 github.com/prometheus/procfs v0.8.0/go.mod h1:z7EfXMXOkbkqb9IINtpCn86r/to3BnA0uaxHdg830/4= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= +github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= +github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= +github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo= github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE= diff --git a/pkg/loadbalancers/address_manager.go b/pkg/loadbalancers/address_manager.go index 5e8b928849..9c4493527a 100644 --- a/pkg/loadbalancers/address_manager.go +++ b/pkg/loadbalancers/address_manager.go @@ -18,13 +18,13 @@ package loadbalancers import ( "fmt" + "net" "net/http" + compute "google.golang.org/api/compute/v1" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/utils" - compute "google.golang.org/api/compute/v1" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "k8s.io/klog/v2" ) @@ -38,6 +38,21 @@ const ( IPAddrUnmanaged ) +// IPVersion represents compute.Address IpVersion field +type IPVersion = string + +const ( + IPv4Version IPVersion = "IPV4" + IPv6Version IPVersion = "IPV6" + // IPv6LBAddressPrefixLength used for reserving IPv6 addresses. + // Google Cloud reserves not a single IPv6 address, but a /96 range. + // At the moment, no other ranges are supported + IPv6LBAddressPrefixLength = 96 + // IPv6EndpointTypeNetLB is a value on Address.Ipv6EndpointType used specify + // that IPv6 address will be used for NetLB. Required for new IPv6 NetLB address creation. + IPv6EndpointTypeNetLB = "NETLB" +) + // Original file in https://github.com/kubernetes/legacy-cloud-providers/blob/6aa80146c33550e908aed072618bd7f9998837f6/gce/gce_address_manager.go type addressManager struct { logPrefix string @@ -50,9 +65,17 @@ type addressManager struct { subnetURL string tryRelease bool networkTier cloud.NetworkTier + ipVersion IPVersion } -func newAddressManager(svc gce.CloudAddressService, serviceName, region, subnetURL, name, targetIP string, addressType cloud.LbScheme, networkTier cloud.NetworkTier) *addressManager { +func newAddressManager(svc gce.CloudAddressService, serviceName, region, subnetURL, name, targetIP string, addressType cloud.LbScheme, networkTier cloud.NetworkTier, ipVersion IPVersion) *addressManager { + if targetIP != "" { + // Store address in normalized format. + // This is required for IPv6 addresses, to be able to filter by exact address, + // because filtering is happening by regexp and google cloud stores address in the shortest form. + targetIP = net.ParseIP(targetIP).String() + } + // We don't have verification that targetIP matches IPVersion, we will rely on Google cloud API verification return &addressManager{ svc: svc, logPrefix: fmt.Sprintf("AddressManager(%q)", name), @@ -64,6 +87,7 @@ func newAddressManager(svc gce.CloudAddressService, serviceName, region, subnetU tryRelease: true, subnetURL: subnetURL, networkTier: networkTier, + ipVersion: ipVersion, } } @@ -148,6 +172,29 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err newAddr.NetworkTier = am.networkTier.ToGCEValue() } + // This block mitigates inconsistencies in the gcloud API, as revealed through experimentation. + if am.ipVersion == IPv6Version { + // Empty targetIP covers reserving new static address. + if am.targetIP == "" { + // The IpVersion should be set to IPv6 only when creating a new IPv6 address. + // Setting it to IPv4 or setting it while promoting an existing load balancer IPv6 address to static can cause errors. + newAddr.IpVersion = IPv6Version + if am.addressType == cloud.SchemeExternal { + // Ipv6EndpointType is required only when creating a new external IPv6 address. + newAddr.Ipv6EndpointType = IPv6EndpointTypeNetLB + } + } else { + // Non-empty targetIP covers promoting existent load-balancer IP to static one. + + // PrefixLength is only required when converting an existing IPv6 load balancer address to a static one. + newAddr.PrefixLength = IPv6LBAddressPrefixLength + if am.addressType == cloud.SchemeExternal { + // Specifying a Subnetwork will return error when promoting an existing external IPv6 forwarding rule IP to static. + newAddr.Subnetwork = "" + } + } + } + reserveErr := am.svc.ReserveRegionAddress(newAddr, am.region) if reserveErr == nil { if newAddr.Address != "" { @@ -165,6 +212,8 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err return addr.Address, IPAddrManaged, nil } + klog.V(2).Infof("Address reserve error: %v", reserveErr) + if utils.IsNetworkTierMismatchGCEError(reserveErr) { receivedNetworkTier := cloud.NetworkTierPremium if receivedNetworkTier == am.networkTier { @@ -178,10 +227,13 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err return "", IPAddrUndefined, networkTierError } - if !utils.IsHTTPErrorCode(reserveErr, http.StatusConflict) && !utils.IsHTTPErrorCode(reserveErr, http.StatusBadRequest) { + if !utils.IsHTTPErrorCode(reserveErr, http.StatusConflict) && + !utils.IsHTTPErrorCode(reserveErr, http.StatusBadRequest) && + !utils.IsHTTPErrorCode(reserveErr, http.StatusPreconditionFailed) { // If the IP is already reserved: // by an internal address: a StatusConflict is returned // by an external address: a BadRequest is returned + // by an external IPv6 address: a 412 Precondition not met error is returned return "", IPAddrUndefined, reserveErr } @@ -220,7 +272,7 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err } func (am *addressManager) validateAddress(addr *compute.Address) error { - if am.targetIP != "" && am.targetIP != addr.Address { + if am.targetIP != "" && !IsSameIP(am.targetIP, addr.Address) { return fmt.Errorf("IP mismatch, expected %q, actual: %q", am.targetIP, addr.Address) } if addr.AddressType != string(am.addressType) { @@ -232,6 +284,16 @@ func (am *addressManager) validateAddress(addr *compute.Address) error { return nil } +func IsSameIP(ip1 string, ip2 string) bool { + parsedIP1 := net.ParseIP(ip1) + parsedIP2 := net.ParseIP(ip2) + if parsedIP1 == nil || parsedIP2 == nil { + return false + } + + return parsedIP1.Equal(parsedIP2) +} + func (am *addressManager) isManagedAddress(addr *compute.Address) bool { return addr.Name == am.name } diff --git a/pkg/loadbalancers/address_manager_test.go b/pkg/loadbalancers/address_manager_test.go index 57f827f3b1..2425c5c9fc 100644 --- a/pkg/loadbalancers/address_manager_test.go +++ b/pkg/loadbalancers/address_manager_test.go @@ -17,6 +17,7 @@ limitations under the License. package loadbalancers import ( + "net" "testing" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" @@ -42,7 +43,7 @@ func TestAddressManagerNoRequestedIP(t *testing.T) { require.NoError(t, err) targetIP := "" - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -53,7 +54,7 @@ func TestAddressManagerBasic(t *testing.T) { require.NoError(t, err) targetIP := "1.1.1.1" - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -69,7 +70,7 @@ func TestAddressManagerOrphaned(t *testing.T) { err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -81,7 +82,7 @@ func TestAddressManagerStandardNetworkTier(t *testing.T) { require.NoError(t, err) targetIP := "1.1.1.1" - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierStandard) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierStandard, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeExternal), cloud.NetworkTierStandard.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -92,7 +93,7 @@ func TestAddressManagerStandardNetworkTierNotAvailableForInternalAddress(t *test require.NoError(t, err) targetIP := "1.1.1.1" - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierStandard) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierStandard, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierPremium.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -109,7 +110,7 @@ func TestAddressManagerOutdatedOrphan(t *testing.T) { err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv4Version) testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } @@ -125,7 +126,7 @@ func TestAddressManagerExternallyOwned(t *testing.T) { err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) ipToUse, ipType, err := mgr.HoldAddress() require.NoError(t, err) assert.NotEmpty(t, ipToUse) @@ -145,7 +146,7 @@ func TestAddressManagerNonExisting(t *testing.T) { require.NoError(t, err) targetIP := "1.1.1.1" - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, IPv4Version) svc.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = test.InsertAddressNotAllocatedToProjectErrorHook _, _, err = mgr.HoldAddress() @@ -162,7 +163,7 @@ func TestAddressManagerWrongTypeReserved(t *testing.T) { addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeInternal)} err = svc.ReserveRegionAddress(addr, vals.Region) - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, IPv4Version) _, _, err = mgr.HoldAddress() require.Error(t, err) @@ -179,7 +180,7 @@ func TestAddressManagerExternallyOwnedWrongNetworkTier(t *testing.T) { addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeInternal), NetworkTier: string(cloud.NetworkTierStandard)} err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err, "") - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) svc.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = test.InsertAddressNetworkErrorHook _, _, err = mgr.HoldAddress() if err == nil || !utils.IsNetworkTierError(err) { @@ -198,12 +199,56 @@ func TestAddressManagerBadExternallyOwned(t *testing.T) { err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) ad, _, err := mgr.HoldAddress() assert.NotNil(t, err) // FIXME require.Equal(t, ad, "") } +// TestAddressManagerIPv6 tests the typical case of reserving and releasing an IPv6 address. +func TestAddressManagerIPv6(t *testing.T) { + testCases := []struct { + desc string + targetIP string + }{ + { + desc: "Defined IPv6 address", + targetIP: "1111:2222:3333:4444:5555:0:0:0", + }, + { + desc: "Empty IPv6 address", + targetIP: "", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + svc, err := fakeGCECloud(vals) + if err != nil { + t.Fatalf("fakeGCECloud(%v) returned error %v", vals, err) + } + + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, tc.targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv6Version) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, tc.targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) + }) + } +} + +// TestAddressManagerEmptyIPv6 tests the typical case of reserving and releasing an IPv6 address. +func TestAddressManagerEmptyIPv6(t *testing.T) { + svc, err := fakeGCECloud(vals) + require.NoError(t, err) + targetIP := "" + + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv6Version) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) +} + func testHoldAddress(t *testing.T, mgr *addressManager, svc gce.CloudAddressService, name, region, targetIP, scheme, netTier string) { ipToUse, ipType, err := mgr.HoldAddress() require.NoError(t, err) @@ -213,7 +258,8 @@ func testHoldAddress(t *testing.T, mgr *addressManager, svc gce.CloudAddressServ addr, err := svc.GetRegionAddress(name, region) require.NoError(t, err) if targetIP != "" { - assert.EqualValues(t, targetIP, addr.Address) + expectedTargetIP := net.ParseIP(targetIP).String() + assert.EqualValues(t, expectedTargetIP, addr.Address) } assert.EqualValues(t, scheme, addr.AddressType) assert.EqualValues(t, addr.NetworkTier, netTier) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index bad04ac266..ee58eb62d2 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -314,10 +314,10 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw // If the network is not a legacy network, use the address manager if !l4netlb.cloud.IsLegacyNetwork() { nm := types.NamespacedName{Namespace: l4netlb.Service.Namespace, Name: l4netlb.Service.Name}.String() - addrMgr := newAddressManager(l4netlb.cloud, nm, l4netlb.cloud.Region() /*subnetURL = */, "", frName, ipToUse, cloud.SchemeExternal, netTier) + addrMgr := newAddressManager(l4netlb.cloud, nm, l4netlb.cloud.Region() /*subnetURL = */, "", frName, ipToUse, cloud.SchemeExternal, netTier, IPv4Version) // If network tier annotation in Service Spec is present - // check if it match network tiers from forwarding rule and external ip Address. + // 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(existingFwdRule, netTier, addrMgr); err != nil { diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index d496c3b527..bbd945c05b 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -18,6 +18,7 @@ package loadbalancers import ( "fmt" + "strings" "time" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -37,12 +38,12 @@ const ( IPVersionIPv6 = "IPV6" ) -func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { +func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, existingIPv6FwdRule *composite.ForwardingRule, ipv6ToUse string) (*composite.ForwardingRule, error) { start := time.Now() - expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options) + expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options, ipv6ToUse) if err != nil { - return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v) returned error %w, want nil", bsLink, options, err) + return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v, %s) returned error %w, want nil", bsLink, options, ipv6ToUse, err) } klog.V(2).Infof("Ensuring internal ipv6 forwarding rule %s for L4 ILB Service %s/%s, backend service link: %s", expectedIPv6FwdRule.Name, l4.Service.Namespace, l4.Service.Name, bsLink) @@ -50,11 +51,6 @@ func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (* klog.V(2).Infof("Finished ensuring internal ipv6 forwarding rule %s for L4 ILB Service %s/%s, time taken: %v", expectedIPv6FwdRule.Name, l4.Service.Namespace, l4.Service.Name, time.Since(start)) }() - existingIPv6FwdRule, err := l4.forwardingRules.Get(expectedIPv6FwdRule.Name) - if err != nil { - return nil, fmt.Errorf("l4.forwardingRules.GetForwardingRule(%s) returned error %w, want nil", expectedIPv6FwdRule.Name, err) - } - if existingIPv6FwdRule != nil { equal, err := EqualIPv6ForwardingRules(existingIPv6FwdRule, expectedIPv6FwdRule) if err != nil { @@ -80,7 +76,7 @@ func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (* return createdFr, err } -func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { +func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ipv6ToUse string) (*composite.ForwardingRule, error) { frName := l4.getIPv6FRName() frDesc, err := utils.MakeL4IPv6ForwardingRuleDescription(l4.Service) @@ -104,6 +100,7 @@ func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOpti fr := &composite.ForwardingRule{ Name: frName, Description: frDesc, + IPAddress: ipv6ToUse, IPProtocol: string(protocol), Ports: ports, LoadBalancingScheme: string(cloud.SchemeInternal), @@ -248,3 +245,17 @@ func EqualIPv6ForwardingRules(fr1, fr2 *composite.ForwardingRule) (bool, error) fr1.Subnetwork == fr2.Subnetwork && fr1.NetworkTier == fr2.NetworkTier, nil } + +func ipv6IPToUse(ipv6FwdRule *composite.ForwardingRule, requestedSubnet string) string { + if ipv6FwdRule == nil { + return "" + } + if requestedSubnet != ipv6FwdRule.Subnetwork { + // reset ip address since subnet is being changed. + return "" + } + + // Google Cloud creates ipv6 forwarding rules with IPAddress in CIDR form, + // but to create static address you need to specify address without range + return strings.Split(ipv6FwdRule.IPAddress, "/")[0] +} diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index e7b9c59da7..d4a9600095 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -309,56 +309,87 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service options := l4.getILBOptions() subnetworkURL, err := l4.getServiceSubnetworkURL(options) + + bsName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) + existingBS, err := l4.backendPool.Get(bsName, meta.VersionGA, l4.scope) + if utils.IgnoreHTTPNotFound(err) != nil { + klog.Errorf("Failed to lookup existing backend service, ignoring err: %v", err) + } + // Reserve existing IP address before making any changes - var existingFR *composite.ForwardingRule + var existingIPv4FR *composite.ForwardingRule var ipv4ToUse string if !l4.enableDualStack || utils.NeedsIPv4(l4.Service) { - existingFR, err = l4.getOldForwardingRule() - ipv4ToUse = l4lbIPToUse(l4.Service, existingFR, subnetworkURL) + existingIPv4FR, err = l4.getOldIPv4ForwardingRule(existingBS) + ipv4ToUse = l4lbIPToUse(l4.Service, existingIPv4FR, subnetworkURL) expectedFRName := l4.GetFRName() if !l4.cloud.IsLegacyNetwork() { nm := types.NamespacedName{Namespace: l4.Service.Namespace, Name: l4.Service.Name}.String() // ILB can be created only in Premium Tier - addrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedFRName, ipv4ToUse, cloud.SchemeInternal, cloud.NetworkTierPremium) + addrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedFRName, ipv4ToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) ipv4ToUse, _, err = addrMgr.HoldAddress() if err != nil { result.Error = fmt.Errorf("EnsureInternalLoadBalancer error: addrMgr.HoldAddress() returned error %w", err) return result } - klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IP %q", nm, ipv4ToUse) + klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv4 address %q", nm, ipv4ToUse) 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 { - klog.Errorf("EnsureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err) + klog.Errorf("EnsureInternalLoadBalancer: failed to release IPv4 address reservation, possibly causing an orphan: %v", err) + } + }() + } + } + + // Reserve existing IPv6 address before making any changes + var existingIPv6FR *composite.ForwardingRule + var ipv6ToUse string + if l4.enableDualStack && utils.NeedsIPv6(l4.Service) { + existingIPv6FR, err = l4.getOldIPv6ForwardingRule(existingBS) + ipv6ToUse = ipv6IPToUse(existingIPv6FR, subnetworkURL) + expectedIPv6FRName := l4.getIPv6FRName() + if !l4.cloud.IsLegacyNetwork() { + nm := types.NamespacedName{Namespace: l4.Service.Namespace, Name: l4.Service.Name}.String() + // ILB can be created only in Premium Tier + ipv6AddrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedIPv6FRName, ipv6ToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv6Version) + ipv6ToUse, _, err = ipv6AddrMgr.HoldAddress() + if err != nil { + result.Error = fmt.Errorf("EnsureInternalLoadBalancer error: ipv6AddrMgr.HoldAddress() returned error %w", err) + return result + } + klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv6 address %q", nm, ipv6ToUse) + 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 := ipv6AddrMgr.ReleaseAddress(); err != nil { + klog.Errorf("EnsureInternalLoadBalancer: failed to release IPv6 address reservation, possibly causing an orphan: %v", err) } }() } } - // if Service protocol changed, we must delete forwarding rule before changing backend service, - // otherwise, on updating backend service, google cloud api will return error - bsName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) - existingBS, err := l4.backendPool.Get(bsName, meta.VersionGA, l4.scope) - if utils.IgnoreHTTPNotFound(err) != nil { - klog.Errorf("Failed to lookup existing backend service, ignoring err: %v", err) - } + + // if Service protocol changed, we must delete forwarding rule before changing backend service, + // otherwise, on updating backend service, google cloud api will return error if existingBS != nil && existingBS.Protocol != string(protocol) { - if existingFR != nil { - err = l4.forwardingRules.Delete(existingFR.Name) + klog.Infof("Protocol changed from %q to %q for service %s", existingBS.Protocol, string(protocol), l4.NamespacedName) + if existingIPv4FR != nil { + // Delete ipv4 forwarding rule if it exists + err = l4.forwardingRules.Delete(existingIPv4FR.Name) if err != nil { - klog.Errorf("Failed to delete forwarding rule %s, err %v", existingFR.Name, err) + klog.Errorf("Failed to delete forwarding rule %s, err %v", existingIPv4FR.Name, err) } } - if l4.enableDualStack { + if l4.enableDualStack && existingIPv6FR != nil { // Delete ipv6 forwarding rule if it exists - oldIPv6FrName := l4.getIPv6FRNameWithProtocol(existingBS.Protocol) - err = l4.forwardingRules.Delete(oldIPv6FrName) + err = l4.forwardingRules.Delete(existingIPv6FR.Name) if err != nil { - klog.Errorf("Failed to delete ipv6 forwarding rule %s, err %v", oldIPv6FrName, err) + klog.Errorf("Failed to delete ipv6 forwarding rule %s, err %v", existingIPv6FR.Name, err) } } } @@ -373,9 +404,9 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service result.Annotations[annotations.BackendServiceKey] = bsName if l4.enableDualStack { - l4.ensureDualStackResources(result, nodeNames, options, bs, existingFR, subnetworkURL, ipv4ToUse) + l4.ensureDualStackResources(result, nodeNames, options, bs, existingIPv4FR, existingIPv6FR, subnetworkURL, ipv4ToUse, ipv6ToUse) } else { - l4.ensureIPv4Resources(result, nodeNames, options, bs, existingFR, subnetworkURL, ipv4ToUse) + l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FR, subnetworkURL, ipv4ToUse) } if result.Error != nil { return result @@ -437,14 +468,14 @@ func (l4 *L4) provideIPv4HealthChecks(nodeNames []string, result *L4ILBSyncResul return hcResult.HCLink } -func (l4 *L4) ensureDualStackResources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingFR *composite.ForwardingRule, subnetworkURL, ipToUse string) { +func (l4 *L4) ensureDualStackResources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingIPv4FwdRule, existingIPv6FwdRule *composite.ForwardingRule, subnetworkURL, ipv4ToUse, ipv6ToUse string) { if utils.NeedsIPv4(l4.Service) { - l4.ensureIPv4Resources(result, nodeNames, options, bs, existingFR, subnetworkURL, ipToUse) + l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FwdRule, subnetworkURL, ipv4ToUse) } else { l4.deleteIPv4ResourcesOnSync(result) } if utils.NeedsIPv6(l4.Service) { - l4.ensureIPv6Resources(result, nodeNames, options, bs.SelfLink) + l4.ensureIPv6Resources(result, nodeNames, options, bs.SelfLink, existingIPv6FwdRule, ipv6ToUse) } else { l4.deleteIPv6ResourcesOnSync(result) } @@ -541,23 +572,14 @@ func (l4 *L4) hasAnnotation(annotationKey string) bool { return false } -func (l4 *L4) getOldForwardingRule() (*composite.ForwardingRule, error) { - bsName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) - // Check if protocol has changed for this service. In this case, forwarding rule has different protocol and name - existingBS, err := l4.backendPool.Get(bsName, meta.VersionGA, l4.scope) - err = utils.IgnoreHTTPNotFound(err) - if err != nil { - klog.Errorf("Failed to lookup existing backend service, ignoring err: %v", err) - } - +func (l4 *L4) getOldIPv4ForwardingRule(existingBS *composite.BackendService) (*composite.ForwardingRule, error) { servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) + + oldFRName := 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) - // Delete forwarding rule if it exists - oldProtocolFRRName := l4.getFRNameWithProtocol(existingBS.Protocol) - return l4.forwardingRules.Get(oldProtocolFRRName) + oldFRName = l4.getFRNameWithProtocol(existingBS.Protocol) } - return l4.forwardingRules.Get(l4.GetFRName()) + return l4.forwardingRules.Get(oldFRName) } diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 8338a3ba8e..934dbc2066 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -19,6 +19,7 @@ limitations under the License. import ( "context" "fmt" + "net" "reflect" "strings" "testing" @@ -1399,12 +1400,9 @@ func TestDualStackInternalLoadBalancerModifyProtocol(t *testing.T) { if err != nil { return false, err } - // we don't reserve addresses for IPv6 forwarding rules - if fr.IpVersion == IPVersionIPv6 { - return false, nil - } - addr, err := l4.cloud.GetRegionAddressByIP(fr.Region, fr.IPAddress) + ipv6Address := net.ParseIP(fr.IPAddress).String() + addr, err := l4.cloud.GetRegionAddressByIP(fr.Region, ipv6Address) if utils.IgnoreHTTPNotFound(err) != nil { return true, err } diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index 79f0a97505..3c04c4fef8 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -23,6 +23,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/annotations" + "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog/v2" @@ -32,8 +33,8 @@ import ( // - IPv6 Forwarding Rule // - IPv6 Firewall // it also adds IPv6 address to LB status -func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string) { - ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options) +func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, existingIPv6FwdRule *composite.ForwardingRule, ipv6ToUse string) { + ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options, existingIPv6FwdRule, ipv6ToUse) if err != nil { klog.Errorf("ensureIPv6Resources: Failed to ensure ipv6 forwarding rule - %v", err) syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource @@ -181,3 +182,15 @@ func (l4 *L4) deleteIPv6NodesFirewall() error { return l4.deleteFirewall(ipv6FirewallName) } + +func (l4 *L4) getOldIPv6ForwardingRule(existingBS *composite.BackendService) (*composite.ForwardingRule, error) { + servicePorts := l4.Service.Spec.Ports + protocol := utils.GetProtocol(servicePorts) + + oldIPv6FRName := l4.getIPv6FRName() + if existingBS != nil && existingBS.Protocol != string(protocol) { + oldIPv6FRName = l4.getIPv6FRNameWithProtocol(existingBS.Protocol) + } + + return l4.forwardingRules.Get(oldIPv6FRName) +} diff --git a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/mock.go b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/mock.go index 8d5a63fe5d..6841e1f2f6 100644 --- a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/mock.go +++ b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/mock.go @@ -18,7 +18,8 @@ limitations under the License. // These methods are used to override the mock objects' methods in order to // intercept the standard processing and to add custom logic for test purposes. // -// // Example usage: +// // Example usage: +// // cloud := cloud.NewMockGCE() // cloud.MockTargetPools.AddInstanceHook = mock.AddInstanceHook package mock @@ -245,7 +246,12 @@ func convertAndInsertAlphaAddress(key *meta.Key, obj gceObject, mAddrs map[meta. } if addr.Address == "" { - addr.Address = fmt.Sprintf("1.2.3.%d", addressAttrs.IPCounter) + if addr.IpVersion == "IPV6" { + addr.Address = fmt.Sprintf("1111:2222:3333:4444:5555:%d:0:0", addressAttrs.IPCounter) + addr.PrefixLength = 96 + } else { + addr.Address = fmt.Sprintf("1.2.3.%d", addressAttrs.IPCounter) + } addressAttrs.IPCounter++ } @@ -1038,7 +1044,6 @@ func SetSslPolicyBetaTargetHTTPSProxyHook(ctx context.Context, key *meta.Key, re return nil } - // InsertFirewallsUnauthorizedErrHook mocks firewall insertion. A forbidden error will be thrown as return. func InsertFirewallsUnauthorizedErrHook(ctx context.Context, key *meta.Key, obj *ga.Firewall, m *cloud.MockFirewalls) (bool, error) { return true, &googleapi.Error{Code: http.StatusForbidden} diff --git a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/observe.go b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/observe.go index 81adb205ef..561422dd37 100644 --- a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/observe.go +++ b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/observe.go @@ -32,7 +32,9 @@ type CallObserver interface { End(ctx context.Context, key *RateLimitKey, err error) } -var callObserverContextKey = &struct{}{} +type contextKey string + +var callObserverContextKey = contextKey("call observer") // WithCallObserver adds a CallObserver that will be called on the // operation being called. @@ -54,7 +56,7 @@ func callObserverStart(ctx context.Context, key *CallContextKey) { } co, ok := obj.(CallObserver) if !ok { - panic(fmt.Sprintf("expected CallObserver, got %T", co)) + panic(fmt.Sprintf("expected CallObserver, got %T", obj)) } co.Start(ctx, key) } @@ -66,7 +68,7 @@ func callObserverEnd(ctx context.Context, key *CallContextKey, err error) { } co, ok := obj.(CallObserver) if !ok { - panic(fmt.Sprintf("expected CallObserver, got %T", co)) + panic(fmt.Sprintf("expected CallObserver, got %T", obj)) } co.End(ctx, key, err) } diff --git a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go index eb261823c6..ea1cf24e64 100644 --- a/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go +++ b/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go @@ -49,17 +49,45 @@ type ResourceID struct { func (r *ResourceID) Equal(other *ResourceID) bool { switch { case r == nil && other == nil: - return true + return true case r == nil || other == nil: - return false + return false case r.ProjectID != other.ProjectID || r.Resource != other.Resource: - return false + return false case r.Key != nil && other.Key != nil: - return *r.Key == *other.Key + return *r.Key == *other.Key case r.Key == nil && other.Key == nil: - return true + return true default: - return false + return false + } +} + +// ResourceMapKey is a flat ResourceID that can be used as a key in maps. +type ResourceMapKey struct { + ProjectID string + Resource string + Name string + Zone string + Region string +} + +func (rk ResourceMapKey) ToID() *ResourceID { + return &ResourceID{ + ProjectID: rk.ProjectID, + Resource: rk.Resource, + Key: &meta.Key{Name: rk.Name, Zone: rk.Zone, Region: rk.Region}, + } +} + +// MapKey returns a flat key that can be used for referencing in maps. +func (r *ResourceID) MapKey() ResourceMapKey { + return ResourceMapKey{ + ProjectID: r.ProjectID, + Resource: r.Resource, + Name: r.Key.Name, + Zone: r.Key.Zone, + Region: r.Key.Region, } } @@ -78,18 +106,28 @@ func (r *ResourceID) SelfLink(ver meta.Version) string { return SelfLink(ver, r.ProjectID, r.Resource, r.Key) } +func (r *ResourceID) String() string { + switch r.Key.Type() { + case meta.Zonal: + return fmt.Sprintf("%s:%s/%s/%s", r.Resource, r.ProjectID, r.Key.Zone, r.Key.Name) + case meta.Regional: + return fmt.Sprintf("%s:%s/%s/%s", r.Resource, r.ProjectID, r.Key.Region, r.Key.Name) + } + return fmt.Sprintf("%s:%s/%s", r.Resource, r.ProjectID, r.Key.Name) +} + // ParseResourceURL parses resource URLs of the following formats: // -// global// -// regions/// -// zones/// -// projects/ -// projects//global// -// projects//regions/// -// projects//zones/// -// [https://www.googleapis.com/compute/]/projects//global// -// [https://www.googleapis.com/compute/]/projects//regions/// -// [https://www.googleapis.com/compute/]/projects//zones/// +// global// +// regions/// +// zones/// +// projects/ +// projects//global// +// projects//regions/// +// projects//zones/// +// [https://www.googleapis.com/compute/]/projects//global// +// [https://www.googleapis.com/compute/]/projects//regions/// +// [https://www.googleapis.com/compute/]/projects//zones/// func ParseResourceURL(url string) (*ResourceID, error) { errNotValid := fmt.Errorf("%q is not a valid resource URL", url) diff --git a/vendor/github.com/kr/pretty/diff.go b/vendor/github.com/kr/pretty/diff.go index 6aa7f743a2..40a09dc648 100644 --- a/vendor/github.com/kr/pretty/diff.go +++ b/vendor/github.com/kr/pretty/diff.go @@ -41,7 +41,12 @@ type Printfer interface { // It calls Printf once for each difference, with no trailing newline. // The standard library log.Logger is a Printfer. func Pdiff(p Printfer, a, b interface{}) { - diffPrinter{w: p}.diff(reflect.ValueOf(a), reflect.ValueOf(b)) + d := diffPrinter{ + w: p, + aVisited: make(map[visit]visit), + bVisited: make(map[visit]visit), + } + d.diff(reflect.ValueOf(a), reflect.ValueOf(b)) } type Logfer interface { @@ -66,6 +71,9 @@ func Ldiff(l Logfer, a, b interface{}) { type diffPrinter struct { w Printfer l string // label + + aVisited map[visit]visit + bVisited map[visit]visit } func (w diffPrinter) printf(f string, a ...interface{}) { @@ -96,6 +104,28 @@ func (w diffPrinter) diff(av, bv reflect.Value) { return } + if av.CanAddr() && bv.CanAddr() { + avis := visit{av.UnsafeAddr(), at} + bvis := visit{bv.UnsafeAddr(), bt} + var cycle bool + + // Have we seen this value before? + if vis, ok := w.aVisited[avis]; ok { + cycle = true + if vis != bvis { + w.printf("%# v (previously visited) != %# v", formatter{v: av, quote: true}, formatter{v: bv, quote: true}) + } + } else if _, ok := w.bVisited[bvis]; ok { + cycle = true + w.printf("%# v != %# v (previously visited)", formatter{v: av, quote: true}, formatter{v: bv, quote: true}) + } + w.aVisited[avis] = bvis + w.bVisited[bvis] = avis + if cycle { + return + } + } + switch kind := at.Kind(); kind { case reflect.Bool: if a, b := av.Bool(), bv.Bool(); a != b { diff --git a/vendor/github.com/kr/pretty/formatter.go b/vendor/github.com/kr/pretty/formatter.go index df61d8d19e..249f089ef0 100644 --- a/vendor/github.com/kr/pretty/formatter.go +++ b/vendor/github.com/kr/pretty/formatter.go @@ -8,6 +8,7 @@ import ( "text/tabwriter" "github.com/kr/text" + "github.com/rogpeppe/go-internal/fmtsort" ) type formatter struct { @@ -37,7 +38,7 @@ func (fo formatter) passThrough(f fmt.State, c rune) { s := "%" for i := 0; i < 128; i++ { if f.Flag(i) { - s += string(i) + s += string(rune(i)) } } if w, ok := f.Width(); ok { @@ -97,6 +98,14 @@ func (p *printer) printValue(v reflect.Value, showType, quote bool) { return } + if v.IsValid() && v.CanInterface() { + i := v.Interface() + if goStringer, ok := i.(fmt.GoStringer); ok { + io.WriteString(p, goStringer.GoString()) + return + } + } + switch v.Kind() { case reflect.Bool: p.printInline(v, v.Bool(), showType) @@ -123,10 +132,10 @@ func (p *printer) printValue(v reflect.Value, showType, quote bool) { writeByte(p, '\n') pp = p.indent() } - keys := v.MapKeys() + sm := fmtsort.Sort(v) for i := 0; i < v.Len(); i++ { - k := keys[i] - mv := v.MapIndex(k) + k := sm.Key[i] + mv := sm.Value[i] pp.printValue(k, false, true) writeByte(pp, ':') if expand { diff --git a/vendor/github.com/rogpeppe/go-internal/LICENSE b/vendor/github.com/rogpeppe/go-internal/LICENSE new file mode 100644 index 0000000000..49ea0f9288 --- /dev/null +++ b/vendor/github.com/rogpeppe/go-internal/LICENSE @@ -0,0 +1,27 @@ +Copyright (c) 2018 The Go Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem.go b/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem.go new file mode 100644 index 0000000000..4e4c29459e --- /dev/null +++ b/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem.go @@ -0,0 +1,23 @@ +//go:build go1.12 +// +build go1.12 + +package fmtsort + +import "reflect" + +const brokenNaNs = false + +func mapElems(mapValue reflect.Value) ([]reflect.Value, []reflect.Value) { + // Note: this code is arranged to not panic even in the presence + // of a concurrent map update. The runtime is responsible for + // yelling loudly if that happens. See issue 33275. + n := mapValue.Len() + key := make([]reflect.Value, 0, n) + value := make([]reflect.Value, 0, n) + iter := mapValue.MapRange() + for iter.Next() { + key = append(key, iter.Key()) + value = append(value, iter.Value()) + } + return key, value +} diff --git a/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem_1.11.go b/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem_1.11.go new file mode 100644 index 0000000000..873bf7f5e8 --- /dev/null +++ b/vendor/github.com/rogpeppe/go-internal/fmtsort/mapelem_1.11.go @@ -0,0 +1,24 @@ +//go:build !go1.12 +// +build !go1.12 + +package fmtsort + +import "reflect" + +const brokenNaNs = true + +func mapElems(mapValue reflect.Value) ([]reflect.Value, []reflect.Value) { + key := mapValue.MapKeys() + value := make([]reflect.Value, 0, len(key)) + for _, k := range key { + v := mapValue.MapIndex(k) + if !v.IsValid() { + // Note: we can't retrieve the value, probably because + // the key is NaN, so just do the best we can and + // add a zero value of the correct type in that case. + v = reflect.Zero(mapValue.Type().Elem()) + } + value = append(value, v) + } + return key, value +} diff --git a/vendor/github.com/rogpeppe/go-internal/fmtsort/sort.go b/vendor/github.com/rogpeppe/go-internal/fmtsort/sort.go new file mode 100644 index 0000000000..0fb5187dd8 --- /dev/null +++ b/vendor/github.com/rogpeppe/go-internal/fmtsort/sort.go @@ -0,0 +1,210 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package fmtsort provides a general stable ordering mechanism +// for maps, on behalf of the fmt and text/template packages. +// It is not guaranteed to be efficient and works only for types +// that are valid map keys. +package fmtsort + +import ( + "reflect" + "sort" +) + +// Note: Throughout this package we avoid calling reflect.Value.Interface as +// it is not always legal to do so and it's easier to avoid the issue than to face it. + +// SortedMap represents a map's keys and values. The keys and values are +// aligned in index order: Value[i] is the value in the map corresponding to Key[i]. +type SortedMap struct { + Key []reflect.Value + Value []reflect.Value +} + +func (o *SortedMap) Len() int { return len(o.Key) } +func (o *SortedMap) Less(i, j int) bool { return compare(o.Key[i], o.Key[j]) < 0 } +func (o *SortedMap) Swap(i, j int) { + o.Key[i], o.Key[j] = o.Key[j], o.Key[i] + o.Value[i], o.Value[j] = o.Value[j], o.Value[i] +} + +// Sort accepts a map and returns a SortedMap that has the same keys and +// values but in a stable sorted order according to the keys, modulo issues +// raised by unorderable key values such as NaNs. +// +// The ordering rules are more general than with Go's < operator: +// +// - when applicable, nil compares low +// - ints, floats, and strings order by < +// - NaN compares less than non-NaN floats +// - bool compares false before true +// - complex compares real, then imag +// - pointers compare by machine address +// - channel values compare by machine address +// - structs compare each field in turn +// - arrays compare each element in turn. +// Otherwise identical arrays compare by length. +// - interface values compare first by reflect.Type describing the concrete type +// and then by concrete value as described in the previous rules. +// +func Sort(mapValue reflect.Value) *SortedMap { + if mapValue.Type().Kind() != reflect.Map { + return nil + } + key, value := mapElems(mapValue) + sorted := &SortedMap{ + Key: key, + Value: value, + } + sort.Stable(sorted) + return sorted +} + +// compare compares two values of the same type. It returns -1, 0, 1 +// according to whether a > b (1), a == b (0), or a < b (-1). +// If the types differ, it returns -1. +// See the comment on Sort for the comparison rules. +func compare(aVal, bVal reflect.Value) int { + aType, bType := aVal.Type(), bVal.Type() + if aType != bType { + return -1 // No good answer possible, but don't return 0: they're not equal. + } + switch aVal.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + a, b := aVal.Int(), bVal.Int() + switch { + case a < b: + return -1 + case a > b: + return 1 + default: + return 0 + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + a, b := aVal.Uint(), bVal.Uint() + switch { + case a < b: + return -1 + case a > b: + return 1 + default: + return 0 + } + case reflect.String: + a, b := aVal.String(), bVal.String() + switch { + case a < b: + return -1 + case a > b: + return 1 + default: + return 0 + } + case reflect.Float32, reflect.Float64: + return floatCompare(aVal.Float(), bVal.Float()) + case reflect.Complex64, reflect.Complex128: + a, b := aVal.Complex(), bVal.Complex() + if c := floatCompare(real(a), real(b)); c != 0 { + return c + } + return floatCompare(imag(a), imag(b)) + case reflect.Bool: + a, b := aVal.Bool(), bVal.Bool() + switch { + case a == b: + return 0 + case a: + return 1 + default: + return -1 + } + case reflect.Ptr: + a, b := aVal.Pointer(), bVal.Pointer() + switch { + case a < b: + return -1 + case a > b: + return 1 + default: + return 0 + } + case reflect.Chan: + if c, ok := nilCompare(aVal, bVal); ok { + return c + } + ap, bp := aVal.Pointer(), bVal.Pointer() + switch { + case ap < bp: + return -1 + case ap > bp: + return 1 + default: + return 0 + } + case reflect.Struct: + for i := 0; i < aVal.NumField(); i++ { + if c := compare(aVal.Field(i), bVal.Field(i)); c != 0 { + return c + } + } + return 0 + case reflect.Array: + for i := 0; i < aVal.Len(); i++ { + if c := compare(aVal.Index(i), bVal.Index(i)); c != 0 { + return c + } + } + return 0 + case reflect.Interface: + if c, ok := nilCompare(aVal, bVal); ok { + return c + } + c := compare(reflect.ValueOf(aVal.Elem().Type()), reflect.ValueOf(bVal.Elem().Type())) + if c != 0 { + return c + } + return compare(aVal.Elem(), bVal.Elem()) + default: + // Certain types cannot appear as keys (maps, funcs, slices), but be explicit. + panic("bad type in compare: " + aType.String()) + } +} + +// nilCompare checks whether either value is nil. If not, the boolean is false. +// If either value is nil, the boolean is true and the integer is the comparison +// value. The comparison is defined to be 0 if both are nil, otherwise the one +// nil value compares low. Both arguments must represent a chan, func, +// interface, map, pointer, or slice. +func nilCompare(aVal, bVal reflect.Value) (int, bool) { + if aVal.IsNil() { + if bVal.IsNil() { + return 0, true + } + return -1, true + } + if bVal.IsNil() { + return 1, true + } + return 0, false +} + +// floatCompare compares two floating-point values. NaNs compare low. +func floatCompare(a, b float64) int { + switch { + case isNaN(a): + return -1 // No good answer if b is a NaN so don't bother checking. + case isNaN(b): + return 1 + case a < b: + return -1 + case a > b: + return 1 + } + return 0 +} + +func isNaN(a float64) bool { + return a != a +} diff --git a/vendor/modules.txt b/vendor/modules.txt index ad0d863e46..edf16613b6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -4,7 +4,7 @@ cloud.google.com/go/compute/internal # cloud.google.com/go/compute/metadata v0.2.3 ## explicit; go 1.19 cloud.google.com/go/compute/metadata -# github.com/GoogleCloudPlatform/k8s-cloud-provider v1.23.0 +# github.com/GoogleCloudPlatform/k8s-cloud-provider v1.24.0 ## explicit; go 1.20 github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/filter @@ -124,7 +124,7 @@ github.com/josharian/intern # github.com/json-iterator/go v1.1.12 ## explicit; go 1.12 github.com/json-iterator/go -# github.com/kr/pretty v0.2.0 +# github.com/kr/pretty v0.3.0 ## explicit; go 1.12 github.com/kr/pretty # github.com/kr/text v0.2.0 @@ -172,6 +172,9 @@ github.com/prometheus/common/model github.com/prometheus/procfs github.com/prometheus/procfs/internal/fs github.com/prometheus/procfs/internal/util +# github.com/rogpeppe/go-internal v1.9.0 +## explicit; go 1.17 +github.com/rogpeppe/go-internal/fmtsort # github.com/spf13/cobra v1.6.0 ## explicit; go 1.15 github.com/spf13/cobra From 9e2d9b184de453de770daa6ff13afcb34b959cf5 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Wed, 21 Jun 2023 23:52:23 +0000 Subject: [PATCH 2/2] Fix review comments --- pkg/loadbalancers/address_manager_test.go | 11 -------- pkg/loadbalancers/forwarding_rules_ipv6.go | 12 ++++---- pkg/loadbalancers/l4.go | 33 ++++++++++++---------- pkg/loadbalancers/l4ipv6.go | 7 +++-- 4 files changed, 29 insertions(+), 34 deletions(-) diff --git a/pkg/loadbalancers/address_manager_test.go b/pkg/loadbalancers/address_manager_test.go index 2425c5c9fc..5b46e28eae 100644 --- a/pkg/loadbalancers/address_manager_test.go +++ b/pkg/loadbalancers/address_manager_test.go @@ -238,17 +238,6 @@ func TestAddressManagerIPv6(t *testing.T) { } } -// TestAddressManagerEmptyIPv6 tests the typical case of reserving and releasing an IPv6 address. -func TestAddressManagerEmptyIPv6(t *testing.T) { - svc, err := fakeGCECloud(vals) - require.NoError(t, err) - targetIP := "" - - mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, IPv6Version) - testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue()) - testReleaseAddress(t, mgr, svc, testLBName, vals.Region) -} - func testHoldAddress(t *testing.T, mgr *addressManager, svc gce.CloudAddressService, name, region, targetIP, scheme, netTier string) { ipToUse, ipType, err := mgr.HoldAddress() require.NoError(t, err) diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index bbd945c05b..960ebbd75a 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -38,12 +38,12 @@ const ( IPVersionIPv6 = "IPV6" ) -func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, existingIPv6FwdRule *composite.ForwardingRule, ipv6ToUse string) (*composite.ForwardingRule, error) { +func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, existingIPv6FwdRule *composite.ForwardingRule, ipv6AddressToUse string) (*composite.ForwardingRule, error) { start := time.Now() - expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options, ipv6ToUse) + expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options, ipv6AddressToUse) if err != nil { - return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v, %s) returned error %w, want nil", bsLink, options, ipv6ToUse, err) + return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v, %s) returned error %w, want nil", bsLink, options, ipv6AddressToUse, err) } klog.V(2).Infof("Ensuring internal ipv6 forwarding rule %s for L4 ILB Service %s/%s, backend service link: %s", expectedIPv6FwdRule.Name, l4.Service.Namespace, l4.Service.Name, bsLink) @@ -76,7 +76,7 @@ func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ex return createdFr, err } -func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ipv6ToUse string) (*composite.ForwardingRule, error) { +func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ipv6AddressToUse string) (*composite.ForwardingRule, error) { frName := l4.getIPv6FRName() frDesc, err := utils.MakeL4IPv6ForwardingRuleDescription(l4.Service) @@ -100,7 +100,7 @@ func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOpti fr := &composite.ForwardingRule{ Name: frName, Description: frDesc, - IPAddress: ipv6ToUse, + IPAddress: ipv6AddressToUse, IPProtocol: string(protocol), Ports: ports, LoadBalancingScheme: string(cloud.SchemeInternal), @@ -246,7 +246,7 @@ func EqualIPv6ForwardingRules(fr1, fr2 *composite.ForwardingRule) (bool, error) fr1.NetworkTier == fr2.NetworkTier, nil } -func ipv6IPToUse(ipv6FwdRule *composite.ForwardingRule, requestedSubnet string) string { +func ipv6AddressToUse(ipv6FwdRule *composite.ForwardingRule, requestedSubnet string) string { if ipv6FwdRule == nil { return "" } diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index d4a9600095..1f0491f36b 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -318,21 +318,21 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service // Reserve existing IP address before making any changes var existingIPv4FR *composite.ForwardingRule - var ipv4ToUse string + var ipv4AddressToUse string if !l4.enableDualStack || utils.NeedsIPv4(l4.Service) { existingIPv4FR, err = l4.getOldIPv4ForwardingRule(existingBS) - ipv4ToUse = l4lbIPToUse(l4.Service, existingIPv4FR, subnetworkURL) + ipv4AddressToUse = l4lbIPToUse(l4.Service, existingIPv4FR, subnetworkURL) expectedFRName := l4.GetFRName() if !l4.cloud.IsLegacyNetwork() { nm := types.NamespacedName{Namespace: l4.Service.Namespace, Name: l4.Service.Name}.String() // ILB can be created only in Premium Tier - addrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedFRName, ipv4ToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) - ipv4ToUse, _, err = addrMgr.HoldAddress() + addrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedFRName, ipv4AddressToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv4Version) + ipv4AddressToUse, _, err = addrMgr.HoldAddress() if err != nil { result.Error = fmt.Errorf("EnsureInternalLoadBalancer error: addrMgr.HoldAddress() returned error %w", err) return result } - klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv4 address %q", nm, ipv4ToUse) + klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv4 address %q", nm, ipv4AddressToUse) 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. @@ -345,21 +345,21 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service // Reserve existing IPv6 address before making any changes var existingIPv6FR *composite.ForwardingRule - var ipv6ToUse string + var ipv6AddrToUse string if l4.enableDualStack && utils.NeedsIPv6(l4.Service) { existingIPv6FR, err = l4.getOldIPv6ForwardingRule(existingBS) - ipv6ToUse = ipv6IPToUse(existingIPv6FR, subnetworkURL) + ipv6AddrToUse = ipv6AddressToUse(existingIPv6FR, subnetworkURL) expectedIPv6FRName := l4.getIPv6FRName() if !l4.cloud.IsLegacyNetwork() { nm := types.NamespacedName{Namespace: l4.Service.Namespace, Name: l4.Service.Name}.String() // ILB can be created only in Premium Tier - ipv6AddrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedIPv6FRName, ipv6ToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv6Version) - ipv6ToUse, _, err = ipv6AddrMgr.HoldAddress() + ipv6AddrMgr := newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, expectedIPv6FRName, ipv6AddrToUse, cloud.SchemeInternal, cloud.NetworkTierPremium, IPv6Version) + ipv6AddrToUse, _, err = ipv6AddrMgr.HoldAddress() if err != nil { result.Error = fmt.Errorf("EnsureInternalLoadBalancer error: ipv6AddrMgr.HoldAddress() returned error %w", err) return result } - klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv6 address %q", nm, ipv6ToUse) + klog.V(2).Infof("EnsureInternalLoadBalancer(%v): reserved IPv6 address %q", nm, 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. @@ -404,9 +404,9 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service result.Annotations[annotations.BackendServiceKey] = bsName if l4.enableDualStack { - l4.ensureDualStackResources(result, nodeNames, options, bs, existingIPv4FR, existingIPv6FR, subnetworkURL, ipv4ToUse, ipv6ToUse) + l4.ensureDualStackResources(result, nodeNames, options, bs, existingIPv4FR, existingIPv6FR, subnetworkURL, ipv4AddressToUse, ipv6AddrToUse) } else { - l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FR, subnetworkURL, ipv4ToUse) + l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FR, subnetworkURL, ipv4AddressToUse) } if result.Error != nil { return result @@ -468,14 +468,14 @@ func (l4 *L4) provideIPv4HealthChecks(nodeNames []string, result *L4ILBSyncResul return hcResult.HCLink } -func (l4 *L4) ensureDualStackResources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingIPv4FwdRule, existingIPv6FwdRule *composite.ForwardingRule, subnetworkURL, ipv4ToUse, ipv6ToUse string) { +func (l4 *L4) ensureDualStackResources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingIPv4FwdRule, existingIPv6FwdRule *composite.ForwardingRule, subnetworkURL, ipv4AddressToUse, ipv6AddressToUse string) { if utils.NeedsIPv4(l4.Service) { - l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FwdRule, subnetworkURL, ipv4ToUse) + l4.ensureIPv4Resources(result, nodeNames, options, bs, existingIPv4FwdRule, subnetworkURL, ipv4AddressToUse) } else { l4.deleteIPv4ResourcesOnSync(result) } if utils.NeedsIPv6(l4.Service) { - l4.ensureIPv6Resources(result, nodeNames, options, bs.SelfLink, existingIPv6FwdRule, ipv6ToUse) + l4.ensureIPv6Resources(result, nodeNames, options, bs.SelfLink, existingIPv6FwdRule, ipv6AddressToUse) } else { l4.deleteIPv6ResourcesOnSync(result) } @@ -572,6 +572,9 @@ func (l4 *L4) hasAnnotation(annotationKey string) bool { return false } +// getOldIPv4ForwardingRule returns old IPv4 forwarding rule, with checking backend service protocol, if it exists. +// This is useful when switching protocols of the service, +// because forwarding rule name depends on the protocol, and we need to get forwarding rule from the old protocol name. func (l4 *L4) getOldIPv4ForwardingRule(existingBS *composite.BackendService) (*composite.ForwardingRule, error) { servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index 3c04c4fef8..e92a997e28 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -33,8 +33,8 @@ import ( // - IPv6 Forwarding Rule // - IPv6 Firewall // it also adds IPv6 address to LB status -func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, existingIPv6FwdRule *composite.ForwardingRule, ipv6ToUse string) { - ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options, existingIPv6FwdRule, ipv6ToUse) +func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, existingIPv6FwdRule *composite.ForwardingRule, ipv6AddressToUse string) { + ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options, existingIPv6FwdRule, ipv6AddressToUse) if err != nil { klog.Errorf("ensureIPv6Resources: Failed to ensure ipv6 forwarding rule - %v", err) syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource @@ -183,6 +183,9 @@ func (l4 *L4) deleteIPv6NodesFirewall() error { return l4.deleteFirewall(ipv6FirewallName) } +// getOldIPv6ForwardingRule returns old IPv6 forwarding rule, with checking backend service protocol, if it exists. +// This is useful when switching protocols of the service, +// because forwarding rule name depends on the protocol, and we need to get forwarding rule from the old protocol name. func (l4 *L4) getOldIPv6ForwardingRule(existingBS *composite.BackendService) (*composite.ForwardingRule, error) { servicePorts := l4.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts)