diff --git a/pkg/cloudprovider/providers/gce/gce_addresses.go b/pkg/cloudprovider/providers/gce/gce_addresses.go index 11536431a2d..254c08a3b8e 100644 --- a/pkg/cloudprovider/providers/gce/gce_addresses.go +++ b/pkg/cloudprovider/providers/gce/gce_addresses.go @@ -176,3 +176,15 @@ func (gce *GCECloud) GetBetaRegionAddressByIP(region, ipAddress string) (*comput } return nil, makeGoogleAPINotFoundError(fmt.Sprintf("Address with IP %q was not found in region %q", ipAddress, region)) } + +// TODO(#51665): retire this function once Network Tiers becomes Beta in GCP. +func (gce *GCECloud) getNetworkTierFromAddress(name, region string) (string, error) { + if !gce.AlphaFeatureGate.Enabled(AlphaFeatureNetworkTiers) { + return NetworkTierDefault.ToGCEValue(), nil + } + addr, err := gce.GetAlphaRegionAddress(name, region) + if err != nil { + return handleAlphaNetworkTierGetError(err) + } + return addr.NetworkTier, nil +} diff --git a/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go b/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go index 628a071bdab..1fd8e91a8cc 100644 --- a/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go +++ b/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go @@ -27,6 +27,8 @@ import ( compute "google.golang.org/api/compute/v1" ) +// test + type FakeCloudAddressService struct { count int // reservedAddrs tracks usage of IP addresses @@ -70,7 +72,16 @@ func (cas *FakeCloudAddressService) ReserveAlphaRegionAddress(addr *computealpha } if cas.reservedAddrs[addr.Address] { - return makeGoogleAPIError(http.StatusConflict, "IP in use") + msg := "IP in use" + // When the IP is already in use, this call returns an error code based + // on the type (internal vs external) of the address. This is to be + // consistent with actual GCE API. + switch lbScheme(addr.AddressType) { + case schemeExternal: + return makeGoogleAPIError(http.StatusBadRequest, msg) + default: + return makeGoogleAPIError(http.StatusConflict, msg) + } } if _, exists := cas.addrsByRegionAndName[region]; !exists { @@ -133,6 +144,7 @@ func (cas *FakeCloudAddressService) DeleteRegionAddress(name, region string) err if !exists { return makeGoogleAPINotFoundError("") } + delete(cas.reservedAddrs, addr.Address) delete(cas.addrsByRegionAndName[region], name) return nil @@ -167,6 +179,14 @@ func (cas *FakeCloudAddressService) GetRegionAddressByIP(name, region string) (* return nil, err } +func (cas *FakeCloudAddressService) getNetworkTierFromAddress(name, region string) (string, error) { + addr, err := cas.GetAlphaRegionAddress(name, region) + if err != nil { + return "", err + } + return addr.NetworkTier, nil +} + func convertToV1Address(object gceObject) *compute.Address { enc, err := object.MarshalJSON() if err != nil { @@ -188,6 +208,8 @@ func convertToAlphaAddress(object gceObject) *computealpha.Address { if err := json.Unmarshal(enc, &addr); err != nil { panic(fmt.Sprintf("Failed to convert GCE apiObject %v to alpha address: %v", object, err)) } + // Set the default values for the Alpha fields. + addr.NetworkTier = NetworkTierDefault.ToGCEValue() return &addr } diff --git a/pkg/cloudprovider/providers/gce/gce_alpha.go b/pkg/cloudprovider/providers/gce/gce_alpha.go index bd629769ba3..7bc4b45c72d 100644 --- a/pkg/cloudprovider/providers/gce/gce_alpha.go +++ b/pkg/cloudprovider/providers/gce/gce_alpha.go @@ -22,15 +22,22 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" ) -// All known alpha features -var knownAlphaFeatures = map[string]bool{ - GCEDiskAlphaFeatureGate: true, -} - const ( + // alpha: v1.8 (for Services) + // + // Allows Services backed by a GCP load balancer to choose what network + // tier to use. Currently supports "Standard" and "Premium" (default). + AlphaFeatureNetworkTiers = "NetworkTiers" + GCEDiskAlphaFeatureGate = "GCEDiskAlphaAPI" ) +// All known alpha features +var knownAlphaFeatures = map[string]bool{ + AlphaFeatureNetworkTiers: true, + GCEDiskAlphaFeatureGate: true, +} + type AlphaFeatureGate struct { features map[string]bool } diff --git a/pkg/cloudprovider/providers/gce/gce_forwardingrule.go b/pkg/cloudprovider/providers/gce/gce_forwardingrule.go index c747ed1ddb8..f674b205705 100644 --- a/pkg/cloudprovider/providers/gce/gce_forwardingrule.go +++ b/pkg/cloudprovider/providers/gce/gce_forwardingrule.go @@ -141,3 +141,15 @@ func (gce *GCECloud) DeleteRegionForwardingRule(name, region string) error { return gce.waitForRegionOp(op, region, mc) } + +// TODO(#51665): retire this function once Network Tiers becomes Beta in GCP. +func (gce *GCECloud) getNetworkTierFromForwardingRule(name, region string) (string, error) { + if !gce.AlphaFeatureGate.Enabled(AlphaFeatureNetworkTiers) { + return NetworkTierDefault.ToGCEValue(), nil + } + fwdRule, err := gce.GetAlphaRegionForwardingRule(name, region) + if err != nil { + return handleAlphaNetworkTierGetError(err) + } + return fwdRule.NetworkTier, nil +} diff --git a/pkg/cloudprovider/providers/gce/gce_forwardingrule_fakes.go b/pkg/cloudprovider/providers/gce/gce_forwardingrule_fakes.go index 0cc0188223a..780012791fc 100644 --- a/pkg/cloudprovider/providers/gce/gce_forwardingrule_fakes.go +++ b/pkg/cloudprovider/providers/gce/gce_forwardingrule_fakes.go @@ -102,6 +102,14 @@ func (f *FakeCloudForwardingRuleService) GetRegionForwardingRule(name, region st return nil, err } +func (f *FakeCloudForwardingRuleService) getNetworkTierFromForwardingRule(name, region string) (string, error) { + fwdRule, err := f.GetAlphaRegionForwardingRule(name, region) + if err != nil { + return "", err + } + return fwdRule.NetworkTier, nil +} + func convertToV1ForwardingRule(object gceObject) *compute.ForwardingRule { enc, err := object.MarshalJSON() if err != nil { @@ -123,5 +131,8 @@ func convertToAlphaForwardingRule(object gceObject) *computealpha.ForwardingRule if err := json.Unmarshal(enc, &fwdRule); err != nil { panic(fmt.Sprintf("Failed to convert GCE apiObject %v to alpha fwdRuleess: %v", object, err)) } + // Set the default values for the Alpha fields. + fwdRule.NetworkTier = NetworkTierDefault.ToGCEValue() + return &fwdRule } diff --git a/pkg/cloudprovider/providers/gce/gce_interfaces.go b/pkg/cloudprovider/providers/gce/gce_interfaces.go index 11123bc7454..43e1ccab9cf 100644 --- a/pkg/cloudprovider/providers/gce/gce_interfaces.go +++ b/pkg/cloudprovider/providers/gce/gce_interfaces.go @@ -22,6 +22,8 @@ import ( compute "google.golang.org/api/compute/v1" ) +// These interfaces are added for testability. + // CloudAddressService is an interface for managing addresses type CloudAddressService interface { ReserveRegionAddress(address *compute.Address, region string) error @@ -38,6 +40,9 @@ type CloudAddressService interface { ReserveBetaRegionAddress(address *computebeta.Address, region string) error GetBetaRegionAddress(name string, region string) (*computebeta.Address, error) GetBetaRegionAddressByIP(region, ipAddress string) (*computebeta.Address, error) + + // TODO(#51665): Remove this once the Network Tiers becomes Alpha in GCP. + getNetworkTierFromAddress(name, region string) (string, error) } // CloudForwardingRuleService is an interface for managing forwarding rules. @@ -50,4 +55,7 @@ type CloudForwardingRuleService interface { // Alpha API. GetAlphaRegionForwardingRule(name, region string) (*computealpha.ForwardingRule, error) CreateAlphaRegionForwardingRule(rule *computealpha.ForwardingRule, region string) error + + // Needed for the Alpha "Network Tiers" feature. + getNetworkTierFromForwardingRule(name, region string) (string, error) } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go index 129086e77c5..0b8d6e03830 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go @@ -31,6 +31,7 @@ import ( netsets "k8s.io/kubernetes/pkg/util/net/sets" "github.com/golang/glog" + computealpha "google.golang.org/api/compute/v0.alpha" compute "google.golang.org/api/compute/v1" ) @@ -68,6 +69,19 @@ func (gce *GCECloud) ensureExternalLoadBalancer(clusterName, clusterID string, a glog.V(2).Infof("EnsureLoadBalancer(%v, %v, %v, %v, %v, %v, %v)", loadBalancerName, gce.region, requestedIP, portStr, hostNames, serviceName, apiService.Annotations) + lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName) + // Check the current and the desired network tiers. If they do not match, + // tear down the existing resources with the wrong tier. + netTier, err := gce.getServiceNetworkTier(apiService) + if err != nil { + glog.Errorf("EnsureLoadBalancer(%s): failed to get the desired network tier: %v", lbRefStr, err) + return nil, err + } + glog.V(4).Infof("EnsureLoadBalancer(%s): desired network tier %q ", lbRefStr, netTier) + if gce.AlphaFeatureGate.Enabled(AlphaFeatureNetworkTiers) { + gce.deleteWrongNetworkTieredResources(loadBalancerName, lbRefStr, netTier) + } + // Check if the forwarding rule exists, and if so, what its IP is. fwdRuleExists, fwdRuleNeedsUpdate, fwdRuleIP, err := gce.forwardingRuleNeedsUpdate(loadBalancerName, gce.region, requestedIP, ports) if err != nil { @@ -121,11 +135,10 @@ func (gce *GCECloud) ensureExternalLoadBalancer(clusterName, clusterID string, a } }() - lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName) if requestedIP != "" { // If user requests a specific IP address, verify first. No mutation to // the GCE resources will be performed in the verification process. - isUserOwnedIP, err = verifyUserRequestedIP(gce, gce.region, requestedIP, fwdRuleIP, lbRefStr) + isUserOwnedIP, err = verifyUserRequestedIP(gce, gce.region, requestedIP, fwdRuleIP, lbRefStr, netTier) if err != nil { return nil, err } @@ -135,11 +148,11 @@ func (gce *GCECloud) ensureExternalLoadBalancer(clusterName, clusterID string, a if !isUserOwnedIP { // If we are not using the user-owned IP, either promote the // emphemeral IP used by the fwd rule, or create a new static IP. - ipAddr, existed, err := ensureStaticIP(gce, loadBalancerName, serviceName.String(), gce.region, fwdRuleIP) + ipAddr, existed, err := ensureStaticIP(gce, loadBalancerName, serviceName.String(), gce.region, fwdRuleIP, netTier) if err != nil { return nil, fmt.Errorf("failed to ensure a static IP for the LB: %v", err) } - glog.V(4).Infof("EnsureLoadBalancer(%s): ensured IP address %s", lbRefStr, ipAddr) + glog.V(4).Infof("EnsureLoadBalancer(%s): ensured IP address %s (tier: %s)", lbRefStr, ipAddr, netTier) // If the IP was not owned by the user, but it already existed, it // could indicate that the previous update cycle failed. We can use // this IP and try to run through the process again, but we should @@ -282,8 +295,8 @@ func (gce *GCECloud) ensureExternalLoadBalancer(clusterName, clusterID string, a } } if tpNeedsUpdate || fwdRuleNeedsUpdate { - glog.Infof("EnsureLoadBalancer(%v(%v)): creating forwarding rule, IP %s", loadBalancerName, serviceName, ipAddressToUse) - if err := gce.createForwardingRule(loadBalancerName, serviceName.String(), gce.region, ipAddressToUse, ports); err != nil { + glog.Infof("EnsureLoadBalancer(%v(%v)): creating forwarding rule, IP %s (tier: %s)", loadBalancerName, serviceName, ipAddressToUse, netTier) + if err := createForwardingRule(gce, loadBalancerName, serviceName.String(), gce.region, ipAddressToUse, gce.targetPoolURL(loadBalancerName), ports, netTier); err != nil { return nil, fmt.Errorf("failed to create forwarding rule %s: %v", loadBalancerName, err) } // End critical section. It is safe to release the static IP (which @@ -423,10 +436,12 @@ func (gce *GCECloud) DeleteExternalTargetPoolAndChecks(name, region, clusterID s return nil } -// verifyUserRequestedIP checks the user-provided IP to see whether it can be -// used for the LB. It also returns whether the IP is considered owned by the -// user. -func verifyUserRequestedIP(s CloudAddressService, region, requestedIP, fwdRuleIP, lbRef string) (isUserOwnedIP bool, err error) { +// verifyUserRequestedIP checks the user-provided IP to see whether it meets +// all the expected attributes for the load balancer, and returns an error if +// the verification failed. It also returns a boolean to indicate whether the +// IP address is considered owned by the user (i.e., not managed by the +// controller. +func verifyUserRequestedIP(s CloudAddressService, region, requestedIP, fwdRuleIP, lbRef string, desiredNetTier NetworkTier) (isUserOwnedIP bool, err error) { if requestedIP == "" { return false, nil } @@ -442,7 +457,19 @@ func verifyUserRequestedIP(s CloudAddressService, region, requestedIP, fwdRuleIP } if err == nil { // The requested IP is a static IP, owned and managed by the user. - glog.V(4).Infof("verifyUserRequestedIP: the requested static IP %q (name: %s) for LB %s exists.", requestedIP, existingAddress.Name, lbRef) + + // Check if the network tier of the static IP matches the desired + // network tier. + netTierStr, err := s.getNetworkTierFromAddress(existingAddress.Name, region) + if err != nil { + return false, fmt.Errorf("failed to check the network tier of the IP %q: %v", requestedIP, err) + } + netTier := NetworkTierGCEValueToType(netTierStr) + if netTier != desiredNetTier { + glog.Errorf("verifyUserRequestedIP: requested static IP %q (name: %s) for LB %s has network tier %s, need %s.", requestedIP, existingAddress.Name, lbRef, netTier, desiredNetTier) + return false, fmt.Errorf("requrested IP %q belongs to the %s network tier; expected %s", requestedIP, netTier, desiredNetTier) + } + glog.V(4).Infof("verifyUserRequestedIP: the requested static IP %q (name: %s, tier: %s) for LB %s exists.", requestedIP, existingAddress.Name, netTier, lbRef) return true, nil } if requestedIP == fwdRuleIP { @@ -544,8 +571,8 @@ func (gce *GCECloud) updateTargetPool(loadBalancerName string, existing sets.Str return nil } -func (gce *GCECloud) targetPoolURL(name, region string) string { - return gce.service.BasePath + strings.Join([]string{gce.projectID, "regions", region, "targetPools", name}, "/") +func (gce *GCECloud) targetPoolURL(name string) string { + return gce.service.BasePath + strings.Join([]string{gce.projectID, "regions", gce.region, "targetPools", name}, "/") } func makeHttpHealthCheck(name, path string, port int32) *compute.HttpHealthCheck { @@ -804,23 +831,42 @@ func (gce *GCECloud) ensureHttpHealthCheckFirewall(serviceName, ipAddress, regio return nil } -func (gce *GCECloud) createForwardingRule(name, serviceName, region, ipAddress string, ports []v1.ServicePort) error { +func createForwardingRule(s CloudForwardingRuleService, name, serviceName, region, ipAddress, target string, ports []v1.ServicePort, netTier NetworkTier) error { portRange, err := loadBalancerPortRange(ports) if err != nil { return err } - req := &compute.ForwardingRule{ - Name: name, - Description: fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, serviceName), - IPAddress: ipAddress, - IPProtocol: string(ports[0].Protocol), - PortRange: portRange, - Target: gce.targetPoolURL(name, region), + desc := makeServiceDescription(serviceName) + ipProtocol := string(ports[0].Protocol) + + switch netTier { + case NetworkTierPremium: + rule := &compute.ForwardingRule{ + Name: name, + Description: desc, + IPAddress: ipAddress, + IPProtocol: ipProtocol, + PortRange: portRange, + Target: target, + } + err = s.CreateRegionForwardingRule(rule, region) + default: + rule := &computealpha.ForwardingRule{ + Name: name, + Description: desc, + IPAddress: ipAddress, + IPProtocol: ipProtocol, + PortRange: portRange, + Target: target, + NetworkTier: netTier.ToGCEValue(), + } + err = s.CreateAlphaRegionForwardingRule(rule, region) } - if err = gce.CreateRegionForwardingRule(req, region); err != nil && !isHTTPErrorCode(err, http.StatusConflict) { + if err != nil && !isHTTPErrorCode(err, http.StatusConflict) { return err } + return nil } @@ -883,26 +929,43 @@ func (gce *GCECloud) firewallObject(name, region, desc string, sourceRanges nets return firewall, nil } -func ensureStaticIP(s CloudAddressService, name, serviceName, region, existingIP string) (ipAddress string, existing bool, err error) { +func ensureStaticIP(s CloudAddressService, name, serviceName, region, existingIP string, netTier NetworkTier) (ipAddress string, existing bool, err error) { // If the address doesn't exist, this will create it. // If the existingIP exists but is ephemeral, this will promote it to static. // If the address already exists, this will harmlessly return a StatusConflict // and we'll grab the IP before returning. existed := false - addressObj := &compute.Address{ - Name: name, - Description: fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, serviceName), - } + desc := makeServiceDescription(serviceName) - if existingIP != "" { - addressObj.Address = existingIP - } - - if err = s.ReserveRegionAddress(addressObj, region); err != nil { - if !isHTTPErrorCode(err, http.StatusConflict) { - return "", false, fmt.Errorf("error creating gce static IP address: %v", err) + var creationErr error + switch netTier { + case NetworkTierPremium: + addressObj := &compute.Address{ + Name: name, + Description: desc, + } + if existingIP != "" { + addressObj.Address = existingIP + } + creationErr = s.ReserveRegionAddress(addressObj, region) + default: + addressObj := &computealpha.Address{ + Name: name, + Description: desc, + NetworkTier: netTier.ToGCEValue(), + } + if existingIP != "" { + addressObj.Address = existingIP + } + creationErr = s.ReserveAlphaRegionAddress(addressObj, region) + } + + if creationErr != nil { + // GCE returns StatusConflict if the name conflicts; it returns + // StatusBadRequest if the IP conflicts. + if !isHTTPErrorCode(creationErr, http.StatusConflict) && !isHTTPErrorCode(creationErr, http.StatusBadRequest) { + return "", false, fmt.Errorf("error creating gce static IP address: %v", creationErr) } - // StatusConflict == the IP exists already. existed = true } @@ -913,3 +976,73 @@ func ensureStaticIP(s CloudAddressService, name, serviceName, region, existingIP return addr.Address, existed, nil } + +func (gce *GCECloud) getServiceNetworkTier(svc *v1.Service) (NetworkTier, error) { + if !gce.AlphaFeatureGate.Enabled(AlphaFeatureNetworkTiers) { + return NetworkTierDefault, nil + } + tier, err := GetServiceNetworkTier(svc) + if err != nil { + // Returns an error if the annotation is invalid. + return NetworkTier(""), err + } + return tier, nil +} + +func (gce *GCECloud) deleteWrongNetworkTieredResources(lbName, lbRef string, desiredNetTier NetworkTier) error { + logPrefix := fmt.Sprintf("deleteWrongNetworkTieredResources:(%s)", lbRef) + if err := deleteFWDRuleWithWrongTier(gce, gce.region, lbName, logPrefix, desiredNetTier); err != nil { + return err + } + if err := deleteAddressWithWrongTier(gce, gce.region, lbName, logPrefix, desiredNetTier); err != nil { + return err + } + return nil +} + +// deleteFWDRuleWithWrongTier checks the network tier of existing forwarding +// rule and delete the rule if the tier does not matched the desired tier. +func deleteFWDRuleWithWrongTier(s CloudForwardingRuleService, region, name, logPrefix string, desiredNetTier NetworkTier) error { + tierStr, err := s.getNetworkTierFromForwardingRule(name, region) + if isNotFound(err) { + return nil + } else if err != nil { + return err + } + existingTier := NetworkTierGCEValueToType(tierStr) + if existingTier == desiredNetTier { + return nil + } + glog.V(2).Infof("%s: Network tiers do not match; existing forwarding rule: %q, desired: %q. Deleting the forwarding rule", + logPrefix, existingTier, desiredNetTier) + err = s.DeleteRegionForwardingRule(name, region) + return ignoreNotFound(err) +} + +// deleteAddressWithWrongTier checks the network tier of existing address +// and delete the address if the tier does not matched the desired tier. +func deleteAddressWithWrongTier(s CloudAddressService, region, name, logPrefix string, desiredNetTier NetworkTier) error { + // We only check the IP address matching the reserved name that the + // controller assigned to the LB. We make the assumption that an address of + // such name is owned by the controller and is safe to release. Whether an + // IP is owned by the user is not clearly defined in the current code, and + // this assumption may not match some of the existing logic in the code. + // However, this is okay since network tiering is still Alpha and will be + // properly gated. + // TODO(#51665): Re-evaluate the "ownership" of the IP address to ensure + // we don't release IP unintentionally. + tierStr, err := s.getNetworkTierFromAddress(name, region) + if isNotFound(err) { + return nil + } else if err != nil { + return err + } + existingTier := NetworkTierGCEValueToType(tierStr) + if existingTier == desiredNetTier { + return nil + } + glog.V(2).Infof("%s: Network tiers do not match; existing address: %q, desired: %q. Deleting the address", + logPrefix, existingTier, desiredNetTier) + err = s.DeleteRegionAddress(name, region) + return ignoreNotFound(err) +} diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go index d0f26f8910d..d3caaa216c3 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go @@ -21,8 +21,10 @@ import ( "testing" "github.com/stretchr/testify/assert" - + "github.com/stretchr/testify/require" computealpha "google.golang.org/api/compute/v0.alpha" + + "k8s.io/api/core/v1" ) func TestEnsureStaticIP(t *testing.T) { @@ -32,19 +34,54 @@ func TestEnsureStaticIP(t *testing.T) { region := "us-central1" // First ensure call - ip, existed, err := ensureStaticIP(fcas, ipName, serviceName, region, "") + ip, existed, err := ensureStaticIP(fcas, ipName, serviceName, region, "", NetworkTierDefault) if err != nil || existed || ip == "" { t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, "") = %v, %v, %v; want valid ip, false, nil`, fcas, ipName, serviceName, region, ip, existed, err) } // Second ensure call var ipPrime string - ipPrime, existed, err = ensureStaticIP(fcas, ipName, serviceName, region, ip) + ipPrime, existed, err = ensureStaticIP(fcas, ipName, serviceName, region, ip, NetworkTierDefault) if err != nil || !existed || ip != ipPrime { t.Fatalf(`ensureStaticIP(%v, %v, %v, %v, %v) = %v, %v, %v; want %v, true, nil`, fcas, ipName, serviceName, region, ip, ipPrime, existed, err, ip) } } +func TestEnsureStaticIPWithTier(t *testing.T) { + s := NewFakeCloudAddressService() + serviceName := "" + region := "us-east1" + + for desc, tc := range map[string]struct { + name string + netTier NetworkTier + expected string + }{ + "Premium (default)": { + name: "foo-1", + netTier: NetworkTierPremium, + expected: "PREMIUM", + }, + "Standard": { + name: "foo-2", + netTier: NetworkTierStandard, + expected: "STANDARD", + }, + } { + t.Run(desc, func(t *testing.T) { + ip, existed, err := ensureStaticIP(s, tc.name, serviceName, region, "", tc.netTier) + assert.NoError(t, err) + assert.False(t, existed) + assert.NotEqual(t, "", ip) + // Get the Address from the fake address service and verify that the tier + // is set correctly. + alphaAddr, err := s.GetAlphaRegionAddress(tc.name, region) + require.NoError(t, err) + assert.Equal(t, tc.expected, alphaAddr.NetworkTier) + }) + } +} + func TestVerifyRequestedIP(t *testing.T) { region := "test-region" lbRef := "test-lb" @@ -53,35 +90,150 @@ func TestVerifyRequestedIP(t *testing.T) { for desc, tc := range map[string]struct { requestedIP string fwdRuleIP string + netTier NetworkTier addrList []*computealpha.Address expectErr bool expectUserOwned bool }{ "requested IP exists": { requestedIP: "1.1.1.1", - addrList: []*computealpha.Address{{Name: "foo", Address: "1.1.1.1"}}, + netTier: NetworkTierPremium, + addrList: []*computealpha.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, expectErr: false, expectUserOwned: true, }, "requested IP is not static, but is in use by the fwd rule": { requestedIP: "1.1.1.1", fwdRuleIP: "1.1.1.1", + netTier: NetworkTierPremium, expectErr: false, }, "requested IP is not static and is not used by the fwd rule": { requestedIP: "1.1.1.1", fwdRuleIP: "2.2.2.2", + netTier: NetworkTierPremium, expectErr: true, }, "no requested IP": { + netTier: NetworkTierPremium, expectErr: false, }, + "requested IP exists, but network tier does not match": { + requestedIP: "1.1.1.1", + netTier: NetworkTierStandard, + addrList: []*computealpha.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, + expectErr: true, + }, } { t.Run(desc, func(t *testing.T) { s.SetRegionalAddresses(region, tc.addrList) - isUserOwnedIP, err := verifyUserRequestedIP(s, region, tc.requestedIP, tc.fwdRuleIP, lbRef) + isUserOwnedIP, err := verifyUserRequestedIP(s, region, tc.requestedIP, tc.fwdRuleIP, lbRef, tc.netTier) assert.Equal(t, tc.expectErr, err != nil, fmt.Sprintf("err: %v", err)) - assert.Equal(t, tc.expectUserOwned, isUserOwnedIP, desc) + assert.Equal(t, tc.expectUserOwned, isUserOwnedIP) + }) + } +} + +func TestCreateForwardingRuleWithTier(t *testing.T) { + s := NewFakeCloudForwardingRuleService() + // Common variables among the tests. + ports := []v1.ServicePort{{Name: "foo", Protocol: v1.ProtocolTCP, Port: int32(123)}} + region := "test-region" + target := "test-target-pool" + svcName := "foo-svc" + + for desc, tc := range map[string]struct { + netTier NetworkTier + expectedRule *computealpha.ForwardingRule + }{ + "Premium tier": { + netTier: NetworkTierPremium, + expectedRule: &computealpha.ForwardingRule{ + Name: "lb-1", + Description: `{"kubernetes.io/service-name":"foo-svc"}`, + IPAddress: "1.1.1.1", + IPProtocol: "TCP", + PortRange: "123-123", + Target: target, + NetworkTier: "PREMIUM", + }, + }, + "Standard tier": { + netTier: NetworkTierStandard, + expectedRule: &computealpha.ForwardingRule{ + Name: "lb-2", + Description: `{"kubernetes.io/service-name":"foo-svc"}`, + IPAddress: "2.2.2.2", + IPProtocol: "TCP", + PortRange: "123-123", + Target: target, + NetworkTier: "STANDARD", + }, + }, + } { + t.Run(desc, func(t *testing.T) { + lbName := tc.expectedRule.Name + ipAddr := tc.expectedRule.IPAddress + + err := createForwardingRule(s, lbName, svcName, region, ipAddr, target, ports, tc.netTier) + assert.NoError(t, err) + + alphaRule, err := s.GetAlphaRegionForwardingRule(lbName, region) + assert.NoError(t, err) + assert.Equal(t, tc.expectedRule, alphaRule) + }) + } +} + +func TestDeleteAddressWithWrongTier(t *testing.T) { + region := "test-region" + lbRef := "test-lb" + s := NewFakeCloudAddressService() + + for desc, tc := range map[string]struct { + addrName string + netTier NetworkTier + addrList []*computealpha.Address + expectDelete bool + }{ + "Network tiers (premium) match; do nothing": { + addrName: "foo1", + netTier: NetworkTierPremium, + addrList: []*computealpha.Address{{Name: "foo1", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, + }, + "Network tiers (standard) match; do nothing": { + addrName: "foo2", + netTier: NetworkTierStandard, + addrList: []*computealpha.Address{{Name: "foo2", Address: "1.1.1.2", NetworkTier: "STANDARD"}}, + }, + "Wrong network tier (standard); delete address": { + addrName: "foo3", + netTier: NetworkTierPremium, + addrList: []*computealpha.Address{{Name: "foo3", Address: "1.1.1.3", NetworkTier: "STANDARD"}}, + expectDelete: true, + }, + "Wrong network tier (preimium); delete address": { + addrName: "foo4", + netTier: NetworkTierStandard, + addrList: []*computealpha.Address{{Name: "foo4", Address: "1.1.1.4", NetworkTier: "PREMIUM"}}, + expectDelete: true, + }, + } { + t.Run(desc, func(t *testing.T) { + s.SetRegionalAddresses(region, tc.addrList) + // Sanity check to ensure we inject the right address. + _, err := s.GetRegionAddress(tc.addrName, region) + require.NoError(t, err) + + err = deleteAddressWithWrongTier(s, region, tc.addrName, lbRef, tc.netTier) + assert.NoError(t, err) + // Check whether the address still exists. + _, err = s.GetRegionAddress(tc.addrName, region) + if tc.expectDelete { + assert.True(t, isNotFound(err)) + } else { + assert.NoError(t, err) + } }) } } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_naming.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_naming.go index 2542dbe1c4c..73169b21dc5 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_naming.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_naming.go @@ -84,6 +84,11 @@ func makeBackendServiceDescription(nm types.NamespacedName, shared bool) string // External Load Balancer +// makeServiceDescription is used to generate descriptions for forwarding rules and addresses. +func makeServiceDescription(serviceName string) string { + return fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, serviceName) +} + // makeNodesHealthCheckName returns name of the health check resource used by // the GCE load balancers (l4) for performing health checks on nodes. func makeNodesHealthCheckName(clusterID string) string { diff --git a/pkg/cloudprovider/providers/gce/gce_util.go b/pkg/cloudprovider/providers/gce/gce_util.go index 4f770ab8c46..f21d5113375 100644 --- a/pkg/cloudprovider/providers/gce/gce_util.go +++ b/pkg/cloudprovider/providers/gce/gce_util.go @@ -157,3 +157,19 @@ func makeGoogleAPINotFoundError(message string) error { func makeGoogleAPIError(code int, message string) error { return &googleapi.Error{Code: code, Message: message} } + +func isForbidden(err error) bool { + return isHTTPErrorCode(err, http.StatusForbidden) +} + +// TODO(#51665): Remove this once Network Tiers becomes Beta in GCP. +func handleAlphaNetworkTierGetError(err error) (string, error) { + if isForbidden(err) { + // Network tier is still an Alpha feature in GCP, and not every project + // is whitelisted to access the API. If we cannot access the API, just + // assume the tier is premium. + return NetworkTierDefault.ToGCEValue(), nil + } + // Can't get the network tier, just return an error. + return "", err +}